test(session): cover created-session load failure arms - #121
test(session): cover created-session load failure arms#121cursor[bot] wants to merge 32 commits into
Conversation
Store participant and published-release identity for SessionState::Created with exact replay and fail-closed rebinding. Command-replay persistence stays outside this first slice.
Assert the Database error message and source, and fail the replay SELECT after ON CONFLICT by redirecting search_path so classify runs instead of the insert.
Linux llvm-cov leaves the isolated query_one ? tail uncovered unless the Err arm is an explicit match. Keep the search_path redirect test.
Linux branch coverage missed the later AND operands of exact-replay classification. Rebind each stored field independently, and prove a domain-legal u64::MAX creation time fails closed as ValueOutOfRange.
The replay SELECT failure constructed Database evidence without checking its safe display text or source, leaving those two production lines uncovered on Linux.
SHOW transaction_isolation can fail after the caller transaction is already aborted. Persist must surface that as a typed database error instead of leaving the probe Result uncovered.
Satisfy clippy::manual_let_else in the library test that instantiates AssessmentSessionPersistenceError::Database.
Name instrument_version_ref in the public persist contract and assert the committed version column. Keep TRACEABILITY, changelog, and as-built schema at Active PR #61 rather than promoting the slice to protected-main truth. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Restore persisted created-session identity from PostgreSQL without asking whether the original release still accepts new sessions, so later suspend or retire cannot rewrite provenance. Missing rows return none; later stored states and malformed lookup references fail closed. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Name the opened successor so TRACEABILITY, as-built schema, and ERD point at the persist-and-load head instead of the persist-only #106 slice. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Require the as-built table to name persist-and-load Active PR #109, not persist-only #61. Cover reconstitution of numeric or blank participant, release, and version references, and exercise load-path Database, InvalidStoredIdentity, and ValueOutOfRange through realistic PostgreSQL failures. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Review of 97895f4 vs main@a763735
Verified against the files in /workspace, not the PR body. This is a COMMENT only. Do not approve or merge.
1. Blocking defects
None on this head.
Created-only insert, exact replay, and fail-closed rebinding are unchanged. AssessmentSession::from_persisted_created (src/session.rs:208-246) does not call accepts_new_sessions(). load_assessment_session (src/postgres_assessment_session.rs:167-205) is a reconstitution lookup, not access control. Later-state persist is not implemented and is not claimed.
Claimed coverage is present:
- numeric-like and blank
participant_ref/instrument_release_ref/instrument_version_refintests/session_persisted_identity.rs:88-117 - missing-table
Database, CHECK-dropped corrupt digest →InvalidStoredIdentity, negativecreated_at_unix_ms→ValueOutOfRangeintests/postgres_assessment_session_persistence.rs:362-425 - TRACEABILITY / ERD /
AS_BUILT_SCHEMAname Active PR #121, not #109/#61, not Implemented
Error mapping on the load path is fail-closed: query_opt Err → Database (src/postgres_assessment_session.rs:181-182); later stored state → UnsupportedStoredState (:188-189); negative timestamp → ValueOutOfRange (:192-193); reconstitution failure → InvalidStoredIdentity (:204).
Local check: claimed documentation/session tests and cargo clippy --all-targets -- -D warnings are green. TEST_DATABASE_URL was unset here, so the Postgres load-failure test was not executed in this review environment.
2. Non-blocking issues
src/postgres_assessment_session.rs:54-55—ValueOutOfRangedisplay says the value “exceeds the PostgreSQL range.” The new load arm uses the same variant for a negative stored timestamp (:192-193), which is under-range. Operators will get a misleading message. Not a fail-open.src/postgres_assessment_session.rs:72-73—UnsupportedStoredStatetells the caller to “persist later lifecycle states before loading them.” That is a next-slice instruction, not an as-built claim, but it can be misread as later-state persist already existing.src/postgres_assessment_session.rs:119-126—persist_assessment_sessionaccepts anySessionState::Createdaggregate, including one built byfrom_persisted_created. HTTP start that uses the reconstitution constructor would bypass publication. Residual footgun for the next slice, not a defect in this one.tests/postgres_assessment_session_persistence.rs:354-358— loadInvalidReferenceis only exercised with"12345", not a blank lookup key. Sameok_orarm; not a claimed-coverage miss.docs/architecture/ERD.md:147-155— the target mermaid entity still haslatest_event_atand omitsinstrument_release_ref/ digest. The Active PR footnote at:427is the accurate physical claim. Do not treat the mermaid block as as-built.- Draft + required checks were still queued at review time. Unchecked Postgres box in the PR body is honest and must stay a CI gate.
3. Prefer this head over #109?
Yes. #109/ff82de8 left the load query_opt Err arm, participant/release/version reconstitution tails, load-path InvalidStoredIdentity, load-path ValueOutOfRange, and the as-built #61 pointer uncovered. This head covers those arms and retargets TRACEABILITY / ERD / as-built to Active PR #121. Prefer #121 over #109, persist-only #106/60b090d, and #61/7f7ea0f. Do not land draft #100 (0020) over this slice.
#125 already opened later-state/command-history persist on the uncovered #109 head and still points as-built at #109. Do not open another later-state persist PR. Rebase #125 onto #121 after this lands.
4. Next buyer-facing product gap
HTTP POST /v1/sessions (TRD §18), not another persist slice on this branch.
A stored Created row is not a product a participant can start. After this lands:
- Start path must call
AssessmentSession::new(src/session.rs:163) thenpersist_assessment_session(src/postgres_assessment_session.rs:119). Neverfrom_persisted_created+ persist. - Published-release input is missing on this head:
src/postgres_instrument_release.rsis persist-only. Use #98’s exactrelease_ref+ locale load that returns onlyPublishedevidence eligible for new sessions. - Transport errors belong on #87 (RFC 9457). There is no HTTP module on this head; do not invent OpenAPI ahead of the route.
GET /v1/sessions/{session_ref}/ resume must authorize againstloaded.participant_ref()viaauthorize+ProductPermission::ManageOwnSession(src/authorization.rs:327,:69) and, for anonymous callers,AnonymousSessionContext::is_valid_for_binding_at(src/anonymous_session.rs:148).load_assessment_sessionis not an access-control decision. The command-classify lane is #118, not this PR.- Durable Activate/Pause is the following persist slice: lift the Created-only guards at
persist_assessment_session(:124) andload_assessment_session(:188), persistaccepted_commandsfromAssessmentSession::apply_command(src/session.rs:308), addassessment_session_command. That work is already #125 — rebase it onto #121; do not start a third persist PR.
5. PR body vs code
No material mismatch. Out-of-scope statements match the code. Active PR pointers are #121. Later-state persist is not claimed. The unchecked Postgres test box matches TEST_DATABASE_URL being required. Commit 51e14d8 still says the as-built test names #109; 97895f4 retargeted the files and tests/documentation_architecture_contract.rs:304-319 now requires #121.
Do not merge until exact-head checks and independent last-push approval. I did not use CodeRabbit CLI as review evidence (agent auth is not available here).
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review of 97895f4 (created-session load coverage)
No blocking defect on this head. Prefer #121 over #109/ff82de8, persist-only #106/60b090d, and #61/7f7ea0f. Do not land draft #100 (0020) over this slice.
Created-only insert, exact replay, and fail-closed rebinding are unchanged. from_persisted_created does not call accepts_new_sessions(). load_assessment_session is a reconstitution lookup, not access control. Claimed arms are present:
- numeric/blank participant, release, and version refs
- missing-table
Database, CHECK-dropped digest →InvalidStoredIdentity, negative timestamp →ValueOutOfRange - TRACEABILITY / ERD / as-built name Active PR #121, not #109/#61, not Implemented
Residuals (not merge-blocking)
ValueOutOfRangetext still says the value “exceeds the PostgreSQL range” while the new load arm uses it for a negative stored timestamp.UnsupportedStoredStatesays “persist later lifecycle states before loading them,” which is easy to read as later-state persist already existing. That work is #125 and is still stacked on uncovered #109; rebase #125 onto this head after it lands. Do not open a third later-state PR.persist_assessment_sessionaccepts any Created aggregate, including one fromfrom_persisted_created. HTTP/start must callAssessmentSession::newthen persist. Do not treat load as authorization; resume must checkloaded.participant_ref().- Load
InvalidReferenceis only exercised with"12345", not a blank key.
Merge gate
Draft. Required checks were still queued at review time. The unchecked Postgres box stays a CI gate (TEST_DATABASE_URL). Do not merge until exact-head checks and independent last-push approval.
Next unique product gap
Not another persist slice on this branch. A stored Created row is not a product a participant can start. Next unique work is a start composition that uses AssessmentSession::new plus persist, then HTTP POST /v1/sessions once #87 (RFC 9457) and #98 (published-release load) are the landing vehicles. Do not invent OpenAPI ahead of the first route.
Sent by Cursor Automation: Fix Issues
| fn fmt(&self, formatter: &mut Formatter<'_>) -> std::fmt::Result { | ||
| formatter.write_str(match self { | ||
| Self::ValueOutOfRange => { | ||
| "assessment session persistence value exceeds the PostgreSQL range" |
There was a problem hiding this comment.
ValueOutOfRange is now also the load-path mapping for a negative stored created_at_unix_ms. The text still says the value “exceeds the PostgreSQL range,” which is true for u64::MAX persist overflow and misleading for a negative row. Keep the fail-closed mapping; retarget the caller-facing sentence so an operator repairs the stored timestamp instead of assuming BIGINT overflow.
| "stored assessment-session identity could not be restored; repair the row or persist a valid created session" | ||
| } | ||
| Self::UnsupportedStoredState => { | ||
| "load a created assessment session; persist later lifecycle states before loading them" |
There was a problem hiding this comment.
| require_read_committed(transaction)?; | ||
| if session.state() != SessionState::Created { | ||
| return Err(AssessmentSessionPersistenceError::UnsupportedInitialState); | ||
| } |
There was a problem hiding this comment.
Persist stores whatever Created aggregate it is given and does not re-check publication. That is required for exact replay after a later suspend, but it also means from_persisted_created + persist bypasses AssessmentSession::new. HTTP/start must not use this function as the start boundary. The next unique slice is a start composition that takes a live InstrumentRelease and calls new first.
| assert!(matches!( | ||
| load_assessment_session(&mut transaction, "12345"), | ||
| Err(AssessmentSessionPersistenceError::InvalidReference) | ||
| )); |
There was a problem hiding this comment.
The load InvalidReference arm is only taken with "12345". normalized_reference also rejects blank/whitespace. Add a blank-key case so the same ok_or arm is not covered by a single numeric example.
Point TRACEABILITY, as-built, ERD, and the documentation contract at the start-composition successor. #121 remains the uncovered-start predecessor. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
2 participants
Why
PR #109 can load a created session after a later suspend or retire, but required 100% line/branch coverage does not take the load
query_opterror arm, reconstitution of invalid participant/release/version references, or load-pathInvalidStoredIdentity/ValueOutOfRange. The as-built physical-schema table still named persist-only predecessor #61, which is the wrong merge pointer for this persist-and-load head.What this PR does
participant_ref,instrument_release_ref, andinstrument_version_refonfrom_persisted_created.Database), a CHECK-dropped corrupt digest (InvalidStoredIdentity), and a negative stored timestamp (ValueOutOfRange).docs/architecture/AS_BUILT_SCHEMA.mdat Active PR test(session): cover created-session load failure arms #121, not feat(session): load created sessions without rechecking publication #109 or feat(session): persist created sessions bound to published releases #61. This slice remains Active PR, not Implemented.Out of scope
load_assessment_sessionas authorizationTest plan
cargo test --test documentation_architecture_contract --test session_persisted_identity --test session_release_binding --test session_aggregate_lifecycle --test traceability_active_pr_contractcargo clippy --all-targets -- -D warningscargo test --test postgres_assessment_session_persistence(needsTEST_DATABASE_URL)This is the successor to #109. Prefer this head over #109, persist-only #106, and #61. Do not land draft #100 (
0020) over this slice. Do not merge until exact-head checks and independent last-push approval are satisfied.