refactor(agents)!: rename agents to action verbs - #285
Merged
Merged
Conversation
…e 1 of 4) GAP-1: assert every agent's frontmatter name: matches its filename slug using capitalizeFirst(slug) as the default, with a SHRINKING exception map that starts with one entry (bug-analyzer → BugAnalyzer). The map must be empty after phase 4 — that is the wave's acceptance criterion. GAP-2: agentType: coverage — dynamic commands (which use agentType: not subagent_type) were unverified. The new guard asserts set-equality between the 11-agent _roster.mds table and actual agentType: values in dist, plus roster ⊆ registry. Tolerates the no-space form agentType:"X". Excludes the Explore built-in. Fails loud when dist is absent. GAP-3: orchestrator charter byte-size cap — the session-start-orchestrator hook silently skips injection past 4 096 chars. Assert ≤ 75% of cap (3 072). Also sweeps for retired agent names (vacuous today; active from phase 4). GAP-4: roster model tiers — assert that _roster.mds model column matches each agent's frontmatter model:. Reads from compiled dist/commands/dynamic- build.md (fail-loud when dist absent). All 11 currently match. GAP-5: retired-name sweep — generalizes the dream→learning guard with maximal recall (case-insensitive, no trailing boundary), filesScanned >= 220 corpus guard, and form B derived from frontmatter (not Capitalize(slug)). Ships with an empty retired-names list; populated in phase 4. Fix two existing fail-open sites: - registry-integrity.test.ts Guard 5: was `if (!distExists) return;` - skill-references.test.ts:865: was `continue; // dist not yet built` Both now throw with a message naming `npm run build`.
…-migration mechanism
Key-migration infrastructure — INERT until phase 4 populates LEGACY_AGENT_KEYS.
- LEGACY_AGENT_KEYS: exported Object.create(null) map of old-key → canonical-key.
Prototype-null + Object.hasOwn() throughout to prevent __proto__ injection.
Ships empty; populated in phase 4 when agent renames are applied.
- canonicaliseAgentKeys(): pure function that renames legacy keys in a raw agents
map. Fast-path no-op when the map is empty (common case). Idempotent under
concurrent execution — two worktrees applying simultaneously produce identical
results (no lock required). Guards __proto__ on both old-key and new-key sides.
- Wire into readAgentMapping (all 4 call sites at once). Also fixes the agents: []
array trap: typeof [] === 'object' && [] !== null, so Array.isArray() check added.
- Wire into MIGRATIONS as first scope:'global' entry
('canonicalise-agent-keys-v1'). Reads raw JSON (not readAgentMapping) to avoid
silently dropping user data on round-trip. Handles BOM, empty file, invalid JSON,
missing/null/array agents field. Known issue documented: retries forever on
failure (no cap/backoff) — out of scope for this wave.
- Wire into agents.ts --set: accepts old keys with an info line pointing to the
canonical name.
Tests: 12 new canonicaliseAgentKeys unit tests (fast-path, rename, collision,
idempotency, __proto__ guards, LEGACY_AGENT_KEYS prototype-null assertion, array
trap, integration via readAgentMapping). 11 new migration unit tests (fast-path,
rename + file write, ENOENT, missing agents field, no legacy keys present, invalid
JSON, BOM strip, agents array warning, envelope preservation, idempotency). Updated
MIGRATIONS registry test ('is empty' → asserts canonicalise-agent-keys-v1 entry).
…m A+B)
Atomic identity rename across Form A (slug/filename) and Form B
(frontmatter name: field + spawn key strings) for all 13 non-exempt agents.
Renames:
coder → code (Coder → Code)
designer → design (Designer → Design)
evaluator → evaluate (Evaluator → Evaluate)
researcher → research(Researcher → Research)
reviewer → review (Reviewer → Review)
scrutinizer → scrutinize (Scrutinizer → Scrutinize)
simplifier → simplify(Simplifier → Simplify)
skimmer → skim (Skimmer → Skim)
synthesizer → synthesize (Synthesizer → Synthesize)
tester → test (Tester → Test)
triager → triage (Triager → Triage)
validator → validate (Validator → Validate)
bug-analyzer → diagnose (BugAnalyzer → Diagnose)
Unchanged: git/Git, knowledge/Knowledge, learning/Learning.
Unchanged: /bug-analysis plugin, command, skill.
Preserved: all five "Reviewer Focus Areas" cross-file contract sites.
Preserved: all subagent_type="Explore" Claude Code built-in sites.
Deferred: Form-C prose rewrites and contract variable renames (Phase 3).
Changes:
- git mv all 13 agent files; update frontmatter name: in each
- Update plugins.ts agents[] arrays with new slugs
- Update all spawn strings in .mds and .md command sources
- Add _roster.mds table entries with new names
- Update reviewerThunks → reviewThunks in dynamic-build.mds + test assertion
- Update evaluateVerdict/testVerdict identifiers in _engine.mds
- Empty SLUG_TO_NAME_EXCEPTIONS (diagnose = capitalizeFirst('diagnose'))
- Rename tests/skimmer-agent.test.ts → tests/skim-agent.test.ts
- Update all test fixtures, path literals, and assertions to use new names
Verification: 85 files / 2760 tests / 0 failures; all 5 GAP guards GREEN.
…e 3 — Commit 4) Mechanical renames (new name reads fine as-is): SCRUTINIZER_OUTPUT → SCRUTINIZE_OUTPUT (implement.mds, 2 sites) SIMPLIFIER_COMMITS → SIMPLIFY_COMMITS (self-review.mds, 1 site) SKIMMER_CONTEXT → SKIM_CONTEXT (plan.mds, 4 sites) simplifierTranscript → simplifyTranscript (subagent-skill-preload.test.ts, 2 sites) Disambiguated renames (bare verb form reads wrong): CODER_OUTPUT → CODE_AGENT_OUTPUT (implement.mds, 1 site) CODER_RESULTS → CODE_AGENT_RESULTS (resolve.mds, 7 sites) REVIEWER_LIST → REVIEW_FOCUS_LIST (code-review.mds, 3 sites) REVIEWER_OUTPUTS → REVIEW_FOCUS_OUTPUTS (code-review.mds, 2 sites) SINGLE_CODER → SINGLE_CODE_AGENT (implement.mds + plan.mds + synthesize.md, 20 sites) SEQUENTIAL_CODERS → SEQUENTIAL_CODE_AGENTS (13 sites across same files) PARALLEL_CODERS → PARALLEL_CODE_AGENTS (10 sites across same files) Preserved: ANALYZER_OUTPUTS (generic focus analyzers, not the agent) Preserved: parseReviewFocusAreas (already correct from phase 2)
…rences (Phase 3 — Commit 5) Replace all PascalCase prose references to old noun-form agent names with action-verb equivalents throughout src/assets/ and source TypeScript/test files. Convention applied: write "the Code agent", "each Review agent" (append " agent"); use bare verb only in spawn literals (agentType: "Review") and roster tables. Scope: - 15 agent .md files: headings corrected (e.g. "# Scrutinize agent") - 14 command .mds files + 6 partials: Coder→Code agent, Reviewer→Review agent, Scrutinizer→Scrutinize agent, Simplifier→Simplify agent, Skimmer→Skim agent, Designer→Design agent, Researcher→Research agent, Triager→Triage agent, BugAnalyzer→Diagnose agent, Validator→Validate agent, Evaluator→Evaluate agent, Synthesizer→Synthesize agent, Tester→Test agent - Orchestrator charter: routing tier entries updated (2,177 bytes, well within 3,072 cap) - 9 skill files: Researcher→Research agent, Reviewer→Review agent, etc. - src/core/plugins.ts, src/cli/commands/init.ts: display strings updated - src/cli/agents-view/render.ts: JSDoc example updated - tests/plugins.test.ts, tests/registry-integrity.test.ts: test descriptions updated Protected invariants: - "Reviewer Focus Areas" preserved at all 5 sites (cross-file contract) - invalidator (3 sites), validator in skills (~17 lib refs), /tmp/devflow-tester- (2 sites) untouched - LEGACY_AGENT_KEYS / old-key test data untouched - ANALYZER_OUTPUTS untouched Verified: 85 test files / 2,760 tests pass; all 7 GAP guards green; 0 "agent agents" duplications
… 4 — Commit 6a) Phase 3 left the 13 renamed agent files with lowercase-agent H1 headings (e.g. `# Code agent`) while the three untouched agents (git, knowledge, learning) kept title case (`# Git Agent`). This commit makes all 16 consistent using the pre-existing title-case pattern: Code agent → Code Agent Design agent → Design Agent Diagnose agent → Diagnose Agent Evaluate agent → Evaluate Agent Research agent → Research Agent Review agent → Review Agent Scrutinize agent → Scrutinize Agent Simplify agent → Simplify Agent Skim agent → Skim Agent Synthesize agent → Synthesize Agent Test agent → Test Agent Triage agent → Triage Agent Validate agent → Validate Agent Body prose keeps the settled convention "the Code agent" / "each Review agent" — only the H1 line changes.
Update every remaining reference to old agent names across docs and knowledge bases. All 13 renamed agents now use their new action-verb form throughout: Coder → Code agent / Code Designer → Design agent / Design Evaluator → Evaluate agent / Evaluate Researcher → Research agent / Research Reviewer → Review agent / Review Scrutinizer → Scrutinize agent / Scrutinize Simplifier → Simplify agent / Simplify Skimmer → Skim agent / Skim Synthesizer → Synthesize agent / Synthesize Tester → Test agent / Test Triager → Triage agent / Triage Validator → Validate agent / Validate BugAnalyzer → Diagnose agent / Diagnose Files updated: - CLAUDE.md — model strategy, shared agents roster, orchestration commands, persisting agents, handoff artifact, file-tree comments - README.md — pipeline steps, command table, feature descriptions - docs/cli-reference.md — plugin descriptions - docs/commands.md — command pipeline steps - docs/working-memory.md — KB descriptions and file-tree comments - docs/reference/agent-design.md — frontmatter example, length table - docs/reference/file-organization.md — agents list (line 156) - docs/reference/skills-architecture.md — "Used By" columns, section headings, frontmatter example - docs/reference/skill-catalog.md — compliance skill description - .devflow/features/*/KNOWLEDGE.md — all four shared knowledge bases (resolve-pipeline, dynamic-workflow-engine, compliance-plugin, ambient-orchestrator)
…se 4 — Commit 6c) Documents the full breaking change for the agent rename wave: - Table of all 13 old → new slug (Form A) and name (Form B) mappings - What devflow migrates automatically (agent files, agent-models.json keys) - What users must migrate by hand (their own custom commands/agents) - Downgrade warning: per-agent model overrides silently stop applying (readAgentMapping never reads the version field — test-pinned) - Open-session warning: stale orchestrator charter after upgrade - Claude Code built-in name collision check: verified clear against claude-sonnet-4-6 build 2026-08-18; note to re-check on each major Claude Code upgrade
…S (Phase 4 — Commit 7)
Populate the three stubs left empty after phase 1:
RETIRED_AGENT_FORM_B (agent-name-guards.test.ts): 13 old Form-B names
(Coder, Designer, Evaluator, Researcher, Reviewer, Scrutinizer, Simplifier,
Skimmer, Synthesizer, Tester, Triager, Validator, BugAnalyzer) — activates
the GAP-5 retired-name sweep across src/assets/**/*.{md,mds} and
dist/commands/*.md.
RETIRED_ALLOWLIST (25 entries, bidirectional): every collateral hit is
explicitly categorised (URL_LINK, DATA_FIELD, CONTRACT, CONCEPT, THIRD_PARTY,
EXAMPLE_CODE) and each entry is verified to match at least one live corpus
hit. Protected cross-file contracts (Reviewer Focus Areas — 5 sites, 3 files)
are preserved intentionally. A new it-block asserts every allowlist entry
remains live (stale entries fail the suite).
SLUG_TO_NAME_EXCEPTIONS empty-assertion (GAP-1): new it-block asserts the
exception map is empty — D-RI-1 wave acceptance criterion.
LEGACY_AGENT_KEYS (agent-models.ts): 13 old-slug → new-slug entries enable
the canonicalise-agent-keys-v1 migration to rewrite ~/.devflow/agent-models.json
on user's first init post-upgrade (coder→code, reviewer→review, etc.).
Test harness fix (agent-models.test.ts, migrations.test.ts): both describe
blocks now use beforeEach/afterEach save-clear-restore instead of afterEach-
only-clear, so each test starts from a known-empty map and shipped entries are
restored afterward.
Genuine missed renames from phase 3, fixed rather than allowlisted:
- SIMPLIFIER_OUTPUT → SIMPLIFY_OUTPUT (implement.mds)
- SCRUTINIZER_STATUS/CHANGES/modified_files → SCRUTINIZE_* (self-review.mds)
GAP-5 red/green proof: injecting "<!-- PROBE: Simplifier -->" into
src/assets/skills/security/SKILL.md turned the sweep RED; removal restored GREEN.
85 test files / 2762 tests / 0 failures.
P0 — corrupted agentType literals (Form C scripted pass): _preamble.mds enumerated the valid `agentType` string values as "Code agent, Validate agent, ..." — contradicting both the code example three lines above it and _roster.mds. A workflow script authored from that partial would emit agentType: "Code agent", which does not resolve. This partial is included by every dynamic command. Restored bare values, and the same corruption in the model-tier mapping on the next line. P1 — vacuous test coverage of the shipped rename map (avoids PF-018): Both canonicaliseAgentKeys suites delete every key from the exported LEGACY_AGENT_KEYS in beforeEach and inject synthetic entries, so no test ever exercised the real 13 mappings. Verified by experiment: emptying the shipped map left all 92 tests in the three relevant files green — the exact stale-dist failure mode this wave already hit once. Added GAP-6 in agent-name-guards.test.ts (a file that never mutates the map): pins the 13 pairs, asserts keys are disjoint from the live registry, every value is a live agent, the map is single-hop, and it applies end-to-end against the genuine map. Proven non-vacuous — emptying the map fails 3 of them. Documented the scope limit in both mutating suites. P1 — migration deferral note was inverted: The note claimed a failed canonicalise-agent-keys-v1 would retry forever. It cannot: runGlobalMigration marks any non-throwing return as applied, and that migration catches every I/O failure and returns it as a warning. Probed directly — an unreadable agent-models.json records the migration applied with legacy keys still on disk, never retried. Corrected the note to describe the real behaviour and why it is survivable (readAgentMapping re-canonicalises on every read; the file self-heals on next write), and added the missing test for the untested EACCES read path. P1 — meaning inversion in a load-bearing orchestration contract: "re-Validate agent" reads as an instruction to re-validate the agent rather than to re-run the Validate agent. Now "re-run Validate agent". Also repaired broken pseudo-code (`Code agent(agentType:"Code", ...)`) and a broken report heading (`## Code agent Report:` → `## Implementation Report:`, matching every sibling agent's convention). applies ADR-003 · avoids PF-018 · avoids PF-019
1. skim.md: reword :126 "Skim for structure" → "rskim for structure"
to match the correct phrasing at :66 and avoid ambiguity with the
agent's own name.
2. agent-models.ts / migrations.ts / agents.ts: remove five stale
comments that referenced "phase 1", "phase 4", or "LEGACY_AGENT_KEYS
is empty" — describe current state only.
3. CHANGELOG.md: correct the built-in collision list. The previous list
named devflow's own pre-rename agents as "Claude Code built-ins".
The actual built-ins are only Explore and Plan; updated accordingly.
4. _engine.mds / code.md: reword "multi-Code agent" → "multiple Code
agents" / "chaining multiple Code agents" to eliminate parse ambiguity.
5. diagnose.md, triage.md, git/SKILL.md, _ticket_template.mds,
dynamic-plan.mds, resolve.mds: fix missing articles and number
disagreements left by the scripted prose pass.
6. CLAUDE.md: normalise roster block to bare verb names throughout
("Review agents" → "Review", "Git agent" → "Git", etc.).
7. project-paths.ts + project-paths.cjs: update JSDoc on both sides of
the mirror pair from "coder phase handoff artifact" to "Code agent
phase handoff artifact". Both files changed identically.
8. plan.mds: rename prose references "Explorer" → "Explore" at three
sites (lines 101, 128, 255). All three are prose labels, not
subagent_type= literals — verified before changing.
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 4 of 4 in the
wave/agent-rosterwave. Renames 13 devflow agents from actor-nouns to action-verbs across all three identity forms, adds six structural guards, and migratesagent-models.jsonkeys automatically.BREAKING. See the CHANGELOG entry for the full upgrade note.
codercodesimplifiersimplifydesignerdesignskimmerskimevaluatorevaluatesynthesizersynthesizeresearcherresearchtestertestreviewerreviewtriagertriagescrutinizerscrutinizevalidatorvalidatebug-analyzerdiagnoseUnchanged:
git,knowledge,learning./bug-analysisis NOT renamed — the plugin, command, skill,.devflow/docs/bug-analysis/andtests/bug-analysis/all stay. Only the agent moved.Why this needed guards first
A devflow agent's identity is three independent strings that nothing links: the slug (filename, registry,
agent-models.jsonkeys), the frontmattername:on line 2 — which is what Claude Code actually resolvessubagent_typeagainst — and prose. Form B is not derived from form A:bug-analyzer.mddeclaredname: BugAnalyzer, with the slug appearing nowhere in the file. Nothing insrc/parses that field; only Claude Code does, at spawn time. So renaming the file, the registry and every spawn literal while missing line 2 leaves every guard green and every spawn broken at runtime.Six guards now close that:
agentType:coverage over thedynamic-*commands (~40 spawns that no test read before), tolerating the no-spaceagentType:"X"form.Exploreis exempt as a Claude Code built-in.model:.All five original guards were individually falsified — mutated to RED with the verbatim assertion captured, then restored to GREEN. All dist-reading guards fail loudly naming
npm run buildwhendist/is absent, rather than skipping. Two pre-existing fail-open guards were fixed the same way.Key migration
LEGACY_AGENT_KEYSmaps the 13 old slugs to canonical names, applied by one purecanonicaliseAgentKeysused by both consumers —readAgentMappingand ascope:'global'migration entry — so they cannot drift. It parses raw JSON rather than routing throughreadAgentMapping, which drops invalid values and would silently delete user data on a round trip. Prototype-safe throughout (Object.create(null)+Object.hasOwn), closing the--set constructorargv trap.No
LEGACY_AGENT_NAMESentries were added — PR 1's registry-diff sweep removes the old installed files automatically.Testing
85 files / 2,769 tests / 0 failures; build and
tsc --noEmitclean.Verified end-to-end against a seeded temp HOME: disk migration rewrites keys with values byte-identical and records the migration id, a second
initis 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 it; malformed values survive verbatim rather than being dropped to{};--set coderexits 0 and writescode; a partial install yields exactly 16 agent files, never 26.Defects found and fixed during review
_partials/_preamble.mds, which enumerates the validagentTypestring values, intoCode agent, Validate agent, …. That partial compiles into every dynamic command, so a workflow authored from it would have emittedagentType: "Code agent"and failed to resolve.dist/shipped an empty map while the suite was green. GAP-6 closes it.Notes
subagent_typevalues in their own custom commands and agents — devflow cannot migrate files it does not own.readAgentMappingnever reads theversionfield.