From 56d3e8f82ae9b33db556c799198420bb2213f334 Mon Sep 17 00:00:00 2001 From: Yichen Jiang Date: Wed, 26 Aug 2026 14:42:39 +0800 Subject: [PATCH] fix(agent-presets): answer health from the walk alone, and keep the reason reachable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `import.meta.resolve`'s `parentURL` argument takes effect only under `--experimental-import-meta-resolve`, which no launch passes, so the fallback resolved from this module rather than from the harness — the one question it existed to answer. The disk walk is the whole answer now, and the refusal memo it needed goes with it. A `file:` URL joins the file branch rather than the package one, where a resolver would only normalize it and report a missing target as present, and a row is skipped on the Loader's own `Boolean(disabled)` so `disabled: 0` is checked like the Loader checks it. A broken card says so through `aria-disabled` rather than `disabled`, and refuses the pick in its own handler. `disabled` took it out of the tab order, which with the reason moved onto the badge left it unreachable without a pointer — reachable before this change, so hiding it was a regression rather than a path that never existed. Both notes this decision partly supersedes are updated in place and cross-linked, one README pair loses an editing residue that repeated a sentence, and the single-row diagnostic no longer reads "row row 1". --- ...8-26-preset-health-resolves-rows.i18n.yaml | 4 +- .../2026-08-26-preset-health-resolves-rows.md | 12 +-- ...26-08-26-preset-health-resolves-rows.zh.md | 12 +-- ...-08-09-broken-preset-roster-rows.i18n.yaml | 4 +- .../2026-08-09-broken-preset-roster-rows.md | 4 +- ...2026-08-09-broken-preset-roster-rows.zh.md | 4 +- ...plugin-owned-shipped-preset-root.i18n.yaml | 4 +- ...-08-20-plugin-owned-shipped-preset-root.md | 2 +- ...-20-plugin-owned-shipped-preset-root.zh.md | 2 +- apps/web/tests/agent-preset-selection.e2e.ts | 2 +- .../client/ui-agent-preset/README.i18n.yaml | 4 +- packages/client/ui-agent-preset/README.md | 2 +- packages/client/ui-agent-preset/README.zh.md | 2 +- .../src/client/AgentPresetSeat.tsx | 13 +-- .../src/client/AgentPresetSection.module.css | 25 ++++-- .../src/client/AgentPresetSection.tsx | 19 +++-- .../tests/section.client.spec.tsx | 6 +- .../client/ui-primitives/README.i18n.yaml | 4 +- packages/client/ui-primitives/README.md | 2 +- packages/client/ui-primitives/README.zh.md | 2 +- .../preset/agent-presets/src/discovery.ts | 85 +++++++------------ .../preset/agent-presets/src/specifier.ts | 10 ++- .../agent-presets/tests/discovery.spec.ts | 18 +++- 23 files changed, 130 insertions(+), 112 deletions(-) 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 index 48dbb0f806..0d92211439 100644 --- 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 @@ -2,5 +2,5 @@ # 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: 1615b2b86bee27a282357e1d5f023797672cb9b5 -2026-08-26-preset-health-resolves-rows.zh.md: ea287ae8204aa1730d80199b0f4e86e3cf7b42a3 +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 index 1615b2b86b..909423e373 100644 --- 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 @@ -8,6 +8,8 @@ English | [中文](2026-08-26-preset-health-resolves-rows.zh.md) 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. @@ -16,11 +18,11 @@ The failure it did produce named less than it knew. The Loader's per-row wrapper ## 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` — and only a name that finds nothing there is confirmed through `import.meta.resolve`, whose refusals are then remembered for the process. 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. +**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 is the fast path because the resolver is not one: a registered ESM loader hook turns every `import.meta.resolve` call into a synchronous round-trip to the hooks thread. Under the `tsx` hook the source launch installs, that measured 2ms for a hit and 5ms for a miss against 0.055ms and 0.032ms on bare Node, which put 238ms of resolver time into each roster read. The walk answers the same 135 rows in 0.7ms. Keeping the resolver for names the walk cannot find leaves a read paying for the failures it reports rather than for every row it clears, and keeps a package only a loader can resolve — through tsconfig paths, or an import map — from being called broken. A Node builtin short-circuits ahead of both. +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. -The refusal memo sits behind the walk, never in front of it, so a package installed since is found on disk and a recorded refusal cannot go stale in the direction that matters. +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. @@ -30,7 +32,7 @@ The refusal memo sits behind the walk, never in front of it, so a package instal **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 the badge's tooltip for a pointer, and a visually hidden `role="alert"` node carries it to assistive technology — the card body is disabled when a preset is broken, so it leaves the tab order and the tooltip has no keyboard path. +**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. @@ -58,7 +60,7 @@ The wire already separated the two texts this needs: `message` wraps the cause i **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 a focusable control so the tooltip has a keyboard path.** Rejected for now. The badge sits inside the card's own `