refactor(core): share agent child environment constructor - #880
Conversation
Codex Review SummaryThis 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. |
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Smoke-tested at head Bar: Exercised for real, not asserted. I built both runtimes' child environments from the compiled Result — the child sees Two real fixes the PR body undersells, both visible only in the differential:
Two judgment calls for you, neither blocking:
|
hojinzs
left a comment
hojinzs
left a comment
There was a problem hiding this comment.
Reviewed at head 55d4df0f. Round 1 of at most 6.
The refactor itself is the right shape and stays inside #870's scope — no over-engineering, no drive-by decisions. One thing must change before merge.
Verdict: request changes
P1 — the reordering silently defeats the declared-secret strip. Both runtimes now apply the shared assignments after their secret-removal step, and the constructor re-reads the same sources, so any of the nine AGENT_VISIBLE_SYMPHONY_CONTEXT_ENVIRONMENT_NAMES that a tracker adapter or an MCP config designates as a secret is deleted and then put back. Inline on both call sites with the main differential. No live secret escape today — the nine are non-secret by construction and the clone URL is sanitized — but secretEnvironmentNames also carries operator- and (under trustRepoConfig) repo-supplied MCP source names, so an arbitrary name can reach that set, and the invariant main enforced no longer holds. Fix is a few lines; suggested shape is an excludeNames option on the shared constructor so both runtimes stay identical.
Also P2 (changeset) and P3 (a third construction site) below, plus two inline P3/nits.
Requirements vs #870
| Acceptance criterion | Status |
|---|---|
| Both runtimes build explicit child assignments through the shared helper | ✅ |
| Per-runtime construction deleted | ✅ |
| Adding a host-constructed value requires one edit | |
| A test proves both runtimes pick it up | ✅ structurally (both call the helper); assertions are hand-written per runtime — nit inline |
Out-of-scope items on the issue (allowlist consolidation, declarative non-secret channel, MCP composition removal) are all correctly left alone.
Smoke test
Clean checkout of 55d4df0f, pnpm install --frozen-lockfile → pnpm build (success) → core 354 / runtime-codex 81 / runtime-claude 110 tests, all passing. No preview environment for this change, so I exercised the two child-environment builders directly and ran the identical probes against the main build (bca2fcb) for a differential — hostile host env (HOME/USERPROFILE=/Users/operator, stale SYMPHONY_ISSUE_ID=STALE-999, GIT_ASKPASS, GIT_CONFIG_GLOBAL, AGENT_CREDENTIAL_BROKER_SECRET, GITHUB_TOKEN), run-scoped values through extraEnv, Claude driven through spawnTurn with an injected spawnImpl.
main |
55d4df0f |
|
|---|---|---|
Codex USERPROFILE |
absent | child home ✅ |
Claude USERPROFILE (non-inherit) |
/Users/operator |
child home ✅ |
Codex clone URL from extraEnv |
https://x-access-token:ghs_SECRET@… |
sanitized ✅ |
Run-scoped SYMPHONY_ISSUE_ID over stale host value |
wins | wins ✅ |
| Credential vars in child | absent | absent ✅ |
| Declared-secret name ∩ agent-visible name | stripped | reintroduced ❌ |
HOME / GH_CONFIG_DIR / DOCKER_CONFIG / CODEX_HOME / GIT_TERMINAL_PROMPT=0 all correct on both runtimes.
Answers to the two judgment calls you raised
Precedence. Your overlap analysis is right and I re-derived it: orchestratorRunEnv sets only SYMPHONY_ORCHESTRATOR_URL / SYMPHONY_RUN_ID / SYMPHONY_ORCHESTRATOR_TOKEN, none of the nine, and none of the nine are in AGENT_CHILD_CREDENTIAL_ENVIRONMENT_NAMES. What that analysis misses is the other strip — the secretEnvironmentNames delete loop in Codex and stripTrackerSecrets in Claude — which is where the P1 lives. Keep the new ordering; make it subtract.
Changeset — yes, add one (P2). The PR body reasons from the absence of a changeset:* label on #870, but CLAUDE.md keys the rule on behavior: "Behavior-changing PRs must add a changeset." This changes what the agent child actually receives on both runtimes (Codex gains USERPROFILE; Claude stops handing the operator's host USERPROFILE to an otherwise-isolated child). A patch changeset for @gh-symphony/cli describing the USERPROFILE pinning is the honest record.
One correction to the premise, since it affects how you size that changeset: claim #1 is real at the function boundary but not reachable from any production caller. runtime-codex/src/launcher.ts:44 already builds extraEnv through readAgentVisibleSymphonyContext(env), so the clone URL arrives pre-sanitized; and orchestrator/src/runtime-factory.ts:171 passes context.env, whose TARGET_REPOSITORY_CLONE_URL is deriveCloneUrl(repository.url) — a GitHub API URL with no userinfo. I reproduced your result only by injecting a tokenized URL into extraEnv directly, which nothing in the orchestrator does. So it's defense-in-depth hardening, not a leak that was occurring. Claim #2 is the one that was actually live, and it's slightly bigger than you described — see the Claude inline.
P3 — a third construction site still exists
packages/core/src/runtime/custom-child-env.ts:151-153 still hand-assigns HOME / USERPROFILE / GH_CONFIG_DIR, and omits DOCKER_CONFIG — so the custom-runtime child already diverges from the two agent runtimes on a host-constructed value, and "one edit" isn't true while it stands. #870's scope names only the two runtimes, so I'm not asking you to widen the PR. Either migrate it in a follow-up, or, if the DOCKER_CONFIG omission is deliberate, say so in the §17.5 row you touched in docs/architecture.md so the next person doesn't read the shared constructor as universal.
The docs/architecture.md update is otherwise accurate.
Fix the P1 and add the changeset and I'll approve.
Generated by Claude Code
hojinzs
left a comment
hojinzs
left a comment
There was a problem hiding this comment.
Round 2 — one P2 left, everything from round 1 verified fixed
Round 1 follow-up: all three confirmed closed, not just claimed
I re-ran each finding as a differential rather than reading the diff:
| Round 1 | Status at efa724b |
|---|---|
| P1 Codex — strip reintroduced by last-applied assignments | Fixed. TARGET_REPOSITORY_URL declared secret + agent-visible → absent from plan.env. |
| P1 Claude — same defect, both paths | Fixed. Absent on inherit=true and inherit=false. |
P3 — dead ?? [] on optional sources |
Fixed. Required, fallback gone. |
| nit — hand-asserted parity | Declined with reasoning on the thread. Accepted — see below. |
On the declined nit: the reasoning holds, and I checked the criterion directly rather than taking it on structure. A parity assertion projecting the full shared surface out of both runtimes for identical inputs passes on efa724b and fails on main — so "adding a value once appears in both" is now genuinely true. I'd still rather see that assertion in the tree than in my scratch dir, but it is not a blocker.
Requirements vs. issue #870
- Shared constructor in
core, both runtimes migrated, per-runtime construction deleted — done. - One-edit property — holds; verified by the parity differential above.
- Out-of-scope items (inheritance allowlists, declarative non-secret channel, MCP composition) all correctly left alone. No overengineering: the constructor is 20 lines and takes exactly the two inputs the call sites need.
USERPROFILE is a scope addition beyond the issue text, but it's a real isolation hole (DEFAULT_INHERITED_ENV_KEYS was copying the host value into the Claude non-inherit path while HOME was pinned), it's the natural blast radius of this refactor, and the changeset calls it out. Fine to carry here.
Smoke test
No preview environment on this PR, so black-box locally at the spawn-plan boundary — buildCodexRuntimePlan directly, and buildClaudeSpawnEnv driven through spawnTurn with an injected spawnImpl, on efa724b vs main:
- cross-runtime parity of the shared assignment surface — passes on head, fails on main ✅
- declared secret ∩ agent-visible absent — Codex ✅, Claude inherit ✅, Claude non-inherit ✅
- clone-URL userinfo sanitized when supplied through
extraEnv— ✅ (an incidental improvement: onmainthe rawextraEnvspread landed after the context read on Codex, so userinfo survived; now it doesn't) - pinned-assignment suppression — ❌ see the P2
Repo suites: packages/core/src/runtime, packages/runtime-codex, packages/runtime-claude → 227 passed. CI on efa724b: Test ✅, Container Smoke ✅.
Blocking
P2 — packages/core/src/runtime/agent-child-env.ts. excludeNames subtracts from the pinned isolation assignments as well as the agent-visible context, so an operator/repo-reachable name colliding with HOME now suppresses child-home isolation on Claude where main pinned it. Differential and a three-line fix on the inline comment. Same reachability argument that carried the round-1 P1, so I'm holding it to the same bar.
One nit on CODEX_HOME asymmetry, take it or leave it.
Fix the P2 and I'll approve — I don't have further concerns beyond it.
Generated by Claude Code
hojinzs
left a comment
hojinzs
left a comment
There was a problem hiding this comment.
Round 3 — approved. The round-2 P2 is genuinely closed, and the main differential is clean.
Reviewed at head 3469d2f (the merge of current main). Round 3 of at most 6.
Round 2 follow-up — verified by differential, not by reading the diff
I re-ran the exact probe that produced the round-2 P2, driving buildCodexRuntimePlan directly and buildClaudeSpawnEnv through spawnTurn with an injected spawnImpl, with HOME, USERPROFILE, GH_CONFIG_DIR, DOCKER_CONFIG, CODEX_HOME and TARGET_REPOSITORY_URL all declared secret at once — i.e. a collision on every pinned name simultaneously:
| Pinned name | Codex | Claude inherit=false |
Claude inherit=true |
|---|---|---|---|
HOME |
child home ✅ | child home ✅ | child home ✅ |
USERPROFILE |
child home ✅ | child home ✅ | child home ✅ |
GH_CONFIG_DIR |
…/gh ✅ |
…/gh ✅ |
…/gh ✅ |
DOCKER_CONFIG |
…/.docker ✅ |
…/.docker ✅ |
…/.docker ✅ |
CODEX_HOME |
…/.codex ✅ |
n/a | n/a |
The pinned surface is byte-identical between the clean run and the all-names-collide run on all three paths. Splitting the subtraction so excludeNames reaches only the source-derived context is the right cut, and the CODEX_HOME nit being taken means the Codex boundary is uniform rather than one-name-special.
Critically, the round-1 P1 did not regress while fixing this: TARGET_REPOSITORY_URL, agent-visible and declared secret, is still absent from all three child environments. The two invariants — "a declared secret cannot cross" and "the strip cannot switch off isolation" — now hold at the same time, which is the whole point.
Round-1 P3 (custom-child-env.ts as a third construction site) is discharged the documented way: the §17.5 row now states the custom constructor is separate and does not share the agent-runtime assignment set. Good enough — the next reader won't mistake the shared constructor for universal.
main differential — 12 changes, all in the safe direction
Same probe against bcf80f6. Every single difference is head being stricter; there is no cell where main is safer than head:
main |
3469d2f |
|
|---|---|---|
Codex USERPROFILE |
absent | child home |
Claude USERPROFILE (both paths) |
/Users/operator |
child home |
Codex TARGET_REPOSITORY_CLONE_URL |
https://x-access-token:ghs_SECRET@… |
sanitized |
| Codex pinned names under secret collision | all dropped (HOME, GH_CONFIG_DIR, DOCKER_CONFIG, CODEX_HOME = undefined) |
all pinned |
Claude USERPROFILE under secret collision |
dropped | pinned |
Everything else — credential vars absent, GIT_TERMINAL_PROMPT=0, run-scoped SYMPHONY_ISSUE_ID winning over a stale host value — is unchanged from main.
Two notes on how to read that table, since they affect how the release is described rather than whether it ships:
- The clone-URL sanitization is hardening, not a leak that was occurring — I re-confirmed the round-1 reachability finding at this head.
runtime-codex/src/launcher.ts:44already buildsextraEnvthroughreadAgentVisibleSymphonyContext, andorchestrator/src/runtime-factory.tspassescontext.envwhose clone URL isderiveCloneUrl(repository.url)with no userinfo.buildCodexRuntimePlanhas no other production caller. I reproduce the tokenized value only by injecting it intoextraEnvdirectly, which nothing does. - The row that was live is the Claude
USERPROFILEone, and the Codex collision row is a real robustness gain on a pathmainsilently got wrong.
The changeset covers the USERPROFILE pinning and the secret-exclusion preservation. It doesn't mention the clone-URL hardening — given it isn't reachable, omitting it is defensible, so I'm not asking for a reword.
Requirements vs. issue #870
| Acceptance criterion | Status |
|---|---|
| Both runtimes build explicit child assignments through the shared helper | ✅ |
| Per-runtime construction deleted | ✅ |
| Adding a host-constructed value requires one edit | ✅ |
| A test proves both runtimes pick it up | ✅ |
No over-engineering: the constructor is 22 lines with exactly the three inputs the call sites need. All three out-of-scope items on the issue (inheritance-allowlist consolidation, a declarative non-secret channel, MCP composition removal) are correctly left alone. The USERPROFILE scope addition remains the right call — it's the natural blast radius of the refactor and the changeset records it.
Validation
pnpm install --frozen-lockfile → pnpm build → pnpm lint (0) → pnpm typecheck (0) → pnpm test (full monorepo, exit 0; orchestrator 362, cli 567). Targeted suites packages/core/src/runtime + runtime-codex + runtime-claude: 227 passed. CI on 3469d2f: Test ✅, Container Smoke ✅. No preview environment applies to an in-process spawn-plan boundary, so the smoke test is the black-box probe described above rather than a deployed surface.
The regression tests in the tree pin what matters: the core test asserts the exact assignment set with a pinned-name collision, and both runtime tests feed an overlapping agent-visible/secret name and assert absence while the pinned values survive.
One nit inline about two now-dead entries in DEFAULT_INHERITED_ENV_KEYS — I verified removing them leaves the full child environment identical across all six configurations, so it's cosmetic. Take it or leave it; it is not worth another round.
Nothing blocking remains. Approving.
Generated by Claude Code
| buildAgentChildEnvironmentAssignments({ | ||
| childHome: options.childHome, | ||
| sources: [process.env, options.configEnv, options.inputEnv], | ||
| excludeNames: removedEnvironmentNames, | ||
| }) | ||
| ); |
There was a problem hiding this comment.
nit (non-blocking, do it in the follow-up if you prefer) — HOME and USERPROFILE in DEFAULT_INHERITED_ENV_KEYS (line 636) are now dead entries on this path.
Both are read out of process.env at line 682, survive Object.assign(env, configEnv, inputEnv) and stripTrackerSecrets, and are then unconditionally overwritten by the pinned assignments here. I checked that nothing between the two points reads either name — stripTrackerSecrets reads only SYMPHONY_TRACKER_SECRET_ENVIRONMENT_NAMES off env and repositoryDir/projectDir off arguments, and stripCredentialEnvironmentForAgentChild only deletes. So the inherit list can drop both without changing the child env.
Worth doing only because leaving them reads as though the host value still matters on the isolated path — which is exactly the misreading that produced the USERPROFILE hole this PR fixes. Zero behavioral risk either way; I confirmed the child env is byte-identical with them removed.
Generated by Claude Code
2 participants
Issues
Summary
HOME,USERPROFILE, and runtime config homes consistently, preserve sanitized Symphony context, and add a CLI patch changeset.Change-point diagram
packages/core/src/runtime/agent-child-env.ts→ filters source context → appends pinned isolation assignmentspackages/runtime-codex+packages/runtime-claude→ collect runtime-specific removal sets → call the shared constructorStart here
packages/core/src/runtime/agent-child-env.ts:5— shared assignment constructor and precedence boundarypackages/runtime-codex/src/runtime.ts:657— Codex removal set and final pinnedCODEX_HOMEpackages/runtime-claude/src/adapter.ts:654— Claude inherit/non-inherit constructionUser-Visible Behavior / Operational Impact
HOMEandUSERPROFILE, plus shared GitHub CLI and Docker config paths.Validation
pnpm exec vitest run packages/core/src/runtime/agent-child-env.test.ts packages/runtime-codex/src/runtime.test.ts packages/runtime-claude/src/adapter.test.ts— pass (84 tests, including after merging currentmain)pnpm lint— passpnpm test— passpnpm typecheck— passpnpm build— passChangeset
.changeset/fuzzy-cats-guard.md(patch)Risks & rollback
0bb0fd6,efa724b, and70ef0cetogether to restore per-runtime construction.Changed files
packages/core/src/runtime/agent-child-env.tsand test — shared constructor, source-only exclusions, and exact assignment coveragepackages/core/src/runtime/index.ts— export the constructorpackages/runtime-codex/src/runtime.tsand test — migrate Codex and preserve removed context names plus pinnedCODEX_HOMEpackages/runtime-claude/src/adapter.tsand test — migrate both Claude environment paths and preserve pinned isolation valuesdocs/architecture.md— map the shared built-in boundary and distinct custom-runtime boundary.changeset/fuzzy-cats-guard.md— patch release notePost-merge / human validation
Security
.envfiles, or generated installation tokens are committed