--- phase: 34 reviewers: [codex] reviewed_at: 2026-05-17 plans_reviewed: [34-01-PLAN.md, 34-02-PLAN.md, 34-03-PLAN.md] --- # Cross-AI Plan Review — Phase 34 ## Codex Review ## Summary The plan set is strong overall: it captures the SessionStart-to-Stop pivot, separates doc/script/Rust/UX wording work cleanly, and has unusually good test intent. I would approve Plan 01 with small cleanup, approve Plan 03 after tightening its snapshot contract, and require revisions to Plan 02 before execution. Plan 02 has a few implementation-level contradictions that could either fail compilation or produce ambiguous Stop-hook behavior. ## 34-01 Plan Review ### Strengths - Correctly starts with requirement and roadmap amendment before implementation. - Keeps Rust out of the doc/script phase. - DEPLOY.ps1 changes cover both future maintenance and runtime availability of `CHANGELOG.md`. - Good awareness that CHANGELOG stub must be appended after the bump commit so it remains unstaged. ### Concerns - **MEDIUM:** Verification commands mix Bash syntax into a Windows/PowerShell project (`test`, `grep`, `wc`, `/tmp`). This may fail under the stated shell. - **MEDIUM:** Final verification says `DEPLOY.ps1 -Bump patch -DryRun` must exit 0, while Task 3 explicitly downgrades that to non-blocking due dirty-tree sensitivity. - **LOW:** Creating 30-38 curated changelog entries by hand is large for one task and risks low-quality release notes unless timeboxed and reviewed. - **LOW:** Summary file output is required but not listed in `files_modified`. ### Suggestions - Convert automated verification to PowerShell or explicitly require Git Bash for plan execution. - Reconcile the `-Bump patch -DryRun` gate: either make it required in a temp clean clone or keep it non-blocking consistently. - Add a simple CHANGELOG quality check: valid H2 count, newest-first ordering, no TODO marker, no `[Unreleased]`. ### Risk Assessment: **MEDIUM** Mostly execution-environment risk, not product risk. The plan achieves the phase prerequisites if the shell/verification mismatch is fixed. ## 34-02 Plan Review ### Strengths - Good decomposition: sentinel I/O, parser, payload builder, hook wiring, and subcommand are centralized. - Covers the critical guards: perch resolution, `OWL_ECHO_COMMUNE`, and `stop_hook_active`. - Strong test matrix, including first-install, mismatch, equal sentinel, no-perch, remind rollback, and malformed args. - Avoids adding a regex dependency. ### Concerns - **HIGH:** The dispatch snippet uses `u8::from(version_changelog::run_version_remind(&old))`; `std::process::ExitCode` is opaque and does not reliably convert back to `u8`. This likely will not compile. - **HIGH:** The plan is internally inconsistent on Stop-hook control flow. It says mismatch emits `decision:"block"` and exits, but Task 2 continues to `set_idle_ready` and `spawn_echo_commune_if_live` after calling `maybe_emit_version_change_block()`. Decide whether a block emission should return early. - **MEDIUM:** `src/common/hook_output.rs` is modified in Task 2 but omitted from `files_modified`. - **MEDIUM:** `SPT_OVERRIDE_PKG_VERSION` violates the "compiled-in version source" requirement if honored in normal production runs. It should be gated harder or explicitly test-only/debug-only. - **MEDIUM:** Malformed sentinel version values are not clearly handled before XML emission. A hand-edited sentinel could inject unexpected text into the block. - **LOW:** The single-write-site audit expectation is vague because tests and helper references will also mention `last-seen-version`. ### Suggestions - Make `run_version_remind` either call `std::process::exit` internally or return `Result<(), i32>` / `i32`; do not convert from `ExitCode`. - Change `maybe_emit_version_change_block()` to return an enum such as `VersionPrompt::Emitted | NotEmitted`; in `hook_idle.rs`, explicitly `return` after `Emitted` if the intended behavior is "block only, no later side effects." - Add `src/common/hook_output.rs` to the plan frontmatter. - Gate the override with something like `SPT_TEST_ALLOW_VERSION_OVERRIDE=1`, or only honor it in debug/test builds with clear documentation. - Treat malformed sentinel JSON or malformed version strings as first-install/missing and silently rewrite current version, rather than rendering untrusted text. - Add an integration assertion for whether `spawn_echo_commune_if_live` should or should not run when a version block fires. ### Risk Assessment: **HIGH** The architecture is good, but the current plan has compile-risk and a semantic ambiguity at the core hook emission site. Fixing those before implementation should reduce it to medium/low. ## 34-03 Plan Review ### Strengths - Correctly isolates the user-facing instruction wording from the lower-level Rust work. - Covers all four required AUQ options verbatim. - Explicitly preserves `$OWL version-remind ` as a literal command. - Includes manual UAT for the real Claude Code behavior, which is appropriate for Stop-hook continuation semantics. ### Concerns - **MEDIUM:** Must-haves say the golden fixture snapshots the canonical block, but the task intentionally does substring-only assertions and allows golden drift. That is not really a snapshot test. - **MEDIUM:** The unresolved changelog path wording is contradictory: options say to read `$CLAUDE_PLUGIN_ROOT/CHANGELOG.md`, then extra text says not to read if `NOT_RESOLVED`. - **LOW:** The instruction "return control to whatever the user was working on" may be vague because the Stop hook fires after work is complete. - **LOW:** Full changelog rendering relies on Claude correctly parsing H2 ranges from a file at response time. This is acceptable, but UAT is essential. ### Suggestions - Either enforce full golden equality or rename the artifact to a "reference fixture" and keep substring assertions only. - Generate different instruction text when `changelog_path_resolved == false`; do not include "read `$CLAUDE_PLUGIN_ROOT/CHANGELOG.md`" in that branch. - Add an invariant that no literal `{old}`, `{new}`, `{step_count}`, `{old_date}`, or `{new_date}` remains in the final reason. - In UAT, explicitly verify the Stop hook does not re-fire immediately after `Yes full`, `Yes highlights`, or `Skip`. ### Risk Assessment: **MEDIUM** The plan is directionally sound. The main risk is prompt-instruction brittleness, which the UAT step mitigates. ## Overall Suggestions - Fix Plan 02 before execution; it is the critical path. - Normalize all automated verification to the project's actual Windows/PowerShell environment. - Make block-emission control flow explicit and tested. - Keep the changelog runtime path small and robust: if unavailable, degrade cleanly without contradictory instructions. - Ensure all generated summary files are listed in plan file manifests or explicitly treated as post-plan artifacts. Overall phase risk: **MEDIUM-HIGH until Plan 02 is revised**, then likely **MEDIUM** due the inherent fragility of Stop-hook-to-Claude instruction flow. --- ## Consensus Summary Single reviewer (Codex). Verdict: revise Plan 02 before execution; light cleanup on Plans 01 and 03. ### Top Concerns (severity-ranked) **HIGH (Plan 02 — must fix before execution):** 1. `u8::from(ExitCode)` won't compile — change `run_version_remind` to return `i32` or call `std::process::exit` internally. 2. Stop-hook control-flow ambiguity — `maybe_emit_version_change_block` must explicitly return early on emission OR explicitly proceed to `set_idle_ready` / `spawn_echo_commune_if_live`. Pick one, test it. **MEDIUM:** 3. (Plan 01) Verify commands use bash syntax (`test`, `grep`, `wc`, `/tmp`) in a Windows/PowerShell project; either convert or require Git Bash explicitly. 4. (Plan 01) `-Bump patch -DryRun` final verification contradicts Task 3's downgrade-to-non-blocking; reconcile. 5. (Plan 02) `src/common/hook_output.rs` modified but missing from `files_modified` frontmatter. 6. (Plan 02) `SPT_OVERRIDE_PKG_VERSION` honored in production runs violates compiled-in-only requirement; gate behind debug build or `SPT_TEST_ALLOW_VERSION_OVERRIDE=1`. 7. (Plan 02) Malformed sentinel JSON or malformed version string could inject untrusted text into the block; treat as first-install (silent rewrite). 8. (Plan 03) "Golden fixture" + substring-only assertions is contradictory — either full equality or rename to "reference fixture". 9. (Plan 03) NOT_RESOLVED branch contradicts the "read `$CLAUDE_PLUGIN_ROOT/CHANGELOG.md`" instruction. **LOW:** 10. (Plan 01) 30-38 hand-curated CHANGELOG entries is large; timebox or accept quality variance. 11. (Plan 02) Single-write-site audit expectation vague. 12. (Plan 03) Add invariant: no literal `{old}`/`{new}`/`{step_count}`/`{old_date}`/`{new_date}` placeholders remain in final reason. 13. (Plan 03) UAT should explicitly verify no re-fire after Yes-full / Yes-highlights / Skip. ### Divergent Views N/A (single reviewer). ### Next Step Run `/gsd-plan-phase 34 --reviews` to fold these concerns into a revision pass.