Inbox answer validation isn't atomic with resolving the question
This commit is contained in:
@@ -1,7 +1,5 @@
|
|||||||
# Bugs
|
# Bugs
|
||||||
|
|
||||||
- Inbox answer validation isn't atomic with resolving the question
|
|
||||||
`CheckResponse` reads open state outside the write transaction and `Respond` updates by id alone (`internal/agents/respond.go:26–34`, `:93`; `internal/store/inbox.go:140`), so two concurrent answers both land, or a withdrawal is overwritten. Condition the update on `state='open'`, check RowsAffected, roll back the batch on conflict and return 409.
|
|
||||||
- A malformed file image URL breaks transcript rendering
|
- A malformed file image URL breaks transcript rendering
|
||||||
`decodeURI` throws on e.g. `file:///tmp/100%.png` (`web/src/markdown.ts:347`), and `renderAll` appends events before mounting the composer (`web/src/task.ts:608–609`), so the rest of the transcript and the composer don't render. Catch the decode error and render the plain fallback link.
|
`decodeURI` throws on e.g. `file:///tmp/100%.png` (`web/src/markdown.ts:347`), and `renderAll` appends events before mounting the composer (`web/src/task.ts:608–609`), so the rest of the transcript and the composer don't render. Catch the decode error and render the plain fallback link.
|
||||||
- operations.md describes two incompatible event-stream designs
|
- operations.md describes two incompatible event-stream designs
|
||||||
|
|||||||
@@ -229,6 +229,8 @@ Agents ask questions with the `ask_human` MCP tool. Each question carries a `con
|
|||||||
| `other` (free text in `text`: an answer, a question back, or new instructions) | questions, errors | `answered` |
|
| `other` (free text in `text`: an answer, a question back, or new instructions) | questions, errors | `answered` |
|
||||||
| `dismiss` | errors | `dismissed` |
|
| `dismiss` | errors | `dismissed` |
|
||||||
|
|
||||||
|
A submission is all or nothing. `Store.Respond` resolves each item with an `UPDATE` conditioned on `state = 'open'` in one transaction, so when any item was answered, dismissed or withdrawn by someone else in the meantime (a second tab, the lead's `answer_question`, the agent's `cancel_question`), the whole submission is rolled back and refused with 409, and no response is recorded.
|
||||||
|
|
||||||
Answers reach the agent as one message holding every undelivered `option` and `other` response of the session (`deliveryMessage` in `internal/agents/inbox.go`), never while one of its questions is still open and unanswered (`AwaitingHuman` in `internal/store/inbox.go`); until then they stay held (`inbox_responses.delivered_at` empty). While a turn is running, the task session's PostToolUse hook (`afterTool` in `internal/agents/snapshots.go`, through `deliverMidTurn`) hands them over after the agent's next tool call as the hook's `additionalContext`, after any queued messages (see "Agent sessions"), so the agent reads them alongside that tool's result without being interrupted (for Codex, after each completed tool item, `deliverMidTurn` hands them over with `turn/steer` instead; when the steer fails, `restoreMidTurn` puts the taken queued messages back under their original ids and clears the answers' `delivered_at`, so `Deliver` sends them when the turn ends, and logs `mid-turn delivery restored`); the `user.message` event recording it carries `mid_turn: true` and no `head`, and the transcript doesn't treat it as a turn start. A turn that makes no further tool call leaves them to `Deliver`, which runs after each submission and when a turn ends, and sends them as a new turn once the session is `idle`. The log traces each answer: `inbox response recorded` when it is stored, `deliver held` with the reason, `deliver sent mid-turn`, `deliver sent` with the session's state and whether its agent process had exited or byre was closing, and `undelivered responses` with a reason wherever a session has held answers when its turn ends in a handoff or merge, its agent process ends, startup resumes it, or startup finds it idle. `sqlite3 byre.db "SELECT * FROM inbox_responses WHERE delivered_at IS NULL"` lists the answers held now.
|
Answers reach the agent as one message holding every undelivered `option` and `other` response of the session (`deliveryMessage` in `internal/agents/inbox.go`), never while one of its questions is still open and unanswered (`AwaitingHuman` in `internal/store/inbox.go`); until then they stay held (`inbox_responses.delivered_at` empty). While a turn is running, the task session's PostToolUse hook (`afterTool` in `internal/agents/snapshots.go`, through `deliverMidTurn`) hands them over after the agent's next tool call as the hook's `additionalContext`, after any queued messages (see "Agent sessions"), so the agent reads them alongside that tool's result without being interrupted (for Codex, after each completed tool item, `deliverMidTurn` hands them over with `turn/steer` instead; when the steer fails, `restoreMidTurn` puts the taken queued messages back under their original ids and clears the answers' `delivered_at`, so `Deliver` sends them when the turn ends, and logs `mid-turn delivery restored`); the `user.message` event recording it carries `mid_turn: true` and no `head`, and the transcript doesn't treat it as a turn start. A turn that makes no further tool call leaves them to `Deliver`, which runs after each submission and when a turn ends, and sends them as a new turn once the session is `idle`. The log traces each answer: `inbox response recorded` when it is stored, `deliver held` with the reason, `deliver sent mid-turn`, `deliver sent` with the session's state and whether its agent process had exited or byre was closing, and `undelivered responses` with a reason wherever a session has held answers when its turn ends in a handoff or merge, its agent process ends, startup resumes it, or startup finds it idle. `sqlite3 byre.db "SELECT * FROM inbox_responses WHERE delivered_at IS NULL"` lists the answers held now.
|
||||||
|
|
||||||
## Notifications
|
## Notifications
|
||||||
|
|||||||
@@ -2,6 +2,7 @@ package agents
|
|||||||
|
|
||||||
import (
|
import (
|
||||||
"context"
|
"context"
|
||||||
|
"errors"
|
||||||
"fmt"
|
"fmt"
|
||||||
"log/slog"
|
"log/slog"
|
||||||
"slices"
|
"slices"
|
||||||
@@ -61,6 +62,9 @@ func (m *Manager) Respond(ctx context.Context, user string, creates []api.InboxR
|
|||||||
}
|
}
|
||||||
|
|
||||||
err := m.store.Respond(ctx, user, resolutions)
|
err := m.store.Respond(ctx, user, resolutions)
|
||||||
|
if errors.Is(err, store.ErrNotOpen) {
|
||||||
|
return nil, fmt.Errorf("%w: %w", ErrConflict, err)
|
||||||
|
}
|
||||||
if err != nil {
|
if err != nil {
|
||||||
slog.Error("record inbox responses", "user", user, "err", err)
|
slog.Error("record inbox responses", "user", user, "err", err)
|
||||||
return nil, err
|
return nil, err
|
||||||
|
|||||||
+14
-6
@@ -5,6 +5,7 @@ import (
|
|||||||
"database/sql"
|
"database/sql"
|
||||||
"encoding/json"
|
"encoding/json"
|
||||||
"errors"
|
"errors"
|
||||||
|
"fmt"
|
||||||
"strings"
|
"strings"
|
||||||
"time"
|
"time"
|
||||||
|
|
||||||
@@ -114,6 +115,19 @@ func (s *Store) Respond(ctx context.Context, actor string, resolutions []Resolut
|
|||||||
return s.write(ctx, func(tx *sql.Tx, emit emitFunc) error {
|
return s.write(ctx, func(tx *sql.Tx, emit emitFunc) error {
|
||||||
for _, r := range resolutions {
|
for _, r := range resolutions {
|
||||||
resp := r.Response
|
resp := r.Response
|
||||||
|
res, err := tx.ExecContext(ctx, `UPDATE inbox_items SET state = ?, resolved_by = ?, resolved_at = ? WHERE id = ? AND state = ?`,
|
||||||
|
string(r.State), actor, now(), resp.ItemId, string(api.Open))
|
||||||
|
if err != nil {
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
n, err := res.RowsAffected()
|
||||||
|
if err != nil {
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
if n == 0 {
|
||||||
|
return fmt.Errorf("%w: %s", ErrNotOpen, resp.ItemId)
|
||||||
|
}
|
||||||
|
|
||||||
attachments, err := json.Marshal(resp.Attachments)
|
attachments, err := json.Marshal(resp.Attachments)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return err
|
return err
|
||||||
@@ -137,12 +151,6 @@ func (s *Store) Respond(ctx context.Context, actor string, resolutions []Resolut
|
|||||||
return err
|
return err
|
||||||
}
|
}
|
||||||
|
|
||||||
_, err = tx.ExecContext(ctx, `UPDATE inbox_items SET state = ?, resolved_by = ?, resolved_at = ? WHERE id = ?`,
|
|
||||||
string(r.State), actor, now(), resp.ItemId)
|
|
||||||
if err != nil {
|
|
||||||
return err
|
|
||||||
}
|
|
||||||
|
|
||||||
err = emitItem(ctx, tx, emit, "inbox.updated", resp.ItemId, actor)
|
err = emitItem(ctx, tx, emit, "inbox.updated", resp.ItemId, actor)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return err
|
return err
|
||||||
|
|||||||
Reference in New Issue
Block a user