[Maintain 0007] Apply 3-way review feedback

- arch.md: add protocol-schema.json to codev/protocols/ tree
- arch.md: correct spir/ subtree (remove nonexistent manifest.yaml, add
  protocol.json, builder-prompt.md, prompts/, consult-types/)
- arch.md: refresh codev-skeleton/protocols/ subtree (add aspir, air,
  bugfix, spike)
- lessons-learned.md: move Spec 653 lessons into Critical section
- packages/codev: add @xterm/xterm as explicit devDependency instead of
  relying on transitive resolution through codev-dashboard
- codev/maintain/0007.md: document consultation findings and actions

Author: waleedkadous

codev/maintain/0007.md

@@ -33,7 +33,7 @@ Major features merged since run 0006 (partial list):
 Other ts-prune/depcheck findings were investigated but retained:
 
 - `@vitest/coverage-v8` — dynamically loaded by vitest config at `vitest.config.ts:30` (`provider: 'v8'`), depcheck can't detect this.
-- `@xterm/xterm` missing-dep warning — used only in test file `src/__tests__/filePathLinkProvider.test.ts`, resolved transitively through `@cluesmith/codev-dashboard`; no runtime impact.
+- `@xterm/xterm` — was flagged as transitively resolved; per Gemini's 3-way review, pinned explicitly in `devDependencies` (`^5.5.0`) since the test file imports it directly. Relying on a transitive dashboard dep was fragile.
 - `state.ts` util/annotation/architect exports — used in test files; part of public state API surface.
 - `forge.ts`, `github.ts`, `templates.ts`, `scaffold.ts`, `skeleton.ts`, `team.ts` unused exports — all used by their sibling test files. Conservative: keep public API stable.
 - `cli.ts:378 run()` — exported for bin shims (`afx.js`, `consult.js`).
@@ -44,11 +44,13 @@ Other ts-prune/depcheck findings were investigated but retained:
 **`codev/resources/arch.md`**:
 - Refreshed `afx` commands tree: added `spawn-worktree`, `spawn-roles`, `attach`, `architect`, `shell`, `tower`, `tower-cloud`, `cron`, `team`, `team-update`, `db`; removed non-existent `util.ts`.
 - Refreshed `bin/` tree: added `af.js` (deprecation shim) and `generate-image.js`.
-- Refreshed `codev/protocols/` tree: added `aspir`, `air`, `bugfix`, `release`, `spike` (were missing).
+- Refreshed `codev/protocols/` tree: added `aspir`, `air`, `bugfix`, `release`, `spike`, and `protocol-schema.json` (were missing).
+- Corrected `spir/` subtree: removed nonexistent `manifest.yaml`; added actual files (`protocol.json`, `builder-prompt.md`, `prompts/`, `consult-types/`).
+- Refreshed `codev-skeleton/protocols/` subtree: added `aspir`, `air`, `bugfix`, `spike` (was stale — only listed `spir`, `experiment`, `maintain`).
 - Updated version footer: `v2.0.0-rc.54` → `v3.0.0-rc.9`; date bumped to 2026-04-17.
 
 **`codev/resources/lessons-learned.md`**:
-- Added 3 lessons from Spec 653 (builder handling / TICK removal): start from structural insight, full-repo grep for protocol removal, single verify-pass consultation cadence.
+- Added 3 lessons from Spec 653 (builder handling / TICK removal) at the end of the **Critical (Prevent Major Failures)** section: start from structural insight, full-repo grep for protocol removal, single verify-pass consultation cadence.
 - Updated footer date.
 
 **`AGENTS.md`**:
@@ -80,6 +82,26 @@ Other ts-prune/depcheck findings were investigated but retained:
 - **arch.md full rewrite** — `arch.md` is 1,776 lines; ~90% still accurate. A full rewrite is out of scope for a pre-release MAINTAIN pass.
 - **`ts-prune` noise from barrel files** — `terminal/index.ts` re-exports would need per-file annotation (`// ts-prune-ignore-next`) or a config file to silence. Low value.
 
+## Consultation Log
+
+3-way review run before opening PR (prompt at `/tmp/maintain-0007-consult-prompt.md`):
+
+| Reviewer | Verdict | Key findings |
+|----------|---------|--------------|
+| **Gemini** | REQUEST_CHANGES | Missed `protocol-schema.json` in tree; `codev-skeleton/protocols/` subtree still stale; lessons placed under wrong section heading (ran report said Critical, actually appended to end of file); push back on deferring `@xterm/xterm` — one-line fix, just add the devDep. |
+| **Codex** | APPROVE_WITH_COMMENTS | `spir/` subtree listed nonexistent `manifest.yaml`; `codev-skeleton/protocols/` still stale (same as Gemini); noted CLI examples elsewhere in arch.md (`afx util`, `afx tunnel`, `afx ports`) are stale but out of scope. |
+| **Claude** | APPROVE_WITH_COMMENTS | All three trees and dep removal verified against filesystem. Noted pre-existing duplicate step numbering (`# 3`) in local-install section of CLAUDE.md/AGENTS.md (not introduced here). |
+
+### Actions taken in response
+
+- Added `protocol-schema.json` to `codev/protocols/` tree (Gemini)
+- Fixed `spir/` subtree to match actual files (Codex)
+- Refreshed `codev-skeleton/protocols/` subtree (Gemini + Codex)
+- Moved Spec 653 lessons into the **Critical** section (Gemini)
+- Added `@xterm/xterm` to `packages/codev` devDependencies (Gemini)
+- Not addressed: the stale CLI examples elsewhere in arch.md — Codex flagged these as out-of-scope, and fixing them well would require a broader CLI audit. Deferred to a future MAINTAIN pass.
+- Not addressed: duplicate `# 3` step numbering in CLAUDE.md/AGENTS.md. Pre-existing on `main`; belongs in a separate doc tweak.
+
 ## Summary
 
 First run on the simplified (2-phase) MAINTAIN protocol. The dependency cleanup caught up on the one un-acted item from run 0006's findings (`http-proxy`). Documentation sync was the bulk of the work: arch.md's directory trees had drifted significantly over 1,751 commits (VS Code extension, codev-core split, pnpm migration, afx rename, TICK removal). Everything builds and all unit tests pass against the refreshed tree.

codev/resources/arch.md

@@ -1112,15 +1112,19 @@ codev/                                  # Project root (pnpm monorepo)
 │   ├── protocols/                      # Working copies for development
 │   │   ├── spir/                       # Multi-phase with consultation
 │   │   │   ├── protocol.md
+│   │   │   ├── protocol.json
+│   │   │   ├── builder-prompt.md
 │   │   │   ├── templates/
-│   │   │   └── manifest.yaml
+│   │   │   ├── prompts/
+│   │   │   └── consult-types/
 │   │   ├── aspir/                      # Autonomous SPIR (no human gates)
 │   │   ├── air/                        # Autonomous Implement & Review
 │   │   ├── bugfix/                     # GitHub Issue-driven fixes
 │   │   ├── experiment/                 # Disciplined experimentation
 │   │   ├── release/                    # Version release procedure
 │   │   ├── spike/                      # Time-boxed research
-│   │   └── maintain/                   # Codebase maintenance
+│   │   ├── maintain/                   # Codebase maintenance
+│   │   └── protocol-schema.json        # JSON schema for protocol.json files
 │   ├── specs/                          # Our feature specifications
 │   ├── plans/                          # Our implementation plans
 │   ├── reviews/                        # Our lessons learned
@@ -1135,7 +1139,11 @@ codev/                                  # Project root (pnpm monorepo)
 │   ├── templates/                      # Document templates (CLAUDE.md, arch.md, etc.)
 │   ├── protocols/                      # Protocol definitions
 │   │   ├── spir/
+│   │   ├── aspir/
+│   │   ├── air/
+│   │   ├── bugfix/
 │   │   ├── experiment/
+│   │   ├── spike/
 │   │   └── maintain/
 │   ├── specs/                          # Empty (placeholder)
 │   ├── plans/                          # Empty (placeholder)

codev/resources/lessons-learned.md

@@ -38,6 +38,9 @@ Generalizable wisdom extracted from review documents, ordered by impact. Updated
 - [From 0324] `detached: true` and `child.unref()` are necessary but not sufficient for process independence -- any pipe-based stdio (e.g., `stdio: ['ignore', 'pipe', 'pipe']`) creates a lifecycle dependency between parent and child processes. When the parent exits, the broken pipe triggers unhandled EPIPE errors in the child. Use file FDs or `'ignore'` for truly independent daemon children.
 - [From bugfix-274] Startup ordering matters when multiple subsystems share resources (Shellper sockets). Initialization order creates implicit synchronization -- calling `initInstances()` before `reconcileTerminalSessions()` allowed dashboard polls to race with reconciliation. Document ordering constraints in comments.
 - [From bugfix-274] Defense in depth for race conditions: the startup reorder closes the primary race path, but the `_reconciling` guard provides a safety net for code paths that bypass the primary fix (e.g., direct `/project/.../api/state` requests bypassing `getInstances()`).
+- [From 653] Start from the structural insight, not the feature list. The first three spec drafts built elaborate gate-ceremony machinery (checkpoint PRs, feedback commands, verify notes) that was all eliminated once the core insight — break the 1:1 builder↔PR assumption — was identified. When a spec feels bloated, look for the one structural change that makes the ceremony unnecessary.
+- [From 653] Protocol removal requires full-repo grep, not targeted searches. Removing a protocol touches ~50 files across source, docs, templates, skills, tests, and CLI help text. Scoped searches miss skeleton templates, test fixtures, and user-facing help strings. Run `rg` across the entire repo and verify zero hits before committing.
+- [From 653] Single verify pass + rebuttal is the right consultation cadence. Multi-iteration consult loops (running `consult` manually after each fix) violate `max_iterations=1` and add little marginal value over one rigorous verify pass followed by rebuttals.
 
 ## Security
 
@@ -362,10 +365,6 @@ Generalizable wisdom extracted from review documents, ordered by impact. Updated
 - [From 627] Lifecycle phases (initial-load → buffer-replay → interactive) eliminate magic thresholds. Instead of asking "is this scroll event real?", ask "what phase am I in?" to determine behavior.
 - [From 627] Always add a `reset()` method to state machines that persist across reconnections. The ScrollController's phase transitions were one-way until reconnection revealed the need to return to initial-load.
 
-- [From 653] Start from the structural insight, not the feature list. The first three spec drafts built elaborate gate-ceremony machinery (checkpoint PRs, feedback commands, verify notes) that was all eliminated once the core insight — break the 1:1 builder↔PR assumption — was identified. When a spec feels bloated, look for the one structural change that makes the ceremony unnecessary.
-- [From 653] Protocol removal requires full-repo grep, not targeted searches. Removing a protocol touches ~50 files across source, docs, templates, skills, tests, and CLI help text. Scoped searches miss skeleton templates, test fixtures, and user-facing help strings. Run `rg` across the entire repo and verify zero hits before committing.
-- [From 653] Single verify pass + rebuttal is the right consultation cadence. Multi-iteration consult loops (running `consult` manually after each fix) violate `max_iterations=1` and add little marginal value over one rigorous verify pass followed by rebuttals.
-
 ---
 
 *Last updated: 2026-04-17 (Maintenance run 0007 — v3.0.0 pre-release)*

packages/codev/package.json

@@ -55,6 +55,7 @@
     "@cluesmith/codev-dashboard": "workspace:*",
     "@cluesmith/codev-types": "workspace:*",
     "@playwright/test": "^1.58.0",
+    "@xterm/xterm": "^5.5.0",
     "@types/better-sqlite3": "^7.6.13",
     "@types/js-yaml": "^4.0.9",
     "@types/node": "^22.10.1",