-
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 all 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 |
|---|---|---|
|
|
@@ -300,6 +300,40 @@ 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(); | ||
| const body = JSON.parse( | ||
| String(fetchImpl.mock.calls[0]![1]!.body) | ||
| ) as Record<string, unknown>; | ||
| expect(Object.keys(body).sort()).toEqual(["query"]); | ||
| }); | ||
|
|
||
| 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.
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:
tools/listignores a reloaded spec listtools/callstays bound to the snapshot entrystructuredClonedefeats in-place spec mutationThe 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/liststill looks frozen:For the third: because the test reassigns
specsrather than mutating it, it passes with or withoutstructuredClone— so the deep copy is currently unverified. Mutatingspecs[0]!.namein 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.
Choose a reason for hiding this comment
The 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.