Fix model-routing recovery edge cases

[justin808]

Follow-up to #106 after its review/merge closeout exposed additional recovery and installer edge cases.

Summary

• preserve complete per-lane initial/escalation routes during recovery and route worker-only restarts correctly
• namespace the installed model-routing guide and migrate only provably managed legacy copies/symlinks, including moved sources and linked worktrees
• enforce host-aware prompt budgets, route-group splitting, configured review gates, and bounded resume instructions

Validation

• bin/validate
• bash bin/install-agent-workflows-test.bash
• AGENT_WORKFLOWS_SOURCE_CHECKOUT=1 ruby skills/plan-pr-batch/scripts/check_goal_prompt_size.rb
• AGENT_WORKFLOWS_SOURCE_CHECKOUT=1 ruby skills/pr-batch/bin/model-routing-contract-test.rb
• independent GPT-5.4/xhigh review clean; hosted Claude wording finding fixed and revalidated

QA Evidence

• QA lane: coordinator, jg-codex/pr106-review-followups, no private coordination backend required
• Scope checked: installer migration, saved-handoff recovery, route preservation, prompt budgets, completion/review gates
• Tested at: bd96550
• Automated checks: bin/validate plus focused installer and model-routing contract suites
• Manual checks: not applicable; CLI behavior is covered by copy, symlink, moved-source, linked-worktree, unpacked-source, and collision fixtures
• Findings: all accepted review findings fixed; focused regression and full validation green on exact head
• QA required: yes
• QA required rationale: installer migration and workflow recovery behavior are compatibility-sensitive
• QA lane status: satisfied
• Release-blocking status: clear
• Process-gap disposition: script

Summary by CodeRabbit

• New Features

  • Added a cost-aware model routing guide with route classes, decision criteria, operating modes, safeguards, and verification guidance.
  • Model routing documentation is now installed under a namespaced filename.

• Bug Fixes

  • Installations now remove legacy managed model-routing files while preserving unrelated existing files and symlinks.

• Documentation

  • Updated installation, workflow, and navigation references for the renamed routing guide.
  • Clarified worker handoffs, completion requirements, live-state verification, coordination, and route recovery.

• Tests

  • Expanded coverage for namespaced installation, legacy cleanup, collisions, symlinks, routing recovery, and prompt-size limits.

Codex Decision Log

• Non-blocking: CodeRabbit could not run its incremental review of the final two-file wording/test commit because its review quota was reached.
  • Decision: Proceed using the current-head Claude review and required hosted validation.
  • Why: The repo requires independent review, not a specific bot; Claude reviewed bd96550, found no correctness bugs, all review threads are resolved, and CodeRabbit had approved the prior head.
  • Review later: None.

[justin808]

Review finding dispositions

All verified actionable findings from the PR #106 closeout and follow-up independent review were accepted and fixed on f4eadb64bded33f0720ace8cdd081afe46e24532:

• P1: recovery collapsed mixed lane routes — fixed with complete per-lane route policies and contract coverage.
• P2: generic installed guide could overwrite a consumer-owned collision — fixed by namespacing the guide.
• P2: continuation routing could skip model-route recovery or misclassify a worker-only restart — fixed with explicit recovery/status classification.
• P1: compact Codex prompts lacked safe mixed-route headroom — fixed with measured host-aware budgets and route-group splitting.
• P1/P2: legacy guide migration missed copied, moved, unpacked, linked-worktree, and non-Git source cases — fixed with recorded-source evidence and dedicated fixtures.
• P2: migration could delete an exact-content generic collision without ownership evidence — fixed by removing the content-only fallback and pinning preservation in both install modes.
• P3: migration tests depended on reachable repository history — fixed with self-contained throwaway Git histories.
• P2: compact completion text dropped configured review agents, bounded watch, or resume instructions — fixed and byte-aligned across canonical prompts.
• P3: Claude/generic filled route prompts were not checked against their host limit — fixed in the prompt-size guard.

Evidence: bin/validate passed on the exact head; the final independent GPT-5.4/xhigh review returned “No actionable findings remain.” The original Fable review on #106 had no unresolved inline threads.

[coderabbitai]

Walkthrough

The installer now uses a namespaced model-routing document and removes identifiable legacy artifacts. PR-batch routing, completion contracts, execution rules, and prompt-size validation are updated to preserve lane mappings and enforce revised recovery and verification semantics.

Changes

Installer documentation migration

Layer / File(s)	Summary
Namespaced routing documentation docs/agent-workflows-model-routing.md, docs/README.md, docs/installation-and-upgrades.md	Adds the cost-aware model-routing guide and updates documentation and installation references to its namespaced filename.
Legacy installer migration bin/install-agent-workflows, bin/install-agent-workflows-test.bash	Installs the namespaced document, removes matching managed legacy paths using metadata or source evidence, and tests copy, symlink, collision, worktree, and non-Git scenarios.

PR-batch routing controls

Layer / File(s)	Summary
Model-routing recovery flow skills/plan-pr-batch/SKILL.md, skills/pr-batch/SKILL.md, workflows/pr-processing.md, skills/pr-batch/bin/model-routing-contract-test.rb	Distinguishes batch route recovery from same-lane resumption and requires complete per-lane route mappings.
Completion and execution contracts skills/plan-pr-batch/SKILL.md, skills/pr-batch/SKILL.md, workflows/pr-processing.md, skills/pr-batch/bin/goal-completion-contract-test.rb	Updates preflight, live-state verification, coordination, QA evidence, and watch-window completion semantics.
Route-aware prompt sizing skills/plan-pr-batch/scripts/check_goal_prompt_size.rb, skills/plan-pr-batch/SKILL.md	Adds mixed-route and oversized-route budget checks, route-group splitting rules, and revised required phrase validation.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Handoff
  participant Continuation
  participant Recovery
  participant Worker
  Handoff->>Continuation: classify saved route handoff
  Continuation->>Recovery: select batch recovery or bounded lane recovery
  Recovery->>Worker: preserve lane route and issue continuation instructions

Loading

Possibly related PRs

• shakacode/agent-workflows#50 — Overlaps the prompt-generation and sizing changes in plan-pr-batch.
• shakacode/agent-workflows#52 — Shares the Goal Mode Completion Contract semantics and validation updates.
• shakacode/agent-workflows#87 — Shares the prompt contract and required-phrase validation surfaces.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and broadly matches the PR’s main focus on model-routing recovery fixes, though it omits the installer and prompt-related scope.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch jg-codex/pr106-review-followups

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

Review Summary

Overview: Follow-up to #106 fixing three edge cases: (1) the installer now migrates docs/model-routing.md → docs/agent-workflows-model-routing.md, deleting only provably managed legacy copies/symlinks (via recorded install metadata + git history), while preserving unrelated user files that happen to share the name; (2) prompt/workflow text now preserves per-lane initial/escalation routes during model-routing recovery instead of collapsing them into one batch-wide pair, and distinguishes single-worker restarts (Bounded Status Recovery) from whole-batch route recovery; (3) check_goal_prompt_size.rb gained fixtures for mixed-route and 4-route-group prompts to enforce host-aware budget/headroom and to prove the new "split along route groups" rule is actually load-bearing.

Code quality

• bin/install-agent-workflows's new remove_legacy_managed_model_routing_path is careful and well thought out: it distinguishes symlink vs. regular-file legacy paths, verifies provenance via recorded source/source_revision metadata and git history (including linked worktrees), and only falls back to raw content comparison when there's no git evidence. This is backed by solid test coverage (copy/symlink modes, generic collisions with and without provenance, git worktree source, non-git/unpacked source).
• The installed_metadata_value ruby one-liner reuses the existing JSON-based metadata pattern already used by write_metadata; consistent with existing conventions.
• Prompt-text edits are intentionally mirrored across workflows/pr-processing.md, skills/pr-batch/SKILL.md, and skills/plan-pr-batch/SKILL.md, and there's an existing test (test_canonical_and_dispatch_prompt_contracts_stay_byte_for_byte_aligned) enforcing byte-for-byte alignment of the shared contract line — good, this catches drift.

Issue found (posted inline)

• skills/plan-pr-batch/SKILL.md's "Continuing From Saved Handoffs" text drops the word "or" that's present in the equivalent skills/pr-batch/SKILL.md text: "explicitly requests model-route replacement, identifies workers on a wrong or too-expensive route" reads as a comma splice rather than two clear alternative trigger conditions. Since these two files are meant to encode the same routing-recovery policy, this should match the pr-batch/SKILL.md wording ("...replacement or identifies workers...").

Other observations (non-blocking)

• The Goal Mode Completion Contract text also drops the qualifier "fixable" from "fixable failures" -> "failures", and drops the "or real external blocker" exception clause, tightening the completion bar (any failure now blocks completion, and there's no longer an explicit "real external blocker" escape hatch — only the more general "watch window" + "resume instructions" language). This looks intentional (tests were updated to match), but worth a sanity check that dropping the "real external blocker" escape hatch doesn't regress a legitimate case that motivated its original inclusion.
• remove_legacy_managed_model_routing_path's for source_root in "$repo_root" "$previous_source" loop checks the same path twice when previous_source == repo_root (the common case) — harmless (git calls are cheap and idempotent) but could skip the duplicate to trim one redundant git invocation per install. Not worth blocking on.

Security / performance

• No injection risk: previous_revision/previous_source values are read from the tool's own previously-written JSON metadata and passed as quoted arguments to git/cmp, never shell-evaluated.
• Added ruby/git subprocess calls in the installer are on the CLI install path (not a hot loop), so the added overhead is negligible.

Test coverage

Strong — new installer tests cover copy/symlink modes, generic-name collisions with and without provenance evidence, legacy symlink/file removal, git worktree sources, and non-git previous sources. Prompt-budget script fixtures cover mixed-route and oversized 4-route scenarios for the codex/claude/generic targets.

[claude]

Review Summary

This is a well-scoped follow-up to #106 that hardens the installer's legacy-file migration and tightens several prompt-engineering contracts around model-route recovery and prompt budgeting. Overall the change is careful and unusually thoroughly tested for a shell script; I did not find correctness bugs.

bin/install-agent-workflows — legacy docs/model-routing.md migration

• The new remove_legacy_managed_model_routing_path function only deletes the legacy path when it can prove the file/symlink was installed by this tool:
  • Symlinks are removed only if their target literally equals $repo_root/docs/model-routing.md or the recorded previous_source/docs/model-routing.md (a path-string comparison, so it still works after the file was renamed away in the current checkout).
  • Regular files are removed only after an exact byte comparison (cmp -s) against the git-historical blob at the recorded source_revision, checked against both the current repo_root and the previously recorded source (covers moved sources and linked worktrees, since git -C works fine against a worktree's .git file).
  • A last-resort fallback compares against the previous source's current on-disk copy for non-git installs, but only when metadata actually recorded a source — so a first-ever install or missing/garbled metadata never touches a pre-existing user file (verified this by hand-tracing test_install_namespaces_model_routing_doc_and_preserves_generic_collision and test_install_preserves_exact_content_generic_collision_without_source_evidence).
  • This is a good design: it fails safe (leaves the file alone) whenever provenance can't be established, rather than risking deletion of a user's unrelated docs/model-routing.md.
• Ordering is correct: the legacy-removal pass reads .agent-workflows-install.json before write_metadata overwrites it at the end of the run, so previous_source/previous_revision always reflect the prior install, not the one currently being performed.
• Minor/non-blocking: when a legacy symlink's target matches neither the current repo_root nor the recorded previous_source, it's silently left in place (no content-based fallback for symlinks). That's a reasonable conservative default, but worth knowing it can leave an orphaned symlink around indefinitely if a target has been reinstalled from several different source locations over time without metadata tracking all of them.

skills/plan-pr-batch/scripts/check_goal_prompt_size.rb

• The new mixed_route_fallback / unsplit_four_route fixtures look correctly constructed: they assert the codex four-route fixture must exceed the 4000-char budget (forcing the new route-group-splitting rule to be exercised) while claude/generic stay under their higher budget, and they assert a minimum headroom on the mixed-route codex fallback. This directly backs the new "split along route groups" guidance added to skills/plan-pr-batch/SKILL.md / skills/pr-batch/SKILL.md.

Prompt-contract text edits (workflows/pr-processing.md, both SKILL.md files)

• The compacted Goal Mode Completion Contract line drops "fixable", "triage", and "explicit"/"real external blocker" nuance to save characters, but skills/pr-batch/bin/goal-completion-contract-test.rb was updated in lockstep to assert "watch window", "resume instructions", and "configured review agents" are still present — so the trims look intentional and covered rather than accidental scope loss.
• Preflight: run pr-security-preflight before workers → Preflight: run pr-security-preflight drops the "before workers" qualifier from the compact embedded goal line, but the fuller skills/pr-batch/SKILL.md retains "Before launching high-concurrency public issue/PR work, run the resolved pr-security-preflight helper…", so the ordering constraint isn't actually lost, just no longer duplicated in the size-constrained line.
• The three copies of the goal-prompt template (workflows/pr-processing.md, skills/pr-batch/SKILL.md, skills/plan-pr-batch/SKILL.md) were kept byte-for-byte in sync, which the existing test_canonical_and_dispatch_prompt_contracts_stay_byte_for_byte_aligned test continues to enforce.

Doc rename (docs/model-routing.md → docs/agent-workflows-model-routing.md)

• Checked for stale references across *.md/*.rb — every link/mention was updated consistently; only the installer's own test fixtures still reference the old filename, which is intentional since they simulate legacy installs.

Security

• No new attack surface: the installer only ever operates on local paths already under the user's control (its own target directory and the recorded source/source_revision metadata it wrote itself), and git -C "$source_root" ... / cmp calls take the path as a plain argument rather than interpolating into a shell string, so there's no injection risk even if .agent-workflows-install.json were hand-edited.

Nice work on the fixture coverage for the installer edge cases (copy/symlink collision, non-git source, moved source, linked worktree) — that's exactly the kind of test that catches the subtle "did we just delete a user's file" class of bug before it ships.