Skip to content

fix(cli): let a real filesystem error be itself, not "no extension pin" - #221

Merged
chrischall merged 1 commit into
mainfrom
claude/clearpin-errors
Aug 5, 2026
Merged

fix(cli): let a real filesystem error be itself, not "no extension pin"#221
chrischall merged 1 commit into
mainfrom
claude/clearpin-errors

Conversation

@chrischall

Copy link
Copy Markdown
Owner

The one nit from #220, on a fresh branch off main — #219 merged while I was writing the fix.

clearPin reached its stem fallback by catching any throw from clearExtensionPin, on the assumption that a throw meant "that wasn't a legal server name". It also catches the failures that matter. An EACCES on the identity directory came back as:

no extension pin for @fetchproxy/example-mcp

— and because the stem lookup can never match a slash-form name, that is the message every scoped MCP gets for a permissions problem. It sends the reader looking for a file that is sitting right there, at the moment they are already locked out by a pin.

It now asks first whether the argument is a legal server name (safeIdentityFileBase, newly exported from @fetchproxy/server for it) and only falls back to the stem when it is not. Everything else propagates as itself.

Testing

The new test chmods the identity directory to 0500 and asserts the EACCES surfaces. I checked it discriminates rather than assuming: against the previous implementation it fails with exactly the misleading message above —

AssertionError: expected [Function] to throw error matching /EACCES|EPERM/
  but got 'no extension pin for @fetchproxy/example-mcp'

Skipped when running as root, where the directory mode is advisory.

1233 tests, three consecutive clean runs, tsc -b across all six workspaces.

Closes #220

#220, from the review of #219.

`clearPin` reached its stem fallback by catching ANY throw from
`clearExtensionPin`, on the assumption that a throw meant "that wasn't a legal
server name". It also catches the failures that matter: an EACCES on the
identity directory came back as `no extension pin for @scope/name` — and since
the stem lookup can never match a slash-form name, that is the message every
scoped MCP would get for a permissions problem. It sends the reader looking for
a file that is sitting right there.

It now asks first whether the argument is a legal server name
(`safeIdentityFileBase`, newly exported for it) and only falls back to the stem
when it is not. Everything else propagates as itself.

The test chmods the identity directory to 0500 and asserts the EACCES surfaces;
against the previous implementation it fails with the exact misleading message
above. Skipped when running as root, where the mode is advisory.

Closes #220

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@chrischall chrischall added the bug Something isn't working label Aug 5, 2026
@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 48s —— View job


Code Review: fix(cli): let a real filesystem error be itself, not "no extension pin"

Summary

This PR fixes a bug where clearPin masked real filesystem errors (like EACCES) as "no extension pin" messages. The fix properly validates whether an argument is a legal server name before attempting filesystem operations, so genuine errors propagate instead of being caught and misinterpreted.

Changes Analysis

Logic change (trust.ts:109-130):

  • Validates argument with safeIdentityFileBase() first to determine if it's a legal server name
  • Only falls back to stem lookup if the argument is NOT a legal server name
  • Prevents the catch-all that was swallowing EACCES/EPERM errors from directory operations
  • Correct behavior: scoped names like @fetchproxy/example-mcp are valid per the SAFE_SCOPED regex and proceed directly to clearExtensionPin()

Export (server/index.ts:72):

  • safeIdentityFileBase is properly exported, following the convention that CLI tools can access validation helpers

Test coverage (trust.test.ts:163-181):

  • Tests the exact bug: chmod directory to 0o500 and verify EACCES surfaces instead of misleading "no extension pin" message
  • Correctly skips under root (where file mode is advisory)
  • Cleans up permissions in finally block
  • Test discriminates: confirms it fails with the old code (per PR description: "but got 'no extension pin for @fetchproxy/example-mcp'")

Validation

  • safeIdentityFileBase() validation is sound: rejects empty, "..", traversal attempts, and anything outside SAFE_PLAIN/SAFE_SCOPED regexes
  • Scoped MCPs work correctly: @fetchproxy/example-mcp matches SAFE_SCOPED ✓
  • Ambiguous stems work correctly: @my_org_tool-mcp (no /) doesn't match either regex, falls back to stem lookup ✓
  • Filesystem errors now propagate for the server-name path; only the stem-lookup path catches and translates to "no extension pin" ✓

Testing

The PR description reports: 1233 tests, three consecutive clean runs, tsc across all six workspaces. Existing tests for scoped/ambiguous stem cases (lines 122–161) remain passing and interact correctly with this fix.


Verdict: pass

No issues found. The fix correctly distinguishes between "not a valid server name" and "filesystem error," and the test validates the discriminator works as intended.
| Branch link

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

✅ Auto-review verdict: pass — The fix correctly validates server names before filesystem operations, preventing real errors from being masked as "no extension pin" messages. Logic, exports, and test coverage are all sound.

@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:57
@chrischall
chrischall merged commit c87a864 into main Aug 5, 2026
16 checks passed
@chrischall
chrischall deleted the claude/clearpin-errors branch August 5, 2026 23:58
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 bug Something isn't working 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 #219: fix(server): release only our own extension claim, and stop guessing scoped names

1 participant