fix(dry-run): include failed run-scoped step sample#218
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 (1)
WalkthroughAdds a failed run-scoped dry-run sample, updates sample lookup to return it, and adds tests covering failed-step fields, registry ordering, and dry-run command output. ChangesFailed run sample fixture and integration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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)
src/lib/dry-run/samples.ts (1)
858-879: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSpecial-casing the sentinel run id inline in the generic lookup couples
findSampleto one specific fixture.The
pathOnly === /runs/${SAMPLE_FAILED_RUN_ID}check embeds fixture-specific knowledge directly in the otherwise generic table-lookup loop. As more failure-scenario sentinels are added (per the pattern this PR establishes), this branch will grow with more ad-hoc string comparisons. Consider registering a dedicatedENTRIESitem with an exact-path pattern for the sentinel (ordered before the generic{runId}entry) sofindSamplestays a pure table lookup and new failure fixtures are added purely via data, not control flow.♻️ Sketch of a data-driven alternative
+entry('getRun', 'GET', `/runs/${SAMPLE_FAILED_RUN_ID}`, failedRunSample), entry('getRun', 'GET', '/runs/{runId}', passedRunSample),for (const e of ENTRIES) { if (e.method === upper && e.pattern.test(pathOnly)) { - const body = - e.operationId === 'getRun' && pathOnly === `/runs/${SAMPLE_FAILED_RUN_ID}` - ? failedRunSample - : e.body(requestBody); - return { ...e, body: () => body }; + const body = e.body(requestBody); + return { ...e, body: () => body }; } }Note: this requires the exact-path entry to be tested/ordered before the generic template so the more specific match wins — worth verifying
ENTRIESiteration order and pattern specificity before applying.🤖 Prompt for 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. In `@src/lib/dry-run/samples.ts` around lines 858 - 879, `findSample` is coupling generic lookup logic to the `SAMPLE_FAILED_RUN_ID` fixture via an inline `pathOnly === \`/runs/${SAMPLE_FAILED_RUN_ID}\`` check. Move this special case into the `ENTRIES` data by adding a dedicated exact-path entry for the failed run sentinel and place it before the generic `{runId}` pattern so it wins during iteration. Keep `findSample` as a pure table lookup that just selects the first matching entry and resolves `body` from `ENTRIES` without fixture-specific branching.
🤖 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 `@src/lib/dry-run/samples.ts`:
- Around line 348-398: `passedRunSample` in `samples.ts` is internally
inconsistent because `stepSummary.total/completed/passedCount` says 8 while the
`steps` array only contains 2 entries. Update `passedRunSample` so the summary
counts match the actual number of step objects, or expand the `steps` array to
the full set implied by `stepSummary`; keep the sample aligned with the
`RunResponse` contract used by `test steps --run-id <id> --dry-run`.
---
Nitpick comments:
In `@src/lib/dry-run/samples.ts`:
- Around line 858-879: `findSample` is coupling generic lookup logic to the
`SAMPLE_FAILED_RUN_ID` fixture via an inline `pathOnly ===
\`/runs/${SAMPLE_FAILED_RUN_ID}\`` check. Move this special case into the
`ENTRIES` data by adding a dedicated exact-path entry for the failed run
sentinel and place it before the generic `{runId}` pattern so it wins during
iteration. Keep `findSample` as a pure table lookup that just selects the first
matching entry and resolves `body` from `ENTRIES` without fixture-specific
branching.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 57f5b497-528a-4c65-aa01-a1b27fcd814f
📒 Files selected for processing (3)
src/commands/test.test.tssrc/lib/dry-run/samples.test.tssrc/lib/dry-run/samples.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/dry-run/samples.ts (1)
866-877: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBody is now eagerly computed even when callers never invoke
.body().The change resolves
e.body(requestBody)unconditionally on every match, then rebinds it into a closure. For pure/cheap factories this is harmless, but if anyentry.bodyfactory has side effects or is expensive, this changes evaluation semantics from lazy (invoked at call site) to eager (invoked insidefindSample).Given the current fixtures are simple literal returns, this is low risk, but worth a quick scan for any factory with side effects.
🤖 Prompt for 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. In `@src/lib/dry-run/samples.ts` around lines 866 - 877, The `findSample` implementation in `src/lib/dry-run/samples.ts` now eagerly evaluates `e.body(requestBody)` before returning the matched entry, which changes `body` from lazy to eager execution. Update the matching logic so `body` is only resolved when callers actually invoke it, or verify and document that every `ENTRY.body` factory is side-effect free and cheap; use the `findSample` loop and `ENTRY.body` rebinding as the key spots to inspect.
🤖 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.
Nitpick comments:
In `@src/lib/dry-run/samples.ts`:
- Around line 866-877: The `findSample` implementation in
`src/lib/dry-run/samples.ts` now eagerly evaluates `e.body(requestBody)` before
returning the matched entry, which changes `body` from lazy to eager execution.
Update the matching logic so `body` is only resolved when callers actually
invoke it, or verify and document that every `ENTRY.body` factory is side-effect
free and cheap; use the `findSample` loop and `ENTRY.body` rebinding as the key
spots to inspect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c984bd0-3bf9-45f3-84fe-fd4891518e1e
📒 Files selected for processing (2)
src/lib/dry-run/samples.test.tssrc/lib/dry-run/samples.ts
1 participant
What does this PR do?
Replaces accidentally closed PR #198.
Adds a dry-run path that demonstrates failed run-scoped step output without breaking the default
test wait --dry-runhappy path.After reviewer feedback on the original PR, this uses a sentinel run id approach:
GET /runs/{runId}dry-run sample stays terminalpassedGET /runs/run_failed_samplereturns the failed run-scoped sampletest steps --run-id run_failed_sample --dry-rundemonstrates the failed step mapping offlineRelated issue
Fixes #197
Type of change
Testing
Previously validated on the original PR branch with:
npm test -- src/lib/dry-run/samples.test.ts src/commands/test.test.tsnpm run typechecknpm run format:checknpm run lintNO_COLOR= npm testNotes for reviewers
This keeps wait flows on the default passed sample while still giving agents a documented failed run-scoped step sample for dry-run exploration.
Summary by CodeRabbit