Repository navigation
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
📦 Client bundle boundary
A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in |
|
@codex review |
Automated review: 80/100Good work overall. Strengths
Concerns and suggestions (none blocking)
CI on this head was still running when this review was written. Generated by Claude Code |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Studio, the gateway snippets and the docs tell users to export VERYFRONT_API_KEY, but the CLI and SDK only read VERYFRONT_API_TOKEN, so 'veryfront whoami' said 'Not logged in' with the key the user just set. The CLI and SDK now read VERYFRONT_API_KEY too: - After each env load, a key is copied into the VERYFRONT_API_TOKEN slot once, only when that slot is unset or blank, and with the key's provenance, so every repository trust rule judges a .env key exactly like a .env token (including $NAME expansion taint). - Processes that never load env files (SDK scripts) read the key through getHostEnv. Once the loader has run, getHostEnv never falls back, so a token the CLI removed on purpose cannot come back through the key. - VERYFRONT_API_TOKEN keeps working and wins when both are set. Help, errors, docs and SDK examples now name VERYFRONT_API_KEY; messages about an active credential name the variable the user actually set. The MCP deploy tool classifies the missing-credential error by an exported constant instead of copying its wording. Refs veryfront/veryfront-issue-inbox#4088 Refs veryfront/veryfront-issue-inbox#4220
… hints Codex review on 66ad121: - P1: every test environment scrubber removed VERYFRONT_API_TOKEN but not VERYFRONT_API_KEY, so an exported key became an active credential in test children. Add the key to the Deno suite, Node/Bun runtime, preload, runtime E2E and binary-server scrubbers. - P2: login messages told you to unset VERYFRONT_API_TOKEN even when the key supplied the credential. Name the variable you set in the rejected, unavailable, existing-session and invalid-warning messages, and fix the runs target README sample.
A token file supplies the credential ahead of VERYFRONT_API_TOKEN. Copying a project .env key into the token slot attached the key's env-file provenance to the token file's secret, so a repository-steered API URL could receive it. Skip the alias while VERYFRONT_API_TOKEN_FILE is set.
…ater load veryfront dev --project loads the cwd .env, then the project's. A key aliased from the first load kept its env-file provenance after the second load set VERYFRONT_API_TOKEN_FILE, so the token file's secret could be treated as repository-supplied. Revoke the alias and its provenance whenever a token file is set, instead of only skipping a new alias.
…in every hint Codex review on 8d2ce9c and 59743fb: - getHostEnv no longer falls back to VERYFRONT_API_KEY while a nonblank VERYFRONT_API_TOKEN_FILE is set, so an SDK process (for example Sandbox.create) uses the renewable token file instead of a stale key. - The remaining missing-credential hints now say VERYFRONT_API_KEY: up, remote file tools, runs client, Veryfront Cloud and local providers, the AG-UI no-credentials error and the authentication-required suggestion. 'veryfront config' reports apiToken (VERYFRONT_API_KEY) when the key supplied the token. The docs-agent template exports the key. - Regenerate docs/guides/errors.md, the RSC bundle and the templates manifest.
withoutHostBinaryInfraEnv removed VERYFRONT_API_TOKEN but not the key. The VFS proxy-mode E2E reloads .env after the suite scrubber, so a developer key became the token in compiled children pointed at the real API. Add the key to the blocklist that the VFS and compiled-binary E2E suites share.
An additional env load keeps values defined before it, and it counted the token copied from VERYFRONT_API_KEY as one. With veryfront dev --project and an exported key, the project's explicit VERYFRONT_API_TOKEN was skipped, which broke token-over-key precedence and rejected same-file self-hosted pairs. The precedence check now treats the alias as unset, so only values the user set keep precedence.
RESERVED_TASK_ENV_KEYS stripped VERYFRONT_API_TOKEN but not the key that can now supply it, so a task started with VERYFRONT_API_KEY copied the developer credential into ctx.env, and verified skill scripts, which reuse the same set, could receive it. Reserve the key alongside the token.
In a process that never loads env files, only getHostEnv saw VERYFRONT_API_KEY. The environment snapshot carried no token, so integration discovery and dependency write-back took the host-private branch, which resolves VERYFRONT_API_URL first instead of VERYFRONT_API_BASE_URL. With separate GraphQL and REST hosts, a key-only script reached a different backend than a token-only one. The snapshot now reads the key through readUnloadedVeryfrontApiKey, the rule getHostEnv already used: only before the env loader resolves the key, and never while a token file is set.
…blank token as unset Codex review on f391d47: - P1: the snapshot read the key fallback through the project-scoped reader, so inside a project env scope a project's VERYFRONT_API_KEY became the snapshot token and could authenticate a dependency write-back. The fallback now reads the host environment and never applies inside a project scope. - P2: a whitespace-only VERYFRONT_API_TOKEN shadowed a valid key. A blank token now gives way to the key, as getHostEnv and the env loader already treat it. A blank token with no key is returned as before.
The bundled integrations reference still named VERYFRONT_API_TOKEN as the platform credential for a connect. Regenerated core-skills.generated.ts.
The environment-config test wrote a temporary token file and the sanitize-process-env test mutated Deno.env, so both needed a new semantic disposition. The token-file rule is already covered in env.test.ts, and the sanitize test now checks the exported key list.
c1bafe9 to
742d577
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
On Windows the env loader case-folds names, so a project .env entry such as veryfront_api_key supplies the credential under a lower-case name and passed the exact-match reserved check into task and skill script env. The check now compares the upper-cased name. Also keep the new tests on the shared test primitives: withMockFetch and makeTempDir instead of a raw globalThis.fetch assignment and Deno.makeTempDir, and move the sensitive key list into a module without side effects so its test runs under Bun and Node.
withoutHostBinaryInfraEnv deleted blocked names by exact case. On Windows a project .env can supply veryfront_api_key, which a compiled child process reads as the credential. Match by folded name, as buildTestProcessEnv and buildRuntimeTestProcessEnv already do.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
…t service takes VERYFRONT_API_KEY Any defined token file selector, a blank one included, is the credential: the key never stands in for it, in the env loader's alias or the unloaded read. The standalone agent service config takes VERYFRONT_API_KEY when VERYFRONT_API_TOKEN is unset (token first), so registration sees a token.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e23e38700
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| VERYFRONT_API_TOKEN: env.VERYFRONT_API_TOKEN ?? | ||
| (env.VERYFRONT_API_TOKEN_FILE === undefined ? env.VERYFRONT_API_KEY : undefined), |
There was a problem hiding this comment.
When an agent-service deployment defines VERYFRONT_API_TOKEN as an empty string while providing a valid VERYFRONT_API_KEY, parsing fails at the token's min(1) validation before this transform runs; a whitespace-only token reaches the transform and wins because ?? treats it as present. This differs from the env loader and environment snapshot, which treat blank legacy tokens as unset, so common manifests that retain an empty deprecated variable cannot adopt the newly supported key. Normalize blank token values to undefined before validation/fallback.
Useful? React with 👍 / 👎.
| VERYFRONT_PUBLIC_API_BASE_URL: v.string().url().optional(), | ||
| VERYFRONT_API_TOKEN: v.string().min(1).optional(), | ||
| // The name Veryfront tells users to set; the token wins when both are set (see `env-loader`). | ||
| VERYFRONT_API_KEY: v.string().min(1).optional(), |
There was a problem hiding this comment.
The new agent-service key support leaves the main agent-service documentation factually stale: docs/guides/agent-service-runtime.md:66,149-155 still says registration requires VERYFRONT_API_TOKEN and uses it in the example, while docs/guides/configuration.md:272 and the skill execution description at docs/guides/agents.md:515 likewise omit VERYFRONT_API_KEY. Users following these surfaces are still directed to the deprecated name even though this change establishes the key as the user-facing credential, so update these remaining guides and examples together with the behavior change.
AGENTS.md reference: AGENTS.md:L13-L14
Useful? React with 👍 / 👎.
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Runs same-repo CI on the exact head of #5514 (87b04ec), since CI skips fork PRs. Do not merge. Closed with its branch once #5514 merges.