chore: merge v2/main for the v2.5.0 milestone release - #2215
Conversation
…on branch collectSchemaDefaults recursed only when a nested object declared `properties` directly, so one keeping its fields on a composition branch was skipped — while SchemaForm rendered that branch and displayed its read-only discriminator. The constant was shown and never submitted, and the server rejected the call. The recursion now gates on declaresAnyFields. Signed-off-by: cliffhall <cliff@futurescale.com>
…ults per level
The App deep link seeded defaults and spread its `appArgs` over them, which
replaces a nested object wholesale: `{ config: { kind: "sms" } }` discarded
the nested SMS branch's own defaults, which the form then displayed while the
submitted arguments omitted them. New seedSchemaValues merges the two per
level, picking each nested branch from the nested values, and the supplied
value still wins wherever the two meet.
Signed-off-by: cliffhall <cliff@futurescale.com>
…ll forms, test cast
- matchesConstants checks own-property presence before reading a supplied
value, so an absent argument legally named `constructor` no longer reads the
inherited one and rules out every branch pinning that name.
- anyOfAdmitsNull asks `admitsNull` about each composition-free branch instead
of testing `type` alone, so a branch spelling its nullability as
`{ const: null }` or `{ nullable: true }` is recognized. Refusing composed
branches is what bounds the recursion.
- Drop a double cast from the malformed-applicator fixture.
Signed-off-by: cliffhall <cliff@futurescale.com>
I claimed ENGINE_SMOKES was the single home for "which smokes are engine-sensitive". It was not: npm run smoke still hard-coded the same three, so a fourth entry would have reached the Firefox and on-demand runs and silently skipped the DEFAULT Chromium run -- which is the one GitHub CI executes. The exact drift the list was introduced to remove, left in the one tier that matters most. npm run smoke now ends in smoke:web:chromium, which goes through the runner like the other two tiers. Two things fall out of that: - The Chromium tier passes its engine as a literal argument, so an ambient SMOKE_BROWSER can no longer redirect it. Without that, SMOKE_BROWSER=firefox npm run ci would have run Firefox twice and never exercised Chromium at all -- a gate silently testing something other than what it claims. - run-engine-smokes.test.mjs now asserts that no tier names an individual smoke, that both gated tiers name their engine explicitly, and that the on-demand entry point is the one that follows the environment. This is the durable half: a tier running fewer smokes is invisible at runtime, because it still passes. Mutation-verified -- restoring the old hard- coded chain fails the test. Docs updated to state the wider claim rather than the one that was true of only two tiers. Addresses the suppressed comment on Copilot review 5026921914 of #2133. Signed-off-by: cliffhall <cliff@futurescale.com>
A distinct `const` does not make `oneOf` alternatives mutually exclusive
unless the property carrying it is required: two branches pinning an OPTIONAL
`kind` both match `{}`, so arguments omitting it satisfy more than one
alternative — exactly what `oneOf` forbids. The property must now be required
by the root or by every branch.
Also makes `propertiesOf` total (an unreadable `properties` reads as empty,
with a separate predicate for the one caller that must tell them apart), which
removes a row of unreachable `?? {}` fallbacks, and covers the value/type
comparisons the const checks rest on.
Signed-off-by: cliffhall <cliff@futurescale.com>
…erable properties
- selectBranchIndex asks the supplied NAMES first and falls back to a lone
agreeing constant only when they settle nothing. A matching `const` does
not identify a branch while another candidate leaves that property unpinned:
`{ kind: "email", phone: "555" }` agrees with an email branch whose
`address` is missing while satisfying a phone branch outright.
- isOfferable requires every property VALUE to be readable — JSON Schema's
boolean form is fine, a `null` or an array is not a schema at all and the
web form dereferences one on the way to choosing a widget. Such a branch is
declined rather than handed on to crash a tool panel that would otherwise
have rendered.
Signed-off-by: cliffhall <cliff@futurescale.com>
…remedy Three findings, all from Copilot's suppressed block. 1. The tests never checked that loadBrowser launches the engine it was asked for. The failure-path stand-in rejects identically for every engine, so loadBrowser(root, "firefox") could have called chromium.launch() and still produced a correctly Firefox-labelled error. The smokes could not catch it either: the Firefox gate runs after Chromium is already installed, so a mis-dispatched launch would SUCCEED and report Firefox coverage that never happened. Added a success-path test with distinct per-engine spies asserting the requested engine is invoked, that nothing else is, and that its handle is what comes back. Mutation-verified: hard-coding playwright.chromium fails it. 2. The launch-failure message told the reader to run `npx playwright install --with-deps <engine>`, which is wrong now that the npm scripts run from the repo root rather than clients/web -- I introduced that when I removed the `cd`. Playwright is pinned in clients/web, so a bare root-level npx can fetch a different version and install a browser revision the pinned one still cannot launch. It now points at `npm run smoke:web:<engine>` first, and scopes the raw command to clients/web with the reason. That message named smoke:web:webkit, which did not exist -- only the chromium and firefox scripts did. Added it, and a test asserting every supported engine has one, since a remedy nobody can run is worse than no remedy. Mutation-verified. 3. AGENTS.md said npm run ci "mirrors" the workflow and then, two sentences later, that it is "a superset, not a mirror". It now leads with strict superset, which is the topology this PR is documenting. The README and AGENTS.md remediation wording is corrected the same way as the runtime message, so a reader gets the same advice wherever they find it. Addresses Copilot review 5030668906 on #2133. Signed-off-by: cliffhall <cliff@futurescale.com>
…es the branch JSON Schema's boolean form is legal, but only `true` is harmless: it constrains nothing, while `false` admits no value at all — so a field declared with it can never be filled, and a required one makes the whole branch unsatisfiable. Offering it contradicted every other faithfulness check in the resolver, so such a branch is declined. Signed-off-by: cliffhall <cliff@futurescale.com>
…yped discriminators
- anyOfAdmitsNull recognizes a branch that constrains nothing (`{}`, or
annotations only): it admits every value, null among them, so a field pinned
to `const: null` beside such a union stays submittable. `admitsNull`
cannot say that on its own — an unconstrained schema tells it nothing either
way and it declines rather than guesses — but here the question is narrower.
- convertToolParameters sends a matched `const` as the schema's own typed
value: `kind=2` selects a branch pinned to `const: 2` and was then sent
`"2"`, which that same branch rejects. Only an exact match is
substituted; anything else is the user's input and is left alone.
Signed-off-by: cliffhall <cliff@futurescale.com>
…ull-admitting union
`anyOfAdmitsNull` sent every non-object member to `toBranch` and refused it,
so `{ const: null, anyOf: [true] }` read as non-nullable — a required field
seeded and rendered read-only as `null` and reported missing forever. `true`
is the unconstrained schema and admits every value, `null` among them;
`false` admits none and still does not.
Signed-off-by: cliffhall <cliff@futurescale.com>
The README claimed both showcase tools rendered nothing but the Execute Tool button, which the PR's own screenshots contradict: `echo` carries a root `message` and rendered that one field, so it was callable with half its arguments; only `get_weather`, whose fields live entirely on its `oneOf`, rendered no controls at all. The two now show the two halves deliberately. Signed-off-by: cliffhall <cliff@futurescale.com>
smoke-web-elicitation.mjs told the reader to run `SMOKE_BROWSER=webkit npm run smoke:web:app` -- the App smoke, not the elicitation smoke the file documents. Following it exercises a different path entirely, and it does not fail while doing so: a real smoke runs and passes, so nothing signals that you never tested what you were reading about. This is a direct cost of my own decision two rounds ago to make the two App headers word-for-word identical so they could not drift. The shared PROSE should be identical; the example command is the one line that must not be, and copy-paste does not distinguish them. Both headers now name their own smoke, and both point at npm run smoke:web:webkit for all three. Guarded, since the class is invisible at runtime by construction: run-engine-smokes.test.mjs now asserts no smoke header tells the reader to run a sibling's command, and that every `npm run` it does name is a real script. Both mutation-verified -- restoring the exact bug fails the first, and naming a nonexistent script fails the second. Addresses the suppressed comment on Copilot review 5031065042 of #2133. Signed-off-by: cliffhall <cliff@futurescale.com>
…smokes' into v2/chore/2086-cross-browser-app-smokes
One conflict, in SchemaForm's `resetKey` reset: #2138 added the enlarge-caret Map to clear there while this branch added the union branch selection. Both belong in it, so the merged block clears the carets and re-derives the branch from the incoming values. Signed-off-by: cliffhall <cliff@futurescale.com>
…lers that do not Rendering every `const` read-only assumed the caller had already seeded it, which SchemaForm never did itself: `InlineElicitationRequest` passes its values straight through and gates submit on hasMissingRequiredFields, so a required `const` — a root union's discriminator among them — was displayed and never supplied, leaving Submit disabled with no way to fix it. The form now reports the fixed values upward once per entity, in an effect (the parent's `onChange` cannot be called during our render), adding only names absent from `values` — so a caller that already seeds sees no call at all and the report cannot loop. Signed-off-by: cliffhall <cliff@futurescale.com>
… types, core keywords - convertToolParameters defines its result keys rather than assigning them, so a discriminator legally named `__proto__` reaches the call instead of the prototype setter. It also collapses a nullable declaration before dispatching, so `type: ["number","null"]` coerces as a number rather than being sent as text. - typeNameOf sorts an array `type`: JSON Schema reads it as a SET, so `["number","null"]` and `["null","number"]` are one declaration and must not read as branches disagreeing. - nullableUnion's non-asserting keyword set covers `$defs`, `$id`, `$schema`, `$anchor`, `$dynamicAnchor` and `$vocabulary` — they identify or declare rather than assert. `$ref` stays out: it applies whatever it points at. Signed-off-by: cliffhall <cliff@futurescale.com>
…, enum-spelled null
- coercionProperties collapses each declaration before comparing types. A
nullable schema written as an `anyOf` states no top-level `type`, so
`number | null` and `boolean | null` both read as "no type" and counted
as agreeing — and `value=true` then came back as `NaN` through the first
branch's number.
- admitsNull reads an `enum` offering null, under the same sibling conditions
a `const: null` answers to. Without it `{ enum: [null] }` reported that it
rejects null, and a required read-only field spelled that way stayed
permanently invalid.
Signed-off-by: cliffhall <cliff@futurescale.com>
…cross a branch switch The carry filter dropped a name whose value was `undefined`, but clearing a number or JSON field leaves exactly that: the name present with no value, which is the user's answer rather than an absence. The incoming branch's defaults then put the field's default back and undid the clear. Own-property presence alone is now the test. Signed-off-by: cliffhall <cliff@futurescale.com>
…ired const controls
- A `const` may be an object or an array, with or without a `type`, and
`String({...})` is "[object Object]" — which no CLI argument can equal, so
the only value such a schema accepts never matched. Supplied text is parsed
and compared structurally through the resolver's own comparison (exported as
`sameJsonValue` so the two cannot disagree), for both branch selection and
the substitution.
- The TUI's one-option const control is never marked required: its option may
legitimately be the empty string, which ink-form's required gate can never
accept, so submission would not even reach `decodeFormValues`. The value is
fixed by the schema, and `missingRequiredFields` still validates the call.
Signed-off-by: cliffhall <cliff@futurescale.com>
…hange, label every primitive const - The seeding effect is keyed by the fixed values themselves as well as the entity and branch. A tool refreshed in place keeps its `resetKey` and its branch while its schema changes underneath, so a newly pinned field was rendered read-only and never seeded — leaving a required one permanently unsubmittable. The schema object's identity says nothing (callers rebuild it every render), which is why the key is derived from what has to be seeded. - branchLabel stringifies every primitive constant. `hasDiscriminator` accepts distinct booleans and nulls, so a `true`/`false` union was discriminated perfectly well and still labelled "Option 1"/"Option 2". Signed-off-by: cliffhall <cliff@futurescale.com>
…union changes underneath `branchIndex` is a position, and a tool refreshed in place keeps its `resetKey` — so a union reordered or rewritten with the same number of branches left the index pointing at a different shape than the one the values describe: the picker showed SMS while Execute submitted the email arguments. The selection is now re-derived from the values whenever the alternatives themselves change, keyed by their labels and declared fields. Signed-off-by: cliffhall <cliff@futurescale.com>
…wers, prefixed labels - The seeding effect re-applies constants as well as filling absences: an in-place schema change can move a `const` the user cannot edit, and the read-only field then displayed the new value while `values` still held the old one — which is what would be submitted. Ordinary defaults are still only ever added, so an edited field keeps what the user put there. - hasMissingIn checks own-property presence: a required argument legally named `constructor` resolved to the inherited one and read as supplied, enabling a submit the schema rejects. - The TUI keeps the declared name as a branch field's fallback title. The prefix is an internal field NAME, and buildFields labels from its map key — so the showcase displayed `__b0__address` where the schema says `address`. Signed-off-by: cliffhall <cliff@futurescale.com>
… types The cross-branch agreement check ignored `const`, so two branches pinned to `1` and `"1"` both stated no `type`, both matched the text `1`, and the first one's typed constant was sent. Textually indistinguishable constants of different types are ambiguous, and the raw string the user typed is the honest answer. Signed-off-by: cliffhall <cliff@futurescale.com>
…emas, cover the deep-link path - branchesKey is built from the resolved alternatives themselves, not from labels and field names: two branches can share both while pinning different discriminators or typing a field differently, and a reorder of those went unnoticed — the form then displayed one branch and submitted another. - InspectorView gains a deep-link test with root-union appArgs: they name the second branch, and the assertions pin the picker, the read-only discriminator, THAT branch's default (not the first branch's), and the absence of the other branch's field. The old shallow spread fails it. Signed-off-by: cliffhall <cliff@futurescale.com>
A schema may require a property it never declares: `{ type: "object",
required: ["token"] }` is legal and the tool plainly takes an argument.
declaresAnyFields counted only `properties`, so an App tool shaped that way —
including nested under a composition — was reported input-free and
auto-invoked with `{}`.
Signed-off-by: cliffhall <cliff@futurescale.com>
…ty, required-only branches
- `const` constrains a property that is PRESENT; it neither requires the
property nor acts as a default. It is now supplied automatically only where
the schema also requires the field — in `collectSchemaDefaults`,
`applySchemaConstants` and the TUI's initial values alike — so an optional
`dryRun: { const: true }` no longer turns a valid `{}` call into one that
asks the server to do something. A value that IS present is still corrected
to the constant, whatever supplied it, and the field stays read-only.
- Offerability is judged on the base+member merge rather than the raw member,
so `anyOf: [{ required: ["email"] }, { required: ["phone"] }]` over
root-declared properties is offered. It was declined, which left the gate
checking the base alone and accepting `{}` — which that schema rejects.
- The TUI's branch sections keep the schema's own `required` for the seeding
decision while still rendering every branch field non-required, which is what
keeps a static form submittable.
Signed-off-by: cliffhall <cliff@futurescale.com>
… into Making every pinned field read-only left an optional one unreachable: the schema is equally happy with or without it, but the form displayed a value the user could neither send nor edit. An optional `const` is a yes/no rather than a fixed answer, so it renders as its single choice, clearable — opting in sends the schema's own typed value, opting out leaves the property absent. A required one is still displayed read-only, since there is nothing to decide. Signed-off-by: cliffhall <cliff@futurescale.com>
There was a problem hiding this comment.
🟡 Changes recommended
The OAuth connection-details refresh can leave a rejected storage read unhandled.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 108/333 changed files
- Comments generated: 1
- Review effort level: Balanced
Review round 5 — responseOne finding, accepted. Filed against
This is a straight The correct shape is already used three times in the same file ( Beyond the panel: an unhandled rejection also fails the whole vitest run, surfacing in an unrelated test with a stack pointing at SDK internals. That is the #1947 experience and the reason the rule reads as it does. Running tally
All six are on the v2.6.0 milestone and boarded in Todo. Requesting another pass; every round has reported 108 of 333 changed files and every round has still turned up something new, so it is worth continuing while that holds. |
There was a problem hiding this comment.
🟡 Changes recommended
RFC 6749 Basic credentials are encoded with the wrong URL-encoding algorithm, which can make token revocation fail for valid client credentials.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 108/333 changed files
- Comments generated: 1
- Review effort level: Balanced
Review round 6 — responseOne finding, accepted with a correction to its worked example. Filed against
Correction on which character breaks. A space does not fail: The fixture point is the sharper half. Running tally
All seven are on the v2.6.0 milestone and boarded in Todo. Requesting another pass. |
There was a problem hiding this comment.
🔵 Needs a closer look
The 333-file milestone merge spans authentication, transport, React state, dependencies, test infrastructure, and release mechanics despite prior CI coverage.
Review details
- Files reviewed: 108/333 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
OAuth state can briefly leak across server switches, and some valid union schemas produce TUI forms that cannot be submitted.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 108/333 changed files
- Comments generated: 2
- Review effort level: Balanced
Review rounds 7–8 — responseRound 7 returned 0 comments. I requested one more anyway rather than reading that as the end, and round 8 found two — so the silence was not a signal.
On #2223 — how it passed the gate. On #2224 — it is the function's own argument, one case short. Final tally — nine issues, all on the v2.6.0 milestone, all boarded in Todo
Two findings were declined with reasons: rate-limiting the local test-server fixtures (CodeQL 73), and an "unterminated" backtick at Nothing in this PR changed across all eight rounds, deliberately. It is the v2.5.0 milestone merge; its tree is byte-identical to Requesting one more pass, since round 7's silence proved not to be the end. |
There was a problem hiding this comment.
🔵 Needs a closer look
Although the merged tree and required checks are green, this 333-file, 33-PR release payload requires final human release review.
Review details
- Files reviewed: 108/333 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The release aggregates 333 files across authentication, protocol, UI, and build infrastructure, warranting final human release review despite prior CI validation.
Review details
- Files reviewed: 108/333 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Review closed out — 10 rounds, no remaining commentsRounds 9 and 10 both returned 0 new comments with the same 🔵 "warrants final human release review" verdict. Round 7 also returned zero and round 8 then found two more, so I did not treat the first silence as the end — but two consecutive zero rounds after a productive one is the stopping point. Outcome33 findings triaged across 10 rounds. Nine filed, two declined, nothing changed in this PR.
All nine are on the v2.6.0 milestone, boarded in Todo, assigned. Declined with reasons: CodeQL 73 (rate-limiting the local test-server fixtures — no untrusted caller, and it would make several smokes flaky), and an "unterminated" backtick at Two findings are worth flagging to the human reviewer as more than routine, because both escaped the automated gates by construction:
State of this PR
Ready for human approval. |
v2.5.0 smoke pass — cleanSmoke-tested the whole v2.5.0 payload from a dedicated worktree at Result: 37 / 37 closed milestone issues verified, 0 regressions.
Two notes, neither blocking
Full row-by-row ledger — what was driven and what was actually observed, for every one of the 37 issues: 🤖 Generated with Claude Code |
References #2214
Fifth
v2/main→mainmilestone merge: the v2.5.0 payload — 33 PRs, 213 non-merge commits, including the version bump.Merge + release
git push origin mainis rejected:mainis ruleset-protected and no one can bypass.2.5.0→ Create new tag on publish, Target =main→ Publish.pack:verify, asserts the tag matchespackage.json, and publishes thelatestdist-tag.2.5.0, novprefix.References, notCloses.Version
No bump commit in this PR —
2.4.0 → 2.5.0landed onv2/mainfirst (#2212 / #2213) and arrives here as part of the payload, sov2/mainis never left behindmainafterwards. This is the #2010 procedure, now on its third consecutive release.Conflicts
None.
mainhas not moved since the v2.4.0 merge (#2145) — no Dependabot PR has landed on it in the interval — so its tip was still an ancestor of this branch's first parent and every path three-way merged clean.The merged tree is byte-identical to
origin/v2/main:git diff origin/v2/main HEADis empty, andgit diff --name-only --diff-filter=Ureported zero unmerged paths at merge time. This merge therefore introduces no content that has not already been through CI onv2/main— the verification below rests on that identity, and the PR's own required checks re-run it against the merged commit regardless.What's in it
useSessionRef— #2157), #2162 (useOAuthRecovery— #2153), #2166 (useMcpApps— #2156), #2169 (useConnectionLifecycle— #2154), #2173 (useServerCommands— #2155), #2182 (phase 3:InspectorView's prop wall into domain bundles — #2130), #2191 (the react-hooks findings the decomposition unmasked — #2161)core/react/witheslint-plugin-react-hooks— #2192), #2190 (terminate the five void-discarded OAuth recovery promises — #2165), #2179 (surface failed resource subscribe/unsubscribe — #2174)anyOf/oneOf— #2123)subscriptions/listenended, instead of retrying it eight times — #2097)smoke:tuiasserts survival, not first paint — #2147), #2167 (rename the local pre-push gate offci— #2146), #2160 (the coverage gate in a parallel CI job — #2159), #2178 + #2181 (the render-smoke startup budget, then anchoring its overlap paint to the parent's clock — #2177, #2180)Three milestone issues closed without a PR of their own — #2126, #2030 and #2031 are parent trackers whose sub-issues carried the work. #2132 (the WebKit SSE tail) closed with its cross-chunk
parseSSEhalf already shipped in v2.4.0 via #2135, the WebKit CI matrix entry deliberately left out.Verification
The merge is a pure fast-forward of content: the resulting tree equals
origin/v2/mainbyte for byte, andv2/main's own CI is green atfe35885b. Nothing in this PR is new code, so the gate's result onv2/maintransfers exactly; the required checks on this PR confirm it on the merge commit itself.🤖 Generated with Claude Code
https://claude.ai/code/session_017bussXiM47PBttWqFUkPN1