feat(tools,worker): finalize host tool contracts - #775
Conversation
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
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: fab57d915a
ℹ️ 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".
| const allowedTargets = collectScopeValues({ | ||
| id: context.issue.id, | ||
| identifier: context.issue.identifier, | ||
| nativeRef: context.issue.nativeRef, | ||
| }); |
There was a problem hiding this comment.
Because collectScopeValues recursively accepts every string in nativeRef, non-identifiers such as GitHub's contentType, source state, branch names, or Linear's projectSlug satisfy this guard. For example, a mutation targeting another issue can pass "Issue" as its comment body and be executed with the host credential because the active GitHub issue's native reference contains contentType: "Issue"; the identical Linear helper has the same problem. Whitelist adapter-owned issue/item identifiers instead of treating all opaque native metadata as authorized targets.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved by removing the presence-based scope predicate from both GraphQL adapters. It could not bind the document's actual target, so retaining it as an authorization boundary would be unsafe. Normalized { id, identifier, nativeRef } context still reaches the host adapter and remains internal; the contract now states that callers must constrain arbitrary GraphQL documents. Regression tests verify legitimate host calls continue with realistic context.
| const invocationValues = collectScopeValues(invocation.variables ?? {}); | ||
| if (![...invocationValues].some((value) => allowedTargets.has(value))) { |
There was a problem hiding this comment.
When SYMPHONY_TRACKER_KIND is linear, resolveHostToolAdapters() still advertises github_graphql for repository and PR work, but this allowed set contains only the Linear issue ID, identifier, and native reference. A normal GitHub query whose variables are a repository owner/name or PR node ID therefore has no possible match and is rejected before HTTP, despite the documented cross-provider tool availability. Linear runs need relevant GitHub repository/PR scope in the execution context, or the tool should not be advertised as usable there.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved by removing the tracker-context value-match predicate. github_graphql remains advertised and usable for repository and pull-request operations on Linear-tracked runs, while the Linear issue context is still delivered internally to the host adapter. The tracker documentation again describes this cross-provider capability without claiming issue-target enforcement.
hojinzs
left a comment
hojinzs
left a comment
There was a problem hiding this comment.
Review — round 1 of 6
I verified this branch locally at fab57d9. pnpm lint, pnpm test, pnpm typecheck, pnpm build all pass, and both CI checks (Test, Container Smoke) are green — the Validation section of the description is accurate. The failure I hit first was just a build-order artifact on a cold checkout, not a real one.
Requesting changes on one issue: the active-issue scope guard is not the boundary this PR documents it as.
Smoke test
Rather than rely on reading, I exercised executeGitHubGraphQL directly with a nativeRef copied verbatim from what tracker-github actually produces. Three bypasses, all confirmed executing:
✓ BYPASS A: a low-entropy nativeRef constant authorizes another issue
✓ BYPASS B: an unused decoy variable authorizes an inlined foreign target
✓ BYPASS C: the issue state string is enough to authorize any call
Two of these carried an addComment mutation against a foreign issue node ID all the way to fetch with the host credential. Details and suggested fixes are in the inline comment on packages/tool-github-graphql/src/tool.ts.
Bypass B is the one I'd focus on: the guard reads only invocation.variables and never the query text, so an agent can park the real issue ID in an unused variable and inline the actual target as a GraphQL literal. That survives the identity-field whitelist suggested in the Codex thread — it's a target-binding problem, not a field-selection problem.
Requirement traceability vs #673
Verified present:
- Normalized
{ id, identifier, nativeRef }reaches the tools on both runtimes ✅ - Unsupported tool name → structured failure, session continues (
codex-dynamic-tools.test.ts:68) ✅ - Dead
dynamic_tool_call_*/runToolProcesspaths gone — no remaining references ✅ - Tool specs pinned per session, reload doesn't affect in-flight ✅ (clean implementation, good test)
- §10.5 per-tool contracts documented ✅
Gap: the Docker E2E cited as evidence had its viewer { login } probes rewritten to scoped node(id:) calls because the new guard rejects them. Correct in itself, but it means the E2E now covers the happy path only.
Scope / over-engineering
The scope guard is an addition beyond what #673 asked. The issue asks that context be passed so the adapter can narrow (어댑터가 provider 식별자로 scope 제한(§15.5 narrowing) 가능하게) — not that enforcement ship here. That matters for how you resolve this: dropping the guard and restoring the previous "callers must constrain their own document" doc wording is fully in scope for the issue and is a legitimate way to close this review. What I'd avoid is keeping it as-is, because README.md's security section and both tracker docs now describe a hard pre-auth boundary, and a reader will budget trust against it that the code doesn't earn.
The guard is also simultaneously too weak and too strong: it's bypassable (P1) and it breaks the documented cross-provider capability, since github_graphql is advertised on Linear runs but can never match a Linear issue context (P2).
Prior review
The Codex review on this same commit raised both a P1 and a P2, and neither has a fix or a reply on the thread. My review confirms both independently — P1 with a working reproduction, and P1 is somewhat worse than described. Please respond on those threads too, so the record shows the decision.
Findings
| # | Sev | Where | Issue |
|---|---|---|---|
| 1 | P1 | tool-github-graphql/src/tool.ts:156-186 |
Scope guard doesn't bind the operation target — 3 confirmed bypasses |
| 2 | P1 | tool-linear-graphql/src/tool.ts:77-108 |
Identical copy, same defect; projectSlug authorizes the workspace |
| 3 | P2 | mcp-http-server.ts |
github_graphql advertised but unusable on Linear runs |
| 4 | P2 | docs/trackers/*.md, README.md |
Docs claim "exact ID" enforcement the code doesn't implement |
| 5 | P2 | tool.test.ts (both) |
New tests only cover the zero-variable case, which cannot fail |
| 6 | P3 | both tool packages | assertInvocationTargetsActiveIssue/collectScopeValues duplicated verbatim |
| 7 | nit | both tool packages | if (!context) return makes the check silently opt-in for context-less callers |
The tool-spec freeze — the part the PR title is actually about — is solid and I have no concerns with it. Everything above is about the scope guard that came along with it.
Either of the two paths in finding #1 (bind the target properly, or drop the guard and align the docs) resolves this from my side. The narrow fix — a proper target-binding check plus the regression tests listed inline — is a contained change, so I'd expect this to close in one more round.
Generated by Claude Code
d3f9e6b to
04025b8
Compare
moncher-dev
changed the title
hojinzs
left a comment
hojinzs
left a comment
There was a problem hiding this comment.
Review — round 2 of 6 · Approve
Verified locally at 04025b8. pnpm install && pnpm build && pnpm test && pnpm lint && pnpm typecheck && pnpm format all pass, and both CI checks (Test, Container Smoke) are green on this head.
The round-1 P1 is resolved. You took option 2 — dropping the guard and restoring the caller-owned narrowing contract — which is what I said would close this from my side, and you landed it once across both packages rather than fixing one and missing the other.
Verification
tool.ts in both tool packages is now byte-identical to origin/main (git diff origin/main...HEAD touches neither), and there are no surviving references to assertInvocationTargetsActiveIssue / collectScopeValues anywhere in the tree. The net PR is now the tool-spec freeze plus tests and docs — 182/19 across 12 files, and every remaining line is in scope for #673.
I re-ran my round-1 bypasses plus the cases the guard used to break, against the built packages:
✓ cross-provider GitHub query executes under Linear context
✓ nativeRef itemId absent from payload
✓ payload top-level keys are GraphQL-only (query,variables)
✓ zero-variable query no longer rejected
✓ Linear query executes with context
✓ Linear nativeRef absent from payload
✓ empty-identity context still rejected
And the freeze, end-to-end against a live loopback server:
✓ reload (new array) does not affect in-flight session
✓ reloaded tool name is not callable in-flight
✓ snapshotted tool still executable
✓ in-place spec mutation does not leak (structuredClone)
✓ unknown tool -> structured error
✓ session usable after unknown tool
✓ unauthenticated request rejected 401
That last block is the part the PR title is actually about, and it is correct on all three axes — spec listing, execution binding, and deep-copy isolation.
Round-1 findings
| # | Sev | Status |
|---|---|---|
| 1 | P1 | ✅ Guard removed; 3 bypasses no longer reachable as an authorization claim |
| 2 | P1 | ✅ Removed in the Linear package too, in the same commit |
| 3 | P2 | ✅ Confirmed by smoke: a GitHub repo query now executes under a Linear issue context |
| 4 | P2 | README.md and both tracker docs corrected; docs/architecture.md:97 still claims "active-issue scope validation" |
| 5 | P2 | ✅ Vacuous zero-variable tests replaced with executing tests on realistic nativeRef (see P3 inline) |
| 6 | P3 | ✅ Resolved by deletion rather than extraction — the right call for code that shouldn't exist |
| 7 | nit | n/a — now outside this PR's diff, since tool.ts is untouched |
Requirement traceability vs #673
All acceptance criteria hold, and guard removal doesn't cost any of them — the issue asked that context be passed so the adapter can narrow (어댑터가 provider 식별자로 scope 제한(§15.5 narrowing) 가능하게), not that enforcement ship here:
- Normalized
{ id, identifier, nativeRef }reaches tools on both runtimes ✅ - Unsupported tool name → structured failure, session continues ✅ (Codex
codex-dynamic-tools.test.ts:68; Claude verified by smoke) - Dead
dynamic_tool_call_*/runToolProcesspaths gone ✅ - Tool specs pinned per session, reload doesn't affect in-flight ✅
- §10.5 per-tool contracts documented ✅
- Host credentials absent from child env and generated MCP config ✅
The E2E probes are back to their original repository/workspace query shape, so the Docker evidence once again exercises the real call shape rather than the guard's happy path.
Over-engineering
Nothing left beyond the issue's ask. The one over-reach — enforcement the issue didn't request — is gone. The adapters? injection seam is a fair trade for making the freeze testable, and the routable/routableReason fixture refresh is legitimate rather than scope creep: worker/src/turn-lease.ts:69 reads those fields.
Findings
| # | Sev | Where | Issue |
|---|---|---|---|
| 1 | P2 | docs/architecture.md:97 |
Claims "active-issue scope validation" the code no longer does |
| 2 | P3 | tool.test.ts (both) |
Test passes context but asserts nothing about it; nativeRef non-leak untested |
| 3 | nit | mcp-http-server.test.ts:16 |
Freeze test covers tools/list only, not execution binding or structuredClone |
| 4 | nit | docs/trackers/github.md, worker-protocol.test.ts |
Whitespace/prettier-only churn in the diff |
Approving. Finding #1 is a three-word correction and the last echo of the problem that drove round 1 — please land it before merge, along with #2 if it's cheap; neither warrants another review round, and the rest are optional. Requesting changes again over a doc line would be exactly the ping-pong worth avoiding on a PR that is otherwise clean.
Per #673's merge gate, remember #700 ships in this same release, ordered #673 → #700.
Generated by Claude Code
| synthetic `Archived` state is a GitHub-specific implementation choice, not | ||
| normalized Symphony core behavior. | ||
| - Host-side tracker tools: provider adapters own advertised schemas, credentials, | ||
| and active-issue scope validation. Codex snapshots those schemas in its runtime |
There was a problem hiding this comment.
P2 — this line still claims the enforcement that b78e3b2 removed
provider adapters own advertised schemas, credentials, and active-issue scope validation
This is the last surviving instance of the round-1 finding #4 defect. README.md and both tracker contracts were correctly walked back, but this bullet — added by this PR — was not updated when the guard came out.
After the removal, the only thing left in either adapter is assertTrackerToolExecutionContext (tool-github-graphql/src/tool.ts:144), which checks that issue.id / issue.identifier are non-empty. That is context shape validation, not scope validation: it inspects nothing about the operation's target, and I confirmed a foreign-target document executes fine under an active-issue context.
Suggested wording, which also matches what docs/trackers/*.md now say:
| and active-issue scope validation. Codex snapshots those schemas in its runtime | |
| - Host-side tracker tools: provider adapters own advertised schemas and credentials, | |
| and receive normalized active-issue context that stays host-internal. Callers are | |
| responsible for narrowing documents; adapters do not infer or rewrite a target. | |
| Codex snapshots those schemas in its runtime | |
| plan; Claude snapshots them when its loopback Streamable HTTP MCP server starts. |
The second sentence about snapshotting is accurate as written — I verified both runtimes behave that way.
Generated by Claude Code
There was a problem hiding this comment.
Resolved in 49eae3fe. docs/architecture.md now uses the suggested boundary: adapters own schemas and credentials, receive host-internal normalized issue context, and leave target narrowing to callers without inferring or rewriting the GraphQL target.
| }); | ||
|
|
||
| describe("executeGitHubGraphQL", () => { | ||
| it("executes a repository query while carrying host-side issue context", async () => { |
There was a problem hiding this comment.
P3 — the test's name promises more than its assertions deliver (same for the Linear twin at tool-linear-graphql/src/tool.test.ts:57)
This is a genuine improvement on what it replaced — it uses a realistic nativeRef and proves guard removal doesn't block a legitimate call, which was its stated purpose. But context is passed and then never asserted on. The two assertions are "resolves to the payload" and "fetch called once", both of which hold identically if the 4th argument is dropped entirely.
The invariant worth pinning is the one the contract states in prose and nothing tests:
nativeRefremains host-internal and is never sent as an extra GraphQL payload field.
I verified it holds today — a repo query under a full GitHub nativeRef (including linkedPullRequests URLs and branch names) produced a body with keys query,variables only, and no context string anywhere in it. Two lines pin it:
const body = JSON.parse(String(fetchImpl.mock.calls[0][1]!.body));
expect(Object.keys(body).sort()).toEqual(["query"]);Worth having because this is a claim a reader will treat as load-bearing, and it is currently held up by nothing but the absence of code that would break it.
Generated by Claude Code
There was a problem hiding this comment.
Resolved in 49eae3fe for both GitHub and Linear. Each context-carrying execution test now parses the outbound request body and asserts its only top-level key is query, pinning that normalized context and nativeRef are not serialized into provider payloads.
| }); | ||
|
|
||
| describe("Claude host MCP HTTP server", () => { | ||
| it("freezes the advertised tool specs when the server starts", async () => { |
There was a problem hiding this comment.
nit — the freeze test covers one of the change's three halves
I have no concerns with the implementation; I exercised all three halves against a live server and every one behaves correctly. Just noting what the suite would catch on a regression:
| Behavior | Covered here |
|---|---|
tools/list ignores a reloaded spec list |
✅ |
tools/call stays bound to the snapshot entry |
❌ |
structuredClone defeats in-place spec mutation |
❌ |
The second is the one I'd add, since it's the half that actually matters at runtime — a reload could otherwise re-point execution mid-session while tools/list still looks frozen:
const call = await fetch(server.url, { /* …tools/call name: "reloaded_tool" */ });
// must be rejected; "snapshotted_tool" must still executeFor the third: because the test reassigns specs rather than mutating it, it passes with or without structuredClone — so the deep copy is currently unverified. Mutating specs[0]!.name in place instead would cover both the reassignment and the mutation path in one test.
Non-blocking — the behavior is right, this is only about what a future regression would trip over.
Generated by Claude Code
There was a problem hiding this comment.
Acknowledged as non-blocking. I left this test unchanged in the narrow pre-merge correction: the implementation and owner smoke already verify execution binding and in-place mutation isolation, while this cycle is limited to the required architecture fix and the cheap adapter payload assertions. The existing snapshot test continues to pin the advertised-spec reload behavior.
2 participants
Issues
TL;DR
{ id, identifier, nativeRef }context at the host adapter boundary without exposing provider credentials to child processes, generated MCP configuration, or provider GraphQL payloads.Change-point diagram
Start here
packages/runtime-claude/src/mcp-http-server.tsand its reload regression test.e2e/host-dynamic-tool-e2e.mjsandtest/e2e/claude/claude-docker.spec.ts.docs/trackers/github.mdanddocs/trackers/linear.md.Changes
@gh-symphony/clipatch changeset.Evidence
pnpm lint && pnpm test && pnpm typecheck && pnpm build— pass on head49eae3fe.pnpm exec vitest run packages/tool-github-graphql/src/tool.test.ts packages/tool-linear-graphql/src/tool.test.ts— pass (2 files, 41 tests), including host-internal context payload assertions.TestandContainer Smoke— pass on head49eae3fe.docker compose -f docker-compose.e2e.yml exec -T symphony-e2e node /app/e2e/host-dynamic-tool-e2e.mjs— pass (host_dynamic_tool_e2e=pass).pnpm e2e:claude— pass (5/5).git merge-base --is-ancestor origin/main HEAD— pass.Risks & rollback
Changed files
packages/runtime-claude/src/mcp-http-server.tsandmcp-http-server.test.ts.docs/architecture.md,docs/trackers/github.md, anddocs/trackers/linear.md..changeset/host-scoped-tools.md.Post-merge / human validation
Human Validation