docs(configuration): define agent-child environment boundary - #886
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. |
![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: 6f621603c8
ℹ️ 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".
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".
moncher-dev
left a comment
moncher-dev
left a comment
There was a problem hiding this comment.
Tested at head 6f621603. pnpm install --frozen-lockfile && pnpm build && pnpm test all pass (14 packages, 0 failures), and CI is green. Requesting changes on content, not on the build.
This PR's whole value is that the documentation describes what actually runs. One of the two new paragraphs does not.
The hook allowlist paragraph is wrong about the daemon environment
The new text says:
To expose additional variables from the effective project/daemon environment, set
SYMPHONY_WORKFLOW_HOOK_ENV_ALLOWLISTto a comma-separated list of names
The daemon environment is not a usable source for either the allowlist variable or its targets. hookEnv is built by buildProjectExecutionEnv (packages/orchestrator/src/service.ts:5200) as:
{ ...projectEnv, ...inheritedEnv, ...explicitEnv }
projectEnvisreadProjectEnv— the project.envfile, and nothing else (service.ts:5137).inheritedEnvisprocess.envfiltered byshouldInheritProcessEnvKey, i.e.INHERITED_ENV_ALLOWLIST=CI, HOME, LANG, LOGNAME, PATH, PWD, SHELL, TERM, TMPDIR, USERplusLC_*(service.ts:118).explicitEnvis Symphony's injected hook context.
SYMPHONY_WORKFLOW_HOOK_ENV_ALLOWLIST is not in that allowlist, so setting it in the daemon environment never reaches hookEnv and is silently ignored. A daemon-only target is worse than ignored — this PR turns it into a hard failure. Using the parser this PR exports:
parseWorkflowHookEnvAllowlist("STAGING_API_HOST", {HOME, PATH, SYMPHONY_WORKFLOW_HOOK_ENV_ALLOWLIST:"STAGING_API_HOST"})
-> THROWS: names an environment variable that is not defined: STAGING_API_HOST
parseWorkflowHookEnvAllowlist("STAGING_API_HOST", {HOME, STAGING_API_HOST:"x", ...})
-> ["STAGING_API_HOST"] # project .env only
So an operator who follows this paragraph and puts the variable in the daemon environment does not get a no-op — they get a failing hook. The Codex reviewer flagged this and is right. Either source allowlisted names from process.env, or say plainly that this is a project-.env-only mechanism.
Worth calling out because this repo's own daemon runs with SYMPHONY_ALLOW_WORKFLOW_HOOKS=1 and an after_create hook.
Secondary, and documented so I am not blocking on it: a trailing comma now throws ("A_VAR," -> invalid environment variable name: "") where it was previously filtered out. The new prose does say malformed entries fail, so this is intentional — just an easy accident to make.
The child-boundary table
I checked the table against the code as it stands after #868/#869/#870/#877 and it reads accurately today, including the #agent-visible-symphony-context anchor, which resolves. One dependency worth noting: the "Tracker-declared secrets ... always stripped" row is only unconditionally true while CUSTOM_RUNTIME_RESERVED_AUTH_ENVIRONMENT_NAMES exists in core. PR #885 removes it, which would make seven of those names conditional on the active tracker's declaration. I have asked for changes on #885 for that reason; flagging it here so the two do not land in an order that leaves this table describing something that no longer runs.
Scope note
The diff for docs/configuration.md is 29 additions and 0 deletions. The issue asked for four disagreeing sections to be replaced by one child-boundary section; this appends a fifth statement and leaves the existing four in place. If the intent was consolidation, the old text still needs to be reconciled or removed — otherwise the next reader still has five places to compare.
Moving #871 back to Ready.
moncher-dev
left a comment
moncher-dev
left a comment
There was a problem hiding this comment.
@hojinzs re-reviewed at head 2ade8c02. The substantive finding is fixed and verified. Ready for your review.
Build
pnpm install --frozen-lockfile && pnpm build && pnpm test on a clean worktree of this head: all 14 packages pass, 0 failures. CI green on both jobs.
The hook paragraph now matches the code
Previous text claimed the allowlist exposed variables from the "effective project/daemon environment". New text:
set
SYMPHONY_WORKFLOW_HOOK_ENV_ALLOWLISTin the managed project.env... and define those names in the same file. A daemon-only allowlist setting or target variable is not a source for hook forwarding because hooks inherit only the portable process variables listed above from the daemon.
That is exactly what buildProjectExecutionEnv does. Re-verified against the parser this PR exports, built from 2ade8c02:
allowlist + target in project .env -> ["STAGING_API_HOST"]
target daemon-only (filtered out) -> THROWS: names an environment variable that is not defined: STAGING_API_HOST
empty / malformed entry -> THROWS: contains an invalid environment variable name: ""
The doc now warns about the throwing case instead of recommending it. The wording change from "unknown entry" to "names a variable absent from the effective hook environment" is also more accurate, since that is the condition actually tested.
Rejection-path coverage is present in packages/orchestrator/src/service.test.ts for both branches (malformed not-valid, and absent MISSPELLED_NAME), which is the issue's second acceptance criterion.
Both acceptance criteria on #871 are met
docs/configuration.mddocuments the agent-child boundary and the hook allowlist variable ✅SYMPHONY_WORKFLOW_HOOK_ENV_ALLOWLISTis validated with test coverage for the rejection path ✅
I re-checked the child-boundary table against the code after #868/#869/#870/#877 and it still reads accurately, anchor included.
One scope question for you, and a correction to my own last review
I previously flagged that the docs/configuration.md diff is additions-only (now 31/0) and suggested it should have replaced the four overlapping sections. Re-reading #871, that is not what the issue asks. Its scope line is "Document the agent-child boundary in docs/configuration.md, next to the existing precedence tables" — explicitly additive — and its out-of-scope list is about behaviour, not consolidation. So I do not think this PR should be bounced for it, and I am not treating it as a blocker.
If you do want the four sections consolidated, it is worth its own issue: deleting them is a judgement call about which wording survives, and it is easy to lose accurate detail (the broker table's per-variable audience column, for instance, has no equivalent in the new table).
One small pre-existing inaccuracy I noticed while comparing, not introduced here: the GITHUB_TOKEN_BROKER_SECRET row in Credential Brokers And Git Access says it "is declared as a tracker secret and removed from agent children by default". On main today it is removed by CUSTOM_RUNTIME_RESERVED_AUTH_ENVIRONMENT_NAMES unconditionally, not by the declaration. That row describes the world #885 is trying to create — and #885 is currently failing on exactly that point, so the row is ahead of the code either way. Worth a follow-up once #885 settles.
Ordering note
The child-boundary table's "Tracker-declared secrets ... always stripped" row is unconditionally true only while the core constant exists. #885 removes it and is still red on the compatibility-mode e2e (re-reviewed at d5bd4298 just now). This PR is safe to land first; #885 is the one that would need this table revisited.
hojinzs
left a comment
hojinzs
left a comment
There was a problem hiding this comment.
Review 1/6 — one P1 blocker, otherwise a well-scoped PR
Requirements vs. #871
| Acceptance criterion | Status |
|---|---|
docs/configuration.md documents the agent-child boundary |
✅ docs/configuration.md:397-414, table + the explicit "a project .env entry does not reach the agent child" statement |
docs/configuration.md documents the hook allowlist variable |
✅ docs/configuration.md:92-104 |
SYMPHONY_WORKFLOW_HOOK_ENV_ALLOWLIST validated |
|
| Rejection path has test coverage | |
| No behaviour change to what the child receives (out of scope) | ✅ documentation only, confirmed against agent-child-env.ts / custom-child-env.ts |
I spot-checked the doc claims rather than taking them on trust: the portable hook set matches DEFAULT_HOOK_ENV_KEYS (packages/core/src/workspace/hooks.ts:272-282); the #agent-visible-symphony-context anchor resolves to the heading at docs/configuration.md:634; and "Codex and Claude also receive a private DOCKER_CONFIG" is right — agent-child-env.ts:20 sets it, custom-child-env.ts:152-153 deliberately does not. No over-engineering or unrequested decisions found — the diff is the issue's scope and nothing more, and the changeset is correctly scoped to patch.
The earlier Codex P2 thread is genuinely resolved, not just marked resolved: 2ade8c0 rewrote the paragraph to describe the mechanism as project-.env-only, which is what the reviewer asked for.
Smoke test
No preview environment on this PR, so I ran it locally against the real modules at 2ade8c0: built the workspace, exercised the runHook → executeWorkspaceHook composition directly, and re-ran the touched suite.
npx vitest run packages/orchestrator/src/service.test.ts— 217 passed, matching the PR body- Targeted reproduction of the composed call path — surfaced the P1 below
Findings
| Finding | |
|---|---|
| P1 | Allowlist validation is evaluated as a call argument, so it runs before executeWorkspaceHook's two skipped returns. A typo'd entry now fails dispatch for a project with no hooks configured, and for the default-off SYMPHONY_ALLOW_WORKFLOW_HOOKS case. Via after_create that also rm -rfs the freshly created workspace. |
| P2 | A trailing comma throws invalid environment variable name: "" — neither of the two conditions #871 asks you to reject, and undiagnosable from the message. |
| P3 | The new describe block is inserted between two import statements in service.test.ts, and packs two rejection paths into one it. |
| nit | Duplicate entries are returned undeduplicated. |
What I need for approval
Just the P1 — a guard so the parser runs only when the hook will actually execute, plus a test pinning "bad allowlist + no hook configured stays skipped". The P2 is a small follow-on in the same function. P3 and the nit are yours to take or decline; a reasoned "no" on the thread is fine and I won't re-raise them.
Everything else here is ready. The documentation is accurate and the boundary it describes is the right one to write down.
Generated by Claude Code
hojinzs
left a comment
hojinzs
left a comment
There was a problem hiding this comment.
Review 2/6 — Approved
All four findings from review 1/6 are fixed at e7aaa5b, and I verified each against the real modules rather than taking the thread replies on trust.
Finding disposition
| Finding | Status | |
|---|---|---|
| P1 | Allowlist parsed as a call argument, firing before both skipped returns |
✅ Fixed — service.ts:5056-5079 hoists hookCommand/trusted and gates the parse on hookCommand && trusted |
| P2 | Trailing/doubled comma threw invalid environment variable name: "" |
✅ Fixed — .filter((entry) => entry.length > 0) precedes the regex check |
| P3 | describe wedged between imports; two rejection paths in one it |
✅ Fixed — import list is contiguous again, and rejects a malformed name / rejects an undefined name are separate |
| nit | Duplicates returned undeduplicated | ✅ Accepted — [...new Set(...)] |
The P1 guard is exactly equivalent to executeWorkspaceHook's two skip returns (packages/core/src/workspace/hooks.ts:247-266): same resolveHookCommand(hooks, kind) on the same object, and the same trusted value that is then passed through. No third skip path exists, so there is no case where validation fires on a path that skips.
Smoke test
No preview environment, so I ran this locally at e7aaa5b — clean pnpm install --frozen-lockfile && pnpm build (exit 0), then a direct reproduction of my original P1 repro against the real executeWorkspaceHook composed with the new call-site expression:
no hooks configured + bad allowlist -> skipped ✅ (was: throws)
hook configured/untrusted + bad allowlist -> skipped ✅ (was: throws)
hook configured + trusted + bad allowlist -> throws ✅ validation still has teeth
"A_VAR,,A_VAR, " -> ["A_VAR"] ✅ P2 + nit
The third case matters: gating the parse could have quietly disabled the feature this issue exists to add, and it does not.
npx vitest run packages/orchestrator/src/service.test.ts— 221 passed (was 217; +4 from this commit), matching the PR bodypnpm lint— pass ·pnpm typecheck— pass ·pnpm build— pass- CI green on both
TestandContainer Smoke - The positive path stays covered end-to-end by
applies allowlisted project .env to approved script hooks(service.test.ts:16070), which runs a real hook script reading the forwarded variables — so the wiring is pinned in both directions now delete process.env.SYMPHONY_ALLOW_WORKFLOW_HOOKSin the new test is restored by the suite'safterEach(service.test.ts:579-590), so no cross-test leakage
Requirements vs. #871
| Acceptance criterion | Status |
|---|---|
docs/configuration.md documents the agent-child boundary |
✅ :397-414 |
docs/configuration.md documents the hook allowlist variable |
✅ :92-106, now including the validation-timing sentence |
SYMPHONY_WORKFLOW_HOOK_ENV_ALLOWLIST validated |
✅ |
| Rejection path has test coverage | ✅ parser-level plus both skip paths as an integration test |
| No behaviour change to what the child receives (out of scope) | ✅ documentation only |
The doc followed the code on the P2 change rather than being left stale — empty separators are ignored and duplicate names are folded together replaces the old an empty or malformed entry wording. That is the detail that most often gets missed in a fix like this.
No over-engineering or unrequested decisions. The diff is the issue's scope and nothing beyond it; the changeset is correctly patch.
One non-blocking note — no response needed
You took the call-site gate, which is the option I offered, so this is an observation and not a request. resolveHookCommand is now evaluated twice for every hook — once in runHook, once inside executeWorkspaceHook — which means service.ts has to stay in step with a skip condition owned by core. It is correct today. If a third skip condition is ever added to executeWorkspaceHook, moving the parse inside it (after its guards) is the version that cannot drift. Not worth a change now.
The #885 ordering note from the earlier review still stands: this PR is safe to land first, and #885 is the one that would need the child-boundary table revisited.
Generated by Claude Code
2 participants
Issues
Summary
.env-only hook allowlist.Change-point diagram
.env→ execution guard → allowlist validation → approved hook environmentStart here
packages/orchestrator/src/service.ts:5053— execution guard around allowlist validationpackages/orchestrator/src/service.test.ts:16107— skipped-path regressionsdocs/configuration.md:89— operator contractdocs/configuration.md:397— agent-child boundaryUser-Visible Behavior / Operational Impact
Validation
pnpm exec vitest run packages/orchestrator/src/service.test.ts— pass (221 tests)pnpm lint— passpnpm test— passpnpm typecheck— passpnpm build— pass./e2e/run-standalone-project-e2e.sh— unavailable twice at Docker preflight (exit 69; daemon socket unreachable); tracked in test(e2e): restore Docker daemon access in Symphony workers #889Changeset
.changeset/quiet-hooks-validate.md(patch)Risks & rollback
Changed files
packages/orchestrator/src/service.ts— validate only executing hooks and canonicalize entriespackages/orchestrator/src/service.test.ts— cover parser rejection/normalization and both skip pathsdocs/configuration.md— document hook validation timing and the agent-child boundarydocs/architecture.md— map validation to the orchestrator boundaryAGENT_TEST.md— record authoritative coverage.changeset/quiet-hooks-validate.md— patch release notePost-merge / human validation
Security
.envfiles, or generated installation tokens are committed