fix(tracker-github): reconcile tracker secret declarations - #877
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. |
Board review — PR #877 (issue #869)Smoke-tested in a scratch worktree at head Gate results
Source-level acceptance criteria of #869 verified on
|
hojinzs
left a comment
hojinzs
left a comment
There was a problem hiding this comment.
Review round 1/6 — verified independently at 6102416
I re-ran the verification from source rather than relying on the board-review comment, and reproduced its main claims. Two P2 test-strength defects block; everything else is non-blocking.
Requirement coverage vs #869
| Acceptance criterion | Status |
|---|---|
| Each adapter's declaration covers every name it injects, asserted by test | ✅ Met — mutation-verified |
| Tracker secret names come from the adapter declaration only; no hardcoded copies remain in either runtime |
I confirmed on main that the three source fixes #869 asked for did land ahead of this PR, so a test-only PR here is the right shape:
packages/tracker-github/src/orchestrator-adapter.ts:44-54— declaration now carriesGITHUB_TOKEN_BROKER_URL/GITHUB_TOKEN_BROKER_SECRET/GITHUB_TOKEN_CACHE_PATH, matching every keyresolveWorkerCredentialscan return on either branch.packages/runtime-claude/src/adapter.ts:702-713— the per-runtime hardcodedGH_TOKEN/GITHUB_*/LINEAR_*list named in the issue is gone.packages/runtime-codex/src/runtime.ts:611-619, 662-666— same shape.
On the second criterion. The list wasn't deleted so much as relocated: CUSTOM_RUNTIME_RESERVED_AUTH_ENVIRONMENT_NAMES (packages/core/src/runtime/custom-child-env.ts:74-87) still hardcodes GH_TOKEN, GH_ENTERPRISE_TOKEN, GITHUB_TOKEN, GITHUB_GRAPHQL_TOKEN, GITHUB_TOKEN_BROKER_SECRET, LINEAR_API_KEY, LINEAR_AUTHORIZATION — the exact set #869 quoted — and both runtimes union it into the strip set. So "declaration only" is not literally true, and tracker-specific names now sit in packages/core, which cuts against CLAUDE.md's "Keep tracker-specific behavior out of core layers."
I am not asking for a change here: it arrived via #873, it is a strict superset of the declarations (fail-safe, no leak), and widening this PR to chase it would be scope creep. But it does mean the declaration is not the sole source of truth, and no test in this PR can detect that — the new Claude/Codex cases use a synthetic TRACKER_ADAPTER_SECRET precisely because a real tracker name would be stripped by the core constant regardless. Worth a follow-up issue under #844 rather than a silent close of #869.
Smoke test
Built and ran the suite locally on this head (pnpm install --frozen-lockfile → pnpm build → targeted vitest):
✓ packages/tracker-github/src/tracker-github.test.ts (115 tests) 149ms
✓ packages/runtime-claude/src/adapter.test.ts (38 tests) 165ms
✓ packages/runtime-codex/src/runtime.test.ts
✓ packages/tracker-linear/src/tracker-linear.test.ts (48 tests) 53ms
Test Files 4 passed (4) Tests 245 passed (245)
CI on 6102416: Test ✅, Container Smoke ✅. mergeable_state: clean. No API or UI surface changes, so container smoke is the right ceiling here — agreed that the Docker E2E script isn't applicable to a test-only diff.
Mutation testing
The point of this PR is assertion strength, so I mutated the source instead of reading the tests:
- Dropped
"GITHUB_TOKEN_CACHE_PATH"from the GitHub declaration → test fails naming exactly that key. This is the original #869 defect, and it is caught. ✅ - Gutted
resolveWorkerCredentials(earlyreturn undefined;) → the same test passes. ❌ See the P2 below.
Blocking (P2)
Both are one- to two-line fixes in the tests only; no source change.
tracker-github.test.ts:1285-1292—expect.arrayContaining([])is a tautology, andinjectedNamesis?? {}-guarded on both sides, so the guard evaporates without signal if the credential path ever returns nothing.tracker-linear.test.ts:1502-1504— identical shape.
A contract test whose whole job is "the declaration covers everything injected" shouldn't be able to go green with nothing injected. Pin the resolved credentials with toEqual and the arrayContaining check keeps working as intended.
Non-blocking
- P3 — the GitHub test's fixtures map 1:1 onto the resolver's return branches with nothing linking them; a future third branch slips past silently. A one-line comment suffices.
- nit —
collectMcpSecretEnvironmentNamesreads~/.gh-symphony/mcp.jsonviahomedir()with no override, so the Claude tests aren't fully hermetic. Pre-existing. - Agreed the inline
"LINEAR_GRAPHQL_URL"literal atruntime.ts:665(Codex-only, absent from Claude) is out of scope here.
Scope discipline
No over-engineering and no unrequested decisions: 98 added lines, four test files, zero source changes, no speculative abstraction. Omitting the changeset is correct per CLAUDE.md — no shipped behavior changes.
Push the two P2 fixes and I'll approve on the next round.
Generated by Claude Code
hojinzs
left a comment
hojinzs
left a comment
There was a problem hiding this comment.
Review round 2/6 — Approve, verified at a7cbe93
Both round-1 P2s are fixed, and I confirmed that by re-running the exact mutation that defeated them rather than reading the diff or trusting the thread replies.
Round-1 findings — all closed
| # | Finding | Status |
|---|---|---|
| P2 | tracker-github.test.ts — vacuous arrayContaining([]) pass |
✅ Fixed, mutation-verified |
| P2 | tracker-linear.test.ts — same shape |
✅ Fixed, mutation-verified |
| P3 | GitHub fixtures ↔ resolver branches unlinked | ✅ Comment added |
| nit | collectMcpSecretEnvironmentNames non-hermetic homedir() |
✅ Correctly declined as pre-existing |
The P2 fix is load-bearing, not cosmetic. In round 1, adding an early return undefined; to resolveWorkerCredentials let the declaration-coverage test pass with the credential path gutted. Re-run on this head:
× declares every GitHub credential environment name it can inject
→ expected undefined to deeply equal { Object (GITHUB_GRAPHQL_TOKEN) }
× declares every Linear credential environment name it can inject
→ expected undefined to deeply equal { …(2) }
Both now fail. The floor is real. And the test still catches the defect it was written for — dropping "GITHUB_TOKEN_CACHE_PATH" from secretEnvironmentNames() fails naming exactly that key, which is the original #869 regression.
Scope discipline on the round-2 delta
6102416..a7cbe93 is 15 added lines across two test files: two toEqual pins and one comment. Exactly what was asked, nothing adjacent picked up along the way. No over-engineering, no unrequested decisions, no speculative abstraction.
Requirement coverage vs #869
| Acceptance criterion | Status |
|---|---|
| Each adapter's declaration covers every name it injects, asserted by test | ✅ Met, mutation-verified in both directions |
| Tracker secret names come from the adapter declaration only; no hardcoded copies remain in either runtime | ✅ Met at the runtime level |
Re-confirmed on this head that no tracker-name literals survive in either runtime source — grep for "GH_TOKEN" / "GH_ENTERPRISE_TOKEN" / "LINEAR_API_KEY" / "LINEAR_AUTHORIZATION" across runtime-claude/src and runtime-codex/src (excluding tests) returns nothing. stripTrackerSecrets is now declaredNames ∪ MCP-sourced names ∪ the shared core constant.
Smoke test
No API or UI surface is touched, so the local suite plus CI container smoke is the right ceiling here; there is no preview environment to exercise.
pnpm build✅ ·pnpm lint✅ ·pnpm typecheck✅pnpm test✅ — 2,026 tests, 14 packages, 0 failed- Four touched files: 245 passed (245)
- CI on
a7cbe93:Test✅,Container Smoke✅ pnpm formatreports 72 files, but the identical 72 fail onmain— pre-existing, and none of this PR's four files are among them. Not this PR's.- Base is behind
mainby one docs-only commit (#875), so no conflict risk.
Omitting the changeset remains correct per CLAUDE.md: tests only, no shipped behavior.
Carried forward, not blocking
The round-1 observation stands unchanged and I am explicitly not gating on it: CUSTOM_RUNTIME_RESERVED_AUTH_ENVIRONMENT_NAMES (packages/core/src/runtime/custom-child-env.ts) still hardcodes the exact tracker-name set #869 quoted, so "declaration only" is true per-runtime but not end-to-end, and tracker-specific names sit in packages/core against CLAUDE.md's "keep tracker-specific behavior out of core layers." It arrived via #873, it is a fail-safe superset, and chasing it here would be scope creep. Suggest tracking it as a follow-up under #844 rather than closing #869 silently — your call, I have not filed one.
The "LINEAR_GRAPHQL_URL" literal at runtime-codex/src/runtime.ts:665 (Codex-only, asymmetric with Claude) belongs to that same follow-up.
Approving. Two rounds, both P2s closed with minimal diffs — no reason to hold this further.
Generated by Claude Code
2 participants
Issues
Summary
Change-point diagram
SYMPHONY_TRACKER_SECRET_ENVIRONMENT_NAMES→ Codex and Claude child-environment stripping testsStart here
packages/tracker-github/src/tracker-github.test.ts:1262— GitHub injection/declaration contract and resolver-branch fixturespackages/tracker-linear/src/tracker-linear.test.ts:1490— Linear injection/declaration contractpackages/runtime-codex/src/runtime.test.ts— Codex declaration-driven strippingpackages/runtime-claude/src/adapter.test.ts— Claude declaration-driven strippingUser-Visible Behavior / Operational Impact
Validation
pnpm exec prettier --check packages/tracker-github/src/tracker-github.test.ts packages/tracker-linear/src/tracker-linear.test.ts— passpnpm exec vitest run packages/tracker-github/src/tracker-github.test.ts packages/tracker-linear/src/tracker-linear.test.ts— pass (163 tests)pnpm lint— passpnpm test— passpnpm typecheck— passpnpm build— passChangeset
Risks & rollback
6102416anda7cbe93.Changed files
packages/tracker-github/src/tracker-github.test.ts— asserts declared GitHub secrets cover pinned direct and broker injection resultspackages/tracker-linear/src/tracker-linear.test.ts— asserts declared Linear secrets cover pinned injection resultspackages/runtime-codex/src/runtime.test.ts— asserts Codex strips adapter-declared tracker credentials onlypackages/runtime-claude/src/adapter.test.ts— asserts Claude strips adapter-declared tracker credentials onlyPost-merge / human validation
Security
.envfiles, or generated installation tokens are committed