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 <carlosmflorencio@gmail.com>
This commit is contained in:
Henry
@@ -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
|
||||
|
||||
@@ -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{}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
+232
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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")
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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<stri
|
||||
}
|
||||
|
||||
/**
|
||||
* Force a fresh git-status push whenever the diff panel becomes the active
|
||||
* dockview tab.
|
||||
* Force a fresh git-status push whenever the diff panel becomes visible.
|
||||
*
|
||||
* Background: the diff panel's content is derived from `gitStatus` (the
|
||||
* per-file `.diff` string), which only refreshes when a `session.git.event`
|
||||
@@ -251,28 +250,20 @@ function ChatContent({ panelId, params }: { panelId: string; params: Record<stri
|
||||
* signal, so we ask the backend for a fresh git-status snapshot via the
|
||||
* explicit `session.git.refresh` request. Focus itself remains an ACK-only
|
||||
* control signal, avoiding replay on ordinary task switching. No-op when the
|
||||
* session isn't focused.
|
||||
* session isn't focused. Visibility is used instead of active state because
|
||||
* a right-column group can remain visible while another dockview group owns
|
||||
* global focus.
|
||||
*/
|
||||
function useResyncGitStatusOnTabActivate(panelId: string, sessionId: string | null) {
|
||||
const isVisible = usePanelActive(panelId);
|
||||
|
||||
useEffect(() => {
|
||||
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
|
||||
|
||||
@@ -109,14 +109,14 @@ async function getQuorumGuards(
|
||||
apiClient: { rawRequest: (method: string, path: string) => Promise<Response> },
|
||||
workspaceId: string,
|
||||
taskId: string,
|
||||
): Promise<Array<{ role: string; satisfied: boolean; reason?: string }>> {
|
||||
): Promise<Array<{ role: string; required_count: number; satisfied: boolean; reason?: string }>> {
|
||||
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}`);
|
||||
|
||||
@@ -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`.
|
||||
@@ -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`.
|
||||
+116
@@ -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`.
|
||||
+111
@@ -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`.
|
||||
+107
@@ -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`.
|
||||
+123
@@ -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`.
|
||||
@@ -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`.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
Reference in New Issue
Block a user