test(backend): cover clarification lifecycle clock behavior#42
test(backend): cover clarification lifecycle clock behavior#42devcool20 wants to merge 3 commits into
Conversation
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR extends the e2e full-flow test suite to validate clarification round-trip behavior with clock pause semantics. The changes introduce test infrastructure for capturing request/clarification lifecycle interactions, asserting clock segment pause windows and deadline shifts, and comparing policy-driven behavior divergence between EU and US jurisdictions. ChangesClarification Round-Trip E2E Test
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🤖 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 `@packages/backend/test/e2e/full-flow.api.e2e.test.ts`:
- Line 18: The test uses a too-small CLARIFICATION_RECEIPT_DELAY_MS (currently
10) causing flaky deadline-shift assertions where euDeadlineShiftMs may be 0;
increase CLARIFICATION_RECEIPT_DELAY_MS to a larger value (e.g. 50–200ms
depending on CI/storage resolution) wherever it's defined/used in this file
(symbol: CLARIFICATION_RECEIPT_DELAY_MS) and re-run tests, ensuring assertions
that compute euDeadlineShiftMs are stable; update all occurrences of that
constant in full-flow.api.e2e.test.ts so the clarification receipt delay is
consistently increased.
- Around line 189-203: Add explicit HTTP status assertions for the timeline and
clock explain requests: after sending the GET for
`/requests/${requestId}/timeline` (variables timeline and timelineBody) assert
the response status is the expected success code (e.g., 200) before calling
asEnvelope; do the same for the `/requests/${requestId}/clock/explain` response
(variables clock and clockBody). This ensures the flow test validates the HTTP
contract for the timeline and clock explain endpoints (and replicate the same
assertions for the subsequent similar block around lines 205-231).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 27ab34be-2696-4c1b-9429-4455a80fd3c5
📒 Files selected for processing (1)
packages/backend/test/e2e/full-flow.api.e2e.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use explicit types for function parameters and return values when they enhance clarity
Preferunknownoveranywhen the type is genuinely unknown
Use const assertions (as const) for immutable values and literal types
Leverage TypeScript's type narrowing instead of type assertions
Files:
packages/backend/test/e2e/full-flow.api.e2e.test.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: Use meaningful variable names instead of magic numbers - extract constants with descriptive names
Use arrow functions for callbacks and short functions
Preferfor...ofloops over.forEach()and indexedforloops
Use optional chaining (?.) and nullish coalescing (??) for safer property access
Prefer template literals over string concatenation
Use destructuring for object and array assignments
Useconstby default,letonly when reassignment is needed, nevervar
Alwaysawaitpromises in async functions - don't forget to use the return value
Useasync/awaitsyntax instead of promise chains for better readability
Handle errors appropriately in async code with try-catch blocks
Don't use async functions as Promise executors
Removeconsole.log,debugger, andalertstatements from production code
ThrowErrorobjects with descriptive messages, not strings or other values
Usetry-catchblocks meaningfully - don't catch errors just to rethrow them
Prefer early returns over nested conditionals for error cases
Extract complex conditions into well-named boolean variables
Use early returns to reduce nesting
Prefer simple conditionals over nested ternary operators
Don't useeval()or assign directly todocument.cookie
Avoid spread syntax in accumulators within loops
Use top-level regex literals instead of creating them in loops
Prefer specific imports over namespace imports
Use descriptive names for functions, variables, and types instead of relying on Oxlint + Oxfmt to catch naming issues
Add comments for complex logic, but prefer self-documenting code
Files:
packages/backend/test/e2e/full-flow.api.e2e.test.ts
**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{js,jsx,ts,tsx}: Write assertions insideit()ortest()blocks
Avoid done callbacks in async tests - use async/await instead
Don't use.onlyor.skipin committed code
Keep test suites reasonably flat - avoid excessivedescribenesting
Files:
packages/backend/test/e2e/full-flow.api.e2e.test.ts
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
TL;DR — Adds an e2e test that drives the clarification round-trip over the real HTTP runtime and asserts policy-driven clock behavior for EU (deadline shifts) vs US (deadline holds). The coverage is well-scoped and deterministic; a few small clarity polish suggestions inline.
Key changes
- Add
runClarificationRoundTriphelper — drivescapture → verify → clarify request/receive → fulfil → timeline → clock/explainand returns the clock summary, request id, and timeline event types. - Cover EU clarification clock pause — asserts
clarificationsegment hascountsTowardDeadline: false,pausesincludesclarification, andfinalDueAt > baseDeadline. - Cover US clarification no-shift — asserts segments contain only
base, pause is still recorded, andfinalDueAt === baseDeadline. - Cross-jurisdiction timeline parity — both EU and US emit the same ordered event types.
Summary ｜ 1 file ｜ 1 commit ｜ base: main ← fix/16-clarification-lifecycle-e2e
Jurisdiction substitution vs issue #16
Before: Issue #16 asked for UK as the no-stop comparison.
After: Test uses US, because UK's policy pack currently inherits the sharedstop_clockclarification default.
The PR body calls this out — worth a maintainer eyeball to confirm the substitution is intended rather than papering over a UK policy-pack gap that should itself be fixed.
packages/backend/test/e2e/full-flow.api.e2e.test.ts
Clock-segment duration coupling
Before: No e2e assertion on the relationship between clarification segment duration and deadline shift.
After:requireClockSegment(..., euDeadlineShiftMs)requires the EU clarification segment'sto - fromto equalfinalDueAt - baseDeadline.
Both quantities derive from the same server-side timestamps, so this is effectively an identity check on the policy implementation — flakiness from the delay(10ms) is bounded by the > 0 assertion rather than absolute timing.
packages/backend/test/e2e/full-flow.api.e2e.test.ts
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Bumped
CLARIFICATION_RECEIPT_DELAY_MSfrom 10ms to 200ms to harden the receipt-delay window against CI timestamp resolution. - Added HTTP status assertions for the
/timelineand/clock/explaincalls insiderunClarificationRoundTrip, so the helper now covers the full happy-path status sequence. - Replaced the
[eu.requestId, us.requestId].every(Boolean)truthiness check withexpect(eu.requestId).not.toBe(us.requestId), which actually proves the two round-trips produced distinct requests. - Collapsed the self-equal array assertion for the US clock into a direct
expect(us.clock.finalDueAt).toBe(us.clock.baseDeadline), making the no-shift contract explicit.
Claude Opus ｜ 𝕏
|
Thanks for contributing will check out this PR either later this week or next week, a bit busy traveling currently! |
|
Maintainer review: verified the UK-vs-US substitution — the UK pack inherits the shared |
2 participants
Closes #16
Summary
Adds backend e2e coverage for the clarification lifecycle over the real HTTP runtime.
The new test exercises:
It also verifies policy-driven clock behavior:
Note: the issue mentions UK as the no-stop comparison, but the current UK policy pack uses the shared
stop_clockclarification default. The existing no-stop clarification policy is covered with the US pack instead.Validation
bun x ultracite check packages/backend/test/e2e/full-flow.api.e2e.test.tsbun --cwd packages/backend vitest run test/e2e/full-flow.api.e2e.test.tscd packages/backend && bun run test:e2eNotes
cd packages/backend && bun run typecheckcurrently fails in unrelated persistence crypto code involving NodeBuffertype compatibility inpackages/internals/persistence/src/services/persistence/webhook-secret-encryption.ts.