test(e2e): assert agent-child environment contract - #899
Conversation
Coverage Report
File CoverageNo changed files found. |
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. |
moncher-dev
left a comment
moncher-dev
left a comment
There was a problem hiding this comment.
Blocking. The new assertion is green in CI, but mutation testing shows it cannot fail for the regression classes #872 exists to catch: the expected values are derived by calling the same declaration/builder the production code calls, so the test moves with the implementation.
The spec runs on a Docker-free host, so every result below is reproducible in seconds:
pnpm install --frozen-lockfile && pnpm build
npx vitest run --config test/e2e/claude/vitest.config.ts \
-t "enforces the exported agent-child environment contract"
Mutations run against 7d485848 (all four applied one at a time, suite restored between):
| # | mutation | result |
|---|---|---|
| 1 | delete stripCredentialEnvironmentForAgentChild(env) from the non-inherit branch of buildClaudeSpawnEnv (packages/runtime-claude/src/adapter.ts) |
RED — GITHUB_GIT_HOST: expected 'github.example' to be undefined |
| 2 | remove "GIT_ASKPASS" from AGENT_CHILD_CREDENTIAL_ENVIRONMENT_NAMES (packages/core/src/runtime/custom-child-env.ts) |
GREEN |
| 3 | change DOCKER_CONFIG: join(childHome, ".docker") to join(childHome, "MUTATED-docker") in buildAgentChildEnvironmentAssignments |
GREEN |
| 4 | remove "SYMPHONY_ISSUE_STATE" from AGENT_VISIBLE_SYMPHONY_CONTEXT_ENVIRONMENT_NAMES |
GREEN |
Only #1 is caught.
2 is a real leak, not an unrepresentable one. With GIT_ASKPASS dropped from the declaration the Claude child does receive it; the test misses it only because injectedCredentials and the absence loop both iterate that same array, so the name is never injected. Replacing both with a literal list turns the same mutation red:
const names = ["GIT_ASKPASS", "GITHUB_GIT_HOST"]; // in place of both uses of the constant
// → AssertionError: GIT_ASKPASS: expected 'e2e-secret-GIT_ASKPASS' to be undefinedThe e2e-secret sentinel sweep does not rescue this: a name absent from the declaration is never injected, so it carries no sentinel.
3 is the DOCKER_CONFIG incident the issue cites by name. expect(childEnvironment).toMatchObject(buildAgentChildEnvironmentAssignments(...)) computes the expectation with the production builder, so any change to what that builder assigns changes both sides at once. 4 is the same defect on the visible-context half: dropping a name silently stops shipping it to the child and the test still passes.
Codex is not covered. The new test only drives the Claude harness. packages/runtime-codex/src/runtime.ts has its own stripCredentialEnvironmentForAgentChild(plan.env); commenting it out leaves the entire spec green (10/10 passed), and the codex stub in this file never records process.env.
What would satisfy the acceptance criteria:
- Write the expected credential names and the expected host-constructed assignments as literals in the test, independent of
AGENT_CHILD_CREDENTIAL_ENVIRONMENT_NAMES,AGENT_VISIBLE_SYMPHONY_CONTEXT_ENVIRONMENT_NAMESandbuildAgentChildEnvironmentAssignments. Cross-check the literal list against the exported constant in a separate assertion — that is what makes "a new injected name that is not covered fails the test" true, and it fails loudly when the declaration shrinks. - Assert
HOME,USERPROFILE,GH_CONFIG_DIR,DOCKER_CONFIGandGIT_TERMINAL_PROMPTagainst literal expected paths built from the harness root, not from the builder's return value. - Dump and assert the Codex child's environment too — the codex stub can append
{...process.env}the same waytest/e2e/stubs/claude.shnow does.
Before re-requesting review, please paste the mutation results for 2, 3, 4 and the Codex strip going red.
CI on 7d485848: Test pass, Container Smoke pass. pnpm install --frozen-lockfile && pnpm build && pnpm test all pass locally. The finding is not about the suite being broken — it is about the assertion being unfalsifiable.
|
Passing. Re-ran the full mutation battery at head Baseline: 10/10 pass.
All five red. Expectations are independently sourced. Both paths spawn and dump. Claude via No assertion was weakened. Diffing the spec One non-blocking note for follow-up, recorded so it is not mistaken for coverage this PR claims: on the Codex path the credential-absence loop is currently vacuous. With mutation 5 applied I dumped the Codex child environment and no contract credential name appears at all — not even the Local gate at Requesting @hojinzs for the merge decision. |
hojinzs
left a comment
hojinzs
left a comment
There was a problem hiding this comment.
Approving. The round-1 blocking finding is genuinely fixed — I re-ran the mutation battery myself rather than taking the summary on trust, and all of it reproduces.
Independently reproduced at 354c2617 (clean install, pnpm build, npx vitest run --config test/e2e/claude/vitest.config.ts; each mutation applied alone, source restored and rebuilt between — full pnpm build for the Codex ones since that test spawns packages/worker/dist/index.js). Baseline 10/10 pass.
| # | mutation | claimed | observed |
|---|---|---|---|
| 2 | remove "GIT_ASKPASS" from AGENT_CHILD_CREDENTIAL_ENVIRONMENT_NAMES |
RED | RED — 2 failed / 8 passed |
| 3 | DOCKER_CONFIG → join(childHome, "MUTATED-docker") |
RED | RED — 2 failed / 8 passed |
| 4 | remove "SYMPHONY_ISSUE_STATE" from AGENT_VISIBLE_SYMPHONY_CONTEXT_ENVIRONMENT_NAMES |
RED | RED — 2 failed / 8 passed |
| 5 | comment out stripCredentialEnvironmentForAgentChild(plan.env) in runtime-codex |
RED | RED — 1 failed / 9 passed |
The independent-literal rework is what does it: buildAgentChildEnvironmentAssignments is no longer imported by the spec, so 3 can no longer move both sides at once, and the separate toEqual against the exported constants is what turns 2 and 4 red. Mutation 3 failing on two tests also confirms the Codex path really does assert the managed paths, not just Claude.
Acceptance criteria on #872, checked against what I ran:
- Black-box assertion over the child's environment rather than unit-level expectations — met; the Claude path's leak detection is real, proven by mutation 1.
- Driven by the exported contract so an uncovered name fails — met, and this is the half that was broken in round 1.
- CI
container-smokepasses with the assertion active — met:Container SmokeandTestboth success on354c2617.
No scope creep. The diff is four files, all test/docs, and the environment-name moves in the Codex transport test preserve its original assertions (git_transport_failed:, the ancestor check). No changeset is correct for test-and-docs-only.
Local gate at 354c2617: pnpm install --frozen-lockfile, pnpm build, pnpm lint, pnpm typecheck all pass; pnpm test green across all packages (1611 tests); e2e spec 10/10.
Findings — none blocking, left inline:
- P2,
sanitizeRepositoryCloneUrlis not covered: making it an identity function leaves the suite 10/10 green, because both fixtures use credential-free clone URLs. Userinfo inTARGET_REPOSITORY_CLONE_URLis a credential reaching the child and core special-cases that name to strip it, so it belongs to the contract this PR asserts. One-line fixture change; worth folding in before merge rather than deferring, but it is a pre-existing coverage gap and not a regression this PR introduces, so it does not hold the approval. - P3, on your own Codex note: correct, and I reproduced it — stripping credentials while preserving
GIT_TERMINAL_PROMPTstays green. But every input to Codexplan.envis already a closed allowlist, so no contract credential name can reach it via the launcher at all. The follow-up as you framed it would test an unreachable state; details inline, worth reading before filing it. - P3, five fixture values carry no
e2e-secretsentinel, so the sweep can't catch a rename-class leak for those names. - nit, the declaration cross-check is coupled to object key insertion order.
- nit, the Codex environment assertion rides inside
"fails the Codex worker lifecycle when post-run Git transport cannot publish"(L533). Reusing that spawn is a fair trade, but the test name no longer describes what it asserts, and if it is later restructured or dropped the Codex coverage goes with it silently. A one-line comment at theassertAgentChildEnvironmentcall noting the piggyback would protect it.
The P2 is the only one I'd act on before merge, and it is a one-liner. Everything else is follow-up or judgment.
Generated by Claude Code
| SYMPHONY_ISSUE_IDENTIFIER: "test-owner/test-repo#872", | ||
| SYMPHONY_ISSUE_STATE: "In progress", | ||
| SYMPHONY_TRACKER_KIND: "github-project", | ||
| TARGET_REPOSITORY_CLONE_URL: "https://github.com/test-owner/test-repo.git", |
There was a problem hiding this comment.
P2 — sanitizeRepositoryCloneUrl is not covered by this contract test.
Both fixtures use credential-free clone URLs (this one, and remote on the Codex path at L608, a bare local path). So the sanitizer is only ever exercised on input it passes through unchanged.
Verified by mutation at 354c2617, on top of your battery:
| # | mutation | result |
|---|---|---|
| B | sanitizeRepositoryCloneUrl → return cloneUrl; (identity) |
GREEN — 10/10 pass |
Userinfo in TARGET_REPOSITORY_CLONE_URL is a credential crossing into the agent child, and readAgentVisibleSymphonyContext special-cases exactly this name to strip it. That makes it part of the contract #872 is asking to be asserted end to end, and right now removing the strip is invisible.
The fix is one line here — give the fixture userinfo and expect the sanitized form:
TARGET_REPOSITORY_CLONE_URL:
"https://github.com/test-owner/test-repo.git",
// injected as:
// https://e2e-secret-user:e2e-secret-token@github.com/test-owner/test-repo.gitInjecting the sentinel-bearing form while keeping the sanitized string as the expectation makes mutation B red twice over — once on the toMatchObject value, once on the e2e-secret sweep at L881. Worth folding in rather than deferring, given the cost.
Generated by Claude Code
| const childEnvironment = JSON.parse( | ||
| await readFile(childEnvironmentPath, "utf8") | ||
| ) as NodeJS.ProcessEnv; | ||
| assertAgentChildEnvironment( |
There was a problem hiding this comment.
P3 — your disclosed Codex note is correct, but the follow-up you proposed is probably not worth filing.
I reproduced the vacuity independently:
| # | mutation | result |
|---|---|---|
| A | replace stripCredentialEnvironmentForAgentChild(plan.env) with plan.env.GIT_TERMINAL_PROMPT = "0" |
GREEN — 10/10 pass |
So a Codex regression that leaked credentials while preserving GIT_TERMINAL_PROMPT passes, exactly as you wrote.
But tracing why changes the remedy. Every input to Codex plan.env is already a closed allowlist:
resolveRuntimeProcessEnv()→SAFE_RUNTIME_ENV_KEYS, 11 names, no credentials (runtime.ts:147)config.extraEnv→readAgentVisibleSymphonyContext+HOME/CODEX_HOME/WORKSPACE_RUNTIME_DIR(launcher.ts:44)config.agentEnv→DIRECT_AGENT_ENV_KEYS, the fourOPENAI_*names (runtime.ts:36)
No name in AGENT_CHILD_CREDENTIAL_ENVIRONMENT_NAMES can reach plan.env through the launcher at all. That is why injecting GITHUB_GIT_HOST at L619 never arrives. stripCredentialEnvironmentForAgentChild(plan.env) on this path is unreachable-for-credentials defense in depth, not a live filter.
So "route the credential fixture into the Codex runtime plan" would mean deliberately bypassing that allowlist in a fixture to assert a strip that only fires on input the architecture cannot produce — a test for an unreachable state. I'd either drop that follow-up or reframe it as "assert the Codex input allowlist stays closed", which is the property actually worth protecting and which the absence loop plus toMatchObject already approximate. Recording this so the follow-up isn't filed on the stronger reading.
Generated by Claude Code
| function agentChildCredentialFixtureValue(name: string): string { | ||
| if (name.endsWith("_URL")) | ||
| return `https://e2e-secret-${name.toLowerCase()}.example`; | ||
| if (name.endsWith("_PID") || name === "GIT_CONFIG_COUNT") return "1"; | ||
| if (name === "GIT_CONFIG_NOSYSTEM") return "true"; | ||
| if (name === "GIT_CONFIG_KEY_0") return "credential.helper"; | ||
| if (name === "GIT_CONFIG_VALUE_0") return "e2e-secret-helper"; | ||
| if (name === "GITHUB_GIT_HOST") return "github.example"; | ||
| if (name.includes("PATH") || name === "GIT_DIR" || name === "GIT_WORK_TREE") { | ||
| return `/tmp/e2e-secret-${name.toLowerCase()}`; | ||
| } | ||
| return `e2e-secret-${name}`; |
There was a problem hiding this comment.
P3 — five of the 22 fixture values carry no e2e-secret sentinel, so the sweep at L881 has holes.
GITHUB_GIT_HOST → "github.example", GIT_CONFIG_COUNT/SSH_AGENT_PID → "1", GIT_CONFIG_NOSYSTEM → "true", GIT_CONFIG_KEY_0 → "credential.helper".
Exact-name leaks for these are still caught by the absence loop, so nothing is broken. But the sentinel sweep exists to catch the other class — a credential copied through under a different name — and for these five that class is uncovered: the value carries no marker to find it by.
GITHUB_GIT_HOST is the one that matters, since it is the only one of the five with a free-form value: "e2e-secret-git-host.example" is still a valid hostname and closes the gap at no cost. GIT_CONFIG_KEY_0 could be "credential.e2e-secret-helper" if you want it too; the numeric and boolean ones are genuinely constrained and fine as they are.
Generated by Claude Code
| expect([...AGENT_VISIBLE_SYMPHONY_CONTEXT_ENVIRONMENT_NAMES]).toEqual( | ||
| Object.keys(visibleContext) | ||
| ); |
There was a problem hiding this comment.
nit — this cross-check is coupled to object key insertion order.
Object.keys(visibleContext) makes the declaration comparison order-sensitive. On the Codex path it only lines up because expectedCodexVisibleContext (L603) spreads expectedVisibleContext first, so the overridden keys keep their original positions. Reorder those literal keys — write SYMPHONY_ASSIGNED_BRANCH before the spread, say — and this fails for a reason that has nothing to do with the contract.
Sorting both sides keeps the add/remove tripwire (which is what turns mutations 2 and 4 red) while dropping the coupling:
expect([...AGENT_VISIBLE_SYMPHONY_CONTEXT_ENVIRONMENT_NAMES].sort()).toEqual(
Object.keys(visibleContext).sort()
);Same applies to the credential-names comparison just above, though that one has no spread to trip over.
Generated by Claude Code
2 participants
Issues
Summary
Change-point diagram
Start here
test/e2e/claude/claude-docker.spec.ts:57— independent expected contract names and contexttest/e2e/claude/claude-docker.spec.ts:258— Claude child environment assertiontest/e2e/claude/claude-docker.spec.ts:538— Codex child environment capture through the built workere2e/scenarios/20-agent-child-isolation.md— TC-20 scenario expectationsUser-Visible Behavior / Operational Impact
Validation
pnpm test:worker-startup— pass (10 tests)pnpm lint— passpnpm test— passpnpm typecheck— passpnpm build— pass./e2e/run-standalone-project-e2e.sh— expected exit 69: Docker daemon unavailable on the orchestration host; PRContainer Smokeis authoritative per test(e2e): assert the agent-child environment contract as a black-box check #872Container Smokejob 101322040722 — pass (SUCCESS) on354c26172a4dcc8b2f355ada22ffb264294b9917GIT_ASKPASSfrom the exported credential names — red (literal/export mismatch)DOCKER_CONFIGassignment — red (literal path mismatch)SYMPHONY_ISSUE_STATEfrom visible context — red (literal/export mismatch)Changeset
Risks & rollback
354c261to roll back the rework.Changed files
test/e2e/claude/claude-docker.spec.ts— independently asserts Claude and Codex child environments and cross-checks exported declarations.test/e2e/stubs/claude.sh— captures the complete spawned Claude environment (from the initial cycle).e2e/scenarios/20-agent-child-isolation.md— documents both runtime paths and independent expectations.AGENT_TEST.md— updates the living E2E coverage matrix.Post-merge / human validation
Container Smokepassed with the assertion active.Security
.envfiles, or generated installation tokens are committed