test(identity): full status-code coverage for the identity contract e2e suite - #2005
Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 12776f0e52137e77644e3606f7f6014c93a90b36 and 4c321af. 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe change adds canonical 503 handling for persons-seed queue failures, introduces comprehensive identity endpoint error-contract tests, gates Rust-specific strict validation checks, and expands implementation-specific API coverage requirements. ChangesIdentity error contracts
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
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 `@src/backend/services/identity-resolution/src/api/seed.rs`:
- Around line 222-226: Handle the result of ops_repo::fail in the
try_enqueue_job error path instead of discarding it. Log the database failure
and propagate an appropriate error when updating the operation state fails; only
return the clean retryable 503 response after the failure update succeeds, so
the operation is not left queued without a job.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c46e9943-dd85-4be7-8b2a-3b6b78f8a02f
📥 Commits
Reviewing files that changed from the base of the PR and between 6a2d12a and 12776f0e52137e77644e3606f7f6014c93a90b36.
📒 Files selected for processing (4)
src/backend/services/identity-resolution/src/api/seed.rssrc/ingestion/tests/e2e/identity/test_error_contracts.pysrc/ingestion/tests/e2e/lib/api_coverage.pysrc/ingestion/tests/e2e/lib/identity.py
|
Addressed the CodeRabbit finding in ef969292: the |
12776f0 to
ef96929
Compare
Close the per-status-code gaps of the identity coverage report: validation 400s on the mutating endpoints, 404s for unknown ids on DELETE/GET, the 401/403 gate proven per-route (persons-seed reads, roles/person-roles/ visibility deletes), forest-route param validation, and the nil-tenant 400. Every case was checked against BOTH implementations' source before being added; the two Rust-only behaviors found (revoke-reason validation in DELETE bodies, nil viewed_person_id rejection) are documented divergences and stay out. 28 new tests, green on rust (99 passed) and dotnet (100 passed); coverage gate PASS 18/18. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
… 503 cases
Review round on the error-contract tests:
- Malformed-uuid / query-param 400s proven on both implementations
(DELETE ids, GET /v1/persons-seed/{id}, ?limit= / ?person= / ?active=,
including GET /v1/persons-seed?limit=abc).
- Rust-only strict validation behind a new explicit capability
(supports_strict_input_validation): revoke-reason cap in DELETE bodies,
nil viewed_person_id refusal, malformed subchart person_id.
- Coverage gate now BLOCKS on the full observed code matrix: REQUIRED_EXTRA
extended from the 7 mutation success codes to every proven error code,
with per-suite deltas (dotnet 422-family + legacy lookup vs rust
409-family) via the new IDENTITY_RUST_REQUIRED_EXTRA.
- The queue-full 503 on POST /v1/persons-seed is not deterministically
inducible black-box (capacity is a compile-time constant), so the refusal
is extracted into try_enqueue_job and pinned by Rust unit tests
(closed + full channel -> 503); sea-orm MockDatabase was not an option
(its feature breaks toolkit-db's Clone on DatabaseConnection).
Verified: e2e identity suite green on rust and dotnet; gates PASS
(rust 69/70 coverable codes — the one miss is the legacy endpoint's 200,
an approved removal; dotnet 71/71); cargo test 61 passed; clippy clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
CodeRabbit finding on #2005: the ops_repo::fail result was discarded. The 503 still goes out either way (retry-later is the right caller signal; a row left queued is reclaimed by the startup zombie sweep), but the DB failure is now logged instead of silently swallowed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
ef96929 to
c07772a
Compare
What
Closes the per-status-code gaps of the identity contract suite (#1753) and makes status-code coverage blocking in CI.
New e2e module —
identity/test_error_contracts.py(40 tests)mode).GET /v1/persons-seed/{id}.DELETE .../not-a-uuid,?limit=abc,?person=not-a-uuid,?active=maybe) — probed identical on both implementations.tenant_unresolved(was untested on any route).supports_strict_input_validation(same pattern as the existing capabilities, never probed from runtime): revoke-reason cap in DELETE bodies, present-but-nilviewed_person_idrefusal (.NET creates the grant!), malformed subchart person_id → 400 (.NET binder answers 404). Skipped before any request on dotnet.Coverage gate is now blocking per status code
The committed identity spec declares only
200per route, so the per-code advisory was vacuous.REQUIRED_EXTRAis extended from the 7 mutation success codes to the full observed error-code matrix, with per-suite deltas: dotnet (+422 family, legacy lookup 404) vs rust (+409 family) via the newIDENTITY_RUST_REQUIRED_EXTRA. A disappearing error test or a handler regressing to a different code now failsidentity(-rust)-endpoint-coverage-gateinstead of dimming an advisory. Self-cleaning once the spec declares real codes.The persons-seed 503 (queue full)
Not deterministically inducible black-box: the channel capacity is a compile-time constant (
gear.rs, 100) and the refusal needs the channel full at the instant of the POST. Instead the refusal path is extracted intotry_enqueue_job(behavior unchanged) and pinned by Rust unit tests (closed + full channel → 503).sea-ormMockDatabasewas evaluated and rejected: enabling itsmockfeature breakscf-gears-toolkit-db(mockDatabaseConnectionis notClone).Verification
E2E_IDENTITY_IMPLEMENTATION=rust ./e2e.sh test identity/E2E_IDENTITY_IMPLEMENTATION=dotnet ./e2e.sh test identity/./e2e.sh gates identity(rust)./e2e.sh gates identity(dotnet)cargo test -p identity-resolutioncargo clippy --all-targets/cargo fmtPart of epic #1602 / contract-test issue #1753.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
503 Service Unavailablewhen the seed queue can’t accept new work, avoiding orphaned operations.400 tenant_unresolved, and malformed request parameters produce consistent400errors.Tests