Copilot review on #2105: specification/v2_auth_mid_session.md still said
checkAuthChallengeSatisfied is satisfied for token_expired / unauthorized by
any valid non-expired token. Only token_expired can short-circuit, and only
for a provably unexpired JWT; invalid_token and unauthorized are always false
(which the code already did before this branch).
Signed-off-by: cliffhall <cliff@futurescale.com>
* refactor(web): sharpen src/lib vs src/utils rule and move misfiled modules (#1776)
Codify the empirical rule the web client already followed loosely — utils =
pure functions that compute; lib = things that instantiate, adapt, or touch
the environment — in AGENTS.md and clients/web/README.md, and move the modules
that sat on the wrong side of it.
Moves (source + co-located test):
- lib/sandbox-csp -> utils (pure CSP-string construction; type-only import)
- utils/browserTabVisibility -> lib (registers visibilitychange listeners)
- utils/oauthResume -> lib (reads/writes sessionStorage)
- utils/clearServerOAuthState -> lib (drives live InspectorClient / OAuthStorage)
Borderline audits (kept in place): pendingReauth (pure type declarations),
deepLink / correlateTransportErrors / mcpNetworkHeaders / oauthUx / oauthFlow /
oauthNetworkPhase (pure transforms / type-only core imports) stay in utils;
downloadFile stays whole in lib (dominant side is DOM side effects).
Also relocate the lone src/test/lib/downloadFile.test.ts straggler beside its
source at src/lib/downloadFile.test.ts, and document the test-placement rule
(side-by-side by default; src/test/ only for core/ tests, the integration
project, and shared scaffolding).
Pure move — no behavior change; no build/coverage config keys off the boundary.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
* refactor(web): address review — fix stale paths, move customHeaders, one-way arrows (#1776)
Responds to the @claude review on PR #1784:
1. Stale path references (fixed): update the four in-repo pointers left at
pre-move paths — specification/v2_auth_mid_session.md (oauthResume,
browserTabVisibility → lib/), and the two load-bearing sandbox security
comments (sandbox_proxy.html, sandbox-controller.ts → src/utils/sandbox-csp).
2. lib/types/customHeaders (moved): it's pure (types + pure transforms), the
rule's clearest counterexample. Moved lib/types/customHeaders.{ts,test.ts} →
utils/ (no external importers; only its own test) and removed the
undocumented lib/types/ subdir. Documented that utils/ is the home for pure
shared domain types + their constructors — no separate types/ bucket.
3. One-way arrows (item 3): declare OAuthResumeAuthKind in utils/pendingReauth
and re-export it from lib/oauthResume, so the only cross-directory arrow now
points lib → utils. Codified that invariant in AGENTS.md.
Wording nit: AGENTS.md + README now state diagnostic logging
(console.warn/error) doesn't count as a side effect for the utils rule.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
* docs: address review round 2 — stale customHeaders refs, rule wording (#1776)
Doc-only follow-up to the round-2 @claude review on PR #1784 (no code changed):
1. Stale paths (same class as round-1 item 1): the customHeaders move left
three references to the deleted lib/types/customHeaders.ts path in
specification/ (v2_ux_interfaces_phase_0_4_inventory.md,
v2_ux_interfaces_plan.md ×2) — updated to utils/customHeaders.ts.
2. customHeaders anchor: note in AGENTS.md that its shape is staged for
ServerSettingsForm (per v2_ux_interfaces_plan.md), so its zero-importer
state reads as intentional rather than rot; kept as the no-types/-bucket
illustration and led the anchor list with load-bearing modules.
3. Type-only-import clause: split the conflated clause — a type-only import
(pendingReauth) and re-exporting core's pure functions/constants
(oauthUx/oauthFlow) are distinct reasons to stay in utils; what makes a
module lib is wrapping core's stateful runtime. Fixed in both AGENTS.md
and the web README.
Cosmetic: re-padded the v2_auth_mid_session.md table row to column width.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
* docs: address review round 3 — scope the "no types/ bucket" claim (#1776)
Doc-only follow-up to the round-3 @claude review on PR #1784 (no code changed):
1. "no separate types/ bucket" overreached: clients/web/src/types/ does exist
(navigation.ts + react-syntax-highlighter .d.ts ambient stubs wired via
tsconfig.app.json paths). Scoped the claim in both AGENTS.md and the web
README to "no types/ sub-bucket INSIDE lib/utils", and noted src/types/ is
for ambient .d.ts stubs, not domain types.
2. README example list drifted from AGENTS.md's round-2 load-bearing anchors —
realigned it (inspectorTabs/mcpNetworkHeaders, dropped the zero-importer
customHeaders from the headline list).
3. Clarified v2_ux_interfaces_plan.md:185 so the v2 path reads as the copy's
destination, not as the v1.5 source location.
Follow-up filed as #1785 (board Todo): src/types/navigation.ts InspectorTab is
dead and duplicates utils/inspectorTabs.ts — kept out of this pure-move PR.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
* docs: address review round 4 — reconcile src/types/ claim with #1785 (#1776)
Doc-only follow-up to the round-4 @claude review on PR #1784 (no code changed):
The round-3 scope fix overshot: it asserted src/types/ holds "not domain types
at all", but the dead navigation.ts InspectorTab is a plain-.ts domain type
still living there, and specification/v2_ux_interfaces_plan.md:181-182 actively
prescribes placing InspectorTab there — so AGENTS.md and that spec doc gave
opposite guidance about the same directory.
- AGENTS.md + web README: softened to "not the place for new domain types"
and added a parenthetical that the one lingering domain type (dead
navigation.ts) is tracked for removal in #1785; until then a pure domain
type belongs in utils/, not src/types/.
- v2_ux_interfaces_plan.md:181: marked the tab-enum placement guidance as
superseded by #1785 (live type is InspectorTabId in utils/inspectorTabs.ts).
Self-heals on merge of #1785.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
* docs: address review round 5 — fix garbled #1785 clause, restructure utils rule (#1776)
Doc-only follow-up to the round-5 @claude review on PR #1784 (no code changed):
1. Grammar (item 1): the round-4 #1785 clause read "superseding [#1785] will
remove it" — a participle with no subject. Reworded to "the dead
navigation.ts InspectorTab, which #1785 will remove".
2. Readability (item 2): the src/utils/ bullet had accreted into a single
~1,400-char blob across five review rounds. Promoted the carve-outs to
scannable sub-bullets (Domain types / Diagnostic logging / Importing from
core) and lifted the src/types/ note out to its own line, since it's a
statement about a sibling directory, not about utils.
AGENTS.md only; the README's parallel sentence was already clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
* docs+config: address review round 6 — correct the coverage-glob claim (#1776)
Responds to the round-6 @claude review on PR #1784:
1. "no build/coverage glob" was factually wrong (should-fix): the web coverage
include in clients/web/vite.config.ts DOES enumerate src/lib/** and
src/utils/**. The conclusion (a move between them is coverage-neutral)
survives, but the reason was wrong. Reworded both AGENTS.md and the web
README to say nothing *enforces* the boundary — no path alias, and the
include lists both dirs so a move is coverage-neutral — plus the important
whitelist caveat: a module outside components/lib/utils/server falls out of
the ≥90 gate entirely.
2. Removed a pre-existing duplicate glob in vite.config.ts (src/lib/**/*.{ts,tsx}
appeared twice, lines 161 and 163). Inert (vitest de-dupes) but it sat three
lines from the config the doc now describes.
Nits: gave the README bullet the same round-5 sub-bullet restructure as
AGENTS.md, and made its #1785 reference a real markdown link (bare #N doesn't
autolink in rendered repo markdown).
npm run ci green (config change re-verified through coverage + smokes + Storybook).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Close#1527 acceptance gaps with automated tests (SEP-2468 iss, SEP-2207 offline_access, SEP-2351 discovery shapes, SEP-2352 AS-migration, EraNegotiationFailed→401 unwrap); lean EMA leg 2 and scope helpers on the v2 SDK; drop deprecated TS baseUrl. Reviewed LGTM.
Add a specification/v2_auth.md landing page summarizing the four auth specs
(EMA / XAA, Hardening, Mid-session, Smoke testing) with a section submenu,
mirroring the V2 UX index pattern.
Add a V2 Auth link (after V2 UX) and a V2 New Spec Impact link to every spec
page's top menu so the top-level nav is uniform, and restructure the four
v2_auth_*.md pages onto the shared two-tier menu (top menu + auth submenu).
Docs only.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX
* refactor(web): update OAuth E2E tests to use new authentication method
- Replaced `runGuidedAuth` with `authenticate` in the InspectorClient OAuth end-to-end tests to support quick authentication with CIMD pre-registration.
- Updated comments to reflect the change in authentication method and its support for HTTP test metadata URLs.
- Ensured that the tests continue to validate the expected authorization URL after the change.
Additionally, imported `ensureCimdClientRegistration` in the OAuthManager to streamline client registration during the authentication process.
* Guided-auth removal, OAuth smoke testing, and TUI/CLI support
* Added CLI OAuth runner flags tests
* Added clarifying comment on port 0 binding
* README notes on TUI oauth callback port, prettier fixes for CLI tests
* CIMD validation uses inline like EMA IdP
* Fixed TUI test failures from merge
* test: bring 5 OAuth files over the 90 per-file coverage gate
The guided-auth-removal work added/touched several files that fell below
the repo's >=90 per-file coverage gate (npm run coverage). CI doesn't run
the gate (it's local-only), so validate + test:integration passed while
these gaps slipped through. Bring them all back over the line:
- core/auth/node/runner-oauth-callback.ts: test bad-URL/non-http-scheme
throws, no-port->80 default, and IPv6 hostname bracketing. Annotate the
provably-dead guards (empty host, out-of-range port, String(err)
fallback, pathname "/" fallback) -- all rejected by new URL() upstream --
with justified v8 ignore comments instead of lowering the gate.
- core/auth/cimd.ts: test the AS-doesn't-support-CIMD early return.
- core/react/useClientSettingsDraft.ts: test the functional-updater path,
the stale-onChange-after-close prev===null guard, and the
timer-fires-after-close value===null guard.
- ClientSettingsForm.tsx: test the CIMD checkbox toggle and CIMD URL clear.
- clients/tui/src/utils/oauthDisplay.ts: test static/dcr registration
kinds, the expired IdP session, and the configured-scope/empty-scope
branches of formatScopes.
npm run coverage now passes (exit 0) across web, cli, tui, launcher.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: cliffhall <cliff@futurescale.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
