Add test-shrink skill and orchestration agents - #691
Conversation
Adds an on-demand Claude Code project skill (.claude/skills/test-shrink) that runs an iterative measure -> research -> shrink -> review -> verify loop to reduce unit-test size and generated debug data while keeping every Rust test and every TestPrograms/ program passing. Adds three reusable agent definitions the loop orchestrates: - research-agent: read-only scout that measures and ranks candidates - code-monkey: implements exactly one scoped change with targeted tests - code-reviewer: KEEP/REVISE/REJECT gatekeeper for coverage preservation Guardrails follow the binding Logbie Testing Policy: no weakened assertions, no deleted coverage, no manufactured green; maintainer decisions (release profile, CI config) are reported, never applied. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HXS6JTMGYMqTEm1xiXFtWk
Adds .claude/skills/test-shrink/references/rust-test-optimization.md, a WFL-specific distillation of compiler-testing research (single-binary integration consolidation, check-driver dedup, [profile.test] tuning, snapshot testing with its gotchas, rustc suite hygiene rules), with an incremental migration plan for the ~146-binary tests/ layout and a priority order for shrink passes. SKILL.md now directs Phase 2 research agents to the playbook, ranks binary proliferation as the top hunting ground, adds a test-binary disk-footprint metric, and permits [profile.test] tuning as an ordinary candidate while keeping the release profile, new dev-dependencies, and CI/linker changes as maintainer proposals. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HXS6JTMGYMqTEm1xiXFtWk
IDEA.md at the repository root is the WFL language definition consumed by Hermes and predates the hygiene policy (present since the initial commit). Maintainer-approved root allowlist entry so the repo-hygiene gate passes without relocating the file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HXS6JTMGYMqTEm1xiXFtWk
|
Warning Review limit reached
Next review available in: 115 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| 4. **Keep or revert.** Green → commit (one candidate, one commit, message | ||
| states the measured saving). Anything red or reviewer-rejected → | ||
| `git checkout -- .` / `git restore` back to the last commit. Never carry a | ||
| broken candidate forward while starting the next one. |
There was a problem hiding this comment.
🟡 Cleanup step after a rejected change leaves newly created files behind
The revert instruction only restores tracked files (git checkout -- . / git restore at .claude/skills/test-shrink/SKILL.md:149-150), so any brand-new file created by the rejected attempt stays in the repository and is carried into the next attempt.
Impact: A rejected or broken change is only partly undone, so leftover files pollute the repository and can silently affect later work.
Why tracked-only restore is insufficient in this loop
Phase 3 step 1 briefs code-monkey to implement candidates that explicitly include creating new files — e.g. the playbook's single-binary consolidation creates tests/suite/main.rs and moves files under tests/suite/ (.claude/skills/test-shrink/references/rust-test-optimization.md:23-38), and Phase 1 writes measurement scratch. git checkout -- . and git restore do not delete untracked files, so a rejected consolidation batch leaves tests/suite/* in place, which both violates hard rule 6 (nothing untracked left in the tree, SKILL.md:49-50) and means the "never carry a broken candidate forward" promise at SKILL.md:150 is not actually enforced. A git clean -fd (scoped) or git stash -u-style step is needed.
| 4. **Keep or revert.** Green → commit (one candidate, one commit, message | |
| states the measured saving). Anything red or reviewer-rejected → | |
| `git checkout -- .` / `git restore` back to the last commit. Never carry a | |
| broken candidate forward while starting the next one. | |
| 4. **Keep or revert.** Green → commit (one candidate, one commit, message | |
| states the measured saving). Anything red or reviewer-rejected → | |
| `git restore --source=HEAD --staged --worktree -- .` **and** | |
| `git clean -fd -- tests/` to drop files the attempt created. Never carry a | |
| broken candidate forward while starting the next one. |
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| | Metric | How | | ||
| |---|---| | ||
| | Test source size | `wc -l tests/**/*.rs` total; `du -sh tests/` | |
There was a problem hiding this comment.
🟡 Baseline size measurement skips the very test files being optimized
The baseline line-count command counts only files in sub-folders (wc -l tests/**/*.rs at .claude/skills/test-shrink/SKILL.md:76), so the ~143 test files sitting directly in the test folder are omitted from the measurement.
Impact: The before/after size numbers the loop reports are wrong, and a consolidation that moves files into a sub-folder looks like a huge regression instead of a saving.
Bash glob semantics
Without shopt -s globstar, tests/**/*.rs expands identically to tests/*/*.rs, i.e. only tests/common/*.rs and other subdirectories — excluding all 143 tests/*.rs files that Phase 2's hunting grounds target (.claude/skills/test-shrink/SKILL.md:99-102). Worse, after §1 consolidation moves files under tests/suite/, they suddenly start being counted, so the "after" number can exceed the "before" number for a change that removed lines. Use find tests -name '*.rs' | xargs wc -l (or enable globstar).
| | Test source size | `wc -l tests/**/*.rs` total; `du -sh tests/` | | |
| | Test source size | `find tests -name '*.rs' -print0 \| xargs -0 wc -l \| tail -1`; `du -sh tests/` | |
Was this helpful? React with 👍 or 👎 to provide feedback.
| [profile.test] | ||
| opt-level = 1 # cheap runtime win for interpreter-heavy tests | ||
| debug = 1 # line-tables-only: keeps usable panics/backtraces | ||
| strip = "debuginfo" # shrink test binaries on disk | ||
|
|
||
| # Optimize heavy dependencies harder while keeping WFL code fast to compile: | ||
| [profile.test.package."*"] # or name specific heavy deps (tokio, sqlx, …) | ||
| opt-level = 2 |
There was a problem hiding this comment.
🟡 Recommended test build settings cancel each other out and would break readable crash output
The suggested test-build settings keep line tables and then immediately throw them away (debug = 1 plus strip = "debuginfo" at .claude/skills/test-shrink/references/rust-test-optimization.md:64-66), so a failing test's crash report loses the file and line information the very next paragraph requires the loop to check.
Impact: Anyone following this guidance ends up with unreadable failure output, which is exactly the outcome the accompanying check is meant to prevent.
Details and a second incorrect claim in the same block
debug = 1 emits line-tables-only debug info specifically so panic backtraces stay attributable, but strip = "debuginfo" strips that same information from the binary, so the "panic output in a deliberately-failed test is still readable" requirement at references/rust-test-optimization.md:75-77 can never be satisfied with both settings applied. Either drop strip or drop debug.
Additionally, the [profile.test.package."*"] opt-level = 2 suggestion (references/rust-test-optimization.md:68-70) is presented as a way to "optimize heavy dependencies harder": per Cargo's profile rules the test profile applies to test targets of the workspace members, while dependencies are built with the dev profile, so this stanza would not have the described effect on tokio/sqlx builds.
| [profile.test] | |
| opt-level = 1 # cheap runtime win for interpreter-heavy tests | |
| debug = 1 # line-tables-only: keeps usable panics/backtraces | |
| strip = "debuginfo" # shrink test binaries on disk | |
| # Optimize heavy dependencies harder while keeping WFL code fast to compile: | |
| [profile.test.package."*"] # or name specific heavy deps (tokio, sqlx, …) | |
| opt-level = 2 | |
| [profile.test] | |
| opt-level = 1 # cheap runtime win for interpreter-heavy tests | |
| debug = 1 # line-tables-only: keeps usable panics/backtraces | |
| # NOTE: do NOT add `strip = "debuginfo"` alongside `debug = 1` — stripping | |
| # removes the line tables that keep panics readable. Pick one. | |
| # Dependencies are built with the `dev` profile, so tune them there: | |
| [profile.dev.package."*"] # or name specific heavy deps (tokio, sqlx, …) | |
| opt-level = 2 |
Was this helpful? React with 👍 or 👎 to provide feedback.
| "Cargo.lock", | ||
| "Cargo.toml", | ||
| "GOVERNANCE.md", | ||
| "IDEA.md", # WFL language definition consumed by Hermes — maintainer-approved (Brad, 2026-08-14) |
There was a problem hiding this comment.
🟡 Repository hygiene allowlist is widened instead of the misplaced file being relocated
A root-level document is added to the approved-root-files list (IDEA.md entry at .repo-hygiene.toml:27) rather than being moved to its canonical home, which the project's agent policy explicitly forbids.
Impact: The hygiene gate stops flagging a file that policy says does not belong at the repository root, weakening the layout rule for everyone afterwards.
Rule text and inconsistencies
CLAUDE.md and AGENTS.md both state: "The repo-hygiene CI job blocks violations — fix placement, don't widen allowlists." IDEA.md is a language-definition document, whose canonical home per REPOSITORY_HYGIENE.md:37-56 is Docs/ (maintained docs) or Engineering/designs/ (design record), not the root. The change is also unrelated to this PR's stated scope (test-shrink skill + agents), the comment asserts a maintainer approval and date not recorded anywhere else in the governance suite (CLAUDE.md: "Do not invent maintainer identity or process"), and the accompanying Dev Diary entry (History/dev-diary/2026/2026-08-14-test-shrink-skill.md) does not mention this allowlist change at all.
Prompt for agents
The PR widens the root allowlist in .repo-hygiene.toml to admit IDEA.md, but both CLAUDE.md and AGENTS.md require fixing placement rather than widening allowlists, and REPOSITORY_HYGIENE.md's placement table gives no root home for a language-definition document. Either relocate IDEA.md to its canonical home (Docs/ for maintained docs, or Engineering/designs/ for a design record) and drop the allowlist entry, or — if the maintainer genuinely approved a root exception — record that exception in the authoritative prose policy (REPOSITORY_HYGIENE.md) as well, and split it out of this test-shrink PR with a Dev Diary note, since the current diary entry does not mention it.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Keeping the allowlist entry as-is. Context the review couldn't see: the maintainer (Brad, the PR author) explicitly directed this addition in the session that produced this PR — IDEA.md is consumed in place by Hermes, so relocating it would break that consumer. On the policy mechanics: REPOSITORY_HYGIENE.md §9 states "Every exception is Maintainer-approved and recorded in .repo-hygiene.toml" — the .toml entry with its approval comment is the authoritative record the policy calls for, so no parallel prose entry is required. The "fix placement, don't widen allowlists" rule targets agents self-serving around violations, not maintainer-approved exceptions, which are exactly what this file exists to hold. The Dev Diary entry now records this change and its rationale as of 1039830, addressing the traceability gap this comment fairly pointed out.
Generated by Claude Code
| - **Binary proliferation (playbook §1):** every `.rs` file directly under | ||
| `tests/` becomes its own statically-linked test binary. Consolidating into a | ||
| single suite binary in incremental batches is usually the biggest disk and | ||
| link-time win available. |
There was a problem hiding this comment.
🔍 Consolidation candidate conflicts with documented per-target test commands
The playbook notes that cargo test --test <old_name> granularity is lost after consolidation and says to update testing.md, CI, and the skill (.claude/skills/test-shrink/references/rust-test-optimization.md:41-44). But SKILL.md hard rule 4 restricts the loop to tests/ and explicitly excludes CI workflow files from being touched (rule 5, .claude/skills/test-shrink/SKILL.md:37-48). If any CI job or script names an individual test target, the highest-priority candidate (§1 consolidation) cannot be completed within the loop's own scope rules — worth resolving in the skill text before the first run.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Pull request overview
Adds a new on-demand Claude Code skill (test-shrink) plus reusable orchestration subagents to support iterative size/output reductions in the Rust test suite, along with supporting documentation and repo-hygiene updates.
Changes:
- Added
.claude/skills/test-shrinkskill orchestration plus a Rust test optimization playbook reference. - Added three reusable subagent definitions (
research-agent,code-monkey,code-reviewer) for parallel scouting + scoped implementation + gating review. - Updated repo hygiene allowlist to include
IDEA.mdand recorded the work in a Dev Diary entry.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
History/dev-diary/2026/2026-08-14-test-shrink-skill.md |
Dev Diary entry documenting the new skill, playbook, and agents. |
.repo-hygiene.toml |
Allows IDEA.md at repo root per maintainer approval note. |
.claude/skills/test-shrink/SKILL.md |
Main orchestration skill defining phases, guardrails, and commands for the shrink loop. |
.claude/skills/test-shrink/references/rust-test-optimization.md |
Research playbook for Rust test suite size/build optimization approaches. |
.claude/agents/research-agent.md |
Read-only ranked-candidates scout agent definition. |
.claude/agents/code-reviewer.md |
KEEP/REVISE/REJECT gatekeeper agent definition focused on coverage preservation. |
.claude/agents/code-monkey.md |
Single-change implementer agent definition with scope/verification constraints. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| 2. **Green baseline.** Run the full gate once: | ||
| `cargo test --all`, then `cargo build --release` and | ||
| `./scripts/run_integration_tests.sh`. If anything is red **stop** — report | ||
| the failure instead of optimizing on a broken base. Shrinking on red makes |
| `cargo test` builds in the dev profile: `opt-level = 0`, full debug info. | ||
| Full debug symbols across ~146 (or even 1) test binaries are most of the disk | ||
| cost; `opt-level = 0` makes interpreter-heavy tests slow at runtime. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d416fad97
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 2. **Review — spawn `code-reviewer`** on the diff (`git diff`). Its one | ||
| non-negotiable question: *does the suite still verify everything it | ||
| verified before this diff?* It answers KEEP / REVISE / REJECT with | ||
| evidence. REVISE goes back to a code-monkey once; REJECT means revert now. |
There was a problem hiding this comment.
When a candidate creates a shared helper as an untracked file or uses git mv during test consolidation, plain git diff omits the new file and any index-only changes; Git documents index changes through the separate --cached form. The reviewer can therefore return KEEP without seeing the implementation that determines whether coverage was preserved, and the later plain git restore/git checkout -- . rollback also does not reliably reset that complete state. Inspect the status and review a diff containing staged, unstaged, and new files before accepting a candidate.
AGENTS.md reference: AGENTS.md:L142-L144
Useful? React with 👍 / 👎.
|
|
||
| | Metric | How | | ||
| |---|---| | ||
| | Test source size | `wc -l tests/**/*.rs` total; `du -sh tests/` | |
There was a problem hiding this comment.
In the non-interactive Bash environment used by the skill, globstar is off by default, so tests/**/*.rs matches files exactly one directory below tests/ rather than recursively or at the root. In this repository it currently counts only 1 of 144 Rust test files, omitting all 143 top-level integration tests, which makes the primary source-size baseline and reported savings invalid. Enable globstar and include root files, or use find/rg --files for this metric.
Useful? React with 👍 / 👎.
| | Wall time | time of `cargo test --all` and of the integration script | | ||
| | Test binary footprint | `du -sh target/debug/deps/` and count of test binaries — with ~146 separate files under `tests/`, each is its own binary statically linking the whole compiler with debug info; this is usually where most of the "debug data" lives | |
There was a problem hiding this comment.
When the loop performs its highest-priority consolidation and removes standalone test targets, Cargo does not delete those targets' old executables from target/debug/deps; it adds the new suite binary alongside them. Consequently, comparing du -sh target/debug/deps/ before and after reports no reduction or even growth despite a real clean-build saving, causing the acceptance rule to reject valid candidates or publish misleading metrics. Measure equivalent clean target directories or explicitly inventory artifacts belonging to the current target graph.
Useful? React with 👍 / 👎.
- Phase 3 revert now restores tracked files AND git-cleans untracked files scoped to the candidate's paths, so rejected attempts leave nothing behind (Devin #1) - Test-source-size metric uses find|xargs wc -l instead of a tests/**/*.rs glob that skips top-level files without globstar and would misreport consolidation as a regression (Devin #2) - Playbook profile guidance corrected: strip=debuginfo removed (it cancels debug=1's line tables), dependency opt-levels moved to [profile.dev.package] since deps build with the dev profile, and the test-vs-dev profile split stated accurately (Devin #3, Copilot) - Rule 5 carve-out: mechanical updates to renamed test-target references in testing.md/scripts/CI are part of the consolidation candidate, resolving the scope conflict (Devin #5) - Preflight notes the .ps1 integration-script variant for Windows (Copilot) - Dev Diary records the IDEA.md allowlist rationale and these fixes Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HXS6JTMGYMqTEm1xiXFtWk
- Phase 3 review step now stages everything (git add -A) and reviews git diff --cached HEAD plus git status, so newly created files and index-only changes are visible to the code-reviewer before a KEEP - Test-binary-footprint metric warns that Cargo leaves removed targets' stale binaries in target/debug/deps; compare equivalent clean states or inventory the current target graph's artifacts instead Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HXS6JTMGYMqTEm1xiXFtWk
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
.claude/skills/test-shrink/SKILL.md:66
- Phase 0 says “Run the full gate once” but only runs
cargo test+ release build + integration tests, which contradicts the hard-rule gate definition above that includescargo fmt --all -- --checkandcargo clippy ... -D warnings. This could cause shrink passes to start from a baseline that would fail the documented gates.
2. **Green baseline.** Run the full gate once:
`cargo test --all`, then `cargo build --release` and
`./scripts/run_integration_tests.sh` (`.ps1` on Windows — wherever this
skill says `.sh`, use the platform's variant). If anything is red **stop** — report
.claude/skills/test-shrink/SKILL.md:155
- Phase 3 “Verify” uses
cargo fmt --all, but the hard-rule gate definition requirescargo fmt --all -- --check. Using--checkhere keeps the loop consistent with the documented gate and avoids accidentally introducing formatting-only diffs during the verification step.
3. **Verify.** Targeted tests first (fast feedback), then before committing:
`cargo fmt --all`, `cargo clippy --all-targets --all-features -- -D warnings`,
`cargo test --all`. If the change touched anything the end-to-end programs
Summary
Added an on-demand Claude Code project skill (
test-shrink) and three reusable subagent definitions to support iterative optimization of the WFL Rust test suite. The skill runs a structured loop to reduce test size, debug data, and duplicated harness code while preserving every assertion and keeping all tests passing.Key Changes
.claude/skills/test-shrink/SKILL.md— Main orchestration skill defining a 4-phase loop:.claude/skills/test-shrink/references/rust-test-optimization.md— Research playbook distilled from compiler/interpreter test architecture best practices (matklad, rustc dev-guide, rust-analyzer, pydantic-monty):[profile.test]candidates with readable-panic check)check-style drivers to deduplicate harness code.claude/agents/research-agent.md— Read-only scout that measures and ranks shrink candidates; may web-search current Rust practices; returns ranked list with estimated saving, risk, and suggested approach.claude/agents/code-monkey.md— Implements exactly one scoped change and verifies with targeted tests; enforces scope discipline and guardrail fidelity; reports tightly without making judgment calls.claude/agents/code-reviewer.md— Gatekeeper returning KEEP / REVISE / REJECT with evidence; non-negotiable question is coverage preservation; skeptical by default.repo-hygiene.toml— AddedIDEA.mdto allowed-files list (WFL language definition consumed by Hermes, maintainer-approved)History/dev-diary/2026/2026-08-14-test-shrink-skill.md— Dev Diary entry documenting the skill, playbook, and subagent definitionsNotable Implementation Details
target/reports/test-shrink/or temp dirs (hygiene enforced)src/) is changed; this is tooling and process onlyhttps://claude.ai/code/session_01HXS6JTMGYMqTEm1xiXFtWk