Review: Cross-repo worktree isolation for parallel agents (re-review)

review-418-2026-03-25-r2 Review

review needs-refinement

Project

Re-review of board item #418 after refinements posted on Forgejo issue #188 (comment #7887). Two of three original findings addressed in comments but issue body not updated. One finding fully resolved.

Template Completeness

The refinement declares this a Bug, but the issue body uses the Feature template structure. Validating against both:

Feature template (current body structure)

[x] Type -- present ("Feature" -- should be "Bug" per refinement)
[x] Lineage -- present
[x] Repo -- present
[x] User Story -- present
[x] Context -- present, thorough
[x] File Targets -- present
[x] Acceptance Criteria -- present (6 items)
[x] Test Expectations -- present
[x] Constraints -- present
[x] Checklist -- present
[x] Related -- present

Bug template (if type changed to Bug)

[ ] What Broke -- not present (Context section covers this narratively but not in bug template format)
[ ] Repro Steps -- not present
[ ] Expected Behavior -- not present
[ ] Environment -- not present
[x] Acceptance Criteria -- present
[x] Related -- present

Assessment: The issue body is structured as a Feature and reads as a Feature. If the type stays Feature, template is complete. If the type changes to Bug, the body needs restructuring. Recommend keeping ### Type: Feature and updating board item label to type:feature -- this is new capability (SOP section, hook, agent instructions) motivated by an incident, not a regression fix.

Traceability

[x] story:dev-execute label -- present on board item #418. Resolved since first review.
[x] arch:ci-pipeline label -- present. Acceptable, though arch:agent-spawn might be more precise.
[x] Forgejo issue -- valid, open: forgejo_admin/pal-e-platform#188

All three traceability legs satisfied.

Original Finding 1: Type Mismatch

NOT RESOLVED. Refinement comment states "Type: Bug (not Feature). Updating issue type." but the issue body still reads ### Type\nFeature. The board item labels still say type:bug. The mismatch persists because the comment declared the intent but the issue body was never edited.

Furthermore, the deeper question remains: if this is declared a Bug, the issue body uses Feature template sections (User Story, Context, File Targets, Constraints) and is missing Bug-required sections (What Broke, Repro Steps, Expected Behavior, Environment). Changing the type header alone would create a template mismatch.

Recommendation: Keep ### Type: Feature in the issue body (which matches the body structure) and update the board item label from type:bug to type:feature. This is additive work (new SOP section, new hook, new agent profile section) motivated by an incident -- not a regression that needs to be rolled back.

Original Finding 2: Missing Story Label

RESOLVED. Board item #418 now has story:dev-execute in labels.

Original Finding 3: Repo Placement

PARTIALLY RESOLVED. Refinement comment clarifies: "Primary PR lands on ldraney/claude-custom. SOP updates land in pal-e-docs via MCP tools. Issue stays on pal-e-platform as tracking repo." This is a reasonable split. However, the clarification only exists in the Forgejo comment -- the issue body still says "likely multiple PRs across repos" without specifying which. A Dev agent reading only the issue body would not know where to submit the PR.

Recommendation: Edit the issue body's Checklist section to specify: "PR opened on ldraney/claude-custom for hooks + agent profile" and "SOP updates via pal-e-docs MCP (separate task, not this PR)." Alternatively, add a one-liner to the Repo section.

NOT APPLIED. The refinement comment states "Cleanup criterion added" but the issue body's Acceptance Criteria section is unchanged -- still 6 items, none mentioning /tmp/ cleanup. The original blast radius finding about orphaned /tmp/ clones remains unaddressed in the spec.

File Targets

No changes since first review. All file targets still verified:

[x] hooks/cross-repo-isolation.sh (new) -- confirmed does not exist yet, valid path
[x] agents/dev.md -- exists, no cross-repo isolation instructions present
[x] worktree-workflow SOP -- exists, no Cross-Repo Isolation section
[x] agent-spawn-conventions -- exists, pre-spawn checklist has no cross-repo step
[x] terraform/ and salt/ exclusions -- correct

Repo Placement

See Finding 3 above. Comment clarifies intent but issue body is ambiguous.

Dependencies

No change from first review. Board item #241 (worktree auto-rebase) and #347 (agent-spawn bug) are related but not blocking. No dependency labels needed.

Acceptance Criteria

Same 6 criteria, all testable. Missing the /tmp/ cleanup criterion that the refinement claimed to add.

Blast Radius

Same as first review. The QA agent question was addressed in the refinement: "QA agents are read-only (no Write/Edit tools) so they can't clobber branches." This is a valid answer -- QA agents don't make git changes so cross-repo isolation is Dev-only. This should be noted in the issue body or constraints for the implementing agent's benefit.

Recommendation

Two items must be resolved before READY:

Type alignment: Edit the board item label to type:feature (to match the issue body's Feature structure and template), OR edit the issue body to use the Bug template. The mismatch cannot persist -- the enforcement hooks will flag it. Changing the board item label is the simpler path since the issue body is well-structured as a Feature.
Issue body edits: The refinement comment addressed findings but the issue body was never updated. Three edits needed: (a) Repo/Checklist section: specify PR target is ldraney/claude-custom, (b) Acceptance Criteria: add /tmp/ cleanup criterion, (c) Constraints: note QA agents excluded (read-only, no git changes).

Once these two items are resolved, the ticket is READY. The scope is solid, file targets are verified, traceability is complete, and the work is well-bounded.