diff --git a/AGENTS.md b/AGENTS.md index 59fa67ab63..63c9d83e95 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -42,7 +42,7 @@ Architecture notes and per-area conventions live alongside the code they describ - `apps/backend/internal/agentctl/AGENTS.md` — agentctl HTTP server: route groups, adapter model, ACP protocol. - `apps/backend/internal/agentctl/server/api/AGENTS.md` — reverse-proxy body rewriting (`Accept-Encoding`), iframe-blocking header stripping. - `apps/backend/internal/integrations/AGENTS.md` — adding a new third-party integration (Jira/Linear pattern, both backend and frontend halves). The `/add-integration` skill mirrors this for scaffolding new integrations. -- `apps/backend/internal/office/AGENTS.md` — Office (autonomous agent management): pointer to the 15-file `docs/specs/office/` spec set with status per file, plus local traps (run-vs-task-start, retired `heartbeat` source, continuation summary scoping, participant slate ownership, routine idle-skip, dual advancement signals). +- `apps/backend/internal/office/AGENTS.md` — Office (autonomous agent management): pointer to the `docs/specs/office/` spec set with status per file (a legacy 15-file table plus newer `requirements/`+`system-design/`-split entries, most recently task session termination and the seat-claim decision guard), plus local traps (run-vs-task-start, retired `heartbeat` source, continuation summary scoping, participant slate ownership, routine idle-skip, dual advancement signals). - `apps/desktop/AGENTS.md` — Tauri desktop app: runtime resources, Rust process lifecycle, packaging, signing, and smoke tests. - `apps/web/AGENTS.md` — Vite/React SPA frontend: shadcn imports, Go boot-payload hydration, store slice structure (incl. `office`), WS format, component conventions, TS lint limits. @@ -162,21 +162,18 @@ history and remains immutable. `python3 scripts/list-docs.py specs --system --format paths`; use `--kind legacy` to include legacy sources. Run `python3 scripts/list-docs.py validate` and `python3 scripts/lint-spec-files.py --all` after specification changes. - **Decisions:** Architecture decisions are recorded in `docs/decisions/`. Use `python3 scripts/list-docs.py decisions --format markdown` to find relevant ADRs. The `docs/decisions/INDEX.md` page contains command examples. When making significant architectural choices, create a new ADR via `/record decision`. -- **Plans:** Implementation plans are generated from requirements and system - designs through `/plan`. `docs/plans//plan.md` is a work-package - manifest. Its sibling `task--.md` files are work orders. +- **Plans:** Implementation plans are generated from requirements and system designs through `/plan`. `docs/plans//plan.md` is a work-package manifest. Its sibling `task--.md` files are work orders. ### Plan Implementation -- Requirements and system designs define durable behavior and technical - boundaries. Plans and work orders define implementation scope, dependency - order, and task-level validation. Keep their statuses and results accurate. +- Requirements and system designs define durable behavior and technical boundaries. Plans and work orders define implementation scope, dependency order, and task-level validation. Keep their statuses and results accurate. ### Observability - In dev mode (`KANDEV_MOCK_AGENT=true` or `debug.pprofEnabled`), `/debug/vars` exposes the stdlib expvar handler. Office provider-routing metrics live under `routing_*` (route attempts, fallbacks, parked runs, provider degraded/recovered counters). The metrics are also still emitted as structured `routing.metric.*` zap logs for human debugging. - ADR 0015's step-completion-signal telemetry lives under `workflow_*`: `workflow_step_completion_signal_received_total` (`internal/workflow/signalmetrics/`), labelled by `source` and `agent_type`, counts accepted `step_complete_kandev` signals. Its separate `workflow_step_completion_signal_fallback_used_total` counter is labelled by `agent_type` and counts manual fallback uses. The fallback button does not have a production increment site yet, so the counter remains zero until that UI ships. - Office stall detection (REQ-OFFICE-STALL-VISIBILITY) lives under `office_stall_*`: `office_stall_stranded_signal_total` (labelled by `gate`, which names which of the two watchdog gate sites saw it), `office_stall_decision_waiting_total`, and `office_stall_detector_skipped_total` (labelled by `reason`, so a detector that fails closed is visible rather than silent). Detection only: these counters never accompany a transition, a synthesized decision, or a queued run, because Office tasks are surfaced and never reclaimed. Also emitted as structured zap logs. +- Office session-termination suppression (REQ-OFFICE-SESSION-TERM) lives under `office_session_term_suppressed_total`, labelled by `reason` (one of `task_reassigned`, `participant_removed`, `participant_seat_claimed`) and `outcome` (`runner` or `seat` when the agent still holds that capacity, `read_failed` when the capacity read itself failed). It counts guarded terminations that left a session live, so a suppression is distinguishable from a path that never ran. Both dimensions are closed sets; no task, agent, step or session identifier is ever a label. Also emitted as structured zap logs. ### GitHub Operations diff --git a/apps/backend/internal/office/AGENTS.md b/apps/backend/internal/office/AGENTS.md index 44edd178b9..63a42a8cc8 100644 --- a/apps/backend/internal/office/AGENTS.md +++ b/apps/backend/internal/office/AGENTS.md @@ -4,7 +4,7 @@ ## Spec authority -`docs/specs/office/` (15 files) is the authority for what Office is and why — it outranks any card, register, or comment. **Where code and spec disagree, that is a defect in one of them; do not silently follow the code.** A sixteenth office-tagged spec, [per-agent + per-role tier selection](../../../../docs/specs/office-agent-tier-routing/spec.md), lives in a sibling directory, not under `docs/specs/office/`. +`docs/specs/office/` (15 files, plus later requirements/system-design additions below) is the authority for what Office is and why — it outranks any card, register, or comment. **Where code and spec disagree, that is a defect in one of them; do not silently follow the code.** A sixteenth office-tagged spec, [per-agent + per-role tier selection](../../../../docs/specs/office-agent-tier-routing/spec.md), lives in a sibling directory, not under `docs/specs/office/`. | Spec | Status | Covers | |---|---|---| @@ -23,6 +23,10 @@ | `automations-settings.md` | draft | Scheduling an automation (cron / PR event / webhook) from Settings, not the board | | `testing.md` | shipped | E2E mock harness for task sessions/messages — Playwright can't launch real executors in CI | | `unread-divider.md` | shipped | Slack-style unread divider for background-running task sessions | +| `requirements/task-session-termination.md` + `system-design/task-session-termination-01.md` | shipped | Retained-capacity precondition on the three session-termination call sites (role removal, seat-claim displacement, reassignment): don't end a shared (task, agent) session while the agent still holds another capacity | +| `requirements/seat-claim-decision-guard.md` + `system-design/seat-claim-decision-guard-01.md` | shipped | `claimAutoSeat`'s `NOT EXISTS` guard against a roleless decision race; latent on every shipped decision path but the only defense on that path | + +The two rows above are the only entries reflecting the newer `requirements/` + `system-design/` split; the other 15 predate that migration and this table has not been reconciled with the full current Office spec set. `docs/specs/office/README.md` no longer carries a tracked specification map (see ADR `2026-09-07-on-demand-document-catalogs`) — run `python3 scripts/list-docs.py specs --system office --format paths` for the authoritative, current list of anything not covered here. `office-agent-tier-routing/spec.md`'s own front matter still calls `routing.md` "authoritative" for tiers, provider order, execution profiles, provider health, and wake-reason policy — that predates `routing.md`'s archival in `docs/specs/INDEX.md` and is now stale; trust the INDEX status over the sibling spec's own text. diff --git a/apps/backend/internal/office/dashboard/export_test.go b/apps/backend/internal/office/dashboard/export_test.go new file mode 100644 index 0000000000..25d90c4c08 --- /dev/null +++ b/apps/backend/internal/office/dashboard/export_test.go @@ -0,0 +1,12 @@ +package dashboard + +// SetSessionCapacityReadHook installs a yield point between a guarded +// termination path's own commit and the retained-capacity determination's +// read, and returns a function restoring the previous value. This file is +// compiled only under `go test`, so the hook has no setter at all in a +// production build. +func SetSessionCapacityReadHook(fn func(taskID, agentProfileID string)) func() { + prev := sessionCapacityReadHook + sessionCapacityReadHook = fn + return func() { sessionCapacityReadHook = prev } +} diff --git a/apps/backend/internal/office/dashboard/service_tasks.go b/apps/backend/internal/office/dashboard/service_tasks.go index b4fe507d46..6073951839 100644 --- a/apps/backend/internal/office/dashboard/service_tasks.go +++ b/apps/backend/internal/office/dashboard/service_tasks.go @@ -431,16 +431,11 @@ func (s *DashboardService) addOrRemoveParticipant( } // Flip the participant's office session row to COMPLETED so it leaves // the live indicators and the next add would create a fresh row - // (preserving historical conversation separation). - if s.sessionTerm != nil { - if err := s.sessionTerm.TerminateOfficeSession(ctx, taskID, agentID, sessionTermReasonRoleRemoved); err != nil { - s.logger.Warn("terminate office session on participant removal failed", - zap.String("task_id", taskID), - zap.String("agent_profile_id", agentID), - zap.String("role", role), - zap.Error(err)) - } - } + // (preserving historical conversation separation). Skipped when the + // agent still holds another capacity on this task: the removed role was + // not the only thing addressing it here. + s.terminateSessionUnlessRetained(ctx, taskID, agentID, sessionTermReasonRoleRemoved, + zap.String(roleLogKey, role)) s.publishTaskUpdated(ctx, taskID, []string{field}) s.logParticipantActivity(ctx, taskID, agentID, role, "task_participant_removed") return nil @@ -477,18 +472,11 @@ func (s *DashboardService) applyParticipantAddOutcome( // terminateDisplacedSession ends the displaced agent's live office session // for the role a claim just took the seat away from. Best-effort, // mirroring the removal branch's own termination call: a failure is -// logged, not surfaced. +// logged, not surfaced. An agent that still runs the task, or is seated in +// another role, keeps the session it is still using. func (s *DashboardService) terminateDisplacedSession(ctx context.Context, taskID, displacedAgentID, role string) { - if s.sessionTerm == nil || displacedAgentID == "" { - return - } - if err := s.sessionTerm.TerminateOfficeSession(ctx, taskID, displacedAgentID, sessionTermReasonSeatClaimed); err != nil { - s.logger.Warn("terminate office session on seat claim failed", - zap.String("task_id", taskID), - zap.String("agent_profile_id", displacedAgentID), - zap.String(roleLogKey, role), - zap.Error(err)) - } + s.terminateSessionUnlessRetained(ctx, taskID, displacedAgentID, sessionTermReasonSeatClaimed, + zap.String(roleLogKey, role)) } // cancelDisplacedRun cancels the run the step-entry fan-out queued for the @@ -966,18 +954,13 @@ func (s *DashboardService) runReactivityForAssigneeChange( } // Flip the prev assignee's office session row to COMPLETED so it leaves // the active sessions list. The reactivity pipeline already hard-cancels - // the running execution above; this is the persistent-row counterpart. - // A same-agent reassignment is not a handoff — the pipeline above never - // interrupts it — so this must not terminate the agent's own live - // session out from under it. - if prevAssigneeID != "" && prevAssigneeID != newAssigneeID && s.sessionTerm != nil { - if err := s.sessionTerm.TerminateOfficeSession(ctx, taskID, prevAssigneeID, sessionTermReasonReassigned); err != nil { - s.logger.Warn("terminate prev-assignee office session failed", - zap.String("task_id", taskID), - zap.String("agent_profile_id", prevAssigneeID), - zap.Error(err)) - } - } + // the running execution above, unconditionally: an agent reassigned away + // but still seated as a reviewer stops the turn it no longer owns and + // keeps the row it still reviews through. Those are two different objects + // and only this one is guarded. A same-agent reassignment is not a + // handoff either — the guard's own capacity read observes the agent + // still holding the runner capacity and suppresses termination. + s.terminateSessionUnlessRetained(ctx, taskID, prevAssigneeID, sessionTermReasonReassigned) // Auto-dismiss any inbox entry tied to the prior (task, agent) so // the user isn't asked to triage a failure they already worked // around by reassigning. Counter is intentionally not reset — the @@ -996,6 +979,109 @@ const ( sessionTermReasonSeatClaimed = "participant_seat_claimed" ) +// retainedCapacity names a standing that keeps an agent addressed on a task. +// Two exist, and either one alone keeps the pair's office session live. +type retainedCapacity string + +const ( + capacityNone retainedCapacity = "" + capacityRunner retainedCapacity = "runner" + capacitySeat retainedCapacity = "seat" +) + +// sessionCapacityReadHook is a test-only yield point invoked immediately +// before the determination reads capacity, letting a test seed a capacity +// change in the guarded path's own commit-to-read window +// (AC-OFFICE-SESSION-TERM-002.12). Nil in production; set only through +// SetSessionCapacityReadHook in export_test.go. +var sessionCapacityReadHook func(taskID, agentProfileID string) + +// retainsTaskCapacity reports which capacity, if any, agentProfileID still +// holds on taskID. +// +// The runner half reads the task's execution fields, whose assignee value is +// the shared runner projection. Resolving the runner any other way — a direct +// read of runner seats, say — answers a different question, because the +// projection's last tier deliberately reads runner rows across every step. +// +// The seat half reads the effective slate at the task's current step, which +// already merges template-level rows under per-task precedence and already +// spans every role. The step scope is load-bearing: seats are keyed +// (step, task, role, agent) and survive a step change, so an unscoped read +// would find a seat naming a previous occupant and suppress every future +// termination for the pair. +// +// Both reads run on the read pool and take no lock. The determination is not +// atomic with the mutation that preceded it and does not need to be: each +// guarded path reads strictly after its own commit, so two concurrent paths +// removing different capacities end with the session terminated by whichever +// reads second. +func (s *DashboardService) retainsTaskCapacity( + ctx context.Context, taskID, agentProfileID string, +) (retainedCapacity, error) { + exec, err := s.repo.GetTaskExecutionFields(ctx, taskID) + if err != nil { + return capacityNone, fmt.Errorf("read task runner: %w", err) + } + if exec != nil && exec.AssigneeAgentProfileID == agentProfileID { + return capacityRunner, nil + } + seats, err := s.repo.ListAllTaskParticipants(ctx, taskID) + if err != nil { + return capacityNone, fmt.Errorf("read task participant slate: %w", err) + } + for _, seat := range seats { + if seat.AgentProfileID == agentProfileID { + return capacitySeat, nil + } + } + return capacityNone, nil +} + +// terminateSessionUnlessRetained is the single guarded termination step, +// shared by the three paths that end a session on the loss of one capacity: +// role removal, seat claim displacement, and reassignment. It ends the pair's +// session only when the agent has lost its last capacity on the task. +// +// It fails closed. A capacity read that errors suppresses the termination, +// because a session left live is recovered by the next guarded path that runs +// on the pair, and a session wrongly ended is not. +// +// Suppression is not an error: every caller here already treats termination as +// best-effort, and a suppressed termination is a normal outcome. +func (s *DashboardService) terminateSessionUnlessRetained( + ctx context.Context, taskID, agentProfileID, reason string, extra ...zap.Field, +) { + if s.sessionTerm == nil || taskID == "" || agentProfileID == "" { + return + } + fields := append([]zap.Field{ + zap.String("task_id", taskID), + zap.String("agent_profile_id", agentProfileID), + zap.String("reason", reason), + }, extra...) + + if sessionCapacityReadHook != nil { + sessionCapacityReadHook(taskID, agentProfileID) + } + capacity, err := s.retainsTaskCapacity(ctx, taskID, agentProfileID) + if err != nil { + recordSessionTermSuppressed(reason, sessionTermSuppressReadFailed) + s.logger.Warn("office session termination suppressed: capacity read failed", + append(fields, zap.Error(err))...) + return + } + if capacity != capacityNone { + recordSessionTermSuppressed(reason, string(capacity)) + s.logger.Info("office session termination suppressed: agent retains a capacity", + append(fields, zap.String("retained_capacity", string(capacity)))...) + return + } + if err := s.sessionTerm.TerminateOfficeSession(ctx, taskID, agentProfileID, reason); err != nil { + s.logger.Warn("terminate office session failed", append(fields, zap.Error(err))...) + } +} + // publishTaskUpdated emits an OfficeTaskUpdated event listing the fields // that changed. Frontend subscribers re-fetch the task DTO. Silently // skipped when no event bus is configured. diff --git a/apps/backend/internal/office/dashboard/session_capacity_test.go b/apps/backend/internal/office/dashboard/session_capacity_test.go new file mode 100644 index 0000000000..183f7aee4e --- /dev/null +++ b/apps/backend/internal/office/dashboard/session_capacity_test.go @@ -0,0 +1,256 @@ +package dashboard_test + +import ( + "context" + "testing" + + "github.com/jmoiron/sqlx" + "github.com/kandev/kandev/internal/office/dashboard" +) + +// The retained-capacity determination is unexported, so these tests drive it +// through the participant-removal guarded path: removal always reaches the +// termination step, so whether the terminator was called is a direct readout +// of what the determination decided. + +// seedSeat writes a participant seat directly. A taskID of "" makes it a +// template-level row, which the effective slate projects onto every task at +// that step. +func seedSeat(t *testing.T, db *sqlx.DB, id, stepID, taskID, role, agentID string, decisionRequired int) { + t.Helper() + if _, err := db.Exec(` + INSERT INTO workflow_step_participants + (id, step_id, task_id, role, agent_profile_id, decision_required, position, provenance) + VALUES (?, ?, ?, ?, ?, ?, 0, 'manual') + `, id, stepID, taskID, role, agentID, decisionRequired); err != nil { + t.Fatalf("seed seat %s: %v", id, err) + } +} + +// moveTaskToStep repoints the task at a different step, leaving every seat +// written at the old step in place. That is what production does: seats are +// keyed (step, task, role, agent) and are not cleaned up on a step change. +func moveTaskToStep(t *testing.T, db *sqlx.DB, taskID, stepID string) { + t.Helper() + if _, err := db.Exec(`UPDATE tasks SET workflow_step_id = ? WHERE id = ?`, stepID, taskID); err != nil { + t.Fatalf("move task %s to %s: %v", taskID, stepID, err) + } +} + +// removeReviewerExpectingTermination runs the removal path and reports +// whether the session terminator fired. +func removeReviewer(t *testing.T, deps *testDeps, rt *recordingTerminator, taskID, agentID string) int { + t.Helper() + before := len(rt.calls) + if err := deps.svc.RemoveTaskReviewer(context.Background(), "", taskID, agentID); err != nil { + t.Fatalf("remove reviewer %s: %v", agentID, err) + } + return len(rt.calls) - before +} + +func newCapacityDeps(t *testing.T, taskID string) (*testDeps, *recordingTerminator) { + t.Helper() + deps := newTestDeps(t) + rt := &recordingTerminator{} + deps.svc.SetSessionTerminator(rt) + insertTestTask(t, deps.db, taskID, "ws-cap", "Capacity", "todo", 2) + return deps, rt +} + +// AC-OFFICE-SESSION-TERM-002.1 (runner alternative), -001.3: an agent that is +// still the task's runner keeps its session when a role is removed. +func TestRemoveParticipant_SuppressesWhenAgentIsRunner(t *testing.T) { + deps, rt := newCapacityDeps(t, "task-runner") + ctx := context.Background() + + if err := deps.svc.AddTaskReviewer(ctx, "", "task-runner", "agent-dual"); err != nil { + t.Fatalf("add reviewer: %v", err) + } + if _, err := deps.repo.UpdateTaskAssignee(ctx, "task-runner", "agent-dual"); err != nil { + t.Fatalf("set runner: %v", err) + } + + if got := removeReviewer(t, deps, rt, "task-runner", "agent-dual"); got != 0 { + t.Errorf("runner still seated on the task: want 0 terminations, got %d", got) + } +} + +// AC-OFFICE-SESSION-TERM-002.1 (seat alternative), -002.5, -002.6: an agent +// holding a second seat keeps its session, including when that seat carries +// no decision obligation. +func TestRemoveParticipant_SuppressesWhenAgentHoldsAnotherSeat(t *testing.T) { + deps, rt := newCapacityDeps(t, "task-two-seats") + ctx := context.Background() + + if err := deps.svc.AddTaskReviewer(ctx, "", "task-two-seats", "agent-both"); err != nil { + t.Fatalf("add reviewer: %v", err) + } + // A decision-free approver seat still addresses the agent on the task. + seedSeat(t, deps.db, "seat-approver", "step-task-two-seats", "task-two-seats", + "approver", "agent-both", 0) + + if got := removeReviewer(t, deps, rt, "task-two-seats", "agent-both"); got != 0 { + t.Errorf("agent still holds an approver seat: want 0 terminations, got %d", got) + } +} + +// AC-OFFICE-SESSION-TERM-001.4: losing the last capacity still ends the +// session, carrying the removal path's own reason. +func TestRemoveParticipant_TerminatesWhenNoCapacityRemains(t *testing.T) { + deps, rt := newCapacityDeps(t, "task-last") + ctx := context.Background() + + if err := deps.svc.AddTaskReviewer(ctx, "", "task-last", "agent-only"); err != nil { + t.Fatalf("add reviewer: %v", err) + } + // Someone else runs the task, so the removed agent retains nothing. + if _, err := deps.repo.UpdateTaskAssignee(ctx, "task-last", "agent-runner"); err != nil { + t.Fatalf("set runner: %v", err) + } + + if got := removeReviewer(t, deps, rt, "task-last", "agent-only"); got != 1 { + t.Fatalf("want 1 termination, got %d", got) + } + if call := rt.calls[len(rt.calls)-1]; call.reason != "participant_removed" { + t.Errorf("reason: got %q, want participant_removed", call.reason) + } +} + +// AC-OFFICE-SESSION-TERM-002.4: a template-level seat counts exactly as a +// per-task seat does. +func TestRemoveParticipant_SuppressesForTemplateLevelSeat(t *testing.T) { + deps, rt := newCapacityDeps(t, "task-template") + ctx := context.Background() + + if err := deps.svc.AddTaskReviewer(ctx, "", "task-template", "agent-tmpl"); err != nil { + t.Fatalf("add reviewer: %v", err) + } + seedSeat(t, deps.db, "seat-tmpl", "step-task-template", "", "approver", "agent-tmpl", 1) + + if got := removeReviewer(t, deps, rt, "task-template", "agent-tmpl"); got != 0 { + t.Errorf("template-level approver seat addresses the agent: want 0 terminations, got %d", got) + } +} + +// AC-OFFICE-SESSION-TERM-002.3: a seat recorded at a step the task has left +// must not, on its own, suppress a termination. Without the step bound the +// stale row would suppress every future termination for this pair. +func TestRemoveParticipant_StaleSeatAtLeftStepDoesNotSuppress(t *testing.T) { + deps, rt := newCapacityDeps(t, "task-moved") + ctx := context.Background() + + if err := deps.svc.AddTaskReviewer(ctx, "", "task-moved", "agent-stale"); err != nil { + t.Fatalf("add reviewer: %v", err) + } + if _, err := deps.repo.UpdateTaskAssignee(ctx, "task-moved", "agent-runner"); err != nil { + t.Fatalf("set runner: %v", err) + } + // The task advances. The reviewer seat written at the previous step + // stays readable, but no longer addresses the agent on this task. + moveTaskToStep(t, deps.db, "task-moved", "step-later") + + if got := removeReviewer(t, deps, rt, "task-moved", "agent-stale"); got != 1 { + t.Errorf("stale seat at a left step must not suppress: want 1 termination, got %d", got) + } +} + +// AC-OFFICE-SESSION-TERM-002.9: the determination compares profile +// identifiers and does not require the profile to resolve to a live agent. +func TestRemoveParticipant_SeatNamingUnresolvableProfileStillSuppresses(t *testing.T) { + deps, rt := newCapacityDeps(t, "task-ghost") + + seedSeat(t, deps.db, "seat-ghost-rev", "step-task-ghost", "task-ghost", + "reviewer", "agent-ghost", 1) + seedSeat(t, deps.db, "seat-ghost-app", "step-task-ghost", "task-ghost", + "approver", "agent-ghost", 1) + + if got := removeReviewer(t, deps, rt, "task-ghost", "agent-ghost"); got != 0 { + t.Errorf("seat naming an unresolvable profile still names this agent: want 0, got %d", got) + } +} + +// AC-OFFICE-SESSION-TERM-002.8 (first sentence): with no current step the +// effective slate is empty, so only the runner capacity can be retained. +func TestRemoveParticipant_NoCurrentStepLeavesOnlyRunnerCapacity(t *testing.T) { + deps, rt := newCapacityDeps(t, "task-nostep") + ctx := context.Background() + + if err := deps.svc.AddTaskReviewer(ctx, "", "task-nostep", "agent-seated"); err != nil { + t.Fatalf("add reviewer: %v", err) + } + if _, err := deps.repo.UpdateTaskAssignee(ctx, "task-nostep", "agent-runner"); err != nil { + t.Fatalf("set runner: %v", err) + } + moveTaskToStep(t, deps.db, "task-nostep", "") + + if got := removeReviewer(t, deps, rt, "task-nostep", "agent-seated"); got != 1 { + t.Errorf("empty slate retains no seat capacity: want 1 termination, got %d", got) + } + // The runner is still retained through the projection, which reads + // runner rows across steps. + if got := removeReviewer(t, deps, rt, "task-nostep", "agent-runner"); got != 0 { + t.Errorf("runner capacity survives an empty slate: want 0 terminations, got %d", got) + } +} + +// AC-OFFICE-SESSION-TERM-001.7, -002.8 (second sentence): a task that does not +// resolve is a failed determination, not an absence of capacity. The guarded +// path still reports success. +func TestRemoveParticipant_UnresolvableTaskFailsClosed(t *testing.T) { + deps, rt := newCapacityDeps(t, "task-present") + + if err := deps.svc.RemoveTaskReviewer(context.Background(), "", "task-absent", "agent-x"); err != nil { + t.Fatalf("removal must report success when the determination fails: %v", err) + } + if len(rt.calls) != 0 { + t.Errorf("failed determination must not terminate: got %d calls", len(rt.calls)) + } +} + +// AC-OFFICE-SESSION-TERM-001.8: neither identifier names a pair, so nothing is +// decided and nothing is ended. +func TestRemoveParticipant_EmptyIdentifiersReportSuccess(t *testing.T) { + deps, rt := newCapacityDeps(t, "task-empty") + + if err := deps.svc.RemoveTaskReviewer(context.Background(), "", "task-empty", ""); err != nil { + t.Fatalf("empty agent id must report success: %v", err) + } + if err := deps.svc.RemoveTaskReviewer(context.Background(), "", "", "agent-x"); err != nil { + t.Fatalf("empty task id must report success: %v", err) + } + if len(rt.calls) != 0 { + t.Errorf("empty identifiers must not terminate: got %d calls", len(rt.calls)) + } +} + +// AC-OFFICE-SESSION-TERM-002.12: a capacity granted between the guarded +// path's commit and the determination's read is observed, because the read +// asks about the present. The seat is seeded from inside +// SetSessionCapacityReadHook, which fires after RemoveTaskReviewer's own +// removal has committed and immediately before the determination reads +// capacity — the exact window the AC names — so this only passes for an +// implementation whose read genuinely happens after that window, not one +// that decided from state captured earlier. +func TestRemoveParticipant_CapacityRegainedBeforeReadSuppresses(t *testing.T) { + deps, rt := newCapacityDeps(t, "task-regained") + ctx := context.Background() + + if err := deps.svc.AddTaskReviewer(ctx, "", "task-regained", "agent-back"); err != nil { + t.Fatalf("add reviewer: %v", err) + } + if _, err := deps.repo.UpdateTaskAssignee(ctx, "task-regained", "agent-runner"); err != nil { + t.Fatalf("set runner: %v", err) + } + + restore := dashboard.SetSessionCapacityReadHook(func(taskID, agentProfileID string) { + if taskID == "task-regained" && agentProfileID == "agent-back" { + seedSeat(t, deps.db, "seat-back", "step-task-regained", "task-regained", + "approver", "agent-back", 1) + } + }) + t.Cleanup(restore) + + if got := removeReviewer(t, deps, rt, "task-regained", "agent-back"); got != 0 { + t.Errorf("re-seated agent is addressed on the task: want 0 terminations, got %d", got) + } +} diff --git a/apps/backend/internal/office/dashboard/session_termination_metrics.go b/apps/backend/internal/office/dashboard/session_termination_metrics.go new file mode 100644 index 0000000000..6454e1b79e --- /dev/null +++ b/apps/backend/internal/office/dashboard/session_termination_metrics.go @@ -0,0 +1,46 @@ +package dashboard + +import ( + "expvar" + "strings" +) + +// officeSessionTermSuppressedTotal counts guarded terminations that did not +// end a session, exposed via stdlib's /debug/vars handler. The label model is +// "key=value;key=value..." so a Prometheus translation layer can split on `;` +// and `=` later, mirroring internal/office/scheduler/metrics_vars.go. +// +// A suppression leaves a session live where one used to end. Logged alone it +// is indistinguishable from a path that never ran, so a mistaken +// retained-capacity answer would look exactly like a quiet system. +// +// Counters only. Both dimensions are closed sets - the guarded reason is one +// of three and the outcome is one of three - so no task, agent, step or +// session identifier is ever a label. +var officeSessionTermSuppressedTotal = expvar.NewMap("office_session_term_suppressed_total") + +// sessionTermSuppressReadFailed is the outcome label for a suppression caused +// by a capacity read that failed, as opposed to one caused by a capacity the +// agent genuinely still holds. A read fault and a retained capacity are +// different events and must stay countable apart. +const sessionTermSuppressReadFailed = "read_failed" + +// officeSessionTermLabel builds a "k1=v1;k2=v2;..." expvar map key. Returns an +// empty label for an odd number of arguments rather than guessing. +func officeSessionTermLabel(pairs ...string) string { + if len(pairs)%2 != 0 { + return "" + } + parts := make([]string, 0, len(pairs)/2) + for i := 0; i < len(pairs); i += 2 { + parts = append(parts, pairs[i]+"="+pairs[i+1]) + } + return strings.Join(parts, ";") +} + +// recordSessionTermSuppressed counts one suppressed termination. outcome is +// either the retained capacity or sessionTermSuppressReadFailed. +func recordSessionTermSuppressed(reason, outcome string) { + officeSessionTermSuppressedTotal.Add( + officeSessionTermLabel("reason", reason, "outcome", outcome), 1) +} diff --git a/apps/backend/internal/office/dashboard/session_termination_metrics_test.go b/apps/backend/internal/office/dashboard/session_termination_metrics_test.go new file mode 100644 index 0000000000..ecb480734a --- /dev/null +++ b/apps/backend/internal/office/dashboard/session_termination_metrics_test.go @@ -0,0 +1,119 @@ +package dashboard_test + +import ( + "context" + "expvar" + "strings" + "testing" +) + +// suppressionCounts snapshots the expvar map as label -> count. +func suppressionCounts(t *testing.T) map[string]int64 { + t.Helper() + out := map[string]int64{} + v := expvar.Get("office_session_term_suppressed_total") + if v == nil { + t.Fatal("office_session_term_suppressed_total is not registered") + } + m, ok := v.(*expvar.Map) + if !ok { + t.Fatalf("office_session_term_suppressed_total is %T, want *expvar.Map", v) + } + m.Do(func(kv expvar.KeyValue) { + iv, ok := kv.Value.(*expvar.Int) + if !ok { + t.Errorf("label %q holds %T, want *expvar.Int", kv.Key, kv.Value) + return + } + out[kv.Key] = iv.Value() + }) + return out +} + +func delta(before, after map[string]int64, label string) int64 { + return after[label] - before[label] +} + +// AC-OFFICE-SESSION-TERM-004.3: every suppression is counted, and the two +// suppression causes are counted apart. +func TestSuppressedTermination_IsCounted(t *testing.T) { + deps, rt := newCapacityDeps(t, "task-metric") + ctx := context.Background() + + if err := deps.svc.AddTaskReviewer(ctx, "", "task-metric", "agent-dual"); err != nil { + t.Fatalf("add reviewer: %v", err) + } + if _, err := deps.repo.UpdateTaskAssignee(ctx, "task-metric", "agent-dual"); err != nil { + t.Fatalf("set runner: %v", err) + } + + before := suppressionCounts(t) + if got := removeReviewer(t, deps, rt, "task-metric", "agent-dual"); got != 0 { + t.Fatalf("expected a suppression, got %d terminations", got) + } + // A task that does not resolve fails the determination instead. + if err := deps.svc.RemoveTaskReviewer(ctx, "", "task-gone", "agent-dual"); err != nil { + t.Fatalf("remove on unresolvable task: %v", err) + } + after := suppressionCounts(t) + + retained := "reason=participant_removed;outcome=runner" + failed := "reason=participant_removed;outcome=read_failed" + if got := delta(before, after, retained); got != 1 { + t.Errorf("retained-capacity suppressions: got %d, want 1", got) + } + if got := delta(before, after, failed); got != 1 { + t.Errorf("read-failure suppressions: got %d, want 1", got) + } +} + +// AC-OFFICE-SESSION-TERM-004.3 (second sentence): labels come from closed sets. +// An identifier reaching a label would grow the map without bound, so this +// scans every label the whole package's tests produced. +func TestSuppressionLabels_AreBounded(t *testing.T) { + deps, rt := newCapacityDeps(t, "task-labels") + ctx := context.Background() + + if err := deps.svc.AddTaskReviewer(ctx, "", "task-labels", "agent-bounded"); err != nil { + t.Fatalf("add reviewer: %v", err) + } + if _, err := deps.repo.UpdateTaskAssignee(ctx, "task-labels", "agent-bounded"); err != nil { + t.Fatalf("set runner: %v", err) + } + if got := removeReviewer(t, deps, rt, "task-labels", "agent-bounded"); got != 0 { + t.Fatalf("expected a suppression, got %d terminations", got) + } + + reasons := map[string]bool{ + "task_reassigned": true, "participant_removed": true, "participant_seat_claimed": true, + } + outcomes := map[string]bool{"runner": true, "seat": true, "read_failed": true} + + labels := suppressionCounts(t) + if len(labels) == 0 { + t.Fatal("no labels recorded: the scan below would pass without inspecting anything") + } + for label := range labels { + if strings.Contains(label, "task-labels") || strings.Contains(label, "agent-bounded") { + t.Errorf("label %q carries an identifier", label) + } + fields := map[string]string{} + for _, pair := range strings.Split(label, ";") { + k, v, ok := strings.Cut(pair, "=") + if !ok { + t.Errorf("label %q is not a k=v;k=v string", label) + continue + } + fields[k] = v + } + if len(fields) != 2 { + t.Errorf("label %q: got %d dimensions, want 2", label, len(fields)) + } + if !reasons[fields["reason"]] { + t.Errorf("label %q: reason %q is outside the closed set", label, fields["reason"]) + } + if !outcomes[fields["outcome"]] { + t.Errorf("label %q: outcome %q is outside the closed set", label, fields["outcome"]) + } + } +} diff --git a/apps/backend/internal/office/dashboard/session_termination_test.go b/apps/backend/internal/office/dashboard/session_termination_test.go index 5679188310..cd44fcb6d3 100644 --- a/apps/backend/internal/office/dashboard/session_termination_test.go +++ b/apps/backend/internal/office/dashboard/session_termination_test.go @@ -2,7 +2,12 @@ package dashboard_test import ( "context" + "net/http" + "net/http/httptest" + "strings" + "sync" "testing" + "time" "github.com/kandev/kandev/internal/office/dashboard" ) @@ -154,3 +159,180 @@ func TestSetTaskAssignee_SameAgent_DoesNotTerminateSession(t *testing.T) { t.Fatalf("same-agent reassignment must not dismiss its failure inbox entry, got %+v", fn.calls) } } + +// countingCanceller records hard-cancel calls so the reassignment path's two +// halves can be asserted independently. +type countingCanceller struct { + mu sync.Mutex + called []string + done chan struct{} +} + +// newCountingCanceller returns a countingCanceller ready to signal waitForCancel. +func newCountingCanceller() *countingCanceller { + return &countingCanceller{done: make(chan struct{}, 1)} +} + +func (c *countingCanceller) CancelTaskExecution(_ context.Context, taskID, _ string, _ bool) error { + c.mu.Lock() + c.called = append(c.called, taskID) + c.mu.Unlock() + select { + case c.done <- struct{}{}: + default: + } + return nil +} + +// waitForCancel joins the hard cancel, which the reassignment path dispatches +// on its own goroutine, via the done channel rather than a sleep-poll loop. +func (c *countingCanceller) waitForCancel(t *testing.T) int { + t.Helper() + select { + case <-c.done: + case <-time.After(time.Second): + t.Log("timed out waiting for cancel") + } + c.mu.Lock() + defer c.mu.Unlock() + return len(c.called) +} + +// seedSelfReviewTask builds the shipped fallback shape: one agent is both the +// task's runner and a seated reviewer on it. +func seedSelfReviewTask(t *testing.T, deps *testDeps, taskID, agentID string) { + t.Helper() + ctx := context.Background() + insertTestTask(t, deps.db, taskID, "ws-self", "Self review", "todo", 2) + if err := deps.svc.AddTaskReviewer(ctx, "", taskID, agentID); err != nil { + t.Fatalf("seat %s as reviewer: %v", agentID, err) + } + if _, err := deps.repo.UpdateTaskAssignee(ctx, taskID, agentID); err != nil { + t.Fatalf("make %s the runner: %v", agentID, err) + } +} + +// TestSetTaskAssignee_KeepsSessionOfPrevRunnerStillSeated is +// AC-OFFICE-SESSION-TERM-003.2. Reassignment removes the runner capacity and +// leaves the reviewer seat, so the running execution must still be cancelled +// while the session row is left alone. Asserting only the session row would +// pass for an implementation that wrongly suppressed both. +func TestSetTaskAssignee_KeepsSessionOfPrevRunnerStillSeated(t *testing.T) { + deps := newTestDeps(t) + rt := &recordingTerminator{} + cancels := newCountingCanceller() + deps.svc.SetSessionTerminator(rt) + deps.svc.SetTaskCanceller(cancels) + deps.svc.SetReactivityApplier(&recordingReactivity{ + result: &dashboard.TaskReactivityResult{InterruptSessionID: "task-self"}, + }) + + seedSelfReviewTask(t, deps, "task-self", "agent-dual") + + if err := deps.svc.SetTaskAssigneeAsAgent(context.Background(), "", "task-self", "agent-new"); err != nil { + t.Fatalf("reassign: %v", err) + } + + if len(rt.calls) != 0 { + t.Errorf("prev runner is still a seated reviewer: want 0 terminations, got %d (%+v)", + len(rt.calls), rt.calls) + } + if got := cancels.waitForCancel(t); got != 1 { + t.Errorf("the execution it no longer runs must still be cancelled: got %d cancels", got) + } +} + +// TestSetTaskAssignee_TerminatesWhenPrevRunnerKeepsNothing keeps the other +// half of AC-OFFICE-SESSION-TERM-001.4 honest: with no seat left behind, the +// reassignment path still ends the session. +func TestSetTaskAssignee_TerminatesWhenPrevRunnerKeepsNothing(t *testing.T) { + deps := newTestDeps(t) + rt := &recordingTerminator{} + deps.svc.SetSessionTerminator(rt) + deps.svc.SetReactivityApplier(&recordingReactivity{result: &dashboard.TaskReactivityResult{}}) + + insertTestTask(t, deps.db, "task-plain", "ws-self", "Plain", "todo", 2) + if _, err := deps.repo.UpdateTaskAssignee(context.Background(), "task-plain", "agent-prev"); err != nil { + t.Fatalf("seed runner: %v", err) + } + + if err := deps.svc.SetTaskAssigneeAsAgent(context.Background(), "", "task-plain", "agent-new"); err != nil { + t.Fatalf("reassign: %v", err) + } + if len(rt.calls) != 1 { + t.Fatalf("want 1 termination, got %d (%+v)", len(rt.calls), rt.calls) + } + if rt.calls[0].reason != "task_reassigned" { + t.Errorf("reason: got %q, want task_reassigned", rt.calls[0].reason) + } +} + +// TestAddTaskReviewer_ClaimKeepsSessionOfDisplacedRunner is the claim path's +// half of AC-OFFICE-SESSION-TERM-001.6: displacing the auto-cast reviewer +// seat from the agent that also runs the task must not stop the task. +func TestAddTaskReviewer_ClaimKeepsSessionOfDisplacedRunner(t *testing.T) { + deps := newTestDeps(t) + rt := &recordingTerminator{} + deps.svc.SetSessionTerminator(rt) + insertTestTask(t, deps.db, "claim-dual", "ws-claim-dual", "C", "todo", 2) + + if _, err := deps.db.Exec(` + INSERT INTO agent_profiles (id, agent_id, name, agent_display_name, workspace_id, role, created_at, updated_at) + VALUES ('agent-claiming', '', 'agent-claiming', 'agent-claiming', 'ws-claim-dual', '', datetime('now'), datetime('now')) + `); err != nil { + t.Fatalf("seed claiming agent profile: %v", err) + } + // The auto-cast reviewer is the task's runner: the shipped fallback when + // no candidate is neither runner nor already seated. + if _, err := deps.db.Exec(` + INSERT INTO workflow_step_participants + (id, step_id, task_id, role, agent_profile_id, decision_required, position, provenance) + VALUES ('auto-seat-claim-dual', 'step-claim-dual', 'claim-dual', 'reviewer', 'agent-auto', 1, 0, 'auto') + `); err != nil { + t.Fatalf("seed auto seat: %v", err) + } + if _, err := deps.repo.UpdateTaskAssignee(context.Background(), "claim-dual", "agent-auto"); err != nil { + t.Fatalf("make agent-auto the runner: %v", err) + } + + body := `{"agent_profile_id":"agent-claiming"}` + req := httptest.NewRequest(http.MethodPost, + "/api/v1/office/tasks/claim-dual/reviewers", strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + deps.router.ServeHTTP(w, req) + + if w.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201: %s", w.Code, w.Body.String()) + } + if len(rt.calls) != 0 { + t.Errorf("displaced agent still runs the task: want 0 terminations, got %d (%+v)", + len(rt.calls), rt.calls) + } +} + +// TestSelfReviewSessionSurvivesRemoval_BothIdentityFlagStates is +// AC-OFFICE-SESSION-TERM-003.5. The defect is reachable in the shipped +// configuration (flag off), where a task with a runner routes every office run +// through the runner's session, so the flag must not change the outcome. +func TestSelfReviewSessionSurvivesRemoval_BothIdentityFlagStates(t *testing.T) { + for _, enabled := range []bool{false, true} { + t.Run(map[bool]string{false: "identity_off", true: "identity_on"}[enabled], func(t *testing.T) { + deps := newTestDeps(t) + rt := &recordingTerminator{} + deps.svc.SetSessionTerminator(rt) + deps.svc.SetOfficeSessionIdentity(enabled) + + seedSelfReviewTask(t, deps, "task-flag", "agent-dual") + + if err := deps.svc.RemoveTaskReviewer( + context.Background(), "", "task-flag", "agent-dual"); err != nil { + t.Fatalf("remove reviewer: %v", err) + } + if len(rt.calls) != 0 { + t.Errorf("runner keeps its session regardless of the flag: got %d terminations", + len(rt.calls)) + } + }) + } +} diff --git a/apps/backend/internal/office/repository/sqlite/export_test.go b/apps/backend/internal/office/repository/sqlite/export_test.go new file mode 100644 index 0000000000..ffe9d9c65e --- /dev/null +++ b/apps/backend/internal/office/repository/sqlite/export_test.go @@ -0,0 +1,17 @@ +package sqlite + +import ( + "context" + + "github.com/jmoiron/sqlx" +) + +// SetClaimWindowHook installs a yield point between the registration's +// claimable-seat selection and its reassigning update, and returns a function +// restoring the previous value. This file is compiled only under `go test`, so +// the hook has no setter at all in a production build. +func SetClaimWindowHook(fn func(ctx context.Context, tx *sqlx.Tx, seatID string)) func() { + prev := claimWindowHook + claimWindowHook = fn + return func() { claimWindowHook = prev } +} diff --git a/apps/backend/internal/office/repository/sqlite/participant_claim_decision_guard_postgres_test.go b/apps/backend/internal/office/repository/sqlite/participant_claim_decision_guard_postgres_test.go new file mode 100644 index 0000000000..b969d06384 --- /dev/null +++ b/apps/backend/internal/office/repository/sqlite/participant_claim_decision_guard_postgres_test.go @@ -0,0 +1,134 @@ +package sqlite_test + +import ( + "context" + "testing" + "time" + + "github.com/jmoiron/sqlx" + + settingsstore "github.com/kandev/kandev/internal/agent/settings/store" + "github.com/kandev/kandev/internal/office/repository/sqlite" + taskrepo "github.com/kandev/kandev/internal/task/repository/sqlite" + "github.com/kandev/kandev/internal/testutil" + "github.com/kandev/kandev/internal/workflow/models" + workflowrepo "github.com/kandev/kandev/internal/workflow/repository" +) + +// TestPostgresAddTaskParticipant_ClaimDecisionGuard_RolelessDecisionCommitsInWindow +// is the cross-transaction reading of AC-OFFICE-SEAT-GUARD-002.2: the decision +// genuinely COMMITS from another connection inside the registration's window, +// rather than being written through the registration's own transaction as the +// engine-agnostic test does. +// +// It cannot deadlock. A roleless decision skips ParticipantRoleSeatLockKey - +// which the registration holds for its whole transaction - and takes only +// decisionLockNamespace, which the registration never touches. That skip is +// exactly why this window is reachable at all. +// +// The embedded engine cannot host this test: the office sqlite suite pins the +// pool to one connection, and SQLite serializes writers regardless, so a second +// connection would block behind the open registration transaction rather than +// commit inside it. +// +// Skips unless KANDEV_TEST_POSTGRES_DSN is set. +func TestPostgresAddTaskParticipant_ClaimDecisionGuard_RolelessDecisionCommitsInWindow(t *testing.T) { + dsn := testutil.PostgresDSNFromEnv(t) + db := openIsolatedPostgresMultiConnForClaimRace(t, dsn, 4) + ctx := context.Background() + + if _, _, err := settingsstore.Provide(db, db, nil); err != nil { + t.Fatalf("init settings store: %v", err) + } + if _, err := taskrepo.NewWithDB(db, db, nil); err != nil { + t.Fatalf("init task repo: %v", err) + } + workflowRepo, err := workflowrepo.NewWithDB(db, db, nil) + if err != nil { + t.Fatalf("init workflow repo: %v", err) + } + officeRepo, err := sqlite.NewWithDB(db, db, nil) + if err != nil { + t.Fatalf("init office repo: %v", err) + } + + now := time.Now().UTC() + if _, err := db.ExecContext(ctx, db.Rebind(` + INSERT INTO workflows (id, workspace_id, name, created_at, updated_at) + VALUES ('wf-guard-window', '', 'Guard Window', ?, ?) + `), now, now); err != nil { + t.Fatalf("seed workflow: %v", err) + } + step := &models.WorkflowStep{ + WorkflowID: "wf-guard-window", Name: "Review", Position: 0, StageType: models.StageTypeReview, + } + if err := workflowRepo.CreateStep(ctx, step); err != nil { + t.Fatalf("create step: %v", err) + } + for _, agentID := range []string{"agent-auto", "agent-registering"} { + if _, err := db.ExecContext(ctx, db.Rebind(` + INSERT INTO agents (id, name, created_at, updated_at) VALUES (?, ?, ?, ?) + `), agentID, agentID, now, now); err != nil { + t.Fatalf("seed agent %s: %v", agentID, err) + } + if _, err := db.ExecContext(ctx, db.Rebind(` + INSERT INTO agent_profiles (id, agent_id, name, agent_display_name, workspace_id, role, created_at, updated_at) + VALUES (?, ?, ?, ?, '', '', ?, ?) + `), agentID, agentID, agentID, agentID, now, now); err != nil { + t.Fatalf("seed agent profile %s: %v", agentID, err) + } + } + if _, err := db.ExecContext(ctx, db.Rebind(` + INSERT INTO tasks (id, workflow_step_id, title, created_at, updated_at) + VALUES ('task-guard-window', ?, 'guard', ?, ?) + `), step.ID, now, now); err != nil { + t.Fatalf("seed task: %v", err) + } + if _, err := db.ExecContext(ctx, db.Rebind(` + INSERT INTO workflow_step_participants + (id, step_id, task_id, role, agent_profile_id, decision_required, position, provenance) + VALUES ('seat-guard-window', ?, 'task-guard-window', 'reviewer', 'agent-auto', 1, 0, 'auto') + `), step.ID); err != nil { + t.Fatalf("seed auto seat: %v", err) + } + + // The hook runs inside the registration's open transaction; this commits + // on a different pooled connection, so the decision is durable before the + // reassigning UPDATE evaluates its condition. + var hookErr error + restore := sqlite.SetClaimWindowHook(func(hookCtx context.Context, _ *sqlx.Tx, seatID string) { + if seatID != "seat-guard-window" { + return + } + hookErr = workflowRepo.RecordStepDecision(hookCtx, &models.WorkflowStepDecision{ + TaskID: "task-guard-window", StepID: step.ID, ParticipantID: seatID, + Decision: "approved", DeciderID: "agent-auto", + }) + }) + defer restore() + + result, err := officeRepo.AddTaskParticipant(ctx, "task-guard-window", "agent-registering", "reviewer") + if err != nil { + t.Fatalf("AddTaskParticipant: %v", err) + } + if hookErr != nil { + t.Fatalf("roleless decision must commit inside the window: %v", hookErr) + } + + if result.Outcome != sqlite.ParticipantWriteOutcomeInserted { + t.Errorf("outcome = %v, want Inserted", result.Outcome) + } + if result.DisplacedAgentProfileID != "" { + t.Errorf("displaced = %q, want empty", result.DisplacedAgentProfileID) + } + + var seatAgent string + if err := db.GetContext(ctx, &seatAgent, db.Rebind( + `SELECT agent_profile_id FROM workflow_step_participants WHERE id = 'seat-guard-window'`, + )); err != nil { + t.Fatalf("read seat: %v", err) + } + if seatAgent != "agent-auto" { + t.Errorf("seat agent = %q, want agent-auto: the decision was reattributed", seatAgent) + } +} diff --git a/apps/backend/internal/office/repository/sqlite/participant_claim_decision_guard_test.go b/apps/backend/internal/office/repository/sqlite/participant_claim_decision_guard_test.go new file mode 100644 index 0000000000..268560c821 --- /dev/null +++ b/apps/backend/internal/office/repository/sqlite/participant_claim_decision_guard_test.go @@ -0,0 +1,250 @@ +package sqlite_test + +import ( + "context" + "testing" + + "github.com/jmoiron/sqlx" + + "github.com/kandev/kandev/internal/office/repository/sqlite" +) + +// claimAutoSeat's UPDATE carries a NOT EXISTS condition over decisions against +// the seat it is reassigning. That condition can only add something the +// selection did not already provide when a decision commits between the two +// statements, and the seat exclusion closes that window for every decision +// carrying a role. A roleless decision takes neither the exclusion nor the +// seat validation, so for that path the condition is the whole defense. +// +// Racing real goroutines there would reach the window but is not forced to, +// so such a test would pass most of the time by never entering it. These +// tests use the package's yield point instead. + +// seedRolelessDecision writes a decision against seatID carrying no +// participant role, through the supplied transaction. +func seedRolelessDecision(t *testing.T, ctx context.Context, tx *sqlx.Tx, id, taskID, stepID, seatID string) { + t.Helper() + if _, err := tx.ExecContext(ctx, tx.Rebind(` + INSERT INTO workflow_step_decisions + (id, task_id, step_id, participant_id, decision, decided_at) + VALUES (?, ?, ?, ?, 'approve', '2026-01-01 00:00:00') + `), id, taskID, stepID, seatID); err != nil { + t.Fatalf("seed roleless decision: %v", err) + } +} + +type seatRow struct { + AgentProfileID string `db:"agent_profile_id"` + Provenance string `db:"provenance"` + DecisionRequired int `db:"decision_required"` + Position int `db:"position"` + CreatedAt string `db:"created_at"` +} + +type decisionRow struct { + ID string `db:"id"` + TaskID string `db:"task_id"` + StepID string `db:"step_id"` + ParticipantID string `db:"participant_id"` + Decision string `db:"decision"` + DecidedAt string `db:"decided_at"` + SupersededAt string `db:"superseded_at"` +} + +func readSeat(t *testing.T, repo *sqlite.Repository, seatID string) seatRow { + t.Helper() + var got seatRow + if err := repo.ReaderDB().Get(&got, ` + SELECT agent_profile_id, provenance, decision_required, position, + COALESCE(created_at, '') AS created_at + FROM workflow_step_participants WHERE id = ? + `, seatID); err != nil { + t.Fatalf("read seat %s: %v", seatID, err) + } + return got +} + +func readDecision(t *testing.T, repo *sqlite.Repository, decisionID string) decisionRow { + t.Helper() + var got decisionRow + if err := repo.ReaderDB().Get(&got, ` + SELECT id, task_id, step_id, participant_id, decision, + COALESCE(decided_at, '') AS decided_at, + COALESCE(superseded_at, '') AS superseded_at + FROM workflow_step_decisions WHERE id = ? + `, decisionID); err != nil { + t.Fatalf("read decision %s: %v", decisionID, err) + } + return got +} + +// TestAddTaskParticipant_ClaimDecisionGuard_RolelessDecisionBlocksReassignment +// is AC-OFFICE-SEAT-GUARD-001.2 and -002.1: a decision that exists against the +// selected seat by the time the reassigning update runs must stop the claim, +// leaving the registration to write its own seat instead. +// +// Removing the NOT EXISTS condition from claimAutoSeat must make this fail. +func TestAddTaskParticipant_ClaimDecisionGuard_RolelessDecisionBlocksReassignment(t *testing.T) { + repo := newSearchTestRepo(t) + ctx := context.Background() + + seedParticipantTask(t, repo, "guard-task", "guard-step") + seedAutoSeat(t, repo, "guard-seat", "guard-step", "guard-task", "reviewer", "agent-auto") + seedParticipantAgent(t, repo, "agent-registering") + + before := readSeat(t, repo, "guard-seat") + + restore := sqlite.SetClaimWindowHook(func(hookCtx context.Context, tx *sqlx.Tx, seatID string) { + if seatID != "guard-seat" { + return + } + seedRolelessDecision(t, hookCtx, tx, "guard-decision", "guard-task", "guard-step", seatID) + }) + defer restore() + + result, err := repo.AddTaskParticipant(ctx, "guard-task", "agent-registering", "reviewer") + if err != nil { + t.Fatalf("AddTaskParticipant must report success (AC-OFFICE-SEAT-GUARD-001.6): %v", err) + } + + // AC-OFFICE-SEAT-GUARD-001.5: indistinguishable from finding no claimable + // seat. Nothing was displaced, so nothing is reported as displaced. + if result.Outcome != sqlite.ParticipantWriteOutcomeInserted { + t.Errorf("outcome = %v, want Inserted", result.Outcome) + } + if result.DisplacedAgentProfileID != "" { + t.Errorf("displaced agent = %q, want empty", result.DisplacedAgentProfileID) + } + + // AC-OFFICE-SEAT-GUARD-001.4: the decided seat is untouched in every field. + after := readSeat(t, repo, "guard-seat") + if after != before { + t.Errorf("decided seat mutated:\n got %+v\nwant %+v", after, before) + } + decision := readDecision(t, repo, "guard-decision") + wantDecision := decisionRow{ + ID: "guard-decision", TaskID: "guard-task", StepID: "guard-step", + ParticipantID: "guard-seat", Decision: "approve", + DecidedAt: "2026-01-01 00:00:00", + } + if decision != wantDecision { + t.Errorf("guard decision mutated:\n got %+v\nwant %+v", decision, wantDecision) + } + + // AC-OFFICE-SEAT-GUARD-001.2: the registering agent still gets a seat. + seats, err := repo.ListTaskParticipants(ctx, "guard-task", "reviewer") + if err != nil { + t.Fatalf("ListTaskParticipants: %v", err) + } + if len(seats) != 2 { + t.Fatalf("reviewer seats = %d, want 2 (the decided one plus a fresh one)", len(seats)) + } + found := false + for _, s := range seats { + if s.AgentProfileID == "agent-registering" { + found = true + } + } + if !found { + t.Error("the registering agent was left unseated") + } +} + +// AC-OFFICE-SEAT-GUARD-001.8: a superseded decision blocks the reassignment +// exactly as an active one does. Superseded rows are the audit trail of a +// reworked review, and a seat that has ever been decided carries an +// attribution history that reassigning it would falsify. +func TestAddTaskParticipant_ClaimDecisionGuard_SupersededDecisionAlsoBlocks(t *testing.T) { + repo := newSearchTestRepo(t) + ctx := context.Background() + + seedParticipantTask(t, repo, "sup-task", "sup-step") + seedAutoSeat(t, repo, "sup-seat", "sup-step", "sup-task", "reviewer", "agent-auto") + seedParticipantAgent(t, repo, "agent-registering") + + restore := sqlite.SetClaimWindowHook(func(hookCtx context.Context, tx *sqlx.Tx, seatID string) { + if seatID != "sup-seat" { + return + } + if _, err := tx.ExecContext(hookCtx, tx.Rebind(` + INSERT INTO workflow_step_decisions + (id, task_id, step_id, participant_id, decision, decided_at, superseded_at) + VALUES (?, ?, ?, ?, 'approve', '2026-01-01 00:00:00', '2026-01-02 00:00:00') + `), "sup-decision", "sup-task", "sup-step", seatID); err != nil { + t.Fatalf("seed superseded decision: %v", err) + } + }) + defer restore() + + result, err := repo.AddTaskParticipant(ctx, "sup-task", "agent-registering", "reviewer") + if err != nil { + t.Fatalf("AddTaskParticipant: %v", err) + } + if result.Outcome != sqlite.ParticipantWriteOutcomeInserted { + t.Errorf("outcome = %v, want Inserted: a superseded decision must block too", result.Outcome) + } + if got := readSeat(t, repo, "sup-seat").AgentProfileID; got != "agent-auto" { + t.Errorf("seat agent = %q, want agent-auto", got) + } +} + +// AC-OFFICE-SEAT-GUARD-001.9: the registering agent is seated by the seat it +// wrote, so repeating the registration is governed by the already-seated rule +// and writes nothing further. No retry and no second claim attempt. +func TestAddTaskParticipant_ClaimDecisionGuard_RepeatRegistrationWritesNothing(t *testing.T) { + repo := newSearchTestRepo(t) + ctx := context.Background() + + seedParticipantTask(t, repo, "rep-task", "rep-step") + seedAutoSeat(t, repo, "rep-seat", "rep-step", "rep-task", "reviewer", "agent-auto") + seedParticipantAgent(t, repo, "agent-registering") + + restore := sqlite.SetClaimWindowHook(func(hookCtx context.Context, tx *sqlx.Tx, seatID string) { + if seatID != "rep-seat" { + return + } + seedRolelessDecision(t, hookCtx, tx, "rep-decision", "rep-task", "rep-step", seatID) + }) + t.Cleanup(restore) + if _, err := repo.AddTaskParticipant(ctx, "rep-task", "agent-registering", "reviewer"); err != nil { + t.Fatalf("first registration: %v", err) + } + restore() + + countAfterFirst := participantRowCount(t, repo, "rep-task") + result, err := repo.AddTaskParticipant(ctx, "rep-task", "agent-registering", "reviewer") + if err != nil { + t.Fatalf("repeat registration: %v", err) + } + if result.Outcome != sqlite.ParticipantWriteOutcomeUnchanged { + t.Errorf("outcome = %v, want Unchanged", result.Outcome) + } + if got := participantRowCount(t, repo, "rep-task"); got != countAfterFirst { + t.Errorf("rows = %d, want %d: the repeat must write nothing", got, countAfterFirst) + } +} + +// AC-OFFICE-SEAT-GUARD-002.3: with the hook unset the claim path behaves +// exactly as it does today, so an undecided seat is still claimed in place. +func TestAddTaskParticipant_ClaimDecisionGuard_HookUnsetLeavesClaimUnchanged(t *testing.T) { + repo := newSearchTestRepo(t) + ctx := context.Background() + + seedParticipantTask(t, repo, "unset-task", "unset-step") + seedAutoSeat(t, repo, "unset-seat", "unset-step", "unset-task", "reviewer", "agent-auto") + seedParticipantAgent(t, repo, "agent-registering") + + result, err := repo.AddTaskParticipant(ctx, "unset-task", "agent-registering", "reviewer") + if err != nil { + t.Fatalf("AddTaskParticipant: %v", err) + } + if result.Outcome != sqlite.ParticipantWriteOutcomeClaimed { + t.Fatalf("outcome = %v, want Claimed", result.Outcome) + } + if result.DisplacedAgentProfileID != "agent-auto" { + t.Errorf("displaced = %q, want agent-auto", result.DisplacedAgentProfileID) + } + if n := participantRowCount(t, repo, "unset-task"); n != 1 { + t.Errorf("rows = %d, want 1 (claimed in place)", n) + } +} diff --git a/apps/backend/internal/office/repository/sqlite/participant_claim_postgres_test.go b/apps/backend/internal/office/repository/sqlite/participant_claim_postgres_test.go index ddacef6a03..27e14d732b 100644 --- a/apps/backend/internal/office/repository/sqlite/participant_claim_postgres_test.go +++ b/apps/backend/internal/office/repository/sqlite/participant_claim_postgres_test.go @@ -74,24 +74,29 @@ func openIsolatedPostgresMultiConnForClaimRace(t *testing.T, dsn string, maxConn return db } -// TestPostgresAddTaskParticipant_ClaimDoesNotOverwriteAConcurrentDecision is -// SR21's proof obligation: RecordStepDecision locks on decisionLockNamespace -// while EnsureRoleSeat/AddTaskParticipant lock on participantsLockNamespace — -// two different advisory-lock namespaces that never contend with each other -// on Postgres. Before this fix, AddTaskParticipant's claim UPDATE had no -// decision guard, so a decision committing between the "is this seat -// undecided" read and the claim's UPDATE could be silently reattributed: -// the seat's agent_profile_id would end up naming the claiming agent even -// though the decision on file was recorded by the original auto-cast agent. -// The fix makes the claim's UPDATE conditional on -// `NOT EXISTS (SELECT 1 FROM workflow_step_decisions WHERE participant_id = ?)` -// and falls through to inserting a fresh seat when that affects zero rows -// (see claimAutoSeat in participants.go). +// TestPostgresAddTaskParticipant_ClaimDoesNotOverwriteAConcurrentDecision +// establishes that a role-carrying RecordStepDecision and a concurrent +// AddTaskParticipant serialize on the seat exclusion they share: both acquire +// ParticipantRoleSeatLockKey for the same (task, role) and hold it for the +// whole transaction. It asserts that the pair completes in either acquisition +// order without deadlocking — which is a real question, because the decision +// path takes decisionLockNamespace first and then the seat lock, while the +// registration path takes only the seat lock — and that whichever order the +// two land in, no decision is left attributed to an agent that did not record +// it. // -// This only proves anything under genuine cross-connection concurrency: -// SQLite's SetMaxOpenConns(1) serializes every transaction for free and -// would pass whether or not the guard exists. Skips unless -// KANDEV_TEST_POSTGRES_DSN is set. +// It does NOT establish that claimAutoSeat's NOT EXISTS condition is +// load-bearing, and cannot: the shared exclusion is what prevents a +// role-carrying decision from committing between findClaimableAutoSeat and +// claimAutoSeat, so this test would very likely pass with that condition +// deleted. The condition still matters, on the path this test does not reach: +// recordStepDecisionTx skips the seat exclusion when the decision carries no +// role. That window is covered deterministically by +// participant_claim_decision_guard_test.go. +// +// The serialization this test exercises is only observable under genuine +// cross-connection concurrency: SQLite's SetMaxOpenConns(1) serializes every +// transaction for free. Skips unless KANDEV_TEST_POSTGRES_DSN is set. func TestPostgresAddTaskParticipant_ClaimDoesNotOverwriteAConcurrentDecision(t *testing.T) { const iterations = 15 dsn := testutil.PostgresDSNFromEnv(t) diff --git a/apps/backend/internal/office/repository/sqlite/participants.go b/apps/backend/internal/office/repository/sqlite/participants.go index de14e8e1b1..8fec5c0876 100644 --- a/apps/backend/internal/office/repository/sqlite/participants.go +++ b/apps/backend/internal/office/repository/sqlite/participants.go @@ -227,11 +227,18 @@ func (r *Repository) AddTaskParticipant(ctx context.Context, taskID, agentID, ro } // No claim landed — either no claimable auto seat existed, or the // selected one was removed, reprovenanced, or decided since - // findClaimableAutoSeat's read (claimAutoSeat's guard is a defensive - // backstop: recordStepDecisionTx now shares this transaction's - // ParticipantRoleSeatLockKey exclusion, so it cannot actually interleave - // here). Either way, fall through to inserting a fresh seat rather than - // completing having written nothing. + // findClaimableAutoSeat's read. Either way, fall through to inserting a + // fresh seat rather than completing having written nothing. + // + // Which of the two defenses covers the decided case depends on the + // decision: recordStepDecisionTx acquires this transaction's + // ParticipantRoleSeatLockKey exclusion only when the decision carries a + // role, so a role-carrying decision cannot commit between + // findClaimableAutoSeat and claimAutoSeat. A roleless decision takes + // neither that exclusion nor the seat validation, and for it claimAutoSeat's + // NOT EXISTS condition is the whole defense — see + // participant_claim_decision_guard_test.go, which drives that window + // through claimWindowHook. inserted, err := r.insertManualParticipant(ctx, tx, stepID, taskID, role, agentID) if err != nil { @@ -290,6 +297,10 @@ func (r *Repository) attemptClaim( return nil, nil } + if claimWindowHook != nil { + claimWindowHook(ctx, tx, claim.id) + } + claimed, err := r.claimAutoSeat(ctx, tx, claim.id, agentID) if err != nil { return nil, err @@ -484,6 +495,17 @@ func (r *Repository) findClaimableAutoSeat( return &candidates[0], nil } +// claimWindowHook is a yield point between the statement that selects a +// claimable seat and the statement that reassigns it. It is nil in every build +// that does not set it, nothing production reads it, and it carries no +// behavior of its own. +// +// It exists because that window is the only place claimAutoSeat's decision +// condition can add anything the selection did not already provide, and the +// window cannot be reached by racing goroutines in a way a test can rely on. +// Unexported, so only this package's tests can set it. +var claimWindowHook func(ctx context.Context, tx *sqlx.Tx, seatID string) + // claimAutoSeat reassigns the seat identified by seatID to agentID and // marks it "manual", conditional on the seat still carrying provenance // "auto" and no decision having been recorded against it. Returns false diff --git a/apps/backend/internal/orchestrator/office_session_terminator_test.go b/apps/backend/internal/orchestrator/office_session_terminator_test.go index 1492b580a6..e4683598a1 100644 --- a/apps/backend/internal/orchestrator/office_session_terminator_test.go +++ b/apps/backend/internal/orchestrator/office_session_terminator_test.go @@ -81,6 +81,12 @@ func TestTerminateOfficeSession_MissingPair_NoOp(t *testing.T) { } } +// TestTerminateAllForAgent_CascadesAcrossTasks also covers +// AC-OFFICE-SESSION-TERM-003.1: the cascade is deliberately unguarded. Both +// seeded tasks name agent-a as their runner, so agent-a retains a capacity on +// each, and every one of its sessions must still end. The retained-capacity +// guard lives in the office dashboard service and this path never reaches it - +// an agent instance going away holds nothing anywhere. func TestTerminateAllForAgent_CascadesAcrossTasks(t *testing.T) { ctx := context.Background() repo := setupTestRepo(t) diff --git a/apps/backend/internal/workflow/repository/phase2_sqlite.go b/apps/backend/internal/workflow/repository/phase2_sqlite.go index 9abba19cd4..f24e266bb9 100644 --- a/apps/backend/internal/workflow/repository/phase2_sqlite.go +++ b/apps/backend/internal/workflow/repository/phase2_sqlite.go @@ -811,6 +811,11 @@ const participantsLockNamespace = "workflow-participant-role-seat:" // writers previously locked on disjoint namespaces (participantsLockNamespace // vs decisionLockNamespace) and never actually contended. // +// The exclusion is acquired only when the decision carries a role, so it does +// not cover every decision the store accepts. A roleless decision still +// commits without it, and claimAutoSeat's NOT EXISTS condition is what defends +// the seat on that path. +// // Keyed on task and role only, deliberately narrower than the namespace, // task, workflow and role EnsureRoleSeat locked on before this change. // Registration cannot know the task's workflow before it has resolved the diff --git a/docs/plans/office-seat-claim-decision-guard/plan.md b/docs/plans/office-seat-claim-decision-guard/plan.md new file mode 100644 index 0000000000..835e6abcaa --- /dev/null +++ b/docs/plans/office-seat-claim-decision-guard/plan.md @@ -0,0 +1,246 @@ +--- +created: 2026-09-08 +status: implemented +requirements: + - REQ-OFFICE-SEAT-GUARD-001 + - REQ-OFFICE-SEAT-GUARD-002 +system_design: + - ../../specs/office/system-design/seat-claim-decision-guard-01.md +legacy_specs: [] +--- + +# Implementation Plan: Office Seat Claim Decision Guard + +## Overview + +`claimAutoSeat`'s `NOT EXISTS` condition over `workflow_step_decisions` is +covered by a test whose stated premise stopped being true when the shared seat +exclusion landed. The condition is still load-bearing, but on a path the test +does not reach, and the production comment beside it says the opposite. + +No production behavior changes. Task 01 adds a test-only interleaving hook and +the guard test that actually distinguishes the condition being present from its +being absent. Task 02 corrects the two pieces of prose that assert a false +premise. That order is deliberate: the corrected prose cites the new test as +what covers the window, so the test must exist before the comment can point at +it. + +## Confirmed root cause + +- `claimAutoSeat` (`internal/office/repository/sqlite/participants.go:437`) + carries `NOT EXISTS (SELECT 1 FROM workflow_step_decisions WHERE + participant_id = ?)` on the reassigning `UPDATE`. It adds something + `findClaimableAutoSeat` did not already provide only when a decision commits + between the two statements. +- `recordStepDecisionTx` + (`internal/workflow/repository/phase2_sqlite.go:1097-1102`) acquires + `ParticipantRoleSeatLockKey` - the same exclusion registration holds for its + whole transaction - but only `if d.Role != ""`. A roleless decision skips it. +- `validateDecisionParticipantSeatTx` + (`internal/workflow/repository/phase2_sqlite.go`) returns early when + `d.Role == ""`, so the roleless path takes neither the seat exclusion nor the + seat validation. For that path the `UPDATE`'s condition is the whole defense. +- No shipped caller produces a roleless decision - both office entry points + resolve a role or refuse - so this is latent, not a live defect. +- `TestPostgresAddTaskParticipant_ClaimDoesNotOverwriteAConcurrentDecision` + (`internal/office/repository/sqlite/participant_claim_postgres_test.go:78`) + documents itself as proving the condition load-bearing on the premise that + the two writers "lock on two different advisory-lock namespaces that never + contend". That premise is false since commit `a6c6af2d5`. +- The production comment at `participants.go:169-173` states that + `recordStepDecisionTx` "now shares this transaction's + ParticipantRoleSeatLockKey exclusion, so it cannot actually interleave here", + which holds for a role-carrying decision and not for a roleless one. + +## Scope + +### In scope + +- A test-only interleaving hook in the office sqlite package, called between + the statement that selects the claimable seat and the statement that + reassigns it, nil in every build that does not set it. +- A deterministic guard test that makes a roleless decision exist against the + chosen seat inside that window and asserts the seat is not reassigned and a + fresh seat is written for the registering agent instead. +- A Postgres-gated variant that commits the roleless decision from a real + second connection, covering the literal cross-transaction window. +- Correcting the existing concurrency test's docstring to claim only what + serialization can show. +- Correcting the production comment that describes the guard as unreachable. + +### Out of scope + +- Removing the `NOT EXISTS` condition. `AC-OFFICE-SEAT-GUARD-001.3` requires + it to stay, and the roleless path is why. +- Making the roleless decision path acquire the seat exclusion. It has no role + to key on and would have to derive one from the seat it references, turning a + write into a read-then-lock for a path no shipped caller uses. +- Rejecting roleless decisions. Whether the store should require a role is a + separate contract question about the decision model. +- Adding a `superseded_at IS NULL` filter to either defense. + `AC-OFFICE-SEAT-GUARD-001.8` requires both to keep treating any decision as + blocking. +- Deleting or weakening the existing concurrency test. Its iterations and + multi-connection pool stay; only its docstring changes. +- The sibling follow-up's claim-target-removed test. If it wants the same yield + point, it is the same hook at the same site, but neither card depends on the + other. +- Any observability signal for the declined-claim outcome, which + `AC-OFFICE-SEAT-GUARD-001.5` deliberately makes indistinguishable from "no + claimable seat existed". + +## Technical approach + +### The hook + +An unexported package-level function value in +`internal/office/repository/sqlite`, called from `attemptClaim` +(`participants.go:206`) between `findClaimableAutoSeat` and `claimAutoSeat`. +Unexported, so only the package's own tests can set it, and no build tag is +needed to keep it unreachable. Unset, the call site is a nil check on a value +nothing ever writes, so the production path is the behavior that ships today. +The hook carries no policy: it is a yield point. + +The hook receives the in-flight transaction and the selected seat identifier. +That signature is what makes the deterministic test possible on the embedded +engine, and it is a deviation from the system design's "second connection" +wording - see Open questions. + +### The deterministic test + +Sets the hook to insert a roleless decision row against the selected seat +using the registration's own transaction, then lets `claimAutoSeat` run. The +`UPDATE` affects zero rows, `attemptClaim` returns nil, and the registration +falls through to `insertManualParticipant`. Asserts the auto seat still names +its original agent with its provenance, decision-required flag, position and +creation time unchanged; that the decision row is unchanged; that a fresh +manual seat exists for the registering agent; and that the reported outcome is +`ParticipantWriteOutcomeInserted` rather than `...Claimed`, so none of the +effects a claim earns is triggered. + +This runs on the embedded engine, where the office sqlite suite sets +`SetMaxOpenConns(1)`. Removing the `NOT EXISTS` condition must make it fail; +that check is performed once by hand when the test is written and recorded in +the work order's Results. + +### The Postgres-gated variant + +`openIsolatedPostgresMultiConnForClaimRace` already provides a four-connection +pool. The hook commits a roleless decision from a second connection while the +registration's transaction is open. There is no deadlock: the roleless path +skips `ParticipantRoleSeatLockKey`, takes only `decisionLockNamespace`, and +writes no row the registration holds. Skips unless `KANDEV_TEST_POSTGRES_DSN` +is set, matching the neighbouring test. + +### The prose corrections + +The concurrency test's docstring is re-written to state what its assertions +support: that a role-carrying decision and a registration serialize on the +shared seat exclusion, complete without deadlock in either acquisition order, +and leave no reattributed decision. The production comment at +`participants.go:169-173` is corrected to say that the exclusion closes the +window for a role-carrying decision and that the condition remains the defense +for a roleless one. + +## Tests + +| Acceptance criterion | Evidence | +| --- | --- | +| `AC-OFFICE-SEAT-GUARD-001.1`, `-001.2` | `internal/office/repository/sqlite/participant_claim_decision_guard_test.go` - roleless decision in the window blocks the reassignment | +| `AC-OFFICE-SEAT-GUARD-001.4` | Same file - seat's agent profile, provenance, decision-required flag, position and creation time unchanged, decision unchanged | +| `AC-OFFICE-SEAT-GUARD-001.5`, `-001.6` | Same file - outcome is `Inserted`, no displaced agent reported, registration succeeds | +| `AC-OFFICE-SEAT-GUARD-001.8` | Same file - a superseded decision blocks equally; the selection and the update agree | +| `AC-OFFICE-SEAT-GUARD-001.9` | Same file - repeating the registration writes nothing further under the already-seated rule | +| `AC-OFFICE-SEAT-GUARD-001.3`, `-002.1`, `-002.2` | Same file - the deterministic test fails with the `NOT EXISTS` condition removed, verified by hand | +| `AC-OFFICE-SEAT-GUARD-002.2` (cross-transaction reading) | Same file, Postgres-gated variant on a real second connection | +| `AC-OFFICE-SEAT-GUARD-002.3` | `internal/office/repository/sqlite/participants_test.go` - existing claim tests pass unchanged with the hook unset | +| `AC-OFFICE-SEAT-GUARD-002.4`, `-002.5` | `internal/office/repository/sqlite/participant_claim_postgres_test.go` - docstring asserts serialization and no-deadlock only | +| `AC-OFFICE-SEAT-GUARD-001.7` | No change to `docs/specs/office/requirements/review-participant-seats.md` or the seat-provenance contract | + +## E2E tests + +None. `REQ-OFFICE-SEAT-GUARD-001` records existing behavior and ships no +user-visible change. + +## Work orders + +- [completed] [Task 01: Cover The Claim Decision Guard](task-01-cover-claim-decision-guard.md) +- [completed] [Task 02: Correct The Stale Exclusion Premise](task-02-correct-exclusion-premise.md) + +## Dependency order + +```text +Task 01 -> Task 02 +``` + +Task 02's corrected prose names the test Task 01 adds as what covers the +roleless window. The package is sequential. + +## Verification results + +- Four engine-agnostic guard tests pass; `internal/office/repository/sqlite` + and `internal/workflow/repository` pass with `-race`. +- The by-hand acceptance check was performed: deleting `claimAutoSeat`'s + `NOT EXISTS` condition makes the guard tests fail with the seat reattributed + to the registering agent, and restoring it makes them pass. +- The Postgres-gated variant compiles, vets, and skips cleanly, but has never + run against a real server: this runner has no PostgreSQL and the card forbids + Docker. CI is its first execution. +- `golangci-lint` reports 0 issues across `internal/office/...` and + `internal/workflow/...`; specification lint passes; `git diff --check` is + clean. +- The open question below was resolved as proposed. + +## Risks + +- A test-only hook in a production file is a concession. It is acceptable only + while it stays unexported, unset in production, and carries no behavior; a + reviewer should check all three rather than the first. +- Racing real goroutines on the roleless path would reach the window but is not + forced to, so such a test would pass most of the time by never entering it - + the same defect being fixed, reintroduced. The hook exists to avoid that. +- The deterministic test's premise is that the `UPDATE` sees the decision. If a + future change moves the decision read out of `claimAutoSeat`'s statement, the + test would keep passing while covering nothing. The by-hand removal check is + what catches this, and it must be repeated if the statement is restructured. +- The Postgres variant depends on the roleless path continuing to skip + `ParticipantRoleSeatLockKey`. If that changes, the variant deadlocks rather + than failing clearly; the deterministic test is the primary coverage for that + reason. +- Correcting the concurrency test's docstring without weakening its assertions + is the intent. A reviewer should confirm no `iterations` or pool size was + reduced alongside the prose. +- The sibling seat-provenance hardening card proposes, as its ninth gap, "a + test-only failpoint between `findClaimableAutoSeat` and `claimAutoSeat`" for + `TestPostgresAddTaskParticipant_ConvergesWithConcurrentRemoveOfClaimTarget`. + That is the same site as this hook, for a different test and a different + acceptance criterion. The two do not overlap in coverage, but they collide in + the file: whichever lands first owns the hook, and the second reuses it + rather than adding a parallel one. Check for an existing hook at that site + before writing Task 01. + +## Open questions + +- The system design's "Making the window reachable" section specifies that the + hook's test "commits a roleless decision against the chosen seat from a + second connection". The office sqlite suite opens its database with + `SetMaxOpenConns(1)`, so a second connection issued while the registration's + transaction holds the only pooled connection blocks indefinitely. This plan + therefore has the hook receive the in-flight transaction, writes the decision + through it for the deterministic embedded-engine test, and keeps the literal + cross-connection reading as the Postgres-gated variant. The requirement's + `AC-OFFICE-SEAT-GUARD-002.2` and `-002.3` are satisfied either way, since + they constrain determinism and production inertness rather than the + connection count. Confirm this deviation, or amend the system design's + wording, before Task 01 starts. + + **Resolved as proposed.** The hook takes the transaction, the deterministic + test writes through it and runs on every engine, and the literal + cross-connection reading is kept as the Postgres-gated variant. The setter + additionally lives in `export_test.go` rather than in an in-package test, so + a production build has no setter at all. + +## Package handoff + +Implementation follows the sequential TDD work orders. Update each work order's +status and the plan status after its verification commands pass. diff --git a/docs/plans/office-seat-claim-decision-guard/task-01-cover-claim-decision-guard.md b/docs/plans/office-seat-claim-decision-guard/task-01-cover-claim-decision-guard.md new file mode 100644 index 0000000000..284fc06d02 --- /dev/null +++ b/docs/plans/office-seat-claim-decision-guard/task-01-cover-claim-decision-guard.md @@ -0,0 +1,174 @@ +--- +id: "01-cover-claim-decision-guard" +title: "Cover the claim decision guard" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-OFFICE-SEAT-GUARD-001 + - REQ-OFFICE-SEAT-GUARD-002 +acceptance_criteria: + - AC-OFFICE-SEAT-GUARD-001.1 + - AC-OFFICE-SEAT-GUARD-001.2 + - AC-OFFICE-SEAT-GUARD-001.3 + - AC-OFFICE-SEAT-GUARD-001.4 + - AC-OFFICE-SEAT-GUARD-001.5 + - AC-OFFICE-SEAT-GUARD-001.6 + - AC-OFFICE-SEAT-GUARD-001.8 + - AC-OFFICE-SEAT-GUARD-001.9 + - AC-OFFICE-SEAT-GUARD-002.1 + - AC-OFFICE-SEAT-GUARD-002.2 + - AC-OFFICE-SEAT-GUARD-002.3 +system_design: + - ../../specs/office/system-design/seat-claim-decision-guard-01.md +--- + +# Task 01: Cover The Claim Decision Guard + +## Summary + +Add a test-only interleaving hook between the statement that selects a +claimable seat and the statement that reassigns it, and use it to drive a +deterministic test that fails when `claimAutoSeat`'s `NOT EXISTS` condition is +removed. Production behavior is unchanged: with the hook unset the call site is +a nil check on a value nothing writes. + +## In scope + +- An unexported package-level function value in + `internal/office/repository/sqlite`, invoked from `attemptClaim` between + `findClaimableAutoSeat` and `claimAutoSeat`, receiving the in-flight + transaction and the selected seat identifier. +- A deterministic in-package test that makes a roleless decision exist against + the selected seat inside the window and asserts the seat is not reassigned. +- Assertions that the auto seat's agent profile, provenance, decision-required + flag, position and creation time are unchanged, that the decision is + unchanged, that a fresh manual seat exists for the registering agent, and + that the reported outcome is `Inserted` with no displaced agent. +- A case proving a superseded decision blocks the reassignment identically. +- A case proving the repeat registration writes nothing further. +- A Postgres-gated variant committing the roleless decision from a second + connection, skipped unless `KANDEV_TEST_POSTGRES_DSN` is set. +- A recorded by-hand check that removing the `NOT EXISTS` condition makes the + deterministic test fail. + +## Out of scope + +- Any change to `claimAutoSeat`, `findClaimableAutoSeat`, + `insertManualParticipant`, the seat exclusion, or the registration's + transaction boundaries. +- Any change to `recordStepDecisionTx`, including making the roleless path + acquire the seat exclusion. +- The prose corrections in `participants.go` and the existing Postgres + concurrency test, which Task 02 owns. +- Adding a `superseded_at` filter to either defense. +- Any exported symbol, schema change, or migration. + +## Acceptance + +- With the hook unset, every existing claim test passes unchanged and the + production path is the behavior that ships today. +- The deterministic test enters the window on every run and cannot pass by + skipping it. +- Deleting `NOT EXISTS (SELECT 1 FROM workflow_step_decisions WHERE + participant_id = ?)` from `claimAutoSeat` makes the deterministic test fail; + restoring it makes the test pass. + +## Verification + +```bash +# From the repository root: +cd apps/backend && go test ./internal/office/repository/sqlite/... -race -count=1 +cd apps/backend && go test ./internal/workflow/repository/... -race -count=1 +make -C apps/backend lint +git diff --cached --name-only | grep '\.go$' | xargs -r gofmt -l +git diff --check + +# Postgres-gated variant, when a server is available: +cd apps/backend && KANDEV_TEST_POSTGRES_DSN="$KANDEV_TEST_POSTGRES_DSN" \ + go test ./internal/office/repository/sqlite/... -run ClaimDecisionGuard -count=1 +``` + +## Files likely touched + +- `apps/backend/internal/office/repository/sqlite/participants.go` +- `apps/backend/internal/office/repository/sqlite/participant_claim_decision_guard_test.go` + +## Dependencies + +None. + +## Risks + +- A hook that grows a return value or a branch stops being a yield point and + becomes a policy hook in production code. Keep it a bare call with no result + the production path reads. +- Racing goroutines instead of using the hook would pass most of the time by + never entering the window, which is the defect being fixed. Do not fall back + to a race even if the hook proves awkward. +- The office sqlite suite sets `SetMaxOpenConns(1)`. A test that opens a second + connection while the registration transaction is open blocks indefinitely + rather than failing; the embedded-engine test must write through the + in-flight transaction. +- The Postgres variant depends on the roleless decision path continuing to skip + `ParticipantRoleSeatLockKey`. If that stops being true it will deadlock + rather than fail clearly. +- The by-hand removal check is the only thing proving the test is not vacuous. + Record its result in this work order rather than asserting it was done. + +## Parallelism + +`sequential` + +## Inputs + +- `docs/specs/office/requirements/seat-claim-decision-guard.md`, + REQ-OFFICE-SEAT-GUARD-001 and -002. +- `docs/specs/office/system-design/seat-claim-decision-guard-01.md`, sections + "Two defenses, two windows" and "Making the window reachable", read together + with the plan's Open questions entry on the hook signature. +- `apps/backend/internal/office/repository/sqlite/participants.go:160-243` + (`ensureParticipant` fallthrough and `attemptClaim`) and `:437` + (`claimAutoSeat`). +- `apps/backend/internal/workflow/repository/phase2_sqlite.go:1080-1103` for + the roleless path's lock skip. +- `apps/backend/internal/office/repository/sqlite/participant_claim_postgres_test.go` + for the isolated multi-connection Postgres helper. + +## Results + +- Added the unexported `claimWindowHook` to + `internal/office/repository/sqlite/participants.go`, called from + `attemptClaim` between `findClaimableAutoSeat` and `claimAutoSeat`. Unset, the + site is a nil check on a value nothing writes. +- The setter lives in a new `export_test.go`, which the toolchain compiles only + under `go test`, so a production build has no way to set the hook at all. + This is a small deviation from the design's "an in-package test sets it": all + of this package's existing tests and their fixtures are in the external + `sqlite_test` package, and duplicating that setup in-package to reach an + unexported variable would have been worse than the standard `export_test.go` + idiom. The production symbol stays unexported either way. +- Added four engine-agnostic tests in + `participant_claim_decision_guard_test.go`: the roleless decision blocking + the reassignment, a superseded decision blocking identically, the repeat + registration writing nothing, and the hook-unset path still claiming in + place. +- **By-hand acceptance check performed.** With the `NOT EXISTS` condition + deleted from `claimAutoSeat`, the guard tests fail and show the exact + reattribution the condition prevents: the seat's `agent_profile_id` becomes + `agent-registering` and its provenance flips to `manual` while the decision + on file belongs to `agent-auto`. The hook-unset and repeat-registration tests + pass in both states, as they should. The condition was restored and the suite + re-run green. +- Added the Postgres-gated + `TestPostgresAddTaskParticipant_ClaimDecisionGuard_RolelessDecisionCommitsInWindow`, + which commits the roleless decision through the real `RecordStepDecision` on + a second pooled connection inside the window. + **This test has not been executed against a real server.** This runner has no + PostgreSQL and the card forbids starting Docker, so it was verified only to + compile, to vet clean, and to skip correctly without + `KANDEV_TEST_POSTGRES_DSN`. CI, which supplies a DSN, is its first real run. +- `internal/office/repository/sqlite` and `internal/workflow/repository` pass + with `-race`. `golangci-lint` reports 0 issues across `internal/office/...`, + `internal/workflow/...` and `internal/orchestrator/...`. diff --git a/docs/plans/office-seat-claim-decision-guard/task-02-correct-exclusion-premise.md b/docs/plans/office-seat-claim-decision-guard/task-02-correct-exclusion-premise.md new file mode 100644 index 0000000000..7a21072a7d --- /dev/null +++ b/docs/plans/office-seat-claim-decision-guard/task-02-correct-exclusion-premise.md @@ -0,0 +1,133 @@ +--- +id: "02-correct-exclusion-premise" +title: "Correct the stale exclusion premise" +status: done +wave: 2 +depends_on: + - "01-cover-claim-decision-guard" +plan: "plan.md" +requirements: + - REQ-OFFICE-SEAT-GUARD-001 + - REQ-OFFICE-SEAT-GUARD-002 +acceptance_criteria: + - AC-OFFICE-SEAT-GUARD-002.4 + - AC-OFFICE-SEAT-GUARD-002.5 + - AC-OFFICE-SEAT-GUARD-001.7 +system_design: + - ../../specs/office/system-design/seat-claim-decision-guard-01.md +--- + +# Task 02: Correct The Stale Exclusion Premise + +## Summary + +Two pieces of prose assert that role-carrying decision recording and +registration lock on non-contending exclusions, or that the claim's condition +is therefore unreachable. Both stopped being accurate when the shared seat +exclusion landed. Re-document them to state what is true, without changing any +assertion, iteration count, or pool size. + +## In scope + +- Rewriting `TestPostgresAddTaskParticipant_ClaimDoesNotOverwriteAConcurrentDecision`'s + docstring to claim only what its assertions support: that the two writers + serialize on the shared seat exclusion, complete without deadlock in either + acquisition order, and leave no reattributed decision. +- Correcting the comment at `participants.go:169-173` so it states that the + exclusion closes the window for a role-carrying decision while the `UPDATE`'s + condition remains the defense for a roleless one, and names the test from + Task 01 as the coverage. +- A scan of the office and workflow suites for any other test documentation + repeating the non-contending-namespaces premise. + +## Out of scope + +- Deleting, skipping, or weakening the existing concurrency test, reducing its + iterations, or shrinking its connection pool. +- Any change to production behavior, SQL, or lock acquisition. +- Any acceptance criterion added to or amended in the + participant-seat-provenance contract. +- Renaming the existing test. + +## Acceptance + +- No test documentation in the office or workflow suites asserts that + role-carrying decision recording and registration lock on non-contending + namespaces. +- The existing concurrency test's assertions, iteration count, and pool size + are byte-identical to before this work order. +- The corrected production comment distinguishes the role-carrying case from + the roleless one and states the invariant rather than the history of the bug. + +## Verification + +```bash +# From the repository root: +cd apps/backend && go test ./internal/office/repository/sqlite/... -race -count=1 +make -C apps/backend lint +git diff --cached --name-only | grep '\.go$' | xargs -r gofmt -l +python3 scripts/lint-spec-files.py --all +git diff --check +``` + +## Files likely touched + +- `apps/backend/internal/office/repository/sqlite/participant_claim_postgres_test.go` +- `apps/backend/internal/office/repository/sqlite/participants.go` + +## Dependencies + +- Task 01 adds the test that the corrected comment names as covering the + roleless window. + +## Risks + +- A comment that argues the code is correct instead of stating the invariant + reintroduces the failure this work order is fixing, in a new form. State + which defense covers which window and stop. +- Rewriting the docstring is easy to combine with "tidying" the assertions. + Confirm with a diff that only comment lines changed in the test file. +- Other suites may carry the same premise in passing. The scan is part of the + work order because `AC-OFFICE-SEAT-GUARD-002.5` is written over the whole + suite, not over one file. + +## Parallelism + +`sequential` + +## Inputs + +- `docs/specs/office/requirements/seat-claim-decision-guard.md`, + REQ-OFFICE-SEAT-GUARD-002. +- `docs/specs/office/system-design/seat-claim-decision-guard-01.md`, section + "Verification obligations", items 2 and 3. +- `apps/backend/internal/office/repository/sqlite/participant_claim_postgres_test.go:78-93`. +- `apps/backend/internal/office/repository/sqlite/participants.go:166-173`. +- `CLAUDE.md`, "Code Quality", on comments stating the invariant rather than + the argument for it. + +## Results + +- Rewrote `TestPostgresAddTaskParticipant_ClaimDoesNotOverwriteAConcurrentDecision`'s + docstring to claim only what its assertions support: that the two writers + serialize on the shared seat exclusion, complete in either acquisition order + without deadlock, and leave no reattributed decision. It now says explicitly + that it does not, and cannot, show the NOT EXISTS condition to be + load-bearing, and points at the test that does. +- Confirmed by diff that the change is comment-only: no assertion, iteration + count, or pool size was touched. +- Corrected the claim fallthrough comment in `participants.go`, which described + the condition as a defensive backstop that "cannot actually interleave here". + It now states which defense covers which decision. +- Also corrected `ParticipantRoleSeatLockKey`'s doc comment in + `phase2_sqlite.go`, which presented the shared exclusion as fully closing the + window. It is acquired only when the decision carries a role, and that + omission is the same false completeness this work order exists to remove. + This was a third site, beyond the two the work order named; the suite-wide + scan `AC-OFFICE-SEAT-GUARD-002.5` requires is what surfaced it. +- The scan for the stale premise (`never contend with each other`, + `two different advisory-lock namespaces`, `cannot actually interleave`) now + returns nothing across `internal/`. +- `internal/office/repository/sqlite` and `internal/workflow/repository` pass + with `-race`; `golangci-lint` reports 0 issues across `internal/office/...` + and `internal/workflow/...`; specification lint passes. diff --git a/docs/plans/office-task-session-termination/plan.md b/docs/plans/office-task-session-termination/plan.md new file mode 100644 index 0000000000..784c801302 --- /dev/null +++ b/docs/plans/office-task-session-termination/plan.md @@ -0,0 +1,220 @@ +--- +created: 2026-09-08 +status: implemented +requirements: + - REQ-OFFICE-SESSION-TERM-001 + - REQ-OFFICE-SESSION-TERM-002 + - REQ-OFFICE-SESSION-TERM-003 + - REQ-OFFICE-SESSION-TERM-004 +system_design: + - ../../specs/office/system-design/task-session-termination-01.md +legacy_specs: [] +--- + +# Implementation Plan: Office Task Session Termination + +## Overview + +An office session row is keyed `(task_id, agent_profile_id)`. Three paths end +one on the loss of a single capacity, without asking whether the agent still +holds another on the same task. An agent that is both the task's runner and a +seated reviewer therefore loses the conversation it is still coding through +when it is displaced from, removed from, or reassigned away from one of those +two standings. + +The change is a precondition on three call sites: resolve whether the agent +still holds a capacity on the task, and terminate only when it holds none. +Task 01 delivers the determination, wires all three call sites, and lands the +two distinguishable log records the fail-closed path needs to be diagnosable. +Task 02 adds the suppression counter and the operator-facing documentation. +That order is forced: a fail-closed suppression that logs nothing is +indistinguishable from a path that never ran, so the records ship with the +behavior rather than after it. + +## Confirmed root cause + +- `officeSessionTerminator.TerminateOfficeSession` + (`internal/orchestrator/office_session_terminator.go:32`) resolves the + session by `(taskID, agentInstanceID)` alone and flips the row to + `COMPLETED`. It has no notion of role and cannot acquire one without + importing office. +- Three call sites in `internal/office/dashboard/service_tasks.go` reach it + unconditionally after their own mutation commits: line 435 + (`sessionTermReasonRoleRemoved`), line 484 via `terminateDisplacedSession` + (`sessionTermReasonSeatClaimed`), and line 913 in + `runReactivityForAssigneeChange` (`sessionTermReasonReassigned`). +- `AC-OFFICE-REVIEW-SEATS-002.3` prefers a reviewer that is "neither the + task's runner nor already seated in another participant role" and falls + back when none exists, so an agent holding two capacities is a shipped, + fallback-default outcome rather than a misconfiguration. +- With `features.officeSessionIdentity` disabled - the state every profile + ships - a task that has a runner routes every office run through the + runner's session, so the role-removal and seat-claim paths either no-op + against a non-runner or destroy the task's only live session. +- `AC-OFFICE-SEAT-PROVENANCE-002.12` already scopes the claim path's + termination to an agent holding a live session "in that role". The + implementation is broader than its own frozen criterion because the session + model cannot express the qualifier. + +## Scope + +### In scope + +- One retained-capacity determination in the office dashboard service, + reachable from all three guarded call sites and implemented once. +- Runner capacity resolved through the shared runner projection, not through a + hand-written runner-seat read. +- Seat capacity resolved through the task's effective participant slate at its + current step, covering template-projected seats and every role. +- Fail-closed behavior when either read fails: suppress, record, report + success. +- Two distinguishable records - retained capacity, and read failure - plus a + suppression counter labelled only by bounded dimensions. +- Regression coverage for the step-scoping bound, the reassignment decoupling, + and the shipped-configuration reachability with the flag off. + +### Out of scope + +- Role-scoped session identity. `AC-OFFICE-SESSION-IDENTITY-004.5` forbids the + discriminator column it needs, and the guard delivers the same observable + outcome with no change to session identity. +- The cascade path (`TerminateAllForAgent`). It stays unguarded per + `AC-OFFICE-SESSION-TERM-003.1`. +- Repairing sessions already ended by the current behavior. No migration, no + backfill. +- Any change to participant casting, seat provenance, or claim behavior. +- Ending a session when an agent loses its last capacity through a path other + than the three named ones. +- The `features.officeSessionIdentity` rollout decision. +- Any user-visible surface, copy, or i18n work. + +## Technical approach + +### The determination + +A single unexported method on `*DashboardService` in +`internal/office/dashboard/service_tasks.go`, taking a task identifier and an +agent profile identifier and answering whether the agent retains a capacity, +plus which one it found. It returns a read error separately from the boolean so +the caller can distinguish `AC-OFFICE-SESSION-TERM-004.1` from `-004.2`. + +The runner half reads the task's execution fields through the existing office +repository accessor, whose assignee value is produced by +`sqlite.RunnerProjection` (`internal/office/repository/sqlite/base.go:34`). +`AC-OFFICE-SESSION-TERM-002.2` requires this rather than a direct seat read, +and the projection's third tier is why: it deliberately reads runner rows +across every step ordered by `created_at DESC`, so a hand-written runner-seat +query would disagree with the rest of the system exactly on tasks that have +moved between steps. + +The seat half reads `Repository.ListAllTaskParticipants` +(`internal/office/repository/sqlite/participants.go:517`), which already +resolves the step, returns an empty slate when the task has no current step, +merges template-level rows under per-task precedence, and spans every role. +`AC-OFFICE-SESSION-TERM-002.3`, `-002.4` and `-002.5` fall out of reusing it +rather than being separately implemented. + +Both reads run on the read pool, take no lock, and join no transaction. +`AC-OFFICE-SESSION-TERM-001.10` explains why no coordination is needed: each +guarded path reads strictly after its own commit, so whichever commits second +observes both removals and terminates. + +### The call sites + +Three insertions in one file, each immediately before an existing +`TerminateOfficeSession` call and each leaving every other effect of its path +untouched: + +- `addOrRemoveParticipant` (line ~429), after `RemoveTaskParticipant` commits. +- `terminateDisplacedSession` (line ~478), inside the already-detached context, + beside but not entangled with `cancelDisplacedRun`. +- `runReactivityForAssigneeChange` (line ~911), after the reactivity pipeline's + hard cancel has already run. `AC-OFFICE-SESSION-TERM-003.2` requires that + cancel to stay unguarded, so the guard goes below `hardCancelTaskAsync`, not + above it. + +The empty-input case (`AC-OFFICE-SESSION-TERM-001.8`) short-circuits before the +determination. The terminator's own empty-input no-op stays as it is. + +### Observability + +An expvar map in the dashboard package following +`internal/orchestrator/office_stall_metrics.go`'s label idiom +(`"k1=v1;k2=v2"`), labelled by the guarded reason and the retained capacity - +three reasons and two capacities, both bounded. Suppression and read failure +log at the existing call-site loggers with the task, agent profile and reason, +distinguishably from each other. + +## Tests + +| Acceptance criterion | Evidence | +| --- | --- | +| `AC-OFFICE-SESSION-TERM-001.1`, `-001.4` | `internal/office/dashboard/session_termination_test.go` - agent retaining no capacity still terminates with its existing reason | +| `AC-OFFICE-SESSION-TERM-001.2`, `-002.12` | Same file - determination observes post-commit state; a capacity granted before the read suppresses | +| `AC-OFFICE-SESSION-TERM-001.3`, `-001.6` | Same file - suppressed path writes no session row and returns success | +| `AC-OFFICE-SESSION-TERM-001.7`, `-002.8` | Same file - a failing slate read suppresses rather than terminates | +| `AC-OFFICE-SESSION-TERM-001.8`, `-001.9`, `-001.11` | Same file - empty inputs, no live session, repeated termination | +| `AC-OFFICE-SESSION-TERM-002.1` … `-002.7`, `-002.9` … `-002.11` | `internal/office/dashboard/session_capacity_test.go` - runner-only, seat-only, both, neither; template-projected seat; decision-free role; multiple seats; slate ordering; unresolvable profile | +| `AC-OFFICE-SESSION-TERM-002.3` | Same file - task moved off a step that still carries the agent's seat must not suppress | +| `AC-OFFICE-SESSION-TERM-003.1` | `internal/orchestrator/office_session_terminator_test.go` - cascade stays unguarded | +| `AC-OFFICE-SESSION-TERM-003.2` | `internal/office/dashboard/session_termination_test.go` - reassignment cancels the execution *and* leaves the session row untouched | +| `AC-OFFICE-SESSION-TERM-003.5` | Same file - reachability regression with `features.officeSessionIdentity` off | +| `AC-OFFICE-SESSION-TERM-004.1`, `-004.2`, `-004.4` | Same file - the two records are distinguishable; proceeding terminations keep their reason values | +| `AC-OFFICE-SESSION-TERM-004.3` | `internal/office/dashboard/session_termination_metrics_test.go` - counter increments, labels bounded | + +## E2E tests + +None. `AC-OFFICE-SESSION-TERM-003.4` limits the observable change to a session +row that is not written; the existing live participant indicators render from +session state that already exists, and the requirement's "Out of scope" +excludes any user-visible surface. + +## Work orders + +- [completed] [Task 01: Guard Session Termination On Retained Capacity](task-01-guard-retained-capacity.md) +- [completed] [Task 02: Record Suppressed Session Terminations](task-02-record-suppressed-terminations.md) + +## Dependency order + +```text +Task 01 -> Task 02 +``` + +Task 02 counts an outcome Task 01 creates and documents behavior Task 01 +verifies. The package is sequential. + +## Verification results + +- `internal/office/dashboard` and `internal/orchestrator` pass with `-race`; + no pre-existing test required a change. +- The guard's tests were confirmed non-vacuous by short-circuiting + `retainsTaskCapacity` to "no capacity": 10 new tests fail, and the three + tests covering terminations that must still proceed pass in both states. +- Full-repo `golangci-lint run ./...` reports 0 issues; `go build ./...` + passes; `gofmt` reports no files. +- Specification lint passes. + +## Risks + +- The determination is a suppression, so a mistake in the seat scope leaks + sessions silently rather than failing. `AC-OFFICE-SESSION-TERM-002.3`'s + step bound is the specific defense, and its test must move the task or it + passes without exercising the bound. +- Existing tests in `session_termination_test.go` assert that removal + terminates. Any that incidentally leave the removed agent as the task's + runner will start suppressing and must be corrected by fixing their setup, + not by weakening the assertion. +- The reassignment path's guard sits near `hardCancelTaskAsync`. An + implementation that suppresses both together satisfies the session assertion + and violates `AC-OFFICE-SESSION-TERM-003.2`; both halves must be asserted. +- Two extra read-pool queries per guarded termination. All three paths are + operator-initiated and low frequency, so the cost is accepted rather than + cached. +- `RunnerProjection` is inlined SQL rather than a callable predicate. Reusing + it means reading the task through an existing repository accessor; adding a + bespoke query would reintroduce exactly the disagreement `-002.2` forbids. + +## Package handoff + +Implementation follows the sequential TDD work orders. Update each work order's +status and the plan status after its verification commands pass. diff --git a/docs/plans/office-task-session-termination/task-01-guard-retained-capacity.md b/docs/plans/office-task-session-termination/task-01-guard-retained-capacity.md new file mode 100644 index 0000000000..7719a1df11 --- /dev/null +++ b/docs/plans/office-task-session-termination/task-01-guard-retained-capacity.md @@ -0,0 +1,184 @@ +--- +id: "01-guard-retained-capacity" +title: "Guard session termination on retained capacity" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-OFFICE-SESSION-TERM-001 + - REQ-OFFICE-SESSION-TERM-002 + - REQ-OFFICE-SESSION-TERM-003 + - REQ-OFFICE-SESSION-TERM-004 +acceptance_criteria: + - AC-OFFICE-SESSION-TERM-001.1 + - AC-OFFICE-SESSION-TERM-001.2 + - AC-OFFICE-SESSION-TERM-001.3 + - AC-OFFICE-SESSION-TERM-001.4 + - AC-OFFICE-SESSION-TERM-001.5 + - AC-OFFICE-SESSION-TERM-001.6 + - AC-OFFICE-SESSION-TERM-001.7 + - AC-OFFICE-SESSION-TERM-001.8 + - AC-OFFICE-SESSION-TERM-001.9 + - AC-OFFICE-SESSION-TERM-001.10 + - AC-OFFICE-SESSION-TERM-001.11 + - AC-OFFICE-SESSION-TERM-002.1 + - AC-OFFICE-SESSION-TERM-002.2 + - AC-OFFICE-SESSION-TERM-002.3 + - AC-OFFICE-SESSION-TERM-002.4 + - AC-OFFICE-SESSION-TERM-002.5 + - AC-OFFICE-SESSION-TERM-002.6 + - AC-OFFICE-SESSION-TERM-002.7 + - AC-OFFICE-SESSION-TERM-002.8 + - AC-OFFICE-SESSION-TERM-002.9 + - AC-OFFICE-SESSION-TERM-002.10 + - AC-OFFICE-SESSION-TERM-002.11 + - AC-OFFICE-SESSION-TERM-002.12 + - AC-OFFICE-SESSION-TERM-003.1 + - AC-OFFICE-SESSION-TERM-003.2 + - AC-OFFICE-SESSION-TERM-003.3 + - AC-OFFICE-SESSION-TERM-003.4 + - AC-OFFICE-SESSION-TERM-003.5 + - AC-OFFICE-SESSION-TERM-003.6 + - AC-OFFICE-SESSION-TERM-004.1 + - AC-OFFICE-SESSION-TERM-004.2 + - AC-OFFICE-SESSION-TERM-004.4 +system_design: + - ../../specs/office/system-design/task-session-termination-01.md +--- + +# Task 01: Guard Session Termination On Retained Capacity + +## Summary + +Add one retained-capacity determination to the office dashboard service and +place it in front of all three guarded `TerminateOfficeSession` call sites, so +a session ends only when the agent has lost its last capacity on the task. The +determination fails closed and emits two distinguishable records, so a +suppressed termination is never mistaken for a path that did not run. + +## In scope + +- One unexported determination on `*DashboardService` answering whether an + agent retains a capacity on a task, and naming the capacity it found. +- Runner capacity read through the existing repository accessor whose assignee + value is produced by `sqlite.RunnerProjection`. +- Seat capacity read through `Repository.ListAllTaskParticipants`. +- Guard insertion at the three call sites in `service_tasks.go`: participant + removal, displaced-seat claim, and assignee reassignment. +- Fail-closed handling of a read error: suppress, record, return success. +- The retained-capacity record and the read-failure record, distinguishable + from each other, at the existing call-site loggers. +- Unit coverage for every acceptance criterion listed above, including the + three regressions the system design names as silently passing when written + casually. + +## Out of scope + +- The suppression counter and the operator-facing documentation, which + Task 02 owns. +- Any change to `internal/orchestrator/office_session_terminator.go`. It stays + the idempotent executor with both methods and both interfaces unchanged. +- Guarding `TerminateAllForAgent` or any cascade caller. +- Session creation, reuse, lookup, identity, schema, or index. +- Participant casting, seat provenance, and claim behavior. + +## Acceptance + +- All three guarded call sites route through exactly one determination; no + second copy exists in application code. +- An agent that is both the task's runner and a seated reviewer keeps its + session row untouched when displaced from the reviewer seat, removed from + that role, or reassigned away from the runner standing, and the row still + ends when its last capacity goes. +- A reassignment that suppresses the session termination still hard-cancels the + previous runner's running execution. +- A failing capacity read suppresses the termination, records the failure + distinguishably, and returns success to the caller. + +## Verification + +```bash +# From the repository root: +cd apps/backend && go test ./internal/office/dashboard/... -race -count=1 +cd apps/backend && go test ./internal/orchestrator/... -race -count=1 +cd apps/backend && go test ./internal/office/repository/... -race -count=1 +make -C apps/backend lint +git diff --cached --name-only | grep '\.go$' | xargs -r gofmt -l +git diff --check +``` + +## Files likely touched + +- `apps/backend/internal/office/dashboard/service_tasks.go` +- `apps/backend/internal/office/dashboard/session_termination_test.go` +- `apps/backend/internal/office/dashboard/session_capacity_test.go` +- `apps/backend/internal/orchestrator/office_session_terminator_test.go` + +## Dependencies + +None. + +## Risks + +- The step-scoping bound (`AC-OFFICE-SESSION-TERM-002.3`) passes without being + exercised unless its test moves the task off the step that carries the + agent's seat. Write the move explicitly; a same-step fixture proves nothing. +- The decoupling criterion (`AC-OFFICE-SESSION-TERM-003.2`) passes for an + implementation that wrongly suppresses the hard cancel as well, if only the + session row is asserted. Assert both halves. +- Existing tests assert that participant removal terminates. One whose fixture + incidentally leaves the removed agent as the task's runner will begin + suppressing; correct the fixture rather than relaxing the assertion. +- Placing the guard inside the orchestrator's terminator instead of the + dashboard service would invert the office/orchestrator dependency the + `SessionTerminator` interfaces exist to prevent, and would put + `TerminateAllForAgent` behind a guard `AC-OFFICE-SESSION-TERM-003.1` + requires it to stay clear of. +- Resolving the runner from participant seats rather than the shared projection + reads correctly on a task that has never moved and disagrees with every other + reader on one that has. + +## Parallelism + +`sequential` + +## Inputs + +- `docs/specs/office/requirements/task-session-termination.md`, + REQ-OFFICE-SESSION-TERM-001 through -003. +- `docs/specs/office/system-design/task-session-termination-01.md`, sections + "Where the precondition lives", "The capacity question", "Control flow". +- `apps/backend/internal/office/repository/sqlite/base.go:34` + (`RunnerProjection`) and `participants.go:517` + (`ListAllTaskParticipants`). +- `apps/backend/internal/office/dashboard/session_termination_test.go` for the + existing recording-terminator test pattern. + +## Results + +- Added `retainsTaskCapacity` and `terminateSessionUnlessRetained` to + `internal/office/dashboard/service_tasks.go`. The determination reads the + runner through `GetTaskExecutionFields` (whose assignee value is + `RunnerProjection`) and the slate through `ListAllTaskParticipants`. Both were + already on the dashboard `Repository` interface, so no interface changed. +- Routed all three guarded call sites through the one helper: participant + removal, `terminateDisplacedSession`, and `runReactivityForAssigneeChange`. + The reassignment path's hard cancel was left above the guard, untouched. +- `internal/orchestrator/office_session_terminator.go` is unmodified. +- Added `internal/office/dashboard/session_capacity_test.go` (10 tests) and + four tests plus a two-state flag subtest in `session_termination_test.go`. +- Verified the tests are not vacuous: with `retainsTaskCapacity` short-circuited + to `capacityNone`, 10 of the new tests fail, including both flag states, the + claim path, and the reassignment decoupling test - which fails on its session + assertion while its cancellation assertion still passes, proving the two + halves are independent. The three "must keep working" tests + (`TerminatesWhenNoCapacityRemains`, `StaleSeatAtLeftStepDoesNotSuppress`, + `TerminatesWhenPrevRunnerKeepsNothing`) pass in both states. +- `AC-OFFICE-SESSION-TERM-003.1` needed no new test: the existing + `TestTerminateAllForAgent_CascadesAcrossTasks` already seeds the agent as + runner on both tasks and asserts the cascade still ends every session. Its + docstring now records that. +- Full `internal/office/dashboard` and `internal/orchestrator` suites pass with + `-race`; no pre-existing test needed changing. Full-repo `golangci-lint run + ./...` reports 0 issues. diff --git a/docs/plans/office-task-session-termination/task-02-record-suppressed-terminations.md b/docs/plans/office-task-session-termination/task-02-record-suppressed-terminations.md new file mode 100644 index 0000000000..ccadcd8eed --- /dev/null +++ b/docs/plans/office-task-session-termination/task-02-record-suppressed-terminations.md @@ -0,0 +1,128 @@ +--- +id: "02-record-suppressed-terminations" +title: "Record suppressed session terminations" +status: done +wave: 2 +depends_on: + - "01-guard-retained-capacity" +plan: "plan.md" +requirements: + - REQ-OFFICE-SESSION-TERM-004 +acceptance_criteria: + - AC-OFFICE-SESSION-TERM-004.1 + - AC-OFFICE-SESSION-TERM-004.2 + - AC-OFFICE-SESSION-TERM-004.3 + - AC-OFFICE-SESSION-TERM-004.4 +system_design: + - ../../specs/office/system-design/task-session-termination-01.md +--- + +# Task 02: Record Suppressed Session Terminations + +## Summary + +Count suppressed terminations under the office namespace, labelled only by the +guarded reason and the retained capacity, and document the new counter beside +the office metrics families the repository already lists. A suppression that is +only logged is not queryable, which leaves a mistaken retained-capacity answer +indistinguishable from a quiet system. + +## In scope + +- An expvar map in the office dashboard package counting suppressions, + following the `"k1=v1;k2=v2"` label idiom of + `internal/orchestrator/office_stall_metrics.go`. +- Labels restricted to the guarded reason (three values) and the retained + capacity (two values), plus a distinct label value for a suppression caused + by a read failure. +- A test asserting the counter increments on each suppression path and that no + task, agent, step, or session identifier reaches a label. +- The observability entry in `CLAUDE.md`, listing the new counter beside + `office_stall_*` and `routing_*`. + +## Out of scope + +- The determination and the guarded call sites, which Task 01 owns. +- Any new log record. Task 01 lands both records; this work order only counts. +- A Prometheus exporter or any change to the `/debug/vars` wiring. +- A gauge or snapshot of currently-suppressed sessions. These are event + counters, matching the office stall detectors' stated model. + +## Acceptance + +- Each suppression increments the counter exactly once, with the reason and + capacity that produced it. +- A read-failure suppression is countable separately from a + retained-capacity suppression. +- Every label value comes from a fixed set; a test fails if an identifier is + introduced as a label. +- `CLAUDE.md`'s Observability section names the counter and its label + dimensions. + +## Verification + +```bash +# From the repository root: +cd apps/backend && go test ./internal/office/dashboard/... -race -count=1 +make -C apps/backend lint +cd apps/backend && go build ./... +git diff --check +``` + +## Files likely touched + +- `apps/backend/internal/office/dashboard/session_termination_metrics.go` +- `apps/backend/internal/office/dashboard/session_termination_metrics_test.go` +- `apps/backend/internal/office/dashboard/service_tasks.go` +- `CLAUDE.md` + +## Dependencies + +- Task 01 creates the suppression outcome this work order counts and the call + sites the increment is issued from. + +## Risks + +- An expvar map key built from an unbounded value grows without limit. Both + dimensions are closed sets; assert that in the test rather than trusting the + call sites. +- `expvar.NewMap` panics on a duplicate name. Keep the counter's registration + in one file with a name not already taken by `office_stall_*` or + `routing_*`. +- Documenting the counter in `CLAUDE.md` without the label dimensions makes it + unusable from a query. Name both. + +## Parallelism + +`sequential` + +## Inputs + +- `docs/specs/office/requirements/task-session-termination.md`, + REQ-OFFICE-SESSION-TERM-004. +- `docs/specs/office/system-design/task-session-termination-01.md`, section + "Observability". +- `apps/backend/internal/orchestrator/office_stall_metrics.go` for the label + idiom and the counters-only rationale. +- `CLAUDE.md`, Observability section. + +## Results + +- Added `internal/office/dashboard/session_termination_metrics.go` with + `office_session_term_suppressed_total`, following the + `"k1=v1;k2=v2"` expvar label idiom used by the office scheduler and the + stall detectors. +- Labels are `reason` (three values) and `outcome` (`runner`, `seat`, + `read_failed`), so a read fault is countable apart from a genuinely retained + capacity. Both increments are issued from the single guarded termination + helper, so no call site can forget one. +- `TestSuppressedTermination_IsCounted` asserts one increment per cause using + before/after deltas, since expvar maps are process-global and shared across + the package's tests. `TestSuppressionLabels_AreBounded` scans every label the + package produced, rejecting any that carries an identifier, has other than + two dimensions, or names a value outside the closed sets. It fails rather + than passing vacuously when no label was recorded. +- Documented the counter and both label dimensions in `CLAUDE.md`'s + Observability section, beside `office_stall_*` and `routing_*`. +- `go build ./...` passes, `internal/office/dashboard` passes with `-race`, and + `golangci-lint` reports 0 issues for the package. diff --git a/docs/specs/office/requirements/seat-claim-decision-guard.md b/docs/specs/office/requirements/seat-claim-decision-guard.md new file mode 100644 index 0000000000..59333042f0 --- /dev/null +++ b/docs/specs/office/requirements/seat-claim-decision-guard.md @@ -0,0 +1,195 @@ +--- +status: draft +system: office +created: 2026-09-08 +owners: + - kandev +--- + +# Office Seat Claim Decision Guard Requirements + +## Overview + +`AC-OFFICE-SEAT-PROVENANCE-002.5` requires that a decision already recorded +against an agent is never reattributed to a different agent by a later +registration. Two mechanisms defend it in the shipped code, and they defend +different windows. + +The first is a filter applied while a registration is choosing which seat to +claim: a seat that already carries a decision is not selected. It runs inside +the registration's transaction and covers every decision that was committed +before the registration looked. + +The second is a condition on the update that actually reassigns the seat, +requiring that no decision exists against it at the moment of the write. It +covers only the window *between* those two points - a decision that commits +after the registration chose the seat and before it reassigned it. + +That window is now closed for every decision the shipped product records. +Decision recording serializes on the same per-`(task, role)` exclusion the +registration holds for its whole transaction, so a decision carrying a role +cannot commit inside another writer's window. The exclusion is acquired only +when the decision carries a role, and the decision store deliberately supports +recording one without: the only validation on that path requires a task, a +step, a participant and a verdict. A decision with no role skips the exclusion +entirely, and for that decision the condition on the update is the only thing +standing between a recorded verdict and its silent reattribution to whoever +registers next. + +No shipped caller produces a decision without a role - both entry points +resolve one, or refuse - so this is a latent path, not a live defect. It is +worth stating because the guard defending it is currently documented as +something else. The concurrency test covering this behavior asserts that the +two writers lock on non-contending namespaces and that the condition on the +update is therefore load-bearing. The first half stopped being true when the +shared exclusion was introduced, and the second half is now true only on the +path the test does not exercise. A test that cannot fail is worse than no test, +because it is counted. + +Office owns this contract: it owns the registration path, the decision store's +office-facing entry points, and the exclusion the two share. + +## Terminology + +- **Registration** - a request to seat a named agent profile in a participant + role on a task, which may claim an existing automatically-cast seat or write + a new one. +- **Claimable seat** - the sole automatically-cast, undecided seat in a role's + slate, which a registration may reassign to the registering agent. +- **Seat exclusion** - the per-`(task, role)` mutual exclusion that automatic + casting, registration and role-carrying decision recording all acquire, held + for the duration of the acquiring transaction. +- **Roleless decision** - a decision recorded without a participant role. The + decision store accepts one; it supersedes prior decisions by participant + rather than by decider and role, and does not acquire the seat exclusion. +- **Reattribution** - a seat carrying a recorded decision coming to name an + agent other than the one whose decision is on file, so that the verdict reads + as having been given by an agent that did not give it. + +## Requirements + +### REQ-OFFICE-SEAT-GUARD-001: A recorded decision survives a concurrent claim + +**Intent:** `AC-OFFICE-SEAT-PROVENANCE-002.5` must hold on every path that can +record a decision, including the one that does not participate in the seat +exclusion. The shipped code already satisfies this requirement; it is written +down because nothing states it, nothing tests it, and the code comment +explaining the mechanism describes a different one. + +#### Acceptance criteria + +- **AC-OFFICE-SEAT-GUARD-001.1:** When a decision exists against a seat, a + registration shall not reassign that seat to another agent, whether or not + that decision carries a participant role. +- **AC-OFFICE-SEAT-GUARD-001.2:** When a roleless decision against a seat + commits after a registration has selected that seat as claimable and before + that registration reassigns it, the registration shall reassign no seat and + shall write a new seat for the registering agent instead. +- **AC-OFFICE-SEAT-GUARD-001.3:** The behavior required by + `AC-OFFICE-SEAT-GUARD-001.2` shall not depend on the decision writer and the + registration serializing on the seat exclusion, because the roleless decision + path does not acquire it. An implementation that removes the condition on the + reassigning update and relies on the exclusion alone does not satisfy this + requirement. +- **AC-OFFICE-SEAT-GUARD-001.4:** When a registration declines to reassign a + seat under `AC-OFFICE-SEAT-GUARD-001.2`, it shall leave that seat's agent + profile, provenance, decision-required flag, position and creation time + unchanged, and shall leave the decision itself unchanged. +- **AC-OFFICE-SEAT-GUARD-001.5:** A registration that declines to reassign + under `AC-OFFICE-SEAT-GUARD-001.2` shall report the same outcome, and produce + the same effects, as a registration that found no claimable seat at all: a + new seat is written, and none of the effects a claim earns - the claim + activity record, the displaced agent's session termination, the displaced + agent's queued run cancellation - occurs. Nothing was displaced, so nothing + shall be reported as displaced. +- **AC-OFFICE-SEAT-GUARD-001.6:** A registration shall not be failed by the + condition in `AC-OFFICE-SEAT-GUARD-001.2`. Finding the seat decided is a + normal outcome and shall be reported as success. +- **AC-OFFICE-SEAT-GUARD-001.7:** This requirement adds no acceptance criterion + to the participant-seat-provenance contract and amends none of its criteria. + It names the path on which `AC-OFFICE-SEAT-PROVENANCE-002.5` is defended by + the condition on the update alone. +- **AC-OFFICE-SEAT-GUARD-001.8:** Any decision against the seat shall block the + reassignment, whether or not it has been superseded, and whether or not more + than one exists. Both defenses shall agree on this: a seat the selection + rejects as decided shall not be one the update would accept, and the reverse. + Superseded decisions are retained as the audit trail of a reworked review, and + a seat that has ever been decided has an attribution history that reassigning + it would falsify. +- **AC-OFFICE-SEAT-GUARD-001.9:** A registration that declined to reassign under + `AC-OFFICE-SEAT-GUARD-001.2` shall leave the registering agent seated by the + new seat it wrote, so a repeat of the same registration is governed by the + existing already-seated rule and writes nothing further. This document adds no + retry and no second attempt at the claim. + +### REQ-OFFICE-SEAT-GUARD-002: The guard is verified by a test that fails without it + +**Intent:** A guard whose test passes when the guard is deleted is not covered. +The suite must contain at least one test that distinguishes the two, and the +tests that cannot must stop claiming they do. + +#### Acceptance criteria + +- **AC-OFFICE-SEAT-GUARD-002.1:** The test suite shall contain a test that + fails when the condition required by `AC-OFFICE-SEAT-GUARD-001.3` is removed + from the reassigning update, and that passes when it is present. +- **AC-OFFICE-SEAT-GUARD-002.2:** That test shall reach the window described in + `AC-OFFICE-SEAT-GUARD-001.2` deterministically. Its result shall not depend + on thread or transaction scheduling, and it shall not be capable of passing + by never entering the window it targets. +- **AC-OFFICE-SEAT-GUARD-002.3:** Any mechanism introduced to make + `AC-OFFICE-SEAT-GUARD-002.2` deterministic shall have no effect in a + production build: with the mechanism unset, the code path shall behave + exactly as it does today. +- **AC-OFFICE-SEAT-GUARD-002.4:** A test that establishes that the two writers + serialize on the seat exclusion shall say that, and shall assert an outcome + that serialization can produce. It shall not document itself as establishing + that the condition on the update is load-bearing, which serialization + prevents it from showing. +- **AC-OFFICE-SEAT-GUARD-002.5:** No test documentation in the suite shall + assert that role-carrying decision recording and registration lock on + non-contending exclusions. That premise is false and is the reason + `AC-OFFICE-SEAT-GUARD-002.4` is needed. + +## Out of scope + +- **Removing the condition on the reassigning update.** Named because the + premise that it is unreachable is the tempting conclusion from the shared + exclusion, and it is wrong: `AC-OFFICE-SEAT-GUARD-001.3` requires it to stay, + and the roleless decision path is why. +- **Making the roleless decision path acquire the seat exclusion.** This is the + other way to close the window, and it is deliberately not taken here. The + path has no role to key the exclusion on and would have to derive one from + the seat it references, turning a write into a read-then-lock and widening + decision recording's contention for a path no shipped caller uses. The + condition on the update already delivers the outcome + `AC-OFFICE-SEAT-GUARD-001.2` requires. Recorded so the trade is on the record + rather than an omission. +- **Rejecting roleless decisions.** Whether the decision store should require a + role at all is a separate contract question about the decision model. This + document takes the store's accepted input shapes as given and requires the + seat behavior to be correct for all of them. +- **The concurrency test covering a claim target removed mid-transaction.** A + sibling follow-up owns that test and its own determinism gap. If a shared + mechanism satisfies `AC-OFFICE-SEAT-GUARD-002.3` for both, that is a welcome + outcome and not a requirement of this document, which constrains only the + decision guard's own coverage. +- **Any user-visible surface.** No behavior change ships to a user from this + document: `AC-OFFICE-SEAT-GUARD-001` records what the code already does. No + copy, no i18n, no UI work. + +## Prior art + +The prior-art legs for this card are recorded once, in +[Office Task Session Termination Requirements](task-session-termination.md#prior-art), +with their receipts. Both external legs were unavailable on this runner; the +in-repo prior decisions that applied to this document are +`AC-OFFICE-SEAT-PROVENANCE-002.5`, which this requirement defends without +amending, and the shared seat exclusion introduced to close the role-carrying +decision race, whose own documentation records that the two writers previously +never contended. + +## System design + +The technical design is +[part 1](../system-design/seat-claim-decision-guard-01.md). diff --git a/docs/specs/office/requirements/task-session-termination.md b/docs/specs/office/requirements/task-session-termination.md new file mode 100644 index 0000000000..c73efacd05 --- /dev/null +++ b/docs/specs/office/requirements/task-session-termination.md @@ -0,0 +1,354 @@ +--- +status: draft +system: office +created: 2026-09-08 +owners: + - kandev +--- + +# Office Task Session Termination Requirements + +## Overview + +An office session is keyed on a `(task_id, agent_profile_id)` pair. +REQ-OFFICE-SESSION-IDENTITY-001 makes that pair name exactly one live durable +conversation, and `AC-OFFICE-SESSION-IDENTITY-001.8` states the consequence +explicitly: the office side treats the pair as one conversation *regardless of +which path created the row*. Nothing in that contract says when the +conversation ends. + +Three paths end it today, and all three end it on the loss of a single +capacity: a participant role is removed by hand, a seat is taken from its +occupant by a manual claim, or the task is reassigned. Each flips the one +shared session row to `COMPLETED` without asking whether the agent still holds +another capacity on the same task. + +That question is not hypothetical. `AC-OFFICE-REVIEW-SEATS-002.4`, `-002.5` +and `-002.9` each seat the task's runner, or an agent already seated in another +role, as a reviewer. An agent holding two capacities at once is a normal, +fallback-default outcome of the shipped casting rules, and when such an agent +loses one of them the current behavior ends the session it is still using for +the other. + +In the configuration every profile ships (`features.officeSessionIdentity` +disabled), the defect is sharper than "sometimes over-eager". With that flag +off, a task that has a runner routes *every* office run through the session +owned by the runner, so no session is keyed on a non-runner participant at all. +A termination aimed at a displaced or removed non-runner therefore finds +nothing and does nothing, and a termination aimed at an agent that *is* the +runner finds the task's only live session. On the role-removal and seat-claim +paths, the operation is either a silent no-op or the exact case this document +forbids. Enabling the flag gives each agent its own row and removes the no-op +half, but not the defect: the self-review agent still holds one row for both +capacities. + +Office owns this contract because office owns the rule that a pair names one +conversation, and owns all three paths that end one. The task system owns the +session row's storage and its non-office lifecycle, which this document leaves +untouched. + +## Terminology + +- **Capacity** - a distinct standing an agent holds on a task that causes the + system to address it there. Two exist: the runner capacity and a seat + capacity. +- **Runner capacity** - the agent is the task's effective runner, as that is + resolved for every other reader of the task's runner. +- **Seat capacity** - the agent holds a seat in the task's effective + participant slate, in any role. +- **Effective participant slate** - the seats a reader of the task observes at + the task's current step, after per-task rows take precedence over + template-level rows. It is the same slate the task's own readers see; this + document does not define a second one. +- **Guarded path** - one of the three paths this document constrains: manual + removal of a participant role, displacement of a seat's occupant by a claim, + and reassignment of the task away from its runner. +- **Cascade path** - a path that ends every live session belonging to an agent + across every task, because the agent instance itself is going away. +- **Live** and **terminal** - as defined by + REQ-OFFICE-SESSION-IDENTITY. This document adds no state and redefines + neither. + +## Prior art + +**Our own prior reasoning (wiki):** the leg did not run, and no result is +reported for it. Receipt: the `wiki-query` skill is not installed on this +runner (absent from `~/.claude/skills/` and from the task-local +`.claude/skills/`), `OBSIDIAN_VAULT_PATH` is unset, `~/.obsidian-wiki/` does +not exist, no vault directory exists under `~`, and `qmd` is not on `PATH`, so +neither the QMD path nor the documented `grep` fallback had a vault to resolve. +Recorded as unavailable rather than as an empty result. + +**What others shipped (saas-kb):** the leg did not run. Receipt: no `saas-kb` +MCP server is exposed to this session - the only MCP tools present are the +`kandev` server's - so `search_fsm_docs` could not be called with or without +`category: "ai_sdlc"`. Recorded as unavailable, not as a search that found +nothing. + +**In-repo prior decisions**, which did apply and which carried the weight the +two unavailable legs would have: + +- **REQ-OFFICE-SESSION-IDENTITY-001 and -004** forecloses the obvious fix. + Making session identity role-scoped would add a role dimension to the pair + that -001 has just finished making singular, and + `AC-OFFICE-SESSION-IDENTITY-004.5` forbids the new column such a dimension + would need. The card that raised this defect offered role-scoped identity or + a retained-capacity guard as alternatives; that contract decides between + them, and this document takes the guard. +- **`AC-OFFICE-SEAT-PROVENANCE-002.12`** already scopes the claim path's + termination to an agent "holding a live session for that task **in that + role**". The implementation is broader than its own frozen criterion, because + the session model cannot express "in that role". This document does not amend + that criterion; it makes the behavior conform to what the criterion already + says. +- **`AC-OFFICE-REVIEW-SEATS-002.3`** already treats an agent holding two + capacities as undesirable-but-permitted, preferring a candidate that is + "neither the task's runner nor already seated in another participant role" + and falling back when none exists. The fallback is what makes this defect + reachable, and it is deliberate, so the fix belongs on the termination side + rather than in casting. +- **ADR 0005 (agent model unification)** made `agent_profile_id` shared between + kanban and office, which is why the capacity question must be answered on the + office code path rather than by a constraint on the shared row. + +**What we are doing differently:** the shape this codebase reaches for when a +lifecycle event is over-broad is a narrower key. That is rejected here for the +first reason above, and the alternative is unusual enough to name: the key stays +as wide as it is and the *decision to act on it* acquires a precondition. +Ending the row becomes conditional on "this agent's one conversation about this +task" having stopped being true, rather than on one reason to suspect it might +have. + +## Requirements + +### REQ-OFFICE-SESSION-TERM-001: A session ends only with the last capacity + +**Intent:** A guarded path removes one capacity. It must end the agent's +session only when that was the agent's last one, so an agent that is still +addressed on the task keeps the conversation it is still using. + +**User story:** As an operator claiming a review seat from the agent that is +also writing the code, I want that agent to keep working, so that taking over +the review does not silently stop the task. + +#### Acceptance criteria + +- **AC-OFFICE-SESSION-TERM-001.1:** When a guarded path would end an agent's + office session for a task, the system shall determine whether that agent + retains a capacity on that task, and shall end the session only when it + retains none. +- **AC-OFFICE-SESSION-TERM-001.2:** The determination shall observe state in + which the mutation that removed the capacity has already committed, so the + capacity being removed is never itself observed as retained. A determination + made before that commit would suppress every termination and satisfies + nothing in this document. +- **AC-OFFICE-SESSION-TERM-001.3:** When the agent retains at least one + capacity, the system shall leave the session row entirely unwritten - its + state, its reason and every other field unchanged - and shall report success + to the guarded path's caller. A suppressed termination is not an error and + shall not be reported as one. +- **AC-OFFICE-SESSION-TERM-001.4:** When the agent retains no capacity, the + system shall end the session exactly as it does today: the row becomes + `COMPLETED` carrying the reason that path already supplies, and no other + observable behavior of that path changes. +- **AC-OFFICE-SESSION-TERM-001.5:** The determination shall have exactly one + implementation in application code, reachable from all three guarded paths, + and the system shall not introduce a second. Three copies that agree today + are three copies that can disagree later, which is the failure this criterion + exists to prevent. +- **AC-OFFICE-SESSION-TERM-001.6:** The guarded paths shall be exactly three: + removal of a participant role from a task, displacement of a seat's occupant + by a claim on that seat, and reassignment of a task away from its runner. The + system shall not extend the determination to a path not named here without + amending this criterion. +- **AC-OFFICE-SESSION-TERM-001.7:** If the determination cannot be completed + because the state it reads is unavailable, then the system shall not end the + session, shall record the failure under REQ-OFFICE-SESSION-TERM-004, and + shall report success to the guarded path's caller. The determination fails + closed: ending the session of an agent that is still working is the defect + being removed, and leaving a session row live is recoverable by the next + guarded path that runs, whereas ending one is not. +- **AC-OFFICE-SESSION-TERM-001.8:** When either the task identifier or the + agent profile identifier is empty, the system shall neither evaluate the + determination nor end a session, and shall report success. No pair is named, + so there is nothing to decide. +- **AC-OFFICE-SESSION-TERM-001.9:** When no live session exists for the pair, + the observable outcome shall be identical whether or not the determination + was evaluated. The system may skip it, and a test shall not depend on whether + it did. +- **AC-OFFICE-SESSION-TERM-001.10:** When two guarded paths run concurrently + for the same agent and task, each removing a different capacity, the system + shall end the session. This follows from + `AC-OFFICE-SESSION-TERM-001.2` without further coordination: each path reads + strictly after its own commit, so whichever commits second observes both + removals and finds no retained capacity. The system shall not add a lock, a + retry or a re-read to obtain this outcome, and shall not rely on which path + reads first. +- **AC-OFFICE-SESSION-TERM-001.11:** The system shall not end a session that + the same guarded path has already ended, and repeating a guarded path's + termination step for a pair shall produce no further write. This preserves + the existing idempotency of the termination step unchanged. + +### REQ-OFFICE-SESSION-TERM-002: What counts as a retained capacity + +**Intent:** The determination must ask a question the rest of the system +already answers the same way, and must ask it over a scope that cannot pick up +a capacity the agent no longer holds. + +#### Acceptance criteria + +- **AC-OFFICE-SESSION-TERM-002.1:** The system shall treat an agent as + retaining a capacity on a task when it is the task's effective runner, or + when it holds a seat in the task's effective participant slate, and shall + treat it as retaining none otherwise. The two are alternatives, not an + ordered test: neither takes precedence and the determination shall not depend + on which is evaluated first. +- **AC-OFFICE-SESSION-TERM-002.2:** The system shall resolve the task's + effective runner by the same rule every other reader of that task's runner + uses, so that the determination and the rest of the system cannot disagree + about who the runner is. It shall not resolve the runner by reading + participant seats directly, which would answer a different question: seats + written for a step the task has since left remain readable, and the runner + rule already fixes their precedence. +- **AC-OFFICE-SESSION-TERM-002.3:** The seat scope shall be the task's + effective participant slate at the task's current step. A seat recorded at a + step the task is not standing on shall not, on its own, count as a retained + capacity. Without this bound a task that has moved through steps accumulates + seats naming a previous occupant, and a termination that is correct would be + suppressed forever. +- **AC-OFFICE-SESSION-TERM-002.4:** A template-level seat that the slate + projects onto the task shall count exactly as a per-task seat does. The agent + is addressed on the task either way, which is the only thing the + determination asks. +- **AC-OFFICE-SESSION-TERM-002.5:** Every participant role shall count, + including a role that carries no decision obligation. The determination asks + whether the agent is still addressed on the task, not whether it still owes a + decision. +- **AC-OFFICE-SESSION-TERM-002.6:** The determination shall depend on the + presence of a capacity and not on how many the agent holds. An agent holding + two seats and an agent holding one shall be treated identically. +- **AC-OFFICE-SESSION-TERM-002.7:** The determination shall not depend on the + order in which seats are returned, and shall produce the same result for any + ordering of the same slate. +- **AC-OFFICE-SESSION-TERM-002.8:** When the task has no current step, the + effective slate shall be empty and only the runner capacity shall be capable + of being retained. When the task does not resolve at all, the determination + shall be treated as having failed under `AC-OFFICE-SESSION-TERM-001.7` rather + than as having found no capacity. +- **AC-OFFICE-SESSION-TERM-002.9:** The determination shall compare agent + profile identifiers and shall not require the named profile to resolve to a + live agent. A seat naming a profile that no longer resolves still names this + agent, and an agent instance that is genuinely going away is handled by a + cascade path instead. +- **AC-OFFICE-SESSION-TERM-002.10:** The determination shall produce the same + result on every database engine the product supports. +- **AC-OFFICE-SESSION-TERM-002.11:** The determination selects nothing. It + answers whether any capacity is held, so no ordering, tiebreak or + deterministic choice among capacities is required, and the system shall not + introduce one. Which capacity is named in `AC-OFFICE-SESSION-TERM-004.1`'s + record may be any that was found. +- **AC-OFFICE-SESSION-TERM-002.12:** When a capacity is granted to the agent + between the guarded path's commit and the determination's read, the system + shall observe it and suppress the termination. An agent that has just been + seated again is addressed on the task, and the criterion asks about the + present, not about the state at the moment the capacity was lost. + +### REQ-OFFICE-SESSION-TERM-003: What this contract does not change + +**Intent:** This is a narrow precondition on three call sites. It is stated +negatively as well as positively so that an implementation cannot satisfy it by +a mechanism that also reaches the paths that must keep working, and so that the +inconsistency it deliberately leaves in place is not "fixed". + +#### Acceptance criteria + +- **AC-OFFICE-SESSION-TERM-003.1:** A cascade path shall remain unguarded and + shall continue to end every live session belonging to the agent across every + task. An agent instance being deleted or reconciled away retains no capacity + anywhere by definition, so evaluating the determination there would be a cost + with no possible effect. +- **AC-OFFICE-SESSION-TERM-003.2:** On the reassignment path, cancellation of + the previous runner's *running execution* shall be unchanged, and shall + continue to happen whether or not the session row's termination is + suppressed. The two are deliberately decoupled: the agent must stop the work + it is no longer the runner for, and must keep the conversation it is still + seated for. An implementation that suppresses the cancellation alongside the + termination does not satisfy this document. +- **AC-OFFICE-SESSION-TERM-003.3:** Session creation, reuse, lookup and + identity shall be unchanged. The system shall introduce no new column on the + session store, no new index on it, and no backfill or repair of existing + rows. +- **AC-OFFICE-SESSION-TERM-003.4:** A suppressed termination shall change + exactly one thing relative to today: the session row is not written. It shall + not withhold, delay, duplicate or reorder any participant seat write, + decision, queued or cancelled run, activity entry or change notification that + its guarded path performs, and shall not itself wake, queue or re-queue + anything. +- **AC-OFFICE-SESSION-TERM-003.5:** The behavior required by + REQ-OFFICE-SESSION-TERM-001 and -002 shall hold identically whether + `features.officeSessionIdentity` is enabled or disabled. The defect is + reachable in both states and the fix shall not be conditioned on either. +- **AC-OFFICE-SESSION-TERM-003.6:** No participant casting, seat provenance, or + claim behavior shall change. This document adds no acceptance criterion to + the participant-seat-provenance contract and amends none of its criteria. + +### REQ-OFFICE-SESSION-TERM-004: A suppressed termination is observable + +**Intent:** The new outcome is a session that stays live where one used to end. +Left unrecorded it is indistinguishable from a termination that failed, from a +path that never ran, and from the leak a mistake in +`AC-OFFICE-SESSION-TERM-002.3` would produce. + +#### Acceptance criteria + +- **AC-OFFICE-SESSION-TERM-004.1:** When a termination is suppressed because + the agent retains a capacity, the system shall emit a record identifying the + task, the agent profile, the guarded path's reason, and which capacity was + retained. +- **AC-OFFICE-SESSION-TERM-004.2:** When a termination is suppressed because + the determination failed, the system shall emit a record that is + distinguishable from `AC-OFFICE-SESSION-TERM-004.1`'s and that identifies the + failure. A read fault and a retained capacity are different events and shall + not be reported as one. +- **AC-OFFICE-SESSION-TERM-004.3:** The system shall increment a counter for + each suppression, labelled only by bounded dimensions. It shall not label a + counter with a task, agent, step or session identifier. +- **AC-OFFICE-SESSION-TERM-004.4:** A termination that proceeds shall continue + to record what it records today, with the same reason values. This document + adds no reason value and retires none. + +## Out of scope + +- **Role-scoped session identity.** Named because the card that raised this + defect offered it as the alternative fix, and because it is the shape a + reader will reach for first. It would add a role dimension to the + `(task, agent)` pair that REQ-OFFICE-SESSION-IDENTITY-001 exists to make + singular, and would need a discriminator column + `AC-OFFICE-SESSION-IDENTITY-004.5` forbids. Rejected here, not deferred: the + guard in REQ-OFFICE-SESSION-TERM-001 delivers the same observable outcome + with no change to session identity at all. +- **Repairing sessions already ended by the current behavior.** No migration, + backfill or repair is authorized by this document. A session wrongly + completed before this contract ships stays completed; the pair's next office + wakeup creates a fresh row by the existing rules, which is the recovery that + already exists. +- **Ending a session when an agent loses its last capacity by a path not named + in `AC-OFFICE-SESSION-TERM-001.6`.** An agent can stop being addressed on a + task in ways none of the three guarded paths observes - a step transition + that changes the slate, for one. Those paths end no session today and this + document does not make them start: the scope is the over-eager termination + that exists, not an audit of session lifetime. +- **The `features.officeSessionIdentity` rollout.** Whether that flag is + promoted, and what its enablement requires, is its own decision. + `AC-OFFICE-SESSION-TERM-003.5` only requires this contract to be indifferent + to it. +- **The casting rules that make an agent hold two capacities.** + `AC-OFFICE-REVIEW-SEATS-002.3` through `-002.9` are deliberate and unchanged. +- **Any user-visible surface.** The observable consequence is that a session + stays in the live participant indicators the frontend already renders from + existing session state. No new copy, no new component, no i18n work. + +## System design + +The technical design is +[part 1](../system-design/task-session-termination-01.md). diff --git a/docs/specs/office/system-design/seat-claim-decision-guard-01.md b/docs/specs/office/system-design/seat-claim-decision-guard-01.md new file mode 100644 index 0000000000..9cb8255ff3 --- /dev/null +++ b/docs/specs/office/system-design/seat-claim-decision-guard-01.md @@ -0,0 +1,180 @@ +--- +status: draft +system: office +requirements: + - REQ-OFFICE-SEAT-GUARD-001 + - REQ-OFFICE-SEAT-GUARD-002 +--- + +# Office Seat Claim Decision Guard System Design + +## Purpose and boundaries + +Office owns the registration path that claims a seat, and the office-facing +entry points that record a decision. This design changes no production +behavior: it records which of two overlapping defenses covers which window, and +makes the one that is currently untested testable. + +Contracts used but not owned: the workflow repository's decision store and the +per-`(task, role)` seat exclusion it shares with registration. + +## Requirement mapping + +| Requirement | Design section | +| --- | --- | +| `REQ-OFFICE-SEAT-GUARD-001` | [Two defenses, two windows](#two-defenses-two-windows) | +| `REQ-OFFICE-SEAT-GUARD-002` | [Making the window reachable](#making-the-window-reachable), [Verification obligations](#verification-obligations) | + +## Two defenses, two windows + +A registration that finds a claimable seat runs two statements inside one +transaction: a select that chooses the seat, and an update that reassigns it. + +The **select** rejects a seat that already carries a decision. It is a plain +read inside the registration's transaction and it sees everything committed +before it ran. + +The **update** carries a `NOT EXISTS` condition over decisions against the same +seat, and re-checks the seat's provenance. It can only add something the select +did not already provide when a decision commits *between* the two statements. + +Whether anything can commit there is entirely a question about the seat +exclusion: + +- **A decision carrying a role** acquires the exclusion, which the registration + holds for its whole transaction. It cannot commit in the window. The update's + condition is unreachable through this path, and a test that races this path + cannot distinguish the condition being present from its being absent. +- **A decision carrying no role** does not acquire the exclusion. The decision + writer's own code branches on the role being empty - it supersedes by + participant instead of by decider-and-role, and skips the lock with the + reasoning that there is nothing to serialize against. That reasoning is + incomplete: there is something, and it is this window. For this path the + update's condition is the whole defense. + +Neither defense filters on `superseded_at`, and `AC-OFFICE-SEAT-GUARD-001.8` +requires that they keep agreeing. A seat that has ever been decided stays +unclaimable: the superseded rows are the audit trail of a reworked review, and +reassigning a seat that carries one would leave a verdict attributed to an agent +that did not give it. Adding a `superseded_at IS NULL` filter to either +statement would make the two disagree, and would make a reworked seat claimable +by whoever registers next. + +Because the condition matches on the seat identity rather than on the +decision's role, it covers the roleless decision without modification. Nothing +in the production code needs to change for +`REQ-OFFICE-SEAT-GUARD-001` to hold - which is why that requirement is written +as a statement of existing behavior with the reachability named, not as a +change order. + +No shipped caller produces a roleless decision: both office entry points +resolve a role first and refuse the request when they cannot. The path is +supported by the store and unused by the product, which is the precise sense in +which this is latent. + +## Making the window reachable + +`AC-OFFICE-SEAT-GUARD-002.2` requires the test to enter the window +deterministically. Two approaches were considered. + +**Racing real goroutines** on the roleless path would genuinely reach the +window, because that path takes no exclusion. It is rejected: nothing forces +the interleaving, so the test would pass most of the time by never entering the +window at all - which is the same defect being fixed here, reintroduced in a +different test. A sibling follow-up already records this failure mode against a +neighbouring concurrency test. + +**A test-only interleaving hook** is the design. An unexported package-level +function value in the office repository's sqlite package, called between the +select and the update, is nil in every build that does not set it. The +test-only `SetClaimWindowHook` setter in `export_test.go` makes the yield point +available to the external `sqlite_test` package. The SQLite test writes the +roleless decision through the in-flight transaction. The Postgres-gated test +uses the workflow repository from the hook, so that decision commits through a +second pooled connection before the update runs. + +Three properties make this acceptable rather than a production concession, and +they map to `AC-OFFICE-SEAT-GUARD-002.3`: + +- The variable is unexported, and its setter is compiled only into the test + binary, so production code cannot set it and it needs no build tag. +- Unset, the call site is a nil check on a value that is never written, so the + production path is byte-for-byte the behavior that ships today. +- It carries no behavior of its own. It is a yield point, not a policy hook, + and nothing production reads it. + +The hook is placed in the registration's claim attempt, between the statement +that chooses the seat and the one that reassigns it. That is the only window +the requirement is about. + +If the sibling follow-up's claim-target-removed test wants the same yield +point, it is the same hook at the same site. That is a welcome economy and not +a dependency in either direction - neither card's tests need the other's. + +## Data and contracts + +No schema change, no production interface change, and no production export. The +test binary exposes only the setter needed by the external-package tests. The +condition on the update, the select's filter and the seat exclusion all stay +exactly as they are. + +## Control flow + +Unchanged in production. Under test, one nil-valued call site between two +existing statements of an existing transaction. The SQLite test uses that +transaction directly; the Postgres test uses a separate pooled connection to +prove a real cross-transaction commit. + +## Failure and recovery + +Unchanged. A registration that finds the seat decided at update time affects +zero rows, falls through to inserting a new seat, and reports success - the +existing fallthrough, which `AC-OFFICE-SEAT-GUARD-001.5` and `-001.6` describe +rather than modify. Nothing is retried and nothing is failed. + +## Persistence + +No schema change, no migration, no backfill. The registration's transaction +boundaries and the exclusion it acquires are untouched. + +## Security + +No new surface. The hook is unexported, unset in production, and carries no +input from any request. + +## Observability + +Nothing new. The declined-claim outcome is already indistinguishable from "no +claimable seat existed" by design (`AC-OFFICE-SEAT-GUARD-001.5`), and this +design does not make it distinguishable - a new signal there would report a +path no shipped caller reaches. + +## Verification obligations + +Three changes to the suite, and one of them is a deletion of a claim rather +than of a test: + +1. **The new guard test** (`-002.1`, `-002.2`). Drives a registration with the + hook set, writes a roleless decision against the chosen seat inside the + window, and asserts the decision and seat are unchanged while a new seat is + written for the registering agent. The SQLite test writes through the open + transaction; the Postgres-gated variant commits from a second connection. + Removing the `NOT EXISTS` condition must make these tests fail; that is the + acceptance check for the tests themselves, and should be performed once by + hand when they are written. +2. **The existing concurrency test's documentation** (`-002.4`, `-002.5`). Its + stated premise - that decision recording and registration lock on + non-contending namespaces - is false since the shared exclusion landed. It + is re-documented as establishing what it can: that the two writers serialize + on the shared exclusion, complete without deadlock in either acquisition + order, and leave no reattributed decision. The test's assertions already + support that reading; only its docstring asserts more than it shows. +3. **No test is deleted or weakened.** The existing concurrency test keeps its + iterations and its multi-connection pool - serialization under real + concurrency is worth covering, and the deadlock question is real, since the + decision path acquires two exclusions in a fixed order and the registration + path acquires one of them. + +## Related decisions + +- [ADR 0005 - agent model unification](../../../decisions/0005-agent-model-unification.md) diff --git a/docs/specs/office/system-design/task-session-termination-01.md b/docs/specs/office/system-design/task-session-termination-01.md new file mode 100644 index 0000000000..7303cd66b6 --- /dev/null +++ b/docs/specs/office/system-design/task-session-termination-01.md @@ -0,0 +1,227 @@ +--- +status: draft +system: office +requirements: + - REQ-OFFICE-SESSION-TERM-001 + - REQ-OFFICE-SESSION-TERM-002 + - REQ-OFFICE-SESSION-TERM-003 + - REQ-OFFICE-SESSION-TERM-004 +--- + +# Office Task Session Termination System Design + +## Purpose and boundaries + +Office owns the rule that a `(task, agent)` pair names one durable conversation +and owns the three paths that end one. This design adds a precondition to those +three paths and changes nothing else. + +Contracts this design uses but does not own: the session row's storage and +state machine (task system); the participant seat store and the exclusion that +protects it (workflow system); the effective-runner resolution shared by every +reader of a task's runner (office task projection). None of the three is +modified. + +## Requirement mapping + +| Requirement | Design section | +| --- | --- | +| `REQ-OFFICE-SESSION-TERM-001` | [Where the precondition lives](#where-the-precondition-lives), [Control flow](#control-flow) | +| `REQ-OFFICE-SESSION-TERM-002` | [The capacity question](#the-capacity-question) | +| `REQ-OFFICE-SESSION-TERM-003` | [Control flow](#control-flow) | +| `REQ-OFFICE-SESSION-TERM-004` | [Observability](#observability) | + +## Components and responsibilities + +- **The office dashboard service** owns the three guarded call sites + (`internal/office/dashboard/service_tasks.go`) and gains one new private + helper that answers the capacity question. It already holds the office + repository, which is the only component that can answer it. +- **The orchestrator's session terminator** + (`internal/orchestrator/office_session_terminator.go`) is unchanged. It stays + the dumb executor of "flip this pair's row to `COMPLETED`, idempotently", and + keeps both its methods and its two interfaces exactly as they are. +- **The office repository** supplies the two reads the question needs. Both + already exist: the task's execution fields, whose assignee field is the + shared effective-runner projection, and the task's effective participant + slate at its current step. + +## Where the precondition lives + +The guard goes in the dashboard service, immediately before each +`TerminateOfficeSession` call, not inside the terminator. + +The terminator is in `internal/orchestrator` and reaches the session repository +only. The capacity question needs participant seats and the runner projection, +both owned by `internal/office/repository/sqlite`, which the orchestrator does +not and should not import - the `SessionTerminator` interfaces exist precisely +so office does not import orchestrator and orchestrator does not import office. +Pushing the question down would either invert that dependency or widen the +interface with a callback, for no gain. + +Putting it in the service also keeps the two cascade paths correct by +construction. `TerminateAllForAgent` is a different method on the same type +with different callers (agent deletion, config-sync reconciliation), and +`AC-OFFICE-SESSION-TERM-003.1` requires it to stay unguarded. A guard inside +the shared terminator would have to be argued out of that method; a guard in +the dashboard service never reaches it. + +`AC-OFFICE-SESSION-TERM-001.5` requires one implementation. The three call +sites are three lines in one file calling one unexported helper on the same +receiver; there is no second copy to drift. + +## The capacity question + +One helper, one signature: given a task and an agent profile, does the agent +still hold a capacity on that task? + +**The runner capacity** is read from the task's execution fields, whose +assignee value is produced by the shared runner projection +(`RunnerProjection`). `AC-OFFICE-SESSION-TERM-002.2` requires this rather than +a direct read of runner seats, and the reason is specific: that projection has +three precedence tiers, and its last tier deliberately reads runner rows across +*all* steps, ordered by creation time. A hand-written "is there a runner seat +naming this agent" query would therefore disagree with the projection exactly +when a task has moved between steps - which is the case the guard is most +likely to meet. Reusing the projection makes disagreement impossible instead of +unlikely. + +**The seat capacity** is read from the task's effective slate at its current +step (`ListAllTaskParticipants`), which already applies per-task precedence +over template-level rows and already scopes to the current step. That scoping +is what `AC-OFFICE-SESSION-TERM-002.3` requires and it is load-bearing: seats +are keyed `(step, task, role, agent)` and are not cleaned up when a task +leaves a step, so an unscoped read would find a seat naming a previous occupant +and suppress a correct termination permanently. The slate read is also what +`AC-OFFICE-SESSION-TERM-002.4` and `-002.5` fall out of: it already merges +template rows in, and it already returns every role. + +Neither read is ordered-dependent (`-002.7`): the helper asks whether any +returned row names the agent, and stops. Both are plain reads on the read pool, +taking no lock and joining no transaction - the guard is not trying to be +atomic with anything, and `AC-OFFICE-SESSION-TERM-001.10` explains why it does +not need to be. + +**Failure is closed** (`-001.7`). Either read failing means "do not terminate", +recorded and swallowed. This inverts the usual best-effort default in these +call sites, deliberately: the surrounding code swallows a *termination* +failure because a session left live is recoverable, and the same reasoning says +a session wrongly ended is not. + +## Control flow + +Each guarded path already commits its mutation before it reaches the +termination step, which is what `AC-OFFICE-SESSION-TERM-001.2` needs and why no +reordering is required: + +1. **Role removal.** The participant delete commits, then the service asks the + question and terminates only on "no capacity". The removed role is already + absent from the slate. +2. **Seat claim.** The claim transaction commits with the seat's agent profile + already reassigned to the claiming agent, then the post-commit effects run + on a cancellation-detached context. The displaced agent no longer holds that + seat when the question is asked. The question is inserted before the + existing termination call and leaves the displaced-run cancellation beside + it untouched (`AC-OFFICE-SESSION-TERM-003.4`). +3. **Reassignment.** The assignee update commits - which also rewrites the + runner seat at the task's current step - then the reactivity pipeline runs, + then the termination step. By the time the question is asked, the projection + already returns the new runner, so the previous runner is retained only if + it holds a seat in another role. + +The third is where `AC-OFFICE-SESSION-TERM-003.2` matters. The reactivity +pipeline's hard cancel of the previous runner's running execution happens +*before* the termination step and is not guarded. An agent reassigned away from +the runner role but still seated as a reviewer stops the coding turn it no +longer owns and keeps the row it still reviews through. Those are two different +objects, and the design keeps them decoupled on purpose. + +### Concurrency + +`AC-OFFICE-SESSION-TERM-001.10` is satisfied without coordination, and the +argument is worth recording because the obvious reading is that two concurrent +removals could both suppress. + +Each path reads strictly after its own commit. Take two paths removing +different capacities from the same agent: whichever commits second issues its +read after both commits are visible, observes neither capacity, and terminates. +The first may well suppress - it can still see the second's capacity - but the +outcome is the pair ending with a terminated session either way. The window +therefore closes itself, and adding a lock or a re-read would buy nothing. + +This depends only on each read being a fresh statement issued after that +caller's own commit, which holds on both engines under their default isolation. +It does not depend on which caller reads first. + +## Failure and recovery + +- A failed capacity read suppresses the termination and is recorded + (`-001.7`, `-004.2`). The session row stays live and is re-evaluated by the + next guarded path that runs on the pair. +- A suppressed termination is not an error to its caller (`-001.3`). All three + call sites already treat termination as best-effort and log rather than + propagate; suppression is quieter than that and returns success. +- A session left live because a capacity was retained needs no recovery: it is + the conversation the agent is still using, and the existing reuse rules will + hand it back on the agent's next wakeup. +- A session left live by a *mistaken* retained-capacity answer is the residual + risk this design accepts, and `-004.1`'s record with the retained capacity + named is how it is diagnosed. + +## Persistence + +No schema change. No new column, index, migration or backfill +(`AC-OFFICE-SESSION-TERM-003.3`). The guard adds two reads on the read-only +pool per guarded termination and removes some writes; it opens no transaction +and takes no lock. + +Sessions already ended by the pre-existing behavior are left as they are - the +requirement's "Out of scope" says so explicitly, and no repair is authorized. + +## Security + +No new surface. All three guarded paths already enforce their own +authorization before reaching the termination step - the participant mutators +behind the approve-permission gate, reassignment behind the assign-tasks gate - +and the guard runs after those checks, never instead of one. The question reads +only state the caller has already been authorized to mutate. + +## Observability + +Suppression is logged at the existing call sites' logger with the task, agent +profile, guarded reason and which capacity was retained (`-004.1`), and a read +failure is logged distinguishably (`-004.2`). Both follow the structured-zap +idiom the surrounding code already uses. + +A counter under the office namespace counts suppressions, labelled only by +bounded dimensions - the guarded reason and which capacity was retained +(`-004.3`). Neither is unbounded: the reasons are a fixed set of three and the +capacity is one of two. No task, agent, step or session identifier is a label, +matching `AC-OFFICE-REVIEW-SEATS-004.6`'s rule for the same reason. + +Terminations that proceed keep their existing reason values and log line +(`-004.4`), so the two outcomes remain comparable in one place. + +## Verification obligations + +The capacity question's own tests are ordinary. Two are worth naming because +they are the ones that fail silently if written casually: + +- The **step-scoping** criterion (`-002.3`) needs a task that has *moved*: a + seat written at a step the task has left, naming the agent, must not suppress + a termination. A test that never moves the task passes whether or not the + scope is bounded. +- The **decoupling** criterion (`-003.2`) needs a reassignment of an agent that + keeps a reviewer seat, asserting both halves: the execution is cancelled + *and* the session row is untouched. Asserting only the second passes for an + implementation that wrongly suppresses both. + +The shipped-configuration reachability described in the requirement's overview +also gives a direct regression test that needs no flag: with +`features.officeSessionIdentity` off, an agent that is both runner and +auto-cast reviewer holds the task's only session, and displacing it from the +reviewer seat must leave that session live. + +## Related decisions + +- [ADR 0005 - agent model unification](../../../decisions/0005-agent-model-unification.md)