From 64e1cd3054a885df6c2771aaf54b055b6144a27b Mon Sep 17 00:00:00 2001 From: seal Date: Fri, 7 Aug 2026 12:05:35 -0400 Subject: [PATCH 1/5] docs(designs): forge-poll driver design record (SEA-1810) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Design record for the compass-server forge-poll driver: a ctx-cancellable board-ingestion poll loop that drives the existing `internal/ingest` library against a real GitHub forge so tracker issues appear live in the server projection. Three tasks: - **T1** — a hand-rolled `net/http` GitHub read client (OQ-6 frozen: no new dep), with `sort=updated&since` incremental fetch, `If-None-Match`/304 conditional requests over a per-URL response+`Link`-chain cache, and an `x-ratelimit-*` budget gate that fails fast rather than sleeping. - **T2** — an `ingest.Poller` scheduler: immediate first pass then ticker, sequential per-repo, log-and-continue on error, `nil` on ctx cancel. - **T3** — serve boot wiring behind an all-optional `ForgeConfig`, the token resolved (behind a short TTL) via the DL-052 `server_only` secret path, poller run under the existing serve errgroup. Design layer only — no implementation ships here. Red-teamed by design-critic; its findings are folded (the `since`/ETag-efficacy fix, 304 page-chain caching, resolve-cost TTL, the added test cases) and the PR-C double-fetch consequence + a durable-cursor Option 1.5 are promoted into OQ-A. Freeze is blocked on **OQ-A** — the relationship to the frozen DL-053 subscription model (distinct board-poll loop vs building on the DL-053 machinery). Parked for Matt; OQ-B (live-demo content) is a non-load-bearing deferral. Ledger-impact: none in this PR — Draft record; the DECISIONS.md ledger delta lands at freeze, once Matt rules OQ-A and the record goes Active. Refs SEA-1810 Co-authored-by: Matt Wilkinson --- .../compass-forge-poll-driver/design.md | 536 ++++++++++++++++++ 1 file changed, 536 insertions(+) create mode 100644 docs/designs/product/compass-forge-poll-driver/design.md diff --git a/docs/designs/product/compass-forge-poll-driver/design.md b/docs/designs/product/compass-forge-poll-driver/design.md new file mode 100644 index 00000000..686ddc89 --- /dev/null +++ b/docs/designs/product/compass-forge-poll-driver/design.md @@ -0,0 +1,536 @@ +# Compass forge-poll driver (SEA-1810) + +Status: Draft +Lane: compass-server +Tracker: SEA + +Freeze blocked on OQ-A (see Open Questions) — Matt rules the DL-053 +relationship before this record freezes. This record designs the running +driver that periodically drives the existing greenfield `go/internal/ingest` +library against a real GitHub forge, so board issues ingested from the tracker +appear live in the Compass server projection. Design layer only — no +implementation code ships with this record. + +## Problem / Intent + +Board issues ingested from the external tracker (GitHub) must appear live in +the Compass server projection. Today nothing drives ingestion: `server/serve.go` +wires no poller and imports neither `internal/ingest` nor `internal/forge` +(verified this run — the only mentions of a poller in `go/server` are forward +references, e.g. `server/board.go:40` "the PR-B poll driver … add producers +without changing the executor signature", and the pgtest comment +`server/service_board_pgtest_test.go:14` "exactly as part 3's poller would"). +The `internal/ingest` library (`ingest.go:35-65`) is complete and tested but is +exercised only by its own tests and the board pgtest suite. This slice adds the +running driver: a ctx-cancellable background poll loop started at serve boot, +plus the first real (non-fake) forge read client behind it. + +## Global Constraints + +- **Toolchain:** module `github.com/sealedsecurity/compass/go`, go 1.25.0 + (`go/go.mod`). +- **OQ-6 (FROZEN, Matt 2026-07-27):** the GitHub client is hand-rolled over + `net/http`, **no new dependency** (~300 LOC); go-github is REJECTED — it + lacks a conditional-request + budget mechanism hook. Conditional requests + (`If-None-Match`/ETag → treat `304 Not Modified` as no-change) and honoring + `x-ratelimit-*` response headers as a budget are **mandatory**, not + optional hardening. +- **DL-052 (Active, `DECISIONS.md:75`):** only the Server holds forge + credentials, as a `server_only` declared secret filtered out of container + injection. The driver reads its GitHub token via the Server's secret path + (the `secrets.SpecResolver` instance built at `server/serve.go:287`) — + never from an agent/container, never from a flag that would land in `ps` + output. +- **Error mapping:** every non-2xx forge response maps to + `*forge.StatusError{Status, Message}` (`forge/provider.go:216-227`), so the + Service layer can flatten statuses without inspecting the wire. +- **Idempotency:** a failed or partial poll is safe to retry — "partial + progress is fine — a re-poll is idempotent on the coordinate" + (`ingest.go:48-52`; the coordinate is the store upsert key, + `board/issue_projection.go:69-71`). +- **No projection change:** `board.IssueProjection.PublishIssueUpdate(ctx, + *compassv1.Issue) error` (`board/issue_projection.go:68`) ALREADY satisfies + `ingest.issueSink` (`ingest.go:31-33`). This slice adds zero code to + `internal/board`. +- **Non-goals:** the tracker-status WRITE-back / outbound mirror + + echo-suppress (T3-b / PR-C, a separate slice); the DL-053 agent-notification + subscription delivery machinery (Sessions → `AgentGateway.Control` push). + This record is the BOARD-ingestion read poll only. + +## Approach + +Three pieces, one per task: a concrete GitHub read client in `internal/forge`, +a poll-loop scheduler in `internal/ingest`, and boot wiring + config surface in +`server/serve.go` + `cmd/compass-server/main.go`. + +### The GitHub read client (`forge.GitHub`, new file `go/internal/forge/github.go`) + +A hand-rolled `net/http` client implementing, at minimum, the read half that +satisfies `ingest.forgeReader`: + +```go +ListIssues(ctx context.Context, repo string, f forge.IssueFilter) ([]forge.Issue, error) +``` + +(`ingest.go:23-25` — the interface is unexported but structural, so any +concrete type with this method passes `ingest.NewIngester`, `ingest.go:44`.) +It lives in `internal/forge` because that package is "the ONLY place a forge +credential is used" (ownership-layer record, quoted at +`compass-server-ownership-layer/design.md:732-734`), and it is the first +increment of the full `forge.Provider` (`provider.go:195-207`) — the write +half arrives with later slices and is out of scope here; until then the type +does NOT claim to implement `Provider` (no `var _ Provider` assertion; only +`ListIssues` is built and only `forgeReader` consumes it). + +Behavior, in order of the request lifecycle: + +1. **Auth.** Every request carries `Authorization: Bearer ` where the + token comes from a `TokenSource func(ctx) (string, error)` — resolved per + request batch, not captured at construction, so a rotated `server_only` + secret takes effect without a restart (composes with DL-052; see T3 for the + resolve wiring). +2. **Incremental fetch (`since` + conditional requests).** Two composed + mechanisms keep a steady-state tick cheap. (a) The client requests + `sort=updated&direction=desc&since=`, where `` is the + greatest `updated_at` seen for that repo on the prior successful pass; + GitHub then returns only issues touched at/after it, so a quiet repo costs + ~1 request regardless of size. The watermark is in-memory (per Option 1, + OQ-A); a restart drops it and the next boot pass does one full `state=all` + resync — bounded and self-healing. (OQ-A Option 1.5 would persist it.) + (b) On top of `since`, a per-URL in-memory ETag cache (keyed by + repo+filter+page) sends `If-None-Match`; a `304 Not Modified` returns the + cached parsed `[]forge.Issue` **and the cached `Link` chain for that key** + — a 304 need not re-send `Link`, so the next-page URLs MUST be cached + alongside the ETag or a multi-page repo cannot walk past page 1 — and, per + current GitHub docs, a 304 is not charged against the core rate limit. + **That last fact is load-bearing for the budget math below and MUST be + reverified against current GitHub REST docs at T1** (the rate-limit docs + have been revised before). `since` is the PRIMARY steady-state reducer + because ETags alone self-defeat under GitHub's default `created`-desc + order: one new issue shifts every page boundary and invalidates every + page's ETag, collapsing the "free 304" path to a full paid refetch of + `ceil(N/100)` pages; `sort=updated` + `since` sidesteps that. The ETag + cache is bounded by repos × pages with a fixed size cap (pages grow with + repo size) — no eviction policy beyond the cap at this scale. +3. **Rate budget.** After every response the client records + `x-ratelimit-remaining` and `x-ratelimit-reset`. Before issuing a request, + if `remaining <= reserve` (default reserve 10), it fails fast with a typed + budget error (`forge.ErrBudgetExhausted`, exact shape in T1 Interfaces) + rather than burning the tail of the budget; the poller skips that cycle and + retries next tick (safe: re-poll is idempotent, `ingest.go:51-52`). Budget + exhaustion is an EXPECTED skip, not a failure — the poller detects + `errors.Is(err, forge.ErrBudgetExhausted)` and logs it at `slog.Warn`, NOT + the `slog.Error` path a real provider/sink error takes, so a rate-limited + window is not per-tick Error noise. A `403`/`429` carrying `retry-after` or + a zeroed `x-ratelimit-remaining` is treated the same way. The client never + sleeps holding the caller's poll slot — backoff is the poller's ticker, not + an in-client `time.Sleep`. (Under sustained pressure a skipped tick can + defer ingestion up to a full reset window (~60 min); acceptable given + idempotency, and `since` keeps steady-state spend far under the budget.) +4. **Pagination.** GitHub's list-issues endpoint pages; the client requests + `per_page=100` and follows RFC-5988 `Link: rel="next"` headers, + concatenating pages into one `[]forge.Issue` (the `Ingester` contract is + "fetches every issue for repo", `ingest.go:48`). +5. **Filter mapping.** `forge.IssueFilter{State, Labels}` + (`provider.go:182-188`) maps to `state=`/`labels=` query params. An empty + `State` — which is what `Ingester.Ingest` always sends + (`forge.IssueFilter{}`, `ingest.go:54`) — maps to **`state=all`**, this + provider's documented default (`provider.go:183-184` delegates the empty + case to "the provider's default"). Rationale: the board needs closed + tracker issues to flow so Done/reopen transitions ingest (DL-129: "a user + reopen un-archives via ingestion", `DECISIONS.md:173`); an open-only + default would silently freeze closed issues at their last-seen state. +6. **PR exclusion.** GitHub's issues API returns pull requests as issues + (rows carrying a `pull_request` key); the client drops them — the board's + PR surface is a separate lane, and `forge.Issue` (`provider.go:37-53`) is + issue-shaped only. +7. **Error mapping.** Any non-2xx/non-304 response becomes + `&forge.StatusError{Status: resp.StatusCode, Message: }` (`provider.go:216-227`), wrapped so `errors.As` finds it. Bodies + are returned RAW — the owner-header strip belongs to ingestion + (`provider.go:42-44`, `ingest.go:67-74`), never the provider. + +### The poll-loop scheduler (`ingest.Poller`, new file `go/internal/ingest/poller.go`) + +A small scheduler in the `ingest` package (it composes the package's own +`Ingester`; a new package would be a second convention for no gain): + +- `Run(ctx)` performs one immediate pass at start (so a freshly booted demo + server shows the board without waiting a full interval), then ticks on + `time.Ticker(interval)`. +- Each pass iterates the configured repos and calls + `Ingester.Ingest(ctx, repo)` (`ingest.go:53`) sequentially — one forge, one + budget; parallel per-repo fetches would just race the same rate limit. +- **A failed poll is logged, never fatal.** `Ingest` stops at the first + provider/sink error (`ingest.go:50-51`); the poller records it at + `slog.Error` and moves to the next repo / next tick — the re-poll is + idempotent on the coordinate (`ingest.go:51-52`). The poller returns a + non-nil error only never — its sole exit is ctx cancellation, returning + `nil` so the errgroup drain (see wiring) treats shutdown as clean. +- **Graceful shutdown:** `<-ctx.Done()` between passes and via the ctx + threaded into `Ingest` mid-pass (both `ListIssues` and + `PublishIssueUpdate` take ctx, `ingest.go:54,60`). +- **Observability:** one `slog.Info` per completed repo pass with fields + `repo`, `issues` (count sunk), `not_modified` (bool: whole fetch served + from cache), `dur`, `ratelimit_remaining`; one `slog.Warn` per + budget-skipped pass (`errors.Is(err, forge.ErrBudgetExhausted)` — an + expected condition, never `Error`); one `slog.Error` only per genuinely + failed pass (provider/sink error) with `repo`, `err`. Boot logs one + `slog.Info` with `repos`, `interval`. No per-issue logging — a 500-issue + repo must not emit 500 lines per minute. + +### Boot wiring + config (`server/serve.go`, `cmd/compass-server/main.go`) + +Mirrors the two established precedents exactly: + +- **Config surface:** `server.ServeConfig` (`serve.go:56-89`) gains an + optional `Forge ForgeConfig` field, all-optional exactly like `S3` + (`serve.go:71-75`): absent repos → no poller, today's behavior, zero new + requirements on existing deployments. The CLI (`main.go`) maps flags with + env fallback mirroring the `COMPASS_S3_*` precedence (`main.go:127-138`): + `--forge-repos` / `$COMPASS_FORGE_REPOS` (comma-separated `owner/name`), + `--forge-poll-interval` / `$COMPASS_FORGE_POLL_INTERVAL` (default `1m`), + `--forge-secret` / `$COMPASS_FORGE_SECRET` (the declared secret NAME, + default `GITHUB_FORGE_TOKEN`; the VALUE never crosses a flag), plus + `--forge-host` / `$COMPASS_FORGE_HOST` (default `github.com`, for GHES + later; the API base URL derives from it). +- **Secret resolve:** the poller's `TokenSource` closes over the one + `secrets.SpecResolver` built at `serve.go:287`, calls + `Resolve(ctx, "forge poll")` (`secrets/resolver.go:139`), and selects the + configured name from the returned `[]ResolvedSecret` — the same + single-resolve-surface DL-052 mandates; the row is declared `server_only` + so it never reaches a container (mechanism per the ownership-layer T5, + `compass-server-ownership-layer/design.md:1991-1995`). **Resolve is not + cheap** — `resolver.go:139-176` reads the entire declared-secret registry + from the store, writes a manifest temp file, and drives a full secretspec + provider `Load` (potentially an external provider call) per invocation — so + the `TokenSource` caches the resolved value behind a short TTL (default + `5m`) and re-resolves on expiry OR on a `401`/`403` auth response, NOT once + per poll pass; a token that rotates rarely does not tax the store and + secrets provider every minute, while a rotation still takes effect within + the TTL (or immediately on the next auth failure). A missing/undeclared + secret with forge config present is a startup error — fail fast, but at + `serve.go:287` the resolver is built AFTER the UDS listener binds, so the + validation follows the listener-cleanup path the `Rehydrate` failure + already uses (`serve.go:260-263`), returning the error from `Serve` rather + than a bare `main.go` exit. A resolve that breaks at RUNTIME (provider + outage, a declaration deleted post-boot) surfaces per-pass as an auth error + → `slog.Error` + retry next tick (idempotent); to keep a permanently broken + secret from degrading to silent forever-Error, the poller emits a distinct + `slog.Error` "forge token unresolvable" the first time a resolve fails and + on recovery — a minimal health surface without per-tick spam. +- **Lifecycle:** the poller runs as one more `g.Go` member of the existing + serve errgroup (`serve.go:327-339`) — `g.Go(func() error { return + poller.Run(gctx) })` — so it inherits exactly the scoped lifecycle the + doors have: cancelled on SIGINT/SIGTERM via the `signal.NotifyContext` ctx + (`main.go:146-162`), first-error-wins, drained with everything else. No + bespoke goroutine + done-channel machinery. +- **Ingester assembly:** `ingest.NewIngester(gh, issueBrd, + &compassv1.ForgeRef{Provider: compassv1.ForgeProvider_FORGE_PROVIDER_GITHUB, + Host: cfg.Forge.Host})` (`ingest.go:44`; enum at + `gen/compass/v1/compass.pb.go:449`), where `issueBrd` is the existing + projection instance built at `serve.go:259` — shared writer/reader exactly + as the board pgtest drives it (`service_board_pgtest_test.go:13-15`). + +## Alternatives considered + +- **(a) go-github library — REJECTED (OQ-6, frozen Matt 2026-07-27).** It + would add a dependency for what is ~300 LOC of `net/http`, and its + high-level API lacks the hook this driver's core mechanism needs: + first-class conditional-request (`If-None-Match`/304) handling fused with an + `x-ratelimit-*` budget gate ahead of each call. Hand-rolling makes the + budget/ETag path the PRIMARY path, not a retrofit around a library's + transport. Not re-opened here; recorded as a Global Constraint. +- **(b) Fold this poller into the DL-053 subscription poller — the + load-bearing fork, PARKED as OQ-A.** DL-053 (`DECISIONS.md:76`) freezes a + Postgres-backed FETCH/DELIVERY-cursor conditional-poll subscription model + for agent notification delivery, and DL-129 (`DECISIONS.md:173`) names "the + DL-053 poll" as the path tracker status ingests through. This record + designs a DISTINCT, simpler board-ingestion loop (Option 1). OQ-A now + carries THREE readings — Option 1 (distinct, in-memory cursor), Option 1.5 + (distinct, durable Postgres fetch-cursor from day one), Option 2 (build on + DL-053 machinery) — because tracker status rides the SAME `/issues` rows + this loop fetches, so PR-C on a separate poll double-fetches one endpoint. + See OQ-A for the three options, the double-fetch consequence, and the + recommendation. +- **(c) Webhooks instead of polling — deferred by DL-053 itself** + ("change-detected by conditional polling in v1 (webhooks are an additive + accelerator)", `DECISIONS.md:76`). Same posture applies here: a webhook + receiver is an additive accelerator that sinks through the same + `Ingester`, never a replacement for the poll. +- **(d) Where the loop lives: a bespoke goroutine in `cmd/compass-server` + vs. the serve errgroup — chose the errgroup.** `main.go` owns flags and + signal wiring only (`main.go:41-173`); everything with a lifecycle runs + under `Serve`'s scoped group (`serve.go:321-339`). A cmd-level goroutine + would duplicate drain/first-error semantics the group already audits. +- **(e) Per-repo goroutines in the poller — rejected.** One forge, one rate + budget; sequential per-repo fetches serialize naturally against + `x-ratelimit-*` and keep the poller a trivially testable single loop. +- **(f) `since`-watermark incremental sync vs page ETags alone — chose + both, `since` primary.** GitHub's default `created`-desc list order means + one new issue shifts every page boundary and invalidates every page ETag, + so ETags alone collapse to a full paid refetch on any churn. + `sort=updated&since=` returns only changed rows and reduces a + quiet tick to ~1 request independent of repo size; page ETags then ride on + top for the no-change case. `since` is folded into the request lifecycle + (Approach step 2), not rejected — the two compose. + +## Plan + +### T1 — `forge.GitHub`: the hand-rolled net/http GitHub read client + +New file `go/internal/forge/github.go` (+ `github_test.go`). Implements the +request lifecycle from the Approach: bearer auth via `TokenSource`, +`sort=updated&since=` incremental fetch, ETag conditional requests +with a per-URL response+`Link`-chain cache, `x-ratelimit-*` budget gate with a +typed budget error, `Link`-header pagination at `per_page=100`, `IssueFilter` +→ query-param mapping (empty `State` → `state=all`), `pull_request`-row +exclusion, non-2xx → `*forge.StatusError`. Raw bodies pass through untouched +(`provider.go:42-44`). No `Provider` interface assertion — read half only. + +Interfaces: + +```go +// TokenSource yields the current forge token; called per fetch pass so a +// rotated server_only secret takes effect without restart. +type TokenSource func(ctx context.Context) (string, error) + +// GitHubConfig configures a GitHub read client. +type GitHubConfig struct { + Host string // "github.com" or a GHES host; API base derives from it + Token TokenSource // required + Client *http.Client // nil -> a default client with a sane timeout +} + +func NewGitHub(cfg GitHubConfig) *GitHub + +// Satisfies ingest.forgeReader (ingest.go:23-25) structurally. +func (g *GitHub) ListIssues(ctx context.Context, repo string, f IssueFilter) ([]Issue, error) + +// ErrBudgetExhausted is returned (wrapped) when x-ratelimit-remaining is at or +// under the reserve; the caller skips the cycle and retries next tick. +var ErrBudgetExhausted = errors.New("forge: rate budget exhausted") +``` + +Consumes: `forge.IssueFilter` (`provider.go:182-188`), `forge.Issue` +(`provider.go:37-53`), `forge.StatusError` (`provider.go:216-227`), +`net/http`, `encoding/json` (stdlib only — OQ-6). + +Test cycle (unit, stubbed `http.RoundTripper`; red-first per repo convention): + +1. 200 with issues JSON → parsed `[]forge.Issue` field-by-field (incl. raw + body untouched, labels, state, ForgeAccount). +2. Second call sends `If-None-Match` with the first response's ETag; a + scripted 304 returns the cached slice, zero re-parse. +3. `x-ratelimit-remaining: 0` on response N → call N+1 fails + `errors.Is(..., ErrBudgetExhausted)` WITHOUT issuing a request (assert via + RoundTripper call count); after scripted reset, calls resume. +4. Two-page `Link` pagination concatenates; `per_page=100` asserted on the + request. +5. Empty `IssueFilter.State` → `state=all` on the wire; labels join correctly. +6. A `pull_request`-keyed row is dropped. +7. 403/404/500 → `errors.As(..., *forge.StatusError)` with matching Status and + the GitHub `message` field. +8. Token from `TokenSource` lands as `Authorization: Bearer …`; a TokenSource + error propagates without a request. +9. Multi-page 304: a two-page list is fetched, then a scripted 304 on page 1 + returns the cached slice AND walks the cached `Link` chain to page 2's + cached slice — a repo of >100 issues is served entirely from cache on an + unchanged pass (the page-chain-caching correctness hole). +10. `403`/`429` with `retry-after` (or a zeroed `x-ratelimit-remaining` on the + prior response) → next call fails `errors.Is(..., ErrBudgetExhausted)` + without issuing a request (RoundTripper call count asserted). +11. Absent/malformed `x-ratelimit-*` headers do not wedge the gate — the + client proceeds (treats unknown budget as available) rather than + permanently skipping. +12. ctx cancellation mid-pagination (cancel after page 1) stops promptly and + returns `ctx.Err()`, not a partial concatenation. + +### T2 — `ingest.Poller`: the ctx-cancellable poll-loop scheduler + +New file `go/internal/ingest/poller.go` (+ `poller_test.go`). Immediate first +pass, then ticker; sequential repos; log-and-continue on per-repo error; +`nil` return on ctx cancellation; slog fields per the Approach. + +Interfaces: + +```go +// PollerConfig configures the board-ingestion poll loop. +type PollerConfig struct { + Repos []string // "owner/name", non-empty (validated by the caller) + Interval time.Duration // > 0; the caller defaults it + Log *slog.Logger // nil -> slog.Default() +} + +func NewPoller(ing *Ingester, cfg PollerConfig) *Poller + +// Run polls until ctx is cancelled, then returns nil (clean shutdown). +// Per-repo errors are logged and retried next tick, never returned — a +// re-poll is idempotent on the coordinate (ingest.go:48-52). +func (p *Poller) Run(ctx context.Context) error +``` + +Consumes: `Ingester.Ingest(ctx, repo) error` (`ingest.go:53`), `time.Ticker`, +`log/slog`. + +Test cycle (unit, fake `Ingester` deps — the package's existing +`recordingSink` pattern, `ingest_test.go:15-25`, plus `forge.FakeProvider`): + +1. `Run` performs an immediate pass for every repo before the first tick + (short interval, assert call order/coverage). +2. ctx cancel BETWEEN ticks → `Run` returns `nil` promptly, and ctx cancel + DURING a pass (mid-`Ingest`, before all repos are done) also returns `nil` + promptly — deadline-gated, never a sleep (the pgtest convention, + `service_board_pgtest_test.go:15`). Assert `Run` does NOT return while ctx + is live (it has no non-cancel exit). +3. A scripted `Ingest` failure on repo A does not stop repo B in the same + pass, and repo A is retried on the next tick. +4. Interval respected: N ticks in a bounded window drive exactly N+1 passes + (the +1 is the immediate pass); no busy-loop. +5. Log fields: capture a `slog` test handler and assert a happy pass logs at + Info with `repo`/`issues`/`dur`, a budget-skip logs at Warn, and a genuine + `Ingest` error logs at Error with `err`. + +### T3 — serve boot wiring + config surface + secret resolve + +Touches `server/serve.go`, `cmd/compass-server/main.go` (edits only, no new +files beyond tests). `ServeConfig.Forge` all-optional like `S3`; CLI +flags/env per the Approach; fail-fast on forge-config-present but +secret-unresolvable; poller under the existing errgroup. + +Interfaces: + +```go +// server.ForgeConfig configures the board-ingestion poll driver (SEA-1810). +// All-optional: empty Repos leaves the driver off (today's behavior). +type ForgeConfig struct { + Host string // default "github.com" + Repos []string // "owner/name"; empty -> no poller + SecretName string // declared server_only secret name; default "GITHUB_FORGE_TOKEN" + PollInterval time.Duration // default time.Minute +} +// ServeConfig gains: Forge ForgeConfig +``` + +Consumes: `secrets.SpecResolver.Resolve(ctx, reason) ([]ResolvedSecret, error)` +(`secrets/resolver.go:139`), `secrets.ResolvedSecret{Name, Value, …}` +(`secrets/secrets.go:128-144`; Value redacted under all fmt verbs — safe to +thread), `board.NewIssueProjection` instance from `serve.go:259`, +`ingest.NewIngester` (`ingest.go:44`), `forge.NewGitHub` (T1), +`ingest.NewPoller`/`Run` (T2), the errgroup at `serve.go:327`, +`compassv1.ForgeProvider_FORGE_PROVIDER_GITHUB` +(`gen/compass/v1/compass.pb.go:449`). + +Produces: a `TokenSource` closure over the resolver that re-resolves + +selects `SecretName` on TTL expiry or auth failure (not per pass; see the +Secret-resolve bullet); boot-time validation error text for missing-secret / +bad-repo-format / non-positive interval. + +Test cycle: + +1. Unit: CLI flag/env precedence for the four forge knobs mirrors the S3 + pattern (table test like `resolveNetworkDoor`'s, `main.go:184-212`); + repo-format validation (`owner/name`) rejects garbage at startup. +2. Unit: empty `Forge.Repos` → `Serve` builds no poller (assert via config + plumbing seam, no behavioral change to existing serve tests — the whole + existing suite is the regression net). +3. pgtest (build-tag gated like the board suite): a `Serve`-assembled + Ingester over `forge.FakeProvider` scripted with one issue sinks to the + real projection and lands in `ListBoardIssues` — the end-to-end + boot-wiring proof without touching live GitHub. (The fake satisfies the + same structural `forgeReader`, `ingest.go:20-22`.) +4. pgtest: a CLOSED-state issue from the fake sinks with `ForgeState` set and + the row's `State` left untouched — the no-clobber contract this driver + leans on (`issue_projection.go:207`, `protoToForgeFields` writes + `ForgeState` but never `State`), so board ingestion cannot overwrite a + human-set board column. +5. Unit: the `TokenSource` re-resolves after its TTL and on a scripted auth + failure, and a changed resolver value is used on the next fetch — the + design's stated reason for a resolver closure rather than a captured token. +6. Startup failure: forge config present + secret name absent from the + resolved set → `Serve` returns a descriptive error, following the + listener-cleanup path the `Rehydrate` failure uses (`serve.go:260-263`), + since the resolver is built after the UDS listener binds (`serve.go:287`). + +## Tasks + +- [ ] **T1** — `forge.GitHub` read client (`github.go` + unit tests over a + stubbed RoundTripper: 200/parse, ETag/304, budget gate, pagination, + filter mapping incl. `state=all` default, PR exclusion, StatusError + mapping, bearer auth). +- [ ] **T2** — `ingest.Poller` scheduler (`poller.go` + unit tests: immediate + pass, ctx cancel → nil, per-repo error isolation + retry, interval + discipline, slog fields). +- [ ] **T3** — serve boot wiring: `ServeConfig.Forge`, CLI flags/env, DL-052 + secret resolve via the existing `SpecResolver`, errgroup membership, + fail-fast validation; unit + pgtest end-to-end over `FakeProvider`. + +## Open Questions + +### OQ-A (LOAD-BEARING — blocks the freeze): relationship to DL-053 + +The SEA-1810 issue body scopes this slice as the *simple board-ingestion +poll* (ticker → `Ingester.Ingest` → `IssueProjection` sink), explicitly +distinct from the DL-053 agent-notification subscription poller. But DL-053 +(`DECISIONS.md:76`) and DL-129 (`DECISIONS.md:173`) freeze a Postgres-backed +FETCH/DELIVERY-cursor conditional-poll subscription model as THE way tracker +status is ingested ("through the reverse `TrackerStatusMapping` (DL-053 +poll; echo-suppressed…)"). This slice does NOT ingest tracker STATUS today — +`protoToForgeFields` (`issue_projection.go:208-219`) writes `ForgeState` but +never `State`, and the `:207` guarantee ("the forge-only upsert cannot +clobber a human-set state") means board ingestion and status ingestion are +cleanly separable RIGHT NOW. So Option 1 is not a contradiction of DL-129 +today. The fork is about what happens when PR-C (tracker-status ingestion, +DL-129) lands: tracker status is a field on the SAME `/issues` rows this loop +already fetches (`forge.Issue.State`, `provider.go:46-47`), so PR-C on a +separate DL-053 poll would **re-fetch the identical endpoint** — double +rate-budget spend against one shared limit, the exact waste alternative (e) +rejects — UNLESS DL-129/DL-053 is amended so both ingestions share this +loop's fetch. That collision is the sharp edge Matt should rule on. Three +readings: + +- **Option 1 (this record's stated assumption — recommended):** the + board-ingestion poll is a DISTINCT, simpler loop that sinks to the + projection; it is NOT the DL-053 per-subscriber delivery machinery. + Per-subscriber DELIVERY cursors and the `Sessions` → `AgentGateway.Control` + push path are pure overhead for a projection writer with exactly one, + always-on consumer. **Unpriced cost, now stated:** the convergence with + DL-053's fetch half is asserted, not designed — and it is not a free seam + swap. DL-053's FETCH cursor is per-ARTIFACT; this client's cache is a + per-LIST-PAGE ETag. Different granularities: a list-page ETag cannot become + a per-artifact cursor by moving it behind a store seam, so a real merge is + a rewrite of the fetch layer, not a relocation. Plus the PR-C double-fetch + above is left for a future decision. +- **Option 1.5 (distinct loop, durable cursor from day one — the middle + path):** keep Option 1's distinct, simple loop and its projection sink, but + persist the fetch watermark (the `since` timestamp / page ETags) in a small + Postgres table instead of T1's in-memory map. Cost: one migration + a tiny + store method (T1 gains a cursor-store dependency; T2/T3 unchanged). Buys: + no full `state=all` resync on every restart, survives a future multi-replica + server, and makes the "shared fetch primitive" real rather than aspirational + — a durable per-repo fetch cursor IS DL-053's FETCH-cursor idea in embryo, + so a later convergence is an extension, not a rewrite. Still does NOT take + on per-subscriber delivery. Recommended IF Matt wants the convergence story + to be load-bearing; declined if in-memory-and-resync is acceptable for v1. +- **Option 2:** build this driver ON the DL-053 subscription machinery from + day one (the board as a distinguished subscriber), so tracker-status + ingestion (PR-C, per DL-129) lands on one poll path and no convergence + refactor is ever needed, AND the double-fetch never arises. Cost: the + DL-053 Postgres rows/cursors/delivery plumbing does not exist yet — this + slice would inherit that entire build as a prerequisite, and SEA-1810's + demo-facing scope (issues visibly on the board) would gate on it. + +**Recommendation: Option 1** for fastest demo-facing value, with **Option 1.5 +as the hedge** if the convergence with PR-C/DL-053 must be designed now rather +than deferred. If Matt rules Option 2, T2/T3 are re-cut (T1 survives nearly +intact — the client is the shared fetch half either way). The PR-C +double-fetch is the concrete thing each option resolves differently. PARKED +for Matt; the freeze waits on this ruling. + +### OQ-B (non-load-bearing deferral): live-demo content + +Whether the dogfood demo shows LIVE GitHub issues — which would make this +driver demo-critical and re-sequence its priority — is a later Matt scope +call per the SEA-1810 issue body's own escalation trip-wire. Recorded as an +explicitly non-load-bearing deferral: the design is correct regardless of the +ruling; only scheduling changes. From ce0b76dd8908c49c9672246e905af86077bf81b6 Mon Sep 17 00:00:00 2001 From: seal Date: Fri, 7 Aug 2026 12:32:44 -0400 Subject: [PATCH 2/5] =?UTF-8?q?docs(designs):=20fold=20forge-poll=20review?= =?UTF-8?q?=20findings=20=E2=80=94=20restore=20idempotency=20invariant,=20?= =?UTF-8?q?403=20discriminator,=20no-clobber=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review (skill://review, 1 round, 1 high + 2 medium + 2 low) folded: - HIGH: the since-watermark added in the design-critic pass advanced on fetch success, but Ingester.Ingest sinks-then-stops (ingest.go:60-62) and forge.Issue carries no timestamp (provider.go:37-53), so an in-client watermark could not gate on sink outcome — a mid-pass sink failure stranded every issue below it, voiding the re-poll idempotency invariant. v1 now uses ETag/304 only; a correct durable incremental cursor is homed in OQ-A Option 1.5. Idempotency stated as a load-bearing Global Constraint. - MEDIUM: single-rule 403 disambiguation (rate-limit vs bad-creds vs permission) stated explicitly in Approach step 3, reconciled with step 7. - MEDIUM: T3 no-clobber pgtest respecified to set a human State baseline via store.SetIssueState first, so it can go red (was vacuous over a zero row). - LOW: trimmed the DL-129 reopen over-claim; fixed garbled poller prose. Co-authored-by: Matt Wilkinson --- .../compass-forge-poll-driver/design.md | 172 ++++++++++++------ 1 file changed, 115 insertions(+), 57 deletions(-) diff --git a/docs/designs/product/compass-forge-poll-driver/design.md b/docs/designs/product/compass-forge-poll-driver/design.md index 686ddc89..42fbf660 100644 --- a/docs/designs/product/compass-forge-poll-driver/design.md +++ b/docs/designs/product/compass-forge-poll-driver/design.md @@ -44,10 +44,23 @@ plus the first real (non-fake) forge read client behind it. - **Error mapping:** every non-2xx forge response maps to `*forge.StatusError{Status, Message}` (`forge/provider.go:216-227`), so the Service layer can flatten statuses without inspecting the wire. -- **Idempotency:** a failed or partial poll is safe to retry — "partial - progress is fine — a re-poll is idempotent on the coordinate" +- **Idempotency (load-bearing):** a failed or partial poll is safe to retry — + "partial progress is fine — a re-poll is idempotent on the coordinate" (`ingest.go:48-52`; the coordinate is the store upsert key, - `board/issue_projection.go:69-71`). + `board/issue_projection.go:69-71`). This holds because `Ingester.Ingest` + stops at the first sink error (`ingest.go:60-62`) and the next pass + RE-FETCHES the full issue set, so a fetched-but-not-yet-sunk issue is retried + next tick. **The invariant therefore forbids any fetch-side optimization that + skips re-fetching an un-sunk issue** — a per-repo `since`/`updated` watermark + advanced on fetch success (not full-pass success) would strand every issue + below it whenever a mid-pass sink error fires, permanently absent from the + projection until re-touched upstream. An in-client watermark cannot see the + sink outcome (the client's sole seam is `ListIssues([]Issue, error)` and + `forge.Issue` carries no timestamp, `provider.go:37-53`), so v1 does NOT do + `since` incrementalism; steady-state cost is bounded by ETag/304 (Approach + step 2) instead, and a correct incremental cursor is homed in OQ-A Option 1.5 + where a durable, poller-owned watermark can gate its advance on full-pass + success. - **No projection change:** `board.IssueProjection.PublishIssueUpdate(ctx, *compassv1.Issue) error` (`board/issue_projection.go:68`) ALREADY satisfies `ingest.issueSink` (`ingest.go:31-33`). This slice adds zero code to @@ -89,29 +102,33 @@ Behavior, in order of the request lifecycle: request batch, not captured at construction, so a rotated `server_only` secret takes effect without a restart (composes with DL-052; see T3 for the resolve wiring). -2. **Incremental fetch (`since` + conditional requests).** Two composed - mechanisms keep a steady-state tick cheap. (a) The client requests - `sort=updated&direction=desc&since=`, where `` is the - greatest `updated_at` seen for that repo on the prior successful pass; - GitHub then returns only issues touched at/after it, so a quiet repo costs - ~1 request regardless of size. The watermark is in-memory (per Option 1, - OQ-A); a restart drops it and the next boot pass does one full `state=all` - resync — bounded and self-healing. (OQ-A Option 1.5 would persist it.) - (b) On top of `since`, a per-URL in-memory ETag cache (keyed by - repo+filter+page) sends `If-None-Match`; a `304 Not Modified` returns the - cached parsed `[]forge.Issue` **and the cached `Link` chain for that key** - — a 304 need not re-send `Link`, so the next-page URLs MUST be cached - alongside the ETag or a multi-page repo cannot walk past page 1 — and, per - current GitHub docs, a 304 is not charged against the core rate limit. - **That last fact is load-bearing for the budget math below and MUST be - reverified against current GitHub REST docs at T1** (the rate-limit docs - have been revised before). `since` is the PRIMARY steady-state reducer - because ETags alone self-defeat under GitHub's default `created`-desc - order: one new issue shifts every page boundary and invalidates every - page's ETag, collapsing the "free 304" path to a full paid refetch of - `ceil(N/100)` pages; `sort=updated` + `since` sidesteps that. The ETag - cache is bounded by repos × pages with a fixed size cap (pages grow with - repo size) — no eviction policy beyond the cap at this scale. +2. **Conditional requests (ETag/304) — the steady-state reducer.** A per-URL + in-memory ETag cache (keyed by repo+filter+page) sends `If-None-Match`; a + `304 Not Modified` returns the cached parsed `[]forge.Issue` **and the + cached `Link` chain for that key** — a 304 need not re-send `Link`, so the + next-page URLs MUST be cached alongside the ETag or a multi-page repo cannot + walk past page 1 — and, per current GitHub docs, a 304 is not charged + against the core rate limit. **That last fact is load-bearing for the budget + math below and MUST be reverified against current GitHub REST docs at T1** + (the rate-limit docs have been revised before). On a no-change tick every + page serves from cache: a quiet repo costs zero rate budget and the full + cached slice is re-sunk (idempotent on the coordinate). The ETag cache is + bounded by repos × pages with a fixed size cap (pages grow with repo size) + — no eviction policy beyond the cap at this scale. + + A `since`/`sort=updated` incremental fetch — which would also shrink a + CHANGED tick, not just a no-change one — is deliberately NOT in v1. An + in-client watermark advanced on fetch success breaks the re-poll idempotency + invariant above (a mid-pass sink failure strands the issues below it), and + the client structurally cannot observe sink outcome to advance it correctly + (Global Constraints → Idempotency). So v1 relies on ETag/304 alone, which is + idempotency-safe: a 304 means nothing changed and nothing to sink, and a + changed page returns 200 and re-fetches that page's full set. The cost this + accepts: a repo with any new issue pays a full paid refetch of its changed + pages that tick (GitHub's default `created`-desc order shifts page + boundaries and invalidates page ETags on insert) — bounded, budget-gated + (step 3), and re-priced as the concrete tradeoff of deferring `since` to a + durable cursor (OQ-A Option 1.5, Alternatives (f)). 3. **Rate budget.** After every response the client records `x-ratelimit-remaining` and `x-ratelimit-reset`. Before issuing a request, if `remaining <= reserve` (default reserve 10), it fails fast with a typed @@ -121,12 +138,23 @@ Behavior, in order of the request lifecycle: exhaustion is an EXPECTED skip, not a failure — the poller detects `errors.Is(err, forge.ErrBudgetExhausted)` and logs it at `slog.Warn`, NOT the `slog.Error` path a real provider/sink error takes, so a rate-limited - window is not per-tick Error noise. A `403`/`429` carrying `retry-after` or - a zeroed `x-ratelimit-remaining` is treated the same way. The client never - sleeps holding the caller's poll slot — backoff is the poller's ticker, not - an in-client `time.Sleep`. (Under sustained pressure a skipped tick can - defer ingestion up to a full reset window (~60 min); acceptable given - idempotency, and `since` keeps steady-state spend far under the budget.) + window is not per-tick Error noise. **403 disambiguation (a single rule — + GitHub overloads 403 across rate-limit, bad-credentials, and + repo-permission causes):** a `403`/`429` carrying `retry-after` OR a zeroed + `x-ratelimit-remaining` → `forge.ErrBudgetExhausted` (Warn, budget-skip, NO + token re-resolve); a `401`, or a `403` whose body `message` is a + bad-credentials/permission error with NO rate-limit headers → + `*forge.StatusError` AND signals the caller to re-resolve the token (the T3 + secret-resolve bullet); any other non-2xx → `*forge.StatusError` only + (step 7). The discriminator is the presence of `retry-after` / a zeroed + `x-ratelimit-remaining`, nothing else — so a dead token is never mistaken + for a rate-limit skip (which would retry forever with the dead token, + never re-resolving) and a rate-limit 403 never becomes per-tick Error spam. + The client never sleeps holding the caller's poll slot — backoff is the + poller's ticker, not an in-client `time.Sleep`. (Under sustained pressure a + skipped tick can defer ingestion up to a full reset window (~60 min); + acceptable given idempotency, and ETag/304 keeps steady-state spend near + zero.) 4. **Pagination.** GitHub's list-issues endpoint pages; the client requests `per_page=100` and follows RFC-5988 `Link: rel="next"` headers, concatenating pages into one `[]forge.Issue` (the `Ingester` contract is @@ -137,14 +165,18 @@ Behavior, in order of the request lifecycle: (`forge.IssueFilter{}`, `ingest.go:54`) — maps to **`state=all`**, this provider's documented default (`provider.go:183-184` delegates the empty case to "the provider's default"). Rationale: the board needs closed - tracker issues to flow so Done/reopen transitions ingest (DL-129: "a user - reopen un-archives via ingestion", `DECISIONS.md:173`); an open-only - default would silently freeze closed issues at their last-seen state. + tracker issues to flow so their `ForgeState` stays fresh on the row; an + open-only default would silently freeze closed issues at their last-seen + forge state. (The reopen→un-archive LIFECYCLE transition rides + tracker-STATUS ingestion — PR-C / OQ-A, DL-129 — not this slice, which + writes `ForgeState` but never `State`; `state=all` here only keeps + `ForgeState` current on closed rows.) 6. **PR exclusion.** GitHub's issues API returns pull requests as issues (rows carrying a `pull_request` key); the client drops them — the board's PR surface is a separate lane, and `forge.Issue` (`provider.go:37-53`) is issue-shaped only. -7. **Error mapping.** Any non-2xx/non-304 response becomes +7. **Error mapping.** Any non-2xx/non-304 response that step 3's 403 + disambiguation does NOT classify as a budget skip becomes `&forge.StatusError{Status: resp.StatusCode, Message: }` (`provider.go:216-227`), wrapped so `errors.As` finds it. Bodies are returned RAW — the owner-header strip belongs to ingestion @@ -164,9 +196,9 @@ A small scheduler in the `ingest` package (it composes the package's own - **A failed poll is logged, never fatal.** `Ingest` stops at the first provider/sink error (`ingest.go:50-51`); the poller records it at `slog.Error` and moves to the next repo / next tick — the re-poll is - idempotent on the coordinate (`ingest.go:51-52`). The poller returns a - non-nil error only never — its sole exit is ctx cancellation, returning - `nil` so the errgroup drain (see wiring) treats shutdown as clean. + idempotent on the coordinate (`ingest.go:51-52`). `Run` never returns a + non-nil error — its sole exit is ctx cancellation, returning `nil` so the + errgroup drain (see wiring) treats shutdown as clean. - **Graceful shutdown:** `<-ctx.Done()` between passes and via the ctx threaded into `Ingest` mid-pass (both `ListIssues` and `PublishIssueUpdate` take ctx, `ingest.go:54,60`). @@ -266,14 +298,17 @@ Mirrors the two established precedents exactly: - **(e) Per-repo goroutines in the poller — rejected.** One forge, one rate budget; sequential per-repo fetches serialize naturally against `x-ratelimit-*` and keep the poller a trivially testable single loop. -- **(f) `since`-watermark incremental sync vs page ETags alone — chose - both, `since` primary.** GitHub's default `created`-desc list order means - one new issue shifts every page boundary and invalidates every page ETag, - so ETags alone collapse to a full paid refetch on any churn. - `sort=updated&since=` returns only changed rows and reduces a - quiet tick to ~1 request independent of repo size; page ETags then ride on - top for the no-change case. `since` is folded into the request lifecycle - (Approach step 2), not rejected — the two compose. +- **(f) `since`/`updated` incremental sync — deferred to OQ-A, ETag/304 alone + in v1.** A `since`-watermark would shrink a CHANGED tick (not just a + no-change one), but advancing it correctly requires observing the SINK + outcome, which an in-client watermark cannot do (`forge.Issue` carries no + timestamp and the client's sole seam is `ListIssues`, `provider.go:37-53`); + advancing on fetch success alone strands un-sunk issues on a mid-pass sink + failure and voids the re-poll idempotency invariant (Global Constraints → + Idempotency). v1 therefore uses ETag/304 alone — idempotency-safe, and + enough to make a quiet tick free. The correct home for an incremental cursor + is a DURABLE, poller-owned watermark that advances only on full-pass success + (OQ-A Option 1.5); it is priced there, not adopted here. ## Plan @@ -281,7 +316,7 @@ Mirrors the two established precedents exactly: New file `go/internal/forge/github.go` (+ `github_test.go`). Implements the request lifecycle from the Approach: bearer auth via `TokenSource`, -`sort=updated&since=` incremental fetch, ETag conditional requests +ETag conditional requests with a per-URL response+`Link`-chain cache, `x-ratelimit-*` budget gate with a typed budget error, `Link`-header pagination at `per_page=100`, `IssueFilter` → query-param mapping (empty `State` → `state=all`), `pull_request`-row @@ -329,8 +364,12 @@ Test cycle (unit, stubbed `http.RoundTripper`; red-first per repo convention): request. 5. Empty `IssueFilter.State` → `state=all` on the wire; labels join correctly. 6. A `pull_request`-keyed row is dropped. -7. 403/404/500 → `errors.As(..., *forge.StatusError)` with matching Status and - the GitHub `message` field. +7. 404/500, and a 403 WITHOUT rate-limit headers (a bad-credentials/permission + `message`) → `errors.As(..., *forge.StatusError)` with matching Status and + the GitHub `message` field, AND the client signals a token re-resolve (the + 401/bad-creds-403 discriminator of step 3 — asserted on the ABSENCE of + `retry-after`/zeroed `x-ratelimit-remaining`, so a rate-limit 403 does NOT + take this path). 8. Token from `TokenSource` lands as `Authorization: Bearer …`; a TokenSource error propagates without a request. 9. Multi-page 304: a two-page list is fetched, then a scripted 304 on page 1 @@ -390,6 +429,11 @@ Test cycle (unit, fake `Ingester` deps — the package's existing 5. Log fields: capture a `slog` test handler and assert a happy pass logs at Info with `repo`/`issues`/`dur`, a budget-skip logs at Warn, and a genuine `Ingest` error logs at Error with `err`. +6. Idempotency under mid-pass sink failure (defends the restored invariant): + a fake returns N issues but the sink fails on issue k on the first pass; on + the next tick the poller RE-FETCHES the full set (no `since` skip) and every + issue k..N sinks — none is stranded. Asserts the invariant the v1 + no-`since` decision preserves. ### T3 — serve boot wiring + config surface + secret resolve @@ -439,11 +483,17 @@ Test cycle: real projection and lands in `ListBoardIssues` — the end-to-end boot-wiring proof without touching live GitHub. (The fake satisfies the same structural `forgeReader`, `ingest.go:20-22`.) -4. pgtest: a CLOSED-state issue from the fake sinks with `ForgeState` set and - the row's `State` left untouched — the no-clobber contract this driver - leans on (`issue_projection.go:207`, `protoToForgeFields` writes - `ForgeState` but never `State`), so board ingestion cannot overwrite a - human-set board column. +4. pgtest (the no-clobber contract's runtime proof — the test MUST be able to + go RED): (1) ingest/create the issue via the projection, (2) set a + non-default lifecycle `State` through the part-5 write path + (`store.SetIssueState`, `issues.go:140`), (3) re-ingest a forge UPDATE for + the same coordinate (forge_state open→closed) via the fake, (4) assert + `ForgeState` is updated AND `State` is unchanged from the human-set value + (`issue_projection.go:207`, `protoToForgeFields` writes `ForgeState` but + never `State`). A regression adding `state = EXCLUDED.state` to the + `UpsertIssueForgeFields` SET clause (`issues.go:122-125`) MUST turn this + red — the as-worded "State left untouched" over a zero-value row would stay + green vacuously, so the human-set baseline in step 2 is load-bearing. 5. Unit: the `TokenSource` re-resolves after its TTL and on a scripted auth failure, and a changed resolver value is used on the next fetch — the design's stated reason for a resolver closure rather than a captured token. @@ -524,8 +574,16 @@ readings: as the hedge** if the convergence with PR-C/DL-053 must be designed now rather than deferred. If Matt rules Option 2, T2/T3 are re-cut (T1 survives nearly intact — the client is the shared fetch half either way). The PR-C -double-fetch is the concrete thing each option resolves differently. PARKED -for Matt; the freeze waits on this ruling. +double-fetch is the concrete thing each option resolves differently. +**A consequence surfaced by review that sharpens the fork:** a `since`/`updated` +incremental fetch cannot be done correctly in an in-client watermark (it +cannot observe the sink outcome, so it strands issues on a mid-pass sink +failure — Global Constraints → Idempotency), so v1 Option 1 uses ETag/304 +ONLY and a changed tick pays a full refetch of its changed pages. Option 1.5's +durable, poller-owned cursor is the ONLY place a correct incremental `since` +lives — so if the changed-tick cost or the resync-on-restart matters, that is +an argument FOR 1.5 now rather than deferring it. PARKED for Matt; the freeze +waits on this ruling. ### OQ-B (non-load-bearing deferral): live-demo content From cbe9b265809eff4b619e4ce456024dca8daaddd1 Mon Sep 17 00:00:00 2001 From: seal Date: Fri, 7 Aug 2026 14:02:43 -0400 Subject: [PATCH 3/5] docs(designs): re-cut forge-poll driver on DL-053 (Option 2) + fold red-team Matt ruled OQ-A = Option 2: build the forge-poll driver ON the DL-053 forge-subscription machinery from day one, board as a distinguished subscriber. Re-cut T1-T4 around a durable sink-gated forge_list_cursors fetch cursor; DL-053/DL-129 added as frozen constraints. Folds the design-critic red-team (0 high / 6 medium / 3 low): M1 boundary probe for the stored-has_next walk gap; M3 forge.Issue.UpdatedAt widened at T1 for the DL-129 recency guard; M4 TokenSource invalidation seam; M6 OQ-C owns the LIST-cursor class; L1 multi-replica precondition clause; L2 advanced_at rename on the list cursor; L3 dual startup error text. OQ-D reframed to two sub-forks (M2+M5): land the two spec-d DL-053 tables now vs defer, and the provider-domain CHECK. Refs SEA-1810. Co-authored-by: Matt Wilkinson --- .../compass-forge-poll-driver/design.md | 1168 ++++++++++++----- 1 file changed, 846 insertions(+), 322 deletions(-) diff --git a/docs/designs/product/compass-forge-poll-driver/design.md b/docs/designs/product/compass-forge-poll-driver/design.md index 42fbf660..a0b03926 100644 --- a/docs/designs/product/compass-forge-poll-driver/design.md +++ b/docs/designs/product/compass-forge-poll-driver/design.md @@ -4,12 +4,16 @@ Status: Draft Lane: compass-server Tracker: SEA -Freeze blocked on OQ-A (see Open Questions) — Matt rules the DL-053 -relationship before this record freezes. This record designs the running -driver that periodically drives the existing greenfield `go/internal/ingest` -library against a real GitHub forge, so board issues ingested from the tracker -appear live in the Compass server projection. Design layer only — no -implementation code ships with this record. +OQ-A is RESOLVED — Matt ruled **Option 2** (2026-08-07): this driver is built +ON the DL-053 forge-subscription machinery from day one, with the board as a +distinguished subscriber. Freeze now waits on OQ-C and OQ-D (see Open +Questions) — two shape calls inside the Option-2 build. This record designs +the running driver that periodically drives the existing greenfield +`go/internal/ingest` library against a real GitHub forge, so board issues +ingested from the tracker appear live in the Compass server projection — and +it instantiates the DL-053 fetch-cursor model (migration + store + poll +driver) as it does so. Design layer only — no implementation code ships with +this record. ## Problem / Intent @@ -21,14 +25,57 @@ references, e.g. `server/board.go:40` "the PR-B poll driver … add producers without changing the executor signature", and the pgtest comment `server/service_board_pgtest_test.go:14` "exactly as part 3's poller would"). The `internal/ingest` library (`ingest.go:35-65`) is complete and tested but is -exercised only by its own tests and the board pgtest suite. This slice adds the -running driver: a ctx-cancellable background poll loop started at serve boot, -plus the first real (non-fake) forge read client behind it. +exercised only by its own tests and the board pgtest suite. + +This slice adds the running driver as **the first DL-053 forge subscriber**: +the DL-053 conditional-poll loop (durable Postgres FETCH cursor → conditional +GET → sink → advance), started at serve boot under the serve errgroup, with +the board as its distinguished, always-on local subscriber and the first real +(non-fake) forge read client behind it. The DL-053 forge-subscription tables +do not exist yet (migrations in `go/internal/store/migrations/` stop at +`0014_channel_policy.sql`; a repo-wide search for `forge_subscriptions` / +`forge_artifact_cursors` under `go/` returns nothing — the existing +`0006_delivery_cursors.sql` table is `agent_delivery_cursors`, the comms +agent-notification fan-out, "how far an agent has confirmed delivery on a +channel", a different subsystem entirely). This slice therefore builds the +fetch half of that machinery: the migration, the cursor store, and the poll +driver that PR-C (tracker-status ingestion, DL-129) and the later +agent-notification slice both ride — one fetch path, no convergence refactor +ever needed. ## Global Constraints - **Toolchain:** module `github.com/sealedsecurity/compass/go`, go 1.25.0 (`go/go.mod`). +- **DL-053 (FROZEN, Matt 2026-07-27, `DECISIONS.md:76`) — the target model, + not an option:** "Forge subscriptions are Server-side Postgres rows with a + per-artifact FETCH cursor (advanced on any 200) split from a per-subscriber + DELIVERY cursor (advanced only on that subscriber's own successful notify), + change-detected by conditional polling in v1 (webhooks are an additive + accelerator), delivered by account on the existing `Sessions` → + `AgentGateway.Control` push path." Full text + DDL: + `compass-server-ownership-layer/design.md:942-1019` (Decision 5). This + record consumes that model — it never amends it. The genuine forks in + applying it to a repo-LIST board subscriber are PARKED for Matt as OQ-C (how + the board is represented as a subscriber) and OQ-D (whether the two spec'd + tables land in this migration at all, and if so which provider domain their + CHECK admits). What is NOT a fork: the repo-LIST fetch-cursor CLASS + (`forge_list_cursors`, Approach (a)) is author-adopted as the only coherent + shape for a LIST-granularity ETag — invariant under both OQ-C options, so + ratifying either ratifies the class — and the coordinate alignment to the + tree's 0013 convention (SMALLINT provider + `forge_host`) is the mechanical + application of an existing standard. The two spec'd tables are labeled + `0004_forge` in that (older) record; the tree's migrations reach `0014`, so + if they land they land here as part of `0015_forge_subscriptions.sql`. +- **DL-129 (FROZEN, Matt 2026-08-04, `DECISIONS.md:173`):** tracker native + status "is ingested into the DL-070 server projection through the reverse + `TrackerStatusMapping` (DL-053 poll; echo-suppressed in tracker-status + space, tracker-sourced transitions never mirror back, stale polls dropped + by a recency guard)". That names THIS driver as the poll PR-C rides: + tracker status is a field on the same `/issues` rows this loop fetches + (`forge.Issue.State`, `provider.go:45-46` — "the forge's truth + (\"open\" | \"closed\")"), so PR-C MUST NOT re-fetch the endpoint (see + Approach → the one-fetch-path guarantee). - **OQ-6 (FROZEN, Matt 2026-07-27):** the GitHub client is hand-rolled over `net/http`, **no new dependency** (~300 LOC); go-github is REJECTED — it lacks a conditional-request + budget mechanism hook. Conditional requests @@ -44,98 +91,256 @@ plus the first real (non-fake) forge read client behind it. - **Error mapping:** every non-2xx forge response maps to `*forge.StatusError{Status, Message}` (`forge/provider.go:216-227`), so the Service layer can flatten statuses without inspecting the wire. -- **Idempotency (load-bearing):** a failed or partial poll is safe to retry — - "partial progress is fine — a re-poll is idempotent on the coordinate" - (`ingest.go:48-52`; the coordinate is the store upsert key, - `board/issue_projection.go:69-71`). This holds because `Ingester.Ingest` - stops at the first sink error (`ingest.go:60-62`) and the next pass - RE-FETCHES the full issue set, so a fetched-but-not-yet-sunk issue is retried - next tick. **The invariant therefore forbids any fetch-side optimization that - skips re-fetching an un-sunk issue** — a per-repo `since`/`updated` watermark - advanced on fetch success (not full-pass success) would strand every issue - below it whenever a mid-pass sink error fires, permanently absent from the - projection until re-touched upstream. An in-client watermark cannot see the - sink outcome (the client's sole seam is `ListIssues([]Issue, error)` and - `forge.Issue` carries no timestamp, `provider.go:37-53`), so v1 does NOT do - `since` incrementalism; steady-state cost is bounded by ETag/304 (Approach - step 2) instead, and a correct incremental cursor is homed in OQ-A Option 1.5 - where a durable, poller-owned watermark can gate its advance on full-pass - success. +- **Idempotency (load-bearing, now expressed over the DURABLE cursor):** a + failed or partial poll is safe to retry — "partial progress is fine — a + re-poll is idempotent on the coordinate" (`ingest.go:48-52`; the coordinate + is the store upsert key, `store/migrations/0013_issues.sql:57-59`). The + invariant the durable FETCH cursor must preserve: **a stored list-page ETag + attests that every row of that page's content was durably sunk when the + ETag was stored.** Concretely: the driver advances a page's cursor row ONLY + AFTER every issue on that page has sunk through + `IssueProjection.PublishIssueUpdate` (a durable upsert, + `issue_projection.go:68-74`). A mid-page sink failure leaves the old ETag in + place, so the next tick's conditional GET carries the OLD ETag, the changed + content returns `200`, and the page re-sinks — no issue is ever stranded + behind an advanced cursor. This is the same invariant DL-053's + "advances unconditionally on any 200" expresses for per-artifact rows, + where the fetched state persists as `snapshot` **in the same row as the + ETag** (`compass-server-ownership-layer/design.md:1016`): never advance a + fetch cursor past state you have not durably captured. Here the durable + capture is the projection upsert itself, so the advance follows the sink; + the advance is still NEVER conditioned on any subscriber's wire delivery + (the board has none — see Approach), which is the coupling DL-053 forbids. - **No projection change:** `board.IssueProjection.PublishIssueUpdate(ctx, *compassv1.Issue) error` (`board/issue_projection.go:68`) ALREADY satisfies `ingest.issueSink` (`ingest.go:31-33`). This slice adds zero code to `internal/board`. -- **Non-goals:** the tracker-status WRITE-back / outbound mirror + - echo-suppress (T3-b / PR-C, a separate slice); the DL-053 agent-notification - subscription delivery machinery (Sessions → `AgentGateway.Control` push). - This record is the BOARD-ingestion read poll only. +- **`ingest` imports no store:** the package doc holds that "it imports NO + store. The proto->store mapping edge is part 4 (the sink's real + implementation)" (`ingest.go:7-8`). The driver's durable-cursor dependency + therefore enters `ingest` as a package-local structural interface + (`CursorStore`, T3), with the `*store.Store` adapter living in `server/` + (T4) — the same narrow-seam pattern as `forgeReader`/`issueSink` + (`ingest.go:20-33`). +- **Non-goals:** the tracker-status ingestion itself (PR-C — the reverse + `TrackerStatusMapping`, echo-suppression, recency guard; a separate slice + that consumes the seam this record makes explicit); the tracker-status + WRITE-back / outbound mirror (T3-b); the agent-notification DELIVERY half + of DL-053 (per-agent `forge_subscriptions` rows, `DetectChanges`, + per-artifact comment/check ETags, the `Sessions` → `AgentGateway.Control` + push) — that slice adds subscribers and wire delivery ON the tables and + driver built here, it does not rebuild the fetch path. This record is the + BOARD-ingestion read poll plus the DL-053 fetch machinery it stands on. ## Approach -Three pieces, one per task: a concrete GitHub read client in `internal/forge`, -a poll-loop scheduler in `internal/ingest`, and boot wiring + config surface in -`server/serve.go` + `cmd/compass-server/main.go`. +Four pieces, one per task: a concrete GitHub read client in `internal/forge` +(T1, carried over from the pre-ruling cut nearly intact), the DL-053 cursor +migration + store layer (T2), the conditional-poll driver in `internal/ingest` +(T3), and boot wiring + config surface in `server/serve.go` + +`cmd/compass-server/main.go` (T4). -### The GitHub read client (`forge.GitHub`, new file `go/internal/forge/github.go`) +### The impedance Option 2 must design around: per-artifact vs repo-LIST -A hand-rolled `net/http` client implementing, at minimum, the read half that -satisfies `ingest.forgeReader`: +DL-053's subscription model is **per-artifact**: "An agent subscribes to a +*forge artifact*: an issue or a PR, named by `(provider, repo, kind, number)`" +(`compass-server-ownership-layer/design.md:944-945`), and the FETCH cursor +table is keyed the same way (`:1007-1019`, `PRIMARY KEY (provider, repo, kind, +number)`). SEA-1810's board ingestion is **per-repo LIST**: fetch every issue +in a repo in one paginated `/issues?state=all` walk (`Ingester.Ingest` +"fetches every issue for repo", `ingest.go:48`) and sink each. A list-page +ETag is a caching fact about a URL (repo + filter + page), not about any one +artifact — it cannot be stored per-artifact without inventing data. Three +framings were weighed: -```go -ListIssues(ctx context.Context, repo string, f forge.IssueFilter) ([]forge.Issue, error) +- **(a) The board is a distinguished repo-scoped subscriber — ADOPTED for the + fetch cursor.** The repo LIST is its own fetch-cursor class: a new + `forge_list_cursors` table keyed `(provider, host, repo, page)` holding the + per-page ETag + Link-chain fact (`has_next`), sibling to (not a widening of) + the DL-053 per-artifact `forge_artifact_cursors`. It is the same + fetch-cursor IDEA — a durable conditional-GET cache, advanced with the + durably captured state, shared by every consumer of the fetch — applied at + the LIST granularity. The board's DELIVERY cursor is degenerate: the board + is one always-on, in-process consumer whose "notify" is the synchronous + projection sink inside the poll pass, so sunk-ness is already encoded by + the page-cursor advance (Global Constraints → Idempotency) and no + `delivered_revision` row is needed — exactly the per-subscriber cursor + DL-053 splits out for subscribers whose notify can fail independently of + the fetch, which the board's cannot (blessing this reading is OQ-C). +- **(b) The board subscribes to each issue individually, seeded by a repo + enumeration — REJECTED.** N `forge_subscriptions` rows per repo, and the + enumeration that seeds them IS a repo LIST fetch — so the LIST (and its + paging/ETag problem) remains, plus a per-issue row-churn loop on top. It + removes nothing and adds a reconcile. +- **(c) The LIST fetch feeds per-artifact state as it enumerates — ADOPTED as + the sharing seam.** Every `/issues` LIST row carries the full issue payload + including its forge state (`forge.Issue.State`, `provider.go:45-46`), so + the one LIST fetch can feed N consumers: the board sink today; PR-C's + reverse-status mapping tomorrow (same pass, second sink — it consumes the + rows this fetch already returns, never a second request, though it reads + fields beyond `State`: DL-129's recency guard needs the row's `updated_at`, + which T1 therefore parses into `forge.Issue` from day one, see the + one-fetch-path guarantee); and, when the agent-notification slice lands, + LIST-fed upserts of `forge_artifact_cursors.revision`/`snapshot`/`polled_at` + for subscribed issues (their per-issue `etag` column stays `''` — it belongs + to the single-issue endpoint the LIST never hits; comments/checks ETags + likewise arrive only with that slice's own endpoints). The driver exposes + this as a seam (T3: the per-pass raw-issue batch flows through one + `Ingester.IngestIssues` call a second sink can be composed onto), and this + record keeps every downstream writer OUT of its build. + +**The one-fetch-path guarantee (the property Matt's ruling bought), stated +explicitly:** PR-C never issues a request against `/repos/{owner}/{repo}/issues`. +The endpoint has exactly one caller — this driver's T1 client — and exactly one +cursor state — `forge_list_cursors`. PR-C lands as an additional consumer of +the same pass (the reverse `TrackerStatusMapping` applied to the same +`[]forge.Issue` batch the board sink receives), so DL-129's "DL-053 poll" IS +this driver, the shared rate budget is spent once, and no convergence refactor +is ever needed. What PR-C adds on the fetch side: no request, no endpoint, no +cursor state — only, at most, additional fields parsed off rows this fetch +already returns (the `updated_at` its recency guard needs is parsed here in +T1; `state_reason` similarly, if the reopen mapping wants it). "One fetch +path" is exact; "adds literally nothing" would not be, so the claim is stated +as the former. + +### The DL-053 tables (`0015_forge_subscriptions.sql`) + +This record's DESIGN lands all three tables in one migration — the two +DL-053-spec'd tables plus the repo-LIST cursor class — adapted from the +ownership-layer DDL (`compass-server-ownership-layer/design.md:976-1019`, there +labeled "`0004_forge`"; the label predates the current migration count, so it +lands as `0015`). Two calls in that adaptation are author-decided (mechanical +application of an existing tree standard); one is a genuine fork PARKED for +Matt as OQ-D. + +Author-decided: + +- **Coordinate alignment:** the spec'd DDL keys on `provider TEXT` and omits + the forge host; the tree's frozen issue coordinate is `(forge_provider + SMALLINT CHECK (forge_provider IN (1, 2, 3)), forge_host, repo, number)` + (`0013_issues.sql:29-35` — "forge coordinate: the idempotency key"). The + cursor and subscription keys adopt the 0013 convention (SMALLINT provider + enum + `forge_host TEXT NOT NULL`) so cursor rows join the `issues` + coordinate without casts and GHES hosts never collide on `repo` alone. +- **The new LIST-cursor class:** `forge_list_cursors` (below) is additive — + neither spec'd table changes shape, and it lands regardless of OQ-D (the + driver rides it directly). + +PARKED for Matt (OQ-D): + +- **Whether the two spec'd tables land in THIS slice at all.** Nothing this + slice EXECUTES touches `forge_subscriptions` or `forge_artifact_cursors`: the + driver rides `forge_list_cursors` only, the T2 store methods cover only the + list cursor, and every downstream writer is out of this build. So they land + now purely as anticipatory schema. The design's default (what the DDL below + assumes) is land-now — one forge migration, the shape frozen + pgtest-covered + before its writers exist. The counter-position (defer each spec'd table to + the slice that first writes it) is real: an EMPTY table is the cheapest thing + to `ALTER`, so freezing its shape early buys little over landing it beside + its first writer with more information, and landing it now FORCES the OQ-D + provider-domain call (below) before PR-C or the agent-notification slice + exist to inform it. This is OQ-D's first sub-fork. +- **If they land now: which provider domain their CHECK admits.** DL-053's + spec'd DDL keys `provider TEXT -- "github" | "linear"` + (`compass-server-ownership-layer/design.md:981`) — Linear is a first-class + provider of subscription/cursor rows in the frozen spec. Adopting the 0013 + coordinate's `SMALLINT CHECK (... IN (1, 2, 3))` (GitHub/GitLab/Forgejo, + `0013_issues.sql:33`) consciously NARROWS that domain: it drops `linear` + until the `ForgeProvider` enum and the CHECK grow. This is not "no semantic + change" — DL-129 routes tracker-status ingestion through "the DL-053 poll" + (`DECISIONS.md:173`) and the tracker for MVP is GitHub, so the narrowing is + likely harmless FOR NOW, but a future Linear tracker needs the enum/CHECK + extended (cheap while the tables are empty). OQ-D's second sub-fork; moot if + the first resolves to defer. + +If OQ-D resolves to defer, this migration lands `forge_list_cursors` alone and +the two spec'd tables ride their first writers; the rest of this record is +unchanged (the driver never touched them). The DDL below is the driver's own +working table either way. + +The driver's own working table: + +```sql +-- FETCH cursor for a repo-level issue LIST walk: one row per (coordinate, +-- page). The repo-LIST analogue of forge_artifact_cursors — a durable +-- conditional-GET cache, one writer (the poll driver), N read-side consumers. +-- etag advances ONLY after every row of that page's content is durably sunk +-- (the projection upsert is the snapshot store — see the record's Idempotency +-- constraint). has_next persists the Link-chain fact so a 304 (which need not +-- re-send Link) can keep walking a multi-page repo. NOTE the one gap of the +-- stored-Link walk: content that enters the repo sorted BEYOND the last stored +-- page (an issue transferred/imported with an old created_at, under GitHub's +-- default created-desc order) perturbs no stored page — every page 304s and +-- the last row's has_next=false stops the walk, so the new content is missed +-- until unrelated page-1 activity forces a 200. The driver closes this with a +-- bounded unconditional probe of last_page+1 every Nth all-304 walk (see the +-- poll driver) — no schema change. advanced_at records the last content +-- advance (an etag-storing 200+sink), NOT the last poll: an all-304 tick reads +-- the page but rewrites no row, so the column deliberately names "last change". +CREATE TABLE forge_list_cursors ( + forge_provider SMALLINT NOT NULL CHECK (forge_provider IN (1, 2, 3)), + forge_host TEXT NOT NULL, + repo TEXT NOT NULL, + page INTEGER NOT NULL CHECK (page >= 1), + etag TEXT NOT NULL DEFAULT '', + has_next BOOLEAN NOT NULL DEFAULT FALSE, + advanced_at TIMESTAMPTZ NOT NULL DEFAULT now(), + PRIMARY KEY (forge_provider, forge_host, repo, page) +); ``` -(`ingest.go:23-25` — the interface is unexported but structural, so any -concrete type with this method passes `ingest.NewIngester`, `ingest.go:44`.) -It lives in `internal/forge` because that package is "the ONLY place a forge -credential is used" (ownership-layer record, quoted at -`compass-server-ownership-layer/design.md:732-734`), and it is the first -increment of the full `forge.Provider` (`provider.go:195-207`) — the write -half arrives with later slices and is out of scope here; until then the type -does NOT claim to implement `Provider` (no `var _ Provider` assertion; only -`ListIssues` is built and only `forgeReader` consumes it). +What the durable cursor buys over the pre-ruling in-memory ETag map: no full +`state=all` re-fetch storm on server restart (an unchanged repo is an all-304 +pass from the first post-boot tick — the projection is already durable and +`Rehydrate`-seeded, `serve.go:259-263`, so a 304 page needs no re-sink); +a durable substrate a future multi-replica server COULD share (a precondition, +not a sufficiency — the projection sink assumes single-threaded ingestion per +coordinate, `issue_projection.go:62-67`, so multi-replica would still need +per-coordinate serialization / leader election this slice does not design); +and the "shared fetch primitive" PR-C rides is a real table, not an aspiration +inside one process's heap. -Behavior, in order of the request lifecycle: +### The GitHub read client (`forge.GitHub`, new file `go/internal/forge/github.go`) — T1, survives the ruling + +The pre-ruling T1 survives nearly intact — the client is the shared fetch half +either way. The ONE structural change the ruling forces: the client becomes +**stateless about cursors**. The pre-ruling cut kept a per-URL in-memory +ETag + parsed-response + Link-chain cache inside the client; under Option 2 +the ETag lives in `forge_list_cursors` and the DRIVER owns it (it must — only +the driver can see the sink outcome the advance is gated on, which is exactly +why an in-client cursor was already flagged as structurally unable to be +correct: "the client's sole seam is `ListIssues([]Issue, error)`", +`provider.go:37-53` carries no timestamp and no sink signal). So the client +gains a page-level conditional-fetch primitive that takes the caller's ETag +and reports `NotModified` without caching anything, and the whole-walk +`ListIssues` remains as an unconditional convenience built on it (it is also +the read half of the eventual `forge.Provider`, `provider.go:195-207`; no +`var _ Provider` assertion — read half only). + +A hand-rolled `net/http` client (OQ-6). Behavior, in order of the request +lifecycle: 1. **Auth.** Every request carries `Authorization: Bearer ` where the - token comes from a `TokenSource func(ctx) (string, error)` — resolved per + token comes from `TokenSource.Token(ctx)` — resolved per request batch, not captured at construction, so a rotated `server_only` - secret takes effect without a restart (composes with DL-052; see T3 for the + secret takes effect without a restart (composes with DL-052; see T4 for the resolve wiring). -2. **Conditional requests (ETag/304) — the steady-state reducer.** A per-URL - in-memory ETag cache (keyed by repo+filter+page) sends `If-None-Match`; a - `304 Not Modified` returns the cached parsed `[]forge.Issue` **and the - cached `Link` chain for that key** — a 304 need not re-send `Link`, so the - next-page URLs MUST be cached alongside the ETag or a multi-page repo cannot - walk past page 1 — and, per current GitHub docs, a 304 is not charged - against the core rate limit. **That last fact is load-bearing for the budget - math below and MUST be reverified against current GitHub REST docs at T1** - (the rate-limit docs have been revised before). On a no-change tick every - page serves from cache: a quiet repo costs zero rate budget and the full - cached slice is re-sunk (idempotent on the coordinate). The ETag cache is - bounded by repos × pages with a fixed size cap (pages grow with repo size) - — no eviction policy beyond the cap at this scale. - - A `since`/`sort=updated` incremental fetch — which would also shrink a - CHANGED tick, not just a no-change one — is deliberately NOT in v1. An - in-client watermark advanced on fetch success breaks the re-poll idempotency - invariant above (a mid-pass sink failure strands the issues below it), and - the client structurally cannot observe sink outcome to advance it correctly - (Global Constraints → Idempotency). So v1 relies on ETag/304 alone, which is - idempotency-safe: a 304 means nothing changed and nothing to sink, and a - changed page returns 200 and re-fetches that page's full set. The cost this - accepts: a repo with any new issue pays a full paid refetch of its changed - pages that tick (GitHub's default `created`-desc order shifts page - boundaries and invalidates page ETags on insert) — bounded, budget-gated - (step 3), and re-priced as the concrete tradeoff of deferring `since` to a - durable cursor (OQ-A Option 1.5, Alternatives (f)). +2. **Conditional requests (ETag/304) — the steady-state reducer.** The caller + passes the page's stored ETag; the client sends `If-None-Match` and maps a + `304 Not Modified` to `ListPage{NotModified: true}` — no body, no re-parse, + nothing cached client-side. Per current GitHub docs a 304 is not charged + against the core rate limit. **That fact is load-bearing for the budget + math and MUST be reverified against current GitHub REST docs at T1** (the + rate-limit docs have been revised before). On a no-change tick every page + of a quiet repo costs zero rate budget and sinks nothing (the projection + already holds the rows durably). 3. **Rate budget.** After every response the client records `x-ratelimit-remaining` and `x-ratelimit-reset`. Before issuing a request, if `remaining <= reserve` (default reserve 10), it fails fast with a typed budget error (`forge.ErrBudgetExhausted`, exact shape in T1 Interfaces) - rather than burning the tail of the budget; the poller skips that cycle and + rather than burning the tail of the budget; the driver skips that cycle and retries next tick (safe: re-poll is idempotent, `ingest.go:51-52`). Budget - exhaustion is an EXPECTED skip, not a failure — the poller detects + exhaustion is an EXPECTED skip, not a failure — the driver detects `errors.Is(err, forge.ErrBudgetExhausted)` and logs it at `slog.Warn`, NOT the `slog.Error` path a real provider/sink error takes, so a rate-limited window is not per-tick Error noise. **403 disambiguation (a single rule — @@ -144,33 +349,39 @@ Behavior, in order of the request lifecycle: `x-ratelimit-remaining` → `forge.ErrBudgetExhausted` (Warn, budget-skip, NO token re-resolve); a `401`, or a `403` whose body `message` is a bad-credentials/permission error with NO rate-limit headers → - `*forge.StatusError` AND signals the caller to re-resolve the token (the T3 - secret-resolve bullet); any other non-2xx → `*forge.StatusError` only + `*forge.StatusError` AND, before returning, the client calls + `Invalidate()` on its `TokenSource` (Interfaces below) so the NEXT batch + re-resolves — the T4 TTL cache drops its value on `Invalidate`; the seam is + the client's, since the client is the only component that observes the auth + failure (the T4 secret-resolve bullet wires it); any other non-2xx → + `*forge.StatusError` only (step 7). The discriminator is the presence of `retry-after` / a zeroed `x-ratelimit-remaining`, nothing else — so a dead token is never mistaken for a rate-limit skip (which would retry forever with the dead token, never re-resolving) and a rate-limit 403 never becomes per-tick Error spam. The client never sleeps holding the caller's poll slot — backoff is the - poller's ticker, not an in-client `time.Sleep`. (Under sustained pressure a + driver's ticker, not an in-client `time.Sleep`. (Under sustained pressure a skipped tick can defer ingestion up to a full reset window (~60 min); acceptable given idempotency, and ETag/304 keeps steady-state spend near zero.) 4. **Pagination.** GitHub's list-issues endpoint pages; the client requests - `per_page=100` and follows RFC-5988 `Link: rel="next"` headers, - concatenating pages into one `[]forge.Issue` (the `Ingester` contract is - "fetches every issue for repo", `ingest.go:48`). + `per_page=100`. `ListIssuesPage` reports whether an RFC-5988 `Link: + rel="next"` exists (`HasNext`) — the DRIVER walks pages so it can sink and + advance the cursor per page; `ListIssues` follows the chain itself and + concatenates (the `Ingester` contract is "fetches every issue for repo", + `ingest.go:48`). 5. **Filter mapping.** `forge.IssueFilter{State, Labels}` (`provider.go:182-188`) maps to `state=`/`labels=` query params. An empty - `State` — which is what `Ingester.Ingest` always sends - (`forge.IssueFilter{}`, `ingest.go:54`) — maps to **`state=all`**, this - provider's documented default (`provider.go:183-184` delegates the empty - case to "the provider's default"). Rationale: the board needs closed - tracker issues to flow so their `ForgeState` stays fresh on the row; an - open-only default would silently freeze closed issues at their last-seen - forge state. (The reopen→un-archive LIFECYCLE transition rides - tracker-STATUS ingestion — PR-C / OQ-A, DL-129 — not this slice, which + `State` — which is what the board path always sends — maps to + **`state=all`**, this provider's documented default (`provider.go:183-184` + delegates the empty case to "the provider's default"). Rationale: the + board needs closed tracker issues to flow so their `ForgeState` stays fresh + on the row; an open-only default would silently freeze closed issues at + their last-seen forge state. (The reopen→un-archive LIFECYCLE transition + rides tracker-STATUS ingestion — PR-C, DL-129 — not this slice, which writes `ForgeState` but never `State`; `state=all` here only keeps - `ForgeState` current on closed rows.) + `ForgeState` current on closed rows. Under Option 2 PR-C applies that + mapping to THIS driver's rows — the same `state=all` fetch is its input.) 6. **PR exclusion.** GitHub's issues API returns pull requests as issues (rows carrying a `pull_request` key); the client drops them — the board's PR surface is a separate lane, and `forge.Issue` (`provider.go:37-53`) is @@ -182,34 +393,90 @@ Behavior, in order of the request lifecycle: are returned RAW — the owner-header strip belongs to ingestion (`provider.go:42-44`, `ingest.go:67-74`), never the provider. -### The poll-loop scheduler (`ingest.Poller`, new file `go/internal/ingest/poller.go`) +### The conditional-poll driver (`ingest.Driver`, new file `go/internal/ingest/driver.go`) — the DL-053 poll -A small scheduler in the `ingest` package (it composes the package's own -`Ingester`; a new package would be a second convention for no gain): +The DL-053 poll loop, specialized to its first artifact class (the repo LIST) +and its first subscriber (the board). It lives in the `ingest` package (it +composes the package's own `Ingester`; a new package would be a second +convention for no gain) and consumes the durable cursor through the +package-local `CursorStore` structural interface (Global Constraints → ingest +imports no store). - `Run(ctx)` performs one immediate pass at start (so a freshly booted demo server shows the board without waiting a full interval), then ticks on `time.Ticker(interval)`. -- Each pass iterates the configured repos and calls - `Ingester.Ingest(ctx, repo)` (`ingest.go:53`) sequentially — one forge, one +- Each pass iterates the configured repos sequentially — one forge, one budget; parallel per-repo fetches would just race the same rate limit. -- **A failed poll is logged, never fatal.** `Ingest` stops at the first - provider/sink error (`ingest.go:50-51`); the poller records it at - `slog.Error` and moves to the next repo / next tick — the re-poll is - idempotent on the coordinate (`ingest.go:51-52`). `Run` never returns a - non-nil error — its sole exit is ctx cancellation, returning `nil` so the - errgroup drain (see wiring) treats shutdown as clean. +- **Per-repo pass = the DL-053 read-modify-advance cycle, page-wise:** + + 1. Read the repo's stored page cursors (`CursorStore.ListCursor` → page → + `{etag, hasNext}`). + 2. From page 1: `ListIssuesPage(ctx, repo, forge.IssueFilter{}, page, + storedETag)`. + - **304** → nothing changed on that page and its content is already + durably sunk (the Idempotency invariant: a stored ETag attests the + sink); continue to the next page if the STORED row says `has_next` + (a 304 need not re-send `Link`, so the chain fact is read from the + cursor row, not the response). + - **200** → `Ingester.IngestIssues(ctx, repo, page.Issues)` (translate + + sink each, stop at first sink error — the existing per-issue pipeline, + `ingest.go:58-63`, factored to take a caller-fetched batch); ONLY on + full-page sink success, `UpsertListCursorPage` with the new ETag + + `HasNext`; continue while `HasNext`. + - **Budget error** (`errors.Is(err, forge.ErrBudgetExhausted)`) → Warn, + abandon this repo's pass; already-advanced pages stay advanced (their + content is sunk — safe). + - **Any other error** (provider or sink) → `slog.Error`, abandon this + repo's pass WITHOUT advancing the failing page; next tick re-fetches + it (the old ETag no longer matches the changed content → 200 → + re-sink; idempotent on the coordinate). + 3. After a clean full walk, `PruneListCursorPages(repo, lastPage)` — a repo + that shrank drops its stale tail rows. + 4. **Boundary probe (closes the stored-`has_next` gap, M1):** the stored-Link + walk in step 2 stops at the last stored page, so content that enters + sorted BEYOND that boundary without perturbing any stored page (a + transfer/import with an old `created_at`, under GitHub's default + `created`-desc order) is invisible to a pure 304 walk until unrelated + page-1 activity forces a 200. To bound the staleness, when a walk served + ALL 304s (no page advanced) the driver unconditionally fetches + `lastPage+1` once every Nth such consecutive all-304 walk (default N = 60, + ~hourly at the 1m interval; the counter is per-repo, reset by any 200). + A never-stored page is already an `etag=''` unconditional GET, so the + mechanism exists — a `HasNext`/non-empty result there resumes the normal + walk and re-anchors the tail; an empty 200 costs one request per hour on a + quiet repo. This is a documented probe rule, not a fork. + +- **A failed poll is logged, never fatal.** `Run` never returns a non-nil + error — its sole exit is ctx cancellation, returning `nil` so the errgroup + drain (see wiring) treats shutdown as clean. - **Graceful shutdown:** `<-ctx.Done()` between passes and via the ctx - threaded into `Ingest` mid-pass (both `ListIssues` and - `PublishIssueUpdate` take ctx, `ingest.go:54,60`). + threaded through fetch and sink mid-pass (both `ListIssuesPage` and + `PublishIssueUpdate` take ctx). +- **The board's delivery half is degenerate, by design:** DL-053 splits the + DELIVERY cursor per subscriber because "advancement conditional on a + successful notify" over a shared cursor produces deterministic duplicates + across subscribers with different liveness + (`compass-server-ownership-layer/design.md:949-961`). The board is one + in-process subscriber whose notify IS the synchronous sink inside the pass — + there is no wire delivery to fail independently of the fetch, so its + delivery state is fully encoded by the page-cursor advance and it holds no + `forge_subscriptions` row (OQ-C). Wire subscribers (agents) arrive with the + notification slice and bring their own `delivered_revision` rows, exactly + per spec. +- **The PR-C seam (downstream consumer, OUT of this build):** the per-pass + raw batch (`[]forge.Issue` per 200 page) flows through one + `Ingester.IngestIssues` call. PR-C composes its reverse + `TrackerStatusMapping` sink onto that same batch (and its per-artifact + `forge_artifact_cursors.revision`/`snapshot`/`polled_at` upserts, LIST-fed — + no per-issue GET), in its own slice. Nothing in this record's build writes + `forge_subscriptions` or `forge_artifact_cursors`. - **Observability:** one `slog.Info` per completed repo pass with fields - `repo`, `issues` (count sunk), `not_modified` (bool: whole fetch served - from cache), `dur`, `ratelimit_remaining`; one `slog.Warn` per - budget-skipped pass (`errors.Is(err, forge.ErrBudgetExhausted)` — an - expected condition, never `Error`); one `slog.Error` only per genuinely - failed pass (provider/sink error) with `repo`, `err`. Boot logs one - `slog.Info` with `repos`, `interval`. No per-issue logging — a 500-issue - repo must not emit 500 lines per minute. + `repo`, `issues` (count sunk), `pages`, `not_modified` (bool: whole walk + served 304s), `dur`, `ratelimit_remaining`; one `slog.Warn` per + budget-skipped pass (an expected condition, never `Error`); one + `slog.Error` only per genuinely failed pass (provider/sink error) with + `repo`, `err`. Boot logs one `slog.Info` with `repos`, `interval`. No + per-issue logging — a 500-issue repo must not emit 500 lines per minute. ### Boot wiring + config (`server/serve.go`, `cmd/compass-server/main.go`) @@ -217,43 +484,62 @@ Mirrors the two established precedents exactly: - **Config surface:** `server.ServeConfig` (`serve.go:56-89`) gains an optional `Forge ForgeConfig` field, all-optional exactly like `S3` - (`serve.go:71-75`): absent repos → no poller, today's behavior, zero new - requirements on existing deployments. The CLI (`main.go`) maps flags with + (`serve.go:71-75`): absent repos → no driver, today's behavior, zero new + requirements on existing deployments (the 0015 tables exist but sit empty — + a migration is not a behavior change). The CLI (`main.go`) maps flags with env fallback mirroring the `COMPASS_S3_*` precedence (`main.go:127-138`): `--forge-repos` / `$COMPASS_FORGE_REPOS` (comma-separated `owner/name`), `--forge-poll-interval` / `$COMPASS_FORGE_POLL_INTERVAL` (default `1m`), `--forge-secret` / `$COMPASS_FORGE_SECRET` (the declared secret NAME, default `GITHUB_FORGE_TOKEN`; the VALUE never crosses a flag), plus `--forge-host` / `$COMPASS_FORGE_HOST` (default `github.com`, for GHES - later; the API base URL derives from it). -- **Secret resolve:** the poller's `TokenSource` closes over the one + later; the API base URL derives from it). The configured repo list IS the + board's subscription set under this record's OQ-C recommendation — + declarative, reconciled implicitly every pass (a removed repo simply stops + being polled; its cursor rows are inert and its issues remain in the + projection). +- **Secret resolve:** the driver's `TokenSource` closes over the one `secrets.SpecResolver` built at `serve.go:287`, calls - `Resolve(ctx, "forge poll")` (`secrets/resolver.go:139`), and selects the + `Resolve(ctx, "forge poll")` (`secrets/resolver.go:135`), and selects the configured name from the returned `[]ResolvedSecret` — the same single-resolve-surface DL-052 mandates; the row is declared `server_only` so it never reaches a container (mechanism per the ownership-layer T5, `compass-server-ownership-layer/design.md:1991-1995`). **Resolve is not - cheap** — `resolver.go:139-176` reads the entire declared-secret registry + cheap** — `resolver.go:135-165` reads the entire declared-secret registry from the store, writes a manifest temp file, and drives a full secretspec provider `Load` (potentially an external provider call) per invocation — so - the `TokenSource` caches the resolved value behind a short TTL (default - `5m`) and re-resolves on expiry OR on a `401`/`403` auth response, NOT once - per poll pass; a token that rotates rarely does not tax the store and - secrets provider every minute, while a rotation still takes effect within - the TTL (or immediately on the next auth failure). A missing/undeclared - secret with forge config present is a startup error — fail fast, but at - `serve.go:287` the resolver is built AFTER the UDS listener binds, so the - validation follows the listener-cleanup path the `Rehydrate` failure - already uses (`serve.go:260-263`), returning the error from `Serve` rather - than a bare `main.go` exit. A resolve that breaks at RUNTIME (provider + the `TokenSource` implementation caches the resolved value behind a short TTL + (default `5m`); its `Invalidate()` (the T1 seam) drops the cached value so + the next `Token` call re-resolves, and the client calls it on a + `401`/bad-creds-`403` (Approach step 3). So a resolve happens on TTL expiry + OR on an auth failure, NOT once per poll pass: a token that rotates rarely + does not tax the store and secrets provider every minute, while a rotation + still takes effect within the TTL (or immediately on the next auth failure). + A startup resolve is attempted once when forge config is present, and its two + failure modes get distinct error text so a crash-loop is diagnosable: an + UNDECLARED/misnamed secret (the configured name is absent from the resolved + set) is a permanent misconfiguration — `"forge secret %q not declared"`; a + resolve that ERRORS at boot (secrets provider unreachable, store read + failing) is transient — `"forge secret resolve failed at startup: %w"`. Both + fail fast, but at `serve.go:287` the resolver is built AFTER the UDS listener + binds, so the validation follows the listener-cleanup path the `Rehydrate` + failure already uses (`serve.go:260-263`), returning the error from `Serve` + rather than a bare `main.go` exit. A resolve that breaks at RUNTIME (provider outage, a declaration deleted post-boot) surfaces per-pass as an auth error → `slog.Error` + retry next tick (idempotent); to keep a permanently broken - secret from degrading to silent forever-Error, the poller emits a distinct + secret from degrading to silent forever-Error, the driver emits a distinct `slog.Error` "forge token unresolvable" the first time a resolve fails and on recovery — a minimal health surface without per-tick spam. -- **Lifecycle:** the poller runs as one more `g.Go` member of the existing +- **Cursor-store adapter:** `serve.go` builds the small adapter satisfying + `ingest.CursorStore` over the already-open `*store.Store` (`st`), binding + the driver's `(provider, host)` so the ingest-side interface stays + repo-keyed. This is the store seam the ingest package's no-store rule + requires (Global Constraints), following the established + break-the-cycle/adapter convention of `wireHubServiceCycles` + (`serve.go:298-301`). +- **Lifecycle:** the driver runs as one more `g.Go` member of the existing serve errgroup (`serve.go:327-339`) — `g.Go(func() error { return - poller.Run(gctx) })` — so it inherits exactly the scoped lifecycle the + driver.Run(gctx) })` — so it inherits exactly the scoped lifecycle the doors have: cancelled on SIGINT/SIGTERM via the `signal.NotifyContext` ctx (`main.go:146-162`), first-error-wins, drained with everything else. No bespoke goroutine + done-channel machinery. @@ -273,18 +559,16 @@ Mirrors the two established precedents exactly: `x-ratelimit-*` budget gate ahead of each call. Hand-rolling makes the budget/ETag path the PRIMARY path, not a retrofit around a library's transport. Not re-opened here; recorded as a Global Constraint. -- **(b) Fold this poller into the DL-053 subscription poller — the - load-bearing fork, PARKED as OQ-A.** DL-053 (`DECISIONS.md:76`) freezes a - Postgres-backed FETCH/DELIVERY-cursor conditional-poll subscription model - for agent notification delivery, and DL-129 (`DECISIONS.md:173`) names "the - DL-053 poll" as the path tracker status ingests through. This record - designs a DISTINCT, simpler board-ingestion loop (Option 1). OQ-A now - carries THREE readings — Option 1 (distinct, in-memory cursor), Option 1.5 - (distinct, durable Postgres fetch-cursor from day one), Option 2 (build on - DL-053 machinery) — because tracker status rides the SAME `/issues` rows - this loop fetches, so PR-C on a separate poll double-fetches one endpoint. - See OQ-A for the three options, the double-fetch consequence, and the - recommendation. +- **(b) A distinct, simpler board-ingestion loop with an in-memory ETag cache + (the pre-ruling Option 1 / Option 1.5) — CLOSED by Matt's OQ-A ruling, + 2026-08-07.** Option 1 left the DL-053 convergence asserted-not-designed + (a per-LIST-PAGE in-memory ETag is not a per-artifact cursor behind a store + seam — a later merge would be a fetch-layer rewrite) and left the PR-C + double-fetch of `/issues` for a future decision; Option 1.5 made the cursor + durable but still deferred the subscriber model. Matt ruled Option 2: build + on the DL-053 machinery from day one so PR-C lands on one poll path and no + convergence refactor is ever needed. This record is that build. See Open + Questions → OQ-A (resolved). - **(c) Webhooks instead of polling — deferred by DL-053 itself** ("change-detected by conditional polling in v1 (webhooks are an additive accelerator)", `DECISIONS.md:76`). Same posture applies here: a webhook @@ -295,51 +579,92 @@ Mirrors the two established precedents exactly: signal wiring only (`main.go:41-173`); everything with a lifecycle runs under `Serve`'s scoped group (`serve.go:321-339`). A cmd-level goroutine would duplicate drain/first-error semantics the group already audits. -- **(e) Per-repo goroutines in the poller — rejected.** One forge, one rate +- **(e) Per-repo goroutines in the driver — rejected.** One forge, one rate budget; sequential per-repo fetches serialize naturally against - `x-ratelimit-*` and keep the poller a trivially testable single loop. -- **(f) `since`/`updated` incremental sync — deferred to OQ-A, ETag/304 alone - in v1.** A `since`-watermark would shrink a CHANGED tick (not just a - no-change one), but advancing it correctly requires observing the SINK - outcome, which an in-client watermark cannot do (`forge.Issue` carries no - timestamp and the client's sole seam is `ListIssues`, `provider.go:37-53`); - advancing on fetch success alone strands un-sunk issues on a mid-pass sink - failure and voids the re-poll idempotency invariant (Global Constraints → - Idempotency). v1 therefore uses ETag/304 alone — idempotency-safe, and - enough to make a quiet tick free. The correct home for an incremental cursor - is a DURABLE, poller-owned watermark that advances only on full-pass success - (OQ-A Option 1.5); it is priced there, not adopted here. + `x-ratelimit-*` and keep the driver a trivially testable single loop. +- **(f) `since`/`updated` incremental sync — still deferred, but its correct + home now EXISTS.** A `since` watermark would shrink a CHANGED tick (ETag/304 + only makes the no-change tick free; a changed page pays a full paid refetch + because GitHub's default `created`-desc order shifts page boundaries on + insert). The pre-ruling record could not adopt it: an in-client watermark + cannot observe the sink outcome, so advancing on fetch success strands + un-sunk issues on a mid-pass sink failure (Global Constraints → + Idempotency). Under Option 2 the driver owns a DURABLE, sink-gated cursor — + a `since` watermark advanced only on full-walk success is now a small + additive column on `forge_list_cursors`, correct by the same invariant. It + is still NOT in this slice (v1 cost is bounded and budget-gated without + it); it is recorded as the first cheap optimization the durable cursor + unlocks, adoptable without design change. +- **(g) Widening `forge_subscriptions` to hold the board as a row (nullable + `agent_account_id` + a subscriber-kind discriminator) — recommended + against; carried as the OQ-C alternative.** It would make "the board is a + subscriber" literal at the price of amending DL-053's spec'd shape + (`agent_account_id TEXT NOT NULL REFERENCES agent_accounts`, + `compass-server-ownership-layer/design.md:980`) and forcing every future + subscription query through a kind filter — for a subscriber that needs + neither `delivered_revision` (its delivery is the in-pass sink) nor + Subscribe/Unsubscribe dynamics (its set is operator config). See OQ-C. ## Plan ### T1 — `forge.GitHub`: the hand-rolled net/http GitHub read client -New file `go/internal/forge/github.go` (+ `github_test.go`). Implements the +New file `go/internal/forge/github.go` (+ `github_test.go`). Carried over from +the pre-ruling cut with ONE structural change: the client is stateless about +cursors — the page-level primitive takes the caller's ETag and reports +`NotModified`; no in-memory ETag/response/Link cache exists. Implements the request lifecycle from the Approach: bearer auth via `TokenSource`, -ETag conditional requests -with a per-URL response+`Link`-chain cache, `x-ratelimit-*` budget gate with a -typed budget error, `Link`-header pagination at `per_page=100`, `IssueFilter` -→ query-param mapping (empty `State` → `state=all`), `pull_request`-row -exclusion, non-2xx → `*forge.StatusError`. Raw bodies pass through untouched -(`provider.go:42-44`). No `Provider` interface assertion — read half only. +conditional GET per page, `x-ratelimit-*` budget gate with a typed budget +error + the 403 disambiguation rule, `per_page=100` + `Link`-header `HasNext` +detection, `IssueFilter` → query-param mapping (empty `State` → `state=all`), +`pull_request`-row exclusion, non-2xx → `*forge.StatusError`. Raw bodies pass +through untouched (`provider.go:42-44`). No `Provider` interface assertion — +read half only. One additive field: T1 widens `forge.Issue` +(`provider.go:37-53`) with `UpdatedAt time.Time`, parsed from the LIST row's +`updated_at`, so PR-C's DL-129 recency guard reads it off this same fetch +(Approach → one-fetch-path guarantee); the board sink ignores it. Interfaces: ```go -// TokenSource yields the current forge token; called per fetch pass so a -// rotated server_only secret takes effect without restart. -type TokenSource func(ctx context.Context) (string, error) +// TokenSource yields the current forge token and lets the client drop a cached +// value when it observes an auth failure, so the next batch re-resolves. Token +// is called per fetch batch (not per request) so a rotated server_only secret +// takes effect without restart; the client calls Invalidate on a 401 / +// bad-creds-403 (Approach step 3). The T4 implementation is a TTL cache over +// the SpecResolver whose Invalidate drops the cached value (a bare func would +// have no seam to observe the auth failure through). +type TokenSource interface { + Token(ctx context.Context) (string, error) + Invalidate() +} // GitHubConfig configures a GitHub read client. type GitHubConfig struct { - Host string // "github.com" or a GHES host; API base derives from it - Token TokenSource // required + Host string // "github.com" or a GHES host; API base derives from it + Token TokenSource // required Client *http.Client // nil -> a default client with a sane timeout } func NewGitHub(cfg GitHubConfig) *GitHub -// Satisfies ingest.forgeReader (ingest.go:23-25) structurally. +// ListPage is one conditional page fetch result. On NotModified, Issues is +// nil and ETag/HasNext are zero — the caller's stored cursor row remains the +// truth for both. +type ListPage struct { + Issues []Issue + ETag string // the response ETag to store on sink success + HasNext bool // an RFC-5988 Link rel="next" was present + NotModified bool // 304: content unchanged vs the etag argument +} + +// ListIssuesPage fetches one page (1-based) conditionally: etag == "" is an +// unconditional fetch. The client holds NO cursor state — the caller owns it. +func (g *GitHub) ListIssuesPage(ctx context.Context, repo string, f IssueFilter, page int, etag string) (ListPage, error) + +// ListIssues is the unconditional full walk (concatenated pages), built on +// ListIssuesPage. Satisfies ingest.forgeReader (ingest.go:23-25) structurally +// and is the read half of the eventual forge.Provider (provider.go:200). func (g *GitHub) ListIssues(ctx context.Context, repo string, f IssueFilter) ([]Issue, error) // ErrBudgetExhausted is returned (wrapped) when x-ratelimit-remaining is at or @@ -348,99 +673,229 @@ var ErrBudgetExhausted = errors.New("forge: rate budget exhausted") ``` Consumes: `forge.IssueFilter` (`provider.go:182-188`), `forge.Issue` -(`provider.go:37-53`), `forge.StatusError` (`provider.go:216-227`), +(`provider.go:37-53`, widened here with `UpdatedAt`), `forge.StatusError` +(`provider.go:216-227`), `net/http`, `encoding/json` (stdlib only — OQ-6). Test cycle (unit, stubbed `http.RoundTripper`; red-first per repo convention): 1. 200 with issues JSON → parsed `[]forge.Issue` field-by-field (incl. raw - body untouched, labels, state, ForgeAccount). -2. Second call sends `If-None-Match` with the first response's ETag; a - scripted 304 returns the cached slice, zero re-parse. + body untouched, labels, state, ForgeAccount, and `UpdatedAt` parsed from + the row's `updated_at`). +2. `ListIssuesPage` with a non-empty `etag` sends `If-None-Match`; a scripted + 304 returns `NotModified: true` with nil Issues and no body parse; an + empty `etag` sends NO `If-None-Match` header. 3. `x-ratelimit-remaining: 0` on response N → call N+1 fails `errors.Is(..., ErrBudgetExhausted)` WITHOUT issuing a request (assert via RoundTripper call count); after scripted reset, calls resume. -4. Two-page `Link` pagination concatenates; `per_page=100` asserted on the - request. +4. `HasNext` is true exactly when a `Link: rel="next"` header is present; + `per_page=100` and the requested `page` land on the wire; `ListIssues` + concatenates a scripted two-page walk in order. 5. Empty `IssueFilter.State` → `state=all` on the wire; labels join correctly. 6. A `pull_request`-keyed row is dropped. 7. 404/500, and a 403 WITHOUT rate-limit headers (a bad-credentials/permission `message`) → `errors.As(..., *forge.StatusError)` with matching Status and - the GitHub `message` field, AND the client signals a token re-resolve (the - 401/bad-creds-403 discriminator of step 3 — asserted on the ABSENCE of - `retry-after`/zeroed `x-ratelimit-remaining`, so a rate-limit 403 does NOT - take this path). + the GitHub `message` field, AND the client calls `Invalidate()` on a fake + `TokenSource` exactly once (the 401/bad-creds-403 discriminator of Approach + step 3 — asserted on the ABSENCE of `retry-after`/zeroed + `x-ratelimit-remaining`, so a rate-limit 403 does NOT call `Invalidate`). 8. Token from `TokenSource` lands as `Authorization: Bearer …`; a TokenSource error propagates without a request. -9. Multi-page 304: a two-page list is fetched, then a scripted 304 on page 1 - returns the cached slice AND walks the cached `Link` chain to page 2's - cached slice — a repo of >100 issues is served entirely from cache on an - unchanged pass (the page-chain-caching correctness hole). -10. `403`/`429` with `retry-after` (or a zeroed `x-ratelimit-remaining` on the - prior response) → next call fails `errors.Is(..., ErrBudgetExhausted)` - without issuing a request (RoundTripper call count asserted). -11. Absent/malformed `x-ratelimit-*` headers do not wedge the gate — the +9. `403`/`429` with `retry-after` (or a zeroed `x-ratelimit-remaining` on the + prior response) → next call fails `errors.Is(..., ErrBudgetExhausted)` + without issuing a request (RoundTripper call count asserted). +10. Absent/malformed `x-ratelimit-*` headers do not wedge the gate — the client proceeds (treats unknown budget as available) rather than permanently skipping. -12. ctx cancellation mid-pagination (cancel after page 1) stops promptly and - returns `ctx.Err()`, not a partial concatenation. +11. ctx cancellation mid-`ListIssues`-walk (cancel after page 1) stops + promptly and returns `ctx.Err()`, not a partial concatenation. +12. Reverify against current GitHub REST docs that a 304 is not charged + against the core rate limit; record the doc URL + date in the client's + doc comment (the budget math depends on it). + +### T2 — migration `0015_forge_subscriptions.sql` + the cursor store layer + +New migration `go/internal/store/migrations/0015_forge_subscriptions.sql` and +new file `go/internal/store/forge_cursors.go` (+ +`forge_cursors_pgtest_test.go`, build-tag gated like the sibling +`*_pgtest_test.go` suites). The migration creates the repo-LIST cursor class +`forge_list_cursors` (DDL in the Approach) unconditionally, and — if OQ-D +resolves land-now — the two DL-053-spec'd tables (`forge_subscriptions`, +`forge_artifact_cursors`, adapted from +`compass-server-ownership-layer/design.md:976-1019`, renumbered from that +record's stale `0004_forge` label, keys aligned to the 0013 issue-coordinate +convention: SMALLINT provider enum + `forge_host` in every key; the +provider-domain CHECK is OQ-D's second sub-fork). Store methods cover ONLY the +list cursor in this slice — `forge_subscriptions` / `forge_artifact_cursors` +get their store surface with their writers (the agent-notification slice / +PR-C), whenever they land. + +Interfaces: + +```go +// package store + +// ForgeListPageCursor is one durable page row of a repo's issue-LIST fetch +// cursor (the DL-053 FETCH-cursor model at repo-LIST granularity). ETag "" +// means never fetched (an unconditional GET). +type ForgeListPageCursor struct { + Provider ForgeProvider // GITHUB(1)/GITLAB(2)/FORGEJO(3); never 0 + Host string + Repo string + Page int32 // 1-based + ETag string + HasNext bool +} + +// ForgeListCursor reads every stored page row for the repo, ascending page. +// No rows is a nil slice, not an error (a never-polled repo). +func (s *Store) ForgeListCursor(ctx context.Context, provider ForgeProvider, host, repo string) ([]ForgeListPageCursor, error) -### T2 — `ingest.Poller`: the ctx-cancellable poll-loop scheduler +// UpsertForgeListCursorPage inserts-or-updates one page row (touching +// advanced_at — the last CONTENT advance, since this is called only after a +// 200+sink; an all-304 tick rewrites no row). Called by the driver ONLY after +// the page's content durably sank +// — the advance-attests-sink invariant lives in the caller; the store method +// is a plain upsert. Zero/empty coordinate fields -> ErrInvalidArgument. +func (s *Store) UpsertForgeListCursorPage(ctx context.Context, cur ForgeListPageCursor) error -New file `go/internal/ingest/poller.go` (+ `poller_test.go`). Immediate first -pass, then ticker; sequential repos; log-and-continue on per-repo error; -`nil` return on ctx cancellation; slog fields per the Approach. +// PruneForgeListCursorPages deletes the repo's page rows with page > maxPage +// (a repo whose walk shrank). maxPage < 1 -> ErrInvalidArgument. +func (s *Store) PruneForgeListCursorPages(ctx context.Context, provider ForgeProvider, host, repo string, maxPage int32) error +``` + +Consumes: the migration runner + `pgtest` harness the sibling suites use +(`store/issues_pgtest_test.go` et al.), `ErrInvalidArgument` (the store's +existing sentinel, `issues.go:103-108` pattern). + +Test cycle (pgtest, isolated schema per case like `issues_pgtest_test.go`): + +1. Migration applies cleanly from empty AND from a 0014 database (the + sequential-migration harness proves both); `forge_list_cursors` exists with + its PK/CHECKs (provider CHECK rejects 0, page CHECK rejects 0), plus the two + spec'd tables if OQ-D resolves land-now. +2. `ForgeListCursor` on a never-polled repo → nil, no error. +3. Upsert page 1 then re-upsert with a new ETag → one row, new ETag, + `advanced_at` advanced; rows come back ascending by page. +4. Two hosts / two providers with the same `repo` string do not collide (the + OQ-D coordinate rationale, proven). +5. `PruneForgeListCursorPages(maxPage=2)` on a 4-page repo leaves pages 1-2; + pruning a never-polled repo is a no-op. +6. Invalid input: zero provider / empty host / empty repo / page 0 → + `ErrInvalidArgument` on each method. +7. **(Only if OQ-D resolves land-now)** `forge_subscriptions` FK: inserting a + row with an unknown `agent_account_id` fails (RESTRICT, per the DL-053 DDL + and the 0006-established convention `0006_delivery_cursors.sql:6-9`) — the + table is writer-less this slice but its shape is proven here. If OQ-D + resolves defer, this case drops and test 1 asserts only `forge_list_cursors` + exists. + +### T3 — `ingest.Driver`: the DL-053 conditional-poll driver + +New file `go/internal/ingest/driver.go` (+ `driver_test.go`), plus one +refactor inside `ingest.go`: extract the translate+sink loop of `Ingest` +(`ingest.go:58-63`) into `IngestIssues(ctx, repo, raws)` so the driver can +feed caller-fetched page batches through the existing per-issue pipeline; +`Ingest` becomes `ListIssues` + `IngestIssues` (behavior unchanged — the +existing `ingest_test.go` suite is the regression net). Pass algorithm, +page-wise cursor advance, budget/error classification, logging per the +Approach; `nil` return on ctx cancellation. Interfaces: ```go -// PollerConfig configures the board-ingestion poll loop. -type PollerConfig struct { +// package ingest + +// IngestIssues translates + sinks a caller-fetched batch of raw issues for +// repo: the loop body of Ingest, exposed so the DL-053 driver (which fetches +// page-wise to gate cursor advance on sink success) reuses the one pipeline. +// Stops and returns the first sink error (wrapped); partial progress is fine — +// a re-poll is idempotent on the coordinate. +func (in *Ingester) IngestIssues(ctx context.Context, repo string, raws []forge.Issue) error + +// ListPageCursor is the driver's view of one durable page-cursor row. +type ListPageCursor struct { + Page int + ETag string + HasNext bool +} + +// CursorStore is the durable FETCH-cursor surface the driver needs — a narrow, +// repo-keyed structural seam (the server wiring adapts *store.Store and binds +// provider+host), keeping this package's no-store property (ingest.go:7-8). +type CursorStore interface { + ListCursor(ctx context.Context, repo string) ([]ListPageCursor, error) + UpsertListCursorPage(ctx context.Context, repo string, cur ListPageCursor) error + PruneListCursorPages(ctx context.Context, repo string, maxPage int) error +} + +// pageLister is the fetch seam: the page-level conditional read of the forge +// client (satisfied by *forge.GitHub structurally; faked in tests). +type pageLister interface { + ListIssuesPage(ctx context.Context, repo string, f forge.IssueFilter, page int, etag string) (forge.ListPage, error) +} + +// DriverConfig configures the board-ingestion poll driver. +type DriverConfig struct { Repos []string // "owner/name", non-empty (validated by the caller) Interval time.Duration // > 0; the caller defaults it Log *slog.Logger // nil -> slog.Default() } -func NewPoller(ing *Ingester, cfg PollerConfig) *Poller +func NewDriver(client pageLister, ing *Ingester, cursors CursorStore, cfg DriverConfig) *Driver // Run polls until ctx is cancelled, then returns nil (clean shutdown). // Per-repo errors are logged and retried next tick, never returned — a // re-poll is idempotent on the coordinate (ingest.go:48-52). -func (p *Poller) Run(ctx context.Context) error +func (d *Driver) Run(ctx context.Context) error ``` -Consumes: `Ingester.Ingest(ctx, repo) error` (`ingest.go:53`), `time.Ticker`, -`log/slog`. +Consumes: `Ingester` (`ingest.go:35-46`), `forge.ListPage` / +`forge.ErrBudgetExhausted` (T1), `time.Ticker`, `log/slog`. -Test cycle (unit, fake `Ingester` deps — the package's existing -`recordingSink` pattern, `ingest_test.go:15-25`, plus `forge.FakeProvider`): +Test cycle (unit, fake `pageLister` + fake `CursorStore` + the package's +existing `recordingSink` pattern, `ingest_test.go:13-27`): 1. `Run` performs an immediate pass for every repo before the first tick (short interval, assert call order/coverage). 2. ctx cancel BETWEEN ticks → `Run` returns `nil` promptly, and ctx cancel - DURING a pass (mid-`Ingest`, before all repos are done) also returns `nil` - promptly — deadline-gated, never a sleep (the pgtest convention, - `service_board_pgtest_test.go:15`). Assert `Run` does NOT return while ctx - is live (it has no non-cancel exit). -3. A scripted `Ingest` failure on repo A does not stop repo B in the same - pass, and repo A is retried on the next tick. -4. Interval respected: N ticks in a bounded window drive exactly N+1 passes + DURING a pass also returns `nil` promptly — deadline-gated, never a sleep + (the pgtest convention, `service_board_pgtest_test.go:15`). Assert `Run` + does NOT return while ctx is live (it has no non-cancel exit). +3. Conditional walk: stored cursors `{1: e1 hasNext, 2: e2}` → page 1 fetched + with `etag=e1`; a scripted 304 on page 1 still walks to page 2 (the + stored `HasNext`, not the response, carries the chain); an all-304 pass + sinks nothing and upserts nothing. +4. **Cursor advance gated on sink success (defends the Idempotency + invariant):** a 200 page of N issues whose sink fails at issue k → + `UpsertListCursorPage` is NOT called for that page and the pass aborts for + that repo; next tick re-fetches the page with the OLD etag and every issue + k..N sinks — none stranded. On full-page sink success the upsert carries + the response ETag + HasNext. +5. Prune: a walk that ends at page 2 after cursors held 4 pages calls + `PruneListCursorPages(repo, 2)`; an aborted (error/budget) pass never + prunes. +6. A scripted failure on repo A does not stop repo B in the same pass, and + repo A is retried on the next tick. +7. Budget skip: `ErrBudgetExhausted` from the fetch → Warn (not Error), pass + abandoned, pages already advanced this pass stay advanced. +8. Interval respected: N ticks in a bounded window drive exactly N+1 passes (the +1 is the immediate pass); no busy-loop. -5. Log fields: capture a `slog` test handler and assert a happy pass logs at - Info with `repo`/`issues`/`dur`, a budget-skip logs at Warn, and a genuine - `Ingest` error logs at Error with `err`. -6. Idempotency under mid-pass sink failure (defends the restored invariant): - a fake returns N issues but the sink fails on issue k on the first pass; on - the next tick the poller RE-FETCHES the full set (no `since` skip) and every - issue k..N sinks — none is stranded. Asserts the invariant the v1 - no-`since` decision preserves. +9. Log fields: capture a `slog` test handler and assert a happy pass logs at + Info with `repo`/`issues`/`pages`/`dur`, a budget-skip logs at Warn, and a + genuine error logs at Error with `err`. +10. `IngestIssues` refactor regression: `Ingest` still fetches-then-sinks + identically (existing `ingest_test.go` cases stay green unmodified), and + `IngestIssues` on a batch stops at the first sink error with the same + wrapped error shape (`ingest.go:60-62`). -### T3 — serve boot wiring + config surface + secret resolve +### T4 — serve boot wiring + config surface + secret resolve Touches `server/serve.go`, `cmd/compass-server/main.go` (edits only, no new files beyond tests). `ServeConfig.Forge` all-optional like `S3`; CLI flags/env per the Approach; fail-fast on forge-config-present but -secret-unresolvable; poller under the existing errgroup. +secret-unresolvable; the `ingest.CursorStore` adapter over the open +`*store.Store`; driver under the existing errgroup. Interfaces: @@ -449,141 +904,210 @@ Interfaces: // All-optional: empty Repos leaves the driver off (today's behavior). type ForgeConfig struct { Host string // default "github.com" - Repos []string // "owner/name"; empty -> no poller + Repos []string // "owner/name"; empty -> no driver SecretName string // declared server_only secret name; default "GITHUB_FORGE_TOKEN" PollInterval time.Duration // default time.Minute } // ServeConfig gains: Forge ForgeConfig + +// unexported in server/: the ingest.CursorStore adapter binding the driver's +// forge coordinate half, so the ingest-side seam stays repo-keyed. +type forgeCursorStore struct { + st *store.Store + provider store.ForgeProvider + host string +} +// satisfies ingest.CursorStore: ListCursor / UpsertListCursorPage / +// PruneListCursorPages, each delegating to the T2 store methods with the +// bound (provider, host). ``` Consumes: `secrets.SpecResolver.Resolve(ctx, reason) ([]ResolvedSecret, error)` -(`secrets/resolver.go:139`), `secrets.ResolvedSecret{Name, Value, …}` -(`secrets/secrets.go:128-144`; Value redacted under all fmt verbs — safe to -thread), `board.NewIssueProjection` instance from `serve.go:259`, -`ingest.NewIngester` (`ingest.go:44`), `forge.NewGitHub` (T1), -`ingest.NewPoller`/`Run` (T2), the errgroup at `serve.go:327`, +(`secrets/resolver.go:135`), `secrets.ResolvedSecret{Name, Value, …}` +(`secrets/secrets.go:132-135`; Value redacted under all fmt verbs — safe to +thread), the `board.NewIssueProjection` instance from `serve.go:259`, +`ingest.NewIngester` (`ingest.go:44`), `forge.NewGitHub` (T1), the T2 store +methods, `ingest.NewDriver`/`Run` (T3), the errgroup at `serve.go:327`, `compassv1.ForgeProvider_FORGE_PROVIDER_GITHUB` (`gen/compass/v1/compass.pb.go:449`). Produces: a `TokenSource` closure over the resolver that re-resolves + selects `SecretName` on TTL expiry or auth failure (not per pass; see the -Secret-resolve bullet); boot-time validation error text for missing-secret / -bad-repo-format / non-positive interval. +Secret-resolve bullet); the cursor-store adapter; boot-time validation error +text for missing-secret / bad-repo-format / non-positive interval. Test cycle: 1. Unit: CLI flag/env precedence for the four forge knobs mirrors the S3 pattern (table test like `resolveNetworkDoor`'s, `main.go:184-212`); repo-format validation (`owner/name`) rejects garbage at startup. -2. Unit: empty `Forge.Repos` → `Serve` builds no poller (assert via config +2. Unit: empty `Forge.Repos` → `Serve` builds no driver (assert via config plumbing seam, no behavioral change to existing serve tests — the whole - existing suite is the regression net). + existing suite is the regression net; the 0015 migration alone must not + change any existing test's outcome). 3. pgtest (build-tag gated like the board suite): a `Serve`-assembled - Ingester over `forge.FakeProvider` scripted with one issue sinks to the - real projection and lands in `ListBoardIssues` — the end-to-end - boot-wiring proof without touching live GitHub. (The fake satisfies the - same structural `forgeReader`, `ingest.go:20-22`.) -4. pgtest (the no-clobber contract's runtime proof — the test MUST be able to + pipeline over a fake pager scripted with one issue page sinks to the real + projection, lands in `ListBoardIssues`, AND leaves a + `forge_list_cursors` page-1 row carrying the scripted ETag — the + end-to-end boot-wiring + durable-cursor proof without touching live + GitHub. +4. pgtest (restart-resync proof — the durable cursor's headline buy): after + test 3, rebuild the pipeline over the SAME schema (a "restart") with the + pager scripting a 304 for the stored ETag; assert the pass issues no + unconditional fetch, sinks nothing, and the projection still serves the + issue (Rehydrate covers the read side, `serve.go:259-263`). +5. pgtest (the no-clobber contract's runtime proof — the test MUST be able to go RED): (1) ingest/create the issue via the projection, (2) set a non-default lifecycle `State` through the part-5 write path (`store.SetIssueState`, `issues.go:140`), (3) re-ingest a forge UPDATE for - the same coordinate (forge_state open→closed) via the fake, (4) assert - `ForgeState` is updated AND `State` is unchanged from the human-set value - (`issue_projection.go:207`, `protoToForgeFields` writes `ForgeState` but - never `State`). A regression adding `state = EXCLUDED.state` to the - `UpsertIssueForgeFields` SET clause (`issues.go:122-125`) MUST turn this + the same coordinate (forge_state open→closed) via the fake pager, (4) + assert `ForgeState` is updated AND `State` is unchanged from the human-set + value (`issue_projection.go:208`, `protoToForgeFields` writes `ForgeState` + but never `State`). A regression adding `state = EXCLUDED.state` to the + `UpsertIssueForgeFields` SET clause (`issues.go:121-125`) MUST turn this red — the as-worded "State left untouched" over a zero-value row would stay green vacuously, so the human-set baseline in step 2 is load-bearing. -5. Unit: the `TokenSource` re-resolves after its TTL and on a scripted auth - failure, and a changed resolver value is used on the next fetch — the - design's stated reason for a resolver closure rather than a captured token. -6. Startup failure: forge config present + secret name absent from the - resolved set → `Serve` returns a descriptive error, following the +6. Unit: the T4 `TokenSource` re-resolves after its TTL, and calling its + `Invalidate()` drops the cached value so the next `Token` re-resolves and + the changed resolver value is used on the next fetch — the design's stated + reason for a TTL-cache `TokenSource` with an invalidation seam rather than a + captured token or a bare func. +7. Startup failure, two distinct texts: (a) forge config present + secret NAME + absent from the resolved set → `Serve` returns `"forge secret %q not + declared"`; (b) the resolve call itself ERRORS at boot → `Serve` returns + `"forge secret resolve failed at startup: %w"`. Both follow the listener-cleanup path the `Rehydrate` failure uses (`serve.go:260-263`), - since the resolver is built after the UDS listener binds (`serve.go:287`). + since the resolver is built after the UDS listener binds (`serve.go:287`); + assert the two error strings are distinguishable so a permanent misconfig is + not confused with a transient provider outage. ## Tasks - [ ] **T1** — `forge.GitHub` read client (`github.go` + unit tests over a - stubbed RoundTripper: 200/parse, ETag/304, budget gate, pagination, - filter mapping incl. `state=all` default, PR exclusion, StatusError - mapping, bearer auth). -- [ ] **T2** — `ingest.Poller` scheduler (`poller.go` + unit tests: immediate - pass, ctx cancel → nil, per-repo error isolation + retry, interval - discipline, slog fields). -- [ ] **T3** — serve boot wiring: `ServeConfig.Forge`, CLI flags/env, DL-052 - secret resolve via the existing `SpecResolver`, errgroup membership, - fail-fast validation; unit + pgtest end-to-end over `FakeProvider`. + stubbed RoundTripper: 200/parse incl. `UpdatedAt`, conditional + `ListIssuesPage`/304, budget gate + 403 disambiguation + `TokenSource. + Invalidate` on bad-creds, `HasNext`/pagination, filter mapping incl. + `state=all` default, PR exclusion, StatusError mapping, bearer auth, + 304-uncharged doc reverify). +- [ ] **T2** — migration `0015_forge_subscriptions.sql` (`forge_list_cursors` + always; `forge_subscriptions` + `forge_artifact_cursors` per DL-053, + coordinate-aligned, IF OQ-D resolves land-now) + `store/forge_cursors.go` + list-cursor methods; pgtest-covered. +- [ ] **T3** — `ingest.Driver` conditional-poll driver (`driver.go` + the + `IngestIssues` extraction in `ingest.go`; unit tests: immediate pass, ctx + cancel → nil, conditional walk over stored cursors, sink-gated cursor + advance, prune, per-repo error isolation, budget-skip classification, + interval discipline, slog fields). +- [ ] **T4** — serve boot wiring: `ServeConfig.Forge`, CLI flags/env, DL-052 + secret resolve via the existing `SpecResolver`, the `ingest.CursorStore` + adapter over `*store.Store`, errgroup membership, fail-fast validation; + unit + pgtest end-to-end incl. the durable-cursor restart proof and the + no-clobber proof. ## Open Questions -### OQ-A (LOAD-BEARING — blocks the freeze): relationship to DL-053 - -The SEA-1810 issue body scopes this slice as the *simple board-ingestion -poll* (ticker → `Ingester.Ingest` → `IssueProjection` sink), explicitly -distinct from the DL-053 agent-notification subscription poller. But DL-053 -(`DECISIONS.md:76`) and DL-129 (`DECISIONS.md:173`) freeze a Postgres-backed -FETCH/DELIVERY-cursor conditional-poll subscription model as THE way tracker -status is ingested ("through the reverse `TrackerStatusMapping` (DL-053 -poll; echo-suppressed…)"). This slice does NOT ingest tracker STATUS today — -`protoToForgeFields` (`issue_projection.go:208-219`) writes `ForgeState` but -never `State`, and the `:207` guarantee ("the forge-only upsert cannot -clobber a human-set state") means board ingestion and status ingestion are -cleanly separable RIGHT NOW. So Option 1 is not a contradiction of DL-129 -today. The fork is about what happens when PR-C (tracker-status ingestion, -DL-129) lands: tracker status is a field on the SAME `/issues` rows this loop -already fetches (`forge.Issue.State`, `provider.go:46-47`), so PR-C on a -separate DL-053 poll would **re-fetch the identical endpoint** — double -rate-budget spend against one shared limit, the exact waste alternative (e) -rejects — UNLESS DL-129/DL-053 is amended so both ingestions share this -loop's fetch. That collision is the sharp edge Matt should rule on. Three -readings: - -- **Option 1 (this record's stated assumption — recommended):** the - board-ingestion poll is a DISTINCT, simpler loop that sinks to the - projection; it is NOT the DL-053 per-subscriber delivery machinery. - Per-subscriber DELIVERY cursors and the `Sessions` → `AgentGateway.Control` - push path are pure overhead for a projection writer with exactly one, - always-on consumer. **Unpriced cost, now stated:** the convergence with - DL-053's fetch half is asserted, not designed — and it is not a free seam - swap. DL-053's FETCH cursor is per-ARTIFACT; this client's cache is a - per-LIST-PAGE ETag. Different granularities: a list-page ETag cannot become - a per-artifact cursor by moving it behind a store seam, so a real merge is - a rewrite of the fetch layer, not a relocation. Plus the PR-C double-fetch - above is left for a future decision. -- **Option 1.5 (distinct loop, durable cursor from day one — the middle - path):** keep Option 1's distinct, simple loop and its projection sink, but - persist the fetch watermark (the `since` timestamp / page ETags) in a small - Postgres table instead of T1's in-memory map. Cost: one migration + a tiny - store method (T1 gains a cursor-store dependency; T2/T3 unchanged). Buys: - no full `state=all` resync on every restart, survives a future multi-replica - server, and makes the "shared fetch primitive" real rather than aspirational - — a durable per-repo fetch cursor IS DL-053's FETCH-cursor idea in embryo, - so a later convergence is an extension, not a rewrite. Still does NOT take - on per-subscriber delivery. Recommended IF Matt wants the convergence story - to be load-bearing; declined if in-memory-and-resync is acceptable for v1. -- **Option 2:** build this driver ON the DL-053 subscription machinery from - day one (the board as a distinguished subscriber), so tracker-status - ingestion (PR-C, per DL-129) lands on one poll path and no convergence - refactor is ever needed, AND the double-fetch never arises. Cost: the - DL-053 Postgres rows/cursors/delivery plumbing does not exist yet — this - slice would inherit that entire build as a prerequisite, and SEA-1810's - demo-facing scope (issues visibly on the board) would gate on it. - -**Recommendation: Option 1** for fastest demo-facing value, with **Option 1.5 -as the hedge** if the convergence with PR-C/DL-053 must be designed now rather -than deferred. If Matt rules Option 2, T2/T3 are re-cut (T1 survives nearly -intact — the client is the shared fetch half either way). The PR-C -double-fetch is the concrete thing each option resolves differently. -**A consequence surfaced by review that sharpens the fork:** a `since`/`updated` -incremental fetch cannot be done correctly in an in-client watermark (it -cannot observe the sink outcome, so it strands issues on a mid-pass sink -failure — Global Constraints → Idempotency), so v1 Option 1 uses ETag/304 -ONLY and a changed tick pays a full refetch of its changed pages. Option 1.5's -durable, poller-owned cursor is the ONLY place a correct incremental `since` -lives — so if the changed-tick cost or the resync-on-restart matters, that is -an argument FOR 1.5 now rather than deferring it. PARKED for Matt; the freeze -waits on this ruling. +### OQ-A — RESOLVED (Matt, 2026-08-07): Option 2 + +Matt ruled **Option 2**: this driver is built ON the DL-053 subscription +machinery from day one — the board as a distinguished DL-053 subscriber over +the per-artifact FETCH-cursor model's durable Postgres cursors — so +tracker-status ingestion (PR-C, per DL-129) lands on one poll path, no +convergence refactor is ever needed, and the PR-C `/issues` double-fetch never +arises. The T1 GitHub read client survives the ruling nearly intact (it is the +shared fetch half either way; its one structural change — cursor state moves +out of the client into `forge_list_cursors` — is the Approach's stateless-client +bullet). The superseded Option 1 / Option 1.5 framings and their tradeoffs are +retained in Alternatives (b) for the reasoning trail; the fork is closed and +is not relitigated here. The follow-on shape calls INSIDE Option 2 are OQ-C +and OQ-D below. + +### OQ-C (LOAD-BEARING — blocks the freeze): how the board is represented as a subscriber + +DL-053's subscription row is per-artifact and agent-owned +(`agent_account_id TEXT NOT NULL REFERENCES agent_accounts`, +`compass-server-ownership-layer/design.md:980`); the board subscribes to whole +repos and is not an agent. Two representations: + +- **Config-declared (RECOMMENDED, and what this record designs):** the + board's subscription set IS `ForgeConfig.Repos` — operator config, no + `forge_subscriptions` rows, no delivery cursor (the board's notify is the + synchronous in-pass projection sink, so sunk-ness is already encoded by the + sink-gated `forge_list_cursors` advance; a `delivered_revision` for it + would duplicate what the `issues` table already records). The repo-LIST + fetch state lives in the new sibling table `forge_list_cursors` (Approach); + DL-053's two spec'd tables land untouched in shape and gain their writers + in the later slices. Cost: when agent subscriptions arrive, the poll driver + enumerates targets from two sources (config repos for the LIST class, + subscription rows for the per-artifact class) — acceptable because the two + classes hit different endpoints with different cursor tables either way, + and both share the one client, budget gate, and pass scheduler. +- **A widened subscription row (Alternatives (g)):** nullable + `agent_account_id` + a subscriber-kind discriminator + a repo-level + artifact kind, making the board a literal `forge_subscriptions` row. Buys a + single target-enumeration query; costs an amendment to DL-053's spec'd + shape and a kind-filter on every future subscription query, for a + subscriber with no delivery semantics and no Subscribe/Unsubscribe + dynamics. + +**Recommendation: config-declared.** It consumes DL-053 without amending it +(the brief the ruling set), and the one-fetch-path property PR-C needs holds +identically under both (it hangs on `forge_list_cursors` + the shared pass, +not on how the board's membership is stored). PARKED for Matt; batched by the +main agent. + +### OQ-D (LOAD-BEARING — blocks the freeze): whether (and how) the two spec'd DL-053 tables land in this slice + +The coordinate alignment itself — SMALLINT provider enum + `forge_host` in +every key, matching the tree's frozen 0013 issue coordinate +(`0013_issues.sql:29-35`) rather than the ownership-layer DDL's `provider +TEXT` + host-less key (`compass-server-ownership-layer/design.md:976-1019`) — +is NOT the fork: it is the mechanical application of the tree's existing +standard, author-decided (Approach → author-decided), because diverging would +fork the coordinate vocabulary inside one schema and force casts/joins between +cursor and issue rows. Two things about it ARE genuine forks: + +**D1 — land the two spec'd tables now, or defer them to their first writers.** +Nothing this slice EXECUTES touches `forge_subscriptions` or +`forge_artifact_cursors`: the driver rides `forge_list_cursors` only (Approach +→ the DL-053 tables). So they land now purely as anticipatory schema. + +- **Land-now (what this record designs):** one forge migration; the DL-053 + shape is frozen and pgtest-covered (T2 test 7) before its writers exist, + matching the ruling's "build on the DL-053 machinery from day one" read as + including the schema. Cost: it forces the D2 provider-domain call now. +- **Defer:** `0015` lands `forge_list_cursors` alone; each spec'd table rides + the slice that first writes it (PR-C / agent-notification) with more + information. An EMPTY table is the cheapest thing to `ALTER`, so freezing its + shape early buys little, and deferring dodges D2 entirely until a writer + needs it. The driver is byte-identical either way (it never touches those + tables). Cost: a second forge migration two slices out. + +**Recommendation: land-now** — it is the most direct reading of the day-one +ruling and one migration is tidier, but the buy is genuinely small, so this is +a real call for Matt, not an author default dressed as one. + +**D2 — IF land-now, which provider domain the CHECK admits (moot if D1 = +defer).** DL-053's spec'd DDL keys `provider TEXT -- "github" | "linear"` +(`compass-server-ownership-layer/design.md:981`) — Linear is a first-class +provider of subscription/cursor rows in the frozen spec. Adopting the 0013 +coordinate's `SMALLINT CHECK (forge_provider IN (1, 2, 3))` +(GitHub/GitLab/Forgejo, `0013_issues.sql:33`) consciously NARROWS that domain: +it drops `linear` until the `ForgeProvider` enum and the CHECK grow. This is +NOT "no semantic change" (the pre-fold text's claim, now corrected). DL-129 +routes tracker-status ingestion through "the DL-053 poll" (`DECISIONS.md:173`) +and the MVP tracker is GitHub, so the narrowing is likely harmless for now — +but it is a real domain decision, cheap to make correctly while the tables are +empty and expensive to discover wrong once a Linear tracker arrives. + +- **Narrow to {github, gitlab, forgejo} (RECOMMENDED):** one coordinate + vocabulary across `issues` and the forge tables; a Linear tracker later + extends the enum + CHECK (a one-line migration on empty-or-small tables). +- **Widen the CHECK to include a `linear` enum value now:** honors the DL-053 + spec's stated domain literally, at the cost of introducing an enum value + with no producer this slice or the next. + +PARKED for Matt (both sub-forks); batched by the main agent. If D1 = defer, +D2 does not need ruling now. ### OQ-B (non-load-bearing deferral): live-demo content From 90bda0233152afe34393c748151c18c81e07e3a6 Mon Sep 17 00:00:00 2001 From: seal Date: Fri, 7 Aug 2026 15:39:08 -0400 Subject: [PATCH 4/5] docs(designs): fold forge-poll red-team + freeze (Status: Active + DL-161/162/163) Fold the 10 design-critic findings into the SEA-1810 forge-poll driver record (sound-with-folds verdict) and flip it to Active with its ledger delta, making the design PR merge-ready: - F1: seed reconcile ON CONFLICT DO NOTHING (bootstrap-only insert; the table is authoritative after first insert; a soft-disabled row stays disabled across restarts), DO-UPDATE weighed + rejected in Alternatives. - F2: one boot slog.Warn when polling is disabled but enabled target rows exist for the bound (provider, host); never fail-fast. - F3: justify the deferred issues-CHECK widening (OQ-E) vs the Go ForgeProviderLinear constant closed now. - F4/F5: GitHub repo-string lowercase normalization at the seed boundary; seed rows keyed under --forge-host (host change abandons prior rows). - F6: rename the driver store interface CursorStore -> PollStore. - F7/F8/F10: freeze-delta names both ledger + ownership annotation; forge_artifact_cursors GC deferred with its writers; forge_repo_subscriptions gains updated_at touched on every upsert/enable-flip. - F9: T4 tests assert the seed-visible-before-first-pass ordering directly. Ledger: append DL-161 (the poll driver + forge_list_cursors FETCH-cursor), DL-162 (repo targets as forge_repo_subscriptions table rows, --forge-repos becomes a boot seed), DL-163 (0015 lands four tables, CHECK IN (1,2,3,4)). Co-authored-by: Matt Wilkinson --- docs/designs/product/DECISIONS.md | 3 + .../compass-forge-poll-driver/design.md | 854 ++++++++++++------ 2 files changed, 585 insertions(+), 272 deletions(-) diff --git a/docs/designs/product/DECISIONS.md b/docs/designs/product/DECISIONS.md index 3279ff8c..d2d94baf 100644 --- a/docs/designs/product/DECISIONS.md +++ b/docs/designs/product/DECISIONS.md @@ -88,6 +88,9 @@ check enforces the mechanical half. Full rationale: | DL-088 | The durable session log and the live client trace are two projections of ONE canonical artifact — the SDK session-entry log: the agent emits committed entries, the Server persists them AND projects the block-level trace from the same store. MVP runs two agent→server streams (durable settled-entry log + ephemeral, non-persisted live per-token trace — token streaming is mandatory); collapsing them into one token stream carrying log metadata is the post-MVP follow-up SEA-1580 | Active (Matt, 2026-07-31) | [session persistence §One canonical artifact](compass-agent-session-persistence/design.md#one-canonical-artifact-the-session-log-unified-with-the-trace-r1) | | DL-089 | The agent persists nothing DURABLE: the injected backend is a TEE — it reads and writes the container-local EPHEMERAL session file normally (so the SDK's own loader, compaction, and rewrites work) and tees each committed write upstream as a durable `TranscriptEntry` frame (append→delta, writeFull→checkpoint, awaited per-path for per-session send order; agent-stamped per-lifetime entry_seq, server-rebased to session scope). Zero S3 credentials, zero storage detail; the local file dies with the container. A definitively-erred emit buffers under a bounded cap with escalating warn→error retry and FAILS THE SESSION at cap exhaustion (resumable from the last committed prefix; cap value is tuning, not freeze-scope) | Active (Matt, 2026-07-31) | [session persistence §The SDK seam](compass-agent-session-persistence/design.md#the-sdk-seam-grounded-revised-for-the-tee) | | DL-093 | The Server's durable session-transcript store is TWO-TIER: a Postgres hot-tail (`agent_session_transcript_entries`) holding `[latest checkpoint..now]` = the resume set, per-frame durable at the `CommitConversationFrame` seam and pruned at flush; superseded entries flush to an S3-compatible object-store cold archive (verbatim JSONL segments, PUT-before-prune, indexed by an `agent_session_archive_segments` manifest) at compaction (primary), a high size-cap safety valve, and session-end. Resume reads PG-only (S3 fallback only when the safety valve fired); the archive feeds the post-MVP opt-in analytics layer. Agent+Runner hold zero storage detail; the object-store client/credential is Server-internal (endpoint-agnostic: Garage/R2/MinIO). Realigns with DL-019 | Active (Matt, 2026-07-31) | [session persistence §T4](compass-agent-session-persistence/design.md#t4-compass-server--the-two-tier-durable-transcript-store-pg-hot-tail--object-store-cold-archive) | +| DL-161 | The board-ingestion forge poll is a ctx-cancellable driver in `internal/ingest` (immediate first pass then `time.Ticker`, sequential per-repo, log-and-continue on per-repo error, `nil` on ctx cancel) that instantiates the DL-053 FETCH-cursor model at repo-LIST granularity: a new `forge_list_cursors` table (per `(provider, host, repo, page)` ETag + hasNext) whose page cursor advances ONLY after every issue on that page durably sinks, so the board holds no per-subscriber DELIVERY cursor (its notify is the synchronous in-pass projection sink). One hand-rolled `net/http` GitHub read client (OQ-6 frozen: no new dep), a fail-fast `x-ratelimit-*` budget gate (never in-client sleep), and a DL-052 `server_only` token resolved behind a short TTL. Builds the FETCH half of DL-053; PR-C (DL-129 tracker-status) and the agent-notification slice both ride it | Active (Matt, 2026-08-08) | [forge-poll driver §Approach](compass-forge-poll-driver/design.md#approach) | +| DL-162 | The board's poll targets are durable TABLE rows, not config: a new `forge_repo_subscriptions` table keyed `(forge_provider, forge_host, repo)` with an `enabled` soft-disable flag, enumerated `WHERE enabled` at the top of every pass so add/remove is a live row op needing no restart or deploy. The `--forge-repos` CLI flag becomes a declarative boot SEED reconciled into the table `ON CONFLICT DO NOTHING` (bootstrap-only insert: the table is authoritative after first insert, a soft-disabled row stays disabled across restarts, the seed never deletes/disables/re-enables an existing row); a `--forge-poll` bool runs the driver with an empty seed. The dynamic add/remove mutation RPC/admin surface is a named non-goal of this slice | Active (Matt, 2026-08-08) | [forge-poll driver §OQ-C](compass-forge-poll-driver/design.md#oq-c--resolved-matt-2026-08-08-the-boards-subscription-set-is-a-table) | +| DL-163 | Migration `0015_forge_subscriptions.sql` lands FOUR tables unconditionally — `forge_repo_subscriptions` (DL-162), `forge_list_cursors` (DL-161), and the two DL-053-spec'd tables `agent_forge_subscriptions` (DL-053's `forge_subscriptions`, RENAMED to disambiguate the per-artifact agent-owned subscription from the board's per-repo target) and `forge_artifact_cursors` (both writer-less this slice, shape frozen + pgtest-covered before their writers) — every key coordinate-aligned to the 0013 issue convention (SMALLINT provider + `forge_host`) and every provider CHECK admitting the full declared proto enum `IN (1, 2, 3, 4)` (the CHECK's job is "never UNSPECIFIED(0)", not gating rollout; rollout is gated by which `forge.Provider` has a real client — GitHub only this slice). The `issues` CHECK `IN (1, 2, 3)` widening for Linear ingestion is a separate future-slice prerequisite (OQ-E) | Active (Matt, 2026-08-08) | [forge-poll driver §OQ-D](compass-forge-poll-driver/design.md#oq-d--resolved-matt-2026-08-08-all-four-tables-land-now-checks-admit-the-full-enum) | > Note (2026-07-31, SEA-1570 R5): DL-065's retired internal `ResumeContext > resume = 12` envelope field is NOT silently reintroduced — the collapsed diff --git a/docs/designs/product/compass-forge-poll-driver/design.md b/docs/designs/product/compass-forge-poll-driver/design.md index a0b03926..f65ba1df 100644 --- a/docs/designs/product/compass-forge-poll-driver/design.md +++ b/docs/designs/product/compass-forge-poll-driver/design.md @@ -1,14 +1,19 @@ # Compass forge-poll driver (SEA-1810) -Status: Draft +Status: Active Lane: compass-server Tracker: SEA OQ-A is RESOLVED — Matt ruled **Option 2** (2026-08-07): this driver is built ON the DL-053 forge-subscription machinery from day one, with the board as a -distinguished subscriber. Freeze now waits on OQ-C and OQ-D (see Open -Questions) — two shape calls inside the Option-2 build. This record designs -the running driver that periodically drives the existing greenfield +distinguished subscriber. OQ-C and OQ-D are RESOLVED (Matt, 2026-08-08): the +board's poll targets are TABLE rows — a new `forge_repo_subscriptions` table, +dynamically add/removable, with DL-053's spec'd `forge_subscriptions` landing +RENAMED `agent_forge_subscriptions` — all four forge tables land +unconditionally in migration 0015, and every provider CHECK admits the full +declared enum `IN (1, 2, 3, 4)` (see Open Questions). Freeze now waits only +on the human design review + the review-agent pass. This record designs the +running driver that periodically drives the existing greenfield `go/internal/ingest` library against a real GitHub forge, so board issues ingested from the tracker appear live in the Compass server projection — and it instantiates the DL-053 fetch-cursor model (migration + store + poll @@ -38,10 +43,11 @@ do not exist yet (migrations in `go/internal/store/migrations/` stop at `0006_delivery_cursors.sql` table is `agent_delivery_cursors`, the comms agent-notification fan-out, "how far an agent has confirmed delivery on a channel", a different subsystem entirely). This slice therefore builds the -fetch half of that machinery: the migration, the cursor store, and the poll -driver that PR-C (tracker-status ingestion, DL-129) and the later -agent-notification slice both ride — one fetch path, no convergence refactor -ever needed. +fetch half of that machinery: the four-table migration (including the +board's own target table, `forge_repo_subscriptions`), the target + cursor +store layer, and the poll driver that PR-C (tracker-status ingestion, +DL-129) and the later agent-notification slice both ride — one fetch path, +no convergence refactor ever needed. ## Global Constraints @@ -55,18 +61,41 @@ ever needed. accelerator), delivered by account on the existing `Sessions` → `AgentGateway.Control` push path." Full text + DDL: `compass-server-ownership-layer/design.md:942-1019` (Decision 5). This - record consumes that model — it never amends it. The genuine forks in - applying it to a repo-LIST board subscriber are PARKED for Matt as OQ-C (how - the board is represented as a subscriber) and OQ-D (whether the two spec'd - tables land in this migration at all, and if so which provider domain their - CHECK admits). What is NOT a fork: the repo-LIST fetch-cursor CLASS - (`forge_list_cursors`, Approach (a)) is author-adopted as the only coherent - shape for a LIST-granularity ETag — invariant under both OQ-C options, so - ratifying either ratifies the class — and the coordinate alignment to the - tree's 0013 convention (SMALLINT provider + `forge_host`) is the mechanical - application of an existing standard. The two spec'd tables are labeled - `0004_forge` in that (older) record; the tree's migrations reach `0014`, so - if they land they land here as part of `0015_forge_subscriptions.sql`. + record consumes that model — it never amends it. The two genuine forks in + applying it to a repo-LIST board subscriber are RESOLVED (Matt, + 2026-08-08): the board's subscription set is a TABLE — the new + `forge_repo_subscriptions` (OQ-C) — and the spec'd tables land NOW with + the full provider domain (OQ-D). What was never a fork: the repo-LIST + fetch-cursor CLASS (`forge_list_cursors`, Approach (a)) is author-adopted + as the only coherent shape for a LIST-granularity ETag, and the coordinate + alignment to the tree's 0013 convention (SMALLINT provider + `forge_host`) + is the mechanical application of an existing standard. The two spec'd + tables are labeled `0004_forge` in that (older) record; the tree's + migrations reach `0014`, so they land here as part of + `0015_forge_subscriptions.sql`. +- **The DL-053 `forge_subscriptions` table lands RENAMED + `agent_forge_subscriptions` (Matt's explicit option under OQ-C):** shape + and columns unchanged from the spec'd DDL + (`compass-server-ownership-layer/design.md:978-999`) — a NAMING + clarification, not a shape amendment — disambiguating the two subscription + concepts: `agent_forge_subscriptions` is the per-ARTIFACT, agent-owned + subscription (`agent_account_id TEXT NOT NULL REFERENCES agent_accounts`, + `:980`); `forge_repo_subscriptions` is the board's per-REPO poll target + (new, this record). A future reader maps `agent_forge_subscriptions` back + to DL-053's `forge_subscriptions`; recording the rename — BOTH a + DECISIONS.md ledger row AND a one-line annotation at the ownership-layer + DDL — is a proposed freeze-time delta (see Open Questions → OQ-C), not + part of this Draft PR. +- **Provider domain `IN (1, 2, 3, 4)` on every 0015 table (OQ-D2, Matt + 2026-08-08):** the proto enum already declares all four providers — + `FORGE_PROVIDER_GITHUB = 1 … FORGE_PROVIDER_LINEAR = 4` + (`proto/compass/v1/compass.proto:699-706`; + `gen/compass/v1/compass.pb.go:448-453`). The CHECK's job is "never + UNSPECIFIED(0)", NOT gating rollout — rollout is gated by which + `forge.Provider` has a real client (GitHub only, this slice). The + pre-existing `issues` CHECK `IN (1, 2, 3)` (`0013_issues.sql:32`) excludes + Linear and is a SEPARATE prerequisite for actually ingesting Linear + issues — out of scope here, documented as OQ-E. - **DL-129 (FROZEN, Matt 2026-08-04, `DECISIONS.md:173`):** tracker native status "is ingested into the DL-070 server projection through the reverse `TrackerStatusMapping` (DL-053 poll; echo-suppressed in tracker-status @@ -119,18 +148,22 @@ ever needed. store. The proto->store mapping edge is part 4 (the sink's real implementation)" (`ingest.go:7-8`). The driver's durable-cursor dependency therefore enters `ingest` as a package-local structural interface - (`CursorStore`, T3), with the `*store.Store` adapter living in `server/` + (`PollStore`, T3), with the `*store.Store` adapter living in `server/` (T4) — the same narrow-seam pattern as `forgeReader`/`issueSink` (`ingest.go:20-33`). - **Non-goals:** the tracker-status ingestion itself (PR-C — the reverse `TrackerStatusMapping`, echo-suppression, recency guard; a separate slice that consumes the seam this record makes explicit); the tracker-status WRITE-back / outbound mirror (T3-b); the agent-notification DELIVERY half - of DL-053 (per-agent `forge_subscriptions` rows, `DetectChanges`, + of DL-053 (per-agent `agent_forge_subscriptions` rows, `DetectChanges`, per-artifact comment/check ETags, the `Sessions` → `AgentGateway.Control` push) — that slice adds subscribers and wire delivery ON the tables and - driver built here, it does not rebuild the fetch path. This record is the - BOARD-ingestion read poll plus the DL-053 fetch machinery it stands on. + driver built here, it does not rebuild the fetch path; and the dynamic + add/remove MUTATION surface (RPC or admin UI) for + `forge_repo_subscriptions` — this slice builds the table + the T4 boot + seed reconcile as its only writer; the mutation surface rides a later + slice. This record is the BOARD-ingestion read poll plus the DL-053 fetch + machinery it stands on. ## Approach @@ -166,12 +199,15 @@ framings were weighed: the page-cursor advance (Global Constraints → Idempotency) and no `delivered_revision` row is needed — exactly the per-subscriber cursor DL-053 splits out for subscribers whose notify can fail independently of - the fetch, which the board's cannot (blessing this reading is OQ-C). + the fetch, which the board's cannot (OQ-C resolved this reading: the board + holds no delivery cursor and no per-artifact subscription row; its + repo-level poll targets are `forge_repo_subscriptions` rows — the table + model, Approach → the four forge tables). - **(b) The board subscribes to each issue individually, seeded by a repo - enumeration — REJECTED.** N `forge_subscriptions` rows per repo, and the - enumeration that seeds them IS a repo LIST fetch — so the LIST (and its - paging/ETag problem) remains, plus a per-issue row-churn loop on top. It - removes nothing and adds a reconcile. + enumeration — REJECTED.** N per-artifact `agent_forge_subscriptions` rows + per repo, and the enumeration that seeds them IS a repo LIST fetch — so the + LIST (and its paging/ETag problem) remains, plus a per-issue row-churn loop + on top. It removes nothing and adds a reconcile. - **(c) The LIST fetch feeds per-artifact state as it enumerates — ADOPTED as the sharing seam.** Every `/issues` LIST row carries the full issue payload including its forge state (`forge.Issue.State`, `provider.go:45-46`), so @@ -203,60 +239,129 @@ T1; `state_reason` similarly, if the reopen mapping wants it). "One fetch path" is exact; "adds literally nothing" would not be, so the claim is stated as the former. -### The DL-053 tables (`0015_forge_subscriptions.sql`) - -This record's DESIGN lands all three tables in one migration — the two -DL-053-spec'd tables plus the repo-LIST cursor class — adapted from the -ownership-layer DDL (`compass-server-ownership-layer/design.md:976-1019`, there -labeled "`0004_forge`"; the label predates the current migration count, so it -lands as `0015`). Two calls in that adaptation are author-decided (mechanical -application of an existing tree standard); one is a genuine fork PARKED for -Matt as OQ-D. +### The four forge tables (`0015_forge_subscriptions.sql`) + +Migration 0015 lands FOUR tables unconditionally (OQ-D1, RESOLVED land-now, +Matt 2026-08-08): the board's repo-level target set +(`forge_repo_subscriptions`, new — OQ-C's table model), the repo-LIST fetch +cursor (`forge_list_cursors`), and the two DL-053-spec'd tables +(`agent_forge_subscriptions` — DL-053's `forge_subscriptions`, renamed — and +`forge_artifact_cursors`), the latter two adapted from the ownership-layer +DDL (`compass-server-ownership-layer/design.md:976-1019`, there labeled +"`0004_forge`"; the label predates the current migration count, so it lands +as `0015`). The division of labor: + +- **`forge_repo_subscriptions`** — the board's per-REPO poll targets, + dynamically add/removable: an operator adds a repo → a row; removes → the + row is deleted (or `enabled` flipped false — soft-disable that keeps + cursor history), and its `forge_list_cursors` rows go inert. This REPLACES + the pre-ruling config-declared `ForgeConfig.Repos`-IS-the-subscription-set + model throughout this record: the driver enumerates `WHERE enabled` at the + top of every pass (T3), and the `--forge-repos` flag becomes a declarative + SEED reconciled into the table at boot (T4). Keyed by the forge coordinate + MINUS artifact: `(forge_provider, forge_host, repo)`. +- **`forge_list_cursors`** — the per-page fetch cursor of a target repo's + LIST walk (DDL below; the driver's own working table). +- **`agent_forge_subscriptions` + `forge_artifact_cursors`** — the DL-053 + per-artifact agent-notification machinery, writer-less this slice: nothing + this slice EXECUTES touches them; they land as anticipatory schema, shape + frozen + pgtest-covered (T2 test 7) before their writers (PR-C / the + agent-notification slice) exist. + +Adaptations, now all settled (the two that were OQ-D's sub-forks are ruled; +the rest are the mechanical application of existing tree standards): + +- **Coordinate alignment (author-decided):** the spec'd DDL keys on + `provider TEXT` and omits the forge host; the tree's frozen issue + coordinate is `(forge_provider SMALLINT, forge_host, repo, number)` + (`0013_issues.sql:29-35` — "forge coordinate: the idempotency key"). Every + 0015 key adopts the 0013 convention (SMALLINT provider enum + `forge_host + TEXT NOT NULL`) so cursor and subscription rows join the `issues` + coordinate without casts and GHES hosts never collide on `repo` alone. +- **Provider domain (OQ-D2, RESOLVED, Matt 2026-08-08):** every provider + CHECK in 0015 admits the FULL declared proto enum — `CHECK (forge_provider + IN (1, 2, 3, 4))`, GitHub/GitLab/Forgejo/Linear + (`proto/compass/v1/compass.proto:699-706`). The CHECK's job is "never + UNSPECIFIED(0)", NOT gating rollout: rollout is gated by which + `forge.Provider` has a real client (GitHub only, this slice; Matt: "we + need linear for dogfood, linear and github. gitlab/forgejo can come later + in beta etc."). Admitting the whole declared domain — never a + non-contiguous subset — means the future Linear-ingestion slice needs no + CHECK-widening migration on any 0015 table. (An earlier draft of this + record recommended narrowing to `IN (1, 2, 3)` to match 0013; that was + WRONG — it dropped Linear, which dogfood needs, and mistook the CHECK for + a rollout gate. The 0013 `issues` CHECK itself is the separate OQ-E + prerequisite.) +- **The rename (OQ-C):** `forge_subscriptions` → `agent_forge_subscriptions` + (Global Constraints; columns unchanged from + `compass-server-ownership-layer/design.md:978-999`). + +The three tables beyond the driver's own working table: -Author-decided: +```sql +-- The board's repo-level poll targets (OQ-C, the table model): one row per +-- (provider, host, repo) the poll driver walks. enabled=FALSE soft-disables +-- a target without deleting its cursor history. Populated in v1 by the T4 +-- boot seed reconcile (--forge-repos, bootstrap-only insert: ON CONFLICT DO +-- NOTHING — the table is authoritative after the first insert); a mutation +-- RPC/admin surface is a named non-goal of this slice. For GITHUB the repo +-- string is lowercased at the seed/upsert boundary (GitHub owner/name is +-- case-insensitive-but-case-preserving, so Owner/Name and owner/name must +-- NOT mint two PK rows -> two poll targets -> two issues rows under the +-- 0013 coordinate). updated_at is touched on every upsert/enable-flip, +-- giving an operator a timestamp to correlate a state change against +-- (the audit posture of advanced_at on forge_list_cursors). +CREATE TABLE forge_repo_subscriptions ( + forge_provider SMALLINT NOT NULL CHECK (forge_provider IN (1, 2, 3, 4)), + forge_host TEXT NOT NULL, + repo TEXT NOT NULL, + enabled BOOLEAN NOT NULL DEFAULT TRUE, + created_at TIMESTAMPTZ NOT NULL DEFAULT now(), + updated_at TIMESTAMPTZ NOT NULL DEFAULT now(), + PRIMARY KEY (forge_provider, forge_host, repo) +); -- **Coordinate alignment:** the spec'd DDL keys on `provider TEXT` and omits - the forge host; the tree's frozen issue coordinate is `(forge_provider - SMALLINT CHECK (forge_provider IN (1, 2, 3)), forge_host, repo, number)` - (`0013_issues.sql:29-35` — "forge coordinate: the idempotency key"). The - cursor and subscription keys adopt the 0013 convention (SMALLINT provider - enum + `forge_host TEXT NOT NULL`) so cursor rows join the `issues` - coordinate without casts and GHES hosts never collide on `repo` alone. -- **The new LIST-cursor class:** `forge_list_cursors` (below) is additive — - neither spec'd table changes shape, and it lands regardless of OQ-D (the - driver rides it directly). - -PARKED for Matt (OQ-D): - -- **Whether the two spec'd tables land in THIS slice at all.** Nothing this - slice EXECUTES touches `forge_subscriptions` or `forge_artifact_cursors`: the - driver rides `forge_list_cursors` only, the T2 store methods cover only the - list cursor, and every downstream writer is out of this build. So they land - now purely as anticipatory schema. The design's default (what the DDL below - assumes) is land-now — one forge migration, the shape frozen + pgtest-covered - before its writers exist. The counter-position (defer each spec'd table to - the slice that first writes it) is real: an EMPTY table is the cheapest thing - to `ALTER`, so freezing its shape early buys little over landing it beside - its first writer with more information, and landing it now FORCES the OQ-D - provider-domain call (below) before PR-C or the agent-notification slice - exist to inform it. This is OQ-D's first sub-fork. -- **If they land now: which provider domain their CHECK admits.** DL-053's - spec'd DDL keys `provider TEXT -- "github" | "linear"` - (`compass-server-ownership-layer/design.md:981`) — Linear is a first-class - provider of subscription/cursor rows in the frozen spec. Adopting the 0013 - coordinate's `SMALLINT CHECK (... IN (1, 2, 3))` (GitHub/GitLab/Forgejo, - `0013_issues.sql:33`) consciously NARROWS that domain: it drops `linear` - until the `ForgeProvider` enum and the CHECK grow. This is not "no semantic - change" — DL-129 routes tracker-status ingestion through "the DL-053 poll" - (`DECISIONS.md:173`) and the tracker for MVP is GitHub, so the narrowing is - likely harmless FOR NOW, but a future Linear tracker needs the enum/CHECK - extended (cheap while the tables are empty). OQ-D's second sub-fork; moot if - the first resolves to defer. - -If OQ-D resolves to defer, this migration lands `forge_list_cursors` alone and -the two spec'd tables ride their first writers; the rest of this record is -unchanged (the driver never touched them). The DDL below is the driver's own -working table either way. +-- DL-053's forge_subscriptions, renamed agent_forge_subscriptions (OQ-C) and +-- coordinate-aligned; columns otherwise per the spec'd DDL +-- (compass-server-ownership-layer/design.md:978-999). Writer-less this +-- slice — the agent-notification slice brings its writers. +CREATE TABLE agent_forge_subscriptions ( + id TEXT PRIMARY KEY, + agent_account_id TEXT NOT NULL REFERENCES agent_accounts (account_id) ON DELETE RESTRICT, + forge_provider SMALLINT NOT NULL CHECK (forge_provider IN (1, 2, 3, 4)), + forge_host TEXT NOT NULL, + repo TEXT NOT NULL, + kind SMALLINT NOT NULL CHECK (kind IN (1, 2)), + number BIGINT NOT NULL, + delivered_revision TEXT NOT NULL DEFAULT '', + delivered_at TIMESTAMPTZ, + created_at TIMESTAMPTZ NOT NULL DEFAULT now(), + UNIQUE (agent_account_id, forge_provider, forge_host, repo, kind, number) +); +CREATE INDEX agent_forge_subscriptions_artifact_idx + ON agent_forge_subscriptions (forge_provider, forge_host, repo, kind, number); + +-- DL-053's per-artifact FETCH cursor, coordinate-aligned; columns otherwise +-- per the spec'd DDL (compass-server-ownership-layer/design.md:1007-1019). +-- Writer-less this slice — PR-C / the agent-notification slice bring its +-- writers; its DL-053-spec'd garbage-collection ("collected when the +-- artifact's last subscription is deleted") is likewise a writer-slice +-- concern, deferred with the writers. +CREATE TABLE forge_artifact_cursors ( + forge_provider SMALLINT NOT NULL CHECK (forge_provider IN (1, 2, 3, 4)), + forge_host TEXT NOT NULL, + repo TEXT NOT NULL, + kind SMALLINT NOT NULL CHECK (kind IN (1, 2)), + number BIGINT NOT NULL, + etag TEXT NOT NULL DEFAULT '', -- issue/PR endpoint + comments_etag TEXT NOT NULL DEFAULT '', -- comments endpoint + checks_etag TEXT NOT NULL DEFAULT '', -- check-runs endpoint (PRs only) + revision TEXT NOT NULL DEFAULT '', + snapshot JSONB, -- last observed state, for DetectChanges + polled_at TIMESTAMPTZ NOT NULL DEFAULT now(), + PRIMARY KEY (forge_provider, forge_host, repo, kind, number) +); +``` The driver's own working table: @@ -278,7 +383,7 @@ The driver's own working table: -- advance (an etag-storing 200+sink), NOT the last poll: an all-304 tick reads -- the page but rewrites no row, so the column deliberately names "last change". CREATE TABLE forge_list_cursors ( - forge_provider SMALLINT NOT NULL CHECK (forge_provider IN (1, 2, 3)), + forge_provider SMALLINT NOT NULL CHECK (forge_provider IN (1, 2, 3, 4)), forge_host TEXT NOT NULL, repo TEXT NOT NULL, page INTEGER NOT NULL CHECK (page >= 1), @@ -399,17 +504,22 @@ The DL-053 poll loop, specialized to its first artifact class (the repo LIST) and its first subscriber (the board). It lives in the `ingest` package (it composes the package's own `Ingester`; a new package would be a second convention for no gain) and consumes the durable cursor through the -package-local `CursorStore` structural interface (Global Constraints → ingest +package-local `PollStore` structural interface (Global Constraints → ingest imports no store). - `Run(ctx)` performs one immediate pass at start (so a freshly booted demo server shows the board without waiting a full interval), then ticks on `time.Ticker(interval)`. -- Each pass iterates the configured repos sequentially — one forge, one - budget; parallel per-repo fetches would just race the same rate limit. +- Each pass first enumerates its targets — `PollStore.ListEnabledRepos`, + the `forge_repo_subscriptions WHERE enabled` read (OQ-C's table model) — + then iterates them sequentially: one forge, one budget; parallel per-repo + fetches would just race the same rate limit. A row added between ticks is + polled on the next pass; a disabled/deleted row simply stops being polled + (its `forge_list_cursors` rows are inert and its issues remain in the + projection). No restart either way. - **Per-repo pass = the DL-053 read-modify-advance cycle, page-wise:** - 1. Read the repo's stored page cursors (`CursorStore.ListCursor` → page → + 1. Read the repo's stored page cursors (`PollStore.ListCursor` → page → `{etag, hasNext}`). 2. From page 1: `ListIssuesPage(ctx, repo, forge.IssueFilter{}, page, storedETag)`. @@ -460,22 +570,26 @@ imports no store). in-process subscriber whose notify IS the synchronous sink inside the pass — there is no wire delivery to fail independently of the fetch, so its delivery state is fully encoded by the page-cursor advance and it holds no - `forge_subscriptions` row (OQ-C). Wire subscribers (agents) arrive with the - notification slice and bring their own `delivered_revision` rows, exactly - per spec. + `agent_forge_subscriptions` row — its membership is repo-level, a + `forge_repo_subscriptions` row (OQ-C, resolved). Wire subscribers (agents) + arrive with the notification slice and bring their own `delivered_revision` + rows, exactly per spec. - **The PR-C seam (downstream consumer, OUT of this build):** the per-pass raw batch (`[]forge.Issue` per 200 page) flows through one `Ingester.IngestIssues` call. PR-C composes its reverse `TrackerStatusMapping` sink onto that same batch (and its per-artifact `forge_artifact_cursors.revision`/`snapshot`/`polled_at` upserts, LIST-fed — no per-issue GET), in its own slice. Nothing in this record's build writes - `forge_subscriptions` or `forge_artifact_cursors`. + `agent_forge_subscriptions` or `forge_artifact_cursors`. - **Observability:** one `slog.Info` per completed repo pass with fields `repo`, `issues` (count sunk), `pages`, `not_modified` (bool: whole walk served 304s), `dur`, `ratelimit_remaining`; one `slog.Warn` per budget-skipped pass (an expected condition, never `Error`); one `slog.Error` only per genuinely failed pass (provider/sink error) with - `repo`, `err`. Boot logs one `slog.Info` with `repos`, `interval`. No + `repo`, `err`. Boot logs one `slog.Info` with `seed_repos` (the reconciled + seed count) and `interval`; the live target count is per-pass (`repos` on + the pass Info line would mislead — targets are table rows, not boot + state). No per-issue logging — a 500-issue repo must not emit 500 lines per minute. ### Boot wiring + config (`server/serve.go`, `cmd/compass-server/main.go`) @@ -484,20 +598,61 @@ Mirrors the two established precedents exactly: - **Config surface:** `server.ServeConfig` (`serve.go:56-89`) gains an optional `Forge ForgeConfig` field, all-optional exactly like `S3` - (`serve.go:71-75`): absent repos → no driver, today's behavior, zero new - requirements on existing deployments (the 0015 tables exist but sit empty — - a migration is not a behavior change). The CLI (`main.go`) maps flags with - env fallback mirroring the `COMPASS_S3_*` precedence (`main.go:127-138`): - `--forge-repos` / `$COMPASS_FORGE_REPOS` (comma-separated `owner/name`), - `--forge-poll-interval` / `$COMPASS_FORGE_POLL_INTERVAL` (default `1m`), + (`serve.go:71-75`): forge polling disabled → no driver, today's behavior, + zero new requirements on existing deployments (the 0015 tables exist but + sit empty — a migration is not a behavior change). The CLI (`main.go`) + maps flags with env fallback mirroring the `COMPASS_S3_*` precedence + (`main.go:127-138`): `--forge-repos` / `$COMPASS_FORGE_REPOS` + (comma-separated `owner/name`) — now a declarative SEED for + `forge_repo_subscriptions`, NOT the live subscription set; + `--forge-poll-interval` / `$COMPASS_FORGE_POLL_INTERVAL` (default `1m`); `--forge-secret` / `$COMPASS_FORGE_SECRET` (the declared secret NAME, - default `GITHUB_FORGE_TOKEN`; the VALUE never crosses a flag), plus + default `GITHUB_FORGE_TOKEN`; the VALUE never crosses a flag); `--forge-host` / `$COMPASS_FORGE_HOST` (default `github.com`, for GHES - later; the API base URL derives from it). The configured repo list IS the - board's subscription set under this record's OQ-C recommendation — - declarative, reconciled implicitly every pass (a removed repo simply stops - being polled; its cursor rows are inert and its issues remain in the - projection). + later; the API base URL derives from it); plus `--forge-poll` / + `$COMPASS_FORGE_POLL` (bool, default false) to run the driver with an + EMPTY seed (targets already in the table). Polling is enabled iff + `--forge-poll` is set OR the seed list is non-empty; only then are the + driver, the startup secret resolve, and the seed reconcile built. +- **Polling-disabled boot Warn:** when polling is DISABLED (`--forge-poll` + unset AND an empty seed) but `forge_repo_subscriptions` holds `enabled` + rows for the bound `(provider, host)`, boot emits ONE `slog.Warn` ("forge + polling disabled but N enabled targets exist; set --forge-poll") — one + query at boot (the store is already open at the gate decision), a warning + and never fail-fast (a deployment that lands 0015 plus a manual row insert + without the flag keeps today's no-driver behavior). The count covers ONLY + the bound `(provider, host)`'s enabled rows, so it gives no false comfort + about rows abandoned under a prior `--forge-host` (see the seed + reconcile). +- **Seed reconcile (v1 population of `forge_repo_subscriptions`):** at boot, + before the driver's first pass, each seed repo is INSERTED into + `forge_repo_subscriptions` with `enabled = TRUE` under `ON CONFLICT … DO + NOTHING` — a bootstrap-only insert: the flag creates rows that do not yet + exist and leaves an existing row entirely untouched; the TABLE is + authoritative after the first insert (Matt's OQ-C ruling intent — "needs + to be easily able to add/remove repos" — read to its end: the table, not + the flag, is the source of truth, dynamically). The seed is ADDITIVE, + never destructive — doubly so: a repo present in the table but absent + from the flag is NOT deleted and NOT disabled, AND a row an operator + soft-disabled STAYS disabled across restarts regardless of the flag (the + flag never flips an existing row's `enabled` — this eliminates the + footgun where a repo soft-disabled during an incident, still listed in + the deploy's `--forge-repos`, is silently re-enabled by a routine + restart). Re-enabling, like removal, is a table operation + (`SetForgeRepoSubscriptionEnabled` / SQL, or the future mutation + surface). For GITHUB each seed repo string is lowercased before insert + (the normalization rule in the DDL comment); renamed/transferred-repo + drift (GitHub serves a 301 that `net/http` follows silently while the + coordinate keeps the stale name) is OUT OF SCOPE for this slice. Seed + rows are keyed under the configured `--forge-host`: changing + `--forge-host` between boots ABANDONS (does not migrate) the prior + host's rows — they stay present and enabled but are never polled (the + adapter binds one `(provider, host)`), and the polling-disabled boot + Warn above counts only the BOUND `(provider, host)`'s enabled rows. The + live target set is ALWAYS the table (`WHERE enabled`, read per pass, + T3); the flag only feeds it new rows. This keeps a zero-UI operational + path for the MVP while the table supports dynamic add/remove; the + mutation RPC/admin surface is a named non-goal (Global Constraints). - **Secret resolve:** the driver's `TokenSource` closes over the one `secrets.SpecResolver` built at `serve.go:287`, calls `Resolve(ctx, "forge poll")` (`secrets/resolver.go:135`), and selects the @@ -530,8 +685,8 @@ Mirrors the two established precedents exactly: secret from degrading to silent forever-Error, the driver emits a distinct `slog.Error` "forge token unresolvable" the first time a resolve fails and on recovery — a minimal health surface without per-tick spam. -- **Cursor-store adapter:** `serve.go` builds the small adapter satisfying - `ingest.CursorStore` over the already-open `*store.Store` (`st`), binding +- **Poll-store adapter:** `serve.go` builds the small adapter satisfying + `ingest.PollStore` over the already-open `*store.Store` (`st`), binding the driver's `(provider, host)` so the ingest-side interface stays repo-keyed. This is the store seam the ingest package's no-store rule requires (Global Constraints), following the established @@ -595,15 +750,30 @@ Mirrors the two established precedents exactly: is still NOT in this slice (v1 cost is bounded and budget-gated without it); it is recorded as the first cheap optimization the durable cursor unlocks, adoptable without design change. -- **(g) Widening `forge_subscriptions` to hold the board as a row (nullable - `agent_account_id` + a subscriber-kind discriminator) — recommended - against; carried as the OQ-C alternative.** It would make "the board is a - subscriber" literal at the price of amending DL-053's spec'd shape - (`agent_account_id TEXT NOT NULL REFERENCES agent_accounts`, +- **(g) Widening the DL-053 subscription table to hold the board as a row + (nullable `agent_account_id` + a subscriber-kind discriminator) — + REJECTED; OQ-C resolved to a separate repo-level table instead.** It would + make "the board is a subscriber" literal at the price of amending DL-053's + spec'd shape (`agent_account_id TEXT NOT NULL REFERENCES agent_accounts`, `compass-server-ownership-layer/design.md:980`) and forcing every future subscription query through a kind filter — for a subscriber that needs neither `delivered_revision` (its delivery is the in-pass sink) nor - Subscribe/Unsubscribe dynamics (its set is operator config). See OQ-C. + per-artifact granularity (its targets are whole repos). Matt's OQ-C ruling + keeps the two concepts in two tables: `agent_forge_subscriptions` (the + DL-053 per-artifact row, renamed) and `forge_repo_subscriptions` (the + board's repo-level targets). See OQ-C. +- **(h) Seed conflict semantic `ON CONFLICT … DO UPDATE SET enabled = TRUE` + (flag-wins-for-its-members) — REJECTED in favor of `DO NOTHING` + (bootstrap-only insert).** Under DO-UPDATE the flag re-enables any + soft-disabled row still listed in `--forge-repos` at every boot, so + KEEPING a repo disabled requires a config edit AND a redeploy — + re-coupling the disable path to the deploy cycle, undoing exactly the + runtime mutability the OQ-C table ruling bought — and a repo an operator + soft-disabled during an incident is silently re-triggered by a routine + restart. Under DO NOTHING the flag only bootstraps rows that do not + exist; every subsequent state change is a table operation. Weighed here + explicitly so the DO NOTHING choice is ratified knowingly at design-PR + review (OQ-C). ## Plan @@ -713,22 +883,35 @@ Test cycle (unit, stubbed `http.RoundTripper`; red-first per repo convention): against the core rate limit; record the doc URL + date in the client's doc comment (the budget math depends on it). -### T2 — migration `0015_forge_subscriptions.sql` + the cursor store layer +### T2 — migration `0015_forge_subscriptions.sql` + the target/cursor store layer New migration `go/internal/store/migrations/0015_forge_subscriptions.sql` and new file `go/internal/store/forge_cursors.go` (+ `forge_cursors_pgtest_test.go`, build-tag gated like the sibling -`*_pgtest_test.go` suites). The migration creates the repo-LIST cursor class -`forge_list_cursors` (DDL in the Approach) unconditionally, and — if OQ-D -resolves land-now — the two DL-053-spec'd tables (`forge_subscriptions`, -`forge_artifact_cursors`, adapted from +`*_pgtest_test.go` suites). The migration creates all FOUR tables +unconditionally (OQ-D1 resolved land-now): `forge_repo_subscriptions` + +`forge_list_cursors` (DDL in the Approach) + the two DL-053-spec'd tables +(`agent_forge_subscriptions` — renamed from the spec's `forge_subscriptions` +— and `forge_artifact_cursors`, adapted from `compass-server-ownership-layer/design.md:976-1019`, renumbered from that record's stale `0004_forge` label, keys aligned to the 0013 issue-coordinate -convention: SMALLINT provider enum + `forge_host` in every key; the -provider-domain CHECK is OQ-D's second sub-fork). Store methods cover ONLY the -list cursor in this slice — `forge_subscriptions` / `forge_artifact_cursors` -get their store surface with their writers (the agent-notification slice / -PR-C), whenever they land. +convention: SMALLINT provider enum + `forge_host` in every key; every +provider CHECK admits `IN (1, 2, 3, 4)` per OQ-D2). Store methods cover the +list cursor AND `forge_repo_subscriptions` in this slice — the driver reads +targets from it and the T4 boot reconcile inserts new rows into it (`ON +CONFLICT DO NOTHING`); +`agent_forge_subscriptions` / `forge_artifact_cursors` get their store +surface with their writers (the agent-notification slice / PR-C). + +One enum-constant note: `store.ForgeProvider` today stops at Forgejo — +`ForgeProviderUnspecified=0, GitHub=1, GitLab=2, Forgejo=3` +(`store/issues.go:30-35`), and its mirror comment (`issues.go:26-27`) omits +Linear even though the proto declares `FORGE_PROVIDER_LINEAR = 4` +(`gen/compass/v1/compass.pb.go:452`). T2 adds `ForgeProviderLinear +ForgeProvider = 4` (and updates the mirror comment) so the Go domain matches +the 0015 CHECK domain; no behavior change — no producer passes 4 this slice +(the T4 wiring is GITHUB-only) — but a store-level constant must exist for a +domain the schema admits. Interfaces: @@ -739,7 +922,7 @@ Interfaces: // cursor (the DL-053 FETCH-cursor model at repo-LIST granularity). ETag "" // means never fetched (an unconditional GET). type ForgeListPageCursor struct { - Provider ForgeProvider // GITHUB(1)/GITLAB(2)/FORGEJO(3); never 0 + Provider ForgeProvider // GITHUB(1)/GITLAB(2)/FORGEJO(3)/LINEAR(4); never 0 Host string Repo string Page int32 // 1-based @@ -762,33 +945,74 @@ func (s *Store) UpsertForgeListCursorPage(ctx context.Context, cur ForgeListPage // PruneForgeListCursorPages deletes the repo's page rows with page > maxPage // (a repo whose walk shrank). maxPage < 1 -> ErrInvalidArgument. func (s *Store) PruneForgeListCursorPages(ctx context.Context, provider ForgeProvider, host, repo string, maxPage int32) error + +// ForgeRepoSubscription is one board poll target: a repo the poll driver +// walks (OQ-C's table model). Enabled=false soft-disables the target without +// deleting its cursor history. +type ForgeRepoSubscription struct { + Provider ForgeProvider + Host string + Repo string + Enabled bool +} + +// EnsureForgeRepoSubscription inserts the target if absent; on conflict it +// DOES NOTHING — the T4 seed reconcile is a bootstrap-only insert and the +// table is authoritative after the first insert (the seed never deletes, +// disables, or re-enables an existing row). +// Zero/empty coordinate fields -> ErrInvalidArgument. +func (s *Store) EnsureForgeRepoSubscription(ctx context.Context, sub ForgeRepoSubscription) error + +// ListEnabledForgeRepoSubscriptions reads the enabled targets for one +// (provider, host), ascending repo — the driver's per-pass target +// enumeration. No rows is a nil slice, not an error. +func (s *Store) ListEnabledForgeRepoSubscriptions(ctx context.Context, provider ForgeProvider, host string) ([]ForgeRepoSubscription, error) + +// SetForgeRepoSubscriptionEnabled flips one target's enabled bit, touching +// updated_at (the soft-disable path; the admin mutation surface is a later +// slice's — this method exists for it, for operators via SQL parity, and +// for tests). +// Unknown coordinate -> ErrNotFound; zero/empty fields -> ErrInvalidArgument. +func (s *Store) SetForgeRepoSubscriptionEnabled(ctx context.Context, provider ForgeProvider, host, repo string, enabled bool) error ``` Consumes: the migration runner + `pgtest` harness the sibling suites use -(`store/issues_pgtest_test.go` et al.), `ErrInvalidArgument` (the store's -existing sentinel, `issues.go:103-108` pattern). +(`store/issues_pgtest_test.go` et al.), `ErrInvalidArgument` / +`ErrNotFound` (the store's existing sentinels — the `issues.go:103-108` and +`issues.go:151-153` patterns). Test cycle (pgtest, isolated schema per case like `issues_pgtest_test.go`): 1. Migration applies cleanly from empty AND from a 0014 database (the - sequential-migration harness proves both); `forge_list_cursors` exists with - its PK/CHECKs (provider CHECK rejects 0, page CHECK rejects 0), plus the two - spec'd tables if OQ-D resolves land-now. -2. `ForgeListCursor` on a never-polled repo → nil, no error. -3. Upsert page 1 then re-upsert with a new ETag → one row, new ETag, + sequential-migration harness proves both); all four tables exist with + their PKs and CHECKs (page CHECK rejects 0; kind CHECKs reject 0/3). +2. Provider CHECK domain, proven on `forge_repo_subscriptions` AND + `forge_list_cursors`: providers 1..4 each insert cleanly; 0 and 5 are + each rejected — the CHECK admits the full declared enum and nothing else + (OQ-D2 at the schema). +3. `ForgeListCursor` on a never-polled repo → nil, no error. +4. Upsert page 1 then re-upsert with a new ETag → one row, new ETag, `advanced_at` advanced; rows come back ascending by page. -4. Two hosts / two providers with the same `repo` string do not collide (the - OQ-D coordinate rationale, proven). -5. `PruneForgeListCursorPages(maxPage=2)` on a 4-page repo leaves pages 1-2; +5. Two hosts / two providers with the same `repo` string do not collide (the + coordinate rationale, proven). +6. `PruneForgeListCursorPages(maxPage=2)` on a 4-page repo leaves pages 1-2; pruning a never-polled repo is a no-op. -6. Invalid input: zero provider / empty host / empty repo / page 0 → - `ErrInvalidArgument` on each method. -7. **(Only if OQ-D resolves land-now)** `forge_subscriptions` FK: inserting a - row with an unknown `agent_account_id` fails (RESTRICT, per the DL-053 DDL - and the 0006-established convention `0006_delivery_cursors.sql:6-9`) — the - table is writer-less this slice but its shape is proven here. If OQ-D - resolves defer, this case drops and test 1 asserts only `forge_list_cursors` - exists. +7. `agent_forge_subscriptions` FK shape: inserting a row with an unknown + `agent_account_id` fails (RESTRICT, per the DL-053 DDL and the + 0006-established convention `0006_delivery_cursors.sql:6-9`) — the table + is writer-less this slice but its shape is proven here. Unconditional + (OQ-D1 resolved land-now). +8. Repo-subscription CRUD: ensure-insert creates the row enabled; + re-ensure of the same coordinate is idempotent (one row); ensure-insert + over a soft-disabled row leaves it DISABLED (`ON CONFLICT DO NOTHING` — + the bootstrap-only semantic; this leg goes RED against a DO-UPDATE + regression); `ListEnabledForgeRepoSubscriptions` returns only enabled + rows for the asked (provider, host), ascending repo, and nil for none; + `SetForgeRepoSubscriptionEnabled(false)` removes the row from the + enabled list WITHOUT deleting it and advances `updated_at` (the audit + column); unknown coordinate → `ErrNotFound`. +9. Invalid input: zero provider / empty host / empty repo / page 0 → + `ErrInvalidArgument` on each method (cursor and subscription both). ### T3 — `ingest.Driver`: the DL-053 conditional-poll driver @@ -797,7 +1021,11 @@ refactor inside `ingest.go`: extract the translate+sink loop of `Ingest` (`ingest.go:58-63`) into `IngestIssues(ctx, repo, raws)` so the driver can feed caller-fetched page batches through the existing per-issue pipeline; `Ingest` becomes `ListIssues` + `IngestIssues` (behavior unchanged — the -existing `ingest_test.go` suite is the regression net). Pass algorithm, +existing `ingest_test.go` suite is the regression net). Each pass begins +with a fresh target enumeration (`PollStore.ListEnabledRepos` — the +`forge_repo_subscriptions WHERE enabled` read, bound to the driver's +provider+host by the T4 adapter), so a target added or disabled between +ticks takes effect on the next pass with no restart. Pass algorithm, page-wise cursor advance, budget/error classification, logging per the Approach; `nil` return on ctx cancellation. @@ -820,10 +1048,16 @@ type ListPageCursor struct { HasNext bool } -// CursorStore is the durable FETCH-cursor surface the driver needs — a narrow, -// repo-keyed structural seam (the server wiring adapts *store.Store and binds -// provider+host), keeping this package's no-store property (ingest.go:7-8). -type CursorStore interface { +// PollStore is the durable target + FETCH-cursor surface the driver needs +// — a narrow, repo-keyed structural seam (the server wiring adapts +// *store.Store and binds provider+host), keeping this package's no-store +// property (ingest.go:7-8). Named for the whole surface the poller uses: +// its first method is target enumeration, not cursor state. +// ListEnabledRepos is the per-pass target enumeration +// (forge_repo_subscriptions WHERE enabled — OQ-C's table model); the three +// cursor methods are the page-cursor surface. +type PollStore interface { + ListEnabledRepos(ctx context.Context) ([]string, error) ListCursor(ctx context.Context, repo string) ([]ListPageCursor, error) UpsertListCursorPage(ctx context.Context, repo string, cur ListPageCursor) error PruneListCursorPages(ctx context.Context, repo string, maxPage int) error @@ -835,14 +1069,15 @@ type pageLister interface { ListIssuesPage(ctx context.Context, repo string, f forge.IssueFilter, page int, etag string) (forge.ListPage, error) } -// DriverConfig configures the board-ingestion poll driver. +// DriverConfig configures the board-ingestion poll driver. The target repos +// are NOT config — the driver reads them from PollStore.ListEnabledRepos +// at the top of every pass. type DriverConfig struct { - Repos []string // "owner/name", non-empty (validated by the caller) Interval time.Duration // > 0; the caller defaults it Log *slog.Logger // nil -> slog.Default() } -func NewDriver(client pageLister, ing *Ingester, cursors CursorStore, cfg DriverConfig) *Driver +func NewDriver(client pageLister, ing *Ingester, cursors PollStore, cfg DriverConfig) *Driver // Run polls until ctx is cancelled, then returns nil (clean shutdown). // Per-repo errors are logged and retried next tick, never returned — a @@ -853,11 +1088,12 @@ func (d *Driver) Run(ctx context.Context) error Consumes: `Ingester` (`ingest.go:35-46`), `forge.ListPage` / `forge.ErrBudgetExhausted` (T1), `time.Ticker`, `log/slog`. -Test cycle (unit, fake `pageLister` + fake `CursorStore` + the package's +Test cycle (unit, fake `pageLister` + fake `PollStore` + the package's existing `recordingSink` pattern, `ingest_test.go:13-27`): -1. `Run` performs an immediate pass for every repo before the first tick - (short interval, assert call order/coverage). +1. `Run` performs an immediate pass over every enabled target + (`ListEnabledRepos` faked) before the first tick (short interval, assert + call order/coverage). 2. ctx cancel BETWEEN ticks → `Run` returns `nil` promptly, and ctx cancel DURING a pass also returns `nil` promptly — deadline-gated, never a sleep (the pgtest convention, `service_board_pgtest_test.go:15`). Assert `Run` @@ -888,38 +1124,57 @@ existing `recordingSink` pattern, `ingest_test.go:13-27`): identically (existing `ingest_test.go` cases stay green unmodified), and `IngestIssues` on a batch stops at the first sink error with the same wrapped error shape (`ingest.go:60-62`). +11. Per-pass target enumeration is live: a repo added to the fake + `ListEnabledRepos` result between ticks is polled on the next pass; a + repo removed/disabled between ticks is not; zero targets → an idle pass + (no fetch, no sink, no Error log). +12. `ListEnabledRepos` returning an error → `slog.Error`, the pass is + skipped, `Run` keeps ticking (never fatal), and the next tick + re-enumerates. ### T4 — serve boot wiring + config surface + secret resolve Touches `server/serve.go`, `cmd/compass-server/main.go` (edits only, no new files beyond tests). `ServeConfig.Forge` all-optional like `S3`; CLI -flags/env per the Approach; fail-fast on forge-config-present but -secret-unresolvable; the `ingest.CursorStore` adapter over the open +flags/env per the Approach; the boot seed reconcile that inserts +`--forge-repos` into `forge_repo_subscriptions` (bootstrap-only, `ON +CONFLICT DO NOTHING`, before the driver's first pass); the polling-disabled +boot Warn (the Approach's boot-Warn bullet); fail-fast on +forge-polling-enabled but secret-unresolvable; the `ingest.PollStore` +adapter over the open `*store.Store`; driver under the existing errgroup. Interfaces: ```go // server.ForgeConfig configures the board-ingestion poll driver (SEA-1810). -// All-optional: empty Repos leaves the driver off (today's behavior). +// All-optional: polling disabled (empty SeedRepos, Poll false) leaves the +// driver off (today's behavior). type ForgeConfig struct { Host string // default "github.com" - Repos []string // "owner/name"; empty -> no driver + SeedRepos []string // "owner/name" (--forge-repos): boot-reconciled + // into forge_repo_subscriptions (bootstrap-only + // insert, ON CONFLICT DO NOTHING; lowercased + // for GITHUB) — a SEED, not the live target set + Poll bool // --forge-poll: run the driver even with an + // empty seed (targets already in the table) SecretName string // declared server_only secret name; default "GITHUB_FORGE_TOKEN" PollInterval time.Duration // default time.Minute } -// ServeConfig gains: Forge ForgeConfig +// ServeConfig gains: Forge ForgeConfig; polling enabled iff +// Poll || len(SeedRepos) > 0 -// unexported in server/: the ingest.CursorStore adapter binding the driver's +// unexported in server/: the ingest.PollStore adapter binding the driver's // forge coordinate half, so the ingest-side seam stays repo-keyed. -type forgeCursorStore struct { +type forgePollStore struct { st *store.Store provider store.ForgeProvider host string } -// satisfies ingest.CursorStore: ListCursor / UpsertListCursorPage / -// PruneListCursorPages, each delegating to the T2 store methods with the -// bound (provider, host). +// satisfies ingest.PollStore: ListEnabledRepos (→ the T2 +// ListEnabledForgeRepoSubscriptions, mapped to repo strings) / ListCursor / +// UpsertListCursorPage / PruneListCursorPages, each delegating to the T2 +// store methods with the bound (provider, host). ``` Consumes: `secrets.SpecResolver.Resolve(ctx, reason) ([]ResolvedSecret, error)` @@ -933,24 +1188,42 @@ methods, `ingest.NewDriver`/`Run` (T3), the errgroup at `serve.go:327`, Produces: a `TokenSource` closure over the resolver that re-resolves + selects `SecretName` on TTL expiry or auth failure (not per pass; see the -Secret-resolve bullet); the cursor-store adapter; boot-time validation error -text for missing-secret / bad-repo-format / non-positive interval. +Secret-resolve bullet); the boot seed reconcile (each `SeedRepos` entry +lowercased for GITHUB and inserted enabled into `forge_repo_subscriptions` +via the T2 ensure-insert, `ON CONFLICT DO NOTHING`, before the driver's +first pass); the polling-disabled boot Warn; the poll-store adapter; +boot-time validation +error text for missing-secret / bad-repo-format / non-positive interval. Test cycle: -1. Unit: CLI flag/env precedence for the four forge knobs mirrors the S3 - pattern (table test like `resolveNetworkDoor`'s, `main.go:184-212`); - repo-format validation (`owner/name`) rejects garbage at startup. -2. Unit: empty `Forge.Repos` → `Serve` builds no driver (assert via config +1. Unit: CLI flag/env precedence for the five forge knobs (repos seed, poll + enable, interval, secret name, host) mirrors the S3 pattern (table test + like `resolveNetworkDoor`'s, `main.go:184-212`); repo-format validation + (`owner/name`) rejects garbage at startup; case normalization: a seed of + `Owner/Name` reconciles to ONE lowercased `owner/name` row, never two PK + rows. +2. Unit: forge polling disabled (empty `SeedRepos`, `Poll` false) → `Serve` + builds no driver and attempts no secret resolve (assert via config plumbing seam, no behavioral change to existing serve tests — the whole existing suite is the regression net; the 0015 migration alone must not - change any existing test's outcome). + change any existing test's outcome); `Poll` true with an empty seed → + the driver IS built (table-driven targets, idle passes until rows exist); + polling disabled with enabled `forge_repo_subscriptions` rows present + for the bound (provider, host) → boot emits exactly ONE `slog.Warn` + ("forge polling disabled but N enabled targets exist"; capture a slog + test handler), and no Warn when no enabled rows exist. 3. pgtest (build-tag gated like the board suite): a `Serve`-assembled pipeline over a fake pager scripted with one issue page sinks to the real projection, lands in `ListBoardIssues`, AND leaves a `forge_list_cursors` page-1 row carrying the scripted ETag — the end-to-end boot-wiring + durable-cursor proof without touching live - GitHub. + GitHub. The target repo enters via the seed reconcile, and the ordering + assertion is DIRECT: assert the seeded `forge_repo_subscriptions` row is + visible BEFORE the fake pager's first call (instrument the fake pager to + snapshot the row at call time, or — if the harness can shrink the + interval — assert explicit call ordering), not merely inferred from a + 1m-interval timeout. 4. pgtest (restart-resync proof — the durable cursor's headline buy): after test 3, rebuild the pipeline over the SAME schema (a "restart") with the pager scripting a 304 for the stored ETag; assert the pass issues no @@ -972,7 +1245,7 @@ Test cycle: the changed resolver value is used on the next fetch — the design's stated reason for a TTL-cache `TokenSource` with an invalidation seam rather than a captured token or a bare func. -7. Startup failure, two distinct texts: (a) forge config present + secret NAME +7. Startup failure, two distinct texts: (a) forge polling enabled + secret NAME absent from the resolved set → `Serve` returns `"forge secret %q not declared"`; (b) the resolve call itself ERRORS at boot → `Serve` returns `"forge secret resolve failed at startup: %w"`. Both follow the @@ -980,6 +1253,18 @@ Test cycle: since the resolver is built after the UDS listener binds (`serve.go:287`); assert the two error strings are distinguishable so a permanent misconfig is not confused with a transient provider outage. +8. pgtest (the seed-reconcile semantic — bootstrap-only insert, `ON + CONFLICT DO NOTHING`, never destructive and never mutating; this test + MUST be able to go RED against both a destructive-sync and a DO-UPDATE + regression): reconcile with seed `{a/b, c/d}` → both rows enabled; + insert `e/f` directly (the "table-added" repo) and re-run the reconcile + with seed `{a/b}` → `c/d` and `e/f` BOTH survive, still enabled (a repo + dropped from the flag is NOT auto-deleted or disabled); disable `c/d` + via `SetForgeRepoSubscriptionEnabled(false)` and re-run the reconcile + with `c/d` STILL in the seed → `c/d` STAYS disabled (the flag never + flips an existing row's `enabled`; this leg goes RED against a + DO-UPDATE-SET-enabled regression); disable `e/f` (absent from the seed) + and re-run → stays disabled. ## Tasks @@ -989,20 +1274,27 @@ Test cycle: Invalidate` on bad-creds, `HasNext`/pagination, filter mapping incl. `state=all` default, PR exclusion, StatusError mapping, bearer auth, 304-uncharged doc reverify). -- [ ] **T2** — migration `0015_forge_subscriptions.sql` (`forge_list_cursors` - always; `forge_subscriptions` + `forge_artifact_cursors` per DL-053, - coordinate-aligned, IF OQ-D resolves land-now) + `store/forge_cursors.go` - list-cursor methods; pgtest-covered. +- [ ] **T2** — migration `0015_forge_subscriptions.sql` (all four tables + unconditionally: `forge_repo_subscriptions` + `forge_list_cursors` + + `agent_forge_subscriptions` + `forge_artifact_cursors`, + coordinate-aligned, every provider CHECK `IN (1, 2, 3, 4)`) + + `store/forge_cursors.go` list-cursor + repo-subscription methods + + `store.ForgeProviderLinear = 4`; pgtest-covered (incl. the CHECK-domain + and FK-shape proofs). - [ ] **T3** — `ingest.Driver` conditional-poll driver (`driver.go` + the `IngestIssues` extraction in `ingest.go`; unit tests: immediate pass, ctx - cancel → nil, conditional walk over stored cursors, sink-gated cursor - advance, prune, per-repo error isolation, budget-skip classification, - interval discipline, slog fields). -- [ ] **T4** — serve boot wiring: `ServeConfig.Forge`, CLI flags/env, DL-052 - secret resolve via the existing `SpecResolver`, the `ingest.CursorStore` - adapter over `*store.Store`, errgroup membership, fail-fast validation; - unit + pgtest end-to-end incl. the durable-cursor restart proof and the - no-clobber proof. + cancel → nil, per-pass target enumeration via `ListEnabledRepos`, + conditional walk over stored cursors, sink-gated cursor advance, prune, + per-repo error isolation, budget-skip classification, interval + discipline, slog fields). +- [ ] **T4** — serve boot wiring: `ServeConfig.Forge` (seed + poll-enable), + CLI flags/env, the boot seed reconcile into `forge_repo_subscriptions` + (bootstrap-only insert, `ON CONFLICT DO NOTHING`), the polling-disabled + boot Warn, DL-052 secret resolve via the existing `SpecResolver`, + the `ingest.PollStore` adapter over `*store.Store`, errgroup + membership, fail-fast validation; unit + pgtest end-to-end incl. the + durable-cursor restart proof, the no-clobber proof, and the + seed-reconcile semantic. ## Open Questions @@ -1018,96 +1310,114 @@ shared fetch half either way; its one structural change — cursor state moves out of the client into `forge_list_cursors` — is the Approach's stateless-client bullet). The superseded Option 1 / Option 1.5 framings and their tradeoffs are retained in Alternatives (b) for the reasoning trail; the fork is closed and -is not relitigated here. The follow-on shape calls INSIDE Option 2 are OQ-C -and OQ-D below. - -### OQ-C (LOAD-BEARING — blocks the freeze): how the board is represented as a subscriber - -DL-053's subscription row is per-artifact and agent-owned -(`agent_account_id TEXT NOT NULL REFERENCES agent_accounts`, -`compass-server-ownership-layer/design.md:980`); the board subscribes to whole -repos and is not an agent. Two representations: - -- **Config-declared (RECOMMENDED, and what this record designs):** the - board's subscription set IS `ForgeConfig.Repos` — operator config, no - `forge_subscriptions` rows, no delivery cursor (the board's notify is the - synchronous in-pass projection sink, so sunk-ness is already encoded by the - sink-gated `forge_list_cursors` advance; a `delivered_revision` for it - would duplicate what the `issues` table already records). The repo-LIST - fetch state lives in the new sibling table `forge_list_cursors` (Approach); - DL-053's two spec'd tables land untouched in shape and gain their writers - in the later slices. Cost: when agent subscriptions arrive, the poll driver - enumerates targets from two sources (config repos for the LIST class, - subscription rows for the per-artifact class) — acceptable because the two - classes hit different endpoints with different cursor tables either way, - and both share the one client, budget gate, and pass scheduler. -- **A widened subscription row (Alternatives (g)):** nullable - `agent_account_id` + a subscriber-kind discriminator + a repo-level - artifact kind, making the board a literal `forge_subscriptions` row. Buys a - single target-enumeration query; costs an amendment to DL-053's spec'd - shape and a kind-filter on every future subscription query, for a - subscriber with no delivery semantics and no Subscribe/Unsubscribe - dynamics. - -**Recommendation: config-declared.** It consumes DL-053 without amending it -(the brief the ruling set), and the one-fetch-path property PR-C needs holds -identically under both (it hangs on `forge_list_cursors` + the shared pass, -not on how the board's membership is stored). PARKED for Matt; batched by the -main agent. - -### OQ-D (LOAD-BEARING — blocks the freeze): whether (and how) the two spec'd DL-053 tables land in this slice - -The coordinate alignment itself — SMALLINT provider enum + `forge_host` in -every key, matching the tree's frozen 0013 issue coordinate -(`0013_issues.sql:29-35`) rather than the ownership-layer DDL's `provider -TEXT` + host-less key (`compass-server-ownership-layer/design.md:976-1019`) — -is NOT the fork: it is the mechanical application of the tree's existing -standard, author-decided (Approach → author-decided), because diverging would -fork the coordinate vocabulary inside one schema and force casts/joins between -cursor and issue rows. Two things about it ARE genuine forks: - -**D1 — land the two spec'd tables now, or defer them to their first writers.** -Nothing this slice EXECUTES touches `forge_subscriptions` or -`forge_artifact_cursors`: the driver rides `forge_list_cursors` only (Approach -→ the DL-053 tables). So they land now purely as anticipatory schema. - -- **Land-now (what this record designs):** one forge migration; the DL-053 - shape is frozen and pgtest-covered (T2 test 7) before its writers exist, - matching the ruling's "build on the DL-053 machinery from day one" read as - including the schema. Cost: it forces the D2 provider-domain call now. -- **Defer:** `0015` lands `forge_list_cursors` alone; each spec'd table rides - the slice that first writes it (PR-C / agent-notification) with more - information. An EMPTY table is the cheapest thing to `ALTER`, so freezing its - shape early buys little, and deferring dodges D2 entirely until a writer - needs it. The driver is byte-identical either way (it never touches those - tables). Cost: a second forge migration two slices out. - -**Recommendation: land-now** — it is the most direct reading of the day-one -ruling and one migration is tidier, but the buy is genuinely small, so this is -a real call for Matt, not an author default dressed as one. - -**D2 — IF land-now, which provider domain the CHECK admits (moot if D1 = -defer).** DL-053's spec'd DDL keys `provider TEXT -- "github" | "linear"` -(`compass-server-ownership-layer/design.md:981`) — Linear is a first-class -provider of subscription/cursor rows in the frozen spec. Adopting the 0013 -coordinate's `SMALLINT CHECK (forge_provider IN (1, 2, 3))` -(GitHub/GitLab/Forgejo, `0013_issues.sql:33`) consciously NARROWS that domain: -it drops `linear` until the `ForgeProvider` enum and the CHECK grow. This is -NOT "no semantic change" (the pre-fold text's claim, now corrected). DL-129 -routes tracker-status ingestion through "the DL-053 poll" (`DECISIONS.md:173`) -and the MVP tracker is GitHub, so the narrowing is likely harmless for now — -but it is a real domain decision, cheap to make correctly while the tables are -empty and expensive to discover wrong once a Linear tracker arrives. - -- **Narrow to {github, gitlab, forgejo} (RECOMMENDED):** one coordinate - vocabulary across `issues` and the forge tables; a Linear tracker later - extends the enum + CHECK (a one-line migration on empty-or-small tables). -- **Widen the CHECK to include a `linear` enum value now:** honors the DL-053 - spec's stated domain literally, at the cost of introducing an enum value - with no producer this slice or the next. - -PARKED for Matt (both sub-forks); batched by the main agent. If D1 = defer, -D2 does not need ruling now. +is not relitigated here. The follow-on shape calls INSIDE Option 2 were OQ-C +and OQ-D — both since RESOLVED (Matt, 2026-08-08), below. + +### OQ-C — RESOLVED (Matt, 2026-08-08): the board's subscription set is a TABLE + +Matt: "needs to be easily able to add/remove repos, so I think it needs to +be a table? It can be a different one than forge_subscriptions if needed, or +that can be renamed to agent_forge_subscriptions, etc." + +Resolution, as applied through this record: + +- **New table `forge_repo_subscriptions`** — the board's repo-level poll + targets, keyed `(forge_provider, forge_host, repo)`, with `enabled + BOOLEAN` for soft-disable (DDL in the Approach). The driver enumerates + `WHERE enabled` at the top of every pass (T3), so add/remove is a row + operation, live at the next tick, no restart and no deploy. +- **DL-053's `forge_subscriptions` lands RENAMED + `agent_forge_subscriptions`** (Matt's explicit option) — shape unchanged; + the rename disambiguates the per-ARTIFACT, agent-owned subscription from + the board's per-REPO target (Global Constraints). Proposed freeze-time + delta, BOTH halves (prose only in this Draft PR): (a) a DECISIONS.md + ledger row recording the table model + the rename, AND (b) a one-line + annotation at the ownership-layer DDL + (`compass-server-ownership-layer/design.md:983`, the `CREATE TABLE + forge_subscriptions`) pointing forward — "lands as + `agent_forge_subscriptions`; see compass-forge-poll-driver" — so a reader + entering from the frozen ownership-layer record who greps + `forge_subscriptions` finds the mapping instead of nothing. No + DECISIONS.md or ownership-layer edit rides this Draft PR + (`Ledger-impact: none in this PR — Draft record`). +- **v1 population is the T4 boot seed reconcile** — `--forge-repos` + inserted enabled where absent, `ON CONFLICT DO NOTHING`: additive, never + destructive, never mutating an existing row — and the dynamic mutation + RPC/admin surface is a named non-goal of this slice. The ON CONFLICT + semantic is author-resolved to DO NOTHING (the DO-UPDATE-SET-enabled + alternative is weighed and rejected in Alternatives (h)); flagged for + Matt's ratification at design-PR review. + +For the trail: this record previously RECOMMENDED config-declared +(`ForgeConfig.Repos` as the subscription set, no rows); Matt ruled it out +because the target set must be mutable at runtime without a deploy. The +widened-subscription-row alternative stays rejected (Alternatives (g)). The +one-fetch-path guarantee is unaffected — it hangs on `forge_list_cursors` + +the shared pass, not on where the target set lives. The board still holds NO +delivery cursor: its notify is the synchronous in-pass sink, so that half of +the original question resolves unchanged (Approach (a)). + +### OQ-D — RESOLVED (Matt, 2026-08-08): all four tables land now; CHECKs admit the full enum + +**D1 — land-now.** Migration 0015 unconditionally lands +`forge_repo_subscriptions`, `forge_list_cursors`, +`agent_forge_subscriptions`, and `forge_artifact_cursors` (Approach → the +four forge tables). The defer alternative (each spec'd table rides its first +writer) is closed; T2 test 7 — the FK-shape proof on the writer-less +`agent_forge_subscriptions` — is unconditional. The coordinate alignment +(SMALLINT provider + `forge_host` in every key, per `0013_issues.sql:29-35`) +was never the fork and stands as author-decided. + +**D2 — every provider CHECK admits the FULL declared enum, `IN (1, 2, 3, +4)`.** Matt: "why did we drop linear? we need linear for dogfood, linear and +github. gitlab/forgejo can come later in beta etc." The earlier +recommendation here — narrow to `{github, gitlab, forgejo}` to match the +0013 coordinate — was WRONG and is corrected: it dropped Linear, which +dogfood needs, and it mistook the CHECK for a rollout gate. The CHECK's job +is "never UNSPECIFIED(0)"; rollout is gated by which `forge.Provider` has a +real client (GitHub only, this slice — no producer for gitlab/forgejo/linear +issues exists in the tree). The proto enum already declares all four +providers (`FORGE_PROVIDER_LINEAR = 4`, `proto/compass/v1/compass.proto:704` +— "DL-051's issues-only forge source: a Linear-origin Issue whose repo is +the project key"), so the 0015 tables admit the whole declared domain (never +a non-contiguous subset like `IN (1, 4)`) and the future Linear slice needs +no CHECK-widening migration on any of them. The `issues` table's own +`IN (1, 2, 3)` is the separate prerequisite documented as OQ-E. + +### OQ-E (out of scope — the Linear-ingestion prerequisite; follow-up filed by the driver) + +SEA-1810's new tables are Linear-ready (their CHECKs admit 4), but actually +INGESTING Linear issues has three prerequisites, ALL out of scope for this +GitHub-read slice and belonging to the future Linear-ingestion slice: + +- **The `issues` CHECK:** `0013_issues.sql:32` is `forge_provider SMALLINT + NOT NULL CHECK (forge_provider IN (1, 2, 3))` — "GitHub/GitLab/Forgejo; + never UNSPECIFIED(0)". It EXCLUDES Linear(4): the projection cannot store + a Linear-provider issue today. Widening it to `IN (1, 2, 3, 4)` is one + migration on that ONE table. +- **The store Go enum:** `store.ForgeProvider` constants stop at + `ForgeProviderForgejo = 3` (`store/issues.go:30-35`). T2 already adds + `ForgeProviderLinear = 4` so the Go domain matches the 0015 CHECK domain — + this half is closed by this slice; the future slice adds its producer. +- **A Linear `forge.Provider` client** (DL-051, `DECISIONS.md:126`: the + forge adapter is "GitHub first, Linear issues-only"; `ForgeRef.host` for + Linear is the constant "linear.app", `compass.proto:711-712`). + +Because the 0015 tables already admit 4, that future slice touches the ONE +`issues` CHECK plus a client — not the whole forge-table set. The driver +files the follow-up issue; this record only documents the prerequisite. Not +load-bearing for this slice's freeze. + +Why the asymmetry (`issues` CHECK deferred in SQL, Go domain closed now) is +deliberate: the Go constant `store.ForgeProviderLinear = 4` is required THIS +slice because 0015's own store methods must be able to name domain value 4 +(the four new tables' CHECKs admit it), while the `issues` CHECK guards a +table SEA-1810 never writes with provider=4 (the driver is bound to GITHUB; +no Linear producer exists in the tree) — and widening a guard ahead of its +producer would weaken 0013's "every issue is forge-backed" documentation, so +the widening correctly waits for the Linear producer. This deferral is +author-resolved pending Matt's review: he may tie-break to fold the +`issues`-CHECK widening into 0015 if he prefers SQL/Go symmetry. ### OQ-B (non-load-bearing deferral): live-demo content From 36a3fc47883155e5916488199c4b82b26f5cf887 Mon Sep 17 00:00:00 2001 From: seal Date: Fri, 7 Aug 2026 16:38:58 -0400 Subject: [PATCH 5/5] =?UTF-8?q?docs(designs):=20fold=20PR=20#219=20review?= =?UTF-8?q?=20=E2=80=94=20accurate=20ledger-state=20prose,=20defer=20DL-05?= =?UTF-8?q?3=20annotation=20(SEA-1883),=20citation=20drifts,=20empty-probe?= =?UTF-8?q?=20row?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round (design-critic + review agent) surfaced 2 medium + 2 low, all documentation/ledger-cross-reference; the poll/cursor/seed/budget model passed every soundness lens. - M1: correct the stale 'Draft'/'Ledger-impact: none' prose — the record is Active and its ledger delta (DL-161/162/163) rides this PR. - M2: the DL-053 ownership-layer forward-annotation is deferred to SEA-1883 (editing a frozen record needs Matt's call); stated explicitly in OQ-C, and the mis-cited target line :983 corrected to :978. - L1: DECISIONS.md citations drifted +3 by this PR's own DL rows — :173->:176 (DL-129), :126->:129 (DL-051). - L2: an empty boundary-probe page writes no cursor row (skip the upsert on a zero-issue page), so the probe never persists an unprunable empty tail row. Co-authored-by: Matt Wilkinson --- .../compass-forge-poll-driver/design.md | 38 +++++++++++-------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/docs/designs/product/compass-forge-poll-driver/design.md b/docs/designs/product/compass-forge-poll-driver/design.md index f65ba1df..408b202a 100644 --- a/docs/designs/product/compass-forge-poll-driver/design.md +++ b/docs/designs/product/compass-forge-poll-driver/design.md @@ -82,10 +82,10 @@ no convergence refactor ever needed. subscription (`agent_account_id TEXT NOT NULL REFERENCES agent_accounts`, `:980`); `forge_repo_subscriptions` is the board's per-REPO poll target (new, this record). A future reader maps `agent_forge_subscriptions` back - to DL-053's `forge_subscriptions`; recording the rename — BOTH a - DECISIONS.md ledger row AND a one-line annotation at the ownership-layer - DDL — is a proposed freeze-time delta (see Open Questions → OQ-C), not - part of this Draft PR. + to DL-053's `forge_subscriptions`. The rename is recorded in the ledger as + DL-163 (rides THIS PR); the paired one-line forward annotation at the frozen + ownership-layer DDL is deferred to SEA-1883 (editing a frozen record needs + Matt's call — see Open Questions → OQ-C). - **Provider domain `IN (1, 2, 3, 4)` on every 0015 table (OQ-D2, Matt 2026-08-08):** the proto enum already declares all four providers — `FORGE_PROVIDER_GITHUB = 1 … FORGE_PROVIDER_LINEAR = 4` @@ -96,7 +96,7 @@ no convergence refactor ever needed. pre-existing `issues` CHECK `IN (1, 2, 3)` (`0013_issues.sql:32`) excludes Linear and is a SEPARATE prerequisite for actually ingesting Linear issues — out of scope here, documented as OQ-E. -- **DL-129 (FROZEN, Matt 2026-08-04, `DECISIONS.md:173`):** tracker native +- **DL-129 (FROZEN, Matt 2026-08-04, `DECISIONS.md:176`):** tracker native status "is ingested into the DL-070 server projection through the reverse `TrackerStatusMapping` (DL-053 poll; echo-suppressed in tracker-status space, tracker-sourced transitions never mirror back, stale polls dropped @@ -554,7 +554,11 @@ imports no store). A never-stored page is already an `etag=''` unconditional GET, so the mechanism exists — a `HasNext`/non-empty result there resumes the normal walk and re-anchors the tail; an empty 200 costs one request per hour on a - quiet repo. This is a documented probe rule, not a fork. + quiet repo. An empty probe page (`HasNext=false`, zero issues) writes NO + cursor row — the per-page upsert is skipped when the page has zero issues, + so the probe never persists an empty tail row that would then 304 forever + and sit outside `PruneListCursorPages`' post-walk `page > lastPage` drop. + This is a documented probe rule, not a fork. - **A failed poll is logged, never fatal.** `Run` never returns a non-nil error — its sole exit is ctx cancellation, returning `nil` so the errgroup @@ -1329,17 +1333,19 @@ Resolution, as applied through this record: - **DL-053's `forge_subscriptions` lands RENAMED `agent_forge_subscriptions`** (Matt's explicit option) — shape unchanged; the rename disambiguates the per-ARTIFACT, agent-owned subscription from - the board's per-REPO target (Global Constraints). Proposed freeze-time - delta, BOTH halves (prose only in this Draft PR): (a) a DECISIONS.md - ledger row recording the table model + the rename, AND (b) a one-line - annotation at the ownership-layer DDL - (`compass-server-ownership-layer/design.md:983`, the `CREATE TABLE - forge_subscriptions`) pointing forward — "lands as + the board's per-REPO target (Global Constraints). The rename lands as a + two-half freeze-time delta, SPLIT across this PR and a follow-up: (a) a + DECISIONS.md ledger row recording the table model + the rename — this is + DL-163, and it RIDES THIS PR; and (b) a one-line forward annotation at the + ownership-layer DDL (`compass-server-ownership-layer/design.md:978`, above + the `CREATE TABLE forge_subscriptions`) — "lands as `agent_forge_subscriptions`; see compass-forge-poll-driver" — so a reader entering from the frozen ownership-layer record who greps - `forge_subscriptions` finds the mapping instead of nothing. No - DECISIONS.md or ownership-layer edit rides this Draft PR - (`Ledger-impact: none in this PR — Draft record`). + `forge_subscriptions` finds the mapping instead of nothing. Half (b) is + DEFERRED to SEA-1883, not folded here: it edits a frozen Active record, + which needs Matt's explicit call (flagged in OQ-C for his ratification at + this design-PR review). This PR's ledger delta is DL-161/DL-162/DL-163 in + DECISIONS.md. - **v1 population is the T4 boot seed reconcile** — `--forge-repos` inserted enabled where absent, `ON CONFLICT DO NOTHING`: additive, never destructive, never mutating an existing row — and the dynamic mutation @@ -1399,7 +1405,7 @@ GitHub-read slice and belonging to the future Linear-ingestion slice: `ForgeProviderForgejo = 3` (`store/issues.go:30-35`). T2 already adds `ForgeProviderLinear = 4` so the Go domain matches the 0015 CHECK domain — this half is closed by this slice; the future slice adds its producer. -- **A Linear `forge.Provider` client** (DL-051, `DECISIONS.md:126`: the +- **A Linear `forge.Provider` client** (DL-051, `DECISIONS.md:129`: the forge adapter is "GitHub first, Linear issues-only"; `ForgeRef.host` for Linear is the constant "linear.app", `compass.proto:711-712`).