chore: add refactor-sequencing guardrail and review check #354

Merged

jwilger-ai-bot merged 1 commit from chore/guardrail-refactor-sequencing into main

2026-04-12 08:21:21 -07:00

jwilger-ai-bot commented

2026-04-12 08:15:24 -07:00

(Migrated from github.com)

Summary

Adds guardrails identified during review of PR #353 (issue #281), where continuous polling functionality was deleted during an API surface reduction because the replacement API path wasn't built before the old path was removed.

Changes

New rule: .claude/rules/refactor-sequencing.md — When migrating functionality between API surfaces, establish the new reachability path before removing the old one. Dead code that appears solely because the replacement path hasn't been built signals an incomplete migration, not code to delete.
REVIEW.md "What to Block" item #16 — Flag functionality removal (deleting types, methods, behaviors, or tests) during a refactor when the removal wasn't an explicit part of the task plan. Visibility reduction is not removal, but deleting code that becomes "dead" after a visibility change is.
REVIEW.md "API Design" checklist — Verify refactors that reduce API surface preserve internal functionality with new reachability paths before old paths are removed.

Root Cause Analysis

During PR #353, the agent:

Correctly reduced visibility (pub → pub(crate)) per ADR-030
Compiler correctly reported newly-dead code (PollMode, AwaitingPollSleep, etc.)
Agent correctly followed the dead-code rule and deleted it
Agent correctly followed test-boundary rules and deleted tests of now-internal APIs

Each step followed an existing rule, but the sequencing was wrong. The old reachability was removed before the new reachability existed, making working code appear dead. The new guardrail prevents this by requiring the add-before-remove sequence.

Test plan

Pre-commit hooks pass
Rule file follows existing .claude/rules/ style conventions
REVIEW.md items are actionable and specific

## Summary Adds guardrails identified during review of PR #353 (issue #281), where continuous polling functionality was deleted during an API surface reduction because the replacement API path wasn't built before the old path was removed. ### Changes - **New rule: `.claude/rules/refactor-sequencing.md`** — When migrating functionality between API surfaces, establish the new reachability path before removing the old one. Dead code that appears solely because the replacement path hasn't been built signals an incomplete migration, not code to delete. - **REVIEW.md "What to Block" item #16** — Flag functionality removal (deleting types, methods, behaviors, or tests) during a refactor when the removal wasn't an explicit part of the task plan. Visibility reduction is not removal, but deleting code that becomes "dead" after a visibility change is. - **REVIEW.md "API Design" checklist** — Verify refactors that reduce API surface preserve internal functionality with new reachability paths before old paths are removed. ### Root Cause Analysis During PR #353, the agent: 1. Correctly reduced visibility (`pub` → `pub(crate)`) per ADR-030 2. Compiler correctly reported newly-dead code (`PollMode`, `AwaitingPollSleep`, etc.) 3. Agent correctly followed the dead-code rule and deleted it 4. Agent correctly followed test-boundary rules and deleted tests of now-internal APIs Each step followed an existing rule, but the sequencing was wrong. The old reachability was removed before the new reachability existed, making working code appear dead. The new guardrail prevents this by requiring the add-before-remove sequence. ## Test plan - [x] Pre-commit hooks pass - [x] Rule file follows existing `.claude/rules/` style conventions - [x] REVIEW.md items are actionable and specific

jwilger (Migrated from github.com) approved these changes

2026-04-12 08:16:38 -07:00