fix(sdd): fail closed on empty task results - #2186
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe plugin treats empty or malformed SDD task results as terminal failures. It blocks downstream SDD phases and preserves existing artifacts. A benchmark validates this behavior and skips when dependencies are unavailable. Windows executable detection is also supported. ChangesSDD task-result handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Benchmark
participant NodeHarness
participant ReviewResultPlugin
participant SDDOrchestrator
Benchmark->>NodeHarness: Execute empty task-result scenario
NodeHarness->>ReviewResultPlugin: Submit empty task_result
ReviewResultPlugin-->>NodeHarness: Return sdd_task_result_empty
NodeHarness->>SDDOrchestrator: Attempt downstream SDD phase
SDDOrchestrator->>ReviewResultPlugin: Route sdd-apply
ReviewResultPlugin-->>SDDOrchestrator: Block with terminal failure
NodeHarness-->>Benchmark: Report failures and unchanged artifact
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bench/axis_sdd_task_result.go`:
- Around line 97-102: Update the Node subprocess setup around cmd.Run in the
benchmark flow to use context.WithTimeout with an appropriate bounded duration,
then create the command via exec.CommandContext. Ensure the context is canceled
and timeout failures propagate as command errors while preserving the existing
output capture and command arguments.
- Around line 47-59: The sddTaskResultUnavailable preflight must verify that the
available Node runtime can parse and strip the TypeScript plugin, not merely
that node exists on PATH. Add a capability check using the existing
plugin/harness execution path and return the documented unavailable message when
unsupported; update the corresponding axis description to state the required
unflagged Node version or --experimental-strip-types support.
In `@internal/assets/opencode/plugins/review-result-artifacts.ts`:
- Around line 496-504: Update the returned hooks object’s dispose method to
clear both admissionRecoveries and failedSDDSessions, preserving dispose as a
complete reset point for reused session IDs.
In `@internal/assets/review_plugin_recovery_test.go`:
- Around line 51-65: Update the SDD scenario mapping and table in
TestSDDTaskResultFailuresAreTerminalAndScoped so the scenario that supplies an
empty result also uses the "sdd-profile-empty" name, exercising the
"sdd-propose-cheap" phase and isSDDPhase suffix branch. Remove or replace the
currently unreachable empty-case naming while preserving the malformed
scenario’s existing coverage.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e63a4c44-9449-4f09-b7ca-93d7d4bf5f29
📒 Files selected for processing (10)
bench/axis_sdd_task_result.gobench/axis_sdd_task_result_test.gobench/main.gobench/main_test.gobench/runner.gointernal/assets/assets_test.gointernal/assets/opencode/plugins/review-result-artifacts.tsinternal/assets/opencode/sdd-orchestrator.mdinternal/assets/review_plugin_recovery_test.gointernal/assets/skills/_shared/sdd-phase-common.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bench/axis_sdd_task_result.go`:
- Around line 59-79: Update sddTaskResultNodeUnavailable to run the Node
capability probe with exec.CommandContext and a bounded timeout, creating the
context before launching node and cancelling it afterward. Preserve the existing
output handling and error message behavior, including reporting timeout-related
errors through the current detail fallback.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 44dd069a-fc46-452d-af06-a934b4e40827
⛔ Files ignored due to path filters (1)
testdata/golden/sdd-opencode-multi-settings.goldenis excluded by!testdata/**
📒 Files selected for processing (5)
bench/axis_sdd_task_result.gobench/axis_sdd_task_result_test.gointernal/assets/assets_test.gointernal/assets/opencode/plugins/review-result-artifacts.tsinternal/assets/review_plugin_recovery_test.go
|
The fail-closed, non-retryable behavior in this PR matches the recovery semantics we need. Preserving artifacts and blocking downstream phases is the correct response when hidden side effects cannot be ruled out. One scope gap needs clarification before this PR closes #2117: my reported occurrence involved a background Could you either:
The currently available beta build |
|
Scope note: #2117's approved scope covers only unsuffixed and profile-scoped |
d256280
into
Gentleman-Programming:main
3 participants
🔗 Linked Issue
Closes #2117
🏷️ PR Type
type:bug— Bug fix (non-breaking change that fixes an issue)type:feature— New featuretype:docs— Documentation onlytype:refactor— Code refactoringtype:chore— Build, CI, or tooling changestype:breaking-change— Breaking change📝 Summary
tr01-sdd-empty-task-resultbenchmark friction journey.📂 Changes
sdd-task-resultaxis and Windows executable-boundary support.🧪 Test Plan
go test ./internal/assets -run "^TestSDDTaskResultFailuresAreTerminalAndScoped$" -count=1go test ./internal/assets -run "^TestReviewResultArtifactsPluginContract$" -count=1go test . -run "^(TestSDDTaskResultAxisRegistration|TestSDDTaskResultAxisSkipsWithoutExternalPlugin|TestExecutableRecognizesBuiltBenchmarkBinary)$" -count=1frombench1 completed, 0 unsupported, 0 failed0 completed, 1 unsupported, 0 failedgit diff --checkgo test ./...frombenchis blocked on Windows by pre-existing POSIX fake-binary tests:TestProbeCapabilityRejectsAMissingFlag,TestProbeAndHelpProbeDoNotShareACacheEntry, andTestReadBackBlanksGitTrace.✅ Contributor Checklist
status:approvedtype:*label is appliedCo-Authored-Bytrailers💬 Notes for Reviewers
The provider/runtime may have performed hidden side effects before returning an invalid result, so this change intentionally does not retry. It marks the session terminal for SDD and requires an explicit user decision. Receipt-driven development was globally disabled for this work unit, so delivery is reported as disabled/unmanaged rather than carrying a native review receipt.
Summary by CodeRabbit
New Features
Bug Fixes