chore: merge v2/main for the v2.6.0 milestone release - #2304
Open
cliffhall wants to merge 233 commits into
Open
Conversation
…2217) OAuth state is keyed by server URL, but `clearServerOAuthAndDisconnect` decided "is this the active connection" from the catalog entry id. Those are different identities and nothing keeps them in sync: `serverList` enforces no URL uniqueness, so two entries with distinct ids against one URL are a supported state — and a natural one, since separate entries are how a user keeps different names, headers or per-server settings against the same server. Clearing the inactive entry therefore deleted the shared URL-keyed blob — the active session's tokens, DCR client id and PKCE state — and, with #2144 in, revoked its grant at the authorization server, while the id check took the inactive branch: no live-client clear, no disconnect, no session cleanup. The active session was left connected on credentials that no longer existed, with nothing told to the user; the break then surfaced somewhere else entirely, on the next refresh, 401 or reload. The branch now compares the OAuth storage key of the cleared entry against the active entry's, and treats a match as affecting the active session — live-client clear, disconnect, session cleanup — exactly as clearing the active entry does. The existing id and client-identity checks stay on top: they guard the separate stale-session race from #2144, and the snapshotted id they revalidate is now the active one rather than the cleared entry's, which for a shared-key clear are not the same. The toast says why the session went away. The alternative, prohibiting duplicate URLs in `serverList`, would remove a legitimate workflow to fix a bug in the consumer and could not repair catalogs that already hold duplicates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VGbw6TezNWnXZSCSp7sqb4 Signed-off-by: cliffhall <cliff@futurescale.com>
The connection-details refresh effect in useOAuthRecovery voided inspectorClient.getOAuthState() with a lone .then and no rejection handler. In the browser that read goes through the remote OAuth store, so it is a network round trip that can reject — a backend that is down or restarting, a 401 on the API token, malformed stored state. void silences no-floating-promises without terminating anything, so every such failure became an unhandled rejection, on the initial refresh and on every oauthComplete refresh. Terminate the chain with a .catch that clears the panel details rather than leaving the last successful read on screen: the panel reports the current OAuth state, and a stale answer is indistinguishable from a fresh one. The cancelled guard is repeated on the catch so a rejection arriving after unmount does not write. The void stays, now with the one-line justification AGENTS.md asks for — a synchronous useEffect body cannot await. Two tests cover it. Both fail against the unfixed source with unhandled rejections, which is the failure mode itself: an unhandled rejection fails the whole vitest run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GBkPzbuxgyqiz39ytmQzMB Signed-off-by: cliffhall <cliff@futurescale.com>
`useOAuthRecovery` cleared its two server-scoped pending-OAuth slots — the deferred re-auth and the step-up prompt — in a `useEffect` keyed on `activeServerId`. An effect runs after the commit, so a server switch painted one frame still carrying the previous server's re-auth banner or step-up prompt: an authorization affordance attached to the wrong server. Reset both during render with `useValueChange` instead. `activeServerId` is a primitive, so it satisfies the helper's `Object.is` stability requirement, and the previous value is read from the `setState` updater argument rather than from `sessionRef.current`, keeping the render-phase callback pure. `react-hooks/set-state-in-effect` did not fire on the original because the effect read the previous value through a ref rather than from a prop or state. The test records one entry per committed render and asserts both slots are empty in the first frame after the switch — the pre-existing `waitFor`-based tests pass against the effect version too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S4WZSxx6XMKzRdgPce9CAd Signed-off-by: cliffhall <cliff@futurescale.com>
The retry closure belongs to the prompt, but the two paths that clear the step-up slot as a *consequence* of something else — the server-switch reset and `resetOAuthRecoveryState` on disconnect — left `pendingStepUpRetryRef` installed. The next prompt to open without a retry of its own would inherit it, so authorizing an ambient step-up on server B ran the command server A was left mid-way through. Clear it in the existing mirror effect whenever the slot is null, so no arm has to remember. A ref write in an effect, since the render-phase reset must stay pure. Copilot on #2237. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S4WZSxx6XMKzRdgPce9CAd Signed-off-by: cliffhall <cliff@futurescale.com>
…RL toast Copilot: the stale-session guard deliberately skips `disconnect()` after a switch, and `disconnect()` can also reject (which gets its own toast), so claiming the active session "was disconnected too" is not always true. What is always true is that its stored tokens went with the shared blob and it must reconnect, so the message says that instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VGbw6TezNWnXZSCSp7sqb4 Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot: `runClear`'s double-click guard deduped by `server.id`, which is the same identity mismatch this PR fixes, one layer up. Two catalog entries against one URL share one credential blob, one grant and one revocation, so an id-keyed guard let exactly the race it exists to prevent through between them — concurrent store writes, concurrent RFC 7009 requests, and two contradictory teardown toasts. The guard now keys on `oauthClearKey`, a pure helper that resolves the OAuth storage key and falls back to the entry id for a config that has none (stdio), where there is no shared state to collide over. The id fallback keeps such entries distinct from each other rather than collapsing them onto one key, and both forms are prefixed so a user-supplied id can never impersonate a URL. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VGbw6TezNWnXZSCSp7sqb4 Signed-off-by: cliffhall <cliff@futurescale.com>
Clearing the retry from the mirror effect raced: an async command continuation can open a prompt and install its retry before a previous commit's passive effect flushes, and that effect's stale `pendingStepUp === null` snapshot would then delete the new prompt's retry. Install it in `trySetPendingStepUp` instead, in the same synchronous step that opens the prompt — `null` for an ambient prompt, the operation for a command-scoped one. Every prompt comes through there and `handleStepUpAuthorize` is the only reader, so a slot cleared as a side effect can leave a stale closure behind harmlessly: the next prompt overwrites it. A refused prompt still installs nothing (#2165). Copilot round 3 on #2237. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S4WZSxx6XMKzRdgPce9CAd Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot: `oauthClearKey`'s unit tests prove the derivation, but a regression that dropped the `has` check, added or deleted the wrong key, or invoked the clear twice would still have passed. Adds an App.test.tsx case that connects entry A, holds its clear open on a deferred `clearOAuthTokens`, then drives a clear for entry B — a different catalog id against the same URL — and asserts the underlying clear ran exactly once, and once more after the first settles, so the key is proven released rather than leaking a permanently dead control. Verified to fail against the id-keyed guard. Two supporting changes it needs: - an `open-settings-b` control on the mocked InspectorView, the only way to reach a settings modal for a server other than the active one; - `@inspector/core/mcp/remote/index.js` becomes a partial mock. `getWebProxiedFetch` reaches for `createRemoteFetch` there, and the bare stub threw a missing-export error before the clear under test ever ran — the failure surfaced as the generic "Could not clear the stored OAuth state" toast. The case carries an explicit 20s timeout and `delay: null`: it drives three full modal interaction sequences against the whole App tree, which runs past the 5s default when the suite is under load. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VGbw6TezNWnXZSCSp7sqb4 Signed-off-by: cliffhall <cliff@futurescale.com>
…recovery-reset-during-render fix(web): reset pending OAuth slots during render, not in an effect
Copilot review round 1. Refreshes are concurrent — an `oauthComplete` can start a second read while the first is still in flight — and nothing makes them settle in order. The new catch made that visible: a slow earlier read could reject *after* a newer one succeeded and clear the fresh result. Guard both handlers with a monotonically increasing sequence so only the newest read writes. The two tests were also too weak to detect their own guards, as the review pointed out. Both started from an undefined panel, so neither could tell a correct handler from one that merely swallowed the error: - The rejection test now seeds a successful read first, then rejects an `oauthComplete` refresh and asserts the already-loaded details clear. - The post-cleanup test now reconnects with a replacement client that populates details, then rejects the stale read, and asserts the current details survive. - A third test covers the ordering guard directly. Each of the three guards has exactly one test that fails without it, verified by mutation: dropping the sequence check, dropping the clear, and dropping the `cancelled` check each break one test and no others. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GBkPzbuxgyqiz39ytmQzMB Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot, and the repo's own rule against an unjustified `as unknown as`. The `useServers` cast was the more useful catch: removing it surfaced that the mock omitted `reorderServers` and `importSource`, so App code calling either would have read `undefined` with this test still passing type-check. The full result shape is supplied instead, matching the `mockServersWith` helper above. The client cast becomes an intersection — the instances are already typed `EventTarget`, so naming the test-only spy on top of that expresses what is needed without erasing the rest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VGbw6TezNWnXZSCSp7sqb4 Signed-off-by: cliffhall <cliff@futurescale.com>
…2217) Copilot: the catalog entry is mutable and the live client is not. A card can be edited while connected — `onConfigSubmit` writes the catalog and does not rebuild the client — so after A connects to X and is edited to Y, the entry reads Y while the session is still authorized against X. Both consumers read the entry, so both got it wrong in the same way: - the hook compared a cleared entry against A's *entry* URL, so clearing another entry still at X missed the match and deleted and revoked the live client's X-keyed credentials without disconnecting it — the exact failure this PR exists to prevent; - `runClear` locked on the entry's key, so a clear of edited-A (`url:Y`) and a clear of entry B at X (`url:X`) took different locks while both performed the same X-keyed operation through the same live client. `InspectorClient.getTransportConfig()` returns the config it was constructed with, which is what its credentials are keyed under, so both now resolve from that and fall back to the catalog entry only when there is no client. The rule moves into one exported function, `resolveOAuthClearIdentity`. Two copies of it that disagree is precisely the class of bug #2217 is, and the two consumers each re-deriving it is how this round's finding came to exist at all. Both fakes gain a `getTransportConfig`, and three tests cover the split: the resolver's own two (the client's URL wins; an active clear locks on the client's key) and a hook case asserting an edited active entry still disconnects when the entry sharing the client's real URL is cleared. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VGbw6TezNWnXZSCSp7sqb4 Signed-off-by: cliffhall <cliff@futurescale.com>
…refresh-unhandled-rejection fix: terminate the OAuth connection-details refresh promise chain
…clear-shared-url fix: key the OAuth clear's active-session check on the storage key
…issue Adds npm audit fix (root + every client) as the first sub-step of the release skill's bump step, before npm version, so a release is never gated on remembering to check separately. Never --force; anything audit fix can't resolve is left to the dependabot-alert pipeline (#2229) or a follow-up issue. Disables Dependabot npm version updates in dependabot.yml (a version-update PR carries no issue and no board card) and replaces them with a monthly scheduled sweep that runs npm outdated across the root install and every client, filing or updating one idempotent tracking issue instead of an auto-generated PR. Closes #2231 Part of #2229 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017N9ha27Qg6LHpBQP7G7zPY Signed-off-by: cliffhall <cliff@futurescale.com>
The npm-entry removal in `.github/dependabot.yml` is split out of this PR so that every change to that one file lands in a single reviewable piece — alongside the `github-actions` entry, which #2229 had declared out of scope but which opens the same issue-less, board-cardless PR the decision exists to remove. Reverts `.github/dependabot.yml` to its `v2/main` state, leaving this PR with the release-time `npm audit fix` and the monthly `npm outdated` sweep: the replacement flow, not the switch-off. Both new files' header comments claimed the switch-off had already happened, which is false until #2235 lands. They now say #2235 does it, and that until then Dependabot's npm PRs and this sweep overlap — duplicate signal rather than conflicting action. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RWMJENjs1mExXE5XPJTEei Signed-off-by: cliffhall <cliff@futurescale.com>
Five findings, all valid. `runOutdated` accepted every exit status and returned `result.stdout ?? ""`. A failed `npm outdated` (exit 2 on a registry or config error) also prints nothing, so the empty string parsed to an empty package list — and across five installs a total outage produced output byte-identical to a clean sweep. It now throws on any status other than the documented 0/1. `main()` was untested, which is how that reached review: the helper-only suite passed while the silent-success path was live. It now takes its spawn function as a parameter and is driven by a fake, covering npm failure, create vs. edit, milestone handling, both no-op paths and the missing-repo guard. The no-op path returned before looking for the marker issue, so once every install caught up, a still-open tracking issue kept its obsolete package table indefinitely — contradicting the body's own promise to update in place. It now rewrites through `buildClearedBody()`. It deliberately does not close the issue: the sweep takes no board actions, and closing one whose card a maintainer may have moved would make the board assert work shipped that this script cannot verify shipped. The release step mandated `npm audit fix`, which AGENTS.md forbids. The reason is not `--force`: plain `audit fix` resolves an advisory with no upward escape in range by silently downgrading, as it did to esbuild across three installs in #2058, and `local:gate` has no version-regression check to catch it. The step is now `npm audit` report-only, with fixes applied deliberately via a direct bump or an `overrides` entry. An unmilestoned issue is swept into Incoming, not Todo — Todo asserts a maintainer signed off. Message corrected and pinned by a test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RWMJENjs1mExXE5XPJTEei Signed-off-by: cliffhall <cliff@futurescale.com>
`main refuses to run without a repo` called `main(undefined, …)` so the default parameter would fire, then asserted it throws. The default reads `process.env.GITHUB_REPOSITORY` — which GitHub Actions sets on every run — so the assertion held only where the variable happened to be unset. It passed locally and failed CI, the one environment where the default is always populated. The test now clears the variable and restores it, and a second test covers the other half of the default: with it set, `main(undefined, …)` uses it instead of throwing. Between them the default's behavior is pinned in both environments rather than inherited from whichever one is running. Verified both ways: 14 pass with the variable unset and with it set, and the full `test:scripts` suite is 411/411 under the CI environment shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RWMJENjs1mExXE5XPJTEei Signed-off-by: cliffhall <cliff@futurescale.com>
Closes #2235. Switches the old dependency flow off now that #2232 landed the replacement. Removes the five npm ecosystem entries (root plus each client under clients/*) and, resolving the question #2229 left open, the github-actions entry as well — so .github/dependabot.yml goes away outright rather than being emptied, which its schema does not allow. The github-actions entry had exactly the property #2229 exists to remove: it opened a grouped monthly PR carrying no `Closes #N` and no board card, the one standing exception to "every PR references an issue". Deleting it unreplaced would have left 9 actions unwatched, and `npm outdated` says nothing about actions, so the monthly sweep now also checks every `uses:` ref under .github/workflows and renders the stale ones as one more section of the same tracking issue. Ranking comes from the release LIST, not `releases/latest`. That endpoint returns the release GitHub designates most recent, not the greatest version, so an action publishing a maintenance release for an older major (a v6.9.1 cut after v8.0.0) would make a workflow pinned to v7 compare against v6 and read as current — silently missing a whole major upgrade, the one thing this check exists to catch. Staleness is compared only to the precision the ref specifies. `v7` is a moving major tag that GitHub repoints at every v7.x release, so `v7` against a highest of `v7.0.1` is current and only `v8` makes it stale; an exactly-pinned `v7.0.0` is behind `v7.0.1`; a SHA pin is deliberately immovable and is never reported. A release lookup suppresses only a 404 — the legitimate "this action has never cut a release" answer — and throws on anything else. Treating a rate limit or an expired token as "no release" is indistinguishable from "not stale", and since every action here already sits on its latest major, the resulting empty section is byte-identical to a healthy run. `buildClearedBody` now speaks for both halves: once actions are in scope, its npm-only wording would assert a clean bill of health the sweep never checked. Dependabot security updates are unaffected — they are configured in repo settings, not in this file, and kept working while it was missing entirely (#1833, #1840). That note moves into the workflow header rather than dying with the file; #2233 is where they are turned off deliberately. The script header, the workflow header and the generated issue body all say version-update PRs rather than claiming Dependabot is replaced wholesale. Also renames the workflow's npm-outdated job to dependency-sweep now that the sweep is no longer npm-only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NGtzPg3UxMLszysQXXfqax Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot review round 4 on #2239. Both findings are regressions I introduced in round 3 and both are correct. Round 3 moved the lookup from `releases/latest` to the release LIST, to rank by version rather than by GitHub's designated latest — but kept the 404 suppression that only made sense for the old endpoint. The list endpoint answers "this action cuts no releases" with a successful empty array, so a 404 there does not mean that at all: it means the repository is missing or inaccessible, i.e. a `uses:` ref the sweep cannot check. Converting it to null silently dropped a broken or renamed action from the flow that replaced Dependabot — the same silent-success shape this PR has been closing everywhere else, reintroduced one layer down. Every non-zero status is now fatal and `isMissingRelease` is gone entirely; the benign no-releases case is the successful empty array, covered by its own test. `buildClearedBody` also overstated its all-clear. Refs pinned to a commit SHA or a branch return null from `parseVersionRef` and are never ranked, so "no workflow `uses:` ref is behind" asserted a check that had not happened for them. It now says version-pinned refs and names the exclusion explicitly. Both guards are mutation-checked: restoring the 404 suppression, and dropping the SHA/branch qualification, each fail exactly one test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NGtzPg3UxMLszysQXXfqax Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot review round 5 on #2239. Round 4 made every non-zero release-list response fatal and deleted `isMissingRelease`, but `latestReleaseTag`'s JSDoc still promised `@throws when the lookup fails for any reason other than a 404` — the exact behavior that change removed. The PR description carried the same stale claim. A contract that documents the opposite of the code is worse than none: the next reader reasonably trusts it, and here it would tell them a missing action repo is silently tolerated when it now fails the sweep. The contract now reserves `null` for a successful response carrying no usable release — an action that has never cut one, or whose tags are all unparseable — and states that a failed lookup is never `null`. Documentation only; no behavior change, and the 29 tests are untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NGtzPg3UxMLszysQXXfqax Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot review round 7 on #2239. The lookup read one page of 100 and the doc comment justified it with an assumption rather than a guarantee: "the greatest version is within it for any real action". The list is ordered by release DATE, not version, so an action that keeps cutting maintenance releases on lower majors pushes the genuine maximum off page 1. Ranking what remained would report a lower major as highest — the exact false-current result that dropping `releases/latest` in round 3 was meant to prevent, arrived at by a different route. Now passes `--paginate`. Cost is unchanged for every action this repo uses (57 and 64 releases are the largest, so still one request each); it only costs more where correctness actually required it. The command-shape test now requires `--paginate` as well as the list endpoint. It has to: pagination is invisible in the result — one page and every page return an identical-looking tag list until the day they do not — so nothing about the returned value can detect its absence. Verified by mutation: removing `--paginate` fails exactly one test. Adding the flag also moved the URL out of args[1], which the test's own filter had hardcoded — it caught that itself. Both the filter and fakeSpawn's matcher are now argument-position agnostic. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NGtzPg3UxMLszysQXXfqax Signed-off-by: cliffhall <cliff@futurescale.com>
Closes #2233 — the alert-consuming half of #2229. #2235 removed `.github/dependabot.yml`, ending Dependabot's version-update PRs; this ends its security-update PRs and replaces them with a daily sweep that turns the alerts into ordinary board items. - `scripts/dependabot-alerts.mjs` groups alerts by `(package, manifest_path, first_patched_version)` — one issue per BUMP, not per advisory — re-checks each vulnerable range against `v2/main`'s own lockfile before filing, and keys idempotency on a marker comment naming every GHSA the issue covers. - `.github/workflows/dependabot-alerts.yml` runs it daily and on `workflow_dispatch`, with `vulnerability-alerts: read`. - `semver` is declared at the repo root, per Dependency placement: `scripts/` is root-owned code with no manifest of its own. - The sibling `dependency-refresh` comments no longer claim security updates are unaffected, and AGENTS.md gains the flow both halves now follow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq Signed-off-by: cliffhall <cliff@futurescale.com>
- `--slurp` the paginated alert feed; a bare `--paginate` emits one JSON array per page and `JSON.parse` rejects it past 100 open alerts. - Put `fixedIn` in the marker and in the existing-issue lookup, so a second bump of one package cannot merge into the first one's issue. - Only an authorization-shaped failure of the `automated-security-fixes` read becomes UNVERIFIED; a rate limit or 5xx now throws. - Distinguish "card never added" (benign, triage picks it up) from "card added, field not set" — the latter finishes every group, then fails the run. - Don't board an unmilestoned issue at Todo; `Incoming` <=> no milestone. - Comment before rewriting the marker, and give the comment its own marker, so a failed comment cannot be skipped forever. - Ask a direct dependency's range to be raised, not widened to `>=`. - Test `main()` through an injected spawn, as the sibling sweep does. - Correct the docs that named the removed release-time `npm audit fix`, the ones promising an unconditional guard, and the sibling sweep's issue body claiming security updates remain enabled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq Signed-off-by: cliffhall <cliff@futurescale.com>
- Remove the literal NUL bytes that were separating the grouping key's fields; they classified the whole source file as binary, so repo searches skipped it. The key is a JSON array now, with no separator left to justify. - De-duplicate advisory comments per GHSA rather than per whole set. A run that comments and then fails before the marker edit left the next run computing a different set, which matched nothing and announced the same advisory twice. - Refresh the issue title on edit; it carries the advisory count, so it went stale as soon as an issue grew past what it was filed with. - Correct the test file's header, which still claimed main() was left to workflow_dispatch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq Signed-off-by: cliffhall <cliff@futurescale.com>
Derive the remediation from WHICH copies are vulnerable, not from whether the package is declared. A manifest can declare a safe `pkg@4` while a dependency drags a vulnerable `pkg@3` into a nested folder; the old boolean called that "direct" and asked for a range bump that would have changed nothing, omitting the override the nested copy needs. - `lockfileEntries` keeps each copy's tree path and whether it is the hoisted one; `lockfileVersions` is now derived from it. - `remediation(affected, declared)` returns both flags, so the issue can ask for a range bump, an overrides pin, or explicitly both. The body lists the vulnerable copies and their paths. - An undeclared hoisted copy counts as transitive: it got there the same way any other transitive copy did, and no declared range reaches it. (Found by a test written for this change.) The `vulnerability-alerts: read` finding is declined — verified against a real runner, which reports `VulnerabilityAlerts: read` and reads the alerts successfully. See the PR comment for the log. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq Signed-off-by: cliffhall <cliff@futurescale.com>
- Narrow each group to the advisories the installed copies are actually in range of. Two advisories can share a package, manifest and patched version while having different vulnerable ranges, so a group-level "does any match?" left the marker, title, severity and table all claiming an advisory that does not apply on this branch. - Escape every free-form Markdown cell, the vulnerable range included. A semver range may contain `||`, which is also the column separator. - Paginate the open-issue lookup instead of capping it at 100. The marker lookup is what makes the sweep idempotent, so a truncated list would file a duplicate for every issue it could not see — at the same scale the alert fetch is built to handle. Pull requests, which the issues endpoint also returns, are dropped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq Signed-off-by: cliffhall <cliff@futurescale.com>
Eight findings. Two were visible in a terminal capture I had already taken
and read past.
**A digest mismatch rendered with no digests.** `verifySkillResource` sets
`reason` for a SIZE mismatch — which short-circuits before hashing — while a
digest mismatch carries `expectedDigest` / `actualDigest` and no `reason`, so
the TUI printed a bare `✗ notes.md` with nothing to act on. `failureDetail`
now falls back to the expected/actual pair, truncated to fit the pane.
**A dynamic skill's read failure rendered nowhere.** Its synthetic
`read-error` row lives in the report, not the manifest, and the manifest is
empty for such a skill by definition — so the pane said only "Verification
FAILED". There is a Read failures block for any report file the manifest does
not cover, matched on normalized identity.
**The non-finite sentinel could be aliased.** A listing whose value genuinely
was `{"#non-finite":"NaN"}` canonicalized identically to a served `.nan`, so
the round-3 fix moved the bug rather than closing it. Any encoding into the
value space can be aliased by a document containing the encoding, so the
comparison is structural now (`jsonLikeEqual`) and there is no sentinel at
all; `Object.is` gives NaN === NaN while keeping ±Infinity distinct.
Serialization is used only for the message.
**`parseSkillFrontmatter("null")` returned `{ fields: {} }`,** contradicting
its own non-mapping contract: an explicit null scalar parses to exactly what
an empty block does. They are told apart by the source, since the parsed
value cannot.
Also: the `files` doc comment still claimed it is empty for a dynamic skill,
which the round-2 fix made untrue; and three inline Mantine elements with two
static styling props each are extracted to `.withProps()` constants, per the
convention the rest of the file follows.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc
Signed-off-by: cliffhall <cliff@futurescale.com>
**A cyclic YAML alias crashed the tool.** `meta: &m [*m]` parses cleanly into a self-referential array, and the frontmatter comparison is a recursive walk, so a hostile server could take down `--verify` or the TUI with a stack overflow instead of receiving a finding. Verified before fixing: it really did raise `RangeError: Maximum call stack size exceeded`. Rejected at the parse, so no consumer has to be cycle-safe on its own, and it becomes an ordinary `frontmatter-unparsable` finding. Detection is per-PATH rather than per-graph — `seen` unwinds on the way back up — so an alias reused across siblings, which is ordinary YAML and represents fine in JSON, is not mistaken for a cycle. A depth bound closes the same hole by its other door: a legal, acyclic but absurdly nested document exhausts the stack just as well, and a cycle check alone passes it. **The web frontmatter check was gated on the presentation MIME.** It ran off `previewParts`, which exists only when the displayed type is recognized as markdown — so a `SKILL.md` a server typed `text/plain`, or served as a base64 blob, skipped a mandatory comparison while the pane still read as clean. It now decodes the same fetched bytes the digest is taken over, which also stops the two answers describing different derivations of the payload. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall <cliff@futurescale.com>
`&a [1, *a]` puts the self-reference after a plain value, so a guard that only inspected the head of a sequence would miss it. Already caught; this records that it is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall <cliff@futurescale.com>
Four findings. **The CLI dropped what it promised to print.** `getSkill` transforms the `skills/get` envelope to a `SkillEntry`, so every other member the loose schema accepted was gone before the CLI saw it — including the `ttlMs` / `cacheScope` SEP-2640 explicitly leaves open. The code contradicted a comment two lines above it saying a CLI whose contract is "print the result" must not reshape one. `getSkillResult` returns the envelope whole; `getSkill` is a one-line unwrap over it, which matches the callers: the UIs want the entry, the CLI wants the result. **The frontmatter verdict and the digest came from different reads.** The preview fetch and the Verify fetch are separate `resources/read` calls, so a resource changing between them could pair a verified digest with a frontmatter verdict for other bytes. `VerificationState.entryText` now holds the SKILL.md as the verification read it, written in the SAME state update as the verdict — a separate write was clobbered, because `write` returns a fresh object and drops anything not named in it. The preview stays the fallback so a reader who has not clicked Verify still gets the check. **The Conformance badge excluded the frontmatter findings** it renders, so it said `0 error(s)` above a red mismatch. Digest and size mismatches stay in their own badge: "the listing is wrong" and "the bytes are wrong" are different answers. **A repeated skill URI collided two React keys** in the TUI list, in a pane whose job is to show both entries. Index-keyed like the manifest rows. The web fixture also gains its collision pair in the served-file lookup — without it they were handed another skill's SKILL.md, which the badge fix correctly began reporting as a frontmatter error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall <cliff@futurescale.com>
**The depth guard covered only the YAML side.** The listing arrives over JSON-RPC so it cannot be cyclic, but it is just as unbounded in depth — and both `jsonLikeEqual` and its message formatter walk it, so a server advertising a deeply nested value crashed the tool exactly as a cyclic served one did. Reproduced first: 60,000 levels against a shallow served value gives `RangeError: Maximum call stack size exceeded`. `jsonGraphError` is shared now and runs over `entry.frontmatter` before the file is parsed — there is no point reading a SKILL.md if the thing to compare it against is unusable. **A frontmatter mismatch stayed behind a collapsed section.** Round 7 made the badge count these findings; the alerts explaining a mandatory verification failure were still one click away on a section that opens collapsed for a structurally clean entry. Conformance now reveals itself when they appear, keyed on the entry so it fires once rather than fighting a user who collapses it again. That reveal exposed three fixtures serving one skill's SKILL.md for every URI — a genuine frontmatter mismatch the check was right to report. The stale- batch invariant test and the Storybook fixtures now derive each file from the frontmatter its own entry advertises, with digests computed from those bytes rather than hard-coded, so the class of drift is closed rather than the instances patched. `SAMPLE_FM` is the single source for both halves. One story subtlety worth recording: only a skill's OWN SKILL.md is derived. Serving it for `notes.md` too made the tampered fixture fail its SIZE check first, which is a different finding from the digest mismatch that story exists to demonstrate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall <cliff@futurescale.com>
**Verification reads were unbounded.** The 512-entry limit is checked by `checkSkillConformance` — as a warning, since the SEP makes it an interoperability bound rather than a MUST — but checking it constrains nothing, so a hostile server advertising a million entries had the tool perform a million sequential reads after the report already knew the manifest was over. Capped at `SKILL_MAX_RESOURCE_ENTRIES`, with the finding still reported so bounding the work does not silence the reason it was unnecessary. `manifestListsSelf` is computed over the READ slice, so a self-entry pushed past the cap still reaches the fallback: the frontmatter comparison is mandatory and must not be lost to a limit that bounds other files. **A malformed skill URI still produced a directory root.** `skillUriIdentity` falls back to the raw string, so `not a uri/SKILL.md` yielded the root `not a uri` and enabled the Directory section — letting the UI build a request from a URI the conformance checks had already rejected. The comment above it claimed otherwise. `normalizeSkillUri` now, with the rule written down: identity is for COMPARING two spellings, not for deciding a URI is well-formed enough to build a request from. The existing regression test passed against a weaker input that failed for lacking the suffix and never reached the fallback; it is a table of three shapes now. **The test-server guide overstated its own coverage,** claiming four of the five conformance scenarios map onto fixtures. Three do: there is no size-mismatch fixture, because the fixture can override an advertised digest and nothing else, so that path is covered by unit tests. Corrected, with what adding one would take. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall <cliff@futurescale.com>
Round 10 raised no new inline comments; three suppressed findings, two fixed and one declined with a reason. **`Object.is` held `0` and `-0` distinct**, so a listing carrying JSON `0` against a served YAML `-0` produced a mismatch — reported as "the listing says 0 but the served SKILL.md says 0", a false finding with an unintelligible explanation. JSON does not distinguish them, so neither may this: `===` for finite numbers, `Object.is` kept only for the non-finite cases it was introduced for. **Browsing a supporting file erased an observed failure.** The frontmatter check ran off whatever the viewer was showing, so opening a second file made `showingSkillMd` false and dropped the finding and its error count, with the skill unchanged. The entry's text is skill-scoped now, written by whichever read produces it first — the on-selection preview or a verification — and invalidated only with `manifestKey`. That also subsumes the earlier same-fetch requirement rather than sitting beside it. **Declined: the second stderr line on exit 7.** `--verify` writes its summary and then the ordinary `ErrorEnvelope` that EVERY non-zero exit writes, which is the documented contract and exactly what `--strict` does. Suppressing it would make this one command's failure output unparseable by a caller branching on `.code`. What was wrong is the README, which implied stderr carries only the summary; it now states both lines and why. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall <cliff@futurescale.com>
**Reads were bounded by entry count but not by total bytes.** A manifest can sit at exactly 512 entries and declare a gigabyte each, so round 9's cap still let a server dictate unbounded bandwidth and time after `size-limit-exceeded` had already been reported. `boundedManifest` caps on both interoperability limits now. Sizes are summed exactly as `totalSkillBytes` sums them, so the bound and the finding that reports the overage cannot disagree about the total. A read is skipped only when the running total would CROSS the limit, so a conforming skill — at most 16 MiB by definition — is never truncated; there is a test for that, because a bound that protected against a hostile server by giving a wrong answer about a good one would be a poor trade. The self-file fallback still applies, so the mandatory frontmatter check survives either cap. **The `vitest.shared.mts` pin rationale had gone stale.** It said `yaml` is reached only through `test-servers/src`, which this PR made untrue by importing it from `core/mcp/skillFile.ts`. That comment is what a future dependency-placement change would read before removing or reclassifying the pin, so it now names the `core/` path and the bundler `external` lists that follow from it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall <cliff@futurescale.com>
**The read bounds could produce a false `ok: true`.** Entries past the cap are never fetched, `fileFailed` only examines fetched rows, and `resource-limit-exceeded` is a WARNING — so a manifest whose 513th file was tampered with reported the skill verified. That trades a denial of service for a wrong answer, which is the failure this whole PR argues against. A truncated read is now recorded as `incomplete` and forces `ok: false`, reported separately so a consumer can tell "this skill is wrong" from "this skill was not fully checked". And a self entry excluded by the cap is now verified against its declared digest, not merely read for frontmatter — otherwise the skill's own SKILL.md was the one file nobody checked. **`entryKey` could crash the TUI.** It called `JSON.stringify` on the whole server-controlled entry, during render. `skillEntryKey` is decided by the same `jsonGraphError` guard that bounds the frontmatter comparison, with a coarse fallback that still separates two unrepresentable entries by URI — collapsing them would show one skill's verdict under another's name. **The tab bar wraps and `tabsHeight` was hard-coded to 1.** An OAuth-capable stdio server serving Skills needs ~107 columns, so it wraps at 132 as well as at 80, and every pane below was sized a row too tall. `tabBarRows` derives it from the same list `Tabs` renders, via a shared `visibleTabs(flags)` — the duplicated filter being how the two would drift apart again. Nine tests in a new `tabsConfig.test.ts`, including the 80-column regression. Two assert properties rather than instances: rows never decrease as the terminal narrows, and a tab wider than the row gets its own rather than looping. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall <cliff@futurescale.com>
**The byte budget trusted server-declared sizes.** A manifest advertising `size: 1` — or, since the wire schema deliberately accepts it for reporting, no size at all — sailed through the declared budget and then served arbitrarily large bodies. The 16 MiB safeguard protected against an honest server only, which is no safeguard. The walk now tracks bytes ACTUALLY received and stops issuing reads once the real total crosses the cap, marking the report `incomplete` and so `ok: false`. The declared prefilter is kept rather than replaced: it refuses to schedule an obviously oversized set, and the received-byte counter stops one that lied. The count happens after verifying the crossing file, so that file is still reported rather than fetched and discarded.⚠️ This bounds the total across responses, not any single one: a first response larger than the cap is already in memory before it can be measured, which would need a streaming read the client API does not expose. Stated in the code rather than left to look closed. **A modern `skills/get` did not require `resultType`.** The "left open" question SEP-2640 states covers `ttlMs` / `cacheScope` and only those; `resultType` is base-protocol per SEP-2322 and appears in the SEP's own `skills/get` example. Requiring it of `resources/directory/read` but not here was an inconsistency in the module rather than a distinction the spec draws. Era-selected now, with the caching attributes still optional. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall <cliff@futurescale.com>
**The TUI ignored `incomplete`.** Round 12 added that field precisely so a consumer could tell "not fully checked" from a real failure, and then the one client that reads the report did not consume it — which made the field decorative. The pane shows an Incomplete: block with the report's own reason, and the status line reads INCOMPLETE rather than FAILED. It sits ABOVE the manifest, not beside the status line: it explains the list that follows — only the first N rows were fetched, the rest stay marked `·` because nobody looked at them — and below a 512-row manifest it would be off-screen, which is the same as absent. Found by writing the test, whose first version asserted against a 600-entry fixture and could see neither string. The test triggers truncation through the BYTE budget instead, so the manifest stays three rows and the assertions measure the pane rather than the test's viewport. **The capped self-entry was reported under the wrong URI.** A manifest may write its self-entry in a normalized-equivalent form; the fallback recorded `entry.uri`, so manifest rows keyed on the declared spelling matched nothing while the normalized extra-files filter suppressed it as already covered. The verdict existed in the report and appeared nowhere on screen. Recorded under `declaredSelf.uri` now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall <cliff@futurescale.com>
**Verification is a tri-state now, because two states could not be right.** Round 12 was correct that `ok: true` for a manifest whose unread 513th file may be tampered with is a false pass. Round 15 is correct that forcing `ok: false` calls a server nonconformant for exceeding limits SEP-2640 states as SHOULD NOT, with hosts free to support more — contradicting this module's own rule that a warning never fails a report. Both hold, so: verified -> exit 0 everything checked, everything passed failed -> exit 7 a MUST was broken incomplete -> exit 8 nothing checked was wrong; the walk was cut short `ok` keeps its narrow meaning, `allSkillsVerified` is stricter than `every(r => r.ok)` since it selects the exit code, and `anySkillFailed` separates 7 from 8. A job that tolerates oversized catalogs can allow 8 and still fail on 7. **`manifestKey` still used `JSON.stringify` on the entry** — the same crash I fixed in the TUI in round 12, one file over. **Directory children were navigable outside the skill root.** A server could return `skill://other-skill/...` and clicking it left the tree, with "Up" only comparing equality against the root. Containment is checked on the normalized URI now, and an outside child renders as text rather than a link. Worth recording from the tests: `..` cannot escape the authority, so `skill://a/nested/../x.md` resolves back inside and must stay navigable. **The TUI matched report rows by raw URI**, which missed a row recorded under an equivalent spelling while `extraReportFiles` suppressed it as covered. Normalized, like the membership test beside it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall <cliff@futurescale.com>
**A hole my own round-13 change opened.** The byte budget added a `break`, and `manifestListsSelf` described the bounded slice rather than the rows the walk reached — so breaking before a later self-entry left the flag true, suppressed the fallback, and skipped the mandatory frontmatter check entirely. It is `selfAttempted` now, set inside the loop, and marked before the read: a row the walk reached but could not read has still been attempted, and its failure belongs to the loop rather than to a second fetch. **A preview could overwrite a verification's own bytes.** The digest verdict on screen was computed from the verification's fetch; replacing only the text let the frontmatter findings describe different bytes, recreating the mixed-fetch verdict that state exists to prevent. `entryTextVerified` marks which read produced it, and a preview no longer wins over a verification. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall <cliff@futurescale.com>
Four of the five findings share one root cause: the tri-state `outcome` added in round 15 was not propagated to consumers, which kept branching on `ok` — true for an `incomplete` report, since nothing that was checked was wrong. - CLI `summarizeSkillVerification` counts off `outcome`, not `ok`, so a truncated walk no longer prints "no conformance errors" one line before exiting SKILL_INCOMPLETE. A mixed failed/incomplete catalog reports both counts rather than letting the louder verdict hide the quieter. - `verifySkills` sets the truncation reason only when entries were actually left unread. A file crossing the byte budget as the FINAL entry stopped nothing, and "Stopped after 3 of 3" both read as a contradiction and demoted a fully-read skill out of `verified`. - The TUI status line switches on `outcome` through a `Record` over the union, so the INCOMPLETE arm is reachable and a fourth outcome would be a type error rather than a silently missing label. - The `incomplete` doc paragraph said it makes `ok` false. It does not, deliberately — corrected, and it now names `outcome` as the thing a consumer must branch on. - The web sidebar composes in the per-URI `duplicate-name` warning, so two colliding skills are badged in the catalog instead of looking clean until one is selected. The suppressed `selfAttempted` finding is stale — fixed in round 16. The existing TUI INCOMPLETE test was passing for the wrong reason: its fixture understated every size, which is itself a mismatch, so the report was `failed` and only the old `ok`-first branch printed INCOMPLETE. It is rebuilt with honest digests and sizes, so truncation comes from the declared-size prefilter and the report is genuinely incomplete. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall <cliff@futurescale.com>
Three findings, all in paths hardened by earlier rounds. - The 16 MiB read budget was charged only after a content block had matched by URI *and* decoded. A server could answer every manifest row with one enormous block labelled a different URI (so `contentsFor` found nothing) or one enormous invalid-base64 blob (so `skillFileBytes` threw), bank zero against the cap, and have the walk issue up to 512 more. `responseBytes` now charges the raw response before selection or decoding; the exact decoded length is substituted when there is one, so the common path is still measured precisely. - The Directory section checked that a child was inside the skill root but not that it was a child of the directory actually being read. A grandchild, or the directory echoing itself back, passed containment and was rendered as navigable. Non-direct entries are now shown and labelled rather than linked — `resources/directory/read` answers with direct children, so an entry that is not one is itself the finding. - Containment was decided on the normalized URI while navigation sent and stored the raw one. For `skill://r/a/../templates` the first Up produced `skill://r/a/..` and a second walked into `skill://r/a`, a directory nothing had validated. Directory descent passes the normalized URI, and the Up arithmetic moved into `parentOfSkillUri`, documented as valid only on a normalized URI. The suppressed comment (two stderr lines on a failing `--verify`) is declined again: `--strict` produces the same shape in `emit-result.ts`, and clients/cli/README.md already documents it as such. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall <cliff@futurescale.com>
Four findings, all real. - No catalog-level bound. The per-skill caps bound what ONE entry can cost; nothing bounded how many entries there are, and SEP-2640 puts no ceiling on a catalog — every entry costs at least one resources/read, so a large listing made `--verify` run indefinitely and transfer unboundedly. Adds SKILL_MAX_CATALOG_SKILLS / SKILL_MAX_CATALOG_BYTES. Entries past the budget are still reported, with their static findings and an `incomplete` reason, rather than dropped or failed: they were not checked, which is neither a pass nor a verdict against the server. - `responseBytes` charged `text.length`, which counts UTF-16 code units. That undercharges non-ASCII by up to 3x, so a decoy block of emoji kept the counter under 16 MiB while the wire carried far more. Adds `utf8Length`, an allocation-free UTF-8 byte count — deliberately not TextEncoder, which would copy a payload the server chose the size of. - `checkSkillNameCollisions` was O(N^2) in both work and output for a group of N: every entry filtered all N URIs and embedded the other N-1. Duplicate names are legal and a server controls N. Now names a bounded sample and counts the rest. - `--verify` help documented exit 7 but not exit 8. The three-tier bounding (per skill, per skill on the wire, per run) is now a table in clients/cli/README.md, since the run bound is this tool's limit rather than the spec's and should not read as a conformance rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall <cliff@futurescale.com>
…s-phase3 feat: Skills extension phase 3 — CLI methods, TUI pane, resources/directory/read, and the frontmatter cross-check
#2298) The release skill described a v2 release as three steps and left four things a release actually depends on implicit or absent: that it is two pull requests against two different bases, the production smoke of the release candidate, the ledger artifact the maintainers review, and where a smoke finding gets fixed. It also gave only the `git tag` path, which is the fallback rather than the norm. Restructure it around PR 1 -> PR 2 -> Release: - a "The shape" table contrasting the two PRs by branch, base, contents, verification and merge condition, plus why they must not be folded together and why PR 1 merges before PR 2 is opened; - step 2 broken into the tree-hash check and production smoke, the ledger's structure, and the fix-on-`v2/main` rule that keeps the merge tree byte-identical to `origin/v2/main`; - step 3 leading with the GitHub UI path, keeping the CLI commands as the by-hand equivalent. Docs only. The `npm audit fix`, `--no-git-tag-version`, tag-`origin/main`, no-`v`-prefix and #2010 warnings are all preserved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RsmR1iQstcrEzJFmgZXGLi Signed-off-by: cliffhall <cliff@futurescale.com>
…se-skill-two-pr-flow docs(release): make the two-PR flow, the production smoke, the ledger and the UI tag explicit
…ories `npm audit` at release time reported hono <=4.13.4 against three moderate advisories: GHSA-gqvv-2mrq-wpjv (toSSG() writes outside the output dir), GHSA-g6gw-c38x-mqfc (unbounded dot-notation nesting in parseBody() can exhaust memory) and GHSA-crvj-82cr-hjcx (the query parser reads parameters after the URL fragment, creating cache-key and proxy differentials). hono is a runtime dependency of `core/`, so the fix raises the declared floor rather than only moving the lockfile: a published install resolves this range from the root manifest, and `^4.13.1` would still have let a consumer land on a vulnerable 4.13.x. Refs #2300 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RsmR1iQstcrEzJFmgZXGLi Signed-off-by: cliffhall <cliff@futurescale.com>
Ahead of the v2.6.0 milestone merge into `main`. The bump belongs on `v2/main` so it flows into `main` with the rest of the milestone's work — doing it on the merge branch instead leaves `v2/main` reading a stale version and lets the bump leak into unrelated PRs (#2010). There is one version number in the repo; the clients carry none. No tag is created here: the release tag points at the merge commit on `main`. Closes #2300 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RsmR1iQstcrEzJFmgZXGLi Signed-off-by: cliffhall <cliff@futurescale.com>
…-2-6-0 chore(release): audit, hono advisory fix, and bump to 2.6.0
There was a problem hiding this comment.
🔵 Needs a closer look
The 213-file release payload has a functional schema-form issue and still lacks the required completed gate result and smoke ledger.
Pull request overview
Merges the complete v2.6.0 development tree into main for release.
Changes:
- Adds Skills support across core, web, CLI, and TUI.
- Expands protocol fixtures, verification, and dependency automation.
- Applies the 2.6.0 version and dependency updates.
Blocking finding: SchemaForm.tsx:949-952 does not reopen raw JSON when switching directly between two unsupported root-union schemas after raw mode was manually disabled. Key the reset by schema/entity identity and add a regression test.
File summaries
| File | Description |
|---|---|
.github/** |
Updates CI and dependency/security automation. |
core/** |
Adds Skills protocol, verification, state, and auth behavior. |
clients/web/** |
Adds Skills UI and schema-form updates. |
clients/cli/** |
Adds Skills commands and output handling. |
clients/tui/** |
Adds Skills views and interaction support. |
scripts/** |
Adds dependency monitoring and gate tooling. |
test-servers/** |
Expands protocol fixtures and showcase configurations. |
docs/** |
Documents new workflows and test servers. |
package.json / lockfiles |
Updates dependencies and version to 2.6.0. |
vitest.shared.mts |
Documents the updated shared YAML dependency ownership. |
Review details
- Files reviewed: 167/213 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Member
Author
Verification complete — ledger attachedLedger artifact: https://claude.ai/code/artifact/e02d91dc-92d2-4fac-8418-17407a2d4c50 This merge is ready for review. Every contribution closed in the v2.6.0 milestone has been verified against the production build (the packaged bin and the built bundles, not Verdict
Merge tree is byte-identical to What was actually drivenEleven contributions were exercised by hand against running test servers, across all three clients. A few of the more decisive ones:
Two rows are test-backed rather than hand-driven, and are marked as such#2242 (CIMD provenance) has no fixture that enables CIMD, and #2221 is an unhandled-rejection fix whose observable is the absence of a failure. Both are recorded in the ledger against the specific named tests that hold them, rather than presented as driven. A CIMD fixture is the missing piece if we want to drive that one by hand next cycle. Findings — neither blocking
After merge: tag 🤖 Generated with Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2303
The v2.6.0 milestone merge:
v2/main→main, arriving whole. This branch carries no commits of its own — it is the merge commit and nothing else.Tree identity
The merge commit's tree is byte-identical to
origin/v2/main, which is what makes this a pure merge and the release candidate the same tree that was developed and gated:Version on the merge tree: 2.6.0 (bumped on
v2/mainin #2300 / PR #2302, per #2010).Milestone payload
42 closed contributions, plus release step 1 (#2300). The only issue left open in the milestone is this merge (#2303).
Verification — complete
npm run local:gatein a dedicated worktree with a fullnpm install— pass (9,346 tests, 11 smokes).vite dev), through whichever clients each change touches — done, 42 / 42.Ready for review. The verification summary is in the comment below; 0 regressions found.
If the smoke finds something
The fix goes on
v2/mainthrough an ordinary PR, thenv2/mainis merged into this branch again so it arrives the same way everything else did — keeping the merge tree identical toorigin/v2/main(#2000 → #2092; #2215 → #2216–2224). No commit is authored on this branch.After merge
Tag
origin/mainwith the bare2.6.0— novprefix — and publish the GitHub Release, which is what fires thepublishandpublish-github-container-registryjobs.🤖 Generated with Claude Code
https://claude.ai/code/session_01RsmR1iQstcrEzJFmgZXGLi