fix(office): keep an agent's session alive while it's still working the task (#3556)
Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Carlos Florencio <carlosmflorencio@gmail.com>
This commit is contained in:
Henry
@@ -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 <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/<initiative>/plan.md` is a work-package
|
||||
manifest. Its sibling `task-<NN>-<short-slug>.md` files are work orders.
|
||||
- **Plans:** Implementation plans are generated from requirements and system designs through `/plan`. `docs/plans/<initiative>/plan.md` is a work-package manifest. Its sibling `task-<NN>-<short-slug>.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
|
||||
|
||||
|
||||
@@ -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.
|
||||
|
||||
|
||||
@@ -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 }
|
||||
}
|
||||
@@ -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.
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
@@ -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"])
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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))
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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 }
|
||||
}
|
||||
+134
@@ -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)
|
||||
}
|
||||
}
|
||||
+250
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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)
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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.
|
||||
@@ -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/...`.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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).
|
||||
@@ -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).
|
||||
@@ -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)
|
||||
@@ -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)
|
||||
Reference in New Issue
Block a user