feat(model-groups): surface effective modalities and fail early on spawn (issue #26)

[grzegorznowak]

Implements issue #26 — "Let the spawn system read model modalities to understand the tasks it's good/bad at".

Related to agenticoding/pi-agenticoding issue #26 (spawn reading model modalities). No issue is auto-closed by this PR.

What this PR does

The spawn system and the main session now understand what each Model Group is capable of, and fail early when a delegated task asks for a capability the selected model/group cannot deliver.

Capabilities are pluggable, not hard-coded to modalities.

• A small model-groups/constraints/ kernel generalizes how model capabilities are derived and checked. One constraint descriptor owns the whole concern for a single capability: how to read the fact off a Model, how to combine it across the group's members, how to reconcile a user override, how to check a spawn requirement, and how to present it in the UI/prompt.
• Modalities are currently the only registered constraint (production registry is exactly modalities). Future capabilities (e.g. a minimum context-window) can be added as another descriptor plus tests, with no edits to the layer that loads config, routes spawns, or renders the UI. This is demonstrated by a synthetic test-only descriptor that runs through the whole path without ever appearing in production code.

Persisted overrides use a single versioned, forwards-compatible shape.

• A group's override lives under a generic constraints map on the group (e.g. constraints.modalities).
• Unknown keys (future capabilities the current plugin does not know about) round-trip unchanged, so a newer config does not degrade.
• Config version stays 2; since this branch has not shipped v2 yet, no migration path is needed.
• For existing v1 configs, behavior is unchanged (a v1 modalityOverride key is preserved as opaque data and dropped on the first v2 write).

Spawn fail-early.

• The spawn tool accepts constraints (a keyed object, schema-generated from the descriptors), normalized at one boundary before routing.
• The router checks the declared requirements against the routed group's capabilities and the exact selected model. Modality violations keep the exact existing missing-modality SpawnRouteError; unknown requirement keys are rejected; the check holds before any child session is created.

Main-session awareness and TUI.

• before_agent_start injects each group's capability summary (e.g. websearch (text, image)) into the orchestrator system prompt, reusing the existing hook.
• The TUI editor and its empty/common + stale-override warnings source from the same constraint layer.

Scope note: intentionally refactor-only

This PR deliberately does not add a cost, minimum-context, or parameter-count feature. It lays the pluggable foundation and exercises it with a synthetic test descriptor so the extension point is proven rather than aspirational. Two constraints of the derivation are handled separately: deciding whether group capability means "all registered members" vs "only authenticated/usable members" is a separate issue, and a parameter-count source does not exist in the host Model type today (model-name inference is not an option).

Review feedback addressed

Review cycle 1 (contract + first implementation review)

• Legacy configs with a malformed modalityOverride previously crashed load; they now validate cleanly and recover with a .bak backup.
• Wording of the "stale override" behavior corrected: persisted stale entries are retained/excised on read, while invalid additions are rejected on mutation.
• Added tests for the load regression, CRUD rejection, the modal editor commit/Automatic interactions, boot notification counts, plain-inherited requiredModalities, and tool-schema validation.

Code-review implementation pass

• Added a registered spawn-tool test asserting requiredModalities rejection throws before any child session (zero factory calls, both session maps empty).
• A success-path registered-spawn test now also asserts liveChildSessions is cleared.
• Documented the locked v1 pass-through no-migration decision and the intended CRUD-only scope of the low-level save helper.
• Corrected the integration fixture comment (the mock registry's Claude is unresolved; it does not add text/empty-common counts the comment implied).
• TUI modal-editor commit tests now select rows by rendered label rather than positional row numbers / hard-coded subset counts.

Pluggability review

• Replaced the modality hard-wiring with the constraint layer + generic envelopes described above.
• Unsupported/wrong requirement handling, empty-common labeling, stale-override editing, and the CRUD-only scope were confirmed or tightened.

Validation (full battery, all PASS)

• npm run typecheck
• npm test (669/669)
• npm run test:e2e (16/16)
• npm run test:snapshots:check (11/11)
• npm run test:compat:current (0.84.2)
• npm run test:package-host
• git diff --check clean; no test-only descriptor key leaks into product files; no stray console output

Refers to #26.

[grzegorznowak]

On the failing current-pi check

The current-pi failure on this PR is not caused by the changes here — it is a pre-existing host-skew issue:

• test:compat:current installs @earendil-works/pi-*@latest (currently 0.84.x) into a temp copy and typechecks against it, while this repo's devDeps pin 0.82.0.
• Pi added registerMarkdownTransformer to ExtensionAPI in 0.84.0. The test double in tests/unit/helpers.ts (deliberate compile-time tripwire) and tests/e2e/test-host.ts are not yet synced, producing exactly the two errors this run reports (tests/unit/helpers.ts Type 'true' is not assignable to type 'never'; tests/e2e/test-host.ts missing registerMarkdownTransformer).
• This reproduces byte-identically on a clean baseline with these changes stashed — zero involvement of this PR's files. The gate is already red on upstream main independently.

Dependency: PR #23 (fix/spawn: harden abort/reset race handling, bump Pi to 0.84.1) addresses exactly this — it adds the registerMarkdownTransformer stub to createTestPI(), modernizes the compat lanes, and bumps Pi to 0.84.1; its current-pi check passes. This PR is intentionally left scoped to the modality feature (no rebase onto #23). Once #23 lands and main picks up the host sync, rebasing this branch (or re-running after merge) should bring current-pi green here without any change to this PR's files.

[grzegorznowak]

Code-review findings (implementation pass) — in scope for this PR

Reviewed against base 4efc9cf; items below are PR-introduced (the pre-existing store hardening cluster — fsync, locking, symlink containment, backup-on-corrupt-overwrite — is filed separately in #28, not blocking here).

1. HIGH — v1 config with a valid hand-added modalityOverride becomes active despite the version gate (regression from the gap-close)

model-groups/store.ts:46:

defineGroup(groups, name, { ...rawDef, models, ...(sourceVersion >= 2 && override.value !== undefined ? { modalityOverride: override.value } : {}) });

The ...rawDef spread copies modalityOverride before the sourceVersion >= 2 conditional, and the conditional only decides whether to re-set it. So a v1 config hand-edited with modalityOverride: ["text"] (a shape that's now valid since validateOverride runs for all versions after the gap-close) loads as v2 with the override active — silently clamping the group's capabilities and potentially cancelling an image-required spawn that automatic common modalities would have allowed.

Suggested fix: strip the field from the spread for sourceVersion < 2, e.g. const { modalityOverride: _drop, ...def } = rawDef; when v1, or destructure it out before spreading.

2. MED — no test exercises requiredModalities rejection through the registered spawn tool

tests/unit/spawn.test.ts covers rejection via direct executeSpawn and schema via Value.Check, but never through registerSpawnTool(...).execute. A wrapper regression that drops/rewrites requiredModalities would stay green. Add a registered-tool invocation with a factory spy asserting SpawnRouteError, zero factory calls, and both session maps empty.

3. MED — success-path spawn test doesn't assert liveChildSessions cleared

The ordinary successful registered-spawn test asserts only childSessions.size === 0; abort/error paths assert both maps. A success-path live-session leak would pass. Assert liveChildSessions.size === 0 on the happy path too.

4. LOW — exported saveModelGroups bypasses the union-cap invariant

createGroup/updateGroup gate overrides against the registry-derived member union, but the exported low-level saveModelGroups has no registry and will persist a syntactically-valid-but-unsupported override (surfaces later only as a "stale override" warning). Either gate at the API boundary or document that cap enforcement is CRUD-only.

5. LOW — spawn group gate derives effective set without hasConfiguredAuth filtering

router.ts group gate uses registry membership only; a modality carried only by an unauthenticated member can pass the group gate. The per-routed-model check backstops this (not exploitable), but worth deciding whether "effective" should mean usable-members-only.

6. LOW — integration fixture mis-describes its own scenario

tests/unit/model-groups-integration.test.ts (empty/stale boot-count test) comments claim claude is text-capable, but the mock registry only contains gpt-5 — Claude is unresolved. The aggregate counts pass under several incorrect implementations; the store-level tests cover this precisely, but the fixture comment should match reality (or the fixture be made real).

7. LOW — brittle TUI editor test

tests/unit/model-groups-tui.test.ts (modality editor commit test) hardcodes row numbering and a long exact keystroke sequence ("row 8 of Automatic + 8 subsets"). Any correct-but-layout-different UI change breaks it. Prefer selecting by rendered label rather than positional keys.

---

Not blocking / verified fine: derivation algebra, RNG/registry race (gate checks the exact routed model object), pre-factory rejection can't leak child sessions, escaping of persisted names and closed-vocab modality labels, editor focus-retention on error.

Pre-existing HIGH store findings (no fsync, no locking, symlink containment, backup-on-corrupt-overwrite) → tracked in #28.

[grzegorznowak]

Code-review findings — all addressed (triage + patch)

Thanks for the thorough pass. Every in-scope item is resolved. Note: the repo restructured during the #23 merge (model-groups/ moved to top-level; src/model-groups paths in the review no longer exist), but all findings reproduced at their current locations.

• Fix README references [warmup pr] #1 HIGH (v1 valid-override pass-through): documented as an intentional, LOCKED no-migration decision in normalizeGroups (comment above the spread). We are deliberately not migrating v1 configs; automatic-common modalities remain valid for v2. No behavior change.
• feat: Readonly Mode #2 MED: added a registered spawn-tool test asserting requiredModalities rejection throws SpawnRouteError (unusable-group / missing-modality) with zero session-factory calls and both childSessions/liveChildSessions empty — covers the wrapper-drop regression you flagged.
• Control automatic handoff availability #3 MED: happy-path registered-spawn test now also asserts liveChildSessions.size === 0.
• Epic: Model tag routing for spawn specialization #4 LOW: documented on the exported saveModelGroups that it is a low-level primitive (used by rename/delete/move); cap enforcement is CRUD-only by design, enforced in createGroup/updateGroup. No signature change.
• fix: replace vague ledger-capture guidance with concrete criteria #5 LOW (deferred A4): re-deferred with the gate semantics documented — the group gate uses registry membership; the per-routed-model missing-check backstops the unauthenticated-member case, so it is not exploitable.
• feat(notebook): rename ledger to notebook with subject-oriented grounding and add topic-awareness layerRefactor/notebook #6 LOW: corrected the integration fixture comment to match the mock registry (claude unresolved → empty common; the override is stale).
• feat: add readonly mode with bash safety and spawn child filtering #7 LOW: TUI modality-editor commit test now selects rows by rendered label (selectRenderedLabel) instead of positional row numbers / an 8-subset count.

Validation after patch: typecheck PASS · unit 652/652 (+1 new test) · e2e 16/16 · snapshots 11/11 · compat:current 0.84.2 PASS · package-host PASS · git diff --check clean.

Pre-existing HIGH store findings (fsync/locking/symlink/backup) remain tracked in #28, out of scope here as you filed them.

[grzegorznowak]

Debt review — all 7 items triaged + resolved (Option C landed)

Reviewed the operator's code-debt pass against the current tree (head 0091fdc, after the B29 debt-easy fixes). All seven PR-specific items are addressed:

#1 (HIGH, pluggability gap) — resolved via Option C. The modality vertical slice (vocab, derivation, persistence, validation, boot counts, TUI editor, spawn schema, route gate) is now backed by a typed, compile-time constraint registry with generic persisted override and spawn requirement envelopes (model-groups/constraints/). Modalities are the first registered constraint; future dimensions (e.g. min-context) add one descriptor + tests, with zero consumer edits — proven structurally in tests by a synthetic scalar testMinContext descriptor traversing resolution → aggregate → persisted codec → reconcile/diagnostic → prompt/editor → group+exact-model via the registered spawn tool (0 factory calls, both session maps empty), while the production registry stays exactly ["modalities"]. Per operator decision this is refactor-only: no cost/context/param-count dimension is materialized.

#2 (four divergent group semantics) — deferred with the auth dimension (deferred debt #2). The engine now receives an explicit ConstraintMemberResolution snapshot; auth-aware aggregation remains a recorded, separate operator decision (not silently resolved here).

#3 (TUI → config projected pollution) — verified fixed by the earlier B29 derived-key strip (persisted config carries only authored/persisted + the new generic envelope; runtime-derived keys and the old modalityOverride/constraints raw are stripped at the save boundary).

#4 (CRUD-only cap) — documented. Generic envelope normalization runs against the registry before mutation; low-level saveModelGroups remains a documented CRUD-only primitive (rename/delete/move are intentionally out of cap scope).

#5 ("name (none)" collision + stale-override editor blind spot) — fixed: the empty-effective prompt label is now unambiguous (no common modalities), and the modality editor builds choices over supported ∪ current override members so stale entries stay visible/selectable.

#6 (requiredModalities machinery) — reconciled: schema is descriptor-generated from the registry; prose stays modality-specific by design (refactor-only); empty == absent documented; the router rejects unknown requirement keys before session creation.

#7 (unbounded uncached derivation) — still deferred (perf), tracked separately. Not resolved by this refactor; a cache/version boundary is a separate follow-up.

All prior review items and B-cycle fixes remain green. Full battery at cd38b (this head): typecheck, unit 669/669, e2e 16/16, snapshots 11/11, compat:current 0.84.2, package-host, git diff --check clean, console scan clean.