Skip to content

refactor(extension-core): stop exporting background helpers nothing imports - #229

Merged
chrischall merged 1 commit into
mainfrom
test/background-split-followups
Aug 9, 2026
Merged

refactor(extension-core): stop exporting background helpers nothing imports#229
chrischall merged 1 commit into
mainfrom
test/background-split-followups

Conversation

@chrischall

Copy link
Copy Markdown
Owner

Addresses the first nit from the #226 auto-review follow-ups (#227).

Nit 1 — seven symbols exported but referenced only within their own module — FIXED

drop export on hello.ts:sameDomainSet, DEFAULT_CAPABILITIES, effectiveCapabilities, DeclaredScope, declaredScope, badge.ts:syncBadge, pending-records.ts:PendingRecordBase

Verified real before changing anything. A whole-tree git grep over origin/main (tracked files only, so node_modules/dist are out of scope, docs are in) shows every one of the seven referenced only inside its own defining module. The apparent cross-file hits are false positives: declaredScope/DeclaredScope in tests/background.test.ts and server-hello.ts are local const declaredScope / const declaredScopeForHash identifiers, not imports. Both barrels (src/background.ts, src/index.ts) were read directly — they re-export handleServerHello / HandleHelloDeps / HandleHelloResult, applyNeedsPairRecord / AnyPendingRecord and the session-scope symbols, and none of the seven.

The sibling exports deliberately stay: badge.ts's four transitions (setConnectionStatus, flashActivity, setPairPendingBadge, clearPairPendingBadge) are imported by socket.ts / server-hello.ts / approval.ts / boot.ts, and PendingPairRecord / PendingScopeUpdateRecord are imported by server-hello.ts. Only the over-exported symbols were narrowed.

No TS4020. PendingRecordBase does not need export to remain reachable as the extends base of two exported interfaces. Confirmed from the emitted declaration rather than assumed:

$ grep -n "interface\|export" packages/extension-core/dist/background/pending-records.d.ts
16:interface PendingRecordBase {
37:export interface PendingPairRecord extends PendingRecordBase {
93:export interface PendingScopeUpdateRecord extends PendingRecordBase {
128:export type AnyPendingRecord = PendingPairRecord | PendingScopeUpdateRecord;
144:export {};

$ grep -c "DeclaredScope" packages/extension-core/dist/background/hello.d.ts
0

Docblock correction. pending-records.ts asserted that PendingRecordBase was "exported here because background.ts still constructs both record kinds." Two things wrong with that: the constructor is server-hello.ts (lines 139 and 214), not background.ts, and the export was not needed for it either way. Rewritten to say what is actually true.

Regression guard

New tests/background-module-surface.test.ts pins all three surfaces. It guards both halves:

  • Value exports via the ES module namespace (Object.keys(module)).
  • Type-only exports via the module source text. Interfaces and type aliases are erased at runtime so they never appear in the namespace, and this package's tsconfig.json sets include: ["src/**/*"]tsc -b never reads tests/, so a stray export interface cannot be caught by a type error either. Without the source-text half, DeclaredScope and PendingRecordBase would have had no automated guard at all.

TDD — the guard was written first and run against the unfixed tree (git stash of the three src/ files, restored after):

$ npx vitest run packages/extension-core/tests/background-module-surface.test.ts   # src at db30f4d
 FAIL  ... > hello.ts exports only the decision function its callers import
+   "DEFAULT_CAPABILITIES", +   "declaredScope", +   "effectiveCapabilities", +   "sameDomainSet"
 FAIL  ... > hello.ts exports only the two types its callers name
+   "DeclaredScope"
 FAIL  ... > badge.ts exports only the four badge transitions its callers import
+   "syncBadge"
 FAIL  ... > pending-records.ts keeps PendingRecordBase file-local
+   "PendingRecordBase"

 Test Files  1 failed (1)
      Tests  4 failed | 1 passed (5)

All four fail without the fix; all five pass with it.

Nit 2 — session-scope-teardown.test.ts ALL_SCOPE_MAPSDEFERRED

derive the list from the module's exports to make the "must fail here" comment true

Not touched by this PR. Its checkbox stays unchecked on #227, which therefore stays open — no Closes.

Verification

Run in CI's order (build-command: npm run build, then test-command: npm test), all after the final edit:

$ npm run build
npm run build exit=0

$ npm test
 Test Files  92 passed (92)
      Tests  1240 passed (1240)          # 1235 on main + 5 new

$ (cd packages/extension-core && npx tsc -b --force)
tsc -b --force exit=0

$ (cd packages/extension-chrome && npx tsx build.ts)
extension-chrome built → packages/extension-chrome/dist
build.ts exit=0

Refs #227

…mports

The background split (#226) carved `background.ts` into purpose-shaped
modules. Seven symbols that were file-private before the split picked up an
`export` on the way out even though nothing outside their own module
references them.

Drop `export` from `hello.ts:sameDomainSet`, `DEFAULT_CAPABILITIES`,
`effectiveCapabilities`, `DeclaredScope`, `declaredScope`;
`badge.ts:syncBadge`; and `pending-records.ts:PendingRecordBase`. The
sibling exports stay: badge.ts's four transitions are imported by
socket.ts / server-hello.ts / approval.ts / boot.ts, and
`PendingPairRecord` / `PendingScopeUpdateRecord` are imported by
server-hello.ts. `PendingRecordBase` needs no `export` to stay reachable as
the `extends` base of those two — the emitted `.d.ts` carries it unexported
alongside `export {}`, so no TS4020.

Also correct the pending-records.ts docblock, which claimed
`PendingRecordBase` was "exported here because `background.ts` still
constructs both record kinds": the constructor is `server-hello.ts`, not
`background.ts`, and the export is not needed either way.

The new tests/background-module-surface.test.ts guards both halves of each
surface. Value exports are asserted through the ES module namespace; the
type-only symbols are erased at runtime and this package's tsconfig includes
only `src`, so `tsc -b` never reads `tests/` and a stray `export interface`
would go uncaught — those are asserted against the module source text.

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

claude Bot commented Aug 9, 2026

Copy link
Copy Markdown

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


Review complete

  • Read CLAUDE.md for conventions
  • Read PR diff (git diff origin/main...HEAD)
  • Review each changed file
  • Verify claims made in PR description
  • Post findings and verdict

Summary

This PR drops export from seven symbols in hello.ts, badge.ts, and pending-records.ts that are only referenced within their own defining module, and adds a regression test (background-module-surface.test.ts) that pins each module's exported surface (both value exports via the namespace object and type-only exports via source-text matching, since tsc -b doesn't type-check tests/ per this package's tsconfig.json).

I independently re-verified the PR's central claims rather than taking them at face value:

  • No cross-file references exist for any of the seven narrowed symbols. Ran git grep -n -w <symbol> across the full tracked tree for sameDomainSet, DEFAULT_CAPABILITIES, effectiveCapabilities, DeclaredScope, declaredScope, syncBadge, and PendingRecordBase. Every hit is inside the symbol's own defining file, its own doc comment, or (for declaredScope) a false-positive local const declaredScope in tests/background.test.ts:890 and a prose mention in docs/superpowers/specs/2026-06-03-capture-headers-host-path-design.md:75 — exactly as the PR description claims.
  • Neither barrel re-exports any of the seven. packages/extension-core/src/background.ts re-exports handleServerHello/HandleHelloDeps/HandleHelloResult, ChromeCookie, applyNeedsPairRecord/AnyPendingRecord, and several handler-level symbols — none of the seven. src/index.ts only re-exports handleServerHello and its types from background.js.
  • The rewritten docblock in pending-records.ts is accurate. It now attributes construction of both pending-record kinds to server-hello.ts; confirmed PendingScopeUpdateRecord is constructed at server-hello.ts:139 and PendingPairRecord at server-hello.ts:214.
  • The new test's expected export lists match the post-diff source for all four describe blocks (hello.ts value/type exports, badge.ts, pending-records.ts).

No correctness, convention, or test-coverage issues found. This is a mechanical, low-risk visibility narrowing with a well-targeted regression test that guards against re-widening (including the type-only exports that tsc -b alone wouldn't catch, per this package's include: ["src/**/*"]).

Note: I was unable to execute npm run build / npm test / npx tsc -b in this sandbox (commands required approval that wasn't available), so I could not independently reproduce the PR description's reported 92 passed / 1240 passed and tsc -b --force exit=0 results. All findings above are grounded in direct source inspection and git grep, not test execution — flagging this as an open question rather than a blocking concern, since the PR description already includes the CI-order verification output.

Verdict: pass

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

✅ Auto-review verdict: pass — Mechanical, low-risk export-narrowing PR; all claims (no cross-file references, no barrel re-exports, accurate docblock, matching regression test) were independently verified via git grep and diff inspection. No correctness or convention issues found.

@chrischall chrischall added the ready-to-merge Owner has reviewed; arm auto-merge to land when CI is green label Aug 9, 2026
@chrischall
chrischall enabled auto-merge (squash) August 9, 2026 14:42
@chrischall
chrischall merged commit 42c39a1 into main Aug 9, 2026
11 checks passed
@chrischall
chrischall deleted the test/background-split-followups branch August 9, 2026 14:42
chrischall added a commit that referenced this pull request Aug 9, 2026
…ally fail (#230)

Closes #227 — the second nit, which #229 deliberately deferred.

## The test promised something it did not do

`ALL_SCOPE_MAPS` carried this comment:

> adding a thirteenth map without adding it to `clearAllSessionScopes`
must fail here

It was a hand-maintained literal, so a thirteenth map forgotten in
`clearAllSessionScopes` would be forgotten *here* too. The suite stays
green and the leak ships.

**Proven before changing anything.** Added an uncleaned `mcpThirteenth`
to `session-scope.ts`, omitted from `clearAllSessionScopes` and from the
literal:

```
Tests  2 passed (2)      ← the claim is false
```

After deriving the list from the module namespace, the same probe:

```
× empties every per-mcpId scope table
× has exactly the scope tables it was last reviewed with
AssertionError: mcpThirteenth survived teardown: expected 1 to be +0
```

Probe removed, suite green again.

## Why this one matters more than a usual test nit

A scope map surviving a WS teardown means **the next connection inherits
the previous session's grant**. "No scope map is ever missed" is the
entire guarantee this file exists for, and a hand-maintained list cannot
make it — it fails in exactly the case it was written to catch.

## On the count assertion

`toHaveLength(12)` stays, renamed and documented as what it actually is:
a deliberate tripwire. The derived test catches a map teardown
*forgets*; the count fires on any new scope map at all, cleaned or not,
so adding one can't be an incidental edit. Bumping that number is the
moment to ask whether the new table also needs seeding, redaction, or a
popup surface.

## Verification

```
npm test                                    92 files, 1240 passed (1240)
packages/extension-core   npx tsc -b        exit 0
packages/extension-chrome npx tsx build.ts  built
```

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01Sb9i5qyBwXYP8EkxdQfySu
chrischall added a commit that referenced this pull request Aug 9, 2026
🤖 I have created a release *beep* *boop*
---


##
[2.1.0](v2.0.0...v2.1.0)
(2026-08-09)


### Features

* **extension:** dial configured remote bridges alongside loopback
([#233](#233))
([cc81e8a](cc81e8a))
* **server:** fall back to FETCHPROXY_WS_PORT for the concentrator port
([#231](#231))
([c221262](c221262))


### Bug Fixes

* **extension:** give a refused hello its binding back, and show each
bridge's own state
([#236](#236))
([1f8da11](1f8da11)),
closes [#234](#234)
* **extension:** refuse download over a remote bridge
([#235](#235))
([306d1f5](306d1f5))


### Refactor

* **extension-core:** split background.ts into purpose-shaped modules
([#226](#226))
([db30f4d](db30f4d)),
closes [#10](#10)
* **extension-core:** stop exporting background helpers nothing imports
([#229](#229))
([42c39a1](42c39a1)),
closes [#227](#227)

---
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.

1 participant