-
Notifications
You must be signed in to change notification settings - Fork 3
feat(tools,worker): finalize host tool contracts #775
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 7 commits
f013525
e51163a
2c0a372
189dbf7
002e911
b78e3b2
04025b8
49eae3f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@gh-symphony/cli": patch | ||
| --- | ||
|
|
||
| Run tracker tools through host-owned runtime integrations with normalized issue context and frozen per-session contracts for #673. |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -13,6 +13,54 @@ afterEach(async () => { | |||||||||
| }); | ||||||||||
|
|
||||||||||
| describe("Claude host MCP HTTP server", () => { | ||||||||||
| it("freezes the advertised tool specs when the server starts", async () => { | ||||||||||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
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 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 Non-blocking — the behavior is right, this is only about what a future regression would trip over. Generated by Claude Code
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||||
| let specs = [ | ||||||||||
| { | ||||||||||
| name: "snapshotted_tool", | ||||||||||
| description: "Initial tool", | ||||||||||
| inputSchema: { | ||||||||||
| type: "object" as const, | ||||||||||
| properties: {}, | ||||||||||
| required: [], | ||||||||||
| additionalProperties: false, | ||||||||||
| }, | ||||||||||
| }, | ||||||||||
| ]; | ||||||||||
| const adapter = { | ||||||||||
| agentToolSpecs: () => specs, | ||||||||||
| executeAgentTool: vi.fn(), | ||||||||||
| }; | ||||||||||
| server = await startClaudeMcpHttpServer({ | ||||||||||
| env: {}, | ||||||||||
| context: { | ||||||||||
| issue: { id: "issue-1", identifier: "owner/repo#1", nativeRef: {} }, | ||||||||||
| }, | ||||||||||
| adapters: [adapter], | ||||||||||
| }); | ||||||||||
| specs = [ | ||||||||||
| { | ||||||||||
| ...specs[0]!, | ||||||||||
| name: "reloaded_tool", | ||||||||||
| description: "Reloaded tool", | ||||||||||
| }, | ||||||||||
| ]; | ||||||||||
|
|
||||||||||
| const response = await fetch(server.url, { | ||||||||||
| method: "POST", | ||||||||||
| headers: { | ||||||||||
| authorization: `Bearer ${server.sessionToken}`, | ||||||||||
| "content-type": "application/json", | ||||||||||
| }, | ||||||||||
| body: JSON.stringify({ jsonrpc: "2.0", id: 1, method: "tools/list" }), | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| await expect(response.json()).resolves.toMatchObject({ | ||||||||||
| result: { | ||||||||||
| tools: [expect.objectContaining({ name: "snapshotted_tool" })], | ||||||||||
| }, | ||||||||||
| }); | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| it("requires its session capability and exposes only the selected host tool", async () => { | ||||||||||
| server = await startClaudeMcpHttpServer({ | ||||||||||
| env: { SYMPHONY_TRACKER_KIND: "github" }, | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -300,6 +300,36 @@ describe("resolveGitHubGraphQLToken", () => { | |
| }); | ||
|
|
||
| describe("executeGitHubGraphQL", () => { | ||
| it("executes a repository query while carrying host-side issue context", async () => { | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3 — the test's name promises more than its assertions deliver (same for the Linear twin at This is a genuine improvement on what it replaced — it uses a realistic The invariant worth pinning is the one the contract states in prose and nothing tests:
I verified it holds today — a repo query under a full GitHub 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved in |
||
| const fetchImpl = vi.fn().mockResolvedValue( | ||
| new Response(JSON.stringify({ data: { viewer: { login: "octo" } } }), { | ||
| status: 200, | ||
| }) | ||
| ); | ||
|
|
||
| await expect( | ||
| executeGitHubGraphQL( | ||
| { query: "query Viewer { viewer { login } }" }, | ||
| { token: "ghs_static" }, | ||
| fetchImpl as typeof fetch, | ||
| { | ||
| issue: { | ||
| id: "issue-1", | ||
| identifier: "owner/repo#1", | ||
| nativeRef: { | ||
| itemId: "project-item-1", | ||
| contentType: "Issue", | ||
| sourceState: "OPEN", | ||
| linkedPullRequests: [], | ||
| linkedPullRequestsTruncated: false, | ||
| }, | ||
| }, | ||
| } | ||
| ) | ||
| ).resolves.toEqual({ data: { viewer: { login: "octo" } } }); | ||
| expect(fetchImpl).toHaveBeenCalledOnce(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| githubGraphQLRateLimitPolicy.reset(); | ||
| }); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2 — this line still claims the enforcement that
b78e3b2removedThis is the last surviving instance of the round-1 finding #4 defect.
README.mdand 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 thatissue.id/issue.identifierare 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/*.mdnow say: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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved in
49eae3fe.docs/architecture.mdnow 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.