From 2a0ec87da71765b89221be364244c4c08748a510 Mon Sep 17 00:00:00 2001 From: Henry Date: Tue, 15 Sep 2026 01:49:02 +0800 Subject: [PATCH] test(office): stop a participant seat from naming no agent (#3662) Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Carlos Florencio --- .../server/process/workspace_tracker_test.go | 22 +- .../internal/office/dashboard/handler_test.go | 9 +- .../participant_claim_cancel_failure_test.go | 149 ++++++++ .../participant_role_refusal_internal_test.go | 93 +++++ .../participant_surface_refusals_test.go | 63 ++++ .../office/dashboard/participants_test.go | 32 +- .../participant_claim_window_postgres_test.go | 232 ++++++++++++ .../sqlite/participant_guard_test.go | 172 +++++++++ .../sqlite/participant_provenance_test.go | 98 +++++ .../participant_step_resolution_test.go | 154 ++++++++ .../office/repository/sqlite/participants.go | 18 + .../sqlite/participants_ops_test.go | 75 ++++ apps/web/components/task/dockview-shared.tsx | 35 +- .../workflow-quorum-transitions.spec.ts | 21 +- .../plan.md | 225 ++++++++++++ .../task-01-store-boundary-guard.md | 105 ++++++ .../task-02-repository-provenance-cases.md | 116 ++++++ ...-03-surface-refusals-and-claim-activity.md | 111 ++++++ .../task-04-cancellation-failure-residual.md | 107 ++++++ .../task-05-deterministic-claim-window.md | 123 +++++++ .../task-06-e2e-quorum-required-count.md | 76 ++++ .../participant-seat-provenance-assurance.md | 307 ++++++++++++++++ ...articipant-seat-provenance-assurance-01.md | 342 ++++++++++++++++++ 23 files changed, 2647 insertions(+), 38 deletions(-) create mode 100644 apps/backend/internal/office/dashboard/participant_claim_cancel_failure_test.go create mode 100644 apps/backend/internal/office/dashboard/participant_role_refusal_internal_test.go create mode 100644 apps/backend/internal/office/dashboard/participant_surface_refusals_test.go create mode 100644 apps/backend/internal/office/repository/sqlite/participant_claim_window_postgres_test.go create mode 100644 apps/backend/internal/office/repository/sqlite/participant_guard_test.go create mode 100644 apps/backend/internal/office/repository/sqlite/participant_step_resolution_test.go create mode 100644 docs/plans/participant-seat-provenance-assurance/plan.md create mode 100644 docs/plans/participant-seat-provenance-assurance/task-01-store-boundary-guard.md create mode 100644 docs/plans/participant-seat-provenance-assurance/task-02-repository-provenance-cases.md create mode 100644 docs/plans/participant-seat-provenance-assurance/task-03-surface-refusals-and-claim-activity.md create mode 100644 docs/plans/participant-seat-provenance-assurance/task-04-cancellation-failure-residual.md create mode 100644 docs/plans/participant-seat-provenance-assurance/task-05-deterministic-claim-window.md create mode 100644 docs/plans/participant-seat-provenance-assurance/task-06-e2e-quorum-required-count.md create mode 100644 docs/specs/office/requirements/participant-seat-provenance-assurance.md create mode 100644 docs/specs/office/system-design/participant-seat-provenance-assurance-01.md diff --git a/apps/backend/internal/agentctl/server/process/workspace_tracker_test.go b/apps/backend/internal/agentctl/server/process/workspace_tracker_test.go index 5ea6761056..09ec348b0f 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_tracker_test.go +++ b/apps/backend/internal/agentctl/server/process/workspace_tracker_test.go @@ -79,12 +79,28 @@ func setupTestRepo(t *testing.T) (string, func()) { runGit(t, localDir, "commit", "-m", "Initial commit") // Add remote and push - runGit(t, localDir, "remote", "add", "origin", remoteDir) + runGit(t, localDir, "remote", "add", "origin", localGitRemotePath(remoteDir)) runGit(t, localDir, "push", "-u", "origin", "main") return localDir, cleanup } +// localGitRemotePath keeps temporary repository paths in the form that Git +// recognizes as local paths on every platform. A raw Windows path such as +// C:\\Temp\\remote.git can be parsed as an SSH-style host named "C", which +// leaves a test waiting for network input. Forward slashes keep the drive +// prefix unambiguous without changing the path returned by Git to callers. +func localGitRemotePath(path string) string { + return filepath.ToSlash(path) +} + +func TestLocalGitRemotePathUsesForwardSlashes(t *testing.T) { + remotePath := filepath.Join(t.TempDir(), "remote.git") + if got := localGitRemotePath(remotePath); strings.ContainsRune(got, '\\') { + t.Fatalf("localGitRemotePath(%q) = %q, want forward slashes", remotePath, got) + } +} + func runGit(t *testing.T, dir string, args ...string) string { t.Helper() // Use git -C flag to ensure we're in the right directory @@ -470,7 +486,7 @@ func TestFilterLocalCommits_PullAndResetScenario(t *testing.T) { writeFile(t, localDir, "README.md", "# Test Repo") runGit(t, localDir, "add", ".") runGit(t, localDir, "commit", "-m", "Initial commit (X)") - runGit(t, localDir, "remote", "add", "origin", remoteDir) + runGit(t, localDir, "remote", "add", "origin", localGitRemotePath(remoteDir)) runGit(t, localDir, "push", "-u", "origin", "main") // Record the starting point (commit X) @@ -478,7 +494,7 @@ func TestFilterLocalCommits_PullAndResetScenario(t *testing.T) { startingSHA = startingSHA[:len(startingSHA)-1] // Clone to upstream clone and make commits there (simulating main evolving) - runGit(t, upstreamClone, "clone", remoteDir, ".") + runGit(t, upstreamClone, "clone", localGitRemotePath(remoteDir), ".") runGit(t, upstreamClone, "config", "user.email", "upstream@test.com") runGit(t, upstreamClone, "config", "user.name", "Upstream User") runGit(t, upstreamClone, "config", "core.hooksPath", "/dev/null") // Disable hooks in test repo diff --git a/apps/backend/internal/office/dashboard/handler_test.go b/apps/backend/internal/office/dashboard/handler_test.go index e445ba7248..33549ff069 100644 --- a/apps/backend/internal/office/dashboard/handler_test.go +++ b/apps/backend/internal/office/dashboard/handler_test.go @@ -102,6 +102,14 @@ type testDeps struct { } func newTestDeps(t *testing.T) *testDeps { + t.Helper() + return newTestDepsWithLogger(t, logger.Default()) +} + +// newTestDepsWithLogger is newTestDeps with a caller-supplied logger, so a +// test can read what the service itself logged — the only way to assert a +// best-effort failure the service swallows on purpose. +func newTestDepsWithLogger(t *testing.T, log *logger.Logger) *testDeps { t.Helper() gin.SetMode(gin.TestMode) @@ -230,7 +238,6 @@ func newTestDeps(t *testing.T) *testDeps { t.Fatalf("create office_task_labels table: %v", err) } - log := logger.Default() activity := shared.NewActivityLogger(repo, log) agentSvc := &stubAgentReader{} costSvc := &stubCostChecker{} diff --git a/apps/backend/internal/office/dashboard/participant_claim_cancel_failure_test.go b/apps/backend/internal/office/dashboard/participant_claim_cancel_failure_test.go new file mode 100644 index 0000000000..ac69dd0940 --- /dev/null +++ b/apps/backend/internal/office/dashboard/participant_claim_cancel_failure_test.go @@ -0,0 +1,149 @@ +package dashboard_test + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" + + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/office/models" +) + +// TestAddTaskReviewer_ClaimSucceedsWhenDisplacedRunCancellationFails covers +// AC-OFFICE-SEAT-ASSURANCE-002.7, pinning the second sentence of +// AC-OFFICE-SEAT-PROVENANCE-006.6. +// +// That sentence is the only place the contract accepts a residual: the claim +// has already committed, the displaced agent's queued run cannot be +// cancelled, and the registration must still report success rather than +// failing an operator's action over cleanup it cannot undo. What it leaves +// behind is one run still runnable for an agent no longer seated in the +// role, and a recorded failure so that is visible rather than silent. +// +// The runs table is renamed aside for the duration of the call rather than +// dropped: after the restore the run is still there and still runnable, +// which is the residual itself. A dropped table would take the evidence with +// it. The rename touches only this test's own isolated in-memory database. +func TestAddTaskReviewer_ClaimSucceedsWhenDisplacedRunCancellationFails(t *testing.T) { + core, logs := observer.New(zapcore.WarnLevel) + log, err := logger.NewFromZap(zap.New(core)) + if err != nil { + t.Fatalf("build observed logger: %v", err) + } + + deps := newTestDepsWithLogger(t, log) + insertTestTask(t, deps.db, "cancelfail", "ws-cancelfail", "C", "todo", 2) + const stepID = "step-cancelfail" + + 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-cancelfail', '', datetime('now'), datetime('now')) + `); err != nil { + t.Fatalf("seed claiming agent profile: %v", err) + } + 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-cancelfail', ?, 'cancelfail', 'reviewer', 'agent-auto', 1, 0, 'auto') + `, stepID); err != nil { + t.Fatalf("seed auto seat: %v", err) + } + + // The run the step-entry fan-out queued for the agent about to be + // displaced. Seeded before the table goes away. + run := &models.Run{ + AgentProfileID: "agent-auto", + Reason: "task_assigned", + Payload: `{"task_id":"cancelfail","workflow_step_id":"` + stepID + `"}`, + Status: models.RunStatusQueued, + CoalescedCount: 1, + } + if err := deps.repo.CreateRun(t.Context(), run); err != nil { + t.Fatalf("seed displaced run: %v", err) + } + + // Take the runs store away for the duration of the registration, and + // put it back whatever happens. + if _, err := deps.db.Exec(`ALTER TABLE runs RENAME TO runs_stashed`); err != nil { + t.Fatalf("stash runs table: %v", err) + } + restored := false + restore := func() { + if restored { + return + } + restored = true + if _, err := deps.db.Exec(`ALTER TABLE runs_stashed RENAME TO runs`); err != nil { + t.Fatalf("restore runs table: %v", err) + } + } + t.Cleanup(restore) + + body := `{"agent_profile_id":"agent-claiming"}` + req := httptest.NewRequest(http.MethodPost, + "/api/v1/office/tasks/cancelfail/reviewers", strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + deps.router.ServeHTTP(w, req) + restore() + + // 1. The request succeeds: the claim committed, and cleanup it cannot + // complete must not be reported to the operator as a failed action. + if w.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201 (cancellation is best-effort): %s", w.Code, w.Body.String()) + } + + // 2. The claimed slate is exactly as a successful claim leaves it. + var seats int + if err := deps.db.Get(&seats, + `SELECT COUNT(*) FROM workflow_step_participants WHERE task_id = 'cancelfail' AND role = 'reviewer'`, + ); err != nil { + t.Fatalf("count seats: %v", err) + } + if seats != 1 { + t.Fatalf("seats = %d, want 1 (claimed in place)", seats) + } + var agentID, provenance string + if err := deps.db.QueryRow( + `SELECT agent_profile_id, provenance FROM workflow_step_participants WHERE id = 'auto-seat-cancelfail'`, + ).Scan(&agentID, &provenance); err != nil { + t.Fatalf("read claimed seat: %v", err) + } + if agentID != "agent-claiming" || provenance != "manual" { + t.Fatalf("seat = (agent=%q provenance=%q), want (agent-claiming, manual)", agentID, provenance) + } + + // 3. The failure is recorded. Asserted on fields, not message text. + entries := logs.FilterFieldKey("agent_profile_id").All() + found := false + for _, e := range entries { + fields := e.ContextMap() + if fields["task_id"] == "cancelfail" && + fields["step_id"] == stepID && + fields["agent_profile_id"] == "agent-auto" && + fields["error"] != nil { + found = true + break + } + } + if !found { + t.Fatalf("no warning naming the task, step and displaced agent with an error; observed: %+v", + logs.All()) + } + + // 4. The displaced agent's run is still runnable — the residual the + // criterion describes, and the reason the table was renamed rather + // than dropped. + gotRun, err := deps.repo.GetRunByID(t.Context(), run.ID) + if err != nil { + t.Fatalf("get displaced run: %v", err) + } + if gotRun.Status != models.RunStatusQueued { + t.Fatalf("displaced run status = %q, want %q (still runnable)", gotRun.Status, models.RunStatusQueued) + } +} diff --git a/apps/backend/internal/office/dashboard/participant_role_refusal_internal_test.go b/apps/backend/internal/office/dashboard/participant_role_refusal_internal_test.go new file mode 100644 index 0000000000..b39159c644 --- /dev/null +++ b/apps/backend/internal/office/dashboard/participant_role_refusal_internal_test.go @@ -0,0 +1,93 @@ +package dashboard + +import ( + "context" + "testing" + + "github.com/kandev/kandev/internal/office/repository/sqlite" +) + +// refusalRepo fails the test if the store is reached at all. The embedded +// interface supplies the full method set; only the participant write is +// overridden, so any other call would panic on the nil interface rather +// than pass silently. +type refusalRepo struct { + Repository + t *testing.T +} + +func (r *refusalRepo) AddTaskParticipant( + _ context.Context, taskID, agentID, role string, +) (sqlite.ParticipantWriteResult, error) { + r.t.Helper() + r.t.Fatalf("store reached for role %q (task %q, agent %q): an unsupported role must be refused "+ + "above the store, writing and claiming nothing", role, taskID, agentID) + return sqlite.ParticipantWriteResult{}, nil +} + +// TestAddOrRemoveParticipant_RefusesUnsupportedRole covers +// AC-OFFICE-SEAT-ASSURANCE-002.5, pinning +// AC-OFFICE-SEAT-PROVENANCE-005.7. +// +// This refusal is not reachable over HTTP, which is why the case is +// white-box rather than a request. The routes are role-fixed: each endpoint +// names its own role and the service exposes only reviewer and approver +// entry points, so no request can carry a bad role. The refusal that +// actually exists is the participantFields lookup miss inside the shared +// body those entry points call, and this exercises the same refusal an +// internal caller — a future agent tool, scheduler or fixup path — would +// meet. +// +// It calls beneath the HTTP surface but above the permission check that +// body performs, so it bypasses no authorization. +// +// A structural assertion over participantFields would be the weaker choice: +// it would pass against a body that consulted the map and then ignored it. +func TestAddOrRemoveParticipant_RefusesUnsupportedRole(t *testing.T) { + svc := &DashboardService{repo: &refusalRepo{t: t}} + + for _, role := range []string{"watcher", "collaborator", "runner", "", "REVIEWER"} { + t.Run("role="+role, func(t *testing.T) { + err := svc.addOrRemoveParticipant(context.Background(), "", "task-1", "agent-1", role, true) + if err == nil { + t.Fatalf("addOrRemoveParticipant(role=%q) = nil, want an error: only reviewer and "+ + "approver may be seated through this surface", role) + } + }) + } +} + +// TestAddOrRemoveParticipant_AcceptsTheSupportedRoles is the other half of +// the boundary: the two roles the surface does expose must get past the +// lookup and reach the store. Without this, a body that refused every role +// would satisfy the case above. +func TestAddOrRemoveParticipant_AcceptsTheSupportedRoles(t *testing.T) { + for _, role := range []string{"reviewer", "approver"} { + t.Run("role="+role, func(t *testing.T) { + reached := false + svc := &DashboardService{repo: &reachedRepo{reached: &reached}} + if err := svc.addOrRemoveParticipant( + context.Background(), "", "task-1", "agent-1", role, true); err != nil { + t.Fatalf("addOrRemoveParticipant(role=%q) = %v, want the store to be reached", role, err) + } + if !reached { + t.Fatalf("role %q never reached the store", role) + } + }) + } +} + +// reachedRepo records that the participant write was called, and reports an +// outcome that earns no post-commit side effects so the zero-value service's +// optional collaborators are never consulted. +type reachedRepo struct { + Repository + reached *bool +} + +func (r *reachedRepo) AddTaskParticipant( + _ context.Context, _, _, _ string, +) (sqlite.ParticipantWriteResult, error) { + *r.reached = true + return sqlite.ParticipantWriteResult{Outcome: sqlite.ParticipantWriteOutcomeUnchanged}, nil +} diff --git a/apps/backend/internal/office/dashboard/participant_surface_refusals_test.go b/apps/backend/internal/office/dashboard/participant_surface_refusals_test.go new file mode 100644 index 0000000000..273b689aaf --- /dev/null +++ b/apps/backend/internal/office/dashboard/participant_surface_refusals_test.go @@ -0,0 +1,63 @@ +package dashboard_test + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// TestAddTaskParticipant_HTTPRejectsEmptyAgentProfileID covers +// AC-OFFICE-SEAT-ASSURANCE-002.4, pinning +// AC-OFFICE-SEAT-PROVENANCE-005.1. +// +// The registration surface refuses a request naming no agent before the +// service or the store is reached. The slate must be untouched: a seat +// naming nobody is admitted by the natural key, counted by the quorum +// guard, and can never be woken or decided, so the refusal is the whole +// protection at this layer. +// +// Both add endpoints are covered because each names its own role, so a +// guard added to one body and not the other would leave half the surface +// open. +func TestAddTaskParticipant_HTTPRejectsEmptyAgentProfileID(t *testing.T) { + for _, tc := range []struct { + name string + path string + role string + }{ + {name: "reviewer", path: "reviewers", role: "reviewer"}, + {name: "approver", path: "approvers", role: "approver"}, + } { + t.Run(tc.name, func(t *testing.T) { + deps := newTestDeps(t) + insertTestTask(t, deps.db, "empty-agent", "ws-empty", "E", "todo", 1) + if _, err := deps.db.Exec( + `UPDATE tasks SET workflow_step_id = 'step-empty' WHERE id = 'empty-agent'`); err != nil { + t.Fatalf("bind task to step: %v", err) + } + + req := httptest.NewRequest(http.MethodPost, + "/api/v1/office/tasks/empty-agent/"+tc.path, + strings.NewReader(`{"agent_profile_id":""}`)) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + deps.router.ServeHTTP(w, req) + + if w.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400: %s", w.Code, w.Body.String()) + } + + var seats int + if err := deps.db.Get(&seats, ` + SELECT COUNT(*) FROM workflow_step_participants + WHERE task_id = 'empty-agent' AND role = ? + `, tc.role); err != nil { + t.Fatalf("count seats: %v", err) + } + if seats != 0 { + t.Fatalf("seats = %d, want 0 (a refused registration writes nothing)", seats) + } + }) + } +} diff --git a/apps/backend/internal/office/dashboard/participants_test.go b/apps/backend/internal/office/dashboard/participants_test.go index 009c59733b..9a96f0d9b6 100644 --- a/apps/backend/internal/office/dashboard/participants_test.go +++ b/apps/backend/internal/office/dashboard/participants_test.go @@ -411,15 +411,35 @@ func TestAddTaskReviewer_ClaimingAutoSeatCancelsDisplacedRunAndLogsActivity(t *t t.Fatalf("displaced run status = %q, want cancelled", gotRun.Status) } - var activityCount int - if err := deps.db.Get(&activityCount, ` - SELECT COUNT(*) FROM office_activity_log + // AC-OFFICE-SEAT-ASSURANCE-002.8: the claim's activity entry is asserted + // by content, not by count. A count alone passes with an empty or wrong + // payload, which is exactly the entry an operator cannot act on — it must + // name the task, the step, the role, the agent displaced and the agent + // that displaced it (AC-OFFICE-SEAT-PROVENANCE-002.9). + var details []string + if err := deps.db.Select(&details, ` + SELECT details FROM office_activity_log WHERE target_id = 'claim1' AND action = 'task_participant_claimed' `); err != nil { - t.Fatalf("count claim activity: %v", err) + t.Fatalf("read claim activity: %v", err) } - if activityCount != 1 { - t.Fatalf("claim activity entries = %d, want 1", activityCount) + if len(details) != 1 { + t.Fatalf("claim activity entries = %d, want 1", len(details)) + } + var payload map[string]string + if err := json.Unmarshal([]byte(details[0]), &payload); err != nil { + t.Fatalf("decode claim activity details %q: %v", details[0], err) + } + for _, field := range []struct{ key, want string }{ + {"task_id", "claim1"}, + {"step_id", stepID}, + {"role", "reviewer"}, + {"displaced_agent_profile_id", "agent-auto"}, + {"claiming_agent_profile_id", "agent-claiming"}, + } { + if got := payload[field.key]; got != field.want { + t.Errorf("claim activity details[%q] = %q, want %q", field.key, got, field.want) + } } } diff --git a/apps/backend/internal/office/repository/sqlite/participant_claim_window_postgres_test.go b/apps/backend/internal/office/repository/sqlite/participant_claim_window_postgres_test.go new file mode 100644 index 0000000000..9468b6243e --- /dev/null +++ b/apps/backend/internal/office/repository/sqlite/participant_claim_window_postgres_test.go @@ -0,0 +1,232 @@ +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" +) + +// waitUntilBlockedBy polls until at least one backend is blocked waiting on +// holderPID, and fails the test if that never happens before the deadline. +// +// Reaching this successfully is the proof the AC-004.8 window was entered: +// the registration's claim search has already selected the locked seat as +// its candidate (a plain SELECT is not blocked by a row lock under MVCC) and +// its conditional UPDATE is now waiting on that lock (an UPDATE is). A +// timeout here means the window was NOT entered, which must fail loudly +// rather than let the case report coverage it did not exercise. +func waitUntilBlockedBy(t *testing.T, db *sqlx.DB, holderPID int) { + t.Helper() + ctx := context.Background() + deadline := time.Now().Add(15 * time.Second) + for time.Now().Before(deadline) { + var blocked int + if err := db.GetContext(ctx, &blocked, ` + SELECT count(*) FROM pg_stat_activity + WHERE pg_blocking_pids(pid) @> ARRAY[$1]::int[] + `, holderPID); err != nil { + t.Fatalf("probe blocked backends: %v", err) + } + if blocked > 0 { + return + } + time.Sleep(25 * time.Millisecond) + } + t.Fatalf("no backend became blocked on holder pid %d within the deadline: "+ + "the claim's conditional UPDATE never waited on the row lock, so the "+ + "AC-OFFICE-SEAT-PROVENANCE-004.8 window was not entered and this case proves nothing", holderPID) +} + +// TestPostgresAddTaskParticipant_ClaimTargetRemovedInsideTheWriteWindow +// covers AC-OFFICE-SEAT-ASSURANCE-002.10, pinning +// AC-OFFICE-SEAT-PROVENANCE-004.8 deterministically. +// +// The window: a seat is selected as claimable, removed before the +// conditional write applies, and the registration must fall through to +// inserting its own seat rather than completing having written nothing. +// +// participants.go's comment on that path says the window "cannot be forced +// deterministically", and +// TestPostgresAddTaskParticipant_ConvergesWithConcurrentRemoveOfClaimTarget +// stands in for it with fifteen concurrent iterations whose split it logs +// but does not assert — so a run that never entered the window passes +// identically to one that did. +// +// The premise is wrong, and the reason is in the code: the claim search is a +// plain read, which a row lock does not block, and the claim is a +// conditional UPDATE of one row by id, which it does. An outside session can +// therefore stand exactly between them: +// +// 1. A holding session takes a row lock on the seat. Nothing else is locked. +// 2. The registration runs. Its advisory lock is uncontended, its identity +// probe misses, its claim search reads the seat through the row lock and +// selects it, and its conditional write blocks. +// 3. The test waits until that block is observable. This is the proof. +// 4. The holder deletes the seat and commits. +// 5. The blocked write re-evaluates, matches no row, reports zero rows +// affected, and the registration falls through and inserts. +// +// No production code changes for this: the window is entered on the shipped +// path, not on a path that exists only under test. The fifteen-iteration +// case is kept — it exercises genuine cross-connection concurrency, which +// this one deliberately does not. +// +// Skips unless KANDEV_TEST_POSTGRES_DSN is set. +func TestPostgresAddTaskParticipant_ClaimTargetRemovedInsideTheWriteWindow(t *testing.T) { + dsn := testutil.PostgresDSNFromEnv(t) + db := openIsolatedPostgresMultiConnForClaimRace(t, dsn, 5) + 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 (?, '', 'Claim Window', ?, ?) + `), "wf-claim-window", now, now); err != nil { + t.Fatalf("seed workflow: %v", err) + } + step := &models.WorkflowStep{ + WorkflowID: "wf-claim-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-window", "agent-human-window"} { + 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 (?, ?, ?, ?, 'ws-claim-window', '', ?, ?) + `), agentID, agentID, agentID, agentID, now, now); err != nil { + t.Fatalf("seed agent profile %s: %v", agentID, err) + } + } + + const taskID = "task-claim-window" + const seatID = "seat-claim-window" + if _, err := db.ExecContext(ctx, db.Rebind(` + INSERT INTO tasks (id, workflow_step_id, title, created_at, updated_at) VALUES (?, ?, 'window', ?, ?) + `), taskID, 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 (?, ?, ?, 'reviewer', 'agent-auto-window', 1, 0, 'auto') + `), seatID, step.ID, taskID); err != nil { + t.Fatalf("seed auto seat: %v", err) + } + + // 1. The holding session locks the seat row and nothing else. + holder, err := db.Connx(ctx) + if err != nil { + t.Fatalf("acquire holder connection: %v", err) + } + defer func() { _ = holder.Close() }() + holdTx, err := holder.BeginTxx(ctx, nil) + if err != nil { + t.Fatalf("begin holder tx: %v", err) + } + defer func() { _ = holdTx.Rollback() }() + + var holderPID int + if err := holdTx.GetContext(ctx, &holderPID, `SELECT pg_backend_pid()`); err != nil { + t.Fatalf("read holder pid: %v", err) + } + var lockedID string + if err := holdTx.GetContext(ctx, &lockedID, + `SELECT id FROM workflow_step_participants WHERE id = $1 FOR UPDATE`, seatID); err != nil { + t.Fatalf("lock seat row: %v", err) + } + + // 2. The registration selects the locked seat, then blocks on the write. + type addResult struct { + result sqlite.ParticipantWriteResult + err error + } + done := make(chan addResult, 1) + go func() { + r, aerr := officeRepo.AddTaskParticipant(ctx, taskID, "agent-human-window", "reviewer") + done <- addResult{result: r, err: aerr} + }() + + // 3. Wait until the block is observable — the window itself. + waitUntilBlockedBy(t, db, holderPID) + + // 4. The holder removes the selected seat and commits. + if _, err := holdTx.ExecContext(ctx, + `DELETE FROM workflow_step_participants WHERE id = $1`, seatID); err != nil { + t.Fatalf("delete seat inside the window: %v", err) + } + if err := holdTx.Commit(); err != nil { + t.Fatalf("commit holder tx: %v", err) + } + + // 5. The blocked write matches no row and the registration inserts. + var got addResult + select { + case got = <-done: + case <-time.After(30 * time.Second): + t.Fatal("AddTaskParticipant did not return after the claim target was removed") + } + if got.err != nil { + t.Fatalf("AddTaskParticipant: %v", got.err) + } + if got.result.Outcome != sqlite.ParticipantWriteOutcomeInserted { + t.Fatalf("outcome = %q, want %q: a zero-row conditional write must fall through to an insert, "+ + "not be reported as a claim", got.result.Outcome, sqlite.ParticipantWriteOutcomeInserted) + } + if got.result.DisplacedAgentProfileID != "" { + t.Errorf("displaced agent = %q, want empty (nothing was claimed)", got.result.DisplacedAgentProfileID) + } + + var seats []struct { + ID string `db:"id"` + AgentProfileID string `db:"agent_profile_id"` + Provenance string `db:"provenance"` + } + if err := db.SelectContext(ctx, &seats, db.Rebind(` + SELECT id, agent_profile_id, provenance FROM workflow_step_participants + WHERE task_id = ? AND role = 'reviewer' + `), taskID); err != nil { + t.Fatalf("read slate: %v", err) + } + if len(seats) != 1 { + t.Fatalf("slate = %+v, want exactly 1 seat: an implementation reporting a claim on a "+ + "zero-row write would leave this empty", seats) + } + if seats[0].AgentProfileID != "agent-human-window" { + t.Errorf("seat agent = %q, want agent-human-window", seats[0].AgentProfileID) + } + if seats[0].Provenance != string(models.ParticipantProvenanceManual) { + t.Errorf("seat provenance = %q, want manual", seats[0].Provenance) + } + if seats[0].ID == lockedID { + t.Errorf("seat id = %q, want a fresh seat: the removed seat cannot be the one that survived", seats[0].ID) + } +} diff --git a/apps/backend/internal/office/repository/sqlite/participant_guard_test.go b/apps/backend/internal/office/repository/sqlite/participant_guard_test.go new file mode 100644 index 0000000000..b3292a3a8b --- /dev/null +++ b/apps/backend/internal/office/repository/sqlite/participant_guard_test.go @@ -0,0 +1,172 @@ +package sqlite_test + +import ( + "context" + "errors" + "testing" + + "github.com/kandev/kandev/internal/office/repository/sqlite" +) + +// seatProvenanceAndAgent reads one seat's agent profile and provenance +// directly off the row, so a case can prove a seat was left alone rather +// than trusting the office-side projection, which carries neither field. +func seatProvenanceAndAgent(t *testing.T, repo *sqlite.Repository, seatID string) (agentID, provenance string) { + t.Helper() + row := repo.ReaderDB().QueryRow( + `SELECT agent_profile_id, provenance FROM workflow_step_participants WHERE id = ?`, seatID) + if err := row.Scan(&agentID, &provenance); err != nil { + t.Fatalf("read seat %s: %v", seatID, err) + } + return agentID, provenance +} + +// TestAddTaskParticipant_RejectsEmptyAgentProfileID covers +// AC-OFFICE-SEAT-ASSURANCE-001.1 and -001.2. The store refuses a +// registration naming no agent, writing no seat, claiming no seat and +// promoting no seat's provenance. The refusal is a sentinel the caller can +// match on, which is what distinguishes it from the nil-error "unchanged" +// the store reports for a task at no step or no task at all. +func TestAddTaskParticipant_RejectsEmptyAgentProfileID(t *testing.T) { + repo := newSearchTestRepo(t) + ctx := context.Background() + + seedParticipantTask(t, repo, "guard-empty", "step-1") + seedAutoSeat(t, repo, "auto-guard-1", "step-1", "guard-empty", "reviewer", "agent-auto") + + result, err := repo.AddTaskParticipant(ctx, "guard-empty", "", "reviewer") + if !errors.Is(err, sqlite.ErrEmptyAgentProfileID) { + t.Fatalf("AddTaskParticipant error = %v, want ErrEmptyAgentProfileID", err) + } + if result != (sqlite.ParticipantWriteResult{}) { + t.Fatalf("result = %+v, want zero value", result) + } + + // No seat written: the slate still holds exactly the seeded auto seat. + if n := participantRowCount(t, repo, "guard-empty"); n != 1 { + t.Fatalf("rows = %d, want 1 (no seat written)", n) + } + // No seat claimed and no provenance promoted. + agentID, provenance := seatProvenanceAndAgent(t, repo, "auto-guard-1") + if agentID != "agent-auto" { + t.Fatalf("seat agent = %q, want agent-auto (not claimed)", agentID) + } + if provenance != "auto" { + t.Fatalf("seat provenance = %q, want auto (not promoted)", provenance) + } +} + +// TestAddTaskParticipant_EmptyAgentRejectionIsDeterministic covers +// AC-OFFICE-SEAT-ASSURANCE-001.4: the rejection carries no state, so the +// same call rejected twice yields the same failure and the store neither +// retries it nor requires a caller to. +func TestAddTaskParticipant_EmptyAgentRejectionIsDeterministic(t *testing.T) { + repo := newSearchTestRepo(t) + ctx := context.Background() + + seedParticipantTask(t, repo, "guard-twice", "step-1") + seedAutoSeat(t, repo, "auto-guard-2", "step-1", "guard-twice", "reviewer", "agent-auto") + + for i := range 2 { + result, err := repo.AddTaskParticipant(ctx, "guard-twice", "", "reviewer") + if !errors.Is(err, sqlite.ErrEmptyAgentProfileID) { + t.Fatalf("call %d error = %v, want ErrEmptyAgentProfileID", i+1, err) + } + if result != (sqlite.ParticipantWriteResult{}) { + t.Fatalf("call %d result = %+v, want zero value", i+1, result) + } + } + + if n := participantRowCount(t, repo, "guard-twice"); n != 1 { + t.Fatalf("rows = %d, want 1 (no seat written by either call)", n) + } + agentID, provenance := seatProvenanceAndAgent(t, repo, "auto-guard-2") + if agentID != "agent-auto" || provenance != "auto" { + t.Fatalf("seat = (%q, %q), want (agent-auto, auto)", agentID, provenance) + } +} + +// TestAddTaskParticipant_EmptyAgentGuardRunsBeforeTransaction covers +// AC-OFFICE-SEAT-ASSURANCE-001.3. Closing the database makes any transaction +// attempt fail, so the sentinel proves that the guard returns before the +// store touches the database. +func TestAddTaskParticipant_EmptyAgentGuardRunsBeforeTransaction(t *testing.T) { + repo := newSearchTestRepo(t) + if err := repo.ReaderDB().Close(); err != nil { + t.Fatalf("close database: %v", err) + } + + result, err := repo.AddTaskParticipant(context.Background(), "guard-closed", "", "reviewer") + if !errors.Is(err, sqlite.ErrEmptyAgentProfileID) { + t.Fatalf("AddTaskParticipant error = %v, want ErrEmptyAgentProfileID", err) + } + if result != (sqlite.ParticipantWriteResult{}) { + t.Fatalf("result = %+v, want zero value", result) + } +} + +// TestAddTaskParticipant_WhitespaceAgentIsNotRejectedByGuard covers +// AC-OFFICE-SEAT-ASSURANCE-001.5. Only the empty identifier is this +// guard's business. A whitespace identifier names no agent profile and +// stays governed by AC-OFFICE-SEAT-PROVENANCE-005.8: it claims no seat and +// displaces no auto seat. Pinning this keeps the store's guard and the +// registration surface's from drifting about what "empty" means — a guard +// that trimmed would reject a value the surface accepts. +func TestAddTaskParticipant_WhitespaceAgentIsNotRejectedByGuard(t *testing.T) { + repo := newSearchTestRepo(t) + ctx := context.Background() + + seedParticipantTask(t, repo, "guard-ws", "step-1") + seedAutoSeat(t, repo, "auto-guard-3", "step-1", "guard-ws", "reviewer", "agent-auto") + + if _, err := repo.AddTaskParticipant(ctx, "guard-ws", " ", "reviewer"); err != nil { + t.Fatalf("AddTaskParticipant(%q) error = %v, want no error: only the empty identifier is rejected", " ", err) + } + + // -005.8 still governs: the auto seat is neither claimed nor promoted. + agentID, provenance := seatProvenanceAndAgent(t, repo, "auto-guard-3") + if agentID != "agent-auto" { + t.Fatalf("seat agent = %q, want agent-auto (whitespace agent cannot claim)", agentID) + } + if provenance != "auto" { + t.Fatalf("seat provenance = %q, want auto (not promoted)", provenance) + } +} + +// TestAddTaskParticipant_EmptyAgentDistinguishableFromUnchanged covers +// AC-OFFICE-SEAT-ASSURANCE-001.2's second sentence. The store reports a +// task standing at no step, and a task that does not exist, as Unchanged +// with a nil error (AC-OFFICE-SEAT-PROVENANCE-005.2, -005.6). The empty +// identifier must not land in that same bucket, or a caller could not tell +// "you called wrong" from "nothing to do" without matching a message. +func TestAddTaskParticipant_EmptyAgentDistinguishableFromUnchanged(t *testing.T) { + repo := newSearchTestRepo(t) + ctx := context.Background() + + seedParticipantTask(t, repo, "guard-nostep", "") + seedParticipantAgent(t, repo, "agent-real") + + // Task at no step: Unchanged, nil error. + result, err := repo.AddTaskParticipant(ctx, "guard-nostep", "agent-real", "reviewer") + if err != nil { + t.Fatalf("no-step task error = %v, want nil", err) + } + if result.Outcome != sqlite.ParticipantWriteOutcomeUnchanged { + t.Fatalf("no-step outcome = %v, want Unchanged", result.Outcome) + } + + // Task that does not exist: Unchanged, nil error. + result, err = repo.AddTaskParticipant(ctx, "guard-absent", "agent-real", "reviewer") + if err != nil { + t.Fatalf("absent task error = %v, want nil", err) + } + if result.Outcome != sqlite.ParticipantWriteOutcomeUnchanged { + t.Fatalf("absent task outcome = %v, want Unchanged", result.Outcome) + } + + // Empty identifier: a sentinel failure, not Unchanged. + _, err = repo.AddTaskParticipant(ctx, "guard-nostep", "", "reviewer") + if !errors.Is(err, sqlite.ErrEmptyAgentProfileID) { + t.Fatalf("empty agent error = %v, want ErrEmptyAgentProfileID", err) + } +} diff --git a/apps/backend/internal/office/repository/sqlite/participant_provenance_test.go b/apps/backend/internal/office/repository/sqlite/participant_provenance_test.go index 8aa268b9c3..f858422a24 100644 --- a/apps/backend/internal/office/repository/sqlite/participant_provenance_test.go +++ b/apps/backend/internal/office/repository/sqlite/participant_provenance_test.go @@ -489,3 +489,101 @@ func assertRunStatus(t *testing.T, repo *sqlite.Repository, id, want string) { t.Errorf("run %s status = %q, want %q", id, got, want) } } + +// TestAddTaskParticipant_IdentityProbePrecedesClaimSearch covers +// AC-OFFICE-SEAT-ASSURANCE-002.1, pinning +// AC-OFFICE-SEAT-PROVENANCE-002.4's ordering. +// +// The slate holds agent A's manual seat and exactly one undecided auto seat +// for a different agent B. Registering A again must report Unchanged off +// the identity probe and leave B's seat alone. An implementation that +// searched for a claimable seat before checking whether the named agent +// already holds one would find B's auto seat and consume it, evicting B in +// service of a registration that had nothing to do. +// +// The second assertion is the one that bites: the outcome alone reads +// Unchanged either way once A already holds a seat, so a case that only +// checked the outcome would pass with B silently displaced. +func TestAddTaskParticipant_IdentityProbePrecedesClaimSearch(t *testing.T) { + repo := newSearchTestRepo(t) + ctx := context.Background() + + seedParticipantTask(t, repo, "probe-first", "step-1") + seedParticipantAgent(t, repo, "agent-a") + seedParticipantAgent(t, repo, "agent-b") + seedManualSeat(t, repo, "manual-a", "step-1", "probe-first", "reviewer", "agent-a") + seedAutoSeat(t, repo, "auto-b", "step-1", "probe-first", "reviewer", "agent-b") + + result, err := repo.AddTaskParticipant(ctx, "probe-first", "agent-a", "reviewer") + if err != nil { + t.Fatalf("AddTaskParticipant: %v", err) + } + if result.Outcome != sqlite.ParticipantWriteOutcomeUnchanged { + t.Fatalf("outcome = %q, want %q (A already holds a seat)", result.Outcome, sqlite.ParticipantWriteOutcomeUnchanged) + } + if result.DisplacedAgentProfileID != "" { + t.Fatalf("displaced agent = %q, want empty (nothing was displaced)", result.DisplacedAgentProfileID) + } + + // B's seat is untouched: still B's, still auto, still claimable by a + // registration that actually needs a seat. + agentID, provenance := seatProvenanceAndAgent(t, repo, "auto-b") + if agentID != "agent-b" { + t.Fatalf("auto seat agent = %q, want agent-b (claim search must not have run)", agentID) + } + if provenance != "auto" { + t.Fatalf("auto seat provenance = %q, want auto (not promoted)", provenance) + } + if n := participantRowCount(t, repo, "probe-first"); n != 2 { + t.Fatalf("rows = %d, want 2 (no seat added or removed)", n) + } +} + +// TestAddTaskParticipant_DisplacedAgentRegistersAgain covers +// AC-OFFICE-SEAT-ASSURANCE-002.2, pinning +// AC-OFFICE-SEAT-PROVENANCE-002.10: a displaced agent coming back gets a +// second manual seat rather than reclaiming the one taken from it. +// +// TestAddTaskParticipant_PromotionConvergencePair's "B then A" ordering +// reaches the same final state, but cannot tell this history from +// insert-then-promote: two manual seats naming both agents is where both +// paths land. So this case asserts the intermediate outcome — that the +// first registration genuinely claimed and named A as displaced — which is +// what makes the two-seat result evidence about displacement. +func TestAddTaskParticipant_DisplacedAgentRegistersAgain(t *testing.T) { + repo := newSearchTestRepo(t) + ctx := context.Background() + + seedParticipantTask(t, repo, "displaced-return", "step-1") + seedParticipantAgent(t, repo, "agent-a") + seedParticipantAgent(t, repo, "agent-b") + seedAutoSeat(t, repo, "auto-dr", "step-1", "displaced-return", "reviewer", "agent-a") + + // B claims A's cast seat, displacing A. + claim, err := repo.AddTaskParticipant(ctx, "displaced-return", "agent-b", "reviewer") + if err != nil { + t.Fatalf("AddTaskParticipant(B): %v", err) + } + if claim.Outcome != sqlite.ParticipantWriteOutcomeClaimed { + t.Fatalf("first outcome = %q, want %q — without a claim this case proves nothing about displacement", + claim.Outcome, sqlite.ParticipantWriteOutcomeClaimed) + } + if claim.DisplacedAgentProfileID != "agent-a" { + t.Fatalf("displaced agent = %q, want agent-a", claim.DisplacedAgentProfileID) + } + + // A registers again at the same step and role: a second manual seat, + // not a reclaim of the seat B now holds. + again, err := repo.AddTaskParticipant(ctx, "displaced-return", "agent-a", "reviewer") + if err != nil { + t.Fatalf("AddTaskParticipant(A again): %v", err) + } + if again.Outcome != sqlite.ParticipantWriteOutcomeInserted { + t.Fatalf("second outcome = %q, want %q", again.Outcome, sqlite.ParticipantWriteOutcomeInserted) + } + if again.DisplacedAgentProfileID != "" { + t.Fatalf("second call displaced %q, want empty (B keeps its seat)", again.DisplacedAgentProfileID) + } + + assertTwoManualSeatsNamingBoth(t, repo, "displaced-return") +} diff --git a/apps/backend/internal/office/repository/sqlite/participant_step_resolution_test.go b/apps/backend/internal/office/repository/sqlite/participant_step_resolution_test.go new file mode 100644 index 0000000000..fbdac15761 --- /dev/null +++ b/apps/backend/internal/office/repository/sqlite/participant_step_resolution_test.go @@ -0,0 +1,154 @@ +package sqlite_test + +import ( + "context" + "testing" + + "github.com/jmoiron/sqlx" + + settingsstore "github.com/kandev/kandev/internal/agent/settings/store" + "github.com/kandev/kandev/internal/office/repository/sqlite" +) + +// participantSeatSchema is the subset of tables AddTaskParticipant reads and +// writes: the task it resolves a step from, the seat slate it mutates, and +// the decisions table findClaimableAutoSeat joins against. +const participantSeatSchema = ` + CREATE TABLE IF NOT EXISTS tasks ( + id TEXT PRIMARY KEY, + workspace_id TEXT NOT NULL DEFAULT '', + workflow_step_id TEXT NOT NULL DEFAULT '', + title TEXT NOT NULL DEFAULT '', + created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, + updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP + ); + CREATE TABLE IF NOT EXISTS workflow_step_participants ( + id TEXT PRIMARY KEY, + step_id TEXT NOT NULL DEFAULT '', + task_id TEXT NOT NULL DEFAULT '', + role TEXT NOT NULL DEFAULT '', + agent_profile_id TEXT NOT NULL DEFAULT '', + decision_required INTEGER NOT NULL DEFAULT 0, + position INTEGER NOT NULL DEFAULT 0, + created_at TIMESTAMP NOT NULL DEFAULT '1970-01-01 00:00:00', + provenance TEXT NOT NULL DEFAULT 'manual' + ); + CREATE TABLE IF NOT EXISTS workflow_step_decisions ( + id TEXT PRIMARY KEY, + task_id TEXT NOT NULL DEFAULT '', + step_id TEXT NOT NULL DEFAULT '', + participant_id TEXT NOT NULL DEFAULT '', + decision TEXT NOT NULL DEFAULT '', + decided_at TIMESTAMP NOT NULL DEFAULT '1970-01-01 00:00:00', + superseded_at TIMESTAMP NULL + ); +` + +// newDivergentHandleRepo builds an office repository whose writer and reader +// point at two different databases, where the same task row names a +// different current step in each. +// +// This exists to make AC-OFFICE-SEAT-PROVENANCE-004.10 falsifiable. +// AddTaskParticipant must resolve the task's step on its transaction handle, +// inside its own exclusion — never through the read-only pool, which would +// escape that exclusion. In every other test both handles point at the same +// database, so swapping stepIDForTaskTx for stepIDForTask is a one-word edit +// that changes nothing any of them observe. Here it changes the answer. +// +// Returns the repo and the writer handle. Assertions must use the writer: the +// repo's own ReaderDB() accessor now points at the wrong database by +// construction. +func newDivergentHandleRepo(t *testing.T, taskID, writerStepID, readerStepID string) (*sqlite.Repository, *sqlx.DB) { + t.Helper() + + writer, err := sqlx.Open("sqlite3", ":memory:") + if err != nil { + t.Fatalf("open writer: %v", err) + } + writer.SetMaxOpenConns(1) + t.Cleanup(func() { _ = writer.Close() }) + + reader, err := sqlx.Open("sqlite3", ":memory:") + if err != nil { + t.Fatalf("open reader: %v", err) + } + reader.SetMaxOpenConns(1) + t.Cleanup(func() { _ = reader.Close() }) + + // Schema initialisation runs against the writer, so the reader needs its + // own tasks table and row spelled out explicitly. + if _, _, err := settingsstore.Provide(writer, writer, nil); err != nil { + t.Fatalf("settings store init: %v", err) + } + repo, err := sqlite.NewWithDB(writer, reader, nil) + if err != nil { + t.Fatalf("new repo: %v", err) + } + if _, err := writer.Exec(participantSeatSchema); err != nil { + t.Fatalf("writer schema: %v", err) + } + if _, err := reader.Exec(participantSeatSchema); err != nil { + t.Fatalf("reader schema: %v", err) + } + + if _, err := writer.Exec( + `INSERT INTO tasks (id, workspace_id, workflow_step_id, title) VALUES (?, 'ws-1', ?, 'Task')`, + taskID, writerStepID); err != nil { + t.Fatalf("seed writer task: %v", err) + } + if _, err := reader.Exec( + `INSERT INTO tasks (id, workspace_id, workflow_step_id, title) VALUES (?, 'ws-1', ?, 'Task')`, + taskID, readerStepID); err != nil { + t.Fatalf("seed reader task: %v", err) + } + return repo, writer +} + +// TestAddTaskParticipant_ResolvesStepOnTheWritingHandle covers +// AC-OFFICE-SEAT-ASSURANCE-002.3, pinning +// AC-OFFICE-SEAT-PROVENANCE-004.10. +// +// The two handles disagree about which step the task stands at. The seat must +// land at the step the writing handle names, because that is the read taken +// inside the exclusion that serialises this writer against automatic casting. +// An implementation reading through the read-only pool lands at the reader's +// step instead, and this case is the only one in the suite that can tell. +// +// A divergent reader row is deliberate rather than an absent one: divergence +// proves the seat landed at the writer's step, where absence would only prove +// the reader was not consulted. +func TestAddTaskParticipant_ResolvesStepOnTheWritingHandle(t *testing.T) { + ctx := context.Background() + repo, writer := newDivergentHandleRepo(t, "split-task", "step-writer", "step-reader") + + if _, err := writer.Exec(` + INSERT INTO agent_profiles (id, agent_id, name, agent_display_name, workspace_id, role, created_at, updated_at) + VALUES ('agent-human', '', 'agent-human', 'agent-human', 'ws-1', '', datetime('now'), datetime('now')) + `); err != nil { + t.Fatalf("seed agent profile: %v", err) + } + + result, err := repo.AddTaskParticipant(ctx, "split-task", "agent-human", "reviewer") + if err != nil { + t.Fatalf("AddTaskParticipant: %v", err) + } + if result.Outcome != sqlite.ParticipantWriteOutcomeInserted { + t.Fatalf("outcome = %q, want %q", result.Outcome, sqlite.ParticipantWriteOutcomeInserted) + } + if result.StepID != "step-writer" { + t.Errorf("reported step = %q, want step-writer (the writing handle's view)", result.StepID) + } + + // Read directly through the writer. ReaderDB intentionally points at the + // divergent database, so using it would verify the fixture instead. + var stepID string + if err := writer.Get(&stepID, ` + SELECT step_id FROM workflow_step_participants + WHERE task_id = 'split-task' AND role = 'reviewer' + `); err != nil { + t.Fatalf("read seat step: %v", err) + } + if stepID != "step-writer" { + t.Errorf("seat landed at step %q, want step-writer — the step was resolved outside the write's exclusion", stepID) + } +} diff --git a/apps/backend/internal/office/repository/sqlite/participants.go b/apps/backend/internal/office/repository/sqlite/participants.go index ede97c5e46..50a274edd1 100644 --- a/apps/backend/internal/office/repository/sqlite/participants.go +++ b/apps/backend/internal/office/repository/sqlite/participants.go @@ -196,6 +196,14 @@ type ParticipantWriteResult struct { DisplacedAgentProfileID string } +// ErrEmptyAgentProfileID is returned by AddTaskParticipant when the +// registration names no agent. A seat exists to name an agent: one naming +// nobody satisfies the natural key, is counted by the quorum guard, and can +// never be woken or decided. Exported as a single identity so callers +// distinguish it from the store's nil-error "unchanged" outcome without +// matching on a message. +var ErrEmptyAgentProfileID = errors.New("participant: agent_profile_id required") + // AddTaskParticipant registers agentID in role for taskID, claiming an // unclaimed automatic seat in place when one exists rather than always // inserting a second seat into the role's slate. Returns @@ -211,7 +219,17 @@ type ParticipantWriteResult struct { // resolved on the transaction handle, inside that lock — never through the // read-only pool, which would escape the exclusion as surely as a // mismatched lock key would on the server dialect. +// +// A registration naming no agent is refused with ErrEmptyAgentProfileID +// before the transaction begins, so it takes no exclusion and leaves +// nothing to roll back. Only the empty identifier: a whitespace identifier +// names no agent profile and stays governed by the claim search's own +// existence check. func (r *Repository) AddTaskParticipant(ctx context.Context, taskID, agentID, role string) (ParticipantWriteResult, error) { + if agentID == "" { + return ParticipantWriteResult{}, ErrEmptyAgentProfileID + } + tx, err := r.db.BeginTxx(ctx, nil) if err != nil { return ParticipantWriteResult{}, fmt.Errorf("begin tx: %w", err) diff --git a/apps/backend/internal/office/repository/sqlite/participants_ops_test.go b/apps/backend/internal/office/repository/sqlite/participants_ops_test.go index 17df10b41c..5da8e775d9 100644 --- a/apps/backend/internal/office/repository/sqlite/participants_ops_test.go +++ b/apps/backend/internal/office/repository/sqlite/participants_ops_test.go @@ -3,6 +3,7 @@ package sqlite_test import ( "context" "testing" + "time" "github.com/kandev/kandev/internal/office/repository/sqlite" ) @@ -130,10 +131,27 @@ func TestAddTaskParticipant_ClaimsUndecidedAutoSeat(t *testing.T) { seedParticipantTask(t, repo, "ap-claim", "step-1") seedAutoSeat(t, repo, "auto-seat-1", "step-1", "ap-claim", "reviewer", "agent-auto") seedParticipantAgent(t, repo, "agent-human") + // Distinctive position and creation time: the invariance assertions + // below compare real values rather than two column defaults. + if _, err := repo.ExecRaw(ctx, ` + UPDATE workflow_step_participants + SET position = 3, created_at = '2020-05-05 01:02:03' + WHERE id = 'auto-seat-1' + `); err != nil { + t.Fatalf("set distinctive seat fields: %v", err) + } + before := readParticipantSeatRow(t, repo, "auto-seat-1") + // Guard against a vacuous comparison: if the seed above silently failed, + // before and after would both hold column defaults and every invariance + // assertion would pass without proving anything. + if before.Position != 3 || before.CreatedAt.IsZero() { + t.Fatalf("seeded seat = %+v, want position 3 and a non-zero created_at", before) + } if _, err := repo.AddTaskParticipant(ctx, "ap-claim", "agent-human", "reviewer"); err != nil { t.Fatalf("AddTaskParticipant: %v", err) } + after := readParticipantSeatRow(t, repo, "auto-seat-1") if n := participantRowCount(t, repo, "ap-claim"); n != 1 { t.Fatalf("rows = %d, want 1 (claimed in place, not duplicated)", n) @@ -154,6 +172,50 @@ func TestAddTaskParticipant_ClaimsUndecidedAutoSeat(t *testing.T) { if provenance != "manual" { t.Errorf("provenance = %q, want %q after claim", provenance, "manual") } + + // AC-OFFICE-SEAT-ASSURANCE-002.9: the agent profile and provenance are + // the only two fields a claim may touch. The seat's identity, its + // decision-required flag, its position in the slate and its creation + // time are the same row's, unchanged — a claim reassigns a seat, it does + // not mint one (AC-OFFICE-SEAT-PROVENANCE-002.2). + if after.ID != before.ID { + t.Errorf("seat id = %q, want %q (a claim reassigns the seat, not replaces it)", after.ID, before.ID) + } + if after.DecisionRequired != before.DecisionRequired { + t.Errorf("decision_required = %v, want %v (unchanged)", after.DecisionRequired, before.DecisionRequired) + } + if after.Position != before.Position { + t.Errorf("position = %d, want %d (unchanged)", after.Position, before.Position) + } + if !after.CreatedAt.Equal(before.CreatedAt) { + t.Errorf("created_at = %v, want %v (unchanged)", after.CreatedAt, before.CreatedAt) + } +} + +// participantSeatRow is the seat as the table stores it. The office-side +// Participant projection carries neither provenance nor a real creation +// time, so an invariance assertion has to read the columns. Named distinctly +// from this package's other seat-row test helper (participant_claim_decision_guard_test.go), +// which reads a different column set for a different test. +type participantSeatRow struct { + ID string `db:"id"` + AgentProfileID string `db:"agent_profile_id"` + Provenance string `db:"provenance"` + DecisionRequired bool `db:"decision_required"` + Position int `db:"position"` + CreatedAt time.Time `db:"created_at"` +} + +func readParticipantSeatRow(t *testing.T, repo *sqlite.Repository, seatID string) participantSeatRow { + t.Helper() + var row participantSeatRow + if err := repo.ReaderDB().Get(&row, ` + SELECT id, agent_profile_id, provenance, decision_required, position, created_at + FROM workflow_step_participants WHERE id = ? + `, seatID); err != nil { + t.Fatalf("read seat row %s: %v", seatID, err) + } + return row } // TestAddTaskParticipant_DoesNotClaimADecidedAutoSeat: once the auto-cast @@ -436,3 +498,16 @@ func participantRowCount(t *testing.T, repo *sqlite.Repository, taskID string) i } return n } + +// seedManualSeat inserts a seat already carrying "manual" provenance — an +// operator's confirmed choice, which no claim may consume. +func seedManualSeat(t *testing.T, repo *sqlite.Repository, id, stepID, taskID, role, agentID string) { + t.Helper() + if _, err := repo.ExecRaw(context.Background(), ` + INSERT INTO workflow_step_participants + (id, step_id, task_id, role, agent_profile_id, decision_required, position, provenance) + VALUES (?, ?, ?, ?, ?, 1, 0, 'manual') + `, id, stepID, taskID, role, agentID); err != nil { + t.Fatalf("seed manual seat: %v", err) + } +} diff --git a/apps/web/components/task/dockview-shared.tsx b/apps/web/components/task/dockview-shared.tsx index e999d74ee9..da22c5f2b8 100644 --- a/apps/web/components/task/dockview-shared.tsx +++ b/apps/web/components/task/dockview-shared.tsx @@ -43,7 +43,7 @@ import { PluginPanelTab } from "./plugin-panel-tab"; import { PromptHistoryContent } from "./prompt-history-panel-host"; import { TodosContent } from "./todos-panel-content"; -import { setPanelTitle, panelPortalManager } from "@/lib/layout/panel-portal-manager"; +import { setPanelTitle } from "@/lib/layout/panel-portal-manager"; import { getWebSocketClient } from "@/lib/ws/connection"; import { usePortalSlot } from "@/lib/layout/panel-portal-host"; import { ENV_SCOPED_DOCKVIEW_COMPONENTS } from "@/lib/state/dockview-env-scoped-components"; @@ -235,8 +235,7 @@ function ChatContent({ panelId, params }: { panelId: string; params: Record { - if (!sessionId) return; - const entry = panelPortalManager.get(panelId); - if (!entry?.api) return; - /** Ask the WebSocket client for a fresh git-status snapshot for the - * session. */ - const refreshNow = () => { - const client = getWebSocketClient(); - client?.refreshSessionData(sessionId); - }; - // If the panel is already active when this effect first runs, - // onDidActiveChange won't fire (no transition) — refresh immediately so the - // initial open benefits from the same WS-event-miss recovery. - if (entry.api.isActive) refreshNow(); - const disposable = entry.api.onDidActiveChange((event) => { - if (event.isActive) refreshNow(); - }); - return () => disposable.dispose(); - }, [panelId, sessionId]); + if (!sessionId || !isVisible) return; + // Visibility is synchronized by usePanelActive, including the initial + // portal-registration race. Ask for a fresh snapshot whenever the panel + // becomes visible or its session changes. + getWebSocketClient()?.refreshSessionData(sessionId); + }, [sessionId, isVisible]); } /** Render the changes/diff viewer for the panel's params (`kind` "all" or diff --git a/apps/web/e2e/tests/office/workflow-quorum-transitions.spec.ts b/apps/web/e2e/tests/office/workflow-quorum-transitions.spec.ts index a67a05408d..d9d9b7bb8e 100644 --- a/apps/web/e2e/tests/office/workflow-quorum-transitions.spec.ts +++ b/apps/web/e2e/tests/office/workflow-quorum-transitions.spec.ts @@ -109,14 +109,14 @@ async function getQuorumGuards( apiClient: { rawRequest: (method: string, path: string) => Promise }, workspaceId: string, taskId: string, -): Promise> { +): Promise> { const res = await apiClient.rawRequest( "GET", `/api/v1/office/workspaces/${workspaceId}/tasks/${taskId}/quorum`, ); expect(res.ok).toBe(true); const body = (await res.json()) as { - guards: Array<{ role: string; satisfied: boolean; reason?: string }>; + guards: Array<{ role: string; required_count: number; satisfied: boolean; reason?: string }>; }; return body.guards; } @@ -215,11 +215,20 @@ test.describe("Office workflow quorum-guarded transitions", () => { // AC-25: the Review step's guard is unsatisfied (reviewer has not // decided yet), so the diagnostic read reports one awaiting entry. + // + // AC-OFFICE-SEAT-PROVENANCE-006.1/-006.2: the guard must require + // exactly one decision. That count is the operator-visible consequence + // of the claim above — a registration that added a second seat instead + // of claiming the cast one leaves the role reading "reviewer" and the + // listing arguably explicable, and shows up only here, as a gate that + // silently never fires because it is waiting on two decisions a single + // reviewer can never supply. await expect - .poll( - async () => (await getQuorumGuards(apiClient, officeSeed.workspaceId, task.id))[0]?.role, - ) - .toBe("reviewer"); + .poll(async () => { + const guard = (await getQuorumGuards(apiClient, officeSeed.workspaceId, task.id))[0]; + return guard && { role: guard.role, requiredCount: guard.required_count }; + }) + .toEqual({ role: "reviewer", requiredCount: 1 }); // AC-25 UI presentation: the badge renders the awaiting state. await testPage.goto(`/office/tasks/${task.id}`); diff --git a/docs/plans/participant-seat-provenance-assurance/plan.md b/docs/plans/participant-seat-provenance-assurance/plan.md new file mode 100644 index 0000000000..bf03d375f4 --- /dev/null +++ b/docs/plans/participant-seat-provenance-assurance/plan.md @@ -0,0 +1,225 @@ +--- +created: 2026-09-09 +status: done +requirements: + - REQ-OFFICE-SEAT-ASSURANCE-001 + - REQ-OFFICE-SEAT-ASSURANCE-002 +system_design: + - ../../specs/office/system-design/participant-seat-provenance-assurance-01.md +--- + +# Implementation Plan: Participant Seat Provenance Assurance + +## Overview + +`participant-seat-provenance.md` is shipped and correct. Three review legs and +an outside-voice pass found no production defect. This initiative closes the two +gaps they did find, and nothing else. + +The first is one guard. `Repository.AddTaskParticipant` will refuse a +registration naming an empty agent profile identifier before it begins a +transaction. Today that call is unreachable — both production callers are HTTP +routes that reject it first — so this changes no observable behaviour. It exists +because the store's write is what makes the seat, and a future internal caller +reaching it directly would otherwise write a seat naming nobody: admitted by the +natural key, counted by the quorum guard, never wakeable, never decidable. + +The second is verification. Nine criteria of the shipped contract hold by +inspection and are held in place by nothing. Each work order below names the +behaviour that must become falsifiable and the mechanism that makes its window +reachable on purpose. Three of those mechanisms are the substance of this plan, +because the shipped code already answered them wrongly once: a window its own +comment calls impossible to force deterministically, a race left to the +scheduler across fifteen unasserted iterations, and an end-to-end assertion that +watches the wrong field. + +`participant-seat-provenance.md` is frozen. Nothing here edits it, and a case +that cannot be made to pass against the shipped implementation is evidence the +assurance spec is wrong — it routes back to the spec step and is never licence +to weaken the case or the code under test (`AC-OFFICE-SEAT-ASSURANCE-002.12`). + +## Backend + +### The store-boundary guard + +- Add an exported sentinel `ErrEmptyAgentProfileID` to + `apps/backend/internal/office/repository/sqlite/`, so two callers cannot + compare against two identities. +- Return it as the first statement of `AddTaskParticipant`, before `BeginTxx` + and therefore before `lockParticipantRoleSeat` acquires the shared exclusion. + A rejected call contends with nobody and has nothing to roll back. +- Compare against the empty string only. No trimming, no normalisation — a + guard that trimmed would disagree with the surface guard about what it is + guarding, and a whitespace identifier is already governed by + `AC-OFFICE-SEAT-PROVENANCE-005.8`. +- Do not place it in the service layer beside the existing role lookup, and do + not remove the surface check. Two layers is the intent: the surface returns a + client error to an operator, the store refuses to write a row. +- Map it to no HTTP status. No shipped route can reach it. + +### Test-only support + +- A logger-accepting variant of the dashboard test harness, so the + cancellation-failure case can read the warning the service already emits. + Existing callers of the harness keep their default logger. + +No other production change. No schema change, no migration, no new metric, log +line or counter. + +## Frontend + +No production frontend change. The E2E work widens one local response type in a +spec file to read a field the API already returns. + +## Tests + +Grouped by the mechanism that makes each window reachable. + +Ordinary sequential cases against a seeded slate: + +- Identity probe precedes claim search: a slate holding agent A's `manual` seat + and agent B's sole undecided `auto` seat, with A registered again, reports no + change and leaves B's seat naming B with provenance `auto` + (`AC-OFFICE-SEAT-ASSURANCE-002.1`). +- A displaced agent registered again writes a second `manual` seat: exactly two + seats, both `manual`, naming both agents (`-002.2`). +- A claim leaves the seat's identifier, decision-required flag, position and + creation time unchanged alongside the agent and provenance it changes + (`-002.9`). Creation time is a real column the office projection does not + carry, so this reads the column directly. +- The claim's activity entry is asserted by content — task, step, role, + displaced agent, claiming agent, each with its correct value — replacing the + current count-only assertion (`-002.8`). + +Divergent handles, no concurrency: + +- The task's current step must be resolved on the writing handle. Build the + repository with its writer on one store and its reader on a second whose task + row names a different step; assert the seat lands at the writer's step + (`-002.3`). Swapping `stepIDForTaskTx` for `stepIDForTask` currently passes the + whole suite; it must fail this. + +Surface refusals: + +- An empty identifier over HTTP is a client error and writes no seat (`-002.4`). +- A role outside `reviewer`/`approver` is refused and writes nothing (`-002.5`). + This is not reachable over HTTP — the routes are role-fixed — so it is a + white-box case in the service's own package. + +Deliberate mid-write window, server dialect: + +- A seat selected as claimable and removed before the conditional write applies: + a holding session takes a row lock, the registration blocks on it, the test + waits until the block is observable through the server's activity view, the + holder deletes and commits, and the registration falls through to inserting + (`-002.10`). Reaching the wait is the proof the window was entered — it + replaces the existing case's unasserted iteration split. The existing + fifteen-iteration case is kept, not replaced: it answers a different question. + +Contracted residual: + +- A claim whose cancellation of the displaced run fails still returns success, + leaves the claimed slate exactly as a successful claim does, records the + failure, and leaves the displaced run runnable (`-002.7`). + +## E2E Tests + +- The existing quorum case already walks an automatic cast followed by a manual + registration naming a different agent. Widen its local response type to carry + the required-decision count and assert it is one, alongside the existing role + assertion rather than instead of it (`-002.6`). A regression to two seats + leaves the role correct and shows up only here. + +## Implementation Waves + +Wave 1 (parallel — disjoint files and packages): + +- [x] [task-01-store-boundary-guard](task-01-store-boundary-guard.md) +- [x] [task-02-repository-provenance-cases](task-02-repository-provenance-cases.md) +- [x] [task-05-deterministic-claim-window](task-05-deterministic-claim-window.md) +- [x] [task-06-e2e-quorum-required-count](task-06-e2e-quorum-required-count.md) + +Wave 2: + +- [x] [task-03-surface-refusals-and-claim-activity](task-03-surface-refusals-and-claim-activity.md) + +Wave 3: + +- [x] [task-04-cancellation-failure-residual](task-04-cancellation-failure-residual.md) + +Tasks 03 and 04 both add files to the `dashboard` package and both touch its +shared test helpers, so they are sequenced rather than run together. + +## Risks + +- **File-size limit.** Revive's 800-effective-line limit applies to test files, + and new tests go in a new file rather than being appended to a large one. + `participant_claim_postgres_test.go` is at 788 lines and + `dashboard/participants_test.go` at 766, so the deterministic-window case, the + cancellation-failure case and the white-box role case each begin a new file. + Measure before appending anywhere; a case that crosses the limit fails lint and + moving it afterwards costs a round. +- **The wait probe.** `-002.10` depends on the registration's block being visible + through the server's activity view. If that proves unreliable, the named + fallback is the failpoint idiom this repository already uses for destructive + cutover migrations — a hook between the claim search and the conditional write, + nil in production. Take it only if the probe fails in practice, and record why + in the case's comment. Do not take it first. +- **Divergent-handle setup.** Schema initialisation runs against the writer, so + the reader store needs its own `tasks` table and row. Every assertion must read + through the writer handle, never the repository's reader accessor, which by + construction now points at the wrong database. +- **Skips must stay visible.** A dialect-gated case that silently passed without + running would report coverage this plan then relies on + (`AC-OFFICE-SEAT-ASSURANCE-002.11`). +- **Helper drift.** The multi-connection Postgres pool builder is reused, not + copied. A second copy would invite the two to drift. +- **Scope.** No case may change production behaviour, weaken an existing + assertion, or relax a criterion of the frozen contract. + +## Results + +All six work orders landed. Production change is 18 lines in one file +(`participants.go`: the sentinel, the guard, and the doc comment recording +its placement); everything else is test-side. + +Each retroactive case was checked for decisiveness by regressing the +implementation and confirming the case went red, then reverting: + +| Criterion | Regression applied | Result | +| --- | --- | --- | +| `-002.1` | claim search moved before the identity probe | RED | +| `-002.3` | `stepIDForTaskTx` → `stepIDForTask` (read-only pool) | RED | +| `-002.8` | claim activity payload emptied | RED | +| `-002.9` | claim also bumps `position` | RED | +| `-002.7` | cancellation failure swallowed without a log | RED | +| `REQ-001` | guard absent (sentinel undefined) | RED, then GREEN with the guard | + +`-002.10` is server-dialect-only and skips on a runner without +`KANDEV_TEST_POSTGRES_DSN`; it is verified by CI, whose backend-tests +workflow supplies a Postgres service. Its `-count=5` flakiness check and its +wait-probe reliability are therefore unverified locally — if the probe flakes +in CI, the named fallback is the failpoint idiom, per task 05. + +`-002.5` gained a positive control (reviewer and approver must reach the +store) so that a body refusing every role could not satisfy it. + +`-002.9`'s seed asserts its own distinctive values before the claim, so a +silently-failed seed cannot make the invariance comparison vacuous. + +`AC-001.7` (both dialects) is structural rather than tested twice: the guard +compares its own argument against the empty string and issues no query, so +there is no dialect-dependent behaviour for a second case to observe. + +## Verification + +Targeted commands are owned by each task. After all tasks: + +```bash +make -C apps/backend fmt +make -C apps/backend test lint +cd apps/web && pnpm e2e:run tests/office/workflow-quorum-transitions.spec.ts +python3 scripts/lint-spec-files.py --all +``` + +Commit through active hooks, then push and open a PR against `main`. diff --git a/docs/plans/participant-seat-provenance-assurance/task-01-store-boundary-guard.md b/docs/plans/participant-seat-provenance-assurance/task-01-store-boundary-guard.md new file mode 100644 index 0000000000..cbed945e52 --- /dev/null +++ b/docs/plans/participant-seat-provenance-assurance/task-01-store-boundary-guard.md @@ -0,0 +1,105 @@ +--- +id: "01-store-boundary-guard" +title: "Seat store refuses a registration naming no agent" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-OFFICE-SEAT-ASSURANCE-001 +acceptance_criteria: + - AC-OFFICE-SEAT-ASSURANCE-001.1 + - AC-OFFICE-SEAT-ASSURANCE-001.2 + - AC-OFFICE-SEAT-ASSURANCE-001.3 + - AC-OFFICE-SEAT-ASSURANCE-001.4 + - AC-OFFICE-SEAT-ASSURANCE-001.5 + - AC-OFFICE-SEAT-ASSURANCE-001.6 + - AC-OFFICE-SEAT-ASSURANCE-001.7 +system_design: + - ../../specs/office/system-design/participant-seat-provenance-assurance-01.md +--- + +# Task 01: Seat store refuses a registration naming no agent + +Implements `REQ-OFFICE-SEAT-ASSURANCE-001` (AC-001.1 through -001.7). + +## Acceptance + +- An exported sentinel `ErrEmptyAgentProfileID` exists in the + `repository/sqlite` package. It is one identity, not a formatted string, so a + caller can distinguish this failure from the store's *unchanged* outcome + without matching on a message (`AC-001.2`). +- `Repository.AddTaskParticipant` returns it with the zero + `ParticipantWriteResult` as its **first statement** — before `BeginTxx`, and + therefore before `lockParticipantRoleSeat` acquires the shared exclusion of + `AC-OFFICE-SEAT-PROVENANCE-004.2` (`AC-001.3`). A rejected call contends with + no other writer and leaves nothing to roll back. +- The comparison is against the empty string only. No trimming, no + normalisation (`AC-001.5`). A whitespace identifier still reaches the store + and stays governed by `AC-OFFICE-SEAT-PROVENANCE-005.8`: it claims no seat and + displaces no `auto` seat. +- No seat is written, no seat is claimed, and no seat's provenance is promoted + on the rejected path (`AC-001.1`). +- The guard consults no store state, so it reaches the same decision on both + supported dialects and on every call (`AC-001.4`, `-001.7`). +- The existing surface check in `dashboard/participants_handlers.go` + (`req.AgentProfileID == ""`) stays exactly where it is and is not moved, + weakened or collapsed into this one. Two layers is the intent. +- The guard is **not** placed in the service layer beside the + `participantFields[role]` lookup in `addOrRemoveParticipant`. Placing it there + would reproduce the gap one layer down. +- Nothing maps the sentinel to an HTTP status. No shipped route can reach it; it + falls through the existing generic error mapping. +- No behaviour observable through the shipped registration surface changes + (`AC-001.6`). + +## Tests + +New file in `repository/sqlite` (do not append to +`participants_ops_test.go`, which is at 438 lines and owned by task 02): + +- An empty identifier returns `ErrEmptyAgentProfileID` (assert with + `errors.Is`), and the task's slate for that role is unchanged — assert against + a seeded slate holding an undecided `auto` seat, so the case proves the seat + was neither claimed nor promoted, not merely that no row was added. +- The same call twice yields the same failure and no state (`AC-001.4`). +- A whitespace-only identifier is **not** rejected by this guard — it proceeds + and leaves the `auto` seat unclaimed per `AC-OFFICE-SEAT-PROVENANCE-005.8`. + This pins the boundary so the two layers cannot drift about what "empty" + means (`AC-001.5`). +- The rejection is distinguishable from the *unchanged* outcome the store + reports for a task at no step and for a task that does not exist: those return + a nil error with `ParticipantWriteOutcomeUnchanged`, this returns a non-nil + sentinel (`AC-001.2`). + +## Verification + +```bash +cd apps/backend && go test ./internal/office/repository/sqlite/... -run 'Participant|Seat' +cd apps/backend && go test ./internal/office/... +make -C apps/backend lint +``` + +Confirm `AC-001.6` directly: `go test ./internal/office/dashboard/...` passes +unchanged, with no test edited to accommodate the guard. + +## Files likely touched + +- `apps/backend/internal/office/repository/sqlite/participants.go` +- `apps/backend/internal/office/repository/sqlite/` — new guard test file +- this task file + +## Inputs + +- `docs/specs/office/requirements/participant-seat-provenance-assurance.md` + (REQ-OFFICE-SEAT-ASSURANCE-001) +- `docs/specs/office/system-design/participant-seat-provenance-assurance-01.md` + ("The store-boundary guard") +- `/tdd` + +## Output contract + +Return a compact handoff capsule with intent/acceptance, base/head SHA, changed +files and entry points, risk tags, exact RED/GREEN verification commands and +results, uncertainties, and this task status set to `done`. Do not edit +`plan.md`. diff --git a/docs/plans/participant-seat-provenance-assurance/task-02-repository-provenance-cases.md b/docs/plans/participant-seat-provenance-assurance/task-02-repository-provenance-cases.md new file mode 100644 index 0000000000..2557f3dac2 --- /dev/null +++ b/docs/plans/participant-seat-provenance-assurance/task-02-repository-provenance-cases.md @@ -0,0 +1,116 @@ +--- +id: "02-repository-provenance-cases" +title: "Repository cases: probe order, re-registration, seat invariants, writing handle" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-OFFICE-SEAT-ASSURANCE-002 +acceptance_criteria: + - AC-OFFICE-SEAT-ASSURANCE-002.1 + - AC-OFFICE-SEAT-ASSURANCE-002.2 + - AC-OFFICE-SEAT-ASSURANCE-002.3 + - AC-OFFICE-SEAT-ASSURANCE-002.9 +system_design: + - ../../specs/office/system-design/participant-seat-provenance-assurance-01.md +--- + +# Task 02: Repository cases — probe order, re-registration, seat invariants, writing handle + +Implements `AC-OFFICE-SEAT-ASSURANCE-002.1`, `-002.2`, `-002.9` and `-002.3`. +All four criteria of `participant-seat-provenance.md` they pin are already +satisfied by shipped code. This task makes them falsifiable. No production +change. + +## Acceptance + +### Identity probe precedes claim search (`-002.1` → `AC-OFFICE-SEAT-PROVENANCE-002.4`) + +- A role slate at the task's current step holds a `manual` seat for agent A and + exactly one undecided `auto` seat for a different agent B. Agent A is + registered again in that role. +- Assert the registration reports `ParticipantWriteOutcomeUnchanged`, **and** + that B's seat still names B with provenance `auto`. The second assertion is + the one that bites: an implementation that searched for a claimable seat + before probing for the named agent's own would consume B's seat. +- Extend `participant_provenance_test.go` (488 lines, has room). + +### Re-registering a displaced agent (`-002.2` → `-002.10`) + +- A claim displaces agent A in favour of agent B; A is then registered again in + the same role at the same step. +- Assert the slate holds exactly two seats, both with provenance `manual`, + naming A and B. + +### Claim leaves the rest of the seat alone (`-002.9` → `-002.2`) + +- Extend the existing coverage at `participants_ops_test.go` (currently checks + agent and provenance across a claim) to also assert the seat's **identifier**, + **decision-required flag**, **position** and **creation time** are unchanged. +- Capture all five before the claim and compare after. Creation time is a real + column on the seat table that the office-side projection does not carry, so + read the column directly rather than the projected struct. + +### The step is resolved on the writing handle (`-002.3` → `-004.10`) + +- Build the office repository with its writer on one store and its reader on a + **second** store, where the same task row names a *different* current step. +- Register a participant. Assert the seat landed at the step the **writer's** + store names. +- This must be decisive in both directions. The shipped implementation reads + through `stepIDForTaskTx` on the transaction and lands at the writer's step; + an implementation using `stepIDForTask` on the read-only pool lands at the + reader's step. Verify by temporarily making that one-word swap and confirming + the case goes red — today that swap passes the whole suite. Revert the swap. +- Prefer a **divergent** step id over an absent reader row: divergence proves + the seat landed at the writer's step, absence only proves the reader was not + consulted. +- Two setup constraints: schema initialisation runs against the writer, so give + the reader store its own `tasks` table and row explicitly; and every assertion + must read through the writer handle directly, never the repository's reader + accessor, which by construction now points at the wrong database. +- New file — this needs its own two-store fixture and does not belong in either + existing file. + +## Constraints + +- No production code changes in this task. A case that cannot be made to pass is + evidence about the spec, not licence to edit the implementation + (`AC-OFFICE-SEAT-ASSURANCE-002.12`). +- Do not weaken or delete any existing assertion. `-002.9` **extends** the + existing case; `-002.1` and `-002.2` are new cases beside existing ones. +- Measure file length before appending. Revive's 800-effective-line limit applies + to test files. + +## Verification + +```bash +cd apps/backend && go test ./internal/office/repository/sqlite/... -run 'Participant|Seat|Claim' +cd apps/backend && go test ./internal/office/... +make -C apps/backend lint +``` + +Record the RED result for the `-002.3` swap experiment described above. + +## Files likely touched + +- `apps/backend/internal/office/repository/sqlite/participant_provenance_test.go` +- `apps/backend/internal/office/repository/sqlite/participants_ops_test.go` +- `apps/backend/internal/office/repository/sqlite/` — new step-resolution test file +- this task file + +## Inputs + +- `docs/specs/office/requirements/participant-seat-provenance.md` — full AC text + for `-002.4`, `-002.10`, `-002.2`, `-004.10` +- `docs/specs/office/system-design/participant-seat-provenance-assurance-01.md` + ("Making each window reachable") +- `/tdd` + +## Output contract + +Return a compact handoff capsule with intent/acceptance, base/head SHA, changed +files and entry points, risk tags, exact RED/GREEN verification commands and +results (including the swap experiment), uncertainties, and this task status set +to `done`. Do not edit `plan.md`. diff --git a/docs/plans/participant-seat-provenance-assurance/task-03-surface-refusals-and-claim-activity.md b/docs/plans/participant-seat-provenance-assurance/task-03-surface-refusals-and-claim-activity.md new file mode 100644 index 0000000000..6e8e94b366 --- /dev/null +++ b/docs/plans/participant-seat-provenance-assurance/task-03-surface-refusals-and-claim-activity.md @@ -0,0 +1,111 @@ +--- +id: "03-surface-refusals-and-claim-activity" +title: "Surface refusals and claim activity payload" +status: done +wave: 2 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-OFFICE-SEAT-ASSURANCE-002 +acceptance_criteria: + - AC-OFFICE-SEAT-ASSURANCE-002.4 + - AC-OFFICE-SEAT-ASSURANCE-002.5 + - AC-OFFICE-SEAT-ASSURANCE-002.8 +system_design: + - ../../specs/office/system-design/participant-seat-provenance-assurance-01.md +--- + +# Task 03: Surface refusals and claim activity payload + +Implements `AC-OFFICE-SEAT-ASSURANCE-002.4`, `-002.5` and `-002.8`, pinning +`AC-OFFICE-SEAT-PROVENANCE-005.1`, `-005.7` and `-002.9`. No production change. + +Sequenced into wave 2 because it and task 04 both add files to the `dashboard` +package and both touch its shared test helpers. + +## Acceptance + +### Empty identifier over HTTP (`-002.4` → `-005.1`) + +- Post a registration naming an empty `agent_profile_id`. Assert a client error + (the guard at `dashboard/participants_handlers.go`, in `addParticipant`), and + that the slate for that task and role is still empty. +- Cover both add endpoints if they are separately routed, since each names its + own role. + +### A role outside reviewer/approver (`-002.5` → `-005.7`) + +- This refusal is **not reachable over HTTP**, and that is worth stating rather + than rediscovering: the routes are role-fixed, each endpoint names its own + role, and the service exposes only reviewer and approver entry points. There + is no request that carries a bad role. +- The refusal that exists is the `participantFields[role]` lookup miss inside + `addOrRemoveParticipant`, the shared body both entry points call. +- **Mechanism: a white-box case**, in the service's own package (`package + dashboard`) rather than its external test package, calling that shared body + with a role outside the pair and asserting it fails and writes nothing. Go + permits both packages in one directory, so this is a **new file**, not a change + to the existing ones. +- A structural assertion over the role map instead would pass against a body + that ignored the map. It is the weaker choice; do not take it. +- This calls beneath the HTTP surface but **above** the permission check the + body performs, so it does not bypass authorization — it exercises the same + refusal an internal caller would meet. + +### Claim activity asserted by content (`-002.8` → `-002.9`) + +- The existing assertion in `dashboard/participants_test.go` counts rows with + `action='task_participant_claimed'` and never inspects the `details` payload, + so a wrong or empty payload passes. +- Replace the count-only check with a content check: decode `details` and assert + it names the **task**, the **step**, the **role**, the **displaced agent + profile** and the **claiming agent profile**, each with its correct value. +- Keep the count assertion as well — exactly one entry, with the right payload. +- `logParticipantClaimActivity` in `dashboard/service_tasks.go` is the writer; + read the payload shape from it rather than guessing key names. + +## Constraints + +- No production code changes. A case that cannot be made to pass is evidence + about the spec (`AC-OFFICE-SEAT-ASSURANCE-002.12`). +- `dashboard/participants_test.go` is at 766 lines against a 800-effective-line + limit. Measure before appending; the content assertion replaces an existing + block so it should roughly break even, but the HTTP refusal case may need a new + file. +- The white-box case must be a new file regardless, because of its package. + +## Verification + +```bash +cd apps/backend && go test ./internal/office/dashboard/... -run 'Participant|Claim|Role' +cd apps/backend && go test ./internal/office/... +make -C apps/backend lint +``` + +Demonstrate `-002.8` is decisive: confirm the case fails if the payload is +emptied. + +## Files likely touched + +- `apps/backend/internal/office/dashboard/participants_test.go` +- `apps/backend/internal/office/dashboard/` — new white-box (`package dashboard`) test file +- `apps/backend/internal/office/dashboard/` — new HTTP refusal test file, if the + existing file has no room +- this task file + +## Inputs + +- `docs/specs/office/requirements/participant-seat-provenance.md` — `-005.1`, + `-005.7`, `-002.9` +- `docs/specs/office/system-design/participant-seat-provenance-assurance-01.md` + ("The two surface refusals") +- `apps/backend/internal/office/dashboard/service_tasks.go` — + `addOrRemoveParticipant`, `logParticipantClaimActivity` +- `/tdd` + +## Output contract + +Return a compact handoff capsule with intent/acceptance, base/head SHA, changed +files and entry points, risk tags, exact RED/GREEN verification commands and +results, uncertainties, and this task status set to `done`. Do not edit +`plan.md`. diff --git a/docs/plans/participant-seat-provenance-assurance/task-04-cancellation-failure-residual.md b/docs/plans/participant-seat-provenance-assurance/task-04-cancellation-failure-residual.md new file mode 100644 index 0000000000..60e1a3e677 --- /dev/null +++ b/docs/plans/participant-seat-provenance-assurance/task-04-cancellation-failure-residual.md @@ -0,0 +1,107 @@ +--- +id: "04-cancellation-failure-residual" +title: "A claim whose displaced-run cancellation fails still succeeds" +status: done +wave: 3 +depends_on: + - "03-surface-refusals-and-claim-activity" +plan: "plan.md" +requirements: + - REQ-OFFICE-SEAT-ASSURANCE-002 +acceptance_criteria: + - AC-OFFICE-SEAT-ASSURANCE-002.7 +system_design: + - ../../specs/office/system-design/participant-seat-provenance-assurance-01.md +--- + +# Task 04: A claim whose displaced-run cancellation fails still succeeds + +Implements `AC-OFFICE-SEAT-ASSURANCE-002.7`, pinning the **second sentence** of +`AC-OFFICE-SEAT-PROVENANCE-006.6`. + +Sequenced after task 03: both add files to the `dashboard` package and this one +introduces a variant of its shared test harness. + +## Problem + +`AC-OFFICE-SEAT-PROVENANCE-006.6`'s second sentence is the only place the +contract accepts a residual: the claim commits, the displaced agent's queued run +cannot be cancelled, the registration still succeeds, and one run stays runnable +for an agent no longer seated. `cancelDisplacedRun` in +`dashboard/service_tasks.go` logs and swallows the error. Nothing tests that +branch. + +## Acceptance + +**Mechanism: take the runs store away for the duration of the call, then give it +back.** Seed the fan-out run for the displaced agent, rename the runs table +aside, drive the registration, then restore it. + +Renaming rather than dropping is what makes the fourth assertion possible: after +the restore the run is still there and still runnable, which *is* the residual +the criterion describes. A dropped table would take the evidence with it. + +Four assertions, one per clause of the sentence: + +1. The request **succeeds** — the registration returns no error. +2. The claimed slate is **exactly as a successful claim leaves it**: one seat, + naming the claiming agent, provenance `manual`. +3. The failure is **recorded**. +4. The displaced agent's run is **still runnable** after the restore. + +For (3), the service takes a logger at construction and the logger package can +be built from a zap logger, so an **observer core at warning level** supplies +it. The existing shared test harness hands the service a default logger, so add +a **logger-accepting variant of that harness** rather than changing every caller +of it. + +Assert on the observed entry's **fields** — the task, the step, the displaced +agent profile — **not** on its message text. + +## Constraints + +- No production code changes. The warning this case asserts already exists and + is already emitted; the case reads it rather than adding it, which is what + makes the assertion evidence about shipped behaviour. +- The harness variant is test-only and must leave existing callers' behaviour + identical. +- The table rename mutates schema **within the test's own isolated database**, + in the same family as the existing case that drops the decision table to stand + in for a storage error. It must reach no shared state, and must restore the + table even if the case fails partway — use `t.Cleanup`. +- New file. `dashboard/participants_test.go` is at 766 lines against a + 800-effective-line limit. + +## Verification + +```bash +cd apps/backend && go test ./internal/office/dashboard/... -run 'Cancel|Displaced|Claim' +cd apps/backend && go test ./internal/office/... +make -C apps/backend lint +``` + +Confirm the harness change is inert: the full `./internal/office/dashboard/...` +suite passes with no existing case edited. + +## Files likely touched + +- `apps/backend/internal/office/dashboard/` — new cancellation-failure test file +- `apps/backend/internal/office/dashboard/` — shared test harness, logger-accepting variant +- this task file + +## Inputs + +- `docs/specs/office/requirements/participant-seat-provenance.md` — `-006.6` +- `docs/specs/office/system-design/participant-seat-provenance-assurance-01.md` + ("A cancellation that fails") +- `apps/backend/internal/office/dashboard/service_tasks.go` — + `cancelDisplacedRun`, `applyParticipantAddOutcome` +- Completed task 03. +- `/tdd` + +## Output contract + +Return a compact handoff capsule with intent/acceptance, base/head SHA, changed +files and entry points, risk tags, exact RED/GREEN verification commands and +results, uncertainties, and this task status set to `done`. Do not edit +`plan.md`. diff --git a/docs/plans/participant-seat-provenance-assurance/task-05-deterministic-claim-window.md b/docs/plans/participant-seat-provenance-assurance/task-05-deterministic-claim-window.md new file mode 100644 index 0000000000..b94af7e649 --- /dev/null +++ b/docs/plans/participant-seat-provenance-assurance/task-05-deterministic-claim-window.md @@ -0,0 +1,123 @@ +--- +id: "05-deterministic-claim-window" +title: "Claim target vanishes mid-write, on purpose" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-OFFICE-SEAT-ASSURANCE-002 +acceptance_criteria: + - AC-OFFICE-SEAT-ASSURANCE-002.10 +system_design: + - ../../specs/office/system-design/participant-seat-provenance-assurance-01.md +--- + +# Task 05: Claim target vanishes mid-write, on purpose + +Implements `AC-OFFICE-SEAT-ASSURANCE-002.10`, pinning +`AC-OFFICE-SEAT-PROVENANCE-004.8`. + +## Problem + +This is the window that protects `attemptClaim`'s zero-rows fallthrough: a seat +selected as claimable by `findClaimableAutoSeat`, removed before `claimAutoSeat`'s +conditional write applies, and the registration falling through to inserting its +own seat rather than completing having written nothing. + +The shipped comment on that path says the window "cannot be forced +deterministically". The Postgres case that stands in for it +(`TestPostgresAddTaskParticipant_ConvergesWithConcurrentRemoveOfClaimTarget`) +runs fifteen concurrent iterations whose split it logs but does not assert — so +a run in which the window was never entered passes identically to one in which +it was. A regression in the fallthrough could pass on an unlucky scheduling run. + +The premise is wrong, and the reason is visible in the code: the claim search is +a plain read and the claim is a conditional `UPDATE` of one row by identifier. A +plain read is not blocked by a row lock; an `UPDATE` is. An outside session can +stand exactly between them. + +## Acceptance + +New file in `repository/sqlite`. `participant_claim_postgres_test.go` is at 788 +lines against a 800-effective-line limit — do not append to it. + +Server dialect, three connections: + +1. A holding session opens a transaction and takes a row lock on the seat + (`SELECT ... FOR UPDATE`). Nothing else is locked. +2. The registration runs in a goroutine. Its advisory lock is uncontended, its + identity probe misses, its claim search reads the seat *through the row lock* + and selects it as the candidate. Its conditional write then blocks. +3. The test waits until the registration's backend is observably waiting on a + lock, through the server's own activity view. **Reaching this wait is the + proof** — it establishes the candidate was selected and the write not yet + applied, which is the window itself. This replaces the existing case's + unasserted iteration split. +4. The holding session deletes the seat and commits. +5. The blocked write re-evaluates, matches no row, reports zero rows affected; + the registration falls through and inserts. + +Assert: the outcome is `Inserted`; exactly one seat exists; it names the +registering agent with provenance `manual`; and its identifier **differs** from +the seat that was removed. An implementation that reported a claim on a zero-row +write would leave the slate empty and the outcome `Claimed`, failing on both +counts. + +- **No production code changes.** Choosing an external handle over the failpoint + idiom is the point: the window is entered on the shipped path, not on a path + that exists only under test. +- The existing fifteen-iteration case is **kept, not replaced**. It exercises + genuine cross-connection concurrency, which the deterministic case does not; + the two answer different questions. +- Reuse the multi-connection Postgres pool builder already defined in that + package. Do not copy it — a second copy would invite the two to drift. +- The case must skip **visibly** when the server dialect is unavailable, using + the same gate the existing Postgres cases use, and must never report success + without having run (`AC-OFFICE-SEAT-ASSURANCE-002.11`). + +## Fallback + +If the wait probe proves unreliable in practice — a server build where the wait +is not visible, or a probe that flakes — fall back to the failpoint idiom this +repository already establishes for destructive cutover migrations: a hook +between the claim search and the conditional write, nil in production, that the +test uses to perform the removal. **Take this only if the probe fails in +practice**, and record why in the case's comment. Do not take it first. + +A flaky case is a defect in the case, not a cost of doing business. The +mechanism is chosen to be deterministic precisely so flakiness is a signal. + +## Verification + +```bash +cd apps/backend && go test ./internal/office/repository/sqlite/... -run 'Postgres.*Claim|ClaimWindow' -count=5 +cd apps/backend && go test ./internal/office/... +make -C apps/backend lint +``` + +`-count=5` is the flakiness check: a deterministic window passes five for five. +Report whether the server dialect was available on this runner; if it was not, +say so plainly rather than reporting the case as covered. + +## Files likely touched + +- `apps/backend/internal/office/repository/sqlite/` — new deterministic-window test file +- this task file + +## Inputs + +- `docs/specs/office/requirements/participant-seat-provenance.md` — `-004.8` +- `docs/specs/office/system-design/participant-seat-provenance-assurance-01.md` + ("The claim target must vanish mid-write, on purpose") +- `apps/backend/internal/office/repository/sqlite/participants.go` — + `attemptClaim`, `findClaimableAutoSeat`, `claimAutoSeat` +- `/tdd` + +## Output contract + +Return a compact handoff capsule with intent/acceptance, base/head SHA, changed +files and entry points, risk tags, exact verification commands and results +(including the `-count=5` run and dialect availability), whether the fallback was +taken and why, uncertainties, and this task status set to `done`. Do not edit +`plan.md`. diff --git a/docs/plans/participant-seat-provenance-assurance/task-06-e2e-quorum-required-count.md b/docs/plans/participant-seat-provenance-assurance/task-06-e2e-quorum-required-count.md new file mode 100644 index 0000000000..a87fbd7fd1 --- /dev/null +++ b/docs/plans/participant-seat-provenance-assurance/task-06-e2e-quorum-required-count.md @@ -0,0 +1,76 @@ +--- +id: "06-e2e-quorum-required-count" +title: "E2E quorum case asserts exactly one decision is required" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-OFFICE-SEAT-ASSURANCE-002 +acceptance_criteria: + - AC-OFFICE-SEAT-ASSURANCE-002.6 +system_design: + - ../../specs/office/system-design/participant-seat-provenance-assurance-01.md +--- + +# Task 06: E2E quorum case asserts exactly one decision is required + +Implements `AC-OFFICE-SEAT-ASSURANCE-002.6`, pinning +`AC-OFFICE-SEAT-PROVENANCE-006.1` and `-006.2`. + +## Acceptance + +- `apps/web/e2e/tests/office/workflow-quorum-transitions.spec.ts` already walks + the scenario the whole contract exists for: a step entry casts an `auto` seat, + an operator then registers a *different* agent, and the slate must end at one + seat. It asserts the guard's role and the participant listing. It does not + assert how many decisions the guard requires — which is the operator-visible + consequence. +- The count is already on the wire. Widen `getQuorumGuards`' local response type + (currently `Array<{ role, satisfied, reason? }>`) to carry the required count, + and assert it is exactly one — **alongside** the existing role assertion, not + instead of it. +- A regression to two seats in that slate must fail this assertion. With two + seats the role stays correct and the listing is arguably explicable; the gate + simply never fires. +- Confirm the field name against the actual `/quorum` response rather than + assuming it. Read the handler's response shape before naming it in the type. +- No new endpoint, fixture or spec file. No production frontend change. + +## Constraints + +- Do not reorder the case. The registration happens **after** the step move + because seats bind to the task's current step; that ordering is the scenario, + not an accident to tidy. +- Do not weaken the existing role or listing assertions. +- New user-facing copy is not involved here, so the i18n gates are not in play — + but if any assertion string is added to a non-E2E file, `t()` rules apply. + +## Verification + +```bash +cd apps/web && pnpm e2e:run tests/office/workflow-quorum-transitions.spec.ts +cd apps/web && pnpm run typecheck +cd apps && pnpm --filter @kandev/web lint +``` + +Demonstrate the assertion is decisive: confirm what the required count reads as +in the passing run, and state in the handoff how a two-seat slate would present. + +## Files likely touched + +- `apps/web/e2e/tests/office/workflow-quorum-transitions.spec.ts` +- this task file + +## Inputs + +- `docs/specs/office/requirements/participant-seat-provenance.md` — `-006.1`, `-006.2` +- `docs/specs/office/system-design/participant-seat-provenance-assurance-01.md` + ("The guard requires exactly one decision") +- `/e2e` + +## Output contract + +Return a compact handoff capsule with intent/acceptance, base/head SHA, changed +files and entry points, risk tags, exact E2E verification commands and results, +uncertainties, and this task status set to `done`. Do not edit `plan.md`. diff --git a/docs/specs/office/requirements/participant-seat-provenance-assurance.md b/docs/specs/office/requirements/participant-seat-provenance-assurance.md new file mode 100644 index 0000000000..7f3c9dce95 --- /dev/null +++ b/docs/specs/office/requirements/participant-seat-provenance-assurance.md @@ -0,0 +1,307 @@ +--- +status: draft +system: office +created: 2026-09-08 +owners: + - kandev +--- + +# Office Participant Seat Provenance Assurance Requirements + +## Overview + +`participant-seat-provenance.md` is shipped and its criteria are met by the +code that satisfies it. Three independent review legs and an outside-voice +pass found no production defect. What they did find is a gap of a different +kind: several of that contract's criteria hold *by inspection* and are not +held in place by anything, and one guard the contract relies on exists at +only one layer. + +Both are the same failure mode seen from two sides. A criterion nothing can +falsify is a criterion the next change can break silently, and a guard at +one layer is a guard the next caller can walk around. This document owns +the two repairs. + +The first is behavioural and small: the seat store itself refuses a +registration that names no agent. Today that is unreachable, because the +only two production callers are HTTP routes that reject it first +(`AC-OFFICE-SEAT-PROVENANCE-005.1`). It is worth having anyway, because the +store's write is what makes the seat, and a future internal caller — an +agent tool, a scheduler, a fixup path — reaching it directly would write a +seat naming nobody, which no reader can interpret and no fan-out can wake. + +The second is a verification contract. It names the specific regressions +the seat-provenance suite must be able to fail on, in terms of the +behaviour that would change, not the code that would change. This is not +a style preference: this system already treats detectability as a +first-class property, because a suite that stays green through a +reintroduced defect is worse than no suite, having spent the cost and +bought the confidence without the coverage. + +Office owns both. The registration surface, the fallback casting and the +seat writer are Office's; the contract they implement is +`participant-seat-provenance.md`, also Office's. Nothing here changes that +document, which is frozen. Everything here either adds a guard beneath it +or pins down how it is proven. + +## Terminology + +- **Seat**, **role slate**, **seat provenance**, **claim**, **automatic + casting**, **manual registration**, **unclaimed seat**, **decided seat** + and **supported dialect** are used exactly as + `participant-seat-provenance.md` defines them. +- **Seat store** — the in-process writer that creates, claims and removes + seats. Reached today only through the registration surface; the surface + and the store are different layers and this document distinguishes them + deliberately. +- **Registration surface** — the operator-facing per-task registration + entry point, reachable for the `reviewer` and `approver` roles, which + the seat store sits behind. +- **The suite** — the automated tests that run in CI for this repository, + taken as a whole. A criterion satisfied by a test that never runs in CI + is not satisfied. +- **Named regression** — a described change in behaviour, stated as + behaviour rather than as an edit to code, that would violate a criterion + of `participant-seat-provenance.md`. + +## Prior art + +Each leg opens with what it searched. + +**Our own prior reasoning — leg unavailable on this runner, and it is +named rather than skipped.** Searched this machine for the `wiki-query` +skill: `~/.claude/skills` holds 57 entries and none is `wiki-query`; +`which wiki-query` reports not found; there is no `~/.obsidian-wiki` +config symlink and no vault directory under `~/Documents`. No +`OBSIDIAN_VAULT_PATH` resolved, so no QMD collection was queried and no +grep fallback ran either. This card was moved onto the `neo` SSH runner, +which does not carry the vault; the leg is unavailable here, not empty. + +What partly stands in for it is that this exact concept was already put +through that leg on the primary box, and the result is recorded in the +shipped contract's own *Prior art*: the vault at +`/Users/henry/Documents/henry/wiki`, QMD collection `wiki`, three queries, +nothing on seat provenance or slates. One adjacent page, +`concepts/optimistic-vs-pessimistic-concurrency`, was load-bearing there +and stays load-bearing here: it is why the two writers block on a shared +exclusion rather than repair a duplicate afterwards, which is in turn why +`REQ-OFFICE-SEAT-ASSURANCE-002` insists the mid-write windows be entered +deliberately rather than hoped for. + +**What others shipped — leg unavailable.** The `saas-kb` MCP server is not +among this session's tools, so `search_fsm_docs` could not be called with +`category: "ai_sdlc"` or any other filter. The shipped contract records +that leg's earlier result for this concept: two queries, top relevance +0.0139, nothing describing a placeholder-versus-preference relation. No +vendor behaviour is copied here. + +**In-repo prior positions — this leg ran.** Searched +`apps/backend/AGENTS.md` and `docs/decisions/INDEX.md` for testing +conventions bearing on determinism and detectability. Three positions +found, all followed rather than re-derived: + +- A test-only failpoint injected mid-operation is an established idiom + here, not a novelty to justify: destructive cutover migrations "inject a + test-only failpoint after each cutover step" to make a window + deterministic. +- Detectability is already treated as first class. The PR-sync guidance + requires a call-count test in as many words, because "an assertion on + the resulting `PRStatus` passes with the whole regression restored" — + the same argument this document generalises. +- Revive's 800-effective-line file limit applies to test files, with new + tests going in a new file rather than being appended to a large one. + Three of the four files this work touches are already between 438 and + 788 lines, so this constrains where the new cases can live. + +**What we are doing differently.** The failpoint idiom above is available +but is not the default chosen here. Where a mid-write window can be +entered from outside the process — by holding a lock the writer must wait +on — that is preferred, because it proves the window on the shipped code +path rather than on a path that only exists when a test is running. The +failpoint stays as the named fallback for a window with no external +handle. + +## Requirements + +### REQ-OFFICE-SEAT-ASSURANCE-001: The seat store refuses a registration naming no agent + +**Intent:** A seat exists to name an agent. A seat naming nobody satisfies +the store's natural key, is admitted silently, is counted by the quorum +guard, and can never be woken or decided — a permanent stall written in +one row. The registration surface already refuses it; the store should +refuse it too, so that the guarantee belongs to the writer rather than to +the caller that happens to be in front of it today. + +#### Acceptance criteria + +- **AC-OFFICE-SEAT-ASSURANCE-001.1:** When a registration reaches the seat + store naming an empty agent profile identifier, the store shall reject + it, writing no seat, claiming no seat and promoting no seat's + provenance. +- **AC-OFFICE-SEAT-ASSURANCE-001.2:** That rejection shall be reported to + the caller as a failure, and shall be distinguishable from the + no-change outcome the store reports for a task that stands at no step or + does not exist (`AC-OFFICE-SEAT-PROVENANCE-005.2`, `-005.6`). A caller + shall be able to tell the two apart without matching on a message + string. +- **AC-OFFICE-SEAT-ASSURANCE-001.3:** The rejection shall occur before the + store begins a transaction and before it acquires the shared exclusion + of `AC-OFFICE-SEAT-PROVENANCE-004.2`. A rejected call shall therefore + contend with no other writer, leave nothing to roll back, and not delay + a concurrent automatic casting or registration for the same task and + role. +- **AC-OFFICE-SEAT-ASSURANCE-001.4:** The rejection shall be deterministic + and carry no state. The same call rejected twice yields the same + failure, the store shall not retry it, and no caller shall be required + to. +- **AC-OFFICE-SEAT-ASSURANCE-001.5:** Only the empty identifier is + rejected by this guard. An identifier that is non-empty but names no + agent profile — including one that is only whitespace — stays governed + by `AC-OFFICE-SEAT-PROVENANCE-005.8`: it claims no seat and displaces no + `auto` seat, and whether it writes one of its own is unchanged. The + boundary is stated so the two layers cannot drift into disagreeing about + what "empty" means. +- **AC-OFFICE-SEAT-ASSURANCE-001.6:** No behaviour observable through the + shipped registration surface shall change. Every request that reaches + the store through that surface already carries a non-empty identifier, + so no status code, activity entry, notification, seat or provenance + differs before and after this guard exists. +- **AC-OFFICE-SEAT-ASSURANCE-001.7:** The guard shall hold on both + supported dialects, and shall reach the same decision on both without + consulting the store, since it inspects only its own argument. + +### REQ-OFFICE-SEAT-ASSURANCE-002: Named seat-provenance regressions fail the suite + +**Intent:** Every criterion below is already satisfied by shipped code. +What is missing is that nothing would notice if it stopped being. Each +criterion names one behaviour that must be asserted and, where the sharp +edge is a specific wrong implementation rather than a missing scenario, +the behaviour change that must turn the suite red. + +**User story:** As an engineer changing seat registration, I want the +suite to fail when I reintroduce a defect this contract already fixed, so +that I learn it from CI rather than from a stalled quorum guard in +production. + +#### Acceptance criteria + +- **AC-OFFICE-SEAT-ASSURANCE-002.1:** The suite shall cover a role slate + at the task's current step holding a `manual` seat for one agent and + exactly one undecided `auto` seat for a different agent, with the first + agent registered again in that role, asserting that the registration + reports no change and that the second agent's seat still names that + agent with provenance `auto`. An implementation that searches for a + claimable seat before checking whether the named agent already holds one + shall fail this case (`AC-OFFICE-SEAT-PROVENANCE-002.4`). +- **AC-OFFICE-SEAT-ASSURANCE-002.2:** The suite shall cover a claim that + displaces one agent in favour of another, followed by the displaced + agent being registered again in the same role at the same step, + asserting the slate then holds exactly two seats, both `manual`, naming + those two agents (`AC-OFFICE-SEAT-PROVENANCE-002.10`). +- **AC-OFFICE-SEAT-ASSURANCE-002.3:** The suite shall cover a registration + whose read-only view of the task names a different current step than + the view its write goes through, asserting the seat lands at the step + the write view names. An implementation that resolves the task's step + outside its own exclusion shall fail this case + (`AC-OFFICE-SEAT-PROVENANCE-004.10`). +- **AC-OFFICE-SEAT-ASSURANCE-002.4:** The suite shall cover a registration + request naming an empty agent profile identifier, asserting the surface + rejects it with a client error and that no seat is written for that task + and role (`AC-OFFICE-SEAT-PROVENANCE-005.1`). +- **AC-OFFICE-SEAT-ASSURANCE-002.5:** The suite shall cover a participant + registration naming a role other than `reviewer` or `approver`, + asserting it is refused and that no seat is written or claimed + (`AC-OFFICE-SEAT-PROVENANCE-005.7`). +- **AC-OFFICE-SEAT-ASSURANCE-002.6:** The end-to-end quorum case that + walks an automatic cast followed by a manual registration naming a + different agent shall assert the resulting guard requires exactly one + decision, not only that the guard names the expected role. A regression + to two seats in that slate shall fail it + (`AC-OFFICE-SEAT-PROVENANCE-006.1`, `-006.2`). +- **AC-OFFICE-SEAT-ASSURANCE-002.7:** The suite shall cover a claim whose + cancellation of the displaced agent's queued run fails, asserting that + the registration still reports success, that the claimed slate is + exactly as a successful claim leaves it, that the failure is recorded, + and that the displaced agent's run is still runnable afterwards + (`AC-OFFICE-SEAT-PROVENANCE-006.6`, second sentence). +- **AC-OFFICE-SEAT-ASSURANCE-002.8:** The claim's activity entry shall be + asserted by content, not by count: the recorded detail shall be checked + to name the task, the step, the role, the displaced agent profile and + the claiming agent profile, each with its correct value. An entry + written with an empty or wrong payload shall fail the case + (`AC-OFFICE-SEAT-PROVENANCE-002.9`). +- **AC-OFFICE-SEAT-ASSURANCE-002.9:** A claim shall be asserted to leave + the claimed seat's identifier, decision-required flag, position and + creation time unchanged, in addition to the agent profile and + provenance it changes (`AC-OFFICE-SEAT-PROVENANCE-002.2`). +- **AC-OFFICE-SEAT-ASSURANCE-002.10:** The case covering a seat removed + after a registration selected it and before the claim was applied shall + enter that window deliberately rather than by scheduling chance. It + shall establish that the seat was selected and then removed, and assert + the registration writes a new `manual` seat for its named agent. An + implementation that reports a claim when its conditional write matched + no seat shall fail it + (`AC-OFFICE-SEAT-PROVENANCE-004.8`). +- **AC-OFFICE-SEAT-ASSURANCE-002.11:** A case that can only be entered on + one supported dialect shall skip explicitly when that dialect is + unavailable, and shall never report success without having run. A + criterion whose only case skipped is not covered, and the suite shall + make that visible rather than green. +- **AC-OFFICE-SEAT-ASSURANCE-002.12:** No case added under this + requirement shall change production behaviour, weaken an existing + assertion, or relax a criterion of `participant-seat-provenance.md`. A + case that cannot be made to pass against the shipped implementation is + evidence about this document, which is then wrong and routes back here, + not licence to edit the code it tests. + +## Out of scope + +- **Rejecting an empty participant role at the seat store.** The store's + own `role` column admits only the five defined roles, so an empty role + is already refused structurally and cannot produce the silent malformed + seat an empty agent identifier can. Which of those five the + registration surface exposes is that surface's policy + (`AC-OFFICE-SEAT-PROVENANCE-005.7`); moving it down a layer would + duplicate the decision in two places that can then disagree. +- **Guarding participant *removal*.** This guard is on the registration + write only. Removal naming an empty agent profile identifier deletes by + exact match on that identifier, so it can only ever match a seat that is + already malformed and never a well-formed one; it is not a destructive + wildcard and needs no guard. Removal's scope stays exactly as + `AC-OFFICE-SEAT-PROVENANCE-003.4` defines it. +- **Rejecting an empty task identifier at the seat store.** A + registration naming no task resolves no current step, and + `AC-OFFICE-SEAT-PROVENANCE-005.6` already contracts that as writing + nothing and raising no failure. Turning it into a rejection would change + contracted, shipped silence, which is a different decision from adding a + guard beneath an existing one. +- **Mapping `REQ-OFFICE-SEAT-ASSURANCE-001`'s failure to an HTTP status.** + No shipped route can reach it, so a status mapping would be a contract + written for a caller that does not exist. The first caller that needs + one brings its own requirement. +- **Changing any criterion of `participant-seat-provenance.md`.** It is + frozen. This document adds a guard beneath it and pins how it is proven; + where the two ever conflict, that document wins and this one is wrong. +- **Introducing a mutation-testing tool or CI gate.** + `REQ-OFFICE-SEAT-ASSURANCE-002` names specific regressions in prose and + asks for cases that catch them. It does not ask for a harness that + generates mutants, nor for a coverage threshold. +- **Coverage for criteria not named above.** The criteria listed are the + ones a review pass found unheld. Silence here about another criterion + means it was not examined, not that it is proven. +- **Waking the claiming agent, surfacing provenance in the API, and + collapsing multiple `manual` seats.** All three are excluded by + `participant-seat-provenance.md` and stay excluded; nothing here reopens + them. + +## Dependencies + +- `participant-seat-provenance.md` owns every criterion referenced above. + This document adds no seating behaviour of its own beyond + `REQ-OFFICE-SEAT-ASSURANCE-001`, and changes none of that document's. +- `review-participant-seats.md` owns automatic casting, which is one of + the two writers every concurrency case here exercises. No criterion in + it changes. +- `step-entry-sequence-execution.md` owns whether a step's entry actions + run at all, and therefore whether an `auto` seat exists to be claimed in + the end-to-end case. diff --git a/docs/specs/office/system-design/participant-seat-provenance-assurance-01.md b/docs/specs/office/system-design/participant-seat-provenance-assurance-01.md new file mode 100644 index 0000000000..7c5986f019 --- /dev/null +++ b/docs/specs/office/system-design/participant-seat-provenance-assurance-01.md @@ -0,0 +1,342 @@ +--- +status: draft +system: office +requirements: + - REQ-OFFICE-SEAT-ASSURANCE-001 + - REQ-OFFICE-SEAT-ASSURANCE-002 +--- + +# Office Participant Seat Provenance Assurance System Design + +## Purpose and boundaries + +`participant-seat-provenance-assurance.md` asks for two things: a guard the +seat store applies to its own argument, and a set of behaviours the suite +must be able to fail on. The first is four lines of code and needs a design +only for where it sits and what it returns. The second is where the real +design work is, and it is not optional detail: a criterion that says "the +suite shall cover X" and stops there hands the builder the hardest question +— *how do you make X happen on purpose* — with no answer. + +Three of those questions have already been answered wrongly once in this +area, in the shipped code's own comments: a window that "cannot be forced +deterministically", a race left to the scheduler across fifteen iterations, +and an end-to-end assertion that watched the wrong field. So this document +names a mechanism for each, and says what to do when the mechanism does not +hold. + +It owns no seating behaviour. `participant-seat-provenance.md` and +`review-participant-seats.md` own that, both frozen here. + +## Requirement mapping + +| Requirement | Sections | +| --- | --- | +| `REQ-OFFICE-SEAT-ASSURANCE-001` | *The store-boundary guard*, *Failure and recovery*, *Security* | +| `REQ-OFFICE-SEAT-ASSURANCE-002` | *Making each window reachable*, *Where the cases live*, *Observability* | + +## The store-boundary guard + +### Placement + +The check is the **first statement** of the seat store's add-participant +entry point — `Repository.AddTaskParticipant` in +`internal/office/repository/sqlite/participants.go` — before the transaction +is begun and therefore before the +shared exclusion of `AC-OFFICE-SEAT-PROVENANCE-004.2` is acquired. That +ordering is the whole of `AC-OFFICE-SEAT-ASSURANCE-001.3`: a rejected call +that has already taken the exclusion would make a malformed argument a +source of contention for correct writers, and a rejected call that has +already opened a transaction would need a rollback to be correct rather +than being trivially correct. + +It is deliberately *not* placed in the service layer beside the existing +role lookup. The service layer is a caller; the point of the guard is that +the store stops depending on which caller is in front of it. Placing it +there would reproduce the gap one layer down. + +The existing surface check stays exactly where it is. Two layers is the +intent, not a duplication to collapse later: the surface returns a client +error to an operator, the store refuses to write a row. Neither replaces +the other, and `AC-OFFICE-SEAT-ASSURANCE-001.6` is what keeps the surface's +behaviour identical. + +### What it returns + +An exported sentinel error value — `ErrEmptyAgentProfileID`, named here so +two callers cannot end up comparing against two different identities — +returned with the zero result. Sentinel +rather than a formatted string because `AC-OFFICE-SEAT-ASSURANCE-001.2` +requires a caller to distinguish this failure from the store's *unchanged* +outcome without matching on a message, and the store's other refusals +(no step, no such task) are reported as *unchanged* with a nil error, which +is exactly the confusion the criterion forbids. + +Nothing maps it to an HTTP status. The one surface that could is already +unreachable by construction, and inventing a status for a caller that does +not exist is the speculative configuration this repository's engineering +principles reject. It falls through the existing generic error mapping, +which is correct for a caller that should not have called. + +The guard consults no store state, so `AC-OFFICE-SEAT-ASSURANCE-001.4` and +`-001.7` are structural: the decision is a comparison against the empty +string on an argument, giving the same answer on every call and on both +dialects, with no query to differ between them. + +Only the empty string. No trimming, no normalisation +(`AC-OFFICE-SEAT-ASSURANCE-001.5`). The surface's own check tests the empty +string too, and a guard that trimmed would accept a value the surface +rejects, or reject one it accepts, and the two layers would then disagree +about what they are guarding. A whitespace identifier names no agent +profile, which `AC-OFFICE-SEAT-PROVENANCE-005.8` already handles by +refusing it the claim search. + +## Making each window reachable + +One subsection per criterion that needs a mechanism. The others — +`002.1`, `002.2`, `002.8`, `002.9` — are ordinary sequential cases against +a seeded slate and need no design beyond naming the assertions, which their +criteria already do. + +### The step must be resolved on the writing handle (`002.3`) + +The store resolves the task's current step twice over in this codebase: +once on the transaction handle, inside its exclusion, and once on the +read-only pool for the listing paths. Swapping one for the other is a +one-word edit that changes nothing any current test observes, because in +every test both handles point at the same database. + +**Mechanism: give them different databases.** Build the office repository +with its writer on one store and its reader on a second, where the same +task row names a *different* current step. Register a participant, then +assert the seat landed at the step the **writer's** store names. + +This is decisive in both directions. The shipped implementation reads +through the transaction and lands at the writer's step. An implementation +that reads through the read-only pool lands at the reader's step — or, if +the reader's row is absent instead of divergent, writes nothing and reports +*unchanged*. Either way the case goes red, and it does so without any +concurrency, on the embedded dialect, in milliseconds. + +Two constraints the builder will otherwise hit. Schema initialisation runs +against the writer, so the reader store must be given its own `tasks` table +and row explicitly. And every assertion afterwards must read through the +writer handle directly, never through the repository's reader accessor, +which by construction now points at the wrong database. + +Prefer a divergent step id over an absent row: divergence proves the seat +landed at the writer's step, where absence only proves the reader was not +consulted. + +### The claim target must vanish mid-write, on purpose (`002.10`) + +This is the window `AC-OFFICE-SEAT-PROVENANCE-004.8` protects: a seat +selected as claimable, removed before the conditional write applies, and +the registration falling through to inserting its own seat rather than +completing having written nothing. The shipped comment on that path states +it "cannot be forced deterministically", and the Postgres case that stands +in for it runs fifteen concurrent iterations whose split it logs but does +not assert — so a run in which the window was never entered passes +identically to one in which it was. + +The premise that it cannot be forced is wrong, and the reason is visible in +the code. The claim search is a plain read; the claim itself is a +conditional `UPDATE` of one row by identifier. A plain read is not blocked +by a row lock, and an `UPDATE` is. So an outside session can stand exactly +between them. + +**Mechanism, on the server dialect, three connections:** + +1. A holding session opens a transaction and takes a row lock on the seat + (`SELECT ... FOR UPDATE`). Nothing else is locked. +2. The registration runs in a goroutine. Its advisory lock is uncontended. + Its identity probe misses. Its claim search reads the seat *through the + row lock* and selects it as the candidate. Its conditional write then + blocks. +3. The test waits until the registration's backend is observably waiting on + a lock, through the server's own activity view. **This wait is the + proof**: reaching it establishes the candidate was selected and the + write not yet applied, which is the window itself. It replaces the + existing case's unasserted iteration split. +4. The holding session deletes the seat and commits. +5. The blocked write re-evaluates, matches no row, and reports zero rows + affected. The registration falls through and inserts. + +Assert the outcome is *inserted*, that exactly one seat exists, that it +names the registering agent with provenance `manual`, and that its +identifier differs from the seat that was removed. An implementation that +reported a claim on a zero-row write would leave the slate empty and the +outcome *claimed*, failing on both counts. + +No production code changes for this. That is the point of choosing an +external handle over the failpoint idiom this repository also uses: the +window is entered on the shipped path, not on a path that exists only under +test. + +**If the wait probe proves unreliable** — a server build where the wait is +not visible, or a probe that flakes — fall back to the failpoint idiom +already established here for destructive cutover migrations: a hook between +the claim search and the conditional write, nil in production, that the +test uses to perform the removal. Take that only if the probe fails in +practice, and record why in the case's comment. Do not take it first. + +The existing fifteen-iteration case is not deleted. It exercises genuine +cross-connection concurrency, which the deterministic case does not; the +two answer different questions and both stay. + +### A cancellation that fails (`002.7`) + +`AC-OFFICE-SEAT-PROVENANCE-006.6`'s second sentence is the only place the +contract accepts a residual: the claim commits, the displaced agent's +queued run cannot be cancelled, the registration still succeeds, and one +run stays runnable for an agent no longer seated. Nothing tests it. + +**Mechanism: take the runs store away for the duration of the call, then +give it back.** Seed the fan-out run for the displaced agent, rename the +runs table aside, drive the registration, then restore it. Renaming rather +than dropping is what makes the fourth assertion possible — after the +restore, the run is still there and still runnable, which is the residual +the criterion describes, and a dropped table would take the evidence with +it. + +Four assertions, one per clause of the sentence: the request succeeds; the +claimed slate is exactly as a successful claim leaves it; the failure is +recorded; the displaced run is still runnable. + +The third needs a logger the test can read. The service takes one at +construction, and the logger package can be built from a zap logger, so an +observer core at warning level supplies it. The existing shared test +harness hands the service a default logger, so this needs a +logger-accepting variant of that harness rather than a change to every +caller of it. Assert on the observed entry's fields — the task, the step, +the displaced agent profile — not on its message text. + +### The guard requires exactly one decision (`002.6`) + +The end-to-end quorum case already walks the scenario the whole contract +exists for: a step entry casts a seat, an operator then registers a +different agent, and the slate must end at one seat. It asserts the guard's +role and the participant listing. It does not assert the number of +decisions the guard requires, which is the operator-visible consequence — +a regression to two seats leaves the role correct and the listing arguably +explicable, and shows up only when the gate silently never fires. + +The count is already on the wire; the case's local response type simply +does not name the field. **Mechanism: widen that type to carry the required +count and assert it is one**, alongside the existing role assertion rather +than instead of it. This is the smallest possible change and needs no new +endpoint, fixture or spec file. + +Its placement in the existing case matters and is already load-bearing +there: the registration happens *after* the step move, because seats bind +to the task's current step. That ordering is the scenario, not an accident +to tidy. + +### The two surface refusals (`002.4`, `002.5`) + +The empty-identifier refusal is reachable over HTTP: post a registration +naming an empty identifier, assert a client error and that the slate for +that task and role is still empty. + +The non-reviewer-approver refusal is **not** reachable over HTTP, and this +is worth stating rather than discovering. The routes are role-fixed: each +endpoint names its own role, and the service exposes only reviewer and +approver entry points. There is no request that carries a bad role. The +refusal that exists is a lookup miss inside the shared body both entry +points call. + +**Mechanism: a white-box case**, in the service's own package rather than +its external test package, calling that shared body with a role outside the +pair and asserting it fails and writes nothing. Go permits both packages in +one directory, so this is a new file, not a change to the existing ones. A +structural assertion over the role map instead would pass against a body +that ignored the map, so it is the weaker choice. + +## Where the cases live + +Revive's 800-effective-line limit applies to test files here, and the +guidance is explicit that new tests go in a new file rather than being +appended to a large one. Three of the four files this work extends are +already close to it — the Postgres claim file and the dashboard participant +file are within a few dozen lines of the limit, and the two repository +files have moderate room. + +So: the deterministic window case and the cancellation-failure case each +begin a new file, and the white-box role case must be a new file regardless +because of its package. Helpers already defined in the existing files of +the same package — notably the multi-connection Postgres pool builder — are +reused, not copied; that helper's own comment explains why it exists in +that package at all, and a second copy would invite the two to drift. + +Measure before appending anywhere. A case added to a file that then crosses +the limit fails lint, and moving it afterwards costs a round. + +## Failure and recovery + +The guard's failure path is the whole of its behaviour: it returns before +acquiring anything, so there is nothing to release, roll back or retry, and +`AC-OFFICE-SEAT-PROVENANCE-005.5`'s "leave the slate as you found it" is +satisfied by never having touched it. + +Everything else in this document is test-side. Its failure mode is a case +that cannot be made to pass, and `AC-OFFICE-SEAT-ASSURANCE-002.12` fixes +what that means: it is evidence this document is wrong, and routes back to +the spec step. It is not licence to weaken the case or edit the +implementation under test. A case that turns out to be flaky is likewise a +defect in the case — the mechanisms above are chosen to be deterministic +precisely so that flakiness is a signal rather than a cost of doing +business. + +The dialect-gated cases skip when the server dialect is unavailable, using +the same gate the existing Postgres cases use. +`AC-OFFICE-SEAT-ASSURANCE-002.11` is the reason that gate must remain a +visible skip: a case that silently passed without running would report +coverage this document then relies on. + +## Persistence + +Nothing here changes the schema, a migration, or a stored value. The guard +writes nothing by definition, and the cases assert against columns that +already exist — including the seat's creation time, which is a real column +on the seat table even though the office-side projection does not carry it, +so `AC-OFFICE-SEAT-ASSURANCE-002.9` must read the column directly rather +than the projected struct. + +The cancellation-failure case mutates schema *within its own test database* +by renaming a table aside and back. That is a fixture manipulation of an +isolated store, in the same family as the existing case that drops the +decision table to stand in for a storage error, and it reaches no shared +state. + +## Security + +`AC-OFFICE-SEAT-PROVENANCE-002.11` requires a claim to be subject to the +same authorization as writing a new seat, and that check runs at the +service layer, above the store. The new guard sits below it and changes no +ordering: a caller refused by the permission check never reaches the store +at all, and a caller that passes it is unaffected by a guard that only +rejects an argument no legitimate caller supplies. + +The white-box case for `002.5` calls the shared service body directly, +which is beneath the HTTP surface but *above* the permission check that +body performs, so it does not bypass authorization; it exercises the same +refusal an internal caller would meet. + +## Observability + +No new metric, log line or counter. The cancellation-failure warning that +`AC-OFFICE-SEAT-ASSURANCE-002.7` asserts already exists and is already +emitted; the case reads it rather than adding it, which is what makes the +assertion evidence about shipped behaviour. + +The guard emits nothing. A rejection is returned to a caller that should +not have called, and a log line would be written for a condition no +production path can produce. + +## Related decisions + +- `participant-seat-provenance-01.md` owns the shared exclusion, the + control flow the guard prefixes, the outcome value the store returns, + and the displaced run's cancellation. Unchanged here. +- `review-participant-seats-01.md` owns automatic casting's choice of + agent, which the concurrency cases exercise but do not constrain.