Files
open-design/apps/daemon/src/skills.ts
98bc6d63e6 feat: Critique Theater wireup (activate the stack, M0 dark-launch by default) (#1338)
* feat(web): pure reducer for Critique Theater states (Phase 7.1)

Pure CritiqueState reducer driven by the contracts-level PanelEvent
(the same shape both the live SSE stream and the recorded transcript
emit), so a single reducer powers both the in-flight panel and the
rerun replay. Lifecycle covers run_started → running → (shipped /
degraded / interrupted / failed), with panelist_open / dim /
must_fix / close / round_end events building per-round
CritiquePanelistView entries as they arrive.

Defensive behaviour that surfaced while writing the spec tests:
- Terminal phases (shipped / degraded / interrupted / failed) are
  sticky against further lifecycle events for the same run, except
  for parser_warning which can land late and is recorded in a side
  channel without changing phase.
- A new run_started for a different runId at any time discards the
  prior state and reboots, so the UI can launch consecutive runs
  without an explicit reset action.
- Events whose runId does not match the active run return the same
  state reference, so React's useReducer doesn't re-render
  subscribers on stray traffic.
- Round bookkeeping keys by round number rather than "always last",
  so an out-of-order panelist_dim for round 1 arriving after a
  round 2 dim does not corrupt the round 2 bucket.

Test coverage: 18 cases covering each transition, the runId guard,
sticky-terminal behaviour, the out-of-order round invariant, and
the stable-identity guarantee. Sets up Phase 7.2 and 7.3 to wire
SSE + replay into the same reducer.

* feat(web): useCritiqueStream hook subscribes to SSE and feeds reducer (Phase 7.2)

createCritiqueEventsConnection is a pure connection manager that
mirrors apps/web/src/providers/project-events.ts: opens an
EventSource at /api/projects/:id/events, listens for every name in
CRITIQUE_SSE_EVENT_NAMES, decodes each frame back into a PanelEvent
(stripping the critique. prefix and merging the data payload), and
hands it to the caller's onEvent. Reconnect uses exponential
backoff (1s → 30s) and resets on `ready`; malformed payloads drop
with a dev-mode warning rather than tearing the stream.

useCritiqueStream wraps the manager in a useReducer that owns the
CritiqueState. enabled=false or a null projectId tears down the
connection cleanly; switching projectId closes the old connection
and opens a fresh one. The returned dispatch lets local UI
synthesise actions (e.g. an Esc keypress firing a synthetic
interrupted while a kill request is in flight); production traffic
comes from the SSE stream.

Test coverage:
- sse.test.ts (10 cases, node env): subscription set covers every
  CRITIQUE_SSE_EVENT_NAMES channel; payload decoding lifts the wire
  shape back to PanelEvent; malformed JSON is swallowed and does
  not stop the stream; exponential backoff schedule and ready-reset
  semantics are pinned with a setTimeout seam; close() cancels
  pending reconnects and shuts the live source; no-op fallback
  when EventSource is unavailable.
- useCritiqueStream.test.tsx (6 cases, jsdom env): idle pre-event,
  reducer driven by synthetic actions, no connection when disabled
  or projectId is null, clean close on unmount, projectId change
  reopens cleanly.

* feat(web): useCritiqueReplay hook drives reducer from transcript file (Phase 7.3)

Fetches the per-run NDJSON transcript (one PanelEvent per line),
parses every line via the shared isPanelEvent predicate, and
dispatches into the same CritiqueState reducer the live SSE stream
uses. A single reducer means the UI rendering a replay can be
identical to the live panel, and a UI mounting both
useCritiqueStream and useCritiqueReplay in parallel does not have
to reconcile two state shapes.

speed knob is `paused | instant | live | { intervalMs: N }`.
- instant flushes every event synchronously, useful for opening a
  finished run already at its terminal state.
- intervalMs paces dispatches at a fixed cadence so the reviewer
  can watch the run unfold.
- paused parses the transcript but holds events back until the
  caller advances speed (consumers can drive a scrubber later).
- live is reserved for the future "playback at original cadence"
  feature, currently treated as instant; replay timestamps are not
  yet persisted with each event so honest pacing requires a
  follow-up Phase 7+ task.

gunzip seam handles `.ndjson.gz` transcripts via
DecompressionStream when present; the production fetch path picks
between text and arrayBuffer based on the URL extension. Both seams
are injectable so the unit tests don't need to spin up a real
network or a real gzip pipeline.

Test coverage (8 cases, jsdom env):
- Idle status before any URL is provided.
- speed=instant flushes the full transcript synchronously to
  shipped state.
- speed={intervalMs:N} paces with the setTimeout seam, reaching
  done after the last tick.
- speed=paused leaves status=playing with no dispatches.
- Empty transcript reports done with state still idle.
- Fetch rejection surfaces an error status with the message.
- Malformed NDJSON lines are skipped; valid events around them
  still land.
- .gz transcripts route through the gunzip seam.

Closes the Phase 7 plan tasks 7.1 / 7.2 / 7.3 (reducer + stream +
replay), all on one branch ready for review. Phases 8+ (Theater
components) consume these from this PR.

* fix(web): close payload-override gap + paused-resume bug in Critique Theater hooks (Phase 7 review)

Two P1 fixes from lefarcen's review on PR #1307:

SSE payload override

`sseToPanelEvent` previously spread `data` after the channel-derived
`type`, so a payload-provided `type` could override the channel and
route a `critique.run_started` frame into the reducer as a `ship`
action. Reversed the spread so the channel-derived `type` is
authoritative, and revalidated the resulting object through the
contracts-level `isPanelEvent` predicate before returning. Frames
that fail validation (missing runId, empty runId, unknown type) are
dropped, so a malformed or compromised SSE frame can no longer
dispatch a wrong-shape action into the reducer.

Three new sse.test.ts cases pin the regression: hostile `type:'ship'`
in the payload still resolves to `run_started`, missing runId is
dropped, empty runId is dropped.

Replay pause/resume

`useCritiqueReplay` had one big effect keyed on `transcriptUrl`
only, so flipping `speed` from `paused` to `instant` never re-fired
and the held events sat undispatched. Split into a parse effect
(depends on URL, fetches and stores events in state) and a pace
effect (depends on parsed-events + speed, owns the cursor + timers).
The playback cursor lives in a ref that survives pause/resume
cycles, so flipping `paused` -> `instant` flushes from the current
position rather than restarting (which would double-dispatch
`run_started` and reset the reducer).

Two new useCritiqueReplay.test.tsx cases:
- paused-then-instant transitions from `playing` to `done` and
  reaches the shipped terminal phase
- intervalMs paced playback dispatches one event, pauses to drain
  the next scheduled timer, flips to instant, and confirms the
  remaining transcript drains exactly once (cursor was preserved)

Doc consistency

The earlier source comment in useCritiqueReplay.ts claimed `live`
"paces by recorded timestamps" while the impl used zero-delay
timers and the PR body said it behaves like `instant`. Aligned to
reality: `live` currently behaves like `{ intervalMs: 0 }` (events
drain on successive microtasks via setTimeoutFn) because transcripts
do not yet carry per-event timestamps. Honest timestamp-driven
pacing is queued as a Phase 7+ follow-up.

Validated: pnpm guard, pnpm --filter @open-design/web typecheck,
Theater suite 47/47 (up from 42, +3 sse + 2 replay), full web suite
96 files / 888 tests.

* feat(i18n): seed Critique Theater key block (en + zh-CN; other locales fall back via spread)

* feat(web): Theater PanelistLane component (Phase 8.1)

* feat(web): Theater ScoreTicker component (Phase 8.2)

* feat(web): Theater RoundDivider component (Phase 8.3)

* feat(web): Theater InterruptButton component with Escape keybind (Phase 8.4)

* feat(web): Theater TheaterDegraded chip (Phase 8.5)

* feat(web): Theater TheaterCollapsed post-run summary (Phase 8.6)

* feat(web): Theater TheaterTranscript replay surface (Phase 8.7)

* feat(web): Theater TheaterStage top-level container (Phase 8.8)

* feat(web): Theater CSS using existing semantic tokens (no hex literals)

* feat(web): Theater public exports barrel

* fix(web): resolve P2 + P3 review feedback on Phase 8 (PR #1314)

Addresses all 4 P2 + 3 P3 items from codex, Siri-Ray, and lefarcen.

State-lifecycle fixes (3 x P2)
1. Reducer learns a synthetic `__reset__` action (`CritiqueResetAction`).
   Host hooks dispatch it when their gating prop changes so a stale
   run from a prior project / transcript cannot bleed into the next
   context. Reset is idempotent on idle (returns the same reference).
2. `useCritiqueStream` dispatches `__reset__` at the top of its
   connection effect, so a workspace switch from project A (which
   streamed a critique) to project B clears the reducer before the
   new EventSource opens. enabled=false also clears.
3. `useCritiqueReplay` dispatches `__reset__` at the top of its
   parse effect, so transcriptUrl swaps (including swap-to-null after
   a replay reached `shipped`) lift the reducer back to idle before
   the new fetch starts.

SSE validation (1 x P2)
4. `sseToPanelEvent` now runs a per-variant `hasValidVariantShape`
   check after the cheap `isPanelEvent` predicate. A
   `critique.ship` frame missing `composite` / `round` / `status` /
   `artifactRef` is rejected before reaching the reducer, so
   TheaterCollapsed can no longer crash on `undefined.toFixed(1)`.
   Every variant's required fields are validated: run_started
   (protocolVersion, non-empty cast, maxRounds, threshold, scale),
   panelist_* (round, role, plus variant-specific shape), round_end
   (round, composite, mustFix, decision in {continue,ship}, reason),
   ship (round, composite, status, artifactRef.{projectId,artifactId},
   summary), degraded (reason, adapter), interrupted (bestRound,
   composite), failed (cause), parser_warning (kind, position).

Reducer correctness (1 x P2)
5. `panelist_open` now materializes the round + an empty panelist
   view (`{dims: [], mustFixes: []}`) so TheaterStage can highlight
   the in-progress lane the instant the tag opens. Before this, a
   stream that emitted only `panelist_open` after `run_started` left
   `rounds = []` and the UI rendered no current round until a later
   `panelist_dim` arrived.

Polish (3 x P3)
6. Brand role tint swaps from `var(--magenta, var(--accent))` to
   `var(--purple, var(--accent))`. `--purple` is actually defined
   across the design systems; `--magenta` is not, so Brand was
   silently falling through to `--accent` and looking identical to
   Designer.
7. New i18n key `critiqueTheater.interruptedSummary` for the
   interrupted-collapse copy ("Interrupted at round N, best
   composite X.X"). Previously the interrupted branch reused
   `shippedSummary` and the UI read "Shipped at round..." for a run
   that specifically did not ship. Native value in en + zh-CN; other
   locales fall back via `...en` spread.
8. `TheaterDegraded` heading id comes from `useId()` instead of a
   hardcoded `theater-degraded-heading`, so two chips rendered on
   the same page (chat history with multiple completed runs) keep
   their aria-labelledby references unambiguous.

Tests (15 new cases)
- reducer.test.ts (+5): __reset__ on running/terminal/idle, panelist_open materializes round, panelist_open does not stomp prior panelist data.
- sse.test.ts (+6): variant-level rejection for ship without required fields, degraded without adapter, run_started with empty cast, panelist_dim with non-numeric score, round_end with unknown decision, plus a positive fully-formed ship.
- useCritiqueStream.test.tsx (+2): state reset on projectId change, state reset on enabled flip false.
- useCritiqueReplay.test.tsx (+1): state reset on transcriptUrl swap to null after a replay reached shipped.
- TheaterCollapsed.test.tsx (text-pinning update): asserts the interrupted branch reads "Interrupted at round 1" + "best composite 7.9", and explicitly NOT "Shipped at round...".
- TheaterDegraded.test.tsx (+1): two chips on the same page get unique aria-labelledby ids that each resolve to an `<h3>`.

Validated
- pnpm guard clean
- pnpm --filter @open-design/web typecheck clean
- Theater suite: 13 files, 101 tests (was 86 on the first Phase 8 push, +15 new)
- tests/i18n/locales.test.ts 5 of 5 across 18 locales

* feat(web): CritiqueTheaterMount wires SSE + reducer into a single drop-in (Phase 9.1)

* feat(i18n): Critique Theater strings for de + ja + ko + zh-TW (Phase 9.2)

* fix(web): resolve P1 + P2 review feedback on Phase 9 (PR #1315)

Addresses every blocker from codex, Siri-Ray, and lefarcen. The
three state-lifecycle and SSE-validation issues they also flagged
inherit fixes from PR #1314's review pass that this branch now sits
on top of after rebase.

Real daemon kill on Interrupt (P1)
- CritiqueTheaterMount now POSTs to
  /api/projects/:id/critique/:runId/interrupt alongside the
  optimistic local dispatch. Before this fix, clicking Interrupt
  only flipped the React state to interrupted while the daemon job
  kept running. The fetch is best-effort: a 404 (endpoint not wired
  yet, lands in Phase 15) is swallowed with a dev-mode console.warn
  so the UI still moves to the collapsed badge.
- New fetchInterrupt test seam lets RTL assert on the URL / method
  and simulate the "daemon not ready yet" path. Two tests pin both:
  the happy URL proj-42/critique/run-abc/interrupt POSTs, and a
  rejected fetch still flips the UI.

interruptPending reset on new run (P2)
- A ref-backed effect compares the current runId against the last
  one we saw; when it changes, interruptPending is cleared. A user
  who interrupts run-1 and then triggers run-2 from the same mount
  now gets a fresh, enabled kill button instead of one stuck in
  "Interrupting…". Pinned by a new mount test.

Escape keybind scope (P2)
- InterruptButton now checks the keydown target. Escape inside an
  input, textarea, select, or contenteditable element is ignored
  (and any ancestor of those via closest() is treated the same
  way). Body-level focus still fires the keybind so the Theater
  area's affordance keeps working. Four new tests cover textarea,
  input, contenteditable, and the body-focus positive case.

userFacingName i18n key (P2)
- The spec at specs/current/critique-theater.md:6 mandates a single
  critiqueTheater.userFacingName key so the "Design Jury" label can
  be renamed without touching code. Phase 8 introduced
  critiqueTheater.title by mistake; renamed across types.ts, en.ts,
  zh-CN.ts, de.ts, ja.ts, ko.ts, zh-TW.ts, and the lone consumer
  TheaterStage.tsx. The locale alignment test stays green.

Validated
- pnpm guard clean
- pnpm --filter @open-design/web typecheck clean
- Theater suite: 14 files, 112 tests (was 101 before, +11 new for
  the Phase 9 review pass: 3 mount + 4 InterruptButton focus scope;
  the rest were already in #1314's review fix).
- tests/i18n/locales.test.ts 5 of 5 across 18 locales.

* feat(daemon): adapter-degraded registry with TTL (Phase 10.1)

In-memory registry recording adapters that produced malformed or
oversize transcripts so the orchestrator can skip them for a TTL
window (default 24h) instead of cycling through known-bad providers
on every run.

Records carry reason (malformed_block | oversize_block |
missing_artifact), source label, and expiresAt. The test-only
clock seam lets the suite advance time deterministically and prove
that an expired entry stops counting as degraded without anyone
calling clearDegraded.

7/7 vitest cases green.

* feat(daemon): synthetic good + bad adapter fixtures (Phase 10.2)

Two test-only adapters that read the existing v1 transcript
fixtures (happy-3-rounds and malformed-unbalanced) and replay them
as either a full string or a 512-byte chunked stream. The chunked
form is what the conformance harness uses to prove the parser
holds together when the transcript arrives in arbitrary network
slices, not as one buffered blob.

* feat(daemon): adapter conformance harness (Phase 10.3)

runAdapterConformance pulls a transcript through the same
parseCritiqueStream pipeline the orchestrator uses and classifies
the outcome as shipped, degraded, or failed. On a degraded
outcome it forwards the matched reason to the adapter-degraded
registry, so a single nightly conformance run is what populates
the skip list rather than the orchestrator learning each adapter
is broken at request time.

5/5 vitest cases green covering shipped, malformed degraded,
oversize degraded, no-ship failure, and the harness-thrown
failure path.

* test(e2e): Critique Theater Playwright suite (Phase 11)

Six tests, one viewport per visual case, deterministic SSE
fixtures stubbed via page.route(). Adds the suite to
test:ui:extended so the existing extended-UI lane picks it up.

Coverage:

  1. Happy path: a single mounted theater plays the full
     fixture (1 run_started, 5 panelists open / dim / must_fix /
     close, 1 round_end, 1 ship) and ends on the score badge.
  2. Interrupt mid-run: the panelist that is open at the time
     the interrupt button is clicked closes with an interrupted
     marker and the transcript freezes there.
  3. Visual regression at 375x720 mobile.
  4. Visual regression at 768x1024 tablet.
  5. Visual regression at 1280x800 desktop.
  6. A11y role tree: the theater region exposes a labelled
     landmark, each panelist lane is a group with an accessible
     name, the score is a status live region.

All SSE traffic is stubbed by page.route so the suite runs in CI
without a daemon. The toggle is seeded via localStorage by
bootAppWithCritiqueEnabled so the gate behaves as if Settings
flipped it on. typecheck clean; playwright --list reports 6.

* test(web): reducer p99 bench at 10k iterations (Phase 13.1)

Locks the documented 2ms budget for the Critique Theater reducer
on a representative SSE script (27 actions, one full happy run)
behind a regression gate. Asserts p99 stays under 4ms (2x the
documented budget) so CI runners with a noisy neighbour do not
flake while a real regression to 20ms or 200ms still trips.

The bench is a vitest case rather than a bare microbenchmark so
it runs in the same CI lane as every other web test and does not
need a parallel runner.

* test(web): critique surface coverage walker (Phase 13.2)

Walks the public critique surface (11 SSE event names, 5 panelist
roles, 6 lifecycle phases, 9 named i18n keys) and asserts each
named symbol appears in both the src corpus and the test corpus.
The walker is the gate that catches a rename in one half of the
codebase without a matching update in the other half: a future
PR that drops 'panelist_must_fix' from the reducer without also
removing its test reference fails this suite.

62 assertions, one per symbol per corpus.

* docs: Critique Theater user guide (Phase 14.1)

Seven sections aimed at end users (not contributors):

  1. What is Design Jury
  2. How it works (the five panelists, auto-converging rounds,
     the composite formula)
  3. Settings (the M1 toggle and what it does)
  4. Reading the score badge
  5. Replay surface
  6. Troubleshooting (degraded, interrupted, failed)
  7. FAQ

The composite formula is documented as
    designer * 0 + critic * 0.4 + brand * 0.2 + a11y * 0.2 + copy * 0.2
because anyone trying to reverse-engineer the score is going to
search for those weights and the docs are the place they should
land first.

* docs(daemon): critique module AGENTS map (Phase 14.2)

Daemon-side wayfinder for the apps/daemon/src/critique directory.
Tables every file, what owns what invariant, and the 'when you
change anything here' guide so a future contributor does not
have to reverse-engineer the rollout resolver before adding a
new SSE event.

* docs(web): Theater module AGENTS map (Phase 14.3)

Web-side mirror of the daemon AGENTS map. Same file table, same
invariants section, same change-impact guide, sized to the
Theater component package.

* feat(daemon): rollout flag resolver (Phase 15.1)

Single decision point every caller consults to know whether the
orchestrator should wire the critique pipeline for a given run.
Priority:

  1. Skill-level policy (required wins, opt-out wins inversely)
  2. Per-project override from the Settings toggle
  3. OD_CRITIQUE_ENABLED env override
  4. Rollout phase default
       M0 dark-launch      false
       M1 settings only    false (toggle is off until the user flips it)
       M2 per-skill        true if skill opted in
       M3 global default   true

OD_CRITIQUE_ROLLOUT_PHASE parser defaults to M0 on unknown input
so a fresh install never surprises a user with the feature on.

10/10 vitest cases green covering every cell of the matrix.

* feat(web): Settings toggle hook for Critique Theater (Phase 15.2)

React hook that reads critiqueTheaterEnabled from the existing
open-design:config localStorage blob and stays in sync via:

  - the platform storage event (cross-tab)
  - a open-design:critique-theater-toggle CustomEvent (same-tab)

Same-tab event is the one that fires when the Settings panel saves
in the current window: the toggle and every mounted theater update
without a page reload.

setCritiqueTheaterEnabled(next) is the imperative setter the Settings
panel calls. It preserves the rest of the stored config (mode, apiKey,
etc.) and dispatches the same-tab event after the localStorage write.

The web hook reflects what the user toggled; the daemon-side
isCritiqueEnabled is the final routing authority (project override,
env, rollout phase). When they disagree, the daemon wins for backend
gating and the web reflects the toggle state.

6/6 vitest cases green covering first read, stored read, same-tab
event flip, config preservation, corrupted JSON tolerance, and
cross-tab storage event.

* test(web): Phase 15 toggle hook failure-mode coverage (PR #1320)

lefarcen P2 on PR #1320 flagged that the PR body claimed safe
behavior for disabled localStorage, non-object JSON, and missing
CustomEvent shim, but the suite only covered corrupt JSON plus
happy-path storage events. Added four failure-mode tests so the
swallowed errors are not silently traded for a throw in a future
refactor:

1. Returns false on a stored JSON value that parses to an array
   (non-object). Catches a regression where the guard treats
   anything truthy as a config blob.
2. Returns false on a stored JSON value of literal 'null'.
   typeof null === 'object' in JS, so the guard has to check null
   explicitly; this test pins that check.
3. Returns false when localStorage.getItem throws (private mode /
   disabled storage / SecurityError). The hook must swallow and
   return false so the rest of the app keeps rendering.
4. setCritiqueTheaterEnabled still dispatches the same-tab
   CustomEvent when localStorage.setItem throws (quota exceeded /
   disabled storage). The dispatch path is the in-session
   broadcast that keeps every mounted hook coherent even when
   persistence is unavailable; verified by mounting two probes
   and asserting both flip after the setter is called with a
   throwing setItem.

10/10 vitest cases green (6 existing + 4 new).

* fix(web): honor CustomEvent payload in toggle hook listener (PR #1320)

Both Siri-Ray (blocking) and lefarcen (P2 new) caught the same
real bug in the failure-mode test I added in affcdd27: the test
asserts the in-session UI flips when localStorage.setItem throws,
but the CustomEvent listener was ignoring the event's typed
detail and just calling readToggle(). Under a throwing setItem
the localStorage value is stale (or absent), so the listener
would see the OLD value and the test would fail (or worse, the
production claim 'in-session event keeps mounts coherent' was
hollow).

Fixed the hook, not the test: the listener now reads
event.detail.enabled when it is a boolean, falling back to
readToggle() only for malformed events or for cross-tab storage
events (which do not carry a typed payload). The setter already
dispatched the detail; the listener just was not consuming it.

Test changes:

  - The existing 'setItem throws' test now asserts the right
    behavior for the right reason. Updated the inline comment to
    say the listener reads from detail, not localStorage.
  - New test 'falls back to readToggle when the CustomEvent
    carries no usable detail' pins the fallback path: a
    malformed dispatcher (no detail, or detail.enabled not a
    boolean) degrades cleanly instead of throwing or being
    silently ignored.

11 / 11 vitest cases green (10 prior + 1 new fallback).

* feat(daemon): route critique spawn-path eligibility through the rollout resolver

The wireup edit Phase 10 and Phase 15 carved out: today server.ts gates
the critique pipeline on critiqueCfg.enabled, which is just the
OD_CRITIQUE_ENABLED env var. After this commit it gates on
isCritiqueEnabled(...) from the Phase 15 resolver, so the full
priority matrix is live:

  1. Per-skill od.critique.policy veto (opt-out / required)
  2. Per-project override (M1 Settings toggle, written through the
     existing Phase 6 settings endpoint)
  3. OD_CRITIQUE_ENABLED env override (power-user lane / CI fixtures)
  4. OD_CRITIQUE_ROLLOUT_PHASE default
       M0 dark-launch      false
       M1 settings only    false
       M2 per-skill        only when skillPolicy === 'opt-in'
       M3 global default   true

Default behaviour on a fresh install is unchanged: the resolver
returns false at M0 without an env override or a project override,
so prod traffic falls through to the legacy single-pass path
exactly the way it did before.

Inputs threaded today: phase from OD_CRITIQUE_ROLLOUT_PHASE,
envOverride from OD_CRITIQUE_ENABLED. skillPolicy and projectOverride
are passed as null for the v1 cutover; the daemon-side handler that
round-trips critiqueTheaterEnabled on the project settings row and
the od.critique.policy frontmatter resolver land as the next two
commits in this branch.

The three call sites that used critiqueCfg.enabled (the brand-thread
guard, the skill-thread guard, the top-line critiqueShouldRun
compound) now read from a single locally-scoped critiqueEnabledForRun
boolean, so the eligibility check is computed exactly once per spawn
and the prompt composer + orchestrator stay in lockstep the way
the existing comment already promised.

Tests still green: daemon vitest 22 / 22 across rollout +
conformance + adapter-degraded. Daemon typecheck clean.

* feat(web): mount CritiqueTheaterMount in ProjectView

The web counterpart of the daemon wireup. ProjectView now renders
<CritiqueTheaterMount projectId={project.id} enabled={...} /> as a
sibling of <AppChromeHeader> inside the top-level <div className="app">.

The mount is the drop-in from the Phase 9 stack: it owns the SSE
subscription, the kill-request handshake, and the phase-aware swap
from the live <TheaterStage> to the collapsed badge once a run
settles. The mount returns null until the daemon emits a
critique.run_started for the active project, so the visual surface
is byte-for-byte unchanged for users who have not opted in.

Enabled wiring: useCritiqueTheaterEnabled() reads the M1 Settings
toggle from the existing open-design:config localStorage blob and
stays in sync with both the platform storage event (cross-tab) and
the same-tab open-design:critique-theater-toggle CustomEvent the
Phase 15 setter dispatches. The hook honors the event payload
directly so a private-mode browser that cannot persist the toggle
still updates the in-session UI correctly.

The daemon-side gate (isCritiqueEnabled in apps/daemon/src/server.ts)
remains the authority for whether a run is actually wired through
the critique pipeline. This hook only governs whether the web layer
renders the resulting SSE stream when the daemon emits one. The
two-layer gate is intentional: an integrator embedding the Theater
in a custom UI can flip the web visibility independent of the
daemon's routing decision, and a daemon-side env override flips
backend gating without touching the web's localStorage.

Tests still green: web Theater suite 181 / 181 across 16 files.
Web typecheck clean.

* feat(daemon): resolve od.critique.policy frontmatter at the spawn site

The next step in the wireup branch's ladder: replace the placeholder
`skillPolicy: null` with the actual value parsed from the active
skill's SKILL.md frontmatter.

Three small edits, one new field on a public type:

1. SkillInfo gains a `critiquePolicy: SkillCritiquePolicy` field
   carrying the parsed `od.critique.policy` token (required /
   opt-in / opt-out / null). The field is null when the skill has
   no opinion, which lets the lower-priority resolver tiers
   (projectOverride, envOverride, phase default) decide.

2. listSkills() populates the new field via a small
   `normalizeCritiquePolicy` helper that tolerates the YAML
   scalar's casing and trims whitespace. Unknown tokens collapse
   to null so a typo in SKILL.md cannot accidentally force the
   panel on or off; it just falls through. Derived example cards
   inherit the parent's policy.

3. server.ts captures `skill.critiquePolicy` into a hoisted
   `skillCritiquePolicy` variable inside the existing skill-load
   block, then threads it into the isCritiqueEnabled call as the
   skillPolicy input. The hoisting keeps the variable in scope at
   the resolver call site without restructuring the spawn handler.

After this commit, the priority matrix the rollout resolver was
designed for is live for its top tier. The previous commit wired
env + phase; this one wires skill. The projectOverride input
remains null pending the next commit that extends the Phase 6
settings endpoint.

Daemon vitest: 10 / 10 rollout cases pass against the new wiring.
Daemon typecheck: clean.

* feat(daemon): feed projectOverride into the rollout resolver from project metadata

Replaces the placeholder `projectOverride: null` in the spawn
handler with the actual value the Settings panel writes onto the
project's metadata blob: `critiqueTheaterEnabled?: boolean`.

The read is defensive at the boundary: the metadata object is
typed loosely (it round-trips through SQLite as a free-form JSON
blob), so the spawn handler narrows to `boolean` and falls
through to `null` for any other shape. A missing key, a malformed
value, or a project that has never visited Settings collapses to
`null`, which is exactly the resolver's "no opinion, fall
through to env / phase" signal.

The `critique` frontmatter slot also gets typed on the
SkillFrontmatter shape so the `od.critique.policy` chain the
previous commit introduced no longer needs a bracket-access
cast. Same pattern as the existing `craft`, `preview`, and
`design_system` nested-record slots.

After this commit, every tier of the rollout resolver's priority
matrix is wired:

  1. skillPolicy   (from SKILL.md od.critique.policy)
  2. projectOverride (from project metadata critiqueTheaterEnabled)
  3. envOverride   (from OD_CRITIQUE_ENABLED)
  4. rollout phase (from OD_CRITIQUE_ROLLOUT_PHASE)

The write path for projectOverride still flows through the
existing project-update handler the Settings panel already uses
to persist project metadata; no new endpoint is needed. The
Settings UI button that calls setCritiqueTheaterEnabled and
posts the new field is the next commit on this branch.

Daemon typecheck: clean. Daemon vitest: 10 / 10 rollout cases
still green against the new wiring.

* fix(daemon): forward critique events to project sinks + align composer gate (PR #1338)

Two codex review items addressed in one commit since they share the
same root cause (resolver-enabled run hits a transport / prompt
contract that was still env-gated):

P1 (transport mismatch). The daemon emits critique.* SSE frames
through critiqueBus -> design.runs.emit, which fans out on
/api/runs/:runId/events. The web CritiqueTheaterMount subscribes to
/api/projects/:projectId/events (it's project-scoped, not run-
scoped, because the mount lives at the project workspace and
follows the user across runs). Result: in production the mount
never sees a real frame and the e2e tests' stubbed routes hide the
mismatch.

Fixed by extending critiqueBus.emit to fan out to BOTH sinks: the
existing runs.emit transport, AND the per-project event-sinks map.
The project-events route emits via sse.send(payload.type, payload),
so we pack the SSE channel name onto payload.type and let the sink
push the right channel. The web sseToPanelEvent overwrites type
from the channel name on the way back into a PanelEvent, so the
round-trip stays correct.

P2 (prompt gate misalignment). composeSystemPrompt reads
cfg.enabled to decide whether to append the panel addendum, but
critiqueCfg.enabled is loaded from OD_CRITIQUE_ENABLED only. A run
the resolver enabled via phase / project / skill (env unset) would
have critiqueShouldRun = true while critiqueCfg.enabled remained
false, dropping the panel prompt while still routing through
runOrchestrator -> parser waits for tags that never arrive -> run
degrades.

Fixed by passing a derived config { ...critiqueCfg, enabled: true }
to the composer when critiqueShouldRun is true. The composer's own
gate now agrees with the resolver decision on every input the
spec defines.

Daemon typecheck: clean. Daemon vitest: 10 / 10 rollout cases
still green against the new wiring.

* fix: address PerishCode P1 + P2 follow-ups on PR #1338

Two follow-up items PerishCode flagged on the activation PR.
Non-blocking but both are real:

1. Phase 11 e2e suite was wired into test:ui:extended but lands
   the user on '/' (home route) where ProjectView (and therefore
   CritiqueTheaterMount) is never rendered. With the suite as
   written, every assertion would time out the first time the
   lane runs in CI, contradicting the PR body's claim that the
   suite stays parked behind test.describe.fixme.

   The state diverged from my earlier Phase 11 work because the
   merge from main on commit 4ab719c6 brought in #1307's
   squash-merged version of the e2e file (the pre-fixme shape).

   Re-applied test.describe.fixme to the describe block plus
   removed ui/critique-theater.test.ts from the test:ui:extended
   script in e2e/package.json. Added a file-header docblock
   explaining what the follow-up commit needs to do: replace
   goto('/') with /projects/:id navigation similar to
   app-design-files.test.ts, split the SSE fixture into a live
   prefix and terminal suffix (Codex P2 on PR #1320), and commit
   the first PNG baselines.

2. bestRoundOf in CritiqueTheaterMount returned the LAST round
   with a numeric composite, not the round with the HIGHEST
   composite, while bestCompositeOf correctly returned the max.
   A run that closed round 1 at 8.5 and round 2 at 6.0 would
   dispatch interrupted { bestRound: 2, composite: 8.5 } on a
   user-clicked interrupt.

   Folded the two helpers into a single bestRoundAndComposite
   that walks state.rounds once and returns the matching pair so
   the two values cannot drift. The onInterrupt callback now
   destructures from one helper instead of two independent reads.
   Falls back to (state.activeRound, 0) when no round has closed
   with a composite yet.

Web typecheck: clean. CritiqueTheaterMount.test.tsx: 7 / 7 cases
still green against the new helper.

* fix: wire M1 project override end-to-end + correct deferred-surface doc claims (PR #1338)

Three lefarcen P2s on the latest review pass, all real:

1. M1 project override was half-wired: the daemon read
   metadata.critiqueTheaterEnabled but the web setter only
   wrote localStorage. A user opt-in would render the Theater
   on the web (localStorage was set) while the daemon resolved
   projectOverride=null and skipped critique unless env / phase
   already permitted. Two halves talking past each other.

   Extended setCritiqueTheaterEnabled to accept an optional
   { projectId, fetchProjectSettings } options bag. When a
   projectId is supplied, the setter ALSO sends a
   PATCH /api/projects/:id with { metadata: { critiqueTheaterEnabled
   } } so the daemon's spawn-time resolver picks the same value up
   on the next generation. The existing project-routes endpoint
   already accepts arbitrary metadata patches, so no new endpoint
   is needed. The local write + the CustomEvent dispatch still
   fire before the PATCH, so a network failure does not unwind
   the in-session UI flip. Three new vitest cases pin the new
   path: PATCHes when projectId is provided, skips when it is
   not, swallows a rejected PATCH so the in-session UI still
   flips.

2. Rollout docs (docs/critique-theater.md section 3) claimed the
   Settings toggle persists into the daemon settings store, but
   the previous implementation only had a localStorage reader /
   writer plus a daemon read of project metadata, with no
   round-trip. Rewrote the section to lead with the four-tier
   resolver (skill policy / project override / env / phase),
   document that the setter now round-trips via the existing
   PATCH endpoint when given a projectId, and call out the
   Settings panel UI control as a deliberate follow-up.

3. Troubleshooting table pointed users at /api/metrics/critique
   (Phase 12, deferred) and 'od adapters clear-degraded <id>'
   (CLI wrapper that does not exist). Replaced the metrics
   reference with the local conformance harness command
   (pnpm --filter @open-design/daemon vitest run
   tests/critique-conformance.test.ts) that ships today, with a
   note that the Phase 12 dashboard surfaces this status as a
   series once that PR lands. Replaced the CLI command with the
   programmatic clearDegraded() helper that exists today and
   flagged the CLI wrapper as planned follow-up.

Web typecheck: clean. Toggle hook tests: 14 / 14 green (11
existing + 3 new for the round-trip path).

* test(web): multi-round interrupt regression for bestRoundAndComposite (PR #1338)

lefarcen P3 follow-up to the previous bestRoundAndComposite fix:
the existing CritiqueTheaterMount.test.tsx interrupt cases only
exercised a single-round state, so a future refactor back to two
independent helpers wouldn't be caught by the test suite even
though it'd reintroduce the round / composite drift bug.

Added a regression case that:

  1. Drives the reducer through two complete rounds with the
     full 5-role cast closing at distinct composites: round 1
     at 8.5, round 2 at 6.0 (the high-composite round is NOT the
     most recent one).
  2. Clicks Interrupt + waits for the daemon ack via the test
     seam fetcher returning 204.
  3. Asserts the collapsed badge displays "round 1" (the
     correct best-composite round), and queryByText for
     "round 2 ... 8.5" returns null (the buggy pairing
     would have produced that string).

The bestRoundAndComposite helper walks state.rounds in one pass
and returns the matching pair, so the round number and the
composite cannot drift apart. This test locks the fix in: a
refactor that splits the helpers back into independent walks
will be caught here.

8 / 8 vitest cases green on the file.

* fix(web): read-merge-write the project metadata in setCritiqueTheaterEnabled (PerishCode P2 on PR #1338)

The previous round-trip sent { metadata: { critiqueTheaterEnabled: next } }
as the entire PATCH body. The daemon's project-routes handler only
re-stamps three immutable fields (baseDir, importedFrom,
fromTrustedPicker) before calling updateProject(db, id, patch),
which then does a shallow { ...existing, ...patch } in apps/daemon/
src/db.ts. So patch.metadata replaces the row's metadata wholesale,
dropping kind, templateId, linkedDirs, and every other field the rest
of the app reads.

No in-tree caller passes projectId today (only vitest cases), so the
bug had not surfaced yet. But the surface is documented in
docs/critique-theater.md section 3 and the function's own JSDoc as
the M1 round-trip path, so it would have shipped as a latent footgun
for the next integrator: a Settings UI follow-up, or any third party
that wires the setter into a project-aware surface.

Fix: read-merge-write rather than a bare patch.

- GET /api/projects/:id to read the row's current metadata.
- Spread that metadata into the PATCH body and overlay
  critiqueTheaterEnabled: next on top, mirroring the partial-metadata
  pattern already used in ChatComposer.tsx for linkedDirs.
- PATCH the merged object.

Failure handling:
- GET fails: skip the PATCH entirely. We cannot construct a safe
  merged body without the current state, and a bare patch would
  wipe other metadata. The in-session CustomEvent fired earlier in
  the setter still keeps every mounted hook consistent; the next
  save retries the round-trip.
- PATCH fails: log in dev. The in-session UI is already correct via
  the CustomEvent.

Tests (TDD, red-first):

- 'GETs the project then PATCHes with merged metadata when a
  projectId is supplied': stubs a GET that returns
  { kind: 'template', templateId: 'modern-blog', linkedDirs: [...] }
  and asserts the PATCH body equals the merge plus the toggle.
- 'PATCHes with just the toggle when the project has no prior
  metadata': stubs a GET that returns no metadata block.
- 'skips the PATCH (does not stomp metadata) when the prefetch GET
  fails': stubs a rejecting GET and asserts only the GET fires.
- 'swallows a rejected PATCH after a successful prefetch': stubs a
  successful GET and a rejecting PATCH; asserts the in-session UI
  still flips via the CustomEvent.

Doc updated on the setter's JSDoc to describe the new three-step
flow (localStorage, CustomEvent, read-merge-write PATCH) and the
two failure modes.

Verified:
- pnpm --filter @open-design/web typecheck clean.
- pnpm --filter @open-design/web test: 111 files / 1055 tests green
  (was 1052, +3 from the new merge-flow cases).

* fix(web): restore wait-for-daemon-ack pattern on Theater interrupt

Same regression as flagged on PR #1316 post-main-merge: the
optimistic local dispatch fired before the POST resolved, so a
daemon 404 / 409 still terminalized the UI and the real SSE
terminal event got ignored by the sticky interrupted phase.

Snapshot runId / bestRound / composite at click time, dispatch
interrupted only on res.ok, clear interruptPending on rejection or
non-2xx so the user can retry. Tests cover rejection + 404 leaving
the run on the live stage; the 204 path waits for the ack.

* fix(test): add projectKind prop to FileViewer deck render after v0.7.0 merge

* fix(daemon): address PerishCode P3 trio on PR #1338 (emit helper reuse + spawn-input coverage + restored docs)

---------

Co-authored-by: Nagendhra <nagendhra405@gmail.com>
2026-05-14 20:37:06 +08:00

1002 lines
39 KiB
TypeScript

// Skill registry. Scans one or more on-disk roots for SKILL.md files, parses
// front-matter, returns listing. No watching in this MVP — re-scans on every
// GET /api/skills, which is fine for dozens of skills.
//
// Roots are passed in priority order: the first one wins on `id` collisions
// so user-imported skills under USER_SKILLS_DIR can shadow a built-in skill
// of the same name without erasing the built-in copy.
import type { Dirent } from "node:fs";
import { cp, mkdir, readdir, readFile, rm, stat, writeFile } from "node:fs/promises";
import path from "node:path";
import { parseFrontmatter } from "./frontmatter.js";
import type { SkillCritiquePolicy } from "./critique/rollout.js";
import { SKILLS_CWD_ALIAS } from "./cwd-aliases.js";
// Persisted skill ids on existing projects can outlive a folder rename.
// listSkills() derives the id from the SKILL.md frontmatter `name`, so once
// a skill is renamed the old id stops resolving and composeSystemPrompt
// silently drops the skill body for projects saved against the old id.
// This map forwards deprecated ids to their current canonical id; callers
// resolve through findSkillById() before scanning the listing. Leave entries
// here for at least one stable release after a rename so on-disk projects
// keep composing with the intended skill prompt.
export const SKILL_ID_ALIASES = Object.freeze({
"editorial-collage": "open-design-landing",
"editorial-collage-deck": "open-design-landing-deck",
});
type SkillMode = "image" | "video" | "audio" | "deck" | "design-system" | "template" | "prototype";
type SkillSurface = "web" | "image" | "video" | "audio";
type SkillPlatform = "desktop" | "mobile" | null;
type JsonRecord = Record<string, unknown>;
interface SkillFrontmatter extends JsonRecord {
name?: unknown;
description?: unknown;
triggers?: unknown;
od?: JsonRecord & {
craft?: JsonRecord;
preview?: JsonRecord;
design_system?: JsonRecord;
critique?: JsonRecord;
category?: unknown;
};
}
// Indicates whether a skill came from a user-writable root (the first root
// passed to listSkills) or from a built-in repo root (any later root). The
// UI uses this to render an origin pill and to gate destructive actions:
// only `user` skills can be deleted via /api/skills/:id.
export type SkillSource = "user" | "built-in";
export interface SkillInfo {
id: string;
name: string;
description: string;
triggers: unknown[];
mode: SkillMode;
surface: SkillSurface;
source: SkillSource;
craftRequires: string[];
platform: SkillPlatform;
scenario: string;
// Optional human-readable category (e.g. "image-generation", "video",
// "design-systems"). Surfaced as a filter pill in Settings → Skills so a
// large pre-loaded catalogue (e.g. curated design/creative skills from the
// upstream awesome-* lists) stays scannable. Not part of system-prompt
// composition; purely a UI hint.
category: string | null;
previewType: string;
designSystemRequired: boolean;
defaultFor: string[];
upstream: string | null;
featured: number | null;
fidelity: "wireframe" | "high-fidelity" | null;
speakerNotes: boolean | null;
animations: boolean | null;
examplePrompt: string;
aggregatesExamples: boolean;
/**
* Per-skill Critique Theater override declared via `od.critique.policy`
* in the skill's SKILL.md frontmatter. The daemon's rollout resolver
* uses this as the highest-priority signal when deciding whether to
* wire the critique pipeline for a generation: `required` forces the
* panel on regardless of project / env / phase defaults, `opt-out`
* forces it off, `opt-in` lets the panel run only at M2+ rollout
* phases, `null` means the skill has no opinion and the lower-priority
* tiers (project override, env override, phase default) decide.
*/
critiquePolicy: SkillCritiquePolicy;
body: string;
dir: string;
}
interface DerivedExample {
key: string;
}
export interface DerivedSkillIdParts {
parentId: string;
childKey: string;
}
function isRecord(value: unknown): value is JsonRecord {
return Boolean(value) && typeof value === "object";
}
function asSkillFrontmatter(value: unknown): SkillFrontmatter {
return isRecord(value) ? (value as SkillFrontmatter) : {};
}
export function resolveSkillId(id: unknown): unknown {
if (typeof id !== "string" || id.length === 0) return id;
return (SKILL_ID_ALIASES as Readonly<Record<string, string>>)[id] ?? id;
}
// Lookup helper that mirrors `skills.find((s) => s.id === id)` but first
// rewrites any deprecated id to its current canonical form. Use this at
// every site that resolves a stored or external skill id; calling
// `.find()` directly will silently miss aliased ids.
export function findSkillById(skills: unknown, id: unknown): SkillInfo | undefined {
if (!Array.isArray(skills) || typeof id !== "string" || id.length === 0) {
return undefined;
}
const canonical = resolveSkillId(id);
return (skills as SkillInfo[]).find((s) => s.id === canonical);
}
// Accept either a single root path or an array. When given multiple roots,
// the first one wins on id collisions so user-imported skills under
// USER_SKILLS_DIR can shadow a built-in skill of the same name without
// erasing the bundled copy. Each surfaced summary carries a `source`
// (`"user"` for the first root, `"built-in"` for any later root) so the
// UI can render an origin pill and gate the delete control.
export async function listSkills(
skillsRoots: string | readonly string[],
): Promise<SkillInfo[]> {
const roots = Array.isArray(skillsRoots) ? skillsRoots : [skillsRoots];
const out: SkillInfo[] = [];
const seenIds = new Set<string>();
for (let rootIdx = 0; rootIdx < roots.length; rootIdx += 1) {
const skillsRoot = roots[rootIdx];
if (!skillsRoot) continue;
const source: SkillSource = rootIdx === 0 ? "user" : "built-in";
let entries: Dirent[] = [];
try {
entries = await readdir(skillsRoot, { withFileTypes: true });
} catch {
continue;
}
for (const entry of entries) {
if (!entry.isDirectory() && !entry.isSymbolicLink()) continue;
const dir = path.join(skillsRoot, entry.name);
const skillPath = path.join(dir, "SKILL.md");
try {
const stats = await stat(skillPath);
if (!stats.isFile()) continue;
const raw = await readFile(skillPath, "utf8");
const { data: parsedData, body } = parseFrontmatter(raw) as {
data: unknown;
body: string;
};
const data = asSkillFrontmatter(parsedData);
const parentId =
typeof data.name === "string" && data.name ? data.name : entry.name;
// Skip when an earlier root already surfaced this id — the first
// root wins so user shadows built-in. Done before we read the
// rest of the frontmatter to keep the shadowed-skill path cheap.
if (seenIds.has(parentId)) continue;
seenIds.add(parentId);
const hasAttachments = await dirHasAttachments(dir);
const mode = normalizeMode(data.od?.mode, body, data.description);
const surface = normalizeSurface(data.od?.surface, mode);
const platform = normalizePlatform(
data.od?.platform,
mode,
body,
data.description,
);
const scenario = normalizeScenario(
data.od?.scenario,
body,
data.description,
);
const category = normalizeCategory(data.od?.category);
const designSystemRequired =
typeof data.od?.design_system?.requires === "boolean"
? data.od.design_system.requires
: true;
const upstream =
typeof data.od?.upstream === "string" ? data.od.upstream : null;
const previewType =
typeof data.od?.preview?.type === "string"
? data.od.preview.type
: "html";
const description =
typeof data.description === "string" ? data.description : "";
const parentBody = hasAttachments
? withSkillRootPreamble(body, dir)
: body;
// Pre-compute derived examples so the parent entry can advertise
// `aggregatesExamples` in the same push. The frontend uses that
// flag to hide the parent card from the gallery (its preview would
// duplicate one of the derived cards), while the daemon keeps the
// parent in the listing so `findSkillById` still resolves it for
// system-prompt composition and id alias lookups.
const derivedExamples = await collectDerivedExamples(dir);
const aggregatesExamples = derivedExamples.length > 0;
out.push({
id: parentId,
name: parentId,
description,
triggers: Array.isArray(data.triggers) ? data.triggers : [],
mode,
surface,
source,
craftRequires: normalizeCraftRequires(data.od?.craft?.requires),
platform,
scenario,
category,
previewType,
designSystemRequired,
defaultFor: normalizeDefaultFor(data.od?.default_for),
upstream,
featured: normalizeFeatured(data.od?.featured),
// Optional metadata hints used by 'Use this prompt' fast-create
// so the resulting project mirrors the shipped example.html.
// Each hint is only consumed when its kind matches the skill
// mode; missing hints fall back to the new-project defaults.
fidelity: normalizeFidelity(data.od?.fidelity),
speakerNotes: normalizeBoolHint(data.od?.speaker_notes),
animations: normalizeBoolHint(data.od?.animations),
examplePrompt: derivePrompt(data),
aggregatesExamples,
critiquePolicy: normalizeCritiquePolicy(data.od?.critique?.policy),
body: parentBody,
dir,
});
// Surface every example sitting next to a SKILL.md as its own card
// so a single skill (e.g. live-artifact) can ship a small gallery
// of hand-crafted samples without needing one SKILL.md per sample.
// Each derived card inherits the parent's mode/platform/surface/
// scenario so existing TYPE/SURFACE filters keep working; the
// synthetic id `<parent>:<child>` lets `/api/skills/:id/example`
// resolve straight to the matching HTML on disk. We deliberately
// do not inherit `featured` so derived cards never crowd the
// magazine row.
for (const example of derivedExamples) {
const derivedId = `${parentId}:${example.key}`;
if (seenIds.has(derivedId)) continue;
seenIds.add(derivedId);
out.push({
id: derivedId,
name: humanizeExampleName(example.key),
description,
triggers: Array.isArray(data.triggers) ? data.triggers : [],
mode,
surface,
source,
craftRequires: [],
platform,
scenario,
category,
previewType,
designSystemRequired,
defaultFor: [],
upstream,
featured: null,
fidelity: normalizeFidelity(data.od?.fidelity),
speakerNotes: normalizeBoolHint(data.od?.speaker_notes),
animations: normalizeBoolHint(data.od?.animations),
examplePrompt: derivePrompt(data),
aggregatesExamples: false,
// Derived cards inherit the parent's critique policy so a
// single SKILL.md that opts in (or out) applies the same
// gate to every example in its gallery.
critiquePolicy: normalizeCritiquePolicy(data.od?.critique?.policy),
// Inherit the parent's full SKILL.md body so 'Use this prompt'
// on a derived card seeds the agent with the same workflow
// the parent describes. Without this, picking a derived card
// would compose an empty system prompt.
body: parentBody,
dir,
});
}
} catch {
// Skip unreadable entries — this is discovery, not validation.
}
}
}
return out;
}
// Discover example artifacts that live alongside SKILL.md under
// `<dir>/examples/`. Only the single-file layout is surfaced:
//
// `examples/<name>.html` — pre-baked, self-contained sample.
//
// We deliberately do not surface the subfolder layout (e.g. live-artifact's
// `examples/<name>/template.html` + `data.json`) because those templates
// still hold `{{data.x}}` placeholders that only the daemon-side renderer
// fills in. Showing the raw template would render visible placeholder
// braces in the gallery — worse than not surfacing the example at all.
// To ship a subfolder-style example, place the baked output beside the
// folder as `examples/<name>.html` (the canonical render) and keep the
// subfolder around as agent-readable source.
async function collectDerivedExamples(dir: string): Promise<DerivedExample[]> {
const examplesDir = path.join(dir, "examples");
let entries: Dirent[] = [];
try {
entries = await readdir(examplesDir, { withFileTypes: true });
} catch {
return [];
}
const out: DerivedExample[] = [];
for (const entry of entries) {
if (!entry.isFile()) continue;
if (!entry.name.toLowerCase().endsWith(".html")) continue;
const key = entry.name.replace(/\.html$/i, "");
if (!isSafeExampleKey(key)) continue;
out.push({ key });
}
// Stable order so the gallery renders the same sequence on every reload.
out.sort((a, b) => a.key.localeCompare(b.key));
return out;
}
// Reject keys that could escape the examples folder or break the
// `<parent>:<child>` id format. Letters/digits/dash/dot/underscore only,
// and never the dotfile path-traversal patterns.
function isSafeExampleKey(key: string): boolean {
if (!key || key.startsWith(".")) return false;
if (key.includes(":")) return false;
return /^[A-Za-z0-9._-]+$/.test(key);
}
// Turn a basename like `stock-portfolio-live` into a title-cased label
// (`Stock Portfolio Live`) so the gallery card has a readable heading
// without forcing every example to ship its own frontmatter.
function humanizeExampleName(key: string): string {
return key
.replace(/[-_]+/g, " ")
.replace(/\s+/g, " ")
.trim()
.split(" ")
.map((word) =>
word.length === 0
? word
: word.charAt(0).toUpperCase() + word.slice(1).toLowerCase()
)
.join(" ");
}
// Used by `/api/skills/:id/example` to resolve a derived id back to its
// on-disk file. Returns null when the key is unsafe; the route checks
// `fs.existsSync` against the returned path before reading.
export function resolveDerivedExamplePath(parentDir: string, childKey: string): string | null {
if (!isSafeExampleKey(childKey)) return null;
return path.join(parentDir, "examples", `${childKey}.html`);
}
// Split a `<parent>:<child>` synthetic id into its two halves. Returns
// null for non-derived ids so the caller can fall through to the regular
// listing-based lookup.
export function splitDerivedSkillId(id: unknown): DerivedSkillIdParts | null {
if (typeof id !== "string") return null;
const idx = id.indexOf(":");
if (idx <= 0 || idx === id.length - 1) return null;
const parentId = id.slice(0, idx);
const childKey = id.slice(idx + 1);
if (!isSafeExampleKey(childKey)) return null;
return { parentId, childKey };
}
// Skills that ship side files (e.g. `assets/template.html`, `references/*.md`)
// need the agent to know where the skill lives on disk — relative paths in the
// SKILL.md body would otherwise resolve against the agent's CWD, which is the
// project folder (`.od/projects/<id>/`), not the skill folder.
//
// We prepend a short preamble that advertises two paths:
//
// 1. A CWD-relative alias path (`.od-skills/<folder>/`) — the primary one.
// Before spawning the agent the chat handler copies the active skill
// into `<cwd>/.od-skills/<folder>/` (see `cwd-aliases.ts`), so this
// path is inside the agent's working directory on every CLI and is
// not blocked by directory-access policies (issue #430).
// 2. The absolute repo path — a fallback for the cases the staged copy
// cannot exist for: `/api/runs` calls without a project (cwd falls
// back to the repo root, where the absolute path *is* an in-cwd
// path), or environments where staging fails. Claude/Copilot are
// additionally given `--add-dir` for that absolute path, so the
// fallback round-trips even under their permission policy.
//
// Authoring guidance lives in the preamble itself so an agent can pick
// the right form on its own without daemon-side feature detection.
function withSkillRootPreamble(body: string, dir: string): string {
const referencedFiles = collectReferencedSideFiles(body);
const folder = path.basename(dir);
const skillRootRel = `${SKILLS_CWD_ALIAS}/${folder}`;
const exampleFile = referencedFiles[0];
const relativeGuidance = exampleFile
? "> below references side files such as `" + exampleFile + "`, prefer the\n" +
"> relative form rooted at the first path above — e.g. open `" +
skillRootRel + "/" + exampleFile + "`."
: "> below references side files, prefer the relative form rooted at the\n" +
"> first path above.";
const absoluteGuidance = exampleFile
? "> back to the absolute path: `" + path.join(dir, exampleFile) + "`."
: "> back to the absolute skill root above.";
const preamble = [
"> **Skill root (relative to project):** `" + skillRootRel + "/`",
"> **Skill root (absolute fallback):** `" + dir + "`",
">",
"> This skill ships side files alongside `SKILL.md`. When the workflow",
relativeGuidance,
"> If that path is not reachable from your working directory, fall",
absoluteGuidance,
"> Either form resolves to the same file; the relative form keeps you",
"> inside the project working directory, which is preferred.",
...(referencedFiles.length > 0
? [
">",
"> Known side files in this skill: " +
referencedFiles.map((file) => "`" + file + "`").join(", ") +
".",
]
: []),
"",
"",
].join("\n");
return preamble + body;
}
function collectReferencedSideFiles(body: string): string[] {
const files = new Set<string>();
const matches = body.matchAll(/\b(?:assets|references)\/[A-Za-z0-9._-]+\b/g);
for (const match of matches) files.add(match[0]);
if (/\bexample\.html\b/.test(body)) files.add("example.html");
return Array.from(files).sort();
}
async function dirHasAttachments(dir: string): Promise<boolean> {
try {
const entries = await readdir(dir, { withFileTypes: true });
return entries.some(
(e) =>
e.name !== "SKILL.md" &&
(e.isDirectory() || /\.(md|html|css|js|json|txt)$/i.test(e.name))
);
} catch {
return false;
}
}
// Craft sections live at <projectRoot>/craft/<name>.md. We accept any
// alphanumeric+dash slug here so adding a new section is as simple as
// dropping a file in craft/ and listing its name in the skill — no
// daemon-side allowlist to keep in sync. The compose path checks the
// file actually exists before injecting; missing files fall through
// silently. The frontend can render the requested list verbatim.
function normalizeCraftRequires(value: unknown): string[] {
if (!Array.isArray(value)) return [];
const seen = new Set<string>();
const out: string[] = [];
for (const v of value) {
if (typeof v !== "string") continue;
const slug = v.trim().toLowerCase();
if (!slug || !/^[a-z0-9][a-z0-9-]*$/.test(slug)) continue;
if (seen.has(slug)) continue;
seen.add(slug);
out.push(slug);
}
return out;
}
function normalizeDefaultFor(value: unknown): string[] {
if (!value) return [];
if (Array.isArray(value)) return value.map(String);
return [String(value)];
}
// Optional `od.fidelity` hint for prototype skills. Only 'wireframe' and
// 'high-fidelity' are meaningful — anything else collapses to null so the
// caller falls back to the form default ('high-fidelity').
function normalizeFidelity(value: unknown): "wireframe" | "high-fidelity" | null {
if (value === "wireframe" || value === "high-fidelity") return value;
return null;
}
// Coerce truthy / falsy strings ("true", "yes", "false", "no") and booleans
// to a real boolean. Returns null for anything we can't interpret so the
// caller knows to fall back to the form default.
function normalizeBoolHint(value: unknown): boolean | null {
if (typeof value === "boolean") return value;
if (typeof value === "string") {
const v = value.trim().toLowerCase();
if (v === "true" || v === "yes" || v === "1") return true;
if (v === "false" || v === "no" || v === "0") return false;
}
return null;
}
/**
* Coerce `od.critique.policy` from SKILL.md frontmatter into the
* three-value union the rollout resolver expects. Anything unrecognised
* resolves to `null` (no opinion), which falls through to the
* project / env / phase default tiers. The frontmatter value is
* authored as a YAML scalar:
*
* od:
* critique:
* policy: required # or 'opt-in', 'opt-out'
*/
// Exported so the spawn-input glue tests can pin the trim / lowercase /
// reject-typo behavior in isolation from `listSkills()` filesystem
// scanning (PerishCode P3 on PR #1338).
export function normalizeCritiquePolicy(value: unknown): SkillCritiquePolicy {
if (typeof value !== "string") return null;
const v = value.trim().toLowerCase();
if (v === "required" || v === "opt-in" || v === "opt-out") return v;
return null;
}
// Coerce `od.featured` into a numeric priority. Lower numbers float to the
// top of the Examples gallery; `true` is treated as priority 1; anything
// missing/unrecognised becomes null so non-featured skills keep their
// natural alphabetical order.
function normalizeFeatured(value: unknown): number | null {
if (value === true) return 1;
if (typeof value === "number" && Number.isFinite(value)) return value;
if (typeof value === "string" && value.trim()) {
const n = Number(value);
if (Number.isFinite(n)) return n;
}
return null;
}
// Prefer an explicitly authored `od.example_prompt`. Fall back to the
// skill description's first sentence — it's already written in actionable
// language ("Admin / analytics dashboard in a single HTML file…") so it
// serves as a passable starter prompt.
function derivePrompt(data: SkillFrontmatter): string {
const explicit = data.od?.example_prompt;
if (typeof explicit === "string" && explicit.trim()) return explicit.trim();
const desc =
typeof data.description === "string" ? data.description.trim() : "";
if (!desc) return "";
const collapsed = desc.replace(/\s+/g, " ").trim();
const firstSentence = collapsed.match(/^.+?[.!?。!?](?:\s|$)/)?.[0]?.trim();
return (firstSentence || collapsed).slice(0, 320);
}
function inferMode(body: unknown, description: unknown): SkillMode {
const hay = `${description ?? ""}\n${body ?? ""}`.toLowerCase();
if (/\bimage|poster|illustration|photography|图片|海报|插画/.test(hay)) return "image";
if (/\bvideo|motion|shortform|animation|视频|动效|短片/.test(hay)) return "video";
if (/\baudio|music|jingle|tts|sound|音频|音乐|配音|音效/.test(hay)) return "audio";
if (/\bppt|deck|slide|presentation|幻灯|投影/.test(hay)) return "deck";
if (/\bdesign[- ]system|\bdesign\.md|\bdesign tokens/.test(hay))
return "design-system";
if (/\btemplate\b/.test(hay)) return "template";
return "prototype";
}
function normalizeMode(value: unknown, body: unknown, description: unknown): SkillMode {
if (
value === "image" || value === "video" || value === "audio" || value === "deck" ||
value === "design-system" || value === "template" || value === "prototype"
) return value;
return inferMode(body, description);
}
const KNOWN_SURFACES = new Set<SkillSurface>(["web", "image", "video", "audio"]);
function normalizeSurface(value: unknown, mode: SkillMode): SkillSurface {
if (typeof value === "string") {
const v = value.trim().toLowerCase();
if (KNOWN_SURFACES.has(v as SkillSurface)) return v as SkillSurface;
}
if (mode === "image" || mode === "video" || mode === "audio") return mode;
return "web";
}
// Validate platform tag — only desktop / mobile are meaningful for the
// Examples gallery. Falls back to autodetecting "mobile" from descriptions
// so legacy skills sort under the right pill without authoring changes.
function normalizePlatform(value: unknown, mode: SkillMode, body: unknown, description: unknown): SkillPlatform {
if (value === "desktop" || value === "mobile") return value;
if (mode !== "prototype") return null;
const hay = `${description ?? ""}\n${body ?? ""}`.toLowerCase();
if (/mobile|phone|ios|android|手机|移动端/.test(hay)) return "mobile";
return "desktop";
}
// Normalise a scenario tag to a small fixed vocabulary so the filter pills
// stay tidy. Unknown values pass through verbatim so authors can experiment;
// missing values default to "general".
const KNOWN_SCENARIOS = new Set([
"general",
"engineering",
"product",
"design",
"marketing",
"sales",
"finance",
"hr",
"operations",
"support",
"legal",
"education",
"personal",
]);
// Normalise a free-form category tag. Limits the set of accepted characters
// to lowercase letters, digits, and dashes so the value can flow straight
// into the UI as a filter pill class without escaping. Empty / non-string
// values become null so the filter row hides instead of rendering an empty
// pill. We intentionally do not lock down a fixed vocabulary here — the
// curated catalogue under skills/ owns the canonical category set, and
// user-imported skills are free to introduce their own.
function normalizeCategory(value: unknown): string | null {
if (typeof value !== "string") return null;
const slug = value.trim().toLowerCase();
if (!slug) return null;
if (!/^[a-z0-9][a-z0-9-]*$/.test(slug)) return null;
return slug.slice(0, 64);
}
function normalizeScenario(value: unknown, body: unknown, description: unknown): string {
if (typeof value === "string") {
const v = value.trim().toLowerCase();
if (v) return v;
}
const hay = `${description ?? ""}\n${body ?? ""}`.toLowerCase();
if (/finance|invoice|expense|budget|p&l|revenue/.test(hay)) return "finance";
if (/\bhr\b|onboarding|payroll|employee|人事/.test(hay)) return "hr";
if (/marketing|campaign|brand|landing/.test(hay)) return "marketing";
if (/runbook|incident|deploy|engineering|sre|api/.test(hay))
return "engineering";
if (/spec|prd|roadmap|product manager|product team/.test(hay))
return "product";
if (/design system|moodboard|mockup|ui kit/.test(hay)) return "design";
if (/sales|quote|proposal|lead/.test(hay)) return "sales";
if (/operations|ops|logistics|inventory/.test(hay)) return "operations";
return "general";
}
// Surface the vocabulary so callers (frontend filter UI) could mirror it
// later if they want to. Not exported today, kept here for documentation.
void KNOWN_SCENARIOS;
// ---------------------------------------------------------------------------
// User-skill import / delete primitives
// ---------------------------------------------------------------------------
// User-imported skills live under <runtimeData>/user-skills/<slug>/SKILL.md.
// We treat that directory as fully owned by the daemon, so import/delete are
// simple: write or rm the slug folder and let listSkills() pick the change up
// on the next /api/skills request. The slug is derived from the user-supplied
// `name` (alphanumeric + dash) and prefixed with `user-` only when an existing
// built-in skill folder shares the same id, to avoid colliding with a
// repo-shipped folder.
export type SkillImportErrorCode =
| "BAD_REQUEST"
| "CONFLICT"
| "NOT_FOUND"
| "INTERNAL_ERROR";
export class SkillImportError extends Error {
readonly code: SkillImportErrorCode;
constructor(code: SkillImportErrorCode, message: string) {
super(message);
this.code = code;
this.name = "SkillImportError";
}
}
const RESERVED_SLUGS = new Set(["", ".", ".."]);
export function slugifySkillName(name: unknown): string {
if (typeof name !== "string") return "";
const lowered = name.trim().toLowerCase();
const cleaned = lowered
.replace(/[^a-z0-9\-_]+/g, "-")
.replace(/^-+|-+$/g, "")
.replace(/-+/g, "-");
if (!cleaned || RESERVED_SLUGS.has(cleaned)) return "";
return cleaned.slice(0, 64);
}
function escapeYamlString(value: unknown): string {
return String(value).replace(/\\/g, "\\\\").replace(/"/g, '\\"');
}
interface BuildSkillMarkdownInput {
name: string;
description: string;
body: string;
triggers: string[];
}
function buildSkillMarkdown({
name,
description,
body,
triggers,
}: BuildSkillMarkdownInput): string {
// Always emit `name` as a quoted scalar so YAML never coerces it to a
// number / boolean / null. Without the quotes, parseYamlSubset() would
// re-read names like '123', 'true', or 'null' as non-string literals,
// and importUserSkill()'s round-trip ("imported skill could not be
// re-read") would fail for those ids. See PR #955 review feedback.
const lines: string[] = ["---", `name: "${escapeYamlString(name)}"`];
if (description && description.trim().length > 0) {
lines.push("description: |");
for (const ln of description.trim().split(/\r?\n/)) {
lines.push(` ${ln}`);
}
}
if (triggers.length > 0) {
lines.push("triggers:");
for (const t of triggers) {
const trimmed = typeof t === "string" ? t.trim() : "";
if (!trimmed) continue;
lines.push(` - "${escapeYamlString(trimmed)}"`);
}
}
lines.push("---", "", body.trim(), "");
return lines.join("\n");
}
export interface SkillImportInput {
name?: unknown;
description?: unknown;
body?: unknown;
triggers?: unknown;
}
export interface SkillImportResult {
id: string;
slug: string;
dir: string;
}
function isErrnoException(err: unknown): err is NodeJS.ErrnoException {
return Boolean(err) && typeof err === "object" && "code" in (err as object);
}
export async function importUserSkill(
userSkillsRoot: string,
input: SkillImportInput,
): Promise<SkillImportResult> {
const name = typeof input?.name === "string" ? input.name.trim() : "";
const description =
typeof input?.description === "string" ? input.description : "";
const body = typeof input?.body === "string" ? input.body : "";
if (!name) {
throw new SkillImportError("BAD_REQUEST", "skill name required");
}
if (!body || body.trim().length === 0) {
throw new SkillImportError("BAD_REQUEST", "skill body required");
}
const slug = slugifySkillName(name);
if (!slug) {
throw new SkillImportError(
"BAD_REQUEST",
"skill name must produce a valid slug (a-z, 0-9, dash)",
);
}
const triggersRaw = Array.isArray(input?.triggers) ? input.triggers : [];
const triggers = triggersRaw
.map((t) => (typeof t === "string" ? t.trim() : ""))
.filter(Boolean);
await mkdir(userSkillsRoot, { recursive: true });
const dir = path.join(userSkillsRoot, slug);
// Refuse to overwrite an existing folder. The caller can DELETE first
// when intentionally replacing a skill.
try {
const existing = await stat(dir);
if (existing) {
throw new SkillImportError(
"CONFLICT",
`a user skill with slug "${slug}" already exists`,
);
}
} catch (err) {
if (err instanceof SkillImportError) throw err;
if (isErrnoException(err) && err.code !== "ENOENT") {
throw new SkillImportError(
"INTERNAL_ERROR",
`could not check skill dir: ${err.message ?? err}`,
);
}
}
await mkdir(dir, { recursive: true });
const md = buildSkillMarkdown({ name, description, body, triggers });
await writeFile(path.join(dir, "SKILL.md"), md, "utf8");
return { id: name, slug, dir };
}
export interface SkillUpdateInput {
name: string;
description?: unknown;
body?: unknown;
triggers?: unknown;
// Original on-disk dir for the skill being edited. When the caller is
// shadowing a built-in for the first time (i.e. `sourceDir` differs
// from the user shadow target and the shadow folder does not exist
// yet), `updateUserSkill` clones every entry except `SKILL.md` from
// `sourceDir` into the shadow so the bundled side tree (assets/,
// references/, scripts/, examples/, ...) keeps resolving through the
// /api/skills/:id/files, /example, and /assets/* routes after the
// edit. Without this, listSkills() promotes the shadow folder to the
// active dir but the resolvers see only the user-authored SKILL.md
// and the rest of the skill silently disappears (mrcfps PR #955
// review). When omitted (or pointing at the same folder) the call
// only writes SKILL.md and leaves any previously-cloned side files
// alone so subsequent edits do not clobber the user's tweaks.
sourceDir?: string;
}
// Overwrite (or create-on-demand) a user-owned SKILL.md. For built-in
// skills this writes a "shadow" copy under USER_SKILLS_DIR/<slug>/ that
// the next listSkills() pass will surface in place of the bundled copy.
// On the very first shadow-creation we also clone the built-in's side
// files (assets/, references/, scripts/, examples/, ...) so the shadow
// folder is self-contained and downstream resolvers — `/api/skills/:id/
// files`, `/example`, `/assets/*`, the system-prompt preamble, and the
// per-turn cwd staging — keep finding the bundled tree even though the
// user's `SKILL.md` is what we serve.
export async function updateUserSkill(
userSkillsRoot: string,
input: SkillUpdateInput,
): Promise<SkillImportResult> {
const name = typeof input?.name === "string" ? input.name.trim() : "";
if (!name) {
throw new SkillImportError("BAD_REQUEST", "skill name required");
}
const description =
typeof input?.description === "string" ? input.description : "";
const body = typeof input?.body === "string" ? input.body : "";
if (!body || body.trim().length === 0) {
throw new SkillImportError("BAD_REQUEST", "skill body required");
}
const slug = slugifySkillName(name);
if (!slug) {
throw new SkillImportError(
"BAD_REQUEST",
"skill name must produce a valid slug (a-z, 0-9, dash)",
);
}
const triggersRaw = Array.isArray(input?.triggers) ? input.triggers : [];
const triggers = triggersRaw
.map((t) => (typeof t === "string" ? t.trim() : ""))
.filter(Boolean);
await mkdir(userSkillsRoot, { recursive: true });
const dir = path.join(userSkillsRoot, slug);
const dirExisted = await stat(dir)
.then(() => true)
.catch(() => false);
// Only clone on the very first shadow over a built-in. If `dirExisted`
// is true, we are editing an already-shadowed skill (or a pure user
// skill); re-cloning would clobber the user's tweaks under the side
// tree. If `sourceDir` is missing or already points at the shadow,
// there is nothing to clone — same dir.
const shouldCloneSideFiles =
!dirExisted &&
typeof input.sourceDir === "string" &&
input.sourceDir.length > 0 &&
path.resolve(input.sourceDir) !== path.resolve(dir);
if (shouldCloneSideFiles) {
try {
await cloneSkillSideFiles(input.sourceDir!, dir);
} catch {
// Non-fatal: SKILL.md still lands below. Side-file resolvers will
// 404 individual entries instead of erasing the whole edit, which
// matches the pre-fix behaviour for unreachable assets.
await mkdir(dir, { recursive: true });
}
} else {
await mkdir(dir, { recursive: true });
}
const md = buildSkillMarkdown({ name, description, body, triggers });
await writeFile(path.join(dir, "SKILL.md"), md, "utf8");
return { id: name, slug, dir };
}
// Copy every entry in `sourceDir` into `destDir` except `SKILL.md` and
// dotfiles. Used by `updateUserSkill` to build a self-contained shadow
// folder over a built-in skill on first edit. We dereference symlinks
// for the same reason `stageActiveSkill` does — the shadow lives under
// runtime data and must not link back into a read-only resource tree.
async function cloneSkillSideFiles(
sourceDir: string,
destDir: string,
): Promise<void> {
await mkdir(destDir, { recursive: true });
let entries: Dirent[] = [];
try {
entries = await readdir(sourceDir, { withFileTypes: true });
} catch {
return;
}
for (const entry of entries) {
if (entry.name === "SKILL.md") continue;
if (entry.name.startsWith(".")) continue;
const src = path.join(sourceDir, entry.name);
const dst = path.join(destDir, entry.name);
await cp(src, dst, {
recursive: true,
dereference: true,
preserveTimestamps: true,
});
}
}
export interface SkillFileEntry {
// Path relative to the skill's on-disk directory. Forward-slashes only.
path: string;
// 'file' | 'directory'. We do not surface symlinks or other file types.
kind: "file" | "directory";
// Byte size for files; null for directories.
size: number | null;
}
const SKILL_FILES_MAX_ENTRIES = 500;
const SKILL_FILES_MAX_DEPTH = 6;
// Walk a skill directory and return a flat list of files/folders. Used by
// the Settings → Skills detail panel to render a small file tree next to
// the SKILL.md preview. Skips dotfiles, symlinks, and anything past
// `SKILL_FILES_MAX_DEPTH` so a pathological skill folder cannot stall the
// daemon. The cap on entries protects against large bundled assets folders.
export async function listSkillFiles(skillDir: string): Promise<SkillFileEntry[]> {
const out: SkillFileEntry[] = [];
const seen = new Set<string>();
async function walk(dir: string, depth: number): Promise<void> {
if (depth > SKILL_FILES_MAX_DEPTH) return;
if (out.length >= SKILL_FILES_MAX_ENTRIES) return;
let entries: Dirent[] = [];
try {
entries = await readdir(dir, { withFileTypes: true });
} catch {
return;
}
entries.sort((a, b) => a.name.localeCompare(b.name));
for (const entry of entries) {
if (out.length >= SKILL_FILES_MAX_ENTRIES) return;
if (entry.name.startsWith(".")) continue;
// Refuse symlinks defensively — readdir's withFileTypes already
// returns isSymbolicLink(), but we double-check via the Dirent's
// kind methods to keep this aligned with the read paths elsewhere.
if (entry.isSymbolicLink()) continue;
const abs = path.join(dir, entry.name);
const rel = path.relative(skillDir, abs).split(path.sep).join("/");
if (seen.has(rel)) continue;
seen.add(rel);
if (entry.isDirectory()) {
out.push({ path: rel, kind: "directory", size: null });
await walk(abs, depth + 1);
} else if (entry.isFile()) {
let size: number | null = null;
try {
const s = await stat(abs);
size = s.size;
} catch {
size = null;
}
out.push({ path: rel, kind: "file", size });
}
}
}
await walk(skillDir, 0);
return out;
}
export async function deleteUserSkill(
userSkillsRoot: string,
id: string,
): Promise<void> {
const slug = slugifySkillName(id);
if (!slug) {
throw new SkillImportError("BAD_REQUEST", "invalid skill id");
}
const dir = path.join(userSkillsRoot, slug);
const root = path.resolve(userSkillsRoot);
const target = path.resolve(dir);
if (target !== dir || !target.startsWith(root + path.sep)) {
// Defence-in-depth: refuse to delete anything outside the user-skills
// root. The slugify above already strips traversal characters.
throw new SkillImportError("BAD_REQUEST", "invalid skill path");
}
try {
await stat(target);
} catch (err) {
if (isErrnoException(err) && err.code === "ENOENT") {
throw new SkillImportError("NOT_FOUND", "user skill not found");
}
throw err;
}
await rm(target, { recursive: true, force: true });
}