feat(trackers): add agent tool execution contract - #754
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: f920ec55dd
ℹ️ 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".
| return env.SYMPHONY_TRACKER_KIND === "linear" | ||
| ? linearTrackerAdapter | ||
| : githubProjectTrackerAdapter; |
There was a problem hiding this comment.
When SYMPHONY_TRACKER_KIND is linear, buildCodexRuntimePlan still advertises both github_graphql and linear_graphql (packages/runtime-codex/src/runtime.ts:592-601), but this resolver sends every advertised call to linearTrackerAdapter. Consequently, a Linear-tracked agent calling the advertised GitHub tool for repository or PR work now receives Unknown Linear agent tool; select the adapter by tool ownership or stop advertising that tool.
AGENTS.md reference: AGENTS.md:L58-L61
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in d90496af: advertised tools are now mapped to their owning adapter by tool name, so Linear sessions retain github_graphql alongside linear_graphql.
| const result = await adapter.executeAgentTool( | ||
| toolName, | ||
| argumentsValue, | ||
| context | ||
| ); |
There was a problem hiding this comment.
When credentials or custom provider URLs exist only in a worker/package .env, loadLauncherEnvironment() places them in the env argument without mutating process.env, but this call no longer passes that environment to the adapter and both new adapter implementations read process.env directly. Such locally launched Codex sessions therefore lose authentication or use the wrong endpoint even though startup loaded the values successfully; retain the supplied environment in the execution contract rather than switching to the global process environment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in d90496af: AgentToolExecutionContext now carries the loaded host environment, and both adapters derive credentials and endpoints from that context rather than ambient process.env.
hojinzs
left a comment
hojinzs
left a comment
There was a problem hiding this comment.
Review 1/6 — Request changes
The contract shape in packages/core is the right one and the per-tool docs are genuinely good (all six §10.5 columns present for both providers). But converging the Codex path onto it introduced two regressions I reproduced against this branch's build, and the E2E doesn't yet cover the acceptance criterion #732 names.
Verification performed
| Check | Result |
|---|---|
pnpm install && pnpm build |
pass |
pnpm lint |
pass |
pnpm typecheck |
pass |
pnpm test |
1 failure — runtime-claude/src/spawn.test.ts ("no output for 80ms"); passes on re-run, timing-sensitive and untouched by this diff. Not a PR defect. |
Smoke: host tool with .env-sourced credential |
fail — see P1-1 |
Smoke: github_graphql under SYMPHONY_TRACKER_KIND=linear |
fail — see P1-2 |
Smoke: github_graphql under a GitHub tracker with exported creds |
pass |
No preview environment on this PR, so smoke testing was done locally against the built packages/worker/dist through the real adapters with only the provider HTTP boundary stubbed — the same boundary TC-19 stubs.
P1
-
.env-sourced tracker credentials are silently dropped. The adapters readprocess.env(tracker-github:89-93,tracker-linear:244-246) while the worker threadslauncherEnv, which merges.envfiles overprocess.env. A token configured only in.envnow yieldstool_execution_failed: Either GITHUB_GRAPHQL_TOKEN or the runtime token broker configuration is required.and no provider request. The whole broker path is affected too. TheObject.assign(process.env, env)added toe2e/host-dynamic-tool-e2e.mjsand thevi.stubEnvcalls added to three test files are the symptom — they adapt the tests to the regression instead of catching it. -
github_graphqlis broken for every Linear-tracked project.resolveTrackerToolAdapterpicks one adapter by tracker kind, but the Codex runtime advertisesgithub_graphqlunconditionally and addslinear_graphqlon top when the tracker is Linear. Result under a Linear tracker:Unknown Linear agent tool: github_graphql. The removedswitchhandled both names. Dispatch should be by tool name across the advertised set, not by tracker kind.
P2
-
Only half the call paths converged.
runtime-claude/src/mcp-http-server.tsstill hardcodes tool names, schemas and directexecuteGitHubGraphQL/executeLinearGraphQLcalls. #732 scopes this as converging the paths A/B left temporary — the Claude transport is untouched by the new contract, and now advertises schemas that differ from the onesdocs/trackers/*.mddocuments as the contract. -
E2E doesn't exercise a tracker transition or comment. #732 requires the Docker E2E to perform 트래커 전이·코멘트 through the host-side tool. Both the new Codex helper and the (unmodified) Claude spec only run
query { viewer { login } }. The documented "Mutations: Permitted" contract line — the one carrying write authority — has no test behind it. -
docs/trackers/github.md:19asserts something false —github_graphqlis not advertised only under a GitHub adapter.
P3 / nit
- Default
allowedToolNamesis derived from the env-selected adapter even whendependencies.adapteris injected. @gh-symphony/tool-linear-graphqlis now an unused direct dep ofpackages/worker;docs/architecture.md:113,119andCLAUDE.md:51,54dependency graphs are stale.- nit — 7 changed files fail
npx prettier --check, including both newdocs/trackers/*.mdand the e2e helper. The repo is broadly unformatted already (92 files), so this is low priority and not a merge blocker.
Scope
No over-engineering found — the added types are all within #732's stated scope. The one unrequested design decision is the env-based single-adapter resolution, and it is what produced P1-2; #732 specifies the contract but not how the worker selects an adapter.
P1-1 and P1-2 are merge blockers. P2-3 and P2-4 are the stated 완료 기준 of the issue, so I'd want them in this PR rather than deferred — but if you have a reasoned case for splitting either into a follow-up, say so on the thread and I'll take it.
Generated by Claude Code
|
리뷰 보완 (독립 검증, 1회차 Request changes에 추가) — (1) main 충돌: 09:27–28Z에 #738/#747/#734 머지로 |
f920ec5 to
d90496a
Compare
8dc393b to
5daad36
Compare
hojinzs
left a comment
hojinzs
left a comment
There was a problem hiding this comment.
Review 2/6 — Approve
All eight findings from round 1 are fixed, and I verified each against this branch's build rather than taking the thread replies at face value. Both P1 regressions are genuinely gone, and the two P2s that were the issue's stated 완료 기준 are now implemented rather than deferred. What's left is two nits, both non-blocking.
Verification performed
| Check | Result |
|---|---|
pnpm install && pnpm build |
pass |
pnpm lint |
pass |
pnpm typecheck |
pass |
pnpm test |
pass — 28 suites green. Last round's runtime-claude/spawn.test.ts timing flake did not reproduce. |
TC-19 Codex helper (e2e/host-dynamic-tool-e2e.mjs, repointed at the local dist) |
host_dynamic_tool_e2e=pass |
pnpm e2e:claude |
not run — no Docker daemon in this environment (/var/run/docker.sock absent). Environment limit, not a PR defect; the Container Smoke and Test checks are green on 5daad36. |
Smoke: .env-sourced credential, Codex path (was P1-1) |
pass — Bearer dotenv-sourced-token reached the provider with nothing exported in process.env |
Smoke: github_graphql under SYMPHONY_TRACKER_KIND=linear (was P1-2) |
pass — both tools resolve; requests hit api.github.com and api.linear.app respectively |
| Smoke: unknown tool + allowlist gating | pass — unknown_tool for an unowned name, and for an owned name excluded from allowedToolNames |
| Smoke: Claude MCP transport, same scenarios | pass — tools/list → [github_graphql, linear_graphql], both additionalProperties: false; tools/call used the .env-sourced host credential; unknown tool → -32602 |
Smoke testing again ran locally against the built packages through the real adapters with only the provider HTTP boundary stubbed — the same boundary TC-19 stubs. No preview environment on this PR.
Round 1 findings
| # | Finding | Status |
|---|---|---|
| P1-1 | .env-sourced credentials dropped |
Fixed — AgentToolExecutionContext.environment carries the resolved host env; both adapters read it. The Object.assign(process.env, env) in the e2e helper is gone, which was the check I asked for. Guarded: reverting tracker-github to process.env fails tracker-github.test.ts:39. |
| P1-2 | github_graphql broken under a Linear tracker |
Fixed — dispatch is by tool name across the advertised set, with the unknown_tool failure retained. See my inline note on the test guard. |
| P2-3 | Only the Codex path converged | Fixed — mcp-http-server.ts now drives agentToolSpecs()/executeAgentTool(); both transports hand out identical schemas, so the docs/trackers/*.md tables are true for both. |
| P2-4 | E2E had no transition or comment | Fixed — both halves now run query + scoped addComment + updateProjectV2ItemFieldValue, asserting the outgoing documents and variables. The mutation half of the contract has coverage behind it. |
| P2-5 | False statement in docs/trackers/github.md |
Fixed — states plainly that github_graphql is always advertised. |
| P3-6 | Default allowlist ignored an injected adapter | Fixed — both derive from the same resolved-or-injected set. |
| P3-7 | Stale dep + drifted dependency graphs | Fixed — tool-linear-graphql dropped from worker; docs/architecture.md:127,130,133 and CLAUDE.md updated, including the new runtime-claude → tracker-* edge. No cycle introduced. |
| nit-8 | Prettier drift | Fixed — pnpm format is clean. |
Also confirmed: TC-19's third assertion is now worded as existing coverage that this PR extends, which was the wording concern; the changeset is present; the branch merges into main cleanly (2 commits behind, no conflict).
Scope
Still no over-engineering — everything added stays inside #732's stated scope, and the round-1 items were fixed at the root rather than papered over. The context.environment extension is the right shape: it keeps the spec's context={issue} object form while removing the ambient dependency.
One thing deliberately not converged, which I agree with: runtime-codex/src/runtime.ts:180-252 still builds its own tool definitions, because a Codex RuntimeToolDefinition carries the MCP command/args/env that an AgentToolSpec has no notion of. The input schemas are identical to the adapter-owned ones today (same properties, required, additionalProperties: false), so no documented contract line is false — only the human-readable description differs between runtimes. That's #700's cleanup, not this PR's.
Remaining
Two nits, both inline, both fine as follow-ups: a missing unit-test guard on the Codex-side adapter resolution, and the resolution rule being duplicated across two packages. Neither blocks merge, and neither is worth another round — approving.
Generated by Claude Code
| authorizationHeader: env.LINEAR_AUTHORIZATION, | ||
| }; | ||
| ): readonly OrchestratorTrackerAdapter[] { | ||
| return env.SYMPHONY_TRACKER_KIND === "linear" |
There was a problem hiding this comment.
nit — the P1-2 fix has no unit-test guard on this function (non-blocking).
The routing fix itself is correct; I reproduced it working. But resolveTrackerToolAdapters — the function whose single-adapter form was the regression — is never exercised under SYMPHONY_TRACKER_KIND=linear by any test. codex-dynamic-tools.test.ts:125 ("routes GitHub tools alongside Linear tools") injects dependencies.adapters stubs, so it validates executeCodexDynamicToolCall's dispatch but not this resolution; :94 uses real resolution but only under the default GitHub kind.
Verified by reverting this function to the buggy form on top of your branch:
return env.SYMPHONY_TRACKER_KIND === "linear"
- ? [githubProjectTrackerAdapter, linearTrackerAdapter]
+ ? [linearTrackerAdapter]
: [githubProjectTrackerAdapter];pnpm --filter @gh-symphony/worker test
Test Files 26 passed (26)
Tests 194 passed (194)
The whole worker suite stays green while github_graphql is broken for every Linear-tracked project again.
The Claude side is already guarded — the same revert in mcp-http-server.ts fails mcp-http-server.test.ts:41, because that test drives real resolution through env alone. A one-line equivalent here (call executeCodexDynamicToolCall with SYMPHONY_TRACKER_KIND=linear and no injected dependencies, asserting linear_graphql resolves) would close the asymmetry. Worth doing, but not worth another round on its own.
Generated by Claude Code
| authorizationHeader: env.LINEAR_AUTHORIZATION, | ||
| }; | ||
|
|
||
| function resolveHostToolAdapters( |
There was a problem hiding this comment.
nit — "which adapters are enabled for this session" is now duplicated across two packages (non-blocking).
resolveHostToolAdapters here and resolveTrackerToolAdapters in packages/worker/src/codex-dynamic-tools.ts:78-84 are byte-identical. That's exactly the divergence class P2-3 was about: this PR made both transports share the tool contract, but the session's enabled-adapter rule stayed a per-transport copy, so a future tracker kind (or a change to the Linear condition) has to be applied in two places or the two runtimes silently advertise different tool sets.
Cheap to close — hoist one resolveHostToolAdapters(env) into a shared module and import it from both. Follow-up is fine; the two copies agree today and both are covered.
Generated by Claude Code
5daad36 to
b6c2e7c
Compare
2 participants
Issues — Closed #732
TL;DR
Formalizes adapter-owned host-side tracker tools for Codex and Claude, preserving loaded host configuration and executing each advertised tool through its owning tracker adapter.
Change-point diagram
Start here
packages/core/src/contracts/tracker-adapter.tsdefines the shared tool specification and execution context.packages/worker/src/codex-dynamic-tools.tsroutes each advertised tool to its owning adapter.packages/runtime-claude/src/mcp-http-server.tsconsumes the same adapter-owned tool contract.e2e/scenarios/19-host-side-tracker-tools.mddocuments the Docker acceptance scenario.Changes
agentToolSpecs()andexecuteAgentTool()to GitHub and Linear adapters, using normalized issue context and the loaded host environment.mainpreserved the new TC-19 table row with the current repository regression matrix.Evidence
pnpm lint— pass.pnpm test— pass.pnpm typecheck— pass.pnpm build— pass.docker compose --project-name symphony-732-rework -f docker-compose.e2e.yml exec -T symphony-e2e node /app/e2e/host-dynamic-tool-e2e.mjs— pass.pnpm e2e:claude— pass (5 Docker tests).Risks & rollback
The shared contract is now the host-tool execution boundary. Reverting this PR restores the previous routing without data migration or tracker-state changes.
Changed files
Core contract; GitHub/Linear adapters; Codex/Claude transports; unit and Docker E2E tests; tracker documentation; ADR; architecture/test guidance; CLI changeset.
Post-merge / human validation