Skip to content

fix(extension): use the guarded caps local for the cookie heading - #217

Merged
chrischall merged 1 commit into
mainfrom
fix/popup-caps-guard
Aug 5, 2026
Merged

fix(extension): use the guarded caps local for the cookie heading#217
chrischall merged 1 commit into
mainfrom
fix/popup-caps-guard

Conversation

@chrischall

Copy link
Copy Markdown
Owner

Closes #216.

It's a crash, not a consistency nit

caps is the defensively-guarded form — it falls back to ['fetch'] because callers may omit capabilities. My cookie heading read pending.capabilities directly, so an omitted field throws:

TypeError: Cannot read properties of undefined (reading 'includes')

That throw is inside renderPopup, so the pair popup renders nothing. The one UI a user depends on to see what they're approving fails closed into a blank panel — a worse outcome than the wrong heading that line was added to fix.

Filed as "inconsistent with the established defensive pattern", which is accurate but undersells it. Verified with a test before fixing: renders a pending-pair state with capabilities omitted, asserts no throw, and asserts the names still appear under "Read cookies" — the fallback is ['fetch'], which grants no cookie access at all, so the read heading is correct in that state.

The pattern in these three findings

All three review rounds since write_cookies merged found the same class of mistake: code that was right for the case I had in mind and wrong for the case the surrounding code had already handled. The guard was six lines above the line I added. So was the doc block I displaced, and the sibling pending* maps before that.

The correction isn't "be more careful" — it's to read the enclosing function for existing conventions before adding a call, rather than after a reviewer points at one.

1195 tests pass.

🤖 Generated with Claude Code

https://claude.ai/code/session_012o2nXwu7tov6j7ciBEpigo

Reported as a consistency nit on #215; it is a crash. `caps` is the
defensively-guarded form — it falls back to ['fetch'] because callers may
omit `capabilities` — and the cookie heading read `pending.capabilities`
directly, so an omitted field throws:

  TypeError: Cannot read properties of undefined (reading 'includes')

That throws inside `renderPopup`, so the pair popup renders NOTHING. The
one UI the user depends on to see what they are approving fails closed
into a blank panel, which is a worse outcome than the wrong heading this
line was added to fix.

Covered by a test that renders a pending-pair state with `capabilities`
omitted and asserts no throw, plus that the names still appear under the
read heading — the fallback is ['fetch'], which grants no cookie access,
so "Read cookies" is the correct label in that state.

Closes #216

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012o2nXwu7tov6j7ciBEpigo
@github-actions github-actions Bot added the auto-review Trigger Claude + Copilot review on this PR label Aug 5, 2026
@claude

claude Bot commented Aug 5, 2026

Copy link
Copy Markdown

Claude finished @chrischall's task in 39s —— View job


PR Review: fix(extension): use the guarded caps local for the cookie heading

Status: ✅ Complete


Summary

This PR fixes a critical crash in the pair-approval popup. The bug occurs when an MCP omits the capabilities field—code at line 701 (now 705) directly accessed pending.capabilities.includes(), throwing TypeError: Cannot read properties of undefined inside renderPopup, which rendered the popup completely blank. This is worse than a wrong label because users can't see what they're approving.

The fix correctly switches to the defensively-guarded caps variable, which exists on line 672 for precisely this scenario and falls back to ['fetch'].

Findings

✅ No issues found.

The change is minimal, correct, and well-aligned with project conventions:

  1. Correctness: The fix uses caps (the guarded form) instead of pending.capabilities (unguarded). The caps variable was already introduced on lines 672–674 for exactly this defensive pattern, and the code already uses caps on line 678 for the capability list rendering. This reintroduces consistency.

  2. Documentation: The established defensive pattern is documented on lines 668–674 with clear comments explaining why callers may omit capabilities and why the fallback to ['fetch'] is necessary. The new comment (lines 701–704) explains the specific reason this code path must use caps: a throw inside renderPopup renders nothing at all.

  3. Test coverage: The new test 'falls back rather than throwing when capabilities is omitted' (lines 1067–1091) directly validates the bug scenario:

    • Creates a pending object with no capabilities field
    • Asserts renderPopup() does not throw
    • Verifies the cookie keys still render under "Read cookies" (correct, since fallback is ['fetch'], which grants no cookie access)
  4. Severity is reflected: The PR description correctly characterizes this as a crash, not just an inconsistency—the popup renders nothing when capabilities are omitted, a worse outcome than a wrong heading.


Verdict: pass
| PR branch

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

✅ Auto-review verdict: pass — The PR correctly fixes a critical crash in the pair-approval popup by using the defensively-guarded caps variable instead of directly accessing pending.capabilities. The fix is minimal, properly tested, and aligns with established defensive coding patterns.

@chrischall chrischall added the ready-to-merge Owner has reviewed; arm auto-merge to land when CI is green label Aug 5, 2026
@chrischall
chrischall enabled auto-merge (squash) August 5, 2026 23:19
@chrischall
chrischall merged commit f95c832 into main Aug 5, 2026
11 checks passed
@chrischall
chrischall deleted the fix/popup-caps-guard branch August 5, 2026 23:20
chrischall added a commit that referenced this pull request Aug 6, 2026
🤖 I have created a release *beep* *boop*
---


##
[2.0.0](v1.11.0...v2.0.0)
(2026-08-06)


### ⚠ BREAKING CHANGES

* **protocol:** bind the ephemeral key into the ready signature
([#222](#222))

### Features

* **protocol:** add write_cookies, the one verb that can repair a
rotated session
([#211](#211))
([b2557c2](b2557c2))
* **protocol:** bind the ephemeral key into the ready signature
([#222](#222))
([c13aeed](c13aeed))
* **server:** let a request name the tab that relays it
([#207](#207))
([c5d3f4d](c5d3f4d))
* **server:** pin the extension's identity, and verify it on the peer
path ([#213](#213))
([0eeced7](0eeced7))


### Bug Fixes

* **cli:** let a real filesystem error be itself, not "no extension pin"
([#221](#221))
([c87a864](c87a864)),
closes [#220](#220)
* **cli:** validate --via-tab before connecting, like the request URL
([#210](#210))
([959fcc5](959fcc5))
* **extension:** reattach the write_cookies doc block, and name the
writable cookies as writable
([#215](#215))
([2730c4a](2730c4a))
* **extension:** use the guarded caps local for the cookie heading
([#217](#217))
([f95c832](f95c832))
* **server:** release only our own extension claim, and stop guessing
scoped names
([#219](#219))
([3d90a64](3d90a64)),
closes [#218](#218)
* **server:** type no-tab rejections so they stop reading as version
mismatches ([#205](#205))
([dc30bd9](dc30bd9))


### Refactor

* **server:** drop the concatBytes imports the signature change orphaned
([#224](#224))
([4985ba7](4985ba7)),
closes [#223](#223)

---
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 #215: fix(extension): reattach the write_cookies doc block, and name the writable cookies as writable

1 participant