Skip to content

fix(graphql): address all four tracked nits from PR #178's auto-review - #180

Merged
chrischall merged 1 commit into
mainfrom
claude/graphql-nits-followup
Jul 29, 2026
Merged

fix(graphql): address all four tracked nits from PR #178's auto-review#180
chrischall merged 1 commit into
mainfrom
claude/graphql-nits-followup

Conversation

@chrischall

Copy link
Copy Markdown
Owner

Summary

Follow-up to #178 (merged) — resolves every item left open in the auto-review-followup issue.

  1. openEncryptedFrameDetailed's fromB64 calls sat outside every try — an iv like "A" passes BASE64_RE (which doesn't enforce length % 4) but still makes atob throw, out of a function documented as "never throws". Moved both calls inside the decrypt try, and reduced openEncryptedFrame to a thin rethrow-wrapper over openEncryptedFrameDetailed so the decrypt/parse/validate sequence exists in exactly one place.
  2. content.ts's spoofed-graphql-res guard covered null/undefined but not arrays/primitives that assertObject also rejects — a spoofed data:[] or data:"x" from a same-window page script still reached assertObject(raw.data) and closed the bridge. Widened the guard to match assertObject's own object/array/null check, with parametrized tests over null, undefined, array, string, number, boolean.
  3. capture-logger.ts's error string was OpenTable-specific ("open a restaurant page and retry") — the only vendor-specific string in extension-core's runtime output, relayed verbatim to every MCP using the graphql capability. Made generic.
  4. graphql-query.test.ts only exercised ok:true — the actual bug the auto-review found (op:'graphql_query' failing validateInnerResponse) could only ever show up on the ok:false path, so this suite could not have caught it. Added an end-to-end case driving the real WS + encryption + validation + dispatch stack, asserting graphqlQuery() rejects, the extension WebSocket stays open, and a second call over the same connection still succeeds.

Closes #179

Test plan

  • npm test — 1064/1064 passing
  • npm run build — clean across all 7 workspaces
  • npm run typecheck — clean
  • No version bumps

🤖 Generated with Claude Code

Follow-up to #178 (merged) — resolves every item left open in #179.

1. openEncryptedFrameDetailed's two fromB64 calls sat outside every try,
   so an iv like "A" (passes BASE64_RE, which doesn't enforce length % 4)
   made atob throw out of a function documented as "never throws", with
   no stage assigned. Moved both calls inside the decrypt try, and
   reduced openEncryptedFrame to a thin rethrow-wrapper over
   openEncryptedFrameDetailed so the decrypt/parse/validate sequence
   exists in exactly one place.

2. content.ts's spoofed-graphql-res guard covered null/undefined but not
   the arrays and non-object primitives assertObject also rejects — a
   spoofed data:[] or data:"x" from a same-window page script still
   reached assertObject(raw.data) and closed the bridge. Widened the
   guard to match assertObject's own object/array/null check, with
   parametrized tests covering null, undefined, array, string, number,
   and boolean.

3. capture-logger.ts's "operation ... not yet observed on this tab" error
   said "open a restaurant page and retry" — the only OpenTable-specific
   string in extension-core's runtime output, relayed verbatim to every
   MCP using the graphql capability regardless of vendor. Made it
   generic: "open a page on the site that triggers this GraphQL
   operation, then retry".

4. graphql-query.test.ts's integration suite only exercised ok:true —
   the actual bug the auto-review found (op:'graphql_query' failing
   validateInnerResponse) could only ever show up on the ok:false path,
   so this suite could not have caught it. Added an end-to-end case that
   drives the real WS + encryption + validation + dispatch stack with an
   ok:false response, asserting graphqlQuery() rejects, the extension
   WebSocket stays open, AND a second call over the same connection
   still succeeds — proving the bridge survives, not just doesn't
   immediately close.

Closes #179

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions github-actions Bot added the auto-review Trigger Claude + Copilot review on this PR label Jul 29, 2026
@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

Claude finished @chrischall's task in 1m 28s —— View job


Reviewing PR #180

  • Read CLAUDE.md conventions
  • Read PR diff (git diff origin/main...HEAD)
  • Review each changed file
  • Post findings

Review

This is a clean, well-scoped follow-up that resolves all four items from #178's auto-review-followup issue (#179). Went through each change against the diff:

  1. packages/protocol/src/seal.tsopenEncryptedFrameDetailed now moves both fromB64 calls inside the try, so a structurally-valid-but-undecodable base64 IV (e.g. "A", which passes BASE64_RE since it doesn't enforce length % 4) resolves to { stage: 'decrypt-failed' } instead of an uncaught throw. openEncryptedFrame is correctly reduced to a thin wrapper — verified the callers in background.ts/host.ts and all integration tests only do generic try/catch/.rejects.toThrow() on it, so the rethrow-wrapping (result.error instanceof Error ? result.error : new Error(...)) doesn't break any caller that depended on the old direct-throw shape.
  2. packages/extension-core/src/content.ts — the graphql-res spoofing guard now matches assertObject's exact object/array/null check (packages/protocol/src/validate.ts:57-64: typeof x !== 'object' || x === null || Array.isArray(x)). Confirmed the mirrored check in content.ts is equivalent. New parametrized test (it.each over null/undefined/array/string/number/boolean) gives solid coverage of the previously-unguarded shapes.
  3. packages/extension-core/src/capture-logger.ts — the OpenTable-specific "open a restaurant page and retry" string is gone; confirmed no other file in the tree still references it (grep -r "restaurant page" → no matches), so the test-file updates in validate.test.ts and content-graphql.test.ts are consistent with the source change.
  4. packages/server/tests/integration/graphql-query.test.ts — new end-to-end case correctly drives the real WS/encryption/validation/dispatch stack, asserts the call rejects, the socket stays open, and a second call over the same connection succeeds with outboundSeq correctly continuing (not reset) to respect the strictly-increasing inbound-seq requirement.

No correctness, security, or convention issues found. Test/doc changes are consistent with the source changes throughout, and the PR's own claim of 1064/1064 passing lines up with what's in the diff.

Verdict: pass

@github-actions

Copy link
Copy Markdown
Contributor

✅ Auto-review verdict: pass — PR correctly and cleanly resolves all four tracked nits from #178's auto-review, with solid test coverage and no correctness/security/convention issues found.

@chrischall chrischall added the ready-to-merge Owner has reviewed; arm auto-merge to land when CI is green label Jul 29, 2026
@chrischall
chrischall enabled auto-merge (squash) July 29, 2026 22:29
@chrischall
chrischall merged commit 9d88ac9 into main Jul 29, 2026
11 checks passed
@chrischall
chrischall deleted the claude/graphql-nits-followup branch July 29, 2026 22:30
chrischall added a commit that referenced this pull request Jul 29, 2026
🤖 I have created a release *beep* *boop*
---


##
[1.7.0](v1.6.2...v1.7.0)
(2026-07-29)


### Features

* **graphql:** route declared GraphQL ops through the tab's own Apollo
client ([#178](#178))
([0c3fdf4](0c3fdf4))


### Bug Fixes

* **extension-chrome:** build content scripts as classic IIFE so Chrome
injects them
([#175](#175))
([f4a3728](f4a3728))
* **graphql:** address all four tracked nits from PR
[#178](#178
auto-review
([#180](#180))
([9d88ac9](9d88ac9))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-review Trigger Claude + Copilot review on this PR ready-to-merge Owner has reviewed; arm auto-merge to land when CI is green

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Auto-review follow-ups for PR #178: feat(graphql): route declared GraphQL ops through the tab's own Apollo client

1 participant