Add atomic agent turn finalization - #11
Conversation
|
Maintenance check: review routing is deferred because the head commit reports |
2c31aaa to
5d7499a
Compare
![dispatch-reviewer[bot]](https://avatars.githubusercontent.com/u/311562326?s=60&v=4){kind=small}
There was a problem hiding this comment.
Blocking changes requested.
- CI failed for
5d7499a5da54c0d06a783d74cfc9b76e354b49af:cargo fmt --checkreports formatting changes insrc/api.rsandsrc/store.rs; clippy and tests were skipped. - Direct terminal turn updates bypass the new atomic finalization path (inline comment).
| WHERE id = $6 AND conversation_id IN | ||
| (SELECT id FROM conversations WHERE tenant_id = $7 AND owner_ref = $8) | ||
| AND status IN ('pending', 'streaming') | ||
| RETURNING *", |
There was a problem hiding this comment.
PATCH /v1/turns/{id} can still transition an active turn directly to a terminal status. That bypasses the new finalization transaction, leaving a terminal turn with neither its agent output nor its continuation/finalization record. Terminal transitions need to be restricted to finalize_turn (or this endpoint must perform the same transaction).
There was a problem hiding this comment.
Blocking changes remain. The existing unresolved comment identifies that PATCH /v1/turns/{id} still bypasses atomic finalization for terminal transitions; additionally, the new finalization foreign key prevents truncation and regeneration from deleting continuations. CI is successful for c74734c.
| response jsonb NOT NULL, | ||
| response_digest bytea NOT NULL CHECK (octet_length(response_digest) = 32), | ||
| first_seq bigint NOT NULL CHECK (first_seq > 0), | ||
| last_seq bigint NOT NULL CHECK (last_seq >= first_seq - 1), |
There was a problem hiding this comment.
This foreign key uses the default ON DELETE NO ACTION, but both truncate_conversation and regenerate_conversation delete rows from continuations. After any successful finalization, either operation will therefore fail on the turn_finalizations.continuation_id constraint instead of modifying the transcript. Define the intended deletion behavior (for example, cascade/delete the matching finalization record as part of those operations) so existing conversation maintenance endpoints remain usable.
There was a problem hiding this comment.
Blocking migration compatibility issue: active turns that predate this migration become impossible to finalize. CI passed for 7eb0db8.
| @@ -0,0 +1,20 @@ | |||
| ALTER TABLE turns ADD COLUMN reserved_response_id text; | |||
There was a problem hiding this comment.
Existing turns get NULL here, but finalize_turn now requires reserved_response_id to equal the request response ID and PATCH no longer permits terminal statuses. Consequently, any pending/streaming turn already in production at migration time cannot be finalized or otherwise terminally transitioned, so it permanently occupies the active-turn slot. Please backfill/reserve response IDs for existing active turns or retain a compatible path for them.
|
Follow-up: the current |
|
Review routing is deferred: the current head |
|
Hourly maintenance follow-up: PR #11 is not ready for review routing because the current |
|
Maintenance follow-up: this PR is not eligible for review routing because the current head |
|
Follow-up: the current head |
|
Follow-up: the current head |
|
Maintenance follow-up: PR is not eligible for review routing because its current |
|
Maintenance check: PR #11 is not eligible for review routing: its current |
|
Follow-up: the latest |
|
Follow-up: the current head |
|
Hourly maintenance follow-up: the current |
|
Hourly maintenance follow-up: PR #11 is not eligible for review routing because its current |
|
Maintenance check: PR #11 is not eligible for review routing: its only current |
|
Maintenance follow-up: PR head |
|
Maintenance follow-up: current head |
|
Hourly maintenance follow-up: PR remains ineligible for review routing because its current |
|
Follow-up: the current head |
|
Maintenance follow-up: PR #11 is not ready for review routing: its current |
|
Maintenance follow-up: this PR was not routed for review because the required |
|
Maintenance follow-up: PR #11 remains blocked by its current |
|
Follow-up: the current |
|
Maintenance follow-up: the current |
|
Maintenance follow-up: the current |
|
Follow-up: PR is not ready for review routing because the current head |
|
Follow-up: the current head |
|
Maintenance follow-up: current head |
|
Follow-up: the current PR head |
|
Hourly maintenance check: the current head |
|
Follow-up: the current |
|
Maintenance follow-up: PR #11 is not eligible for review routing because its current |
|
Follow-up: the current head |
|
Maintenance follow-up: the current head |
|
Maintenance follow-up: PR #11 is not eligible for review routing: its current |
|
Current head |
Summary
Testing
git diff --checkcargo test(not run: Cargo is unavailable in the environment)Closes #6