chore: add e2e validation as required final phase in plan-adr#11
Merged
Conversation
Add worktree removal step after each squash-merge to prevent disk exhaustion during large epic runs (~4-8GB per worktree from build artifacts). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Lesson from ADR 0069 tracing epic: 7 PRs shipped, all CI green, but the feature was non-functional from the REPL. Missing e2e btscript test would have caught telemetry code path and serialization bugs. Changes: - Phase example now shows "E2E btscript test" as explicit Phase 4 - New phasing principle #5: e2e validation is REQUIRED for user-facing features - Explains why EUnit/BUnit alone are insufficient Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughTwo skill definition documents were updated. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
skills/pick-epic/SKILL.md (1)
91-95: Make post-merge cleanup idempotent to avoid flow interruption.Good addition overall, but as written these commands can hard-fail the run if the worktree/branch was already removed or values are mismatched. Consider guarded cleanup so Wave execution continues safely after partial retries (Line 91–Line 95).
Suggested resilient cleanup snippet
- git worktree remove --force <worktree-path> - git branch -D <branch-name> + git worktree list --porcelain | grep -F "worktree <worktree-path>" >/dev/null && \ + git worktree remove --force <worktree-path> || true + git show-ref --verify --quiet refs/heads/<branch-name> && \ + git branch -D <branch-name> || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/pick-epic/SKILL.md` around lines 91 - 95, Make the post-merge cleanup in SKILL.md resilient by guarding the git worktree removal and branch deletion so they don't hard-fail if already removed or mismatched: before running the git worktree remove --force <worktree-path> check that the worktree actually exists and only remove it if present (or run the removal and swallow non-fatal errors), and before running git branch -D <branch-name> verify the branch reference exists (or delete only if present) so Wave execution continues safely after partial retries; update the cleanup snippet to perform existence checks or noop on absent targets rather than allowing unhandled failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/plan-adr/SKILL.md`:
- Around line 204-205: The wording "The last issue in every epic MUST include an
e2e btscript test" is ambiguous versus the phase example that lists BT-G (E2E)
followed by BT-H (documentation); update the SKILL.md sentence and the example
so they match a single clear rule: either require the E2E test to be the
absolute final issue (change the sentence to "The final issue in every epic MUST
be an e2e btscript test" and move/remove BT-H after it) or allow E2E as the
final phase item (change the sentence to "Every epic's final phase MUST include
an e2e btscript test" and ensure BT-G appears within the final phase before
BT-H), and adjust the example lines showing BT-G/BT-H to reflect the chosen
option.
---
Nitpick comments:
In `@skills/pick-epic/SKILL.md`:
- Around line 91-95: Make the post-merge cleanup in SKILL.md resilient by
guarding the git worktree removal and branch deletion so they don't hard-fail if
already removed or mismatched: before running the git worktree remove --force
<worktree-path> check that the worktree actually exists and only remove it if
present (or run the removal and swallow non-fatal errors), and before running
git branch -D <branch-name> verify the branch reference exists (or delete only
if present) so Wave execution continues safely after partial retries; update the
cleanup snippet to perform existence checks or noop on absent targets rather
than allowing unhandled failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 507650c8-4db1-45b6-ac6f-b9aeb505b754
📒 Files selected for processing (2)
skills/pick-epic/SKILL.mdskills/plan-adr/SKILL.md
Comment on lines
+204
to
+205
| 5. **E2E validation as the final phase**: The last issue in every epic MUST include an e2e btscript test (`tests/e2e/cases/<feature>.btscript`) that exercises the feature end-to-end from the Beamtalk REPL. This catches integration failures invisible to unit tests — missing code path dependencies, serialization bugs, broken describe/list-tools responses. The ADR 0069 tracing epic shipped 7 PRs that all passed CI but were non-functional from the REPL because no e2e test existed. **EUnit and BUnit alone are insufficient for user-facing features.** | ||
|
|
There was a problem hiding this comment.
Clarify whether E2E test should be the last issue or in the last issue.
The principle states "The last issue in every epic MUST include an e2e btscript test," but the phase example (lines 68-70) shows the E2E test as a separate issue (BT-G) followed by documentation (BT-H). This creates ambiguity:
- If BT-H is the last issue, should it include the E2E test alongside documentation?
- Or should the E2E test always be the final issue, with no work items after it?
The intent is clear from context, but precise wording would prevent confusion during implementation.
📝 Suggested clarification
Consider one of these alternatives:
Option 1 (if E2E should be a separate final issue):
-5. **E2E validation as the final phase**: The last issue in every epic MUST include an e2e btscript test
+5. **E2E validation as the final phase**: Every epic MUST include an e2e btscript test as the final validation issue in the last phaseOption 2 (if E2E can be earlier in the final phase):
-5. **E2E validation as the final phase**: The last issue in every epic MUST include an e2e btscript test
+5. **E2E validation as the final phase**: Every epic MUST include an e2e btscript test in the final validation phaseAnd update the example to match the chosen interpretation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/plan-adr/SKILL.md` around lines 204 - 205, The wording "The last issue
in every epic MUST include an e2e btscript test" is ambiguous versus the phase
example that lists BT-G (E2E) followed by BT-H (documentation); update the
SKILL.md sentence and the example so they match a single clear rule: either
require the E2E test to be the absolute final issue (change the sentence to "The
final issue in every epic MUST be an e2e btscript test" and move/remove BT-H
after it) or allow E2E as the final phase item (change the sentence to "Every
epic's final phase MUST include an e2e btscript test" and ensure BT-G appears
within the final phase before BT-H), and adjust the example lines showing
BT-G/BT-H to reflect the chosen option.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Context
The ADR 0069 tracing epic shipped 7 PRs that all passed CI but were completely non-functional from the REPL — telemetry wasn't on the code path, serialization produced unreadable tuples, and MCP describe didn't list the new tools. An e2e btscript test would have caught all three issues.
🤖 Generated with Claude Code
Summary by CodeRabbit