chore: record harness learnings (#3131)

apps/backend/AGENTS.md

@@ -144,7 +144,7 @@ replace state verification, installation association, or HMAC verification.
 - `streams.go` - WebSocket stream connections to agentctl
 - `process_runner.go` - agent process launch and management
 - `profile_resolver.go` - resolves agent profiles/settings
-**Lifecycle callback identity:** Process callbacks must carry the launched PID and generation; ignore delayed callbacks from replaced processes and duplicate callbacks, with tests for both old-generation and duplicate delivery.
+**Lifecycle callback identity:** Process callbacks must retain the launched PID and generation captured when scheduled, revalidate both before state/I/O, and ignore delayed callbacks from replaced processes and duplicates; test replacement before start and after waits. Prompt callbacks and asynchronous prompt errors must likewise carry immutable execution, session, prompt-generation, and turn evidence captured at the result boundary; never reread mutable execution snapshots later, because a successor prompt may already own them. Settle through the correlated terminal path and test both replacement-execution and same-execution successor-prompt races.
 
 **agentctl client** (`internal/agent/runtime/agentctl/`) is the HTTP/WS client used by the lifecycle manager to talk to a running agentctl instance. It is a runtime-tier package and should not be imported outside `internal/agent/runtime/`.
 
@@ -221,26 +221,16 @@ Client (WS) ← Orchestrator ← Lifecycle Manager ←──── stream update
 - **Cross-tier shared code belongs in `internal/common/`.** agentctl ships as a standalone binary uploaded into containers, so neither it nor the backend may own code the other imports — put shared logic in a neutral `internal/common/*` package instead of importing across the boundary or forking a copy (`ptyexec` is the worked example; `subproc`, `gitref`, `securityutil` are older ones). Duplicating to "avoid the dependency" is how the two PTY copies drifted.
 - **Startup configuration ownership:** `internal/common/config/catalog.go` and `source.go` own the stable operator catalog, discovery, precedence, provenance, and typed values; managed agentctl children receive a private subset contract. Do not reparse stable env in executors or copy YAML into public child environments.
 - **Pure computation that only matters on Windows should not carry `//go:build windows`.** One CI job runs on Windows and it tests a package allowlist, so tagged code is easily unverified — loginpty's copy of the Win32 quoting was compiled by no job at all. Keep platform API calls behind the tag, leave string/path helpers untagged, and add packages needing native coverage to the `test-windows` job in `.github/workflows/backend-tests.yml`.
-- **Event-bus wildcard parity:** New NATS wildcard subscriptions must verify equivalent `MemoryEventBus` semantics in `go test ./internal/events/bus`.
+- **Event-bus wildcard parity:** New NATS wildcard subscriptions must verify equivalent `MemoryEventBus` semantics in `go test ./internal/events/bus`. `EventBus.Publish` completion means transport/enqueue, not remote NATS subscriber processing; never use publication as a local ordering acknowledgement. If a waiter must not release until local state is armed, use a construction-supplied synchronous local notifier or durable state transition before publishing fan-out, and test with an async/NATS-like bus fake as well as `MemoryEventBus`.
 - **Repository provider identity:** Provider-backed repositories are keyed by workspace, provider, normalized `provider_host` origin, full owner/namespace, and name. Persist `provider_host` when importing or resolving a remote; do not infer self-managed GitLab rows from owner/name alone. Legacy rows with an empty host have unknown identity and must fail closed for provider write/link operations.
 - **Repository provider branches:** Native task branch pickers derive provider identity only from the persisted workspace repository. GitHub uses the first-party service; manifest-owned providers route through the owning active plugin's workspace-scoped `repositories.branches` action. Do not add provider-specific task-service branches or accept browser-supplied repository descriptors on this path.
-- **PR status sync coverage:** A `github_pr_watches` row is unique per (session, repository, branch). A watch that has **found a PR** is never re-pointed: it is that PR's only sync handle, and both the poller and the on-demand sync iterate watches, so re-pointing it froze the PR at its last-observed checks/review state — which the UI renders as an open, mergeable PR long after it merged, and which silently disabled CI auto-fix and auto-merge for every branch but the one currently checked out. Branch switches (`resetPRWatchForBranchSwitch`) and the git-status branch sync (`syncPRWatchBranch`) therefore only re-point a **searching** watch (`pr_number=0`) of the same repository, and otherwise add a watch for the new branch — one searching watch per (session, repository) plus one per PR actually opened. Watches are still not a complete index of a task's PRs (sessions get deleted, PRs get linked by URL), so the orphan sweep stays. `service_pr_unwatched.go` reconciles the `github_task_prs` rows no watch points at, and deliberately writes **lifecycle fields only** (`state` / `merged_at` / `closed_at`) — check and review aggregates belong to the watch-driven sync that actually fetches reviews and check runs, and the per-PR REST status marks its counts populated even with nothing to count, so widening this path to aggregates persists 0/0 over good data (it broke six PR-popover e2e specs exactly that way). Terminal and detached rows are excluded so the set stays bounded. **The backend is the only writer of `github_task_prs`:** the PR-feedback read makes the same upstream calls the status sync does (`GetPR` + `ListPRReviews` + `ListCheckRuns`, plus comments), so it persists through `SyncTaskPR` (`service_pr_feedback_sync.go`) and lets `github.task_pr.updated` converge every surface. The frontend must never patch PR lifecycle fields into its own store copy — that write cannot reach the DB, so it desyncs the panel from the kanban card and evaporates on reload (it also masked the sync hole above for months). Both derivations share `newPRStatus` so they cannot drift; the persist rides *inside* the TTL cache's fetch closure, so a cache hit costs no write; failures are logged and swallowed because this is a read endpoint; and the row lookup is workspace-scoped (`ListTaskPRsByPRNumber`) to match the credential scope the read was authorized under. Finally, `merged` is the one state `SyncTaskPR` refuses to leave (`resolveTerminalMergeState`) since GitHub cannot un-merge a PR — do **not** generalize that into a `merged > closed > open` rank, because `closed -> open` is a real transition (a PR can be reopened) and ranking it as a regression pins reopened PRs closed forever. **The batched branch query's empty answer is authoritative:** an alias resolving with **zero** nodes has definitively said "no open PR on this branch", so `branchBatchResult.ResolvedEmpty` carries that to `applyBatchedSearchingWatch`, which must not re-ask it through a per-watch `client.FindPRByBranch`. That re-ask was one `gh pr list` (GraphQL) plus one `gh api /repos/...` per searching watch per cycle *forever*, since a branch with no PR keeps that shape — measured at 5,200-9,200 GraphQL points/hour against a 5,000/hour limit with 42 searching watches, parking the poller on `github rate-limited` ~40 min into every reset window. On a definitive negative only the fork **parent** (a repository the batch never asked about) is worth a look, and `forkParentRepositoryForLookup` caches fork identity per (credential scope, owner, repo) since that answer is constant. Two-or-more open PRs is **not** definitive — `selectBatchedBranchPRNode` refuses to guess — so that case and an unresolved alias keep the full fork-network fallback, throttled on the watch's `last_checked_at` (`PRSyncFreshnessWindow`) exactly as `triggerPRDetection` is; the batched path is the production path, so a throttle guarding only the legacy per-watch path guards nothing. Changes here need a **call-count** test (`service_pr_watch_batched_budget_test.go`) — an assertion on the resulting `PRStatus` passes with the whole regression restored.
+- **PR status sync:** For watch identity, PR lifecycle persistence, or batched lookup changes, read [GitHub guidance](internal/github/AGENTS.md).
 - **Execution access:** Workspace-oriented handlers (files, shell, inference, ports, vscode, LSP) MUST use `GetOrEnsureExecution(ctx, sessionID)` — it recovers from backend restarts by creating executions on-demand. Only use `GetExecutionBySessionID` for operations that require a running agent process (prompt, cancel, mode).
 - **Generated-title ownership:** Claim generated-title ownership only after task, config, and Office eligibility is known. Config tasks, Office/External modes, and workspace-only preparation (`StartAgent: false`) must never claim it; only an eligible agent-start task may claim the generated title.
 - **Task lifecycle events:** Any code path that mutates a task row must publish via the event bus (`task.created` / `task.updated` / `task.deleted`) — either by going through `Service.CreateTask` / `UpdateTask` / `DeleteTask` / `ArchiveTask`, or by calling `publishTaskEvent` (or one of the `Publish*` helpers in `service_events.go`) directly. Walking `repository.TaskRepository` straight bypasses event publishing and breaks WS-driven UI like the All-Workflows kanban view. `HandoffService`'s cascade methods learned this the hard way — they now require a `TaskEventPublisher` wired via `SetTaskEventPublisher`. New cascade / bulk / cleanup paths must follow the same pattern. **Workflow steps** follow the same rule with their own publisher: step create/update/delete publish `workflow_step.created` / `.updated` / `.deleted` through `internal/workflow/stepevents.Publisher` (the WS gateway fans them out; the orchestrator re-evaluates queue admission off `.updated`). Both mutation surfaces — REST/WS in `internal/workflow/handlers`, MCP in `internal/mcp/handlers` — share that publisher and differ only in the source label they pass, so a new step-mutation path uses it rather than hand-rolling the payload; promoting a start step demotes the previous one, so also publish `.updated` for every entry in `DemotedStartSteps`.
 - **Workspace deletion side tables:** Any workspace-scoped integration side table without a database foreign key/cascade must be deleted explicitly from its `WorkspaceDeleted` handler. Cover the create → delete lifecycle in a test, and include the same cleanup in E2E reset fixtures; do not assume deleting the workspace row removes orphaned integration metadata.
-- **Testing:** Prefer `testing/synctest` (Go 1.24+) over `time.Sleep` for time-dependent tests. Use `synctest.Test` to wrap tests with tickers or timeouts — it advances fake time instantly when all goroutines are idle. When `synctest` is not feasible (e.g., tests spawning external processes like `git`), use channel-based synchronization (`<-started`, non-blocking `select`) instead of sleep-based waits. Reserve `time.Sleep` only for integration tests that need real subprocess execution time.
-  - **Test cleanup:** Register `t.Cleanup` immediately after creating resources that need teardown (adapters, `io.Pipe` writers, background goroutines) — before any `t.Fatal`/`t.Fatalf` path. Late cleanup registration leaks pipes and goroutines on early failure.
-  - **Joining production goroutines in tests:** When code spawns untracked goroutines (e.g. `fireWakeup`), don't rely on arbitrary sleeps. Join via an observable side effect — e.g. block on `EventTypeComplete` from `a.updatesCh` after unblocking the fake agent. Use short timeouts (~100ms) for in-process negative assertions; reserve multi-second waits for subprocess/integration tests only.
-  - **Path/security tests:** Avoid using the real filesystem root as a fixture root. Build fake absolute roots under `t.TempDir()` with `filepath.Join`; this keeps tests portable across Windows, POSIX, and privileged cloud executors. For error assertions, do not compare raw `%q`/escaped strings with native `filepath` values; assert typed fields or normalize both sides with `filepath.Clean`/`filepath.FromSlash`.
-  - **Filesystem permission tests:** Assert permission-denied behavior only after probing that the current executor enforces the permission bit change. Root-like Sprite executors may bypass `chmod` restrictions.
-  - **Filesystem safety checks:** Carry the original `os.Lstat` `FileInfo` (or opened handle) through every decision; do not re-stat a validated path, which reopens a TOCTOU window before side effects. Test mismatches before writes or manager commands.
-  - **Git indexed-environment tests:** When a test supplies `GIT_CONFIG_COUNT`, `GIT_CONFIG_KEY_n`, and `GIT_CONFIG_VALUE_n`, clear every inherited indexed key first and restore it with `t.Cleanup`. `internal/gitconfigenv` ignores indexes past the count; uncleared parent indexes silently alter the block instead of failing loudly.
-  - **Derived environment contracts:** Drive derived values through the production producer/wiring seam instead of manually seeding keys; pair consumer/process coverage with a producer-boundary assertion so missing publication fails.
-  - **GitLab host-trust tests:** `internal/gitlab`'s `TestMain` in `goleak_test.go` calls the scrub in `env_hermetic_test.go`; `internal/agentctl/server/process`'s `TestMain` clears ambient `KANDEV_GITLAB_HOST`/`GITLAB_HOST`/`GITLAB_TOKEN`. `test_ambient_env` runs both poisoned to catch a regression neither package's clean-env CI shard could catch.
-  - **Full test output:** For local full-suite pass/fail validation, prefer plain `go test -race ./...`. `go test -json ./...` can emit very large JSONL streams; if a wrapper or tracing tool truncates the stream mid-record, downstream JSON parsing may fail even when Go tests passed. Use JSON output mainly for CI artifacts or test-report tooling that explicitly requires it.
-  - **Private executable helpers:** When library code starts `os.Executable()` in a private helper mode, do not rely only on a command package's dispatch or package-specific `TestMain`: an importing package's test binary can be the executable and recursively run tests.
-    Use either an explicitly marked private env-and-argument trampoline before normal test entry, or inject a dedicated helper executable. Cover it from a different importing package, not only the helper package.
+- **Testing:** For backend test changes, load [backend-tests.md](../../.agents/skills/tdd/references/backend-tests.md).
+  It covers time, cleanup, filesystem fixtures, environment isolation, and subprocess helpers.
 
 ### Goroutine ownership and leak testing
 
@@ -253,7 +243,7 @@ Every long-running goroutine must have a single owner with explicit start and st
 - **Leak testing:** packages that spawn goroutines add `goleak.VerifyTestMain(m)` in a per-package `TestMain`. New packages of this kind must follow suit. When a third-party background goroutine genuinely can't be drained, suppress it with `goleak.IgnoreTopFunction(...)` and leave a comment explaining why. Currently instrumented: `internal/gateway/websocket/`, `internal/agent/runtime/lifecycle/`, `internal/agentctl/server/process/`, `internal/orchestrator/`, `internal/github/`, `internal/gitlab/`, `internal/jira/`, `internal/linear/`, `internal/integrations/healthpoll/`.
 
 ## Backups
-- On every SQLite boot, `persistence.Provide` reads `kandev_meta.kandev_version`. If the stored version differs from the binary version (or any user tables exist but no version is recorded), it takes a `VACUUM INTO` snapshot into `backups/` beside the configured SQLite database file before running migrations. The default path is `<home>/data/kandev.db`, with backups in `<home>/data/backups/`.
+- On every SQLite boot, `persistence.Provide` reads `kandev_meta.kandev_version`. If the stored version differs from the binary version (or any user tables exist but no version is recorded), it takes a `VACUUM INTO` snapshot into `backups/` beside the configured SQLite database file before running migrations. `VACUUM INTO` recreates its target, so create staged snapshots inside a same-filesystem private `0700` staging directory and chmod the file to `0600` before validation/install; removing a placeholder and chmodding afterward does not protect the creation window. The default path is `<home>/data/kandev.db`, with backups in `<home>/data/backups/`.
 - Retention: 2 backups kept (newest two by mtime); older ones are pruned after the snapshot succeeds.
 - Postgres: backup is skipped with a log line. Use `pg_dump` for Postgres backups.
 - Boot aborts if the backup fails — the pool is closed and `Provide` returns an error.
@@ -264,7 +254,7 @@ Every long-running goroutine must have a single owner with explicit start and st
 
 ## Schema & migrations (SQLite repository)
 
-`internal/task/repository/sqlite` also runs against PostgreSQL: avoid unguarded SQLite-only `rowid`, JSON, or date syntax and add an environment-gated PostgreSQL behavior test for every changed dialect-sensitive method (schema replay is insufficient).
+`internal/task/repository/sqlite` also runs against PostgreSQL: avoid unguarded SQLite-only `rowid`, JSON, or date syntax and add an environment-gated PostgreSQL behavior test for every changed dialect-sensitive method (schema replay is insufficient). For PostgreSQL read-modify-write invariants, acquire a transaction-scoped `pg_advisory_xact_lock(hashtextextended(namespace+identity, 0))` before the first read or update; SQLite's single-writer lock does not need this, and unique constraints/retries remain defense in depth. Verify with an env-gated real multi-connection test under `go test -race`.
 
 `initSchema()` in `internal/task/repository/sqlite/base_schema.go` runs the `init*Schema` (CREATE TABLE) steps **before** `runMigrations()`. The table-creation DDL uses `CREATE TABLE IF NOT EXISTS`, so on an **existing** database it is a no-op and never adds columns to a table that is already present.