fix(dry-run): include failed run-scoped step sample#198
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe dry-run ChangesDry-run failed step sample
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
zeshi-du
left a comment
There was a problem hiding this comment.
Appreciate the intent (and the companion issue #197 makes the case well), but this reintroduces a defect we already fixed once, so I can't take it in this shape.
History: until 2026-05-21 the GET /runs/{runId} dry-run sample WAS failed-shaped, and it was a bug — test wait --dry-run and test run --wait --dry-run map terminal status through exitCodeForRunStatus (passed → 0, anything else → 1), so every offline happy-path smoke exited 1 and agents learned the wrong canned response. That's why the sample was deliberately flipped to passed-shape (see the comment this PR deletes — "defect-2"). This PR flips it back: with it merged, testsprite test wait run_x --dry-run && next-step breaks again. The rewritten snapshot test makes CI green, but green-by-rewriting-the-guard is exactly what that guard was for.
The underlying tension is real: one route-keyed sample serves two consumers (test wait wants passed; test steps --run-id wants an instructive failure). Two ways to resolve it that I'd accept:
- Sentinel run-id: teach
findSampleto special-case a documented sample id (e.g.run_failed_sample) that returns the failed shape, keeping the default passed.test steps --run-id run_failed_sample --dry-runthen demonstrates the failure mapping, and wait flows stay green. - Route the failure demo elsewhere: the failed-test sample already exists on the testId-scoped result surface (
SAMPLE_TEST_ID_FAILED); extend that entry with the step-level error detail instead of touching the run-scoped default.
Either keeps both consumers correct. Want to take one of these? (If you go with (1), please also restore the passed-shape guard test alongside the new failed-shape one for the sentinel id.)
|
Thanks for the detailed context. I updated this PR to take option 1 (sentinel run-id):
Validation run locally:
|
Summary
getRundry-run sample with a failed run-scoped assertion step carrying non-null error textstatus,failedStepIndex,failureKind,stepSummary) with that failed steptest steps --run-id ... --dry-runkeeps surfacing the failed step error and contributor flagContext
Follow-up to the maintainer request on #136 for the dry-run sample enrichment slice that #167 did not cover.
Fixes #197
Tests
Summary by CodeRabbit
get runresults to correctly represent terminal failed runs, including the correct failed step details, failure kind, and associated error text.getRunresponses and stdout JSON for the failed-run path, ensuring the failing and non-failing steps are classified correctly.