Repository navigation
feat: serve projects over HTTP on Postgres, on the TUI's own rules - #17
Conversation
A second store needs the rules the HTTP contract carries, and every rule belongs in service so it lands once for both stores. service now resolves a UUID or a name for a project, and a UUID or a number for an item, and it filters by status, repo, limit and a completion window. Projects carry a kind and items and tasks carry a completion time. Refusals are typed sentences, so an API can answer the right status and a client can print the words. store/postgres implements store.Store over pgx with goose migrations. InTx takes an advisory lock first, the analog of SQLite's immediate transaction, so a rule's reads still hold at commit. A row trigger writes every change to audit_trail, reading the actor and request id from transaction-local settings bound from the context. service's suite runs on either store, chosen by PROJECTS_TEST_STORE. SQLite mode changes where the rules moved: completing an item with open tasks is refused, deleting a project refuses open orphans and removes completed ones, and reordering dense-ranks a project's positions. Agent-Name: claude-code-unattended Agent-Session: b6774e69-f156-4d1a-9713-9886fbbc39f7 Agent-Run: 01a1b74b-03f4-747a-9881-2a1e47809809 Worklog: 2401.1
projects-api answers the routes existing clients already call under /api/v1, in the answer shapes they already read, so moving a client is a change of address. api/wire holds those shapes apart from model, because a client depends on the contract and never on the server's types. api/schemas holds a JSON Schema per shape, and a test checks a real answer against each. A handler decodes, calls service and encodes. A partial update is read field by field, because only that tells a field left out from one sent as null. POST and GET /api/v1/undo are new, so every operation the TUI performs has a route. A person's client reaches every route and a service client reaches only what projects.items.read lists, by registered pattern. /health answers for the process and /ready for Postgres and the identity provider. TERM drains requests in flight and exits zero, and the logs are JSON on stdout. Agent-Name: claude-code-unattended Agent-Session: b6774e69-f156-4d1a-9713-9886fbbc39f7 Agent-Run: 01a1b74b-03f4-747a-9881-2a1e47809809 Worklog: 2401.1
A cluster runs the API from what a release publishes, so a release now holds an image and a kustomize artifact at the version go-semantic-release cut. One version names the TUI and the API, because a rule change in service moves both. Both artifacts are signed keylessly, and a deployment verifies them against this workflow's identity. deploy/migrate runs the schema migration as a Job before deploy/app rolls the pods. Migrating at startup would let two pods race, and a bad migration would crash-loop what is serving. CI renders and checks each manifest path as the release pins it, so a base that fails is never pushed. The README and the repo's instructions describe the API, and stop naming a sibling product where its name says nothing a reader needs. Agent-Name: claude-code-unattended Agent-Session: b6774e69-f156-4d1a-9713-9886fbbc39f7 Agent-Run: 01a1b74b-03f4-747a-9881-2a1e47809809 Worklog: 2401.1
A position counts every member of a project, and the TUI's list hides archived members. Sending the row's index in that list landed a drop one row too high for each archived member above it, and a move down one row did nothing. The drop now counts every member at or above the row it lands under. Agent-Name: claude-code-unattended Agent-Session: b6774e69-f156-4d1a-9713-9886fbbc39f7 Agent-Run: 01a1b74b-03f4-747a-9881-2a1e47809809 Worklog: 2401.1
testcontainers brings in klauspost/compress, moby/go-archive, x/crypto and x/text at versions govulncheck flags, and the Vulnerabilities job fails on them. Agent-Name: claude-code-unattended Agent-Session: b6774e69-f156-4d1a-9713-9886fbbc39f7 Agent-Run: 01a1b74b-03f4-747a-9881-2a1e47809809 Worklog: 2401.1
datapointchris
left a comment
datapointchris
left a comment
There was a problem hiding this comment.
PR review — system-design
Used: Cloned PR 17 at head. Read the Dockerfile, release.yml, validate.yml, deploy/app, deploy/migrate, release/pin-manifests, api/main.go, api/authentication, the routes' scope table, and goapiauth v0.2.0's RequireBearer. Built or ran nothing.
Standards: No rules cited. I ran no fleet standards search and found no new rule to propose.
Findings: 0 correctness · 0 design · 0 prose
No findings.
Findings as JSON
{
"used": "Cloned PR 17 at head. Read the Dockerfile, release.yml, validate.yml, deploy/app, deploy/migrate, release/pin-manifests, api/main.go, api/authentication, the routes' scope table, and goapiauth v0.2.0's RequireBearer. Built or ran nothing.",
"standards": "No rules cited. I ran no `fleet standards search` and found no new rule to propose.",
"findings": []
}This review does not gate merge.
datapointchris
left a comment
datapointchris
left a comment
There was a problem hiding this comment.
PR review — failure-paths
Used: Read the PR diff and the head's api/main.go, store/postgres/open.go and store.go, api/routes, release.yml, release/pin-manifests, release/install-tools and the deploy manifests. I did not run the failure; I traced it from the code.
Standards: Searched fleet standards search "request to unreachable dependency has no timeout hang". It returned go.md "Every HTTP request carries a deadline" and code-quality.md "A deadline over a polling loop bounds every call inside it". I did not read either in full.
Findings: 1 correctness · 0 design · 0 prose
Correctness
failure-paths.1 store/postgres/open.go:46 · api/settings/settings.go:28 · api/routes/routes.go:107 · store/postgres/store.go:59
Reach: unproven — a run with POSTGRES_HOST pointed at a blackholed address would settle it. The trigger is a Postgres that drops packets, such as a network partition, a firewall drop or a failover. Not a refused connection, which fails fast. The /ready probe and every request that opens a transaction reach it.
A Postgres that drops packets hangs /ready and every request, with no deadline on any of them.
postgres.Connect builds the URL with no connect_timeout. pgxpool.New is lazy, so no dial happens at startup.
readiness calls Ping with r.Context(). That context has no deadline. http.Server.WriteTimeout closes the connection but does not cancel the request context. The handler goroutine stays in the dial until the OS TCP timeout, roughly two minutes. Each kubelet probe adds another stuck goroutine.
InTx has the same gap. It calls pool.Begin(ctx) and then pg_advisory_xact_lock under the request context. A request waits on a dead or slow database, or on the single global write lock, until the client gives up. One slow writer holds the lock and every other write queues behind it. The pool can fill with waiting writers. Reads then wait on Acquire too.
The client sees a hung request, not a 503. The probe only reads 'not ready' because the kubelet's own 1s timeout fires.
This is the code-quality.md rule that a failure allowed to degrade costs only the unit that failed. Here it costs the whole pool.
Fix.
- Add
connect_timeouttoPostgres.URL(). - Wrap each readiness check in
context.WithTimeout, for example 2s, inroutes.readiness. - Bound
InTxwith a statement or lock timeout. Either setlock_timeoutinBindWriter, or wrap the request context incontext.WithTimeoutinscoped. - Map
context.DeadlineExceededto a 503 intable.fail.
Findings as JSON
{
"used": "Read the PR diff and the head's api/main.go, store/postgres/open.go and store.go, api/routes, release.yml, release/pin-manifests, release/install-tools and the deploy manifests. I did not run the failure; I traced it from the code.",
"standards": "Searched `fleet standards search \"request to unreachable dependency has no timeout hang\"`. It returned go.md \"Every HTTP request carries a deadline\" and code-quality.md \"A deadline over a polling loop bounds every call inside it\". I did not read either in full.",
"findings": [
{
"number": "failure-paths.1",
"section": "correctness",
"sites": [
"store/postgres/open.go:46",
"api/settings/settings.go:28",
"api/routes/routes.go:107",
"store/postgres/store.go:59"
],
"reach": "unproven — a run with `POSTGRES_HOST` pointed at a blackholed address would settle it. The trigger is a Postgres that drops packets, such as a network partition, a firewall drop or a failover. Not a refused connection, which fails fast. The `/ready` probe and every request that opens a transaction reach it.",
"body": "**A Postgres that drops packets hangs `/ready` and every request, with no deadline on any of them.**\n\n`postgres.Connect` builds the URL with no `connect_timeout`. `pgxpool.New` is lazy, so no dial happens at startup.\n\n`readiness` calls `Ping` with `r.Context()`. That context has no deadline. `http.Server.WriteTimeout` closes the connection but does not cancel the request context. The handler goroutine stays in the dial until the OS TCP timeout, roughly two minutes. Each kubelet probe adds another stuck goroutine.\n\n`InTx` has the same gap. It calls `pool.Begin(ctx)` and then `pg_advisory_xact_lock` under the request context. A request waits on a dead or slow database, or on the single global write lock, until the client gives up. One slow writer holds the lock and every other write queues behind it. The pool can fill with waiting writers. Reads then wait on `Acquire` too.\n\nThe client sees a hung request, not a 503. The probe only reads 'not ready' because the kubelet's own 1s timeout fires.\n\nThis is the code-quality.md rule that a failure allowed to degrade costs only the unit that failed. Here it costs the whole pool.\n\n**Fix.**\n- Add `connect_timeout` to `Postgres.URL()`.\n- Wrap each readiness check in `context.WithTimeout`, for example 2s, in `routes.readiness`.\n- Bound `InTx` with a statement or lock timeout. Either set `lock_timeout` in `BindWriter`, or wrap the request context in `context.WithTimeout` in `scoped`.\n- Map `context.DeadlineExceeded` to a 503 in `table.fail`."
}
]
}This review does not gate merge.
datapointchris
left a comment
datapointchris
left a comment
There was a problem hiding this comment.
PR review — stale-prose
Used: Read every added comment, doc, help and config sentence at head 9b69e86. Marshalled the wire structs in a temporary test. Ran listenUntilDone past its drain timeout in a temporary test. Applied the migration's Up on a throwaway postgres:16-alpine and timed clones and audit rows. The temporary tests are removed and the clone is clean.
Standards: Applied pull-requests.md § "Sentences the diff adds are checked for what goes stale, and the finding carries the rewrite"; I ran no other standards search and propose no rule.
Findings: 0 correctness · 0 design · 5 prose
Prose
stale-prose.1 README.md:351 · CLAUDE.md:312 · api/wire/wire.go:2
The client contract says an empty field is null, and lists and optional refusal fields answer otherwise. Kind: False.
The sentences:
README.md:351: "Every field is always sent, an empty one asnull."CLAUDE.md:312: "Every field is sent, an empty one asnull, and times are UTC with aZ."api/wire/wire.go:2-3: "every field is always sent, an empty one as null"
I marshalled the wire structs at the head. They returned:
wire.ProjectWithItemCount:..."completed_count":0,"repos":[]}wire.Error:{"error":"Not Found","message":"m"}wire.Probe:{"status":"ok"}
An empty list is [], so the sentence is false for lists. Map says so in the same file: "answers an empty list rather than null". model/project_item.go also tells the API to keep "sending []", because nil and empty mean different things to a pull. A maintainer following these three sentences would break that. errors, ctx and reason carry omitempty, so they are not always sent.
Rewrite for README.md:351 and CLAUDE.md:312: "A row's every field is always sent: an empty value as null, and an empty list as []." CLAUDE.md:312 keeps its clause about times.
Rewrite for wire.go: "every field of a row is always sent, an empty value as null and an empty list as [], so a reader indexing by name finds it."
stale-prose.2 README.md:9
The README says scripts reach the API that projects-api serves, and nothing points at projects-api yet. Kind: False.
The sentence, README.md:9-10: "Scripts and agents reach the same projects and items through the API. projects-api, in this repo, serves it on Postgres."
The diff removed the name of the separate server here. The two sentences now read as one API. The PR body says otherwise: "Nothing is pointed at them by this change" and "sync still reaches the old one". A reader debugging sync or a script would look in this repo for a server that is not answering them.
Rewrite: "Scripts and agents reach the same projects and items through the API the database is bound to. projects-api, in this repo, is one server of that API, on Postgres." This names no client and no migration state, so it holds when clients move.
stale-prose.3 CLAUDE.md:130
CLAUDE.md still places the by-status project order in the stores, and this branch moved it out. Kind: False, a sentence this diff rewrote and its own code contradicts.
The sentence, CLAUDE.md:129-131: "store/store.go states each on its method: a new row's position, an item's updated_at, a restore's handling of a row already there, and the order projects list in by status. Each store states them in its own queries.sql."
At the base, ListProjectsWithItemCount held that order, in a CASE p.status ORDER BY. That method is gone at the head. rg -i status store/store.go returns only ListProjectKinds and ListProjectStatuses. store/store.go:58 reads "ListProjects orders by position and then name", and both queries.sql files do the same. The status order now lives in model.SortProjectsForBoard, called from service/service.go:176.
Rewrite: "store/store.go states each on its method: a new row's position, an item's updated_at, and a restore's handling of a row already there. Each store states them in its own queries.sql." Delete the clause about the order.
stale-prose.4 store/postgres/migrations/00001_projects.sql:110 · README.md:330
Two absolute sentences state one outcome, and a branch of the code gives another. Kind: False.
store/postgres/migrations/00001_projects.sql:110: "Every insert, update and delete to a row of an audited table leaves one entry here". Line 133 of the same file contradicts it: "An update that changes nothing leaves no entry." On Postgres 16, I inserted a row and ran UPDATE projects SET name='a' WHERE name='a'. count(*) on audit_trail stayed at 1. A real rename took it to 2.
Rewrite: "Every insert, delete and changing update to a row of an audited table leaves one entry here, whatever wrote it: a statement, a cascade, or a person at a SQL prompt."
README.md:330: "TERM stops new requests, lets the ones in flight finish for up to 20 seconds, and exits zero." I ran listenUntilDone with a handler that outlasted the drain. It returned after 20 s with draining: context deadline exceeded. main turns any error from run into os.Exit(1) at api/main.go:41.
Rewrite: "TERM stops new requests and lets the ones in flight finish. It exits zero if they finish inside drainTimeout in api/main.go, and non-zero if one outlasts it."
stale-prose.5 store/postgres/postgrestest/postgrestest.go:3 · deploy/app/api.yaml:109 · deploy/app/api.yaml:75 · README.md:336 · CLAUDE.md:305
Measured numbers and code-owned values are written out where nothing keeps them level. Kind: Will go stale for the measurements, and A value the code holds for the rest.
Measured, with no command beside them:
postgrestest.go:3-4: "clones a migrated template, which costs a few milliseconds where migrating costs a hundred." This is already off.CREATE DATABASE ... TEMPLATEtook 26.7, 31.7 and 23.7 ms on a tmpfs Postgres 16. Rewrite: "and each test clones a migrated template, which is cheaper than migrating."deploy/app/api.yaml:109-110: "A resident set of about 22 MiB after a couple of thousand requests over a few hundred rows." Rewrite: delete this sentence, and open the next with "The room above the resident set is for a list answered to several clients at once."
Held by code:
deploy/app/api.yaml:75: "Covers the preStop sleep plus the API's 20 second drain, with room to spare."drainTimeoutowns the value. Rewrite: "Covers the preStop sleep plusdrainTimeoutin api/main.go, with room to spare."README.md:336-346: "the one scope isprojects.items.read", followed by seven route patterns.routes.ServiceScopesowns both, and no test compares the README with it. Rewrite: "It reaches only what its scopes list, inroutes.ServiceScopes." Delete the block.CLAUDE.md:305: "postgresteststarts onepostgres:16-alpineper test binary".postgrestest.go:75owns the tag. Rewrite: "postgresteststarts one Postgres container per test binary".
Findings as JSON
{
"used": "Read every added comment, doc, help and config sentence at head 9b69e86. Marshalled the wire structs in a temporary test. Ran listenUntilDone past its drain timeout in a temporary test. Applied the migration's Up on a throwaway postgres:16-alpine and timed clones and audit rows. The temporary tests are removed and the clone is clean.",
"standards": "Applied `pull-requests.md` § \"Sentences the diff adds are checked for what goes stale, and the finding carries the rewrite\"; I ran no other standards search and propose no rule.",
"findings": [
{
"number": "stale-prose.1",
"section": "prose",
"sites": [
"README.md:351",
"CLAUDE.md:312",
"api/wire/wire.go:2"
],
"reach": "",
"body": "**The client contract says an empty field is `null`, and lists and optional refusal fields answer otherwise.** Kind: False.\n\nThe sentences:\n- `README.md:351`: \"Every field is always sent, an empty one as `null`.\"\n- `CLAUDE.md:312`: \"Every field is sent, an empty one as `null`, and times are UTC with a `Z`.\"\n- `api/wire/wire.go:2-3`: \"every field is always sent, an empty one as null\"\n\nI marshalled the wire structs at the head. They returned:\n- `wire.ProjectWithItemCount`: `...\"completed_count\":0,\"repos\":[]}`\n- `wire.Error`: `{\"error\":\"Not Found\",\"message\":\"m\"}`\n- `wire.Probe`: `{\"status\":\"ok\"}`\n\nAn empty list is `[]`, so the sentence is false for lists. `Map` says so in the same file: \"answers an empty list rather than null\". `model/project_item.go` also tells the API to keep \"sending `[]`\", because nil and empty mean different things to a pull. A maintainer following these three sentences would break that. `errors`, `ctx` and `reason` carry `omitempty`, so they are not always sent.\n\nRewrite for `README.md:351` and `CLAUDE.md:312`: \"A row's every field is always sent: an empty value as `null`, and an empty list as `[]`.\" `CLAUDE.md:312` keeps its clause about times.\n\nRewrite for `wire.go`: \"every field of a row is always sent, an empty value as null and an empty list as [], so a reader indexing by name finds it.\""
},
{
"number": "stale-prose.2",
"section": "prose",
"sites": [
"README.md:9"
],
"reach": "",
"body": "**The README says scripts reach the API that `projects-api` serves, and nothing points at `projects-api` yet.** Kind: False.\n\nThe sentence, `README.md:9-10`: \"Scripts and agents reach the same projects and items through the API. `projects-api`, in this repo, serves it on Postgres.\"\n\nThe diff removed the name of the separate server here. The two sentences now read as one API. The PR body says otherwise: \"Nothing is pointed at them by this change\" and \"`sync` still reaches the old one\". A reader debugging sync or a script would look in this repo for a server that is not answering them.\n\nRewrite: \"Scripts and agents reach the same projects and items through the API the database is bound to. `projects-api`, in this repo, is one server of that API, on Postgres.\" This names no client and no migration state, so it holds when clients move."
},
{
"number": "stale-prose.3",
"section": "prose",
"sites": [
"CLAUDE.md:130"
],
"reach": "",
"body": "**`CLAUDE.md` still places the by-status project order in the stores, and this branch moved it out.** Kind: False, a sentence this diff rewrote and its own code contradicts.\n\nThe sentence, `CLAUDE.md:129-131`: \"`store/store.go` states each on its method: a new row's position, an item's `updated_at`, a restore's handling of a row already there, and the order projects list in by status. Each store states them in its own `queries.sql`.\"\n\nAt the base, `ListProjectsWithItemCount` held that order, in a `CASE p.status` `ORDER BY`. That method is gone at the head. `rg -i status store/store.go` returns only `ListProjectKinds` and `ListProjectStatuses`. `store/store.go:58` reads \"ListProjects orders by position and then name\", and both `queries.sql` files do the same. The status order now lives in `model.SortProjectsForBoard`, called from `service/service.go:176`.\n\nRewrite: \"`store/store.go` states each on its method: a new row's position, an item's `updated_at`, and a restore's handling of a row already there. Each store states them in its own `queries.sql`.\" Delete the clause about the order."
},
{
"number": "stale-prose.4",
"section": "prose",
"sites": [
"store/postgres/migrations/00001_projects.sql:110",
"README.md:330"
],
"reach": "",
"body": "**Two absolute sentences state one outcome, and a branch of the code gives another.** Kind: False.\n\n`store/postgres/migrations/00001_projects.sql:110`: \"Every insert, update and delete to a row of an audited table leaves one entry here\". Line 133 of the same file contradicts it: \"An update that changes nothing leaves no entry.\" On Postgres 16, I inserted a row and ran `UPDATE projects SET name='a' WHERE name='a'`. `count(*)` on `audit_trail` stayed at 1. A real rename took it to 2.\n\nRewrite: \"Every insert, delete and changing update to a row of an audited table leaves one entry here, whatever wrote it: a statement, a cascade, or a person at a SQL prompt.\"\n\n`README.md:330`: \"TERM stops new requests, lets the ones in flight finish for up to 20 seconds, and exits zero.\" I ran `listenUntilDone` with a handler that outlasted the drain. It returned after 20 s with `draining: context deadline exceeded`. `main` turns any error from `run` into `os.Exit(1)` at `api/main.go:41`.\n\nRewrite: \"TERM stops new requests and lets the ones in flight finish. It exits zero if they finish inside `drainTimeout` in `api/main.go`, and non-zero if one outlasts it.\""
},
{
"number": "stale-prose.5",
"section": "prose",
"sites": [
"store/postgres/postgrestest/postgrestest.go:3",
"deploy/app/api.yaml:109",
"deploy/app/api.yaml:75",
"README.md:336",
"CLAUDE.md:305"
],
"reach": "",
"body": "**Measured numbers and code-owned values are written out where nothing keeps them level.** Kind: Will go stale for the measurements, and A value the code holds for the rest.\n\nMeasured, with no command beside them:\n- `postgrestest.go:3-4`: \"clones a migrated template, which costs a few milliseconds where migrating costs a hundred.\" This is already off. `CREATE DATABASE ... TEMPLATE` took 26.7, 31.7 and 23.7 ms on a tmpfs Postgres 16. Rewrite: \"and each test clones a migrated template, which is cheaper than migrating.\"\n- `deploy/app/api.yaml:109-110`: \"A resident set of about 22 MiB after a couple of thousand requests over a few hundred rows.\" Rewrite: delete this sentence, and open the next with \"The room above the resident set is for a list answered to several clients at once.\"\n\nHeld by code:\n- `deploy/app/api.yaml:75`: \"Covers the preStop sleep plus the API's 20 second drain, with room to spare.\" `drainTimeout` owns the value. Rewrite: \"Covers the preStop sleep plus `drainTimeout` in api/main.go, with room to spare.\"\n- `README.md:336-346`: \"the one scope is `projects.items.read`\", followed by seven route patterns. `routes.ServiceScopes` owns both, and no test compares the README with it. Rewrite: \"It reaches only what its scopes list, in `routes.ServiceScopes`.\" Delete the block.\n- `CLAUDE.md:305`: \"`postgrestest` starts one `postgres:16-alpine` per test binary\". `postgrestest.go:75` owns the tag. Rewrite: \"`postgrestest` starts one Postgres container per test binary\"."
}
]
}This review does not gate merge.
datapointchris
left a comment
datapointchris
left a comment
There was a problem hiding this comment.
PR review — code-architecture
Used: Cloned the repo and checked out PR 17 at 9b69e86. Read service, store, api, sync, deploy, release and CI. Ran go list -deps ./api and rg searches for each fact's other sites. Ran neither the API nor the tests, so each behavior stated is read from the code.
Standards: Cited code-quality.md § "Ask whatever owns a fact, the function or enum that computes it, never duplicate its logic at a second site" and data.md § "Seed data is only the enum lookup rows, upserted on every deploy; content is imported through the CLI". Searches on refusal-to-status tables and on image gates before a tag found no rule that applies.
Findings: 0 correctness · 5 design · 0 prose
Design and architecture
code-architecture.1 service/service.go:165 · service/listing.go:66 · service/service.go:466 · service/listing.go:189 · service/service.go:1079 · service/listing.go:224 · service/service.go:1089 · service/listing.go:213 · model/project.go:91 · service/listing.go:98
Reach: The next change is the one the PR names under What this does not do: the CLI and TUI reaching this API. backend.Backend holds the TUI's half of each pair, and sync/backend.go:28 wraps it, so that client has to re-derive each TUI answer from an API answer. Any new read rule costs two edits until then. Four pairs already exist, which is the second instance.
Service answers each read twice, through two method families with different rules. The TUI's family serves backend.Backend. The API's family was added beside it.
| Read | TUI method | API method | Difference |
|---|---|---|---|
| Projects | ListProjectsByStatus |
ListProjects |
Only the API refuses an unknown status. Only the TUI ranks by status first, so all orders differently |
| Items in a project | ListItemsByProject |
ListProjectItems |
The TUI drops archived items. The API applies a filter |
| Search | Search |
SearchItems |
The TUI drops archived items. The API keeps them |
| Blocked | ListBlocked |
ListBlockedItems |
Each reads the store itself |
Each method reads the store and applies its own filter. CLAUDE.md states the goal as a new rule landing once. That holds across stores, and fails across these two families. It is a second owner under code-quality.md § "Ask whatever owns a fact, the function or enum that computes it, never duplicate its logic at a second site".
ListBlockerItems at service/listing.go:234 already has the right shape: it calls GetBlockers and adds the joins.
Fix: give each read one internal method that owns its filter and order. Make the other family a named call over it. ListProjectsByStatus becomes ListProjects plus the board sort. Search becomes the shared search minus archived rows.
code-architecture.2 sync/pull.go:170 · sync/pull.go:223 · store/sqlite/queries.sql:286 · store/sqlite/queries.sql:297 · sync/backend.go:91 · CLAUDE.md:126
Reach: The next change is any screen showing kind or completed_at. The PR says no screen shows completed_at yet. On a synced database, kind reads as the local value or the default life, and completed_at stays null for work finished on the server. This PR is the instance: it added both columns and neither reached the pull.
A pull writes project and item rows through a column list nothing ties to model. CLAUDE.md says a new column joins its undo record and its store's restore, and that TestEveryModelFieldSurvivesAnUndo enforces this. The sync upserts are a third writer of the same rows, and no test reaches them.
fetchJSON[[]model.Project] at sync/pull.go:93 and []model.ProjectItemDetail at sync/pull.go:101 decode into types holding Kind and CompletedAt. UpsertProject and UpsertItem then have no parameter for either field. createProjectPayload also sends no kind. rg 'Kind|CompletedAt' sync returns nothing in that package.
store/sqlite/schema.sql:48 declares the null completed_at on a mirrored item deliberately. Nothing declares the same for kind.
Fix: add both columns to the two upserts and to the create payload, or state the omission in one place. Add a field-coverage test over the pull, in the shape of TestEveryModelFieldSurvivesAnUndo, so the next column fails it.
code-architecture.3 store/sqlite/lookup_rows.sql:3 · store/sqlite/lookup_rows.sql:5 · store/postgres/migrations/00001_projects.sql:6 · store/postgres/migrations/00001_projects.sql:12 · store/sqlite/schema.sql:3 · service/listing.go:327 · model/project.go:15 · model/project.go:57 · model/project.go:92
Reach: The next kind costs two edits: lookup_rows.sql and a new goose migration. The next status costs those two plus ProjectIsTerminal and statusRank, whose defaults decide its behavior without a compile error. This PR is the instance for kinds, since it introduces the table with three values and promises more by insert.
Two files seed the kinds and statuses, and no test compares them. store/sqlite/schema.sql:3 and service/listing.go:327 both say that adding a kind or status is an insert. It is an insert per store, each in its own form. SQLite re-runs lookup_rows.sql on every open. Postgres seeds once inside migration 00001.
rg for ListProjectKinds, "chore", "build" and Known kinds across the test files returns nothing. The suite that runs on both stores therefore never compares these sets.
For statuses, the insert claim is also incomplete. SortProjectsForBoard ranks an unnamed status as closed. ProjectIsTerminal calls it not terminal. A new status would sort among closed projects and carry no closed_at.
The Postgres seed also departs from data.md § "Seed data is only the enum lookup rows, upserted on every deploy; content is imported through the CLI", because a migration runs once.
Fix: declare model.ProjectKinds and model.ProjectStatuses. Add one service test, which runs on both stores, asserting each store answers exactly those sets. Have projects-api migrate upsert the rows from those lists, as SQLite does on open.
code-architecture.4 api/routes/answers.go:71 · api/routes/answers.go:121 · model/errors.go:5
Reach: The next rule that moves into service with a new sentinel has to edit model/errors.go and statusOfRefusal in step. The PR's stated decision is that a rule the other API holds moves into service, and this PR adds many sentinels that way, so more are expected.
A refusal's HTTP answer lives in a table no build or test ties to the sentinel list. model/errors.go declares 21 sentinels and statusOfRefusal lists all 21 today. Their agreement rests on the author remembering.
fail walks the table, then falls through to the fault branch. A new sentinel left out of the table is logged as an error and answers 500 with Internal server error: *model.Refusal, so a client's mistake reads as a server fault. This comes from reading fail, not from a run.
routes_test.go never names statusOfRefusal. TestEveryAnswerMatchesItsSchema drives only a few refusals, and none of them is a new sentinel's.
Fix: export the sentinels as one list in model, and add a test in package routes that fails on an entry with no row. Alternatively, have fail treat any *model.Refusal missing from the table as a client refusal and log the missing row.
code-architecture.5 .dockerignore:2 · Dockerfile:12 · .github/workflows/validate.yml:324 · .github/workflows/release.yml:36 · .github/workflows/release.yml:111 · CLAUDE.md:335
Reach: The next package service or api imports, directly or transitively, costs a .dockerignore line. config shows this already: it is in the image only because repos imports it. If the line is missed, api-publish fails after the tag, and a re-run rebuilds the same broken context. The fix needs a new commit and a new version, so the failed version has binaries and no image.
.dockerignore holds a second copy of the API's import graph, and the first check of that copy runs after the tag exists. The allowlist names api, backend, config, graph, model, repos, service and store. go list -deps ./api gives the same module directories today, so the two agree.
CLAUDE.md:335 states the rule by hand: a new directory joins the allowlist, or the image build fails. rg 'docker|buildx' across validate.yml, .pre-commit-config.yaml and Taskfile.yml returns nothing, so no pull request builds the image.
The only build is in api-publish, which needs: release. The manifests get a gate before the tag, in the deploy-manifests job. The image does not.
Fix: add a docker buildx build with no push to validate.yml, so the release's existing validate gate covers it. A deny-list .dockerignore would also remove the copy, at the cost of a larger context.
Findings as JSON
{
"used": "Cloned the repo and checked out PR 17 at 9b69e86. Read service, store, api, sync, deploy, release and CI. Ran `go list -deps ./api` and `rg` searches for each fact's other sites. Ran neither the API nor the tests, so each behavior stated is read from the code.",
"standards": "Cited code-quality.md § \"Ask whatever owns a fact, the function or enum that computes it, never duplicate its logic at a second site\" and data.md § \"Seed data is only the enum lookup rows, upserted on every deploy; content is imported through the CLI\". Searches on refusal-to-status tables and on image gates before a tag found no rule that applies.",
"findings": [
{
"number": "code-architecture.1",
"section": "design",
"sites": [
"service/service.go:165",
"service/listing.go:66",
"service/service.go:466",
"service/listing.go:189",
"service/service.go:1079",
"service/listing.go:224",
"service/service.go:1089",
"service/listing.go:213",
"model/project.go:91",
"service/listing.go:98"
],
"reach": "The next change is the one the PR names under What this does not do: the CLI and TUI reaching this API. `backend.Backend` holds the TUI's half of each pair, and `sync/backend.go:28` wraps it, so that client has to re-derive each TUI answer from an API answer. Any new read rule costs two edits until then. Four pairs already exist, which is the second instance.",
"body": "**`Service` answers each read twice, through two method families with different rules.** The TUI's family serves `backend.Backend`. The API's family was added beside it.\n\n| Read | TUI method | API method | Difference |\n| --- | --- | --- | --- |\n| Projects | `ListProjectsByStatus` | `ListProjects` | Only the API refuses an unknown status. Only the TUI ranks by status first, so `all` orders differently |\n| Items in a project | `ListItemsByProject` | `ListProjectItems` | The TUI drops archived items. The API applies a filter |\n| Search | `Search` | `SearchItems` | The TUI drops archived items. The API keeps them |\n| Blocked | `ListBlocked` | `ListBlockedItems` | Each reads the store itself |\n\nEach method reads the store and applies its own filter. `CLAUDE.md` states the goal as a new rule landing once. That holds across stores, and fails across these two families. It is a second owner under code-quality.md § \"Ask whatever owns a fact, the function or enum that computes it, never duplicate its logic at a second site\".\n\n`ListBlockerItems` at `service/listing.go:234` already has the right shape: it calls `GetBlockers` and adds the joins.\n\n**Fix:** give each read one internal method that owns its filter and order. Make the other family a named call over it. `ListProjectsByStatus` becomes `ListProjects` plus the board sort. `Search` becomes the shared search minus archived rows."
},
{
"number": "code-architecture.2",
"section": "design",
"sites": [
"sync/pull.go:170",
"sync/pull.go:223",
"store/sqlite/queries.sql:286",
"store/sqlite/queries.sql:297",
"sync/backend.go:91",
"CLAUDE.md:126"
],
"reach": "The next change is any screen showing `kind` or `completed_at`. The PR says no screen shows `completed_at` yet. On a synced database, `kind` reads as the local value or the default `life`, and `completed_at` stays null for work finished on the server. This PR is the instance: it added both columns and neither reached the pull.",
"body": "**A pull writes project and item rows through a column list nothing ties to `model`.** `CLAUDE.md` says a new column joins its undo record and its store's restore, and that `TestEveryModelFieldSurvivesAnUndo` enforces this. The sync upserts are a third writer of the same rows, and no test reaches them.\n\n`fetchJSON[[]model.Project]` at `sync/pull.go:93` and `[]model.ProjectItemDetail` at `sync/pull.go:101` decode into types holding `Kind` and `CompletedAt`. `UpsertProject` and `UpsertItem` then have no parameter for either field. `createProjectPayload` also sends no kind. `rg 'Kind|CompletedAt' sync` returns nothing in that package.\n\n`store/sqlite/schema.sql:48` declares the null `completed_at` on a mirrored item deliberately. Nothing declares the same for `kind`.\n\n**Fix:** add both columns to the two upserts and to the create payload, or state the omission in one place. Add a field-coverage test over the pull, in the shape of `TestEveryModelFieldSurvivesAnUndo`, so the next column fails it."
},
{
"number": "code-architecture.3",
"section": "design",
"sites": [
"store/sqlite/lookup_rows.sql:3",
"store/sqlite/lookup_rows.sql:5",
"store/postgres/migrations/00001_projects.sql:6",
"store/postgres/migrations/00001_projects.sql:12",
"store/sqlite/schema.sql:3",
"service/listing.go:327",
"model/project.go:15",
"model/project.go:57",
"model/project.go:92"
],
"reach": "The next kind costs two edits: `lookup_rows.sql` and a new goose migration. The next status costs those two plus `ProjectIsTerminal` and `statusRank`, whose defaults decide its behavior without a compile error. This PR is the instance for kinds, since it introduces the table with three values and promises more by insert.",
"body": "**Two files seed the kinds and statuses, and no test compares them.** `store/sqlite/schema.sql:3` and `service/listing.go:327` both say that adding a kind or status is an insert. It is an insert per store, each in its own form. SQLite re-runs `lookup_rows.sql` on every open. Postgres seeds once inside migration `00001`.\n\n`rg` for `ListProjectKinds`, `\"chore\"`, `\"build\"` and `Known kinds` across the test files returns nothing. The suite that runs on both stores therefore never compares these sets.\n\nFor statuses, the insert claim is also incomplete. `SortProjectsForBoard` ranks an unnamed status as closed. `ProjectIsTerminal` calls it not terminal. A new status would sort among closed projects and carry no `closed_at`.\n\nThe Postgres seed also departs from data.md § \"Seed data is only the enum lookup rows, upserted on every deploy; content is imported through the CLI\", because a migration runs once.\n\n**Fix:** declare `model.ProjectKinds` and `model.ProjectStatuses`. Add one service test, which runs on both stores, asserting each store answers exactly those sets. Have `projects-api migrate` upsert the rows from those lists, as SQLite does on open."
},
{
"number": "code-architecture.4",
"section": "design",
"sites": [
"api/routes/answers.go:71",
"api/routes/answers.go:121",
"model/errors.go:5"
],
"reach": "The next rule that moves into `service` with a new sentinel has to edit `model/errors.go` and `statusOfRefusal` in step. The PR's stated decision is that a rule the other API holds moves into `service`, and this PR adds many sentinels that way, so more are expected.",
"body": "**A refusal's HTTP answer lives in a table no build or test ties to the sentinel list.** `model/errors.go` declares 21 sentinels and `statusOfRefusal` lists all 21 today. Their agreement rests on the author remembering.\n\n`fail` walks the table, then falls through to the fault branch. A new sentinel left out of the table is logged as an error and answers 500 with `Internal server error: *model.Refusal`, so a client's mistake reads as a server fault. This comes from reading `fail`, not from a run.\n\n`routes_test.go` never names `statusOfRefusal`. `TestEveryAnswerMatchesItsSchema` drives only a few refusals, and none of them is a new sentinel's.\n\n**Fix:** export the sentinels as one list in `model`, and add a test in package `routes` that fails on an entry with no row. Alternatively, have `fail` treat any `*model.Refusal` missing from the table as a client refusal and log the missing row."
},
{
"number": "code-architecture.5",
"section": "design",
"sites": [
".dockerignore:2",
"Dockerfile:12",
".github/workflows/validate.yml:324",
".github/workflows/release.yml:36",
".github/workflows/release.yml:111",
"CLAUDE.md:335"
],
"reach": "The next package `service` or `api` imports, directly or transitively, costs a `.dockerignore` line. `config` shows this already: it is in the image only because `repos` imports it. If the line is missed, `api-publish` fails after the tag, and a re-run rebuilds the same broken context. The fix needs a new commit and a new version, so the failed version has binaries and no image.",
"body": "**`.dockerignore` holds a second copy of the API's import graph, and the first check of that copy runs after the tag exists.** The allowlist names `api`, `backend`, `config`, `graph`, `model`, `repos`, `service` and `store`. `go list -deps ./api` gives the same module directories today, so the two agree.\n\n`CLAUDE.md:335` states the rule by hand: a new directory joins the allowlist, or the image build fails. `rg 'docker|buildx'` across `validate.yml`, `.pre-commit-config.yaml` and `Taskfile.yml` returns nothing, so no pull request builds the image.\n\nThe only build is in `api-publish`, which `needs: release`. The manifests get a gate before the tag, in the `deploy-manifests` job. The image does not.\n\n**Fix:** add a `docker buildx build` with no push to `validate.yml`, so the release's existing `validate` gate covers it. A deny-list `.dockerignore` would also remove the copy, at the cost of a larger context."
}
]
}This review does not gate merge.
datapointchris
left a comment
datapointchris
left a comment
There was a problem hiding this comment.
PR review — useless-test
Used: Built PR 17 in the clone and ran go test -short on ./api/..., ./service/, ./store/... and ./tui/. Ran ./store/postgres/ and PROJECTS_TEST_STORE=postgres ./service/ on a private Postgres. Applied and reverted five mutations: a reworded null refusal, the name index dropped, the number index dropped, the number index collapsing nulls, and an audited undo_log. Read every added and edited test beside its code, and the existing tests over the changed code. Kept because each is the only pin: TestEveryAnswerMatchesItsSchema pins the answer shapes API clients read, and TestEveryModelFieldSurvivesAnUndo pins silent field loss across delete and undo.
Standards: Cited testing.md § "Test behavior the user cares about, not what this commit happened to change", testing.md § "Never assert on rendered output — assert the value it was built from", and pull-requests.md § "Tests the diff adds are checked for whether they test the program or the change".
Findings: 0 correctness · 3 design · 0 prose
Design and architecture
useless-test.1 store/sqlite/open_test.go:331 · store/sqlite/open_test.go:435
Reach: The diff changes Open and migrate, which both tests call, though not the two indexes they exercise. The next change to idx_items_number or idx_projects_name_active pays to read these tests. The name clause holds its own copy of the index DDL, so it keeps passing when indexes.sql changes.
Two tests check SQLite's documented index behavior rather than this program's rules.
Delete these:
store/sqlite/open_test.go,TestOpen_NumberIndexAllowsManyUnnumberedItems: delete the whole test. Its null half checks that a SQLite unique index allows many NULLs, and its own comment says "SQLite does". Its duplicate half repeatsTestOpen_ExistingDatabaseGainsTheUniquenessRules.store/sqlite/open_test.go,TestMigrate_ReplacesTheGlobalNameConstraint: delete the comment andCREATE UNIQUE INDEXat lines 435-443, and thep3insert at lines 449-453. The test types the partial index itself, then checks that SQLite enforces it. Keep thep2insert, because it alone shows the migration removed the globalUNIQUE.
What the mutations returned:
- Removing
idx_projects_name_activefromindexes.sqlfailedTestOpen_ExistingDatabaseGainsTheUniquenessRules.TestMigrate_ReplacesTheGlobalNameConstraintstill passed, so thep3clause cannot see the real index. - Removing
idx_items_numberfailedTestOpen_ExistingDatabaseGainsTheUniquenessRules, so the duplicate number rule stays pinned. - Changing the index to
IFNULL(number,0)failed many service tests, such asTestDependencyCycleDetection, which creates three unnumbered items. So the behavior of many items waiting for a number stays pinned.
No replacement test is needed.
useless-test.2 api/routes/routes_test.go:286
Reach: PATCH /api/v1/project-items/{id} with title: null reaches this clause. The next rewording of messageCannotBeNull in api/routes/request_input.go fails it with nothing wrong.
A refusal test compares a typed sentence byte for byte.
In api/routes/routes_test.go, TestAPartialUpdateTellsNullFromLeftOut, delete the want constant at line 286 and the refused.Message != want clause at line 287. Delete the Message field from the refused struct too. Keep the rest of the test, because it is the only pin of null clearing a field and left-out keeping it.
The sentence is the literal messageCannotBeNull, with title: joined on the front. Line 290 already asserts the field name through loc. The 422 and the single-error count stay on line 287.
I reworded messageCannotBeNull, and this was the only test in ./api/... to fail. The refusal behavior was unchanged.
The contract exception does not apply. The PR body says wording was left unmatched because it is "a sentence no client parses". This falls under testing.md § "Never assert on rendered output — assert the value it was built from".
If the test should name the kind of refusal, assert errors[0].type == "value_error". That is a stable key the handler sets.
useless-test.3 store/postgres/store_test.go:69
Reach: The next change that adds an undo_log trigger fails this clause, and it is a deliberate change. Nothing a user or an operator reads changes when it fails.
An audit test asserts that this diff left one trigger out.
In store/postgres/store_test.go, TestEveryWriteIsAuditedWithItsWriter, delete the seen["undo_log insert"] clause at lines 69-71. Keep the rest of the test, because it pins the writer on each audit row and the cascade being recorded.
The clause restates a decision the PR body lists: "undo_log has no audit trigger." It has the same shape as the testing.md example where a class no longer renders. The migration comment already records the reason.
I added an undo_log audit trigger to the migration and ran both suites on a private Postgres. This clause was the only failure in ./store/postgres/. PROJECTS_TEST_STORE=postgres go test ./service/ still passed every rule.
No replacement test is needed. Audit growth is a storage decision, and it is reviewed in the migration.
Findings as JSON
{
"used": "Built PR 17 in the clone and ran `go test -short` on `./api/...`, `./service/`, `./store/...` and `./tui/`. Ran `./store/postgres/` and `PROJECTS_TEST_STORE=postgres ./service/` on a private Postgres. Applied and reverted five mutations: a reworded null refusal, the name index dropped, the number index dropped, the number index collapsing nulls, and an audited `undo_log`. Read every added and edited test beside its code, and the existing tests over the changed code. Kept because each is the only pin: `TestEveryAnswerMatchesItsSchema` pins the answer shapes API clients read, and `TestEveryModelFieldSurvivesAnUndo` pins silent field loss across delete and undo.",
"standards": "Cited `testing.md` § \"Test behavior the user cares about, not what this commit happened to change\", `testing.md` § \"Never assert on rendered output — assert the value it was built from\", and `pull-requests.md` § \"Tests the diff adds are checked for whether they test the program or the change\".",
"findings": [
{
"number": "useless-test.1",
"section": "design",
"sites": [
"store/sqlite/open_test.go:331",
"store/sqlite/open_test.go:435"
],
"reach": "The diff changes `Open` and `migrate`, which both tests call, though not the two indexes they exercise. The next change to `idx_items_number` or `idx_projects_name_active` pays to read these tests. The name clause holds its own copy of the index DDL, so it keeps passing when `indexes.sql` changes.",
"body": "**Two tests check SQLite's documented index behavior rather than this program's rules.**\n\nDelete these:\n\n- `store/sqlite/open_test.go`, `TestOpen_NumberIndexAllowsManyUnnumberedItems`: delete the whole test. Its null half checks that a SQLite unique index allows many NULLs, and its own comment says \"SQLite does\". Its duplicate half repeats `TestOpen_ExistingDatabaseGainsTheUniquenessRules`.\n- `store/sqlite/open_test.go`, `TestMigrate_ReplacesTheGlobalNameConstraint`: delete the comment and `CREATE UNIQUE INDEX` at lines 435-443, and the `p3` insert at lines 449-453. The test types the partial index itself, then checks that SQLite enforces it. Keep the `p2` insert, because it alone shows the migration removed the global `UNIQUE`.\n\nWhat the mutations returned:\n\n- Removing `idx_projects_name_active` from `indexes.sql` failed `TestOpen_ExistingDatabaseGainsTheUniquenessRules`. `TestMigrate_ReplacesTheGlobalNameConstraint` still passed, so the `p3` clause cannot see the real index.\n- Removing `idx_items_number` failed `TestOpen_ExistingDatabaseGainsTheUniquenessRules`, so the duplicate number rule stays pinned.\n- Changing the index to `IFNULL(number,0)` failed many service tests, such as `TestDependencyCycleDetection`, which creates three unnumbered items. So the behavior of many items waiting for a number stays pinned.\n\nNo replacement test is needed."
},
{
"number": "useless-test.2",
"section": "design",
"sites": [
"api/routes/routes_test.go:286"
],
"reach": "`PATCH /api/v1/project-items/{id}` with `title: null` reaches this clause. The next rewording of `messageCannotBeNull` in `api/routes/request_input.go` fails it with nothing wrong.",
"body": "**A refusal test compares a typed sentence byte for byte.**\n\nIn `api/routes/routes_test.go`, `TestAPartialUpdateTellsNullFromLeftOut`, delete the `want` constant at line 286 and the `refused.Message != want` clause at line 287. Delete the `Message` field from the `refused` struct too. Keep the rest of the test, because it is the only pin of null clearing a field and left-out keeping it.\n\nThe sentence is the literal `messageCannotBeNull`, with `title: ` joined on the front. Line 290 already asserts the field name through `loc`. The 422 and the single-error count stay on line 287.\n\nI reworded `messageCannotBeNull`, and this was the only test in `./api/...` to fail. The refusal behavior was unchanged.\n\nThe contract exception does not apply. The PR body says wording was left unmatched because it is \"a sentence no client parses\". This falls under `testing.md` § \"Never assert on rendered output — assert the value it was built from\".\n\nIf the test should name the kind of refusal, assert `errors[0].type == \"value_error\"`. That is a stable key the handler sets."
},
{
"number": "useless-test.3",
"section": "design",
"sites": [
"store/postgres/store_test.go:69"
],
"reach": "The next change that adds an `undo_log` trigger fails this clause, and it is a deliberate change. Nothing a user or an operator reads changes when it fails.",
"body": "**An audit test asserts that this diff left one trigger out.**\n\nIn `store/postgres/store_test.go`, `TestEveryWriteIsAuditedWithItsWriter`, delete the `seen[\"undo_log insert\"]` clause at lines 69-71. Keep the rest of the test, because it pins the writer on each audit row and the cascade being recorded.\n\nThe clause restates a decision the PR body lists: \"`undo_log` has no audit trigger.\" It has the same shape as the `testing.md` example where a class no longer renders. The migration comment already records the reason.\n\nI added an `undo_log` audit trigger to the migration and ran both suites on a private Postgres. This clause was the only failure in `./store/postgres/`. `PROJECTS_TEST_STORE=postgres go test ./service/` still passed every rule.\n\nNo replacement test is needed. Audit growth is a storage decision, and it is reviewed in the migration."
}
]
}This review does not gate merge.
datapointchris
left a comment
datapointchris
left a comment
There was a problem hiding this comment.
PR review — contracts
Used: Built projects-api at head and ran --version, --help and version. Probed routes through a temporary test on the production auth wrapper, with real signed tokens. Ran the catalog's coverage and go_routes from a clone against head. Searched every registered repo for route, scope and client-id callers, and compared the old API's routes, scopes and query parameters.
Standards: Cited data.md § "A new source of data gets its row in the data catalog in the change that creates it", cli-design.md § "The shape a --json read or an endpoint returns is a JSON Schema file that the data catalog publishes", infrastructure.md § "One OAuth client per (machine × app), isolated by the client_id prefix", and api-design.md § "A client depends on the wire contract, never on the server's types". Also checked api-design.md § "An error answers {error, message}" and § "A product API owns /api/v1 on its app's host", and the head conforms to both.
Findings: 1 correctness · 3 design · 0 prose
Correctness
contracts.1 api/routes/routes.go:58 · api/schemas/project.schema.json:3 · store/postgres/migrations/00001_projects.sql:1 · fleet-data-catalog:producers.yaml:75 · fleet-data-catalog:producers.yaml:98 · fleet-data-catalog:producers.yaml:113 · fleet-data-catalog:producers.yaml:186
Reach: catalog-contracts coverage, which reads every active repo. I ran it from a catalog clone with --checkouts-dir holding only this head. It exits 1 once this merges and the checkout it reads reaches main.
Merging makes the catalog's coverage check fail. The PR adds an HTTP service, ten output schemas and a Postgres store. The catalog lists none of them.
The run returned:
http-api projects 1 files, first api/routes/routes.go
output-schema projects 10 files, first api/schemas/error.schema.json
2 sources the catalog does not account for
It exited 1. data.md § "A new source of data gets its row in the data catalog in the change that creates it" says a new store or HTTP service gets a row in producers.yaml. It also says the check fails a run and files an inbox row.
Coverage does not name the new store, but the same rule requires its row and its retention. The existing projects store row also goes stale. It has engine sqlite and says location: ichrisbirch's Postgres.
Fix: land a catalog change with or before this merge. It takes three rows:
{name: projects, repo: projects, framework: go-net-http, source: api}underhttp_apis- repo: projectsunderoutput_schema_repos- a
sql_storesrow forstore/postgres/migrations, with its retention and theaudit_trailtable in view
Design and architecture
contracts.2 api/routes/routes.go:66 · api/routes/routes.go:94 · fleet-data-catalog:src/fleet_data_catalog/http_routes.py:22
Reach: The catalog's go-net-http route reader, as soon as the http_apis row exists. I ran go_routes on this head's api directory. The next change it costs is that row, which publishes a wrong route list. Each cross-fleet route question then reads that list.
The route spelling hides 26 of the API's 28 routes from the catalog. Each route is written "GET "+prefix+"/projects". The reader's GO_MUX_CONSTANT pattern captures only the constant, so the path after it is lost.
go_routes on this head returned 6 rows:
GET /health
GET /ready
GET /api/v1
POST /api/v1
PATCH /api/v1
DELETE /api/v1
The catalog would record those six. Listing the service clears the coverage check and still publishes a wrong surface, with no failure.
The spelling also defeats a plain search. rg 'GET /api/v1/project-items' finds nothing in routes.go.
Fix: write each pattern as one literal, such as "GET /api/v1/projects", in New and in ServiceScopes. The alternative is teaching GO_MUX_CONSTANT the trailing literal in the catalog.
contracts.3 api/authentication/authentication.go:18 · api/routes/routes.go:25 · README.md:9 · README.md:334 · doit:api/src/doit_api/settings.py:154 · doit:api/src/doit_api/settings.py:193 · doit:api/src/doit_api/readers/icb.py:140 · doit:api/src/doit_api/readers/icb.py:238 · homelab:pyinfra/fleet_service_clients.py:34 · homelab:pyinfra/doit_service.py:38 · ichrisbirch:cli/internal/config/config.go:38
Reach: No caller is pointed here yet, so nothing breaks today. The next change is the cutover. It reaches icb-svc-doit, icb-svc-fleet-runs and the icb CLI, and each needs a new client and scope. I ran the first token shape below through the production auth wrapper. A projects-svc- client holding the old scope is the second.
The PR says the routes and answers match, but no existing credential is admitted, and nothing says so. The new prefix and scope follow infrastructure.md § "One OAuth client per (machine × app), isolated by the client_id prefix". The gap is disclosure. "What this does not do" never lists it, and README.md:9 tells scripts and agents they reach items through this API.
A signed icb-svc-fleet-runs token holding icb.project-items.read returned 401:
{"code":"invalid_token","error":"Unauthorized","message":"the bearer token is not valid"}That message reads as a bad token rather than a wrong product. A projects-svc-x token holding the old scope returned 403 with its scopes reach no route.
The cost is largest in doit. It reads /project-items on the same icb client and token as tasks, articles and books, so the project reads need a second app block and a second reader client.
Fix: add a line to "What this does not do" and to the README section at line 334. It should say that moving a caller takes a new projects-cli- or projects-svc- client, registered in homelab, holding projects.items.read. It should also name the three callers above.
contracts.4 api/schemas/project.schema.json:3 · api/schemas/project-item.schema.json:3 · api/wire/wire.go:36
Reach: The next reader of the catalog contract, and the next reshaped answer. A client has no field saying which shape it holds. No existing caller breaks.
The ten schemas carry no schema_version, against the rule for endpoint shapes. rg schema_version api/ README.md CLAUDE.md returns nothing. cli-design.md § "The shape a --json read or an endpoint returns is a JSON Schema file that the data catalog publishes" says schema_version appears in the document. ~/webapps/nomad/api/schemas/overview.schema.json does this.
The PR's reason is that "existing clients read these shapes as they are". That reason does not rule out a new field, because api-design.md § "A client depends on the wire contract, never on the server's types" has clients ignore unknown fields.
A list route answers a bare array, so the field has nowhere to go on those routes. The :v1 in each $id and the /api/v1 prefix are the only version signals the head has.
Fix: choose one and write it down.
- Add
schema_versionto the object answers. - Or state in
CLAUDE.mdthat/api/v2is this API's version signal, and cite the rule as the exception. Also put nomad's sentence in the schemas: a field added is not a new version, and a field reshaped or removed is.
Findings as JSON
{
"used": "Built `projects-api` at head and ran `--version`, `--help` and `version`. Probed routes through a temporary test on the production auth wrapper, with real signed tokens. Ran the catalog's `coverage` and `go_routes` from a clone against head. Searched every registered repo for route, scope and client-id callers, and compared the old API's routes, scopes and query parameters.",
"standards": "Cited `data.md` § \"A new source of data gets its row in the data catalog in the change that creates it\", `cli-design.md` § \"The shape a --json read or an endpoint returns is a JSON Schema file that the data catalog publishes\", `infrastructure.md` § \"One OAuth client per (machine × app), isolated by the client_id prefix\", and `api-design.md` § \"A client depends on the wire contract, never on the server's types\". Also checked `api-design.md` § \"An error answers {error, message}\" and § \"A product API owns /api/v1 on its app's host\", and the head conforms to both.",
"findings": [
{
"number": "contracts.1",
"section": "correctness",
"sites": [
"api/routes/routes.go:58",
"api/schemas/project.schema.json:3",
"store/postgres/migrations/00001_projects.sql:1",
"fleet-data-catalog:producers.yaml:75",
"fleet-data-catalog:producers.yaml:98",
"fleet-data-catalog:producers.yaml:113",
"fleet-data-catalog:producers.yaml:186"
],
"reach": "`catalog-contracts coverage`, which reads every active repo. I ran it from a catalog clone with `--checkouts-dir` holding only this head. It exits 1 once this merges and the checkout it reads reaches main.",
"body": "**Merging makes the catalog's coverage check fail.** The PR adds an HTTP service, ten output schemas and a Postgres store. The catalog lists none of them.\n\nThe run returned:\n\n```text\nhttp-api projects 1 files, first api/routes/routes.go\noutput-schema projects 10 files, first api/schemas/error.schema.json\n2 sources the catalog does not account for\n```\n\nIt exited 1. `data.md` § \"A new source of data gets its row in the data catalog in the change that creates it\" says a new store or HTTP service gets a row in `producers.yaml`. It also says the check fails a run and files an inbox row.\n\nCoverage does not name the new store, but the same rule requires its row and its retention. The existing `projects` store row also goes stale. It has engine sqlite and says `location: ichrisbirch's Postgres`.\n\n**Fix:** land a catalog change with or before this merge. It takes three rows:\n- `{name: projects, repo: projects, framework: go-net-http, source: api}` under `http_apis`\n- `- repo: projects` under `output_schema_repos`\n- a `sql_stores` row for `store/postgres/migrations`, with its retention and the `audit_trail` table in view"
},
{
"number": "contracts.2",
"section": "design",
"sites": [
"api/routes/routes.go:66",
"api/routes/routes.go:94",
"fleet-data-catalog:src/fleet_data_catalog/http_routes.py:22"
],
"reach": "The catalog's `go-net-http` route reader, as soon as the `http_apis` row exists. I ran `go_routes` on this head's `api` directory. The next change it costs is that row, which publishes a wrong route list. Each cross-fleet route question then reads that list.",
"body": "**The route spelling hides 26 of the API's 28 routes from the catalog.** Each route is written `\"GET \"+prefix+\"/projects\"`. The reader's `GO_MUX_CONSTANT` pattern captures only the constant, so the path after it is lost.\n\n`go_routes` on this head returned 6 rows:\n\n```text\nGET /health\nGET /ready\nGET /api/v1\nPOST /api/v1\nPATCH /api/v1\nDELETE /api/v1\n```\n\nThe catalog would record those six. Listing the service clears the coverage check and still publishes a wrong surface, with no failure.\n\nThe spelling also defeats a plain search. `rg 'GET /api/v1/project-items'` finds nothing in `routes.go`.\n\n**Fix:** write each pattern as one literal, such as `\"GET /api/v1/projects\"`, in `New` and in `ServiceScopes`. The alternative is teaching `GO_MUX_CONSTANT` the trailing literal in the catalog."
},
{
"number": "contracts.3",
"section": "design",
"sites": [
"api/authentication/authentication.go:18",
"api/routes/routes.go:25",
"README.md:9",
"README.md:334",
"doit:api/src/doit_api/settings.py:154",
"doit:api/src/doit_api/settings.py:193",
"doit:api/src/doit_api/readers/icb.py:140",
"doit:api/src/doit_api/readers/icb.py:238",
"homelab:pyinfra/fleet_service_clients.py:34",
"homelab:pyinfra/doit_service.py:38",
"ichrisbirch:cli/internal/config/config.go:38"
],
"reach": "No caller is pointed here yet, so nothing breaks today. The next change is the cutover. It reaches `icb-svc-doit`, `icb-svc-fleet-runs` and the `icb` CLI, and each needs a new client and scope. I ran the first token shape below through the production auth wrapper. A `projects-svc-` client holding the old scope is the second.",
"body": "**The PR says the routes and answers match, but no existing credential is admitted, and nothing says so.** The new prefix and scope follow `infrastructure.md` § \"One OAuth client per (machine × app), isolated by the client_id prefix\". The gap is disclosure. \"What this does not do\" never lists it, and README.md:9 tells scripts and agents they reach items through this API.\n\nA signed `icb-svc-fleet-runs` token holding `icb.project-items.read` returned 401:\n\n```json\n{\"code\":\"invalid_token\",\"error\":\"Unauthorized\",\"message\":\"the bearer token is not valid\"}\n```\n\nThat message reads as a bad token rather than a wrong product. A `projects-svc-x` token holding the old scope returned 403 with `its scopes reach no route`.\n\nThe cost is largest in doit. It reads `/project-items` on the same `icb` client and token as tasks, articles and books, so the project reads need a second app block and a second reader client.\n\n**Fix:** add a line to \"What this does not do\" and to the README section at line 334. It should say that moving a caller takes a new `projects-cli-` or `projects-svc-` client, registered in homelab, holding `projects.items.read`. It should also name the three callers above."
},
{
"number": "contracts.4",
"section": "design",
"sites": [
"api/schemas/project.schema.json:3",
"api/schemas/project-item.schema.json:3",
"api/wire/wire.go:36"
],
"reach": "The next reader of the catalog contract, and the next reshaped answer. A client has no field saying which shape it holds. No existing caller breaks.",
"body": "**The ten schemas carry no `schema_version`, against the rule for endpoint shapes.** `rg schema_version api/ README.md CLAUDE.md` returns nothing. `cli-design.md` § \"The shape a --json read or an endpoint returns is a JSON Schema file that the data catalog publishes\" says `schema_version` appears in the document. `~/webapps/nomad/api/schemas/overview.schema.json` does this.\n\nThe PR's reason is that \"existing clients read these shapes as they are\". That reason does not rule out a new field, because `api-design.md` § \"A client depends on the wire contract, never on the server's types\" has clients ignore unknown fields.\n\nA list route answers a bare array, so the field has nowhere to go on those routes. The `:v1` in each `$id` and the `/api/v1` prefix are the only version signals the head has.\n\n**Fix:** choose one and write it down.\n- Add `schema_version` to the object answers.\n- Or state in `CLAUDE.md` that `/api/v2` is this API's version signal, and cite the rule as the exception. Also put nomad's sentence in the schemas: a field added is not a new version, and a field reshaped or removed is."
}
]
}This review does not gate merge.
datapointchris
left a comment
datapointchris
left a comment
There was a problem hiding this comment.
PR review — fleet-patterns
Used: Read PR 17 at 9b69e86 and ran hadolint on its Dockerfile, which passed. Compared it with psyche, slate, doit, fleet, ypl, meso, nomad, learning, ichrisbirch, goapiauth, forge, reusable-workflows and fleet-data-catalog. No API was started. These matched and gave no finding: the metadata path, because goapiauth exempts it; the migration Job layout; the root-module API, which fleet also has; and the probes.
Standards: Cited data.md § "A new source of data gets its row in the data catalog in the change that creates it", cli-design.md § "The shape a --json read or an endpoint returns is a JSON Schema file that the data catalog publishes", telemetry.md § "A record read later carries its writer's traceparent and baggage, and its reader links back", release.md § "A release.yml calls a shared workflow and pins nothing itself", and repo-structure.md § "The per-repo build surface is declared in a registry, not detected".
Findings: 0 correctness · 4 design · 0 prose
Design and architecture
fleet-patterns.1 store/postgres/migrations/00001_projects.sql:112 · store/postgres/queries.sql:9 · store/postgres/store_test.go:58 · slate:src/slate/alembic/versions/1a7f84e9a913_keep_the_trace_and_baggage_of_each_.py:56 · slate:src/slate/models/record_change.py:97 · slate:tests/models/test_audit_trail.py:46 · ichrisbirch:ichrisbirch/models/record_change.py:82 · ichrisbirch:tests/ichrisbirch/models/test_icb_audit_trail.py:45
Reach: Every write through the new API lands in audit_trail through InTx. Without trace fields, a write cannot be joined to the trace that made it. The next table added to store/postgres/migrations gets no audit trigger and no test fails. The next fix to the trigger function must be made by hand in slate, ichrisbirch and projects.
The audit trail copies slate's trigger from before slate added trace context, and drops the coverage test both siblings hold.
audit_trail_record() here matches slate's function body line for line. Slate added traceparent and baggage columns on 2026-11-02, in revision 1a7f84e9a913, and its after_begin listener binds them. Projects has neither: rg 'traceparent|baggage' api store README.md returns nothing. telemetry.md § "A record read later carries its writer's traceparent and baggage, and its reader links back" asks for those fields. The pull request body gives no reason for leaving them out.
Slate and ichrisbirch both run test_every_table_is_audited_but_what_opts_out, which compares every table with its trigger. Projects asserts five named rows from one flow, so a new table without a trigger passes.
Slate is the better solution, because it holds both the trace fields and the coverage test.
Fix: add both trace columns and their settings to 00001_projects.sql now, since no deployment has applied it. Bind them in BindWriter. Add a test that lists pg_trigger against every table except undo_log, audit_trail and goose's version table.
fleet-patterns.2 api/main.go:1 · store/postgres/migrations/00001_projects.sql:1 · api/schemas/project.schema.json:3 · fleet-data-catalog:producers.yaml:75 · fleet-data-catalog:producers.yaml:95 · fleet-data-catalog:producers.yaml:98 · fleet-data-catalog:producers.yaml:186 · nomad:api/schemas/overview.schema.json:8
Reach: Merging leaves a new HTTP service, a new Postgres store and ten schema files out of the catalog. producers.yaml still gives this store as sqlite, located in ichrisbirch's Postgres. It would still say that when the service changes hands. Any reader of these shapes has no contract to point at, so it copies the fields.
The new API, its Postgres store and its schemas have no row in the data catalog.
fleet-data-catalog/producers.yaml lists psyche, ypl, learning, meso and nomad as go-net-http APIs, and projects is absent. sql_queries names only store/sqlite/queries.sql. output_schema_repos does not list projects, so api/schemas produces no contract, while nomad's API schemas do. The local checkout holds no row, and a search of that repo's open pull requests for projects returned none.
data.md § "A new source of data gets its row in the data catalog in the change that creates it" requires a row for a new HTTP service or store. It says catalog-contracts coverage exits 1 on an unlisted source. I did not run that command against this head.
The pull request also drops schema_version, because existing clients read these shapes as they are. nomad carries it, and cli-design.md § "The shape a --json read or an endpoint returns is a JSON Schema file that the data catalog publishes" asks for it. List answers here are bare arrays, which have no place for the field. That exception belongs in the repo's CLAUDE.md, stated as an exception to the rule.
Fix: open a companion change to producers.yaml. It adds an http_apis row with source: api, a sql_queries row for store/postgres/queries.sql, a sql_stores row using migrations with its retention, and a repo: projects line under output_schema_repos.
fleet-patterns.3 release/install-tools:31 · release/pin-manifests:67 · .github/workflows/release.yml:36 · .github/workflows/validate.yml:336 · slate:release/install-tools:31 · slate:release/pin-manifests:67 · slate:.github/workflows/release.yml:238 · psyche:release/install-tools:31 · psyche:release/pin-manifests:67 · doit:release/install-tools:31 · doit:release/pin-manifests:59
Reach: The next cosign, flux or kustomize bump is a hand edit of version and SHA-256 in four repos: doit, psyche, slate and projects. Forge's pins hold no entry for these tools, so nothing moves or checks them together. The next fix to api-publish lands in slate and has to be re-found here, as its refs/tags/cli/ note already differs.
A fourth copy of the release scripts and publish job lands here, with no shared owner.
diff against psyche and slate shows release/pin-manifests differs only in the image name. release/install-tools differs only in its pin list and usage text. The api-publish job up to signing differs from slate's only in needs, env, permissions, comments and the build-arg name. The deploy-manifests block in validate.yml is the same as slate's. doit carries install-tools and an older single-path pin-manifests.
release.md § "A release.yml calls a shared workflow and pins nothing itself" exists so that a pin moves in one commit. It leaves a publishing job with the caller, so the copy breaks no rule as written. See the proposed rule.
Following slate was right for this change. The cost is one more copy to edit.
Fix: move install-tools, pin-manifests and the publish steps into reusable-workflows, with the image name as an input, and have the four repos call them. A follow-up can do this. Until then, the pull request body should say this is a fourth copy.
fleet-patterns.4 Dockerfile:1 · .pre-commit-config.yaml:309 · terminal-library-fleet:repos.json · slate:.pre-commit-config.yaml:304 · psyche:.pre-commit-config.yaml:350
Reach: The next edit to Dockerfile commits with no hadolint run in pre-commit or CI. The image first builds in api-publish, after the tag exists. So a bad base or instruction surfaces after the release is cut.
The repo now ships an image, but its registry entry declares no docker component.
repos-registry lists projects with go and actions components only. psyche and slate each declare {stack: docker}, and forge generates a generated:docker hadolint block for them. rg docker .pre-commit-config.yaml on this head returns nothing.
I ran hadolint by hand with the generated block's arguments, and the Dockerfile passes today. The gap is the next edit, not this one. repo-structure.md § "The per-repo build surface is declared in a registry, not detected" makes the registry the declaration.
sql_dialect stays sqlite, and this pull request hand-writes a custom Postgres sqlfluff block with a .sqlfluffignore to match. That workaround has a stated reason, and I do not report it.
Fix: add {"stack": "docker", "dir": "."} to projects in the registry, then regenerate with forge in this pull request.
Findings as JSON
{
"used": "Read PR 17 at 9b69e86 and ran hadolint on its Dockerfile, which passed. Compared it with psyche, slate, doit, fleet, ypl, meso, nomad, learning, ichrisbirch, goapiauth, forge, reusable-workflows and fleet-data-catalog. No API was started. These matched and gave no finding: the metadata path, because goapiauth exempts it; the migration Job layout; the root-module API, which fleet also has; and the probes.",
"standards": "Cited `data.md` § \"A new source of data gets its row in the data catalog in the change that creates it\", `cli-design.md` § \"The shape a --json read or an endpoint returns is a JSON Schema file that the data catalog publishes\", `telemetry.md` § \"A record read later carries its writer's traceparent and baggage, and its reader links back\", `release.md` § \"A release.yml calls a shared workflow and pins nothing itself\", and `repo-structure.md` § \"The per-repo build surface is declared in a registry, not detected\".",
"findings": [
{
"number": "fleet-patterns.1",
"section": "design",
"sites": [
"store/postgres/migrations/00001_projects.sql:112",
"store/postgres/queries.sql:9",
"store/postgres/store_test.go:58",
"slate:src/slate/alembic/versions/1a7f84e9a913_keep_the_trace_and_baggage_of_each_.py:56",
"slate:src/slate/models/record_change.py:97",
"slate:tests/models/test_audit_trail.py:46",
"ichrisbirch:ichrisbirch/models/record_change.py:82",
"ichrisbirch:tests/ichrisbirch/models/test_icb_audit_trail.py:45"
],
"reach": "Every write through the new API lands in `audit_trail` through `InTx`. Without trace fields, a write cannot be joined to the trace that made it. The next table added to `store/postgres/migrations` gets no audit trigger and no test fails. The next fix to the trigger function must be made by hand in slate, ichrisbirch and projects.",
"body": "**The audit trail copies slate's trigger from before slate added trace context, and drops the coverage test both siblings hold.**\n\n`audit_trail_record()` here matches slate's function body line for line. Slate added `traceparent` and `baggage` columns on 2026-11-02, in revision `1a7f84e9a913`, and its `after_begin` listener binds them. Projects has neither: `rg 'traceparent|baggage' api store README.md` returns nothing. `telemetry.md` § \"A record read later carries its writer's traceparent and baggage, and its reader links back\" asks for those fields. The pull request body gives no reason for leaving them out.\n\nSlate and ichrisbirch both run `test_every_table_is_audited_but_what_opts_out`, which compares every table with its trigger. Projects asserts five named rows from one flow, so a new table without a trigger passes.\n\nSlate is the better solution, because it holds both the trace fields and the coverage test.\n\n**Fix**: add both trace columns and their settings to `00001_projects.sql` now, since no deployment has applied it. Bind them in `BindWriter`. Add a test that lists `pg_trigger` against every table except `undo_log`, `audit_trail` and goose's version table."
},
{
"number": "fleet-patterns.2",
"section": "design",
"sites": [
"api/main.go:1",
"store/postgres/migrations/00001_projects.sql:1",
"api/schemas/project.schema.json:3",
"fleet-data-catalog:producers.yaml:75",
"fleet-data-catalog:producers.yaml:95",
"fleet-data-catalog:producers.yaml:98",
"fleet-data-catalog:producers.yaml:186",
"nomad:api/schemas/overview.schema.json:8"
],
"reach": "Merging leaves a new HTTP service, a new Postgres store and ten schema files out of the catalog. `producers.yaml` still gives this store as sqlite, located in ichrisbirch's Postgres. It would still say that when the service changes hands. Any reader of these shapes has no contract to point at, so it copies the fields.",
"body": "**The new API, its Postgres store and its schemas have no row in the data catalog.**\n\n`fleet-data-catalog/producers.yaml` lists psyche, ypl, learning, meso and nomad as `go-net-http` APIs, and projects is absent. `sql_queries` names only `store/sqlite/queries.sql`. `output_schema_repos` does not list projects, so `api/schemas` produces no contract, while nomad's API schemas do. The local checkout holds no row, and a search of that repo's open pull requests for `projects` returned none.\n\n`data.md` § \"A new source of data gets its row in the data catalog in the change that creates it\" requires a row for a new HTTP service or store. It says `catalog-contracts coverage` exits 1 on an unlisted source. I did not run that command against this head.\n\nThe pull request also drops `schema_version`, because existing clients read these shapes as they are. nomad carries it, and `cli-design.md` § \"The shape a --json read or an endpoint returns is a JSON Schema file that the data catalog publishes\" asks for it. List answers here are bare arrays, which have no place for the field. That exception belongs in the repo's `CLAUDE.md`, stated as an exception to the rule.\n\n**Fix**: open a companion change to `producers.yaml`. It adds an `http_apis` row with `source: api`, a `sql_queries` row for `store/postgres/queries.sql`, a `sql_stores` row using `migrations` with its retention, and a `repo: projects` line under `output_schema_repos`."
},
{
"number": "fleet-patterns.3",
"section": "design",
"sites": [
"release/install-tools:31",
"release/pin-manifests:67",
".github/workflows/release.yml:36",
".github/workflows/validate.yml:336",
"slate:release/install-tools:31",
"slate:release/pin-manifests:67",
"slate:.github/workflows/release.yml:238",
"psyche:release/install-tools:31",
"psyche:release/pin-manifests:67",
"doit:release/install-tools:31",
"doit:release/pin-manifests:59"
],
"reach": "The next cosign, flux or kustomize bump is a hand edit of version and SHA-256 in four repos: doit, psyche, slate and projects. Forge's pins hold no entry for these tools, so nothing moves or checks them together. The next fix to `api-publish` lands in slate and has to be re-found here, as its `refs/tags/cli/` note already differs.",
"body": "**A fourth copy of the release scripts and publish job lands here, with no shared owner.**\n\n`diff` against psyche and slate shows `release/pin-manifests` differs only in the image name. `release/install-tools` differs only in its pin list and usage text. The `api-publish` job up to signing differs from slate's only in `needs`, env, permissions, comments and the build-arg name. The `deploy-manifests` block in `validate.yml` is the same as slate's. doit carries `install-tools` and an older single-path `pin-manifests`.\n\n`release.md` § \"A release.yml calls a shared workflow and pins nothing itself\" exists so that a pin moves in one commit. It leaves a publishing job with the caller, so the copy breaks no rule as written. See the proposed rule.\n\nFollowing slate was right for this change. The cost is one more copy to edit.\n\n**Fix**: move `install-tools`, `pin-manifests` and the publish steps into `reusable-workflows`, with the image name as an input, and have the four repos call them. A follow-up can do this. Until then, the pull request body should say this is a fourth copy."
},
{
"number": "fleet-patterns.4",
"section": "design",
"sites": [
"Dockerfile:1",
".pre-commit-config.yaml:309",
"terminal-library-fleet:repos.json",
"slate:.pre-commit-config.yaml:304",
"psyche:.pre-commit-config.yaml:350"
],
"reach": "The next edit to `Dockerfile` commits with no hadolint run in pre-commit or CI. The image first builds in `api-publish`, after the tag exists. So a bad base or instruction surfaces after the release is cut.",
"body": "**The repo now ships an image, but its registry entry declares no `docker` component.**\n\n`repos-registry` lists projects with `go` and `actions` components only. psyche and slate each declare `{stack: docker}`, and forge generates a `generated:docker` hadolint block for them. `rg docker .pre-commit-config.yaml` on this head returns nothing.\n\nI ran hadolint by hand with the generated block's arguments, and the Dockerfile passes today. The gap is the next edit, not this one. `repo-structure.md` § \"The per-repo build surface is declared in a registry, not detected\" makes the registry the declaration.\n\n`sql_dialect` stays `sqlite`, and this pull request hand-writes a custom Postgres sqlfluff block with a `.sqlfluffignore` to match. That workaround has a stated reason, and I do not report it.\n\n**Fix**: add `{\"stack\": \"docker\", \"dir\": \".\"}` to projects in the registry, then regenerate with forge in this pull request."
}
]
}This review does not gate merge.
datapointchris
left a comment
datapointchris
left a comment
There was a problem hiding this comment.
PR review — G1
Used: Ran go test ./..., built projects-api, ran migrate and serve on a private Postgres, sent real tokens through the API on Postgres and SQLite, drove TUI moves, ran the real sync engine against the API, ran pin-manifests, and did not build the image or run api-publish.
Standards: Cited data.md § "A migration that replaces an identity seeds its successor from the identitys own counter, never from MAX of the column"; searches on API input validation, stores disagreeing, position ties and rules wired per surface found no rule to cite.
Findings: 4 correctness · 1 design · 0 prose
Correctness
G1.1 tui/app.go:824
Reach: Any TUI database holding two items on one position. The single-row ReorderItem at 132335a wrote those ties, and the PR body's replay table counts position ties in production data. A sync pull mirrors the server's positions.
A drop below a tied row does not land where it was dropped. The body says a moved item lands where it is dropped. reorderItemCmd counts every member with member.Position <= above.Position. That count includes any row tied with above, and a tied row can sit below the drop.
I drove the keys through App.Update over SQLite. The seed was alpha 0, bravo 1, charlie 1, delta 2. I pressed G, m, k, then Enter on delta.
while moving: [alpha bravo delta charlie]
after: [alpha bravo charlie delta]
The item snaps back to where it started. TestAMovedItemLandsWhereItIsDroppedBelowAnArchivedOne misses this because its seed comes from CreateItem, which never ties.
Fix: merge the unarchived and archived lists and sort them by position, created_at and id, the store's own order. Remove the moved item, then use the index of above.ID plus one. Add a test with a tied seed.
G1.2 sync/backend.go:237 · service/service.go:969
Reach: The TUI with sync on: a person adds a task and checks it off before the next pull. sync/backend.go is unchanged on this branch, so the old API receives the same payload with no id.
A task checked off before the next pull comes back unchecked. SyncBackend.CreateTask generates a local id, then pushes only item_id and title. The server therefore assigns a different id. The later PATCH names the local id and gets a 404, and the push code drops the op on a 404. The next pull replaces the local row with the server's unchecked one.
I ran the real sync.Engine and SyncBackend against this branch's API on Postgres.
POST .../tasks {"item_id":"…","title":"t"} -> 201 {"id":"…1d5f-7844-8b2d-e051b53db4a1"…}
PATCH .../tasks/…1d5e-7979-a9ea-f8f66575a20f {"completed":true} -> 404
local tasks before pull: id …a20f, completed true
local tasks after pull: id …db4a1, completed false
The user's check is lost with no error. I did not run this against the old API.
This branch makes the fix small, because CreateProjectItemTask.ID and POST …/tasks now take an id. Fix: add ID to taskPayload and send result.ID, as CreateItem and CreateProject already do. The file is outside this diff, so this can be its own one-commit change.
G1.3 api/main.go:86 · api/settings/settings.go:71 · api/routes/answers.go:96 · README.md:292 · CLAUDE.md:191
Reach: POST /api/v1/projects or a rename through PATCH /api/v1/projects/{id}, from any script or agent, once the manifests are applied. The README names scripts and agents as this API's clients.
The API drops the repo-name ban the TUI enforces. api/main.go:86 builds the service with AssigningItemNumbers alone. RefusingRepoNames is set only in the root main.go, at lines 48 and 119. api/settings has no registry setting. A project named projects is refused in the TUI and has no refusal path through this API. I read this from the wiring and did not send that request.
statusOfRefusal maps ErrRepoNamedProject to 422, but no deployed path can raise it. routes_test.go builds its service the same way, so no test notices.
Two sentences are now false:
- README: "It runs the rules the TUI runs, over a Postgres store instead of SQLite, so a rule behaves the same in both."
- CLAUDE.md: "Enforcement is on the service (
RefusingRepoNames), the one path every create and rename takes."
Fix: add a REPOS_REGISTRY setting to settings.Server, deliver the file through the ConfigMap, and pass the option when it is set. If the gap is deliberate, delete the mapping and rewrite the README sentence: "It runs the rules the TUI runs, except the repo-name ban, which reads a registry file this API has no copy of."
G1.4 store/postgres/store.go:181 · store/postgres/store.go:164 · store/postgres/store.go:459 · api/routes/request_input.go:183 · service/service.go:571 · api/routes/answers.go:127
Reach: unproven — a survey of what the web page, CLI and agents send would settle it. The triggers are a note holding a NUL byte, one project twice in project_ids (for instance by id and by name), and a position above int32.
Three inputs get past service, and two of them behave differently per store. Review point 1 in the body asks whether any rule depends on a store's accidental behavior. These do. I ran each through the routes with a real token.
| Input | Postgres | SQLite |
|---|---|---|
PATCH /projects/{id} with position 4294967297 |
200, stored as 1 | 200, stored as 4294967297 |
POST /projects with position 2147483648 |
201, stored as -2147483648 | not run |
POST /project-items with a title holding \u0000 |
500 | 201 |
project_ids naming one project twice |
500 | 500 |
The wrapped position is the worst case, because a wrong value is written and answered as success. partialInt decodes a 64-bit int, and the store casts it with int32().
Every 500 answers Internal server error: *fmt.wrapError, which tells a client nothing.
A 500 also matters to sync, because sync/push.go retries a 5xx. I wrote a NUL note through SyncBackend against this API. The queue held at PendingCount:2, with the next write stuck behind the note on every retry.
Fix: have service refuse a NUL in text and a position outside int32 with a 422. Collapse repeated projects in CreateItem after resolving them. Make the 500 body name the request id, which the log already holds.
Design and architecture
G1.5 store/postgres/migrations/00001_projects.sql:2 · store/postgres/migrations/00001_projects.sql:33 · store/postgres/store.go:111 · deploy/migrate/migration.yaml:1
Reach: The next change: moving production onto this API. The body's opening states that aim, and its Risk section says no service changes hands yet. The README tells an operator to apply migrate/ and then app/.
Neither route onto production data works from what this PR ships. The body's replay restored a production copy, but that program is not in this repo. "What this does not do" never mentions the move.
Route 1 is migrating the existing database. ichrisbirch/models/project.py already declares project_kinds, projects and item_numbers, so 00001 collides with them. I ran projects-api migrate against a stub database holding only project_kinds. It returned partial migration error … relation "project_kinds" already exists and exited 1. The Job would retry up to backoffLimit and then fail.
Route 2 is a new database with the rows restored. item_numbers then still starts at 100000, whatever numbers the rows hold. With 100000 and 100001 restored, nextval returned 100000. With one row at 100000, POST /project-items answered 500 on uq_project_items_number, and the retry succeeded with 100001. That costs one failed create per number already held, and sync retries each 5xx.
Reading MAX(number) is not enough either. In the other API, item_numbers is shared with other tables, so its counter can pass every surviving item. data.md § "A migration that replaces an identity seeds its successor from the identitys own counter, never from MAX of the column" says a handle must never repeat.
The cutover will have to invent an adoption step and a sequence seed after this schema has shipped. Fix: ship the move in this repo, with the restore steps and a setval from the old sequence's own counter. Name the unique constraint in duplicateError so a collision is not an anonymous 500. If that belongs to a later PR, say so under "What this does not do".
Findings as JSON
{
"used": "Ran `go test ./...`, built `projects-api`, ran `migrate` and serve on a private Postgres, sent real tokens through the API on Postgres and SQLite, drove TUI moves, ran the real sync engine against the API, ran `pin-manifests`, and did not build the image or run `api-publish`.",
"standards": "Cited `data.md` § \"A migration that replaces an identity seeds its successor from the identitys own counter, never from MAX of the column\"; searches on API input validation, stores disagreeing, position ties and rules wired per surface found no rule to cite.",
"findings": [
{
"number": "G1.1",
"section": "correctness",
"sites": [
"tui/app.go:824"
],
"reach": "Any TUI database holding two items on one position. The single-row `ReorderItem` at `132335a` wrote those ties, and the PR body's replay table counts position ties in production data. A sync pull mirrors the server's positions.",
"body": "**A drop below a tied row does not land where it was dropped.** The body says a moved item lands where it is dropped. `reorderItemCmd` counts every member with `member.Position <= above.Position`. That count includes any row tied with `above`, and a tied row can sit below the drop.\n\nI drove the keys through `App.Update` over SQLite. The seed was alpha 0, bravo 1, charlie 1, delta 2. I pressed `G`, `m`, `k`, then Enter on delta.\n\n```text\nwhile moving: [alpha bravo delta charlie]\nafter: [alpha bravo charlie delta]\n```\n\nThe item snaps back to where it started. `TestAMovedItemLandsWhereItIsDroppedBelowAnArchivedOne` misses this because its seed comes from `CreateItem`, which never ties.\n\nFix: merge the unarchived and archived lists and sort them by position, created_at and id, the store's own order. Remove the moved item, then use the index of `above.ID` plus one. Add a test with a tied seed."
},
{
"number": "G1.2",
"section": "correctness",
"sites": [
"sync/backend.go:237",
"service/service.go:969"
],
"reach": "The TUI with sync on: a person adds a task and checks it off before the next pull. `sync/backend.go` is unchanged on this branch, so the old API receives the same payload with no id.",
"body": "**A task checked off before the next pull comes back unchecked.** `SyncBackend.CreateTask` generates a local id, then pushes only `item_id` and `title`. The server therefore assigns a different id. The later `PATCH` names the local id and gets a 404, and the push code drops the op on a 404. The next pull replaces the local row with the server's unchecked one.\n\nI ran the real `sync.Engine` and `SyncBackend` against this branch's API on Postgres.\n\n```text\nPOST .../tasks {\"item_id\":\"…\",\"title\":\"t\"} -> 201 {\"id\":\"…1d5f-7844-8b2d-e051b53db4a1\"…}\nPATCH .../tasks/…1d5e-7979-a9ea-f8f66575a20f {\"completed\":true} -> 404\nlocal tasks before pull: id …a20f, completed true\nlocal tasks after pull: id …db4a1, completed false\n```\n\nThe user's check is lost with no error. I did not run this against the old API.\n\nThis branch makes the fix small, because `CreateProjectItemTask.ID` and `POST …/tasks` now take an id. Fix: add `ID` to `taskPayload` and send `result.ID`, as `CreateItem` and `CreateProject` already do. The file is outside this diff, so this can be its own one-commit change."
},
{
"number": "G1.3",
"section": "correctness",
"sites": [
"api/main.go:86",
"api/settings/settings.go:71",
"api/routes/answers.go:96",
"README.md:292",
"CLAUDE.md:191"
],
"reach": "`POST /api/v1/projects` or a rename through `PATCH /api/v1/projects/{id}`, from any script or agent, once the manifests are applied. The README names scripts and agents as this API's clients.",
"body": "**The API drops the repo-name ban the TUI enforces.** `api/main.go:86` builds the service with `AssigningItemNumbers` alone. `RefusingRepoNames` is set only in the root `main.go`, at lines 48 and 119. `api/settings` has no registry setting. A project named `projects` is refused in the TUI and has no refusal path through this API. I read this from the wiring and did not send that request.\n\n`statusOfRefusal` maps `ErrRepoNamedProject` to 422, but no deployed path can raise it. `routes_test.go` builds its service the same way, so no test notices.\n\nTwo sentences are now false:\n\n- README: \"It runs the rules the TUI runs, over a Postgres store instead of SQLite, so a rule behaves the same in both.\"\n- CLAUDE.md: \"Enforcement is on the service (`RefusingRepoNames`), the one path every create and rename takes.\"\n\nFix: add a `REPOS_REGISTRY` setting to `settings.Server`, deliver the file through the ConfigMap, and pass the option when it is set. If the gap is deliberate, delete the mapping and rewrite the README sentence: \"It runs the rules the TUI runs, except the repo-name ban, which reads a registry file this API has no copy of.\""
},
{
"number": "G1.4",
"section": "correctness",
"sites": [
"store/postgres/store.go:181",
"store/postgres/store.go:164",
"store/postgres/store.go:459",
"api/routes/request_input.go:183",
"service/service.go:571",
"api/routes/answers.go:127"
],
"reach": "unproven — a survey of what the web page, CLI and agents send would settle it. The triggers are a note holding a NUL byte, one project twice in `project_ids` (for instance by id and by name), and a position above int32.",
"body": "**Three inputs get past `service`, and two of them behave differently per store.** Review point 1 in the body asks whether any rule depends on a store's accidental behavior. These do. I ran each through the routes with a real token.\n\n| Input | Postgres | SQLite |\n| --- | --- | --- |\n| `PATCH /projects/{id}` with `position` 4294967297 | 200, stored as 1 | 200, stored as 4294967297 |\n| `POST /projects` with `position` 2147483648 | 201, stored as -2147483648 | not run |\n| `POST /project-items` with a title holding `\\u0000` | 500 | 201 |\n| `project_ids` naming one project twice | 500 | 500 |\n\nThe wrapped position is the worst case, because a wrong value is written and answered as success. `partialInt` decodes a 64-bit `int`, and the store casts it with `int32()`.\n\nEvery 500 answers `Internal server error: *fmt.wrapError`, which tells a client nothing.\n\nA 500 also matters to sync, because `sync/push.go` retries a 5xx. I wrote a NUL note through `SyncBackend` against this API. The queue held at `PendingCount:2`, with the next write stuck behind the note on every retry.\n\nFix: have `service` refuse a NUL in text and a position outside int32 with a 422. Collapse repeated projects in `CreateItem` after resolving them. Make the 500 body name the request id, which the log already holds."
},
{
"number": "G1.5",
"section": "design",
"sites": [
"store/postgres/migrations/00001_projects.sql:2",
"store/postgres/migrations/00001_projects.sql:33",
"store/postgres/store.go:111",
"deploy/migrate/migration.yaml:1"
],
"reach": "The next change: moving production onto this API. The body's opening states that aim, and its Risk section says no service changes hands yet. The README tells an operator to apply `migrate/` and then `app/`.",
"body": "**Neither route onto production data works from what this PR ships.** The body's replay restored a production copy, but that program is not in this repo. \"What this does not do\" never mentions the move.\n\n**Route 1 is migrating the existing database.** `ichrisbirch/models/project.py` already declares `project_kinds`, `projects` and `item_numbers`, so `00001` collides with them. I ran `projects-api migrate` against a stub database holding only `project_kinds`. It returned `partial migration error … relation \"project_kinds\" already exists` and exited 1. The Job would retry up to `backoffLimit` and then fail.\n\n**Route 2 is a new database with the rows restored.** `item_numbers` then still starts at 100000, whatever numbers the rows hold. With 100000 and 100001 restored, `nextval` returned 100000. With one row at 100000, `POST /project-items` answered 500 on `uq_project_items_number`, and the retry succeeded with 100001. That costs one failed create per number already held, and sync retries each 5xx.\n\nReading `MAX(number)` is not enough either. In the other API, `item_numbers` is shared with other tables, so its counter can pass every surviving item. `data.md` § \"A migration that replaces an identity seeds its successor from the identitys own counter, never from MAX of the column\" says a handle must never repeat.\n\nThe cutover will have to invent an adoption step and a sequence seed after this schema has shipped. Fix: ship the move in this repo, with the restore steps and a `setval` from the old sequence's own counter. Name the unique constraint in `duplicateError` so a collision is not an anonymous 500. If that belongs to a later PR, say so under \"What this does not do\"."
}
]
}This review does not gate merge.
There was a problem hiding this comment.
PR review — synthesis
Used: the 27 findings of round 1, from 8 review(s), and nothing else
Standards: none, because this step reads only the findings
Findings: 0 correctness · 5 design · 0 prose
Design and architecture
synthesis.1 explains fleet-patterns.2 · contracts.1
The new API, its Postgres store and its schemas have no row in the data catalog's producers.yaml. Both findings name the same missing catalog rows and the same data.md rule.
synthesis.2 explains G1.3 · stale-prose.1 · stale-prose.2 · stale-prose.3
Prose in README and CLAUDE.md states a guarantee that the code does not hold. The API lacks the repo-name ban, empty lists are not null, and the API scope and status order claims are stale.
synthesis.3 explains G1.5 · contracts.3
The PR does not disclose that moving production onto this API needs more work. The data migration, the sequence seed and the new client credentials and scopes are all left unstated under 'What this does not do'.
synthesis.4 explains code-architecture.1 · code-architecture.3 · code-architecture.4 · code-architecture.5
A fact is held at two sites with nothing tying them together, so a new member means a hand edit in step. The sites are the TUI and API read methods, the lookup seeds in the two stores, the sentinel list and HTTP status table, and the Docker allowlist and import graph.
synthesis.5 explains fleet-patterns.1 · fleet-patterns.4
The Postgres audit trail and Dockerfile tooling lack the coverage that sibling repos hold. The trace fields and the every-table-audited test are missing, and the registry declares no docker component.
One defect, raised more than once
fleet-patterns.2 · contracts.1: The new API, its Postgres store and its schemas have no row in the data catalog's producers.yaml.
Shapes as JSON
{
"shapes": [
{
"number": "synthesis.1",
"section": "design",
"explains": [
"fleet-patterns.2",
"contracts.1"
],
"body": "The new API, its Postgres store and its schemas have no row in the data catalog's `producers.yaml`. Both findings name the same missing catalog rows and the same data.md rule."
},
{
"number": "synthesis.2",
"section": "design",
"explains": [
"G1.3",
"stale-prose.1",
"stale-prose.2",
"stale-prose.3"
],
"body": "Prose in README and CLAUDE.md states a guarantee that the code does not hold. The API lacks the repo-name ban, empty lists are not null, and the API scope and status order claims are stale."
},
{
"number": "synthesis.3",
"section": "design",
"explains": [
"G1.5",
"contracts.3"
],
"body": "The PR does not disclose that moving production onto this API needs more work. The data migration, the sequence seed and the new client credentials and scopes are all left unstated under 'What this does not do'."
},
{
"number": "synthesis.4",
"section": "design",
"explains": [
"code-architecture.1",
"code-architecture.3",
"code-architecture.4",
"code-architecture.5"
],
"body": "A fact is held at two sites with nothing tying them together, so a new member means a hand edit in step. The sites are the TUI and API read methods, the lookup seeds in the two stores, the sentinel list and HTTP status table, and the Docker allowlist and import graph."
},
{
"number": "synthesis.5",
"section": "design",
"explains": [
"fleet-patterns.1",
"fleet-patterns.4"
],
"body": "The Postgres audit trail and Dockerfile tooling lack the coverage that sibling repos hold. The trace fields and the every-table-audited test are missing, and the registry declares no docker component."
}
],
"duplicates": [
{
"defect": "The new API, its Postgres store and its schemas have no row in the data catalog's producers.yaml.",
"findings": [
"fleet-patterns.2",
"contracts.1"
]
}
]
}This review does not gate merge.
PR review round 1Ran: G1, useless-test, code-architecture, fleet-patterns, system-design, stale-prose, failure-paths, contracts, synthesis
Round record{
"grammar": 6,
"round": 1,
"repo": "datapointchris/projects",
"pr": 17,
"head": "9b69e86f838da95f3ddac16d3fcba8705a8cd8c0",
"experiment": "none",
"dispatch": "20261107T195837Z-pr-review",
"roster": "7cfd389c050fe0ef4d6a4f766e70754eb2804889",
"harness": "6a7722b7cb06ebfd9ed1c1f63f514ce24240fb70",
"machine": "queue",
"started": "2026-11-07T19:58:42Z",
"finished": "2026-11-07T20:10:34Z",
"reviewers": [
{
"label": "G1",
"agent": "pr-reviewer",
"model": "opus",
"effort": "high",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/general.md",
"prompt_sha": "a0f85e3b82b4",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 678,
"tokens": {
"input": 42,
"cache_write": 234560,
"cache_read": 3094439,
"output": 61613
},
"tokens_from": "result",
"tokens_by_model": {
"claude-opus-5-5": {
"input": 42,
"cache_write": 234560,
"cache_read": 3094439,
"output": 61613
}
},
"session": "01a1b797-77b3-76ad-90c1-c0c077153357",
"worklog": "2401.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/projects/pull/17#pullrequestreview-5814707407",
"review_id": 5814707407,
"findings": {
"correctness": 4,
"design": 1,
"prose": 0
},
"numbers": [
"G1.1",
"G1.2",
"G1.3",
"G1.4",
"G1.5"
],
"rules": {
"proposed": 1,
"keys": [
"rule/code-architecture/a-rule-a-service-takes-as-a-constructor-option-is-set-at-every-entry-point-and-each-entry-points-test-proves-it-fires"
],
"filed": true,
"why": "",
"path": "/home/chris/.cache/run-pr-reviewers/datapointchris-projects-pr17-round1-20261107T195837Z/records/G1.rules.json"
},
"head": "9b69e86f838da95f3ddac16d3fcba8705a8cd8c0",
"kept": false,
"records": "/home/chris/.cache/run-pr-reviewers/datapointchris-projects-pr17-round1-20261107T195837Z/records",
"started": "2026-11-07T19:58:42Z",
"finished": "2026-11-07T20:10:03Z",
"account": "fleetizen",
"api_key_source": "none",
"machine": "archlinux"
},
{
"label": "useless-test",
"agent": "pr-useless-test-reviewer",
"model": "opus",
"effort": "high",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/useless-test.md",
"prompt_sha": "3700d72236e3",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 308,
"tokens": {
"input": 20,
"cache_write": 126741,
"cache_read": 772287,
"output": 20218
},
"tokens_from": "result",
"tokens_by_model": {
"claude-opus-5-5": {
"input": 20,
"cache_write": 126741,
"cache_read": 772287,
"output": 20218
}
},
"session": "01a1b797-7b6d-7ac3-b317-eb4cc58ca2a9",
"worklog": "2401.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/projects/pull/17#pullrequestreview-5814688552",
"review_id": 5814688552,
"findings": {
"correctness": 0,
"design": 3,
"prose": 0
},
"numbers": [
"useless-test.1",
"useless-test.2",
"useless-test.3"
],
"rules": {},
"head": "9b69e86f838da95f3ddac16d3fcba8705a8cd8c0",
"kept": false,
"records": "/home/chris/.cache/run-pr-reviewers/datapointchris-projects-pr17-round1-20261107T195837Z/records",
"started": "2026-11-07T19:58:45Z",
"finished": "2026-11-07T20:03:54Z",
"account": "fleetizen",
"api_key_source": "none",
"machine": "archlinux"
},
{
"label": "code-architecture",
"agent": "pr-code-architecture-reviewer",
"model": "opus",
"effort": "medium",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/persona.md",
"prompt_sha": "d9be86eb4324",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 239,
"tokens": {
"input": 14,
"cache_write": 157574,
"cache_read": 502176,
"output": 23497
},
"tokens_from": "result",
"tokens_by_model": {
"claude-opus-5-5": {
"input": 14,
"cache_write": 157574,
"cache_read": 502176,
"output": 23497
}
},
"session": "01a1b797-7ae2-7dd8-a177-228246ebe9ec",
"worklog": "2401.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/projects/pull/17#pullrequestreview-5814684054",
"review_id": 5814684054,
"findings": {
"correctness": 0,
"design": 5,
"prose": 0
},
"numbers": [
"code-architecture.1",
"code-architecture.2",
"code-architecture.3",
"code-architecture.4",
"code-architecture.5"
],
"rules": {},
"head": "9b69e86f838da95f3ddac16d3fcba8705a8cd8c0",
"kept": false,
"records": "/home/chris/.cache/run-pr-reviewers/datapointchris-projects-pr17-round1-20261107T195837Z/records",
"started": "2026-11-07T19:58:44Z",
"finished": "2026-11-07T20:02:46Z",
"account": "fleetizen",
"api_key_source": "none",
"machine": "archlinux"
},
{
"label": "fleet-patterns",
"agent": "pr-fleet-patterns-reviewer",
"model": "opus",
"effort": "medium",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/persona.md",
"prompt_sha": "eae343d1d8d5",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 597,
"tokens": {
"input": 24,
"cache_write": 124197,
"cache_read": 777039,
"output": 26780
},
"tokens_from": "result",
"tokens_by_model": {
"claude-opus-5-5": {
"input": 24,
"cache_write": 124197,
"cache_read": 777039,
"output": 26780
}
},
"session": "01a1b797-7bec-74c7-994a-79f213159b88",
"worklog": "2401.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/projects/pull/17#pullrequestreview-5814703674",
"review_id": 5814703674,
"findings": {
"correctness": 0,
"design": 4,
"prose": 0
},
"numbers": [
"fleet-patterns.1",
"fleet-patterns.2",
"fleet-patterns.3",
"fleet-patterns.4"
],
"rules": {
"proposed": 2,
"keys": [
"rule/data/an-audit-trail-is-a-postgres-row-trigger-reading-transaction-local-settings-and-a-test-lists-every-table-against-its-trigger",
"rule/release/an-apps-image-and-manifest-publish-steps-live-in-the-shared-workflows-because-the-callers-copy-pins-tools-by-hand"
],
"filed": true,
"why": "",
"path": "/home/chris/.cache/run-pr-reviewers/datapointchris-projects-pr17-round1-20261107T195837Z/records/fleet-patterns.rules.json"
},
"head": "9b69e86f838da95f3ddac16d3fcba8705a8cd8c0",
"kept": false,
"records": "/home/chris/.cache/run-pr-reviewers/datapointchris-projects-pr17-round1-20261107T195837Z/records",
"started": "2026-11-07T19:58:51Z",
"finished": "2026-11-07T20:08:51Z",
"account": "fleetizen",
"api_key_source": "none",
"machine": "archlinux"
},
{
"label": "system-design",
"agent": "pr-system-design-reviewer",
"model": "sonnet",
"effort": "medium",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/persona.md",
"prompt_sha": "c284e39c894a",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 30,
"tokens": {
"input": 14,
"cache_write": 27561,
"cache_read": 142937,
"output": 2325
},
"tokens_from": "result",
"tokens_by_model": {
"claude-sonnet-5-5": {
"input": 14,
"cache_write": 27561,
"cache_read": 142937,
"output": 2325
}
},
"session": "01a1b797-7a2d-7b8e-a036-6f1258f7b029",
"worklog": "2401.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/projects/pull/17#pullrequestreview-5814672710",
"review_id": 5814672710,
"findings": {
"correctness": 0,
"design": 0,
"prose": 0
},
"numbers": [],
"rules": {},
"head": "9b69e86f838da95f3ddac16d3fcba8705a8cd8c0",
"kept": false,
"records": "/home/chris/.cache/run-pr-reviewers/datapointchris-projects-pr17-round1-20261107T195837Z/records",
"started": "2026-11-07T19:58:43Z",
"finished": "2026-11-07T19:59:16Z",
"account": "fleetizen",
"api_key_source": "none",
"machine": "archlinux"
},
{
"label": "stale-prose",
"agent": "pr-stale-prose-reviewer",
"model": "opus",
"effort": "medium",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/persona.md",
"prompt_sha": "50506a05f936",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 211,
"tokens": {
"input": 16,
"cache_write": 84111,
"cache_read": 328204,
"output": 19581
},
"tokens_from": "result",
"tokens_by_model": {
"claude-opus-5-5": {
"input": 16,
"cache_write": 84111,
"cache_read": 328204,
"output": 19581
}
},
"session": "01a1b797-7d2d-7b25-be7d-4d4843ca2163",
"worklog": "2401.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/projects/pull/17#pullrequestreview-5814682654",
"review_id": 5814682654,
"findings": {
"correctness": 0,
"design": 0,
"prose": 5
},
"numbers": [
"stale-prose.1",
"stale-prose.2",
"stale-prose.3",
"stale-prose.4",
"stale-prose.5"
],
"rules": {},
"head": "9b69e86f838da95f3ddac16d3fcba8705a8cd8c0",
"kept": false,
"records": "/home/chris/.cache/run-pr-reviewers/datapointchris-projects-pr17-round1-20261107T195837Z/records",
"started": "2026-11-07T19:58:51Z",
"finished": "2026-11-07T20:02:25Z",
"account": "fleetizen",
"api_key_source": "none",
"machine": "archlinux"
},
{
"label": "failure-paths",
"agent": "pr-failure-paths-reviewer",
"model": "sonnet",
"effort": "medium",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/persona.md",
"prompt_sha": "56bdd640764b",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 42,
"tokens": {
"input": 14,
"cache_write": 30343,
"cache_read": 167706,
"output": 3744
},
"tokens_from": "result",
"tokens_by_model": {
"claude-sonnet-5-5": {
"input": 14,
"cache_write": 30343,
"cache_read": 167706,
"output": 3744
}
},
"session": "01a1b797-7f5e-782c-ae4e-e8ed84c8807e",
"worklog": "2401.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/projects/pull/17#pullrequestreview-5814673688",
"review_id": 5814673688,
"findings": {
"correctness": 1,
"design": 0,
"prose": 0
},
"numbers": [
"failure-paths.1"
],
"rules": {},
"head": "9b69e86f838da95f3ddac16d3fcba8705a8cd8c0",
"kept": false,
"records": "/home/chris/.cache/run-pr-reviewers/datapointchris-projects-pr17-round1-20261107T195837Z/records",
"started": "2026-11-07T19:58:52Z",
"finished": "2026-11-07T19:59:36Z",
"account": "fleetizen",
"api_key_source": "none",
"machine": "archlinux"
},
{
"label": "contracts",
"agent": "pr-contracts-reviewer",
"model": "opus",
"effort": "medium",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/persona.md",
"prompt_sha": "88b65626bceb",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 378,
"tokens": {
"input": 36,
"cache_write": 143677,
"cache_read": 1710735,
"output": 34343
},
"tokens_from": "result",
"tokens_by_model": {
"claude-opus-5-5": {
"input": 36,
"cache_write": 143677,
"cache_read": 1710735,
"output": 34343
}
},
"session": "01a1b797-7e0c-7e90-8c9a-65da444013c1",
"worklog": "2401.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/projects/pull/17#pullrequestreview-5814692634",
"review_id": 5814692634,
"findings": {
"correctness": 1,
"design": 3,
"prose": 0
},
"numbers": [
"contracts.1",
"contracts.2",
"contracts.3",
"contracts.4"
],
"rules": {},
"head": "9b69e86f838da95f3ddac16d3fcba8705a8cd8c0",
"kept": false,
"records": "/home/chris/.cache/run-pr-reviewers/datapointchris-projects-pr17-round1-20261107T195837Z/records",
"started": "2026-11-07T19:58:52Z",
"finished": "2026-11-07T20:05:12Z",
"account": "fleetizen",
"api_key_source": "none",
"machine": "archlinux"
},
{
"label": "synthesis",
"agent": null,
"model": "sonnet",
"effort": "medium",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/synthesis.md",
"prompt_sha": "40dcb5a50df0",
"status": "ran",
"why": "",
"attempts": 2,
"seconds": 12,
"tokens": {
"input": 6,
"cache_write": 19221,
"cache_read": 21245,
"output": 1569
},
"tokens_from": "result",
"tokens_by_model": {
"claude-sonnet-5-5": {
"input": 6,
"cache_write": 19221,
"cache_read": 21245,
"output": 1569
}
},
"session": "01a1b7a1-f76a-7e55-ad82-8a82ec05e336",
"worklog": "2401.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/projects/pull/17#pullrequestreview-5814708850",
"review_id": 5814708850,
"findings": {
"correctness": 0,
"design": 5,
"prose": 0
},
"numbers": [
"synthesis.1",
"synthesis.2",
"synthesis.3",
"synthesis.4",
"synthesis.5"
],
"rules": {},
"head": "9b69e86f838da95f3ddac16d3fcba8705a8cd8c0",
"kept": false,
"records": "/home/fleet/.cache/run-pr-reviewers/datapointchris-projects-pr17-round1-20261107T195837Z/records",
"started": "2026-11-07T20:10:18Z",
"finished": "2026-11-07T20:10:34Z",
"account": "fleetizen",
"api_key_source": "none",
"machine": "queue"
}
],
"rules": {
"proposed": 3,
"keys": [
"rule/code-architecture/a-rule-a-service-takes-as-a-constructor-option-is-set-at-every-entry-point-and-each-entry-points-test-proves-it-fires",
"rule/data/an-audit-trail-is-a-postgres-row-trigger-reading-transaction-local-settings-and-a-test-lists-every-table-against-its-trigger",
"rule/release/an-apps-image-and-manifest-publish-steps-live-in-the-shared-workflows-because-the-callers-copy-pins-tools-by-hand"
],
"unfiled": []
},
"kept": []
} |
The image first built in the release job, after the tag exists, so a package left out of the .dockerignore allowlist cost a version with binaries and no image. The repo now declares a docker stack, which brings hadolint. The Postgres SQL lint has a hook id of its own, because a second hook named sqlfluff-lint replaced the generated SQLite one on regeneration. Agent-Name: claude-code-unattended Agent-Session: b6774e69-f156-4d1a-9713-9886fbbc39f7 Agent-Run: 01a1b797-8518-7db1-982b-27aaa8245b56 Worklog: 2401.1
A database the single-row reorder wrote holds ties, and a row tied with the one above a drop can sit on either side of it, so the drop now counts members in the order a store lists them. A task pushed with no id got a new one on the server, so a check made before the next pull named an id the server never held. The push dropped that 404, and the pull brought the task back unchecked. Agent-Name: claude-code-unattended Agent-Session: b6774e69-f156-4d1a-9713-9886fbbc39f7 Agent-Run: 01a1b797-8518-7db1-982b-27aaa8245b56 Worklog: 2401.1
…nnot hold A database dropping packets held a request until the client gave up, and pgx waits as long as its context allows. Each request now has a deadline, a connection attempt has its own timeout, and a readiness check answers inside the probe's timeout. A request past its deadline answers 503 rather than 500. A position past int32 and a NUL in a string are both refused at decode, because each store fails on them differently and neither failure is a client's to read. A refusal with no row in statusOfRefusal answers 422 and logs the missing row, so a new refusal kind cannot arrive as a 500. Any other 500 names its request id. Each route pattern is a literal string, so a reader of the source sees the whole route where it is registered. Agent-Name: claude-code-unattended Agent-Session: b6774e69-f156-4d1a-9713-9886fbbc39f7 Agent-Run: 01a1b797-8518-7db1-982b-27aaa8245b56 Worklog: 2401.1
… named twice Two reads of the same rows could disagree, because the TUI's reads and the API's each built their own answer. ListProjectsByStatus, ListItemsByProject, ListArchived, ListBlockedItems and Search now sit over the read the API answers from. CreateItem treats a project named twice as one membership, as a store's primary key would otherwise refuse it as a server fault. Agent-Name: claude-code-unattended Agent-Session: b6774e69-f156-4d1a-9713-9886fbbc39f7 Agent-Run: 01a1b797-8518-7db1-982b-27aaa8245b56 Worklog: 2401.1
…ws from model An audit row names its actor and request, and now also the traceparent and baggage the request arrived with, so a row joins to the trace that wrote it. The request log line carries the traceparent too. Project kinds and statuses each have one list, in model. postgres.Migrate upserts them on every run, so a kind added to the list reaches a database with no new migration file. TestEachStoreHoldsTheKnownKindsAndStatuses holds both stores to it. TestEveryTableIsAuditedButWhatOptsOut fails when a new table brings no trigger. Agent-Name: claude-code-unattended Agent-Session: b6774e69-f156-4d1a-9713-9886fbbc39f7 Agent-Run: 01a1b797-8518-7db1-982b-27aaa8245b56 Worklog: 2401.1
A partial index allowing many unnumbered items and the name constraint's other clauses pin a migration nothing else depends on. Comparing a refusal's message byte for byte fails on wording no client reads. Agent-Name: claude-code-unattended Agent-Session: b6774e69-f156-4d1a-9713-9886fbbc39f7 Agent-Run: 01a1b797-8518-7db1-982b-27aaa8245b56 Worklog: 2401.1
…ler takes The API sets no repo-name ban, because a container holds no registry. /api/v2 is its version signal, and each schema says a field added is not a new version. A client holding neither prefix answers 401, so moving a caller takes a client of its own. Agent-Name: claude-code-unattended Agent-Session: b6774e69-f156-4d1a-9713-9886fbbc39f7 Agent-Run: 01a1b797-8518-7db1-982b-27aaa8245b56 Worklog: 2401.1
The web page, the agents and the scripts reach projects through a separate
API in another language, with its own copy of every rule. A rule added there
never reaches the TUI, and the copies already disagree on what a project holds.
Nothing here could answer over HTTP, so the two could not be brought together.
This serves the same routes and the same answers from this repo, on Postgres,
through the
servicethe TUI uses. Rules the other API held andservicelacked moved into
service. Each release now publishes a signed image andmanifests beside the binaries. Nothing is pointed at them by this change.
Session b6774e69-f156-4d1a-9713-9886fbbc39f7.
What to look at
service/service.go,service/references.go,service/listing.goinde84367and7c95543. Each refusal and each derived field is a rule bothstores now share. Check that none depends on a store's accidental behavior.
store/postgres/store.goandstore/postgres/migrations/00001_projects.sqlin
de84367and87fae1a.InTxtakespg_advisory_xact_lockfirst, andbinds the writer with
set_config(..., true). Check that a rule's reads holdat commit, and that the trigger covers a cascade.
api/routes/request_input.goandapi/routes/answers.goine7bb8ffandd32399e. APATCHtells a left-out field from a null one here, and everyrefusal body is built here. Compare with
api/wire/wire.goandapi/schemas/.api/authentication/authentication.goandroutes.ServiceScopes. Check thata service client reaches only the listed read patterns, and that
/is in noscope.
api-publishin.github/workflows/release.ymlanddeploy/in589c212.That job has never run. Check what a re-run does at each step.
reorderItemCmdandpositionBelowintui/app.goin0960325andcb5219d, andtaskPayloadinsync/backend.goincb5219d.How it was verified
go test ./...passes in 10 packages.PROJECTS_TEST_STORE=postgres go test ./service/runs every rule on a real Postgres and passes in 15 s.TestEveryAnswerMatchesItsSchemavalidates a real answer against each file inapi/schemas.govulncheck ./...reads 0, after raising the modulestestcontainerspulledin at flagged versions.
body, at this head. Each side ran on Postgres, restored from one copy of a
production database. The program and the data are not in this repo.
409 ×12, 410 ×4, 422 ×114. Neither side answered a 5xx.
status. Instants were compared as instants, and no time differed.
read identical to before, every field.
2 ms.
/readyanswered 503 in 2 s andGET /api/v1/projectsanswered 503 in 5 s.POST, 2^32+1 onPATCHand a NUL in a title each answer 422. A project namedtwice in
project_idsanswers 201 with one membership.--read-onlyas uid 10500.API imagenowbuilds it on every pull request.
deploy/appanddeploy/migratepasskubeconform -strictandkube-linter.132335aand on this head, each on a fresh database,measured the reorder, refusal and reader changes listed under What changes.
132335a, andone written by
132335aopens under this head.dependency_idsand/blocked, read with noORDER BYreposent as an empty string"yes"or1for a boolean, and the read straight afterGET /project-items/with a trailing slashHEAD /projectsWhat changes
In the TUI, for anyone upgrading:
because the single-row write tied with the neighbor, and an archived item above
the drop threw it one row high. A drop below a row tied on position lands under
that row.
sends the task's own id, so the check names an id the server holds.
Error: duplicate namenow readsError: An active project named 'alpha' already exists (<id>).Projects:line lists an item's projects by position rather thanby name.
kind,completed_atand two lookup tables.Completing an item stamps
completed_at, and no screen shows it yet.servicenow refuses to complete an itemwith open tasks, and the TUI's own check already refused first. Deleting a
project refuses incomplete orphans, and the TUI has no delete. No screen changes.
For anyone running the release:
go installof the root still builds only the CLI, andgo list -deps .holdsno pgx, goose or goapiauth.
projects-apiis a second binary, atghcr.io/datapointchris/projects, withmanifests at
ghcr.io/datapointchris/projects/manifests. One version names bothit and the CLI.
projects-api migrateruns from a Job indeploy/migrate, applied beforedeploy/app. The README lists its settings and thecosign verifycommand.migratealso adds any kind or status it finds missing frommodel's lists.Each one it adds writes an audit row.
requestDeadline, or one a stalled database never answers, gets a503. Any other 500 names its request id.
traceparentandbaggage. A request log linecarries its
traceparent.New routes the other API lacks:
POSTandGET /api/v1/undo,/health,/ready, and/.well-known/oauth-protected-resourcewhenPUBLIC_URLis set.Decisions, and what they rejected
service, and a handler holds none. Arule in a handler would have to land again for SQLite. Rejected: a thin layer
of API-only rules, which is the divergence this removes.
pg_advisory_xact_lockat the top ofInTx. It is the analog of SQLite'simmediate transaction, and costs one writer at a time, which one person's write
rate never feels. Rejected: serializable, which needs a retry the port does
not state.
api/wireholds the answers, apart frommodel. A model change cannot alteran answer by accident.
model's tags would also drop fields clients index byname.
undo_loghas no audit trigger. It writes on every write, and would copy eachwhole row the real tables already audit.
repo: ""stores no repo. The other API keeps the empty string. Null is whatthe untagged filter expects, and the only sender is one flag on an existing CLI.
request no client sends, or a sentence no client parses.
/readyalso checks the identity provider, throughNewConnecting. A providerdown at start holds the pod unready rather than crash-looping it.
/api/v2is this API's version signal, and no answer carriesschema_version.A list answers a bare array, which has no place for the field. Each schema states
that a field added is not a new version. Rejected: a field on objects alone, which
would leave list answers with no signal.
has no ban either. Rejected: shipping a registry into the image, which would make
one deployment's repo list part of the release.
service.SQLite holds both, so a refusal in
servicewould narrow the TUI's store for alimit only Postgres has.
model, on everymigrate. A new kind then costs onelist edit. Rejected: a seed in each store's migrations, where the next kind needs
a migration file and an edit in step.
sync's pull leaveskindandcompleted_atalone. CLAUDE.md states theomission once, in
sync's carve-out. Rejected: a test holdingsyncto thecolumns, because that engine is due to leave.
ReorderItemskipping them. The web page sends an index among every member, so the wire's
position has to keep meaning that.
Risk and rollback
api-publishruns for the first time. If it fails,the tag and the binaries already exist, and a re-run builds only what is missing.
the previous binary works with them.
api-publish,release/install-toolsandrelease/pin-manifestsare a fourth copyof what three other repos carry, with no shared owner. A cosign, flux or kustomize
bump is a hand edit in each.
What this does not do
syncstill reaches the old one.because
migraterefuses a database already holding these tables. It also setsitem_numbersfrom the old sequence's own counter, because numbers already handedout can outrun the rows left.
projects-cli-orprojects-svc-client holding
projects.items.read, because a client with neither prefix answers401.
/works through{id}directly. Behind a proxy it dependson the proxy keeping
%2F, which is not established.PATCH /projects/{id}keeps the other API's single-row write.The review
#17 (comment)
G1.1fixed —cb5219d.G1.2fixed —cb5219d.G1.3fixed —3e8c3a7. The API sets no ban, so README and CLAUDE.md now say so. ThestatusOfRefusalrow stays, becauseservicestill refuses where a caller sets the ban.G1.4fixed —d32399e,7c95543. Position and NUL are refused at decode, a repeated project collapses inCreateItem, and a 500 names its request id. Reach is unproven, so these were run directly and have no test.G1.5deferred — moving production is a later change, now stated under What this does not do. A sequence behind its rows is a server fault, soduplicateErrorkeeps answering it as a 500 naming the request.code-architecture.1fixed —7c95543.code-architecture.2fixed —3e8c3a7. The omission is stated once in CLAUDE.md's carve-out, with no coverage test, because the sync engine is due to leave.code-architecture.3fixed —87fae1a.code-architecture.4fixed —d32399e. A refusal with no row answers 422 and logs the missing row.code-architecture.5fixed —4ac7478.API imagebuilds on every pull request.useless-test.1fixed —d5a9474.useless-test.2fixed —d5a9474.useless-test.3fixed —87fae1a.fleet-patterns.1fixed —87fae1a.fleet-patterns.2deferred — the data catalog reads this repo's main branch, so its rows land right after the merge. Theschema_versionpart is fixed undercontracts.4.fleet-patterns.3deferred — the shared owner is a change to a shared workflow and four repos, which this branch cannot carry. Risk and rollback names this as a fourth copy.fleet-patterns.4fixed —4ac7478. The repo now declares a docker stack, so hadolint runs in pre-commit and CI.stale-prose.1fixed —3e8c3a7.stale-prose.2fixed —3e8c3a7.stale-prose.3fixed —3e8c3a7.stale-prose.4fixed —87fae1a,3e8c3a7.stale-prose.5fixed —d32399e,87fae1a,3e8c3a7. The scope block went in favor ofroutes.ServiceScopes, keeping the scope's name, which a client author needs.contracts.1deferred — the data catalog reads this repo's main branch, so its rows land right after the merge.contracts.2fixed —d32399e. Every pattern is a literal, and the catalog's route reader now finds 28.contracts.3fixed —3e8c3a7.contracts.4fixed —3e8c3a7. Written as an exception in CLAUDE.md, with the sentence in each schema.failure-paths.1fixed —d32399e. Run against a host dropping packets, as listed under How it was verified, with no test.synthesis.1deferred — same ascontracts.1.synthesis.2fixed — same asG1.3,stale-prose.1,stale-prose.2,stale-prose.3.synthesis.3fixed — the move and the new clients are now stated under What this does not do, as inG1.5andcontracts.3.synthesis.4fixed — same ascode-architecture.1,code-architecture.3,code-architecture.4,code-architecture.5.synthesis.5fixed — same asfleet-patterns.1,fleet-patterns.4.