fix(agents): aliases-only picker, install state, and orphan visibility in the agents TUI - #284
Merged
Merged
Conversation
Foundation A: export `classifyAgentState` / `AgentState` from external-models.ts (single source of truth for --list and TUI state); delete local `RowState` in agents.ts, route `buildListRows` through `classifyAgentState`. Foundation B: add `readInstalledAgentNames(installDir)` to agent-models.ts — one `readdir` call replaces N `fs.access` calls in `buildListRows`. Fix 1: picker cycle now uses `pickerNames(catalog.models)` (aliases only; canonical ID only when model has no aliases) instead of `catalog.selectableNames`. Aliases like 'sol' render bare in the TUI — no "(gpt-5.6-sol)" annotation. Canonical IDs stored via --set are normalised to their picker name on load via `buildPickerNameMap` without writing to disk. `catalog.aliasToId` is no longer referenced in render.ts. Tests: T1 (live-cache exact cycle), T2 (two-alias fixture), T3 (zero-maintenance custom catalog), T4/T14-rewrite (bare alias rendering, red→green falsification recorded), T5 (static guard: aliasToId absent from render.ts).
Extract `mergeTuiRowsIntoMapping` as an exported pure helper from `applyTuiSave` (agents.ts). Only dirty rows modify the mapping — untouched rows (including dormant GPT entries) are byte-identical preserved. This is the inertness guarantee: a row never edited produces no write. Add `rowState(row, proxyEnabled)` to state.ts — pure helper delegating to `classifyAgentState` using the persisted model name (dormantModel when set, otherwise configuredModel) so dormant rows are correctly classified as 'saved-inactive'. Commit 3 will thread installed/inRegistry from AgentRow. Drop "Saved. " prefix from TUI save outro to match --set wording verbatim. Tests: T6 (rowState: active/saved-inactive variants), T12 (mergeTuiRowsIntoMapping: inertness, dirty model, reset to default, dirty effort, pure function). Fix pre-existing test: 'gpt-test-1' canonical id must not appear in picker cycle (alias 'test1' takes its slot — Fix 1 regression in agents-command.test.ts).
Add required `installed` and `inRegistry` fields to AgentRow and optional counterparts to InitRowInput (default true). buildRow populates them; rowState now uses row.installed and row.inRegistry (not hardcoded true/true). buildTuiState: pass installDir, call readInstalledAgentNames once (Foundation B), thread installed per-row, append orphan rows (mapping.agents keys absent from the registry) with inRegistry=false at the end. render.ts: 4th STATE column (AGENT 18, MODEL 32, EFFORT 13, STATE 13 = 78 ≤ 80); renderStateCell(row, proxyEnabled) delegates to rowState; stripAnsi(row.name) mandatory before name-cell rendering (security — arbitrary JSON keys may inject ANSI escape sequences). Tests: T7 (STATE in header), T8 (active state), T9 (not-installed state), T10 (orphan/unknown state), T11 (stripAnsi on name cell).
Add `formatAgentName(name: string): string` to render.ts — capitalizes the first character of the agent name for TUI display. Exactly ONE call site in the row renderer (applied after stripAnsi). The `--list` path in agents.ts does not import or call formatAgentName; raw lowercase names are preserved there. Tests: T13 (formatAgentName unit, TUI capitalization, --list static guard). Update all render-test row-finder calls from lowercase to capitalized names (e.g. 'coder' → 'Coder') to match the new TUI output.
M1 — InitRowInput.installed and .inRegistry are now REQUIRED (not optional). tsc enumerates every construction site; the two production sites in agents.ts already passed explicit values (no change needed there). All 10 buildRow calls in tests/agents-state.test.ts receive explicit installed:true, inRegistry:true — the correct value for normal registry agents. The ?? true fallbacks in buildRow are removed; undefined would have silently produced 'unknown' state. M2a — Width guarantee tests at 80 and 60 cols (AC-P3-WIDTH, 2 new tests in agents-render.test.ts). At 60 cols the responsive-scale block produces column widths agent=13, model=24, effort=10, state=10 (total row = 59 ≤ 60). The keybindingsLine (77 chars) previously exceeded 60 cols — fixed in render.ts by slicing the raw text to dims.cols before applying dim(), so the line is never wider than the terminal at any width. M2b — --list AGENT cell format and round-trip (AC-P3-LIST, 2 new tests in agents-command.test.ts). Every name from buildListRows matches ^[a-z0-9-]+$ (lowercase identifiers only; capitalization is TUI-only via formatAgentName). Round-trip test verifies every name from --list is present in getAllAgentNames() and therefore accepted by --set validation. M3 — T14 falsification confirmed concretely. Temporarily restored the alias→canonical-ID annotation branch in renderModelCell (passing aliasToId through RenderModelCellOptions and renderFrame), ran tests/agents-render.test.ts, captured exit code 1 with assertion: "expected '❯ Coder sol (gpt-5.6-sol)…' not to contain 'sol (gpt-5.6-sol)'" Reverted, re-ran: exit code 0, 50/50 pass. T14 is not vacuous. Suite: 84 files / 2727 tests / 0 failures (+4 tests vs 2723 baseline). tsc --noEmit: clean. npm run build: clean. Co-Authored-By: Claude <noreply@anthropic.com>
Two defects in code added by this PR, both confirmed by falsification (production behavior temporarily reverted, new tests observed RED). 1. STATE column reported a stale dormancy marker (state.ts rowState). rowState keyed on `dormantModel ?? configuredModel`, so a dormant row the user cycled onto a live Claude model kept rendering 'saved-inactive' even though mergeTuiRowsIntoMapping would persist 'opus'. The marker appeared on the keypress but never cleared on one. Now classifies the model the row WILL persist, mirroring the merge rule exactly: configuredModel when dirty, else dormantModel ?? configuredModel. Also drops a "Commit 3 will..." transition comment describing work that already landed (applies ADR-003). 2. Orphan row names could break the frame (render.ts). Fix 3 newly renders arbitrary agent-models.json keys, and stripAnsi preserves TAB and LF by contract — so a key containing \n emitted an embedded newline into a frame line, violating renderFrame's one-string-per-line contract and desyncing terminal.ts's redraw, while \t measured as one char in padToVisible but occupied up to eight terminal columns. New sanitizeCell() collapses both at the display boundary; the raw key is untouched so the save-path merge still targets the real mapping key. Also: keybindings footer now truncates via truncateVisible instead of a bespoke .slice(), so the renderer has one truncation primitive. Tests (+5 net, 171 -> 176 across the three agents files): - rowState: in-session GPT selection with proxy off and nothing saved (fixture where configuredModel and originalModel DIFFER and disagree on the answer, so it distinguishes them); marker clears when a dormant row is cycled onto a Claude model; stays on for a different GPT model; stays on when cycled back onto its own saved value. Replaces an exact-duplicate rowState case (avoids PF-018 mechanism 7, label-only scenario). - render: newline and tab in an orphan name never reach the frame, and STATE stays at its declared column offset. Suite: 84 files / 2732 tests / 0 failures (2727 baseline, +5 accounted per-file). tsc --noEmit clean, npm run build clean.
…g-Analyzer) Title-case each hyphen-separated segment rather than uppercasing only the first character, so multi-segment names render correctly in the TUI: bug-analyzer → Bug-Analyzer my-custom-agent → My-Custom-Agent claude-md-auditor → Claude-Md-Auditor code → Code (single-segment: unchanged) `--list` output remains raw lowercase — the AGENT column is an identifier users copy into `--set`, which exact-matches. Update all 7 locations that pinned the first-char-only behavior: docstring, CHANGELOG Fix 4 entry, and 5 test groups (T10 orphan row, T11 ANSI strip, T13 direct unit assertions, T13 TUI integration, frame "shows all three agents" and "non-cursor rows" checks). Co-Authored-By: Claude <noreply@anthropic.com>
dean0x
added a commit
that referenced
this pull request
Aug 19, 2026
…ents TUI + HUD fixes (#287) > **Do not merge yet.** Two verification steps remain that cannot be run from an automated session — see [Before this can merge](#before-this-can-merge). Everything else is done and evidenced. ## What this is An integration branch carrying five related changes to devflow's agent system. Four were built as separate, individually-reviewable PRs (#282, #283, #284, #285) that each merged into this branch rather than into `main`, because **they are individually reviewable but only jointly verifiable** — PR 4's checks exercise a column PR 3 added, and PR 2's count changes ripple into PR 4's documentation. Reviewing them as one diff against `main` is the intended path. A fifth change — a fix to the HUD status line — was found while reviewing this work and is merged in as `fix/hud-base-branch`. It is described below alongside the others. `main` has been untouched throughout. If this branch is abandoned there is nothing to revert. ## Why Three unrelated problems, bundled because they all land in the same blast radius: 1. **The agent roster read as actor-nouns** (`coder`, `designer`, `reviewer`) where action-verbs were wanted (`code`, `design`, `review`). Cosmetic in isolation, but renaming an agent in this codebase is genuinely dangerous — see the next section. 2. **The CLAUDE.md audit feature was unwanted** and needed removing outright. 3. **The `devflow agents` TUI had four defects**, found by running it in a real terminal: aliases *and* full model IDs both appeared as separate picker stops for the same model; GPT selections were silently accepted while inert; there was no install-state column; and agent names rendered lowercase. --- ## The one thing a reviewer must understand first **A devflow agent's identity is three independent strings, and nothing in the codebase links them.** | Form | Example | Authoritative in | |---|---|---| | **A — slug** | `bug-analyzer` | filenames, `DEVFLOW_PLUGINS[].agents[]`, `~/.claude/agents/devflow/{slug}.md`, `agent-models.json` keys, `devflow agents --set {slug}` | | **B — spawn key** | `BugAnalyzer` | the frontmatter `name:` on line 2 of each agent file | | **C — prose** | "the Coder agent" | agent bodies, skills, the orchestrator charter, rosters, docs | Form B is **not derived** from form A, and it diverged in the shipped tree: `bug-analyzer.md` declared `name: BugAnalyzer` (hyphen dropped), with the slug appearing nowhere in the file. The critical part: **nothing in `src/` ever parses that `name:` field.** Only Claude Code does, at spawn time. So a rename that correctly updates the filename, the registry, and every `subagent_type` literal — but misses line 2 of one agent file — leaves **every test green and every spawn broken at runtime**. There was no guard for this; the equivalent guard existed for skills but had never been written for agents. That risk is why PR 4 lands guards *before* it renames anything, and why the guard work is worth reviewing more carefully than the rename itself. --- ## What's in it ### PR #282 — install/uninstall lifecycle (9 files) Foundation for the rest. Cleanup was previously asymmetric: skills got a registry-diff sweep (gated to full installs only), while agents and commands relied on hand-maintained legacy name lists. - New shared `src/core/orphan-sweep.ts`; one compute site used by both the installer and the uninstaller. - The sweep is now **ungated** and covers agents, commands, and skills — it runs on every install shape, including `--plugin` partial installs. - Wired up `getAllCommandNames()`, which existed but was unused anywhere in `src/`. **The safety property to check:** `getAllAgentNames()` / `getAllCommandNames()` / `getAllSkillNames()` span **all** plugins regardless of which are selected. That is what lets assets belonging to *unselected* plugins survive a partial install — the sweep deletes only names that left the registry entirely. If any code path intersects with the selected-plugin subset, that is a data-loss bug. This is also **why PRs 2 and 4 add zero legacy-list entries**: retired names are swept automatically once they leave the registry. Uninstall fixes in the same PR: `removeAllDevFlow` / `removeSelectedPlugins` / `isDevFlowInstalled` are now exported (previously unexported and therefore untestable); the selective path sweeps retired assets and calls `revertExternalAgents`; `--keep-docs` is honored in `resolveDevflowDirCleanup` (previously an active data-loss path that could prompt to wipe skill shadows and `preference-profile.md`); the artifact list is completed; and a containment precondition prevents any artifact path from resolving to `~/.devflow` itself or above it. **A classification invariant was introduced and is enforced by a test:** `enumerateUserDevFlowContent` (user state, survives unless explicitly confirmed) and the install-artifact list (removed on *every* path, including decline, cancel, and `--keep-docs`) must be **disjoint**. A name in both makes the confirmation prompt untruthful — it is listed as user content that removal would take, then deleted regardless of the answer. `agent-models.json` and `migrations.json` are artifacts; `hud.json` is user state. ### PR #283 — remove the audit-claude feature (23 files) Deletes the `devflow-audit-claude` plugin, the `claude-md-auditor` agent, the `/audit-claude` command, and the now-dead non-selectable-optional carry mechanism. **Why the carry was deleted rather than retargeted:** `devflow-audit-claude` was the only plugin both `optional: true` and inside `partitionSelectablePlugins`'s `EXCLUDED`, so once it is gone the helper's only reachable return value is `[]` — a guard for a now-impossible state. Keeping it would have meant retargeting its 17 `it()` blocks at synthetic fixtures, asserting the helper's algebra against plugins that do not exist. It is replaced by a recorded JSDoc invariant at the surviving seam: *an optional plugin added to `EXCLUDED` needs a re-init carry.* Documentation counts were **re-derived, not decremented** — the previous "23 plugins (12 core + 10 optional language/ecosystem + 1 optional workflow)" was already wrong on both the split and the language-plugin count. Now `22 plugins (12 core + 10 optional)` and `16 agents`. ### PR #284 — agents TUI (11 files) - **Aliases-only picker, rendered bare.** Per model in registry order: contribute all of its aliases, and its canonical ID only if it has none. `sol (gpt-5.6-sol)` now renders as `sol`. `renderModelCell` drops from four branches to three. The other two parentheticals are deliberately kept — `default (opus)` reports the shipped default and `(unavailable)` is a status marker, not an identifier. - **The rule is entirely catalog-driven and requires no devflow-side maintenance.** Nothing is hardcoded; new models from subswitch appear automatically, and a model that later gains an alias starts rendering as the alias with no code change. - **Inertness at selection time** — the dormancy marker now appears on the keypress (and, after review, correctly *clears* on one). - **A fourth STATE column** (budget 2+18+32+13+13 = 78 ≤ 80), plus orphan `agent-models.json` keys rendered as editable rows. - **Capitalized names in the TUI only.** `--list` deliberately stays lowercase: its AGENT column is an identifier users copy into `--set`, which exact-matches. `src/core/model-discovery.ts` is byte-identical on purpose — its `selectableNames` doubles as the `--set` validation allowlist, so narrowing it would have rejected `--set coder --model gpt-5.6-sol`. ### PR #285 — the rename (97 files) | Old | New | Old | New | |---|---|---|---| | `coder` | `code` | `simplifier` | `simplify` | | `designer` | `design` | `skimmer` | `skim` | | `evaluator` | `evaluate` | `synthesizer` | `synthesize` | | `researcher` | `research` | `tester` | `test` | | `reviewer` | `review` | `triager` | `triage` | | `scrutinizer` | `scrutinize` | `validator` | `validate` | | `bug-analyzer` | `diagnose` | | | Unchanged: `git`, `knowledge`, `learning`. **`/bug-analysis` is NOT renamed** — the plugin, the command, the skill, `.devflow/docs/bug-analysis/` and `tests/bug-analysis/` all stay. Only the agent moved. (`bug-analyzer` and `bug-analysis` share a prefix; the only safe discriminator is the trailing `er`/`zer`.) Six guards were added, five of them *before* any rename, and each was individually falsified — mutated until it went red with the assertion captured, then restored: - **GAP-1** form A ↔ form B for every agent, via an exception map that shrank to **empty** and is now asserted empty. - **GAP-2** `agentType:` coverage over the `dynamic-*` commands — roughly 40 spawn sites that **no test read at all** before this. Tolerates the no-space `agentType:"X"` form. - **GAP-3** the orchestrator charter names only live agents, plus a size assertion: the hook that injects it fails **open and silent** past its cap, so an oversized charter is simply never injected with no error. - **GAP-4** roster model tiers vs frontmatter `model:`. - **GAP-5** a retired-name sweep using **maximal recall** (case-insensitive, no word boundary) over a corpus asserted non-empty at 248 files (src/assets + dist/commands + docs/reference + the git-tracked `.devflow/features/` knowledge bases), with a bidirectional allowlist. - **GAP-6** pins the 13 shipped key mappings and their invariants. All dist-reading guards now **fail loudly** naming `npm run build` when `dist/` is absent, rather than skipping. Two pre-existing fail-open guards were fixed the same way. **Key migration:** `LEGACY_AGENT_KEYS` maps the 13 old slugs to canonical names, applied by one pure `canonicaliseAgentKeys` used by **both** consumers — `readAgentMapping` and a `scope:'global'` migration entry — so they cannot drift. It parses raw JSON rather than routing through `readAgentMapping`, which drops invalid values and would silently delete user data on a round trip. Prototype-safe throughout (`Object.create(null)` + `Object.hasOwn`), which also closes an argv trap where `--set constructor` would otherwise return a function into `path.join`. No `LEGACY_AGENT_NAMES` entries were added — PR #282's registry-diff sweep removes the old installed files automatically once the names leave the registry. ### `fix/hud-base-branch` — HUD status-line base detection (2 files) Found while reviewing the above: the status line reported **3 changed files** on a branch that actually had 119. `detectBaseBranch()` in `src/hud/git.ts` scanned the HEAD reflog for the branch most recently checked out *from* and treated that as the merge base. A reflog entry is not a merge base — here it selected a feature branch already fast-forwarded into the current one, so the "base" collapsed to roughly HEAD and the counter collapsed with it. A locally merged or deleted branch poisons it, and because `logs/HEAD` is per-worktree, the same branch reported different numbers from different checkouts. Two further defects in the same file: - **Range semantics were mixed.** The diff used `git diff <ref>` (working tree vs ref) while the ahead/behind arrow fifteen lines above used three-dot `<ref>...HEAD`. When the base carries commits not in HEAD, the diff bleeds in *reverse* changes. - **An uncached `gh pr view` network call sat on the render path**, executed on every status-line render and bounded only by a 1s timeout. The docstring claimed that layer was cached; it was not. Now the default branch resolves deterministically via `origin/HEAD`, falling back through `origin/main|master|develop|trunk` then local equivalents, with **no network call**. When nothing resolves the counter renders as absent rather than as a confidently wrong number. The reflog heuristic and the `gh` layer are deleted outright rather than kept as fallbacks — a fallback that fires in exactly the confusing cases is worse than none. `--fork-point` was deliberately avoided: it depends on reflog data and degrades in fresh clones and worktrees, the very fragility being removed. The diff and the arrow now share a merge base. The diff still includes uncommitted work (useful in a status line); the arrow stays commits-only. That asymmetry is deliberate and commented. **Why its test lives in `tests/integration/`:** `src/hud/git.ts` had zero coverage — every HUD test fed hand-written values into the rendering layer, and `tests/hud-render.test.ts` hardcoded `ahead: 2, filesChanged: 3`, which is precisely the wrong output this bug produced. The new test uses real temporary git repositories, so it belongs in the integration suite the repo already excludes from the default run. Placed in the unit suite it deterministically broke 14 unrelated timeout-prone tests through git-subprocess contention (measured: `main` green twice at ~21s; branch red 3/3 at ~58s with an identical failure set). Relocated, the unit suite returns to the `main` baseline exactly. --- ## How to review this Suggested order, roughly by risk: 1. **`src/core/orphan-sweep.ts` and its two call sites.** This is the only new deletion primitive. Confirm it never intersects with the selected-plugin subset. 2. **The uninstall classification invariant** in `src/cli/commands/uninstall.ts` — the disjointness of user-state vs install-artifact, and the containment precondition. 3. **`tests/agent-name-guards.test.ts`.** This is the safety net for the rename and the highest-value file in the branch. Worth asking of each guard: *can this actually fail?* 4. **`src/core/agent-models.ts` + `src/core/migrations.ts`** — the key migration, especially the raw-JSON parse and the prototype safety. 5. **The rename diff itself.** Large but mechanical. `git diff main...wave/agent-roster -- src/assets/agents/` shows the 13 renames as git renames rather than delete+add. 6. **`src/cli/agents-view/`** for the TUI changes. 7. **`src/hud/git.ts`** for the status-line fix — small, self-contained, and its test is in `tests/integration/`. Two things that look wrong but are deliberate: - **`LEGACY_SKILL_NAMES` and the `LEGACY_SKILLS_*` lists are untouched and must stay so.** They are *deletion manifests* for pre-namespace bare directories at `~/.claude/skills/{name}/`, which sit outside the swept namespace. A prefixed entry there would delete a live install; a bare entry for a post-namespace skill would delete a same-named foreign directory. - **`Reviewer Focus Areas` (5 sites, 3 files) is intentionally NOT renamed.** It is a cross-file contract — `plan.mds` emits the heading and `code.md` parses it — and also a PR-description section name. All five or none. --- ## Verification | | `main` | this branch | |---|---|---| | Unit test files | 84 | 85 | | Unit tests | 2,693 | **2,767** | | Unit failures | 0 | **0** | | Integration files | 3 | **4** | | Integration tests | 16 | **37** | | Integration failures | 0 | **0** | Post-review cleanup (4 commits, `9eaba10..0cd3922`) adjusted two of these numbers: residual retired-name occurrences were purged from `docs/reference/` and the tracked `.devflow/features/` knowledge bases, the GAP-5 corpus was extended to cover both directories (233 → 248 files), and the dead `LEGACY_AGENT_NAMES` list was deleted — its two self-asserting tests account for the 2,769 → 2,767 unit-test delta. Suite re-run green at 85 files / 2,767 tests after the cleanup. The integration suite was run twice at the end, both green. It is excluded from the default `vitest run`, so it is easy to forget — and it holds `subagent-skill-preload`, the only end-to-end exercise of the frontmatter `name:` field. One test-harness fix was needed there: `pack-install.test.ts`'s `--version` step had a 15s budget against an 0.08s true cost, and under parallel load had been measuring 2.9s–15.0s. It tipped over once. `runSync` also converted the resulting SIGTERM into a bare `exit 1` with empty stderr, which made a timeout look exactly like a CLI crash. The budget is now 60s and the signal is surfaced. Packaging integrity was separately confirmed against the real tarball: 16 renamed agents present, deleted files absent, `orphan-sweep.js` compiled in, `files[]` and `bin` unchanged, and `--version` printing `2.0.0` on both `main` and this branch. Build and `tsc --noEmit` clean. The integration suite — which the default vitest config *excludes*, and which nothing in this work had previously exercised — also passes (3 files / 16 tests), including `subagent-skill-preload`, the only end-to-end check of form B. Behaviors verified end-to-end against a seeded temporary `HOME`: - Disk migration rewrites keys with **values byte-identical**, records the migration id, and a second `init` is a no-op. - The read-path alias resolves old keys with the file's **sha256 unchanged**, proving the read path and not the disk path produced the result. - **Malformed values survive verbatim** — `{"model":"not-a-real-model","effort":"bogus"}` is preserved rather than dropped to `{}`. (Routing through `readAgentMapping` would have silently deleted them while a loose assertion still passed.) - `--set coder` exits 0 and writes `code`. - A partial install yields exactly 16 agent files, never 26. - Spawn-key closure over freshly built `dist/`: 66 raw matches, all resolving to a live agent's form-B name or the Claude Code built-in `Explore`. Provenance against `main`: `.github/`, `.claude/`, `.devflow/docs/`, `.devflow/memory/` and `.devflow/learning/` are byte-identical. The only change under `scripts/` is the removal of one element from `scripts/build-mds.ts` for the audit-claude deletion. ### Defects caught during review Recorded because they are the kind a reviewer should look for elsewhere: - **The scripted prose pass corrupted a spawn contract.** `_partials/_preamble.mds` enumerates the valid `agentType` *string values*; the PascalCase pass rewrote them to `Code agent, Validate agent, …`. That partial compiles into every dynamic command, so a workflow authored from it would have emitted `agentType: "Code agent"` and resolved to nothing. - **`LEGACY_AGENT_KEYS` briefly shipped empty** because source was edited after the last build — the suite was green against stale compiled output while every dist-reading guard validated the old artifact. - **No test exercised the shipped key mappings.** Every test emptied the production map first and injected synthetic entries; emptying the real map left all 92 related tests green. That is why the empty map above went unnoticed. GAP-6 closes it. - **A guard-export change silently disarmed three existing guards.** Lifting `EXCLUDED` to a module export so an invariant could be asserted destroyed the test file's independent literal oracle, turning three working guards into tautologies that move with production. --- ## Breaking changes 1. **The 13 agent renames.** Any `subagent_type` / `agentType` value in a user's **own** custom commands or agents must be updated by hand — devflow cannot migrate files it does not own. 2. **`--plugin=audit-claude` is now rejected.** Stale `devflow-audit-claude` entries in existing manifests are inert and pruned on the next partial reinstall. 3. **Per-agent model overrides silently stop applying on downgrade.** Retroactive version detection is impossible: `readAgentMapping` never reads the `version` field, and a test pins that it is ignored. There is no mechanism that could warn a user, so it is documented instead. 4. **A session left open across the upgrade holds a stale orchestrator charter** naming the old agents, and should be restarted. --- ## Known gaps, deliberately out of scope - `revertExternalAgents` on the **selective** uninstall path reverts *every* installed agent, not only those being removed, so surviving agents lose GPT frontmatter until the next `devflow init`. Fixing it is an API change to `RevertOptions`. - `devflow agents --list` does not surface orphan `agent-models.json` keys; only the TUI gained them. - An orphan holding a GPT model while routing is off cannot be cleared through the TUI, because dormancy suppresses the dirty flag at both `default` and the dormant value. Fixing it would change dormancy semantics, which this work deliberately holds constant. - Header/footer lines other than the keybindings line remain unbounded below 60 columns (pre-existing). - Test files are not typechecked — `tsconfig.json` scopes to `src/**/*` (pre-existing). - A failed `canonicalise-agent-keys-v1` migration is silently marked applied rather than retried, because `runGlobalMigration` marks any non-throwing return as applied and this migration returns I/O failures as warnings. Net impact is low: `readAgentMapping` re-canonicalises on every read and the file self-heals on the next write. --- ## Before this can merge Two checks cannot be performed from an automated session and are genuinely outstanding: 1. **Live-spawn verification across all 8 workflows.** This is the *only* check that catches a broken `subagent_type` at runtime, because nothing in `src/` parses the frontmatter `name:` field. It cannot be run from a session that is itself using the old agents — installing the renamed agents mid-session breaks the running session. Sequence: `node dist/cli.js init`, then start a **fresh** session, then exercise the workflows. `/dynamic-build` is the only end-to-end check of `agentType:` spawns. **"All N agents succeeded, 0 errors" is not evidence** — agents can return with zero tool uses while an orchestrator counts them as succeeded. Check per-agent tool-use counts are greater than zero. 2. **A TTY pass on the changes from #284** — 9 picker stops, STATE flipping on keypress, no wrapped rows at 80 **and** 60 columns, `Bug-Analyzer` displayed while the JSON key stays lowercase. Preconditions, or the check is vacuous: warm model cache, `proxy.json` `enabled: false`, at least one registry agent absent from the install directory, and `agent-models.json` seeded with both a canonical-ID pin and an orphan key. One further item is **not objectively verifiable** and needs a human judgment call rather than a test: whether the orchestrator, in practice, conflates `Test` the agent with "test" the noun, or `Review` the agent with "review" the verb. There is no ground truth for this; it wants a read of the live transcripts. **Built-in collision check:** all 13 new names were verified clear against the Claude Code build current at the time of this work — the built-in agent types are `Explore` and `Plan`, and none of the new names collide (`Plan` is taken, but devflow has no `plan` agent). This must be **re-checked on each major Claude Code upgrade**: a future built-in could silently shadow a devflow agent, and no in-repo guard can detect that. --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR 3 of 4 in the
wave/agent-rosterwave. Fixes four defects indevflow agents, all found by running the TUI in a real TTY.Fix 1 — aliases-only picker, rendered bare
The cycle previously offered both an alias and its canonical ID as separate stops for the same model. Now: per model in registry order, contribute all of its aliases, and contribute the canonical ID only if it has none. With the current catalog the cycle is exactly 9 stops —
default, haiku, sonnet, opus, fable, sol, terra, luna, gpt-5.5— wheregpt-5.5keeps its stop precisely because itsaliasesarray is empty.Aliases now render bare:
sol (gpt-5.6-sol)becomessol.renderModelCelldrops from four branches to three and no longer referencescatalog.aliasToId. The other two parentheticals are deliberately kept —default (opus)reports the shipped default and(unavailable)is a status marker, not an identifier.The rule is entirely catalog-driven with zero devflow-side maintenance: nothing is hardcoded, new subswitch models appear automatically, and a model that later gains an alias starts rendering as that alias with no code change.
model-discovery.tsis byte-identical —selectableNamesdoubles as the--setallowlist, so narrowing it would have rejected--set coder --model gpt-5.6-sol. Stored full IDs normalize on read and are never rewritten on disk.Fix 2 — inertness at selection time
GPT selections were accepted silently while inert.
rowStatenow classifies the model the row will actually persist, so the dormancy marker appears on the keypress — and, after review, correctly clears on one too. The save outro reuses the--setwording verbatim and the redundantsavedsuffix is gone.mergeTuiRowsIntoMappingis extracted as an exported pure helper; the save path had no tests at all before this.Fix 3 — install state and orphan visibility
A fourth STATE column (budget 2+18+32+13+13 = 78 ≤ 80;
FIXED_ROWSunchanged soterminal.tsis byte-identical). Orphan keys inagent-models.jsonnow appear as editableunknownrows, and clearing one deletes the key.installedandinRegistryare required fields on bothAgentRowandInitRowInputso the compiler enumerates every construction site — an optional field defaulting totruewould silently mark a not-installed agent as installed, which is the exact state this column exists to reveal.Fix 4 — capitalized names, TUI only
Hyphen-aware
formatAgentNametitle-cases each segment (bug-analyzer→Bug-Analyzer), with a single call site.--listdeliberately stays lowercase: its AGENT column is an identifier users copy into--set, which exact-matches.Testing
84 files / 2,732 tests / 0 failures. Build and
tsc --noEmitclean.Tests were written against specific vacuity traps:
soland contains no(gpt-substring, whiledefault (opus)and(unavailable)are asserted separately so a blanket paren-strip cannot pass.Review found and falsified two further defects:
rowStatehad keyed on the pre-edit value so the marker never cleared, andstripAnsipreserves TAB and LF by contract — so an orphan key containing a newline brokerenderFrame's one-string-per-line contract and desynced the terminal writer. Orphan keys are arbitrary JSON, so that was reachable; cell contents are now sanitized at the display boundary while the raw key is preserved for the save path.A 60-column width test caught a real overflow: the keybindings line is 77 raw characters and is now truncated with the same visible-width primitive the cells use.
Notes and known gaps
countDormantSelections, named in the original acceptance criteria, does not exist and never did —git log -Sfinds it on no ref. The session-selection marker isrowState.--liststill does not surface orphan keys; only the TUI gained them.defaultand the dormant value. Fixing it would change dormancy semantics, which this PR deliberately holds constant.tsconfig.jsonscopes tosrc/**/*(pre-existing).Manual verification still outstanding
This PR's TTY behaviors need a human pass: 9 picker stops, STATE flipping on keypress, outro wording, no wrapped rows at 80 and 60 columns,
Bug-Analyzerdisplayed while the JSON key stays lowercase, and--listagreeing with the TUI. Preconditions: warm model cache,proxy.jsonenabled: false, at least one registry agent absent from the install dir, andagent-models.jsonseeded with both a canonical-ID pin and an orphan key.