Skip to content

fix(agents): honor signed execution environment bindings - #4564

Merged
kwakayama merged 2 commits into
mainfrom
fix/studio-preview-agent-env
Sep 22, 2026
Merged

kwakayama merged 2 commits into
mainfrom
fix/studio-preview-agent-env

Conversation

@kwakayama

@kwakayama kwakayama commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Studio branch-agent runs receive an empty environment even when their project's runtime environment has configured server-side credentials. This change lets the control plane bind an execution environment explicitly in the signed invocation.

Add optional run.project.executionEnvironmentId to the internal invocation contract. For branch sources, load only that environment through the existing project-scoped authorization and credential-scoped cache. Without a binding, a branch still receives no project secrets. The runtime does not discover an environment or choose a preview fallback. Named release-bound environments and bare releases keep their existing behavior.

No provider-specific logic or credential values are added to the protocol, browser, or model messages. Tests use a synthetic generic service key. Paired API PR: https://github.com/veryfront/veryfront-api/pull/5066. It resolves the managed environment from project metadata without loading secret values; the framework change must be deployed first.

Validation: 285 focused runtime/contract/environment checks pass, including an explicitly bound tool environment, unbound branches, and authorization denial. Full framework typecheck, targeted lint, formatting and diff checks pass. Production Studio verification is pending the paired release.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for security reviews. Please try again later.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review 🔄 Running since 2026-09-22T20:30:12.998497Z 5b76ed8 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your trial has ended. Reactivate Greptile to resume code reviews.

@coderabbitai

coderabbitai Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 4d3609a5-ae2a-4250-ae4f-d0e022eed7b0

📥 Commits

Reviewing files that changed from the base of the PR and between fd68f27 and 6706d07.

📒 Files selected for processing (2)
  • src/server/handlers/request/agent-stream.handler.test.ts
  • src/server/handlers/request/agent-stream.handler.ts

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

Branch agent requests now resolve the authorized project preview environment and inject its variables. The handler validates the signed target environment ID. Tests cover preview-only loading and authorization failure before project discovery.

Changes

Branch preview environment resolution

Layer / File(s) Summary
Preview environment resolution and validation
src/server/handlers/request/agent-stream.handler.ts
Branch sources resolve the signed preview environment and load its cached variables. Release sources still return no project variables, and named environment sources retain their existing resolution path.
Preview variable and authorization tests
src/server/handlers/request/agent-stream.handler.test.ts
Tests verify preview variable injection, exclusion of production variables, and a 403 response before project discovery when preview variable access is denied.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant AgentStreamHandler
  participant EnvironmentIdentityResolver
  participant AgentEnvVarCache
  participant EnvironmentAPI
  AgentStreamHandler->>EnvironmentIdentityResolver: Resolve signed preview environment
  EnvironmentIdentityResolver-->>AgentStreamHandler: Authorized preview environment
  AgentStreamHandler->>AgentEnvVarCache: Load preview variables
  AgentEnvVarCache->>EnvironmentAPI: Request environment variables
  EnvironmentAPI-->>AgentEnvVarCache: Variables or 403
  AgentEnvVarCache-->>AgentStreamHandler: Runtime variables or authorization error
Loading

Merge Risk: ⚪ Minimal · up to 6706d

No verified merge-blocking issue remains in the reviewed change.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: agent handling now honors signed execution environment bindings.
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gitar-bot

gitar-bot Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

Gitar is working

Gitar

@kwakayama
kwakayama force-pushed the fix/studio-preview-agent-env branch from f2256dc to 6706d07 Compare September 22, 2026 20:11
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for security reviews. Please try again later.

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your trial has ended. Reactivate Greptile to resume code reviews.

@github-actions

Copy link
Copy Markdown

📦 Client bundle boundary

Entrypoint Modules Source size Server leaks
src/index.client.ts 291 2332 KiB ✅ 0

A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in scripts/lint/client-bundle-baseline.json to burn down.

Copy link
Copy Markdown
Contributor Author

Review score: 15/100 — do not merge as-is (branch is not rebased; carries 16 unrelated/superseded commits)

The one commit that matches this PR's description is solid, but the PR as opened is not mergeable and is not reviewable in its current form.

Blocking issue — stale branch carrying duplicate/superseded history:

  • This branch forked from main at 78ec40a (Fix agent service heartbeat JSON body #4374) and was never rebased. It is 17 commits ahead of current main, but 16 of those 17 commits are not this PR's work — they're a separate, unsynced re-implementation of the "bound the content-keyed JSX transform cache" hardening that was already reviewed and squash-merged via fix(security): bound the content-keyed JSX transform cache #4400 (Sep 7) and further refined via fix(transforms): fence and drain background JSX cache prune passes #4511 (Sep 17).
  • Net effect: this PR's diff shows 3,461 additions / 116 deletions across 13 files, when the actual intended change is 2 files and ~150 lines. GitHub already reports mergeable_state: dirty (merge conflict against main), which confirms this.
  • Merging as-is risks reintroducing an outdated, divergent version of already-shipped security-sensitive cache code and conflicts with what's already on main.
  • Action needed: rebase fix/studio-preview-agent-env onto current main (or cherry-pick just f2256dc onto a fresh branch off main) so the diff contains only the intended change, then re-push.

Review of the actual intended change (commit f2256dc, src/server/handlers/request/agent-stream.handler.ts + its test):

  • Correctly resolves the project's "preview" environment for branch-sourced agent runs, reusing the existing named-environment resolver (resolveNamed) and the shared _agentEnvVarCache, consistent with how the "environment"-typed source path already works.
  • Authorization is sound: the environment lookup is scoped by the request's project-authenticated API token (/projects/{slug}/environments), and the optional expectedEnvironmentId check preserves the existing signed-target/TOCTOU protection when the control-plane payload pins one, without requiring it when it isn't present (branch runs don't always carry a pinned environment id).
  • Test coverage is good: replaces the old "branch never fetches" test with (1) a positive-path test proving preview vars are injected and production secrets are not, and (2) a denial-path test proving a 403 from the environment-authorization check short-circuits before project discovery runs. Both are meaningful regression tests, not just coverage padding.
  • Doc comment on resolveAgentSourceEnvironment was updated to match new behavior — good.
  • Minor/non-blocking: it'd be worth a one-line comment on why expectedEnvironmentId is optional for the branch path (vs. mandatory for the named/environment path) so a future reader doesn't mistake it for an oversight.

Once rebased so the diff is just the environment-resolution fix, this looks close to mergeable — the logic and tests are in good shape. Please fix the branch base first.


Generated by Claude Code

@github-actions

Copy link
Copy Markdown

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Already looking forward to the next diff.

Reviewed commit: 6706d07efb

ℹ️ 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".

@codecov

codecov Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.00000% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...rc/server/handlers/request/agent-stream.handler.ts 93.33% 0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@kwakayama
kwakayama marked this pull request as draft September 22, 2026 20:28
@kwakayama

Copy link
Copy Markdown
Contributor Author

Review score: 96/100

Status: ready to merge after the required merge-queue checks.

Findings:

  • Branch sources now resolve only the authorized project's preview environment and keep bare releases environment-free.
  • Signed preview environment IDs are checked when present; named release sources retain exact environment and active-release validation.
  • Environment-variable lookup remains credential-scoped and cached, while production-only variables are not exposed.
  • Regression coverage verifies preview injection, production-secret isolation, and denial before discovery/execution.

Validation:

  • Exact head: 6706d07efb01a07e3cfee65e9531f567e601231c
  • Focused agent-stream handler suite: passed
  • Project-environment resolver suite: 1 passed / 24 steps
  • CI/CD, CodeQL, SonarQube, coverage, integration, lint, typecheck, format, and E2E checks: green
  • Review-thread audit: no unresolved non-outdated threads

No blocking findings.

@kwakayama
kwakayama marked this pull request as ready for review September 22, 2026 20:30
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for security reviews. Please try again later.

1 similar comment
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for security reviews. Please try again later.

@kwakayama kwakayama changed the title fix(agents): load preview environment for Studio branch runs fix(agents): honor signed execution environment bindings Sep 22, 2026

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your trial has ended. Reactivate Greptile to resume code reviews.

@github-actions

Copy link
Copy Markdown

@codex review

@github-actions

Copy link
Copy Markdown

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Nice work!

Reviewed commit: 5b76ed8e8c

ℹ️ 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".

@sonarqubecloud

Copy link
Copy Markdown

@kwakayama
kwakayama added this pull request to the merge queue Sep 22, 2026
@kwakayama

Copy link
Copy Markdown
Contributor Author

Updated review score: 98/100

Status: ready to merge after the merge-group checks for the current head.

Update reviewed at exact head 5b76ed8e8c581d2d6f3a12ff386ec2c69fab80fb:

  • Branch secret injection now requires the signed executionEnvironmentId; unbound branches remain environment-free.
  • The execution environment is propagated through the public/runtime and internal control-plane contracts and used consistently for env-var loading and request context.
  • Named environment sources retain exact environment plus active-release validation, and bare releases remain environment-free.
  • Added coverage for explicit binding, absent binding, denied access, secret isolation, and contract propagation.

Validation:

  • Agent stream handler suite: 3 passed / 82 steps
  • Runtime invocation contract suite: 1 passed / 23 steps
  • Prior full CI matrix and protected automated review were green; the one Node 24 smoke timeout was an external esm.sh fetch timeout and passed on retry.
  • Review-thread audit: no unresolved non-outdated threads.

No blocking findings.

Merged via the queue into main with commit 60d9761 Sep 22, 2026
111 of 113 checks passed
@kwakayama
kwakayama deleted the fix/studio-preview-agent-env branch September 22, 2026 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant