Model Groups spawn router#14
Conversation
grzegorznowak
force-pushed
the
model-groups-spawn-router
branch
from
479d78e to
907b4e7
Compare
ofriw
left a comment
ofriw
left a comment
There was a problem hiding this comment.
Thanks for the strong PR — the Model Groups architecture is clean and the feature scope is well-contained.
Must fix before merge
1. /model-groups does not guard missing project/model context
The interactive /model-groups command calls createModelGroupsComponent(...) with ctx.modelRegistry and ctx.cwd unconditionally (model-groups/command.ts). But elsewhere in the same subsystem, missing cwd / modelRegistry is explicitly treated as a valid condition and safely skipped (index.ts, refreshModelGroupsState).
That inconsistency means /model-groups can fail when the command is invoked without a fully initialized project/model context, instead of degrading gracefully.
Expected fix: add the same guard pattern in the command path and surface a friendly message when the required context is unavailable.
2. deleteGroup() can report failure after the delete already succeeded
In model-groups/store.ts, deleteGroup() removes the group and saves the updated config first, then loads the other scope to compute otherScopeHasOverride.
If that opposite-scope load throws (for example because recovery hit a backup-failed malformed config), the function throws after the target delete already committed to disk.
That creates a bad correctness property: callers can observe an error for an operation that already mutated persistent state successfully.
Expected fix: make the opposite-scope lookup non-fatal, or compute it before the delete is committed.
3. Test helper duplication
The new model-group test files duplicate helpers like registry(), group(), and withTemp() in several places. That is valid DRY cleanup work, but it is maintenance debt rather than a release blocker.
…fix deleteGroup error-after-commit - model-groups/command.ts: add guard for missing ctx.cwd or ctx.modelRegistry before calling createModelGroupsComponent; surface friendly notification instead of crashing. Matches refreshModelGroupsState pattern in index.ts. - model-groups/store.ts deleteGroup(): load opposite-scope config before saving the delete so a load failure doesn't throw after the delete is already persisted on disk. Follows moveGroup's pre-validation pattern.
|
Addressed the two must-fix items: 1.
|
- Extract duplicated withTemp() and group() into tests/unit/model-groups-helpers.ts - withTemp: unified async version replacing 2 local variants (crud + integration) - group: unified opts-object signature replacing 3 local variants (tui, router, autocomplete) - registry() kept per-file — each test file needs different model sets
3. Test helper DRY cleanupExtracted shared
Existing 190/190 pass. This comment is AI-generated. |
…del-groups, fix deleteGroup error-after-commit - model-groups/command.ts: add guard for missing ctx.cwd or ctx.modelRegistry before calling createModelGroupsComponent; surface friendly notification instead of crashing. Matches refreshModelGroupsState pattern in index.ts. - model-groups/store.ts deleteGroup(): load opposite-scope config before saving the delete so a load failure doesn't throw after the delete is already persisted on disk. Follows moveGroup's pre-validation pattern.
ofriw
left a comment
ofriw
left a comment
There was a problem hiding this comment.
PR #14 Review — Model Groups & Spawn Routing
Assessment
Clean architecture, good test coverage, well-contained scope. One correctness issue to fix before merge. Rest is tech debt.
Issues
1. 🔴 Model group state can go stale when context is unavailable
refreshModelGroupsState in index.ts:47 returns early without clearing state.modelGroups when ctx.cwd or ctx.modelRegistry is missing. Stale groups from a previous valid session remain in state and get injected into the system prompt via before_agent_start (line 208). The session reset in resetState does clear this, but that only runs on /new — not between agent runs within the same extension lifetime. Fix: clear state.modelGroups.groups and state.modelGroups.validation before the early return.
2. 🟡 createModelGroupsComponent is 378 lines
model-groups/tui.ts:76–453. Project guideline: functions above 20 lines should be broken into smaller units. The TUI component contains 7 screen renderers, input handling, and CRUD operations in a single closure. Track as tech debt.
3. 🟡 modelRegistry accessed via as any casts
Four occurrences across index.ts:46–47 and spawn/index.ts:224–225. The ExtensionContext type doesn't include modelRegistry, so every access bypasses type safety. If the upstream property changes shape or name, there's no compile-time protection.
4. 🟢 Unused state field: wizardThinking
model-groups/tui.ts:88. Declared in the TUI state object but never read or written anywhere. The wizard reads thinking options directly via thinkingOptionsFor(currentWizardModel())[state.row].
5. 🟢 Missing SpawnRouteError integration test
model-groups-router.test.ts covers SpawnRouteError at the router level (empty group, no usable models). But no test in spawn.test.ts verifies the error propagates correctly through executeSpawn to the caller — that boundary is untested.
Already addressed
The contributor fixed 3 items from an earlier review pass: context guard in /model-groups command, deleteGroup() save-before-load ordering, and test helper extraction. All verified.
What's good
- Pure router —
resolveSpawnModelRouteis side-effect-free with an injectablerngseam for deterministic testing - Boot validation — eagerly validates groups against the model registry at startup, surfaces misconfigurations via operator notifications instead of failing at spawn time
- Scoped persistence — backup-before-overwrite, typed
ModelGroupsPersistenceErrorwith phase tracking, andpartialMovestate formoveGroup - Names-only prompt guidance — group internals (provider, model, thinking, auth) are never exposed to the child agent
- Test coverage — 6 new test files covering CRUD, routing, TUI, autocomplete, integration, and spawn rendering with good edge-case depth
2 participants
Summary
Implements the Model Groups spawn-router story end to end: operators can define named model groups, get prompt/autocomplete guidance for them, and route spawned child agents through a group while the default inherited spawn path remains unchanged.
What changed
#group-nameautocomplete for effective groups; suggestions show compact provider/model/thinking details for the operator while inserting only prompt text.spawnwith optional exactgrouprouting and stopped advertisingthinking; stale cachedthinkingarguments are accepted but ignored.Out of scope
spawn.Verification
npm testpassed (190/190).npm run test:e2epassed (10/10).npx tsc --noEmitpassed.npm audit --omit=dev --audit-level=moderatepassed.6733bfdsucceeded: https://github.com/agenticoding/pi-agenticoding/actions/runs/27600878769CI note / follow-up
HOME,USERPROFILE,HOMEDRIVE, andHOMEPATHfor model-groups tests.--omit=dev. The full dev audit is still blocked by upstream shrinkwrapped Pi dev dependencies, so this PR does not claim the upstream Pi audit issue is fixed. Restore strict full-dev audit after the upstream fix is released and the lockfile is updated.References
model-tag-routermodel-groups-spawn-router— Model Groups spawn router