Code Review: PR #595

Commit: 5344160944e8dc695fe24f759684c06c37c4ced6

Summary

Docs/rules-only PR adopting governed looping as a first-class apexyard pattern: a new trigger-heuristic rule (.claude/rules/loop-mode.md, sibling of parallel-work / plan-mode), its decision record (AgDR-0068), and the rule-count/list bumps in CLAUDE.md + README.md. No code, no tests in scope.

Checklist Results

• ✅ Architecture & Design: N/A (docs)
• ✅ Code Quality: Pass
• ✅ Testing: N/A (docs, no coverage gate)
• ✅ Security: Pass
• ✅ Performance: N/A
• ✅ PR Description & Glossary: Pass (4-term glossary, Closes #594)
• ✅ Summary Bullet Narrative: Pass (what + why, well above the label-only floor)
• ✅ Technical Decisions (AgDR):Pass — AgDR-0068 linked from CLAUDE.md bullet + PR body; covers the decision implemented
• ✅ Adopter Handbooks: N/A (no handbooks loaded — diff is framework rules/docs only)

Verification performed

• Rule count 11→12 correct in both canonical surfaces: CLAUDE.md layer table and README directory tree. ls .claude/rules/*.md = 12. New rule inserted alphabetically (leak-protection · loop-mode · parallel-work) in both lists.
• Wiring: auto-loads via the existing @.claude/rules/*.md wildcard — no per-file import needed, none missing.
• AgDR numbering: prior max was 0067; 0068 is the next free number. Correct.
• Internal links all resolve: parallel-work.md, plan-mode.md, pr-workflow.md, ../skills/fan-out/SKILL.md. The cited heading ### Plan-level "go" is NOT merge approval exists verbatim in pr-workflow.md. /loop and /schedule correctly have no skill dir — the rule labels them harness-owned, which is accurate.
• Structural parity: loop-mode.md mirrors the precedent spine (When to OFFER / When NOT / Self-check / Backstop / MIT footer); the two extra sections (Which primitive, Guardrails) are warranted by the loop's added surface and don't break the family resemblance.
• Guardrail soundness: "halt at the per-PR CEO merge gate, never self-approve" is consistent with pr-workflow.md and is already enforced in practice by block-unreviewed-merge.sh + /approve-merge (correctly described as the mechanical backstop to an advisory rule). The "verify = build + tests + Rex, not just build" guardrail is well-grounded in the cited v3 view-redesign in-session lesson.
• AgDR-0068 follows the template (context / options / decision / consequences / artifacts), references #594 and the precedent rules, and the decision (Option 1: trigger-heuristic rule, no new engine) matches what was implemented.

Issues Found

None blocking.

Suggestions

• nit (non-blocking, out of scope): AGENTS.md:13 still reads "11 modular rule files" and was not touched by this PR. It's not a regression — AGENTS.md is a systematically-stale snapshot (it also lists 31 hooks / 53 skills / 23 agents / 19 roles against CLAUDE.md's 40 / 59 / 24 / 20), and it states only the count, not the rule names, so there's no list to desync. Worth a separate housekeeping pass to resync AGENTS.md to the canonical counts, but it should not block this PR.

Verdict

APPROVED

The PR is internally consistent across its declared scope (CLAUDE.md + README), the new rule is well-crafted and faithful to its two precedents, the AgDR is complete and matches the implementation, and all links resolve. The only finding is pre-existing drift in an unrelated file.

🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: 5344160944e8dc695fe24f759684c06c37c4ced6

Design Review: PR #595

Commit: 5344160944e8dc695fe24f759684c06c37c4ced6

Summary

Adopts "governed looping" as a first-class apexyard pattern: a trigger-heuristic rule (.claude/rules/loop-mode.md) that proactively recommends a closed loop for repetitive, machine-verifiable, bounded work, binding every loop to apexyard's existing gates as its eval — rather than building a new /loop engine or leaving looping undocumented. Decision is recorded in AgDR-0068.

Review Lens Results

• ✅ Quality attributes / NFRs: Pass — the relevant NFRs here are cost-boundedness and output-correctness; both are addressed by explicit guardrails (budget/iteration ceiling, no-progress detection, verify = build+tests+Rex).
• ✅ Design patterns & structure: Pass — the rule is a faithful structural sibling of parallel-work.md and plan-mode.md (When-to-OFFER / When-NOT / Self-check / Backstop). Reuses the eval layer instead of duplicating it. Correct dependency direction: the new advisory rule leans on the existing mechanical gates, not vice-versa.
• ✅ Technical debt: Pass — no silent debt. Deferred items (mechanical budget enforcement, cron scheduling, a /loop-wrapper skill) are named explicitly in AgDR Consequences as a candidate follow-up spike.
• ✅ Decisions (AgDR linkage): Pass — AgDR-0068 captures the decision with a 3-option table, honest cons on the chosen option ("not mechanically enforced"), and consequences. BLOCKING check satisfied.
• ✅ Risk: Pass — the three real failure modes (runaway cost, unverified output, self-approving merge) are each named and matched to a guardrail. See finding A on the strength of each.
• ✅ Trade-off analysis: Pass — alternatives (new engine / undocumented) are genuinely weighed, not strawmanned. "Re-implements a harness capability + duplicates the gates" is a substantive, correct con against the engine option.
• ✅ Requirements traceability: Pass — AgDR ↔ rule ↔ #594 all cross-link. CLAUDE.md/README rule counts updated 11→12 consistently; Closes #594 present.
• ✅ Migration safety: N/A — no migration artifact.
• ✅ Adopter Handbooks: N/A — framework-internal rule + AgDR; no adopter handbook applies to a .claude/rules/ doc change.

Architecture Assessment

Soundness of the decision (Pass). "Trigger-heuristic rule + reuse existing gates" is the correct architectural choice. The decisive argument — already made in the AgDR and confirmed against the codebase — is that apexyard already ships both halves a loop needs: a skill library (the 59 slash commands) and a real eval layer (Rex, the design/architecture reviews, CI, the merge gates, QA). Building a /loop engine would re-implement harness capabilities (/loop, /schedule, Workflow) and, worse, duplicate the gate logic — creating a second enforcement path that could drift from block-unreviewed-merge.sh. The rule-not-engine call keeps a single source of truth for the gates. The options/consequences analysis is honest: it states the chosen option's central weakness ("not mechanically enforced; relies on the agent following the rule") rather than hiding it.

Consistency / fit (Pass). The rule mirrors the two precedent self-discipline rules section-for-section and slots into the same @.claude/rules/*.md wildcard import with no per-file wiring and no new hook — exactly the established extension pattern for advisory rules. No architectural tension introduced.

Guardrail adequacy — the load-bearing question (Pass, with one finding). I verified the central claim against the actual mechanism rather than taking the prose at face value:

• "Halt at the per-PR CEO merge gate (never self-approve)" is mechanically real, not just prose. block-unreviewed-merge.sh requires a structured <pr>-ceo.approved marker with approved_by=user and skill_version>=2, and pr-workflow.md § "Build agents cannot self-review" documents that a build-class sub-agent cannot nest the Agent tool and is forbidden from writing markers under .claude/session/reviews/. So a loop that tries to self-approve is blocked at the shell level. The rule correctly characterises this as a backstop and the rule itself as advisory — that framing is accurate.
• "Verify = build + tests + Rex (not just build)" is advisory, not mechanically enforced — which the rule is honest about (it lives under self-discipline + Backstop). This is the one gap worth naming: there is no hook that asserts the loop's internal verify stage ran the test suite before the loop trusts an iteration. The merge gate catches it only at the boundary (red CI blocks merge via block-merge-on-red-ci.sh), not mid-loop. The AgDR grounds the rule in the real v3-redesign incident where exactly this gap shipped broken tests, so the risk is correctly identified — but operators should read "verify means tests" as a discipline the rule recommends, not a thing the framework prevents you from skipping mid-loop. This is acceptable for an advisory rule and consistent with how parallel-work/plan-mode are framed; no change required. (See finding A.)

Traceability (Pass). AgDR ↔ rule ↔ #594 linkage is complete and the deferred items are scoped out appropriately.

Blocking Findings

None.

Suggestions

• A (non-blocking, advisory-framing clarity): The rule states two guardrails with identical "MUST" force, but they have different enforcement strength — "halt at CEO gate" is mechanically backed (block-unreviewed-merge.sh), while "verify = build+tests+Rex" is purely self-discipline mid-loop (only enforced at merge boundary by red-CI block). The Backstop section already says "the merge-gate hooks already stop a loop that tries to self-approve," which implicitly draws this line. Optional: a half-sentence in the Guardrails section making explicit that the verify-stage clause is recommended discipline (caught only at the merge boundary, not mid-iteration) would prevent a reader from assuming the framework blocks an under-verified iteration before it happens. Not required — the rule's overall advisory framing is correct and the AgDR is honest about it.

Verdict

APPROVED

The decision is architecturally sound, fits apexyard's established advisory-rule pattern, and — critically — its load-bearing safety claim ("never self-approve") is backed by a real mechanism, not just prose. The one advisory-vs-mechanical distinction in the guardrails is correctly framed by the rule and honestly recorded in the AgDR.

🏛️ Reviewed by Tariq (Solution Architect)
📌 Reviewed commit: 5344160944e8dc695fe24f759684c06c37c4ced6