Code Review: PR #368

Commit: 3556f43e21499356cc8a98886bba6f39a5c1ce5c

Summary

Test-only PR. Single file touched: .claude/hooks/tests/test_split_portfolio_v2_migration.sh (+109/-2). Two coordinated changes:

1. Aligns the test's run_migration() workspace-loop with the live .claude/skills/update/SKILL.md Step 8a recipe (adds the if [ "$name" = "README.md" ]; then continue; fi framework-artefact guard + the WARNING: workspace/<name> exists in BOTH locations — skipped. conflict-skip emission).
2. Adds Case 5 with 7 assertions covering multi-project loop continuation, conflict-skip semantics, and workspace/README.md framework-artefact preservation (per AgDR-0021 § G).

Verified locally — 25/25 pass at HEAD 3556f43.

Checklist Results

• ✅ Architecture & Design: Pass (test-only; mirrors live recipe)
• ✅ Code Quality: Pass
• ✅ Testing: Pass — this PR IS the testing
• ✅ Security: N/A (no security surface)
• ✅ Performance: N/A
• ✅ PR Description & Glossary: Pass — narrative bullets, populated Glossary explaining Conflict-skip path, Framework-artefact preservation, Fixture-vs-spec drift
• ✅ Summary Bullet Narrative: Pass — all three bullets answer what + why
• ✅ Technical Decisions (AgDR): N/A — no new decisions introduced; the underlying conflict-skip + README-preservation semantics are already captured in AgDR-0021 § G/H. This PR test-pins them.
• ✅ Adopter Handbooks: N/A — migration-safety.md (blocking) doesn't apply (no SQL/ORM migration diff); clean-architecture-layers.md (advisory) doesn't apply (bash test); commit-message-quality.md (advisory) check: commit message is well-formed (test(#320): prefix, body explains motivation + breaks down the change + lists assertions).

Issues Found

None.

Targeted-review walkthrough

Per the operator's 9 review focus points:

1. Fixture-vs-spec drift fix correctness. Side-by-side comparison of SKILL.md lines 538-558 vs the new test fixture lines 135-147: semantically equivalent (README skip, conflict-skip with WARNING + continue, mv otherwise). One minor divergence noted below in Suggestions.

2. "Additive only" claim for run_migration(). Verified by running the test: 25/25 pass. Cases 1-4 use build_pre_v2's workspace/demo only; loop iterates workspace/*, basename is demo (not README.md), sibling has no pre-existing demo, so neither new branch fires. Behaviourally unchanged.

3. Case 5 sandbox setup cleanliness. Setup builds on build_pre_v2's existing sandbox state. The only assumptions made about build_pre_v2: that it creates $SB/public/workspace/demo (used as the conflict-target) and that the registered project is named demo (used in Assertion 5's grep). These are stable + visible in build_pre_v2 lines 53, 61, 96. If build_pre_v2 is later refactored to rename demo, Case 5 would need a matching update — but that's a normal test-fixture coupling, not a hidden one.

4. Loop continuation check (Assertion 3). Tight. The check [ -d "$SB/private/workspace/gamma" ] && [ -f "$SB/private/workspace/gamma/README.md" ] would fail if the loop changed from continue to break/exit on conflict, because alphabetically gamma > demo in both POSIX/C and en_US.UTF-8 locales (verified by glob expansion test) — so gamma is always processed after the conflict on demo fires. Robust across locales.

5. WARNING regex tightness. Assertion 5 uses grep -qE "WARNING: workspace/demo exists in BOTH locations" — matches the SKILL.md prefix exactly, but does NOT include the — skipped. suffix. If the suffix text drifts, the assertion won't catch it. Minor — flagged in Suggestions.

6. README.md NOT-in-sibling sanity (Assertion 7). Safe. build_pre_v2 never writes a README.md into private/workspace/ (private/workspace is created only by mkdir -p "$SB/private/workspace/demo" in Case 5's setup, and only demo/README.md is written inside it). The only way Assertion 7 fails is if the migration loop incorrectly moves the public-fork workspace/README.md to the sibling — which is exactly the regression the assertion is designed to catch.

7. Subshell stderr propagation. Verified empirically: f() { (echo stderr >&2); }; f 2>"$OUT" correctly captures the inner subshell's stderr to $OUT. The (...) subshell shape in run_migration() does NOT swallow stderr from the outer 2>"$STDERR_OUT" redirect.

8. AC coverage. All 6 ACs in #320 met. 18 → 25 assertions (+7, target ≥5). Multi-project + conflict-skip + README-preservation all exercised. Existing cases all still pass.

9. Scope discipline. Single file, 1 commit, +109/-2. Zero drive-by edits. Clean.

Suggestions

These are nits — none block approval.

1. Stderr-vs-stdout discrepancy between test fixture and SKILL.md. The test fixture's WARNING emission uses >&2 (stderr); SKILL.md's recipe at line 553 uses bare echo (stdout). Both make sense in isolation — stderr is the conventional channel for operator-facing warnings — but they diverge. If you intended the test to be more strict than the spec (pin stderr as the canonical channel for operator warnings), consider a follow-up PR adding >&2 to SKILL.md line 553. If you intended to mirror SKILL.md byte-for-byte, the >&2 in the test fixture is the discrepancy. Either resolution is fine; document the intent so future readers know. The PR's commit message says "to stderr on conflict" which suggests stderr is the intended canonical channel; surfacing this as a docs-clarification follow-up rather than a blocker.

2. WARNING regex could be tighter. Assertion 5's grep -qE "WARNING: workspace/demo exists in BOTH locations" catches prefix drift but not suffix drift (the — skipped. tail). If the operator-facing intent is to pin the full message, consider extending to "WARNING: workspace/demo exists in BOTH locations — skipped\.?$" (anchored, with the em-dash and trailing period). Low-impact — the prefix is the load-bearing part.

3. Glob ordering portability (informational, no action needed). The loop's for entry in workspace/* is locale-dependent: in LC_ALL=C README.md sorts before alpha (capital R < lowercase a); in UTF-8 locales it sorts last. The test works in both because (a) the README is always skipped regardless of position, and (b) gamma > demo in both locales. Worth noting in a fixture comment if a future case introduces project names that DO depend on locale ordering, but not a problem for this PR.

Verdict

APPROVED (would have been --approve; cross-author fallback to --comment)

The PR cleanly closes Gap 4 of the 4-gap audit: the previously-untested conflict-skip and README-preservation paths are now pinned by 7 new assertions, and the prior fixture-vs-spec drift in run_migration() is closed. Test executes green at 25/25 locally on HEAD 3556f43. Scope is surgically minimal.

The two stylistic suggestions (stderr alignment between test and SKILL.md, regex suffix tightening) are nits worth a follow-up if you want to lock the test as the canonical pin — neither blocks this PR.

🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: 3556f43e21499356cc8a98886bba6f39a5c1ce5c

Re-Review: PR #368 (follow-up) — APPROVED

Commit: d12b92c1aa549d45364b81eec59532e910952edd
Prior approved commit: 3556f43e21499356cc8a98886bba6f39a5c1ce5c

Summary

Follow-up commit addresses 2 of 3 non-blocking nits from prior review. Nit 3 (glob ordering) was informational, skipped by operator choice.

Verification

Nit 1 — Stderr drift (SKILL.md:553) — ✓ Resolved

• Line 553 now reads echo "WARNING: workspace/$name exists in BOTH locations — skipped." >&2
• Spec now aligns with the test fixture's behaviour-correct choice. Warnings to stderr is the right shape — keeps stdout clean for the operator's intended flow output, makes the WARNING capturable independently for test assertions (which is exactly what Assertion 5 does).

Nit 2 — WARNING regex tightness (Assertion 5) — ✓ Resolved

• Old: WARNING: workspace/demo exists in BOTH locations (prefix-only, no anchors)
• New: ^WARNING: workspace/demo exists in BOTH locations — skipped\.$
• Both ends anchored (^...$) catches: (a) prefix-only drift, (b) tail drift on — skipped., (c) extra characters before/after
• Literal period escaped (\.) — matches the actual . character rather than the regex any-char wildcard. Subtle but correct.

Nit 3 — Glob ordering — Operator-skipped per prior review's "informational only" note. Fine.

Drive-by check — diff is exactly the 2 files described: SKILL.md +1/-1, test_split_portfolio_v2_migration.sh +110/-2 (the +110 is the unchanged Case 5 from the original PR; the regex fix is +1/-1 within that addition window).

Test execution — Ran bash .claude/hooks/tests/test_split_portfolio_v2_migration.sh locally against d12b92c: Passed: 25, Failed: 0. All 7 Case 5 assertions green including the tightened Assertion 5.

Checklist Results

• ✅ Architecture & Design: Pass
• ✅ Code Quality: Pass
• ✅ Testing: Pass (25/25)
• ✅ Security: Pass (test-only + spec alignment)
• ✅ Performance: Pass
• ✅ PR Description & Glossary: Pass
• ⚠ Summary Bullet Narrative: Pass (the commit's fix(#320): align SKILL.md WARNING to stderr + tighten test regex summary self-explains)
• ✅ Technical Decisions (AgDR):N/A (no new decisions; reaffirms existing AgDR-0021 § G/H)
• ✅ Adopter Handbooks: N/A (none triggered — diff doesn't match any handbook bucket)

Issues Found

None.

Suggestions

None — clean, surgical follow-up. The stderr alignment is independently the right call (separates output channels) and the regex tightening removes a class of silent-regression risk for future SKILL.md edits.

Verdict

APPROVED (posted as --comment because Rex cannot self-approve via GitHub UI for this fork — sandbox workaround per operator memory; the operator writes the approval marker.)

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