diff --git a/.agents/notes/implemented/architecture/2026-08-26-preset-health-resolves-rows.i18n.yaml b/.agents/notes/implemented/architecture/2026-08-26-preset-health-resolves-rows.i18n.yaml new file mode 100644 index 0000000000..0d92211439 --- /dev/null +++ b/.agents/notes/implemented/architecture/2026-08-26-preset-health-resolves-rows.i18n.yaml @@ -0,0 +1,6 @@ +# Bilingual-pair consistency record (docs/i18n/README.md): the git blob hash of each +# side as of the last confirmed-consistent state. Both languages carry equal authority; +# after editing either side, bring the other along and re-record with: +# pnpm run verify-translation-pairing --write .agents/notes/implemented/architecture/2026-08-26-preset-health-resolves-rows.md +2026-08-26-preset-health-resolves-rows.md: 909423e373bc2368b5fb1e81b1172275920282aa +2026-08-26-preset-health-resolves-rows.zh.md: d2cffae66f7bd7fb78e068bd4242d009b95d3c5c diff --git a/.agents/notes/implemented/architecture/2026-08-26-preset-health-resolves-rows.md b/.agents/notes/implemented/architecture/2026-08-26-preset-health-resolves-rows.md new file mode 100644 index 0000000000..909423e373 --- /dev/null +++ b/.agents/notes/implemented/architecture/2026-08-26-preset-health-resolves-rows.md @@ -0,0 +1,75 @@ +# Agent Note: Preset health resolves the rows it can prove will start + +Status: implemented + +English | [中文](2026-08-26-preset-health-resolves-rows.zh.md) + +## Problem + +A preset the roster listed as healthy could still be impossible to compose. Discovery's health check proved the composition parsed in the loader dialect and held named rows, and deliberately stopped there — it resolved no plugin name and applied no config. + +This note partly supersedes [broken presets are roster rows](../bug-fix/2026-08-09-broken-preset-roster-rows.md), whose rejected "validating deep" alternative is what shipped here, and it moved the reason off the card face; it also relaxes the shipped-roster assertion [plugin-owned shipped preset root](../bug-fix/2026-08-20-plugin-owned-shipped-preset-root.md) records. Both are updated in place. + +`broken` is load-bearing, though, not a card decoration. `presetOptions` drops a broken row from the session pickers so a chooser never defers the discovery to a failed session start, and `resolveMountable` refuses one before spending a mount. Everything downstream therefore reads "not broken" as "will compose". + +The gap surfaced when the [repository naming contract](2026-08-11-repository-naming-contract-and-rename-ledger.md) renamed packages under the pre-release stance. In-repo references moved with it; a preset authored under `/.agent-presets` did not, and one naming `@deepseek-ai/dsh-workspace-context` kept its healthy card, kept its place in the picker, and failed only when a person switched to it. A row naming a package a later release renamed or uninstalled is how an authored preset actually rots, and it was exactly the class the check excluded. + +The failure it did produce named less than it knew. The Loader's per-row wrapper builds a plain `Error` whose message ends with `cause.message` and keeps the cause only as `error.cause`. A group that fails on two rows therefore arrives as one wrapped row whose message is `failed to apply loader entry (cordis:group): loader entries failed to apply`, with the two real reasons reachable through `cause.errors` alone. The mount diagnostic flattened `AggregateError.errors` and never followed `cause`, so it ended at that line and named neither row. + +## Decision + +**Discovery resolves each row it can prove will start, and imports nothing.** The resolve pass runs after the shape check in `packages/preset/agent-presets/src/discovery.ts`, so a malformed composition still answers with the shape reason. A package name is looked up on disk — Node's own upward `node_modules` walk, stopping at `/package.json`. A preset-relative or absolute specifier is statted instead, because `import.meta.resolve` only joins URLs for those and a preset shipping a file that was deleted would otherwise pass. Nothing is evaluated either way. + +The disk lookup, not `import.meta.resolve`, for two reasons. It is the cheap one: a registered ESM loader hook turns every resolver call into a synchronous round-trip to the hooks thread, measured under the `tsx` hook the source launch installs at 2ms for a hit and 5ms for a miss against 0.055ms and 0.032ms on bare Node — 238ms of resolver time per roster read, where the walk answers the same 135 rows in 0.7ms. It is also the only one that can be asked about the harness at all: `import.meta.resolve`'s `parentURL` argument takes effect only under `--experimental-import-meta-resolve`, which no launch passes, so it resolves from the calling module and would answer about this package rather than about the deployment. The resolver that does honour an explicit parent is the Loader's internal one, whose `resolveSync` signature differs between Node 22 and 24. A Node builtin short-circuits ahead of the walk. + +What the walk gives up: a package resolvable only through a loader hook — an import map, or a tree with no `node_modules` — is reported broken. No supported install produces one, because `dsh plugin install` puts every plugin beside the roster. + +**One classifier decides where a row resolves.** `src/specifier.ts` owns the split — `cordis:` builtin, preset-relative, absolute file, package name — and both the mount's import override and discovery's check read it. A row discovery resolved from one base while the mount imported it from another would be reported healthy and then fail to load. + +**A row that may never start is skipped.** `disabled` is the one entry field the [Loader interpolates](2026-08-11-loader-entry-disabled-interpolation.md): a `!!js` expression evaluates against the loader context at mount time, which discovery cannot do from a file. A row carrying anything but an absent, null, or `false` value is left unchecked, and a disabled group takes its children with it. Every shipped preset gates its shell rows this way, so this is the common shape, not a corner. + +**The harness base is a required argument.** `discoverPresets(roots, harnessBase)` and `scanRoot(root, harnessBase)` take it; `AgentPresets` reads `ctx.baseUrl` once in its constructor and throws when it is absent. The base is what makes the question answerable at all — the same package name fails from a preset's own directory and resolves from the installed harness — so an optional one would silently restore the state this check exists to end. + +**The mount diagnostic follows a cause that carries more than its message.** `mountDetail` reads branches from `AggregateError.errors`, or from `error.cause.errors` when the cause is an `AggregateError`; a plain cause chain is already flattened into the message and is not followed, which would print every line twice. Nested branches indent under the row that owns them. + +**The client puts the reason on the badge.** The card face keeps the preset's own description, because a package specifier tells a chooser nothing they can act on there. The host's reason is revealed by hovering the badge or focusing the card, and a visually hidden `role="alert"` node announces it. A broken card says so through `aria-disabled` rather than `disabled` and refuses the pick in its own handler: `disabled` would take it out of the tab order, and with the reason off the face that would leave anyone without a pointer unable to reach it at all. + +**A refused switch says why, where it was refused.** The chip's own label reverts to the preset the session still runs, so without a word the pick simply appears not to have happened. It announces through the shared `Toast`, over the composer column, the way the model picker beside it already reports a rejected selection. Only a pick a person just made is announced — the applier also runs when a session becomes current, and a banner over that would report a refusal nobody asked for. The banner holds for eight seconds rather than the primitive's three, because it carries a cause that names packages and rows; `Toast` gained a `holdMs` for that, which also retired the hazard of a hold constant the stylesheet had to be kept in step with by hand. + +The wire already separated the two texts this needs: `message` wraps the cause in the roster's own "preset X failed to mount" frame, while `details.reason` holds the cause alone. A surface that names the preset itself takes the second, or it says the preset twice. + +## Alternatives considered + +**Check when a preset is selected rather than when the roster is listed.** Rejected. The pickers filter on `broken` before anyone selects, so a preset only checked at selection is still offered, and the reported failure still arrives after the click — the original complaint, relocated. The roster row is where every consumer already reads the verdict. + +**Keep the base optional and skip the check without one.** Rejected. Its failure mode is precisely the bug being fixed, delivered with no signal: healthy cards for presets that cannot compose. `ctx.baseUrl` is set on the root before any scoped context derives from it, so the throw is an assertion about something that does not happen rather than a branch with runtime cost. + +**Import each row instead of resolving it.** Rejected. Importing runs module top-level code on every roster read, which is a side effect a picker must not have, and it is the mount's job — a plugin that throws on apply or waits forever for a service still fails at the first session, by design. + +**Resolve every row through `import.meta.resolve`.** Shipped first and reverted on measurement: correct, and 445ms per roster read, which the client's three concurrent reads turned into 2.45 seconds apiece — the settings section visibly stalled. The resolver is the authority on what imports, but asking it about rows that are plainly installed pays a hooks-thread round-trip for each one. + +**Cache the whole of `compositionProblem` on the existing `CompositionStamp`.** Rejected as the answer to the cost: it would have made repeat reads free while leaving the first read of every edited composition at full price, and it keys resolution on the composition file, which does not change when an install does. The walk removed the cost instead, so nothing needs the stamp. + +**Send the switch failure to the roster card instead of a banner.** Rejected: the card is exactly where the failures that reach a mount are invisible. A composition whose rows all resolve is reported healthy, so "see the settings page for the reason" points at a card that says the preset is fine. + +**Report only the first unresolvable row, matching the shape check.** Rejected. A parse failure can cascade, so naming one is honest there; unresolvable names are independent facts all knowable at once, and reporting them one reload at a time is the avoidable part. + +**Follow `error.cause` unconditionally in `mountDetail`.** Rejected. The Loader's wrapper already appends `cause.message` to the message it builds, so a plain chain would render every line twice. An `AggregateError` cause is the one shape whose detail the message drops. + +**Keep rendering the reason on the card face.** Rejected. The reason names package specifiers and paths, and a picker card that shows them in place of the preset's description trades what a chooser needs for what a fixer needs — while the fixer's copy is one hover away either way. + +**Reuse the icon row's `data-tip` pseudo-element for the tooltip.** Rejected once measured: generated content joins an element's accessible text, so the card's aria snapshot grew a second verbatim copy of a reason the alert already carried. A real `aria-hidden` element keeps exactly one accessible copy — and the existing tooltip is one `nowrap` line sized for an icon label, while this one names package specifiers one per line. + +**Make the badge itself the focusable control.** Rejected: the badge sits inside the card's own ` + <> + { setOpen(false) }} + items={state.options.map((option) => { + const text = presetDisplayText(option, t) + return { + id: option.id, + // Name and description together: the id alone never says what a + // preset does, which is why the roster carries display copy. + label: ( + + {text.name} + {text.description ?? t('noDescription')} + + ), + } + })} + selectedId={state.current} + onSelect={(id) => { + setOpen(false) + const picked = state.options.find(option => option.id === id) + // The fallback is for the row shape `find` cannot promise; the menu's + // items ARE `state.options`, so an emitted id is always one of them. + /* v8 ignore next */ + const name = picked === undefined ? id : presetDisplayText(picked, t).name + void select(id).then((refusal) => { + // Announced only for a pick a person just made: `apply()` also runs + // when a session becomes current, and a banner over that would + // report a refusal nobody asked for. + if (refusal === undefined) return + toastSeq.current += 1 + setToast({ seq: toastSeq.current, text: t('switchRefused', { name, reason: refusal }) }) + }) + }} + align="start" + portal + anchor={( + + )} + /> + {toast !== null && ( + } + holdMs={REFUSAL_HOLD_MS} + // The composer card, which is the content column this chip sits + // above rather than inside — hence a page query, not `closest`. + // Absent, the banner centers on the window, which is off-center + // whenever the sidebar is open. + anchor={document.querySelector('[data-composer-card]')} + onDone={() => { setToast(null) }} + /> )} - /> + ) } diff --git a/packages/client/ui-agent-preset/src/client/AgentPresetSection.module.css b/packages/client/ui-agent-preset/src/client/AgentPresetSection.module.css index 0a8d2fa8a3..3d6043fdfe 100644 --- a/packages/client/ui-agent-preset/src/client/AgentPresetSection.module.css +++ b/packages/client/ui-agent-preset/src/client/AgentPresetSection.module.css @@ -94,12 +94,62 @@ color: var(--dsw-alias-bg-layer-3); } -/* The discovery-reported reason, verbatim: it names the file and the fix. */ -.cardBrokenReason { - font-size: 12px; +/* The discovery-reported reason, verbatim: it names the rows and the fix. + A real element rather than the icon row's `data-tip` pseudo-element, for two + reasons: generated content joins the card's accessible text, where this would + repeat what the alert already carries, and that tooltip is one `nowrap` line + for an icon label while this names package specifiers one per line. */ +.brokenTip { + position: absolute; + top: calc(100% + 6px); + left: 0; + z-index: 1; + width: max-content; + max-width: 100%; + padding: 6px 8px; + border-radius: 6px; + background: var(--dsw-alias-label-primary); + color: var(--dsw-alias-bg-layer-3); + font-size: 11px; + font-weight: 400; line-height: 1.5; - color: var(--dsw-alias-state-error-primary); + text-align: left; + white-space: pre-line; overflow-wrap: anywhere; + opacity: 0; + pointer-events: none; + transition: opacity .12s; +} + +/* Focusing the card reveals it too: a broken card keeps its place in the tab + order (`aria-disabled`, not `disabled`), so this is the keyboard path to a + reason the card face no longer shows. */ +.brokenBadge:hover .brokenTip, +.cardMain:focus-visible .brokenTip { + opacity: 1; +} + +.cardMain[aria-disabled='true'] { + cursor: default; +} + +.cardMain:focus-visible { + outline: 2px solid var(--dsw-alias-brand-primary); + outline-offset: -2px; +} + +/* The same reason, for a screen reader. A pointer reveals the badge's tooltip + and a keyboard reveals it by focusing the card, but neither announces it; + this node is what does. Keeping it out of the card face is what lets a + picker card show the preset's own description rather than a package + specifier. */ +.cardBrokenReason { + position: absolute; + width: 1px; + height: 1px; + overflow: hidden; + clip: rect(0 0 0 0); + white-space: nowrap; } /* The card body is the control that picks the preset. */ @@ -132,6 +182,11 @@ display: flex; align-items: center; gap: 8px; + /* Anchors the broken badge's tooltip: the badge itself stays unpositioned so + its `::after` resolves against the card's own width instead of against a + badge that sits partway across it. A tooltip grown from the badge would + run past the card, and past the section for a card in the last column. */ + position: relative; } .cardName { diff --git a/packages/client/ui-agent-preset/src/client/AgentPresetSection.tsx b/packages/client/ui-agent-preset/src/client/AgentPresetSection.tsx index 59ccb19226..6ec76eea14 100644 --- a/packages/client/ui-agent-preset/src/client/AgentPresetSection.tsx +++ b/packages/client/ui-agent-preset/src/client/AgentPresetSection.tsx @@ -256,23 +256,45 @@ export function AgentPresetSection(props: AgentPresetSectionProps): ReactNode { common act, so it should not hide behind a small button. The action row sits outside it — nesting buttons is invalid, and these act on the card rather than select it. - A broken preset cannot compose a session, so its body is - disabled and the card says why instead of offering it. */} + A broken preset cannot compose a session, so its body + refuses the pick; the reason rides the badge rather than + the card face, which stays the preset's own + description. */}