Skip to content

feat: serve the projects page from its own Caddy image, released with the API - #18

Merged
datapointchris merged 4 commits into
mainfrom
projects-web-port
Nov 8, 2026
Merged

datapointchris merged 4 commits into
mainfrom
projects-web-port

Conversation

@datapointchris

@datapointchris datapointchris commented Nov 7, 2026 •

Copy link
Copy Markdown
Owner

The projects API has no page of its own, so a person reaches projects only through the TUI or a client
holding a token. This adds web/, the Projects page in Vue, served by its own projects-web image on the
API's origin. The API logs the browser in as a confidential client and keeps the session in Postgres, so
the browser holds an httpOnly cookie and never a token. Each release builds, signs and pins the web image
beside the API's, under one version.

What to look at

  1. api/authentication/browser_login.go: the login is gobrowserlogin.Connecting, from v0.2.0. It
    opens the session tables and reads the provider in the background, as the token verifier does. Check
    that /ready leaves it out, that a request with no credential still gets the 401 challenge, and that
    MountBrowserLogin returns the three paths Protect passes with no credential.
  2. api/settings/settings.go: WEB_CLIENT_SECRET without PUBLIC_URL refuses to load. A PUBLIC_URL
    that is neither https nor http on loopback stops the API at start, in NewConnecting.
  3. web/Caddyfile.docker: /api/* and /.well-known/* go to projects-api:8000, /assets/* misses
    answer 404, and everything else is index.html. Check the order of the handle blocks.
  4. web/src/api/client.ts: X-CSRF: 1 on every request, and the 401 rule. A write and a
    holdsTypedInput read reject in place. Any other read navigates to the login once a page, at most
    twice a minute. web/src/api/errors.ts passes that rejection through so its reason reaches the toast.
  5. web/src/App.vue and the Log out button in ProjectsView.vue: after logout the page shows a line
    and reads nothing.
  6. release/pin-manifests and the publish job in release.yml: two images decided, built, pinned and
    signed separately, and a built path running an unpinned image refused.
  7. deploy/app/web.yaml: non-root, read-only root, emptyDir at /data and /config, probes on
    /health, and 64Mi requested and limited.

How it was verified

  • go build ./..., go vet ./api/... and go test ./api/... pass. On a real Postgres,
    TestEveryTableIsAuditedButWhatOptsOut passes with both session tables opted out.

  • go list -deps . reaches neither gobrowserlogin nor api/, so go install of the root is unchanged.

  • A throwaway test drove Protect over a login that never got through:

    GET  /api/v1/projects  no cookie           -> 401 Bearer {"code":"missing_token",...}
    GET  /api/v1/projects  cookie and X-CSRF   -> 503 {"code":"session_store_unavailable",...}
    GET  /api/auth/login                       -> 503 page
    POST /api/auth/logout  no cookie, X-CSRF   -> 204
    POST /api/auth/logout  cookie, no X-CSRF   -> 403
    POST /api/auth/logout  cookie and X-CSRF   -> 503
    

    NewBrowserLogin with PUBLIC_URL=http://projects.lan refuses: RedirectURL "http://projects.lan/api/auth/callback" is neither https nor http on a loopback host.

  • TestTheSchemasListTheKindsAndStatusesModelHolds fails when one schema's kind reads chores.

  • In web/: npm run typecheck, npm run lint and npm run build pass, and Vitest runs 72 tests.
    The client test asserting a refused write keeps its reason fails without the errors.ts passthrough,
    where the toast read "Unable to reach the server. Check your connection."

  • task web:test:e2e, against a fresh stack: 1 Playwright test passed in 10.8s. It logs in through the
    stand-in provider, then creates, edits and deletes a project named smoke/<n> and an item in it. It
    then logs out, sees the logged-out line, and reads 401 from /api/v1/projects with the same cookies.
    The API logged the request path as /api/v1/projects/smoke%2F<n>, so Caddy kept the escape.

  • docker compose config with PROJECTS_PUBLIC_URL=http://localhost:3002 gives the API that
    PUBLIC_URL, and without it http://localhost:8096. task --dry web:dev passes 3002.

  • Caddy under --read-only --cap-drop ALL --security-opt no-new-privileges serves /health as uid 10501,
    with /data and /config writable as a pod's emptyDir is. It held 15 MiB serving the page.

  • Probes through Caddy: / and a client route answer the page with max-age=300. A hashed asset
    answers immutable, and /assets/index-gone.js answers 404. /.well-known/oauth-protected-resource
    answers 200, and /api/v1/projects with no session answers 401.

  • release/pin-manifests with zero digests, then kubeconform -strict against Kubernetes 1.36.5 and
    kube-linter 0.8.3 on each path: 6 resources valid in app/ and 2 in migrate/, no lint errors, no
    new suppression.

  • actionlint 1.7.7 reports nothing on release.yml and validate.yml.

What changes

  • projects-api takes two new settings. WEB_CLIENT_ID defaults to projects-web. WEB_CLIENT_SECRET
    turns the browser login on, and then PUBLIC_URL is required, as https or http on loopback. With no
    secret the API is bearer-only.
  • /ready checks Postgres and the token verifier, and not the browser login. A login that cannot
    start answers 503 on its own routes and on requests carrying a session, and logs why. At Error where
    the provider's discovery rules the client out, and at Warn otherwise.
  • POST /api/auth/logout ends the session at the API. The provider keeps its own, so logging in
    again may complete without asking for anything.
  • The project schemas list kind and status as an enum, with default: life on kind. That
    covers project, project-with-item-count, and the project inside project-item and
    project-item-detail.
  • gobrowserlogin moves to v0.2.0, which adds Connecting and changes nothing New returns.
  • Migration 00002_browser_sessions.sql adds browser_login_attempts and browser_sessions. Neither is
    audited.
  • The provider's projects-web client needs the redirect URI PUBLIC_URL + /api/auth/callback, the
    authorization_code grant with S256 PKCE, client_secret_basic, and the openid and profile scopes.
    A provider advertising pushed authorization requests receives the login that way. Logout needs no
    redirect URI there.
  • A browser session acts as its person and reaches every route. Service clients keep their scopes.
  • A cluster routes the app's whole host to the projects-web Service on port 8080, which passes /api
    and /.well-known to projects-api. The API's own Service needs no route.
  • Each release publishes ghcr.io/datapointchris/projects-web under the API's version, and the
    manifests' app/ gains the projects-web ServiceAccount, Service and Deployment. The release notes'
    image line reads API image:, with a Web image: line after it.
  • The release job is publish, building both images.
  • task setup runs npm ci in web/, because the new commit hooks lint and format with its packages.
    task test runs the web app's unit tests after Go's. task web:dev starts the stack with Vite's
    address as PUBLIC_URL, so a login returns to Vite.
  • go.mod ignores ./web, so ./... never builds or lints Go that an npm package ships.
  • The page lists a new project by name among projects never reordered, because the service gives
    every new project position 0 and the store sorts ties by name. The TUI does the same.

Decisions, and what they rejected

  • The background start is gobrowserlogin.Connecting, exported from the library. New reads the
    provider's discovery once, at start, and the token verifier already starts in the background so the
    API serves while the provider is down. The library owns the session cookie's name, so only it can
    answer ErrNoSession to a request carrying none before the login is built. Rejected: a wrapper in
    api/authentication, which copied goapiauth.Connecting and would have copied the cookie name too.
    Rejected: checking PUBLIC_URL's scheme in settings.LoadServer, which would copy the library's rule.
  • After logout the page shows a line rather than reloading. A reload reads the API, gets a 401, and
    sends the browser through a provider that still holds its session, which logs it straight back in.
  • The modal keeps preselecting Life, as the page it ports does. The value comes from
    DEFAULT_PROJECT_KIND, which a test holds to the schema's default. Rejected: leaving kind unset,
    which would show an empty select.
  • The browser login is optional, keyed on the secret. A deployment with no web app runs the API
    as before. Rejected: requiring the secret everywhere.
  • The styles are a copy, trimmed to what the page uses. The look changes here without reaching
    another app. --shadow-card, --shadow-card-active and --shadow-card-hover sit in their own partial
    rather than with the grid layout they shipped beside, which this page never uses. No .button--small
    rule exists, because its rule always lost to .button on this page. Moving it into _buttons.scss
    made the buttons visibly smaller.
  • A copied template keeps its markup, and the repo's own prettier config reformats it. A computed-style
    dump of all 188 elements under the page, boxes included, is identical before and after.
  • /.well-known/* goes to the API rather than only the metadata path, so no document under it is
    answered with the page.
  • A missing /assets/ file answers 404. Rejected: the SPA fallback for it, which answered the page
    with a year-long immutable header to a tab running an older release.
  • The CI smoke runs on every pull request, in web-stack. It is the only check of Caddy's routes,
    the login, the logout and the escaped slash, which the unit tests cannot reach.
  • The dev stack's provider is mock-oauth2-server, pinned by digest, sharing the API container's
    network.
    The login takes http only on a loopback host, and the browser and API must name one issuer
    URL, so localhost:8097 has to reach it from both.

Risk and rollback

A release deploys the migration and both images. The migration only creates tables, so the release
before it runs on the new schema. A revert stops the web image and the login. It leaves the two tables
until a migration drops them, and a session cookie already issued stops working.

What this does not do

  • No new feature, view or setting on the page beyond Log out.
  • No route, identity-provider client, DNS or NetworkPolicy. A cluster supplies those, as
    deploy/app/web.yaml lists.
  • No theme or font choice. The page is fixed at the stylesheet's default theme and font.
  • No logout at the provider. Ending that session is the provider's own logout page.

The review

Round 1: #18 (comment)

  • G1.1 fixed — 60a9ee6. The login is gobrowserlogin.Connecting, and /ready leaves it out, so a
    login that cannot start no longer takes bearer clients out of the Service. NewConnecting checks the
    config at start, so an http PUBLIC_URL off loopback stops the API instead of retrying forever. A
    request with no session gets 401 missing_token. The scheme check is the library's own rather than a
    second copy in LoadServer.
  • G1.2 fixed — 60a9ee6. POST /api/auth/logout, and a Log out button that shows a logged-out line.
    A reload would log straight back in through the provider's session. The smoke logs out and then
    reads 401.
  • G1.3 fixed — 60a9ee6. Compose's PUBLIC_URL reads PROJECTS_PUBLIC_URL, and task web:dev sets
    it to Vite's address.
  • fleet-patterns.1 fixed — gobrowserlogin v0.2.0 exports Connecting, with Run, Ready, Login,
    Callback, Logout and Authenticate. It logs at Error where discovery rules the client out and at
    Warn otherwise, as goapiauth.Connecting does. projects keeps a constructor and a mount, in
    60a9ee6.
  • fleet-patterns.2 deferred — one publish workflow shared with the other app releasing this way is a
    change to a shared workflow and two repos, which this branch cannot carry. The second image, the
    unpinned-image guard and the per-image published check are recorded for it.
  • fleet-patterns.3 deferred — the two sibling apps' images take both Caddy fixes in their own repos,
    and a shared owner for the image is a change across three. This branch carries the fixed copy.
  • fleet-patterns.4 deferred — a shared client package needs a package release no sibling has yet,
    so its design is open. The sibling's missing typed-input half is a bug in that repo.
  • code-architecture.1 fixed — 60a9ee6. The project schemas list kinds and statuses as an enum,
    with kind's default. TestTheSchemasListTheKindsAndStatusesModelHolds holds them to model, and a
    Vitest test holds PROJECT_KINDS, PROJECT_STATUSES and DEFAULT_PROJECT_KIND to the schema. The
    kind options and the status filter are built from those, with labels in a Record, so a missing
    label fails the typecheck. DEFAULT_PROJECT_KIND stays, checked against the schema, so the modal
    still shows the kind a new project gets.
  • code-architecture.2 deferred — same as fleet-patterns.2. Repeated --image <name>=<ref> pairs fed
    from one list are that workflow's input. Making only this repo list-driven now would be redone when
    the shared workflow takes it.
  • code-architecture.3 fixed — 60a9ee6. MountBrowserLogin returns the paths it serves with no
    credential, and main.go appends them.
  • stale-prose.1 fixed — 60a9ee6. Each comment rewritten as suggested. --phone-bar-height,
    --page-sticky-top, the durations aliases, and the tile and number-input rules are deleted, because
    nothing here reads them.
  • stale-prose.2 fixed — 60a9ee6. The config names task web:test:e2e.
  • stale-prose.3 fixed — 60a9ee6. The manifest names docker stats on the dev stack instead of a
    figure, and the migration says "again as it is used".
  • stale-prose.4 fixed — 60a9ee6. The ports bullet is deleted.
  • contracts.1 fixed — same as G1.1. The test is gobrowserlogin's
    TestConnecting_ChallengesARequestWithNoSessionUntilRunGetsThrough. A throwaway Protect run over an
    unready login answered 401 missing_token with the Bearer challenge.
  • useless-test.1 fixed — 60a9ee6. Both tests are deleted.
  • failure-paths.1 fixed — same as G1.1.
  • synthesis.1 fixed — same as G1.1.
  • synthesis.2 deferred — fleet-patterns.1 is fixed in the library, and the other three copies are
    deferred as fleet-patterns.2, .3 and .4 say.
  • synthesis.3 fixed — same as stale-prose.3.

Session: 65c3b3f7-676b-4350-8ae4-8beee7c7bfad

@datapointchris datapointchris left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR review — system-design

Used: Read the PR diff and checkout: release.yml, validate.yml, pin-manifests, web Dockerfile and Caddyfile, deploy/app manifests, browser-login Go code, migration 00002. Looked in ~/homelab for a projects deployment and found none. Built and ran nothing.
Standards: No rule cited. I ran no fleet standards search, because no finding needed one.
Findings: 0 correctness · 0 design · 0 prose

No findings.

Findings as JSON
{
  "used": "Read the PR diff and checkout: release.yml, validate.yml, pin-manifests, web Dockerfile and Caddyfile, deploy/app manifests, browser-login Go code, migration 00002. Looked in ~/homelab for a projects deployment and found none. Built and ran nothing.",
  "standards": "No rule cited. I ran no `fleet standards search`, because no finding needed one.",
  "findings": []
}

This review does not gate merge.

@datapointchris datapointchris left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR review — failure-paths

Used: Read the head's diff and files: api/main.go, api/authentication/browser_login.go, release.yml, pin-manifests, Caddyfile, client.ts, deploy/web.yaml and the compose file. I ran no failure injection. The findings are traced by reading only.
Standards: No fleet standards search run, so no rule is cited.
Findings: 1 correctness · 0 design · 0 prose

Correctness

failure-paths.1 api/main.go:89 · api/main.go:97 · api/authentication/browser_login.go:101

Reach: unproven — settle it by pointing a head build with WEB_CLIENT_SECRET set at a database without migration 00002, or a bad WEB_CLIENT_ID/secret or issuer, and curling /ready on the pod. The trace shows 503. It also needs a deployed pod that lacks the session tables or has a rejected web-client config. That pod then rejects bearer-token callers too, including the CLI and agents.

A failure in the optional browser login takes the whole API out of its Service.

When WEB_CLIENT_SECRET is set, main.go adds a browser login readiness check. Run retries forever, and Ready() stays non-nil until connect succeeds. connect fails while the session tables are absent or their columns differ. It also fails on any config gobrowserlogin refuses.

The /ready check then fails. Kubernetes drops the pod from the projects-api Service. Bearer-token callers get connection errors from a pod that could serve them.

The code comments say the rest of the API serves while the login is unready. The /ready wiring contradicts that.

At most the rollout stalls, so the old pods keep serving. A restart, a scale-up or a node move after any later fault leaves the pod out of the Service.

Fix: leave browserLogin.Ready out of the checks slice, or report it as informational. Keep the 503 per request and the log warning. Then only browser requests fail while the login is down.

Findings as JSON
{
  "used": "Read the head's diff and files: api/main.go, api/authentication/browser_login.go, release.yml, pin-manifests, Caddyfile, client.ts, deploy/web.yaml and the compose file. I ran no failure injection. The findings are traced by reading only.",
  "standards": "No `fleet standards search` run, so no rule is cited.",
  "findings": [
    {
      "number": "failure-paths.1",
      "section": "correctness",
      "sites": [
        "api/main.go:89",
        "api/main.go:97",
        "api/authentication/browser_login.go:101"
      ],
      "reach": "unproven — settle it by pointing a head build with WEB_CLIENT_SECRET set at a database without migration 00002, or a bad WEB_CLIENT_ID/secret or issuer, and curling /ready on the pod. The trace shows 503. It also needs a deployed pod that lacks the session tables or has a rejected web-client config. That pod then rejects bearer-token callers too, including the CLI and agents.",
      "body": "**A failure in the optional browser login takes the whole API out of its Service.**\n\nWhen `WEB_CLIENT_SECRET` is set, `main.go` adds a `browser login` readiness check. `Run` retries forever, and `Ready()` stays non-nil until `connect` succeeds. `connect` fails while the session tables are absent or their columns differ. It also fails on any config `gobrowserlogin` refuses.\n\nThe `/ready` check then fails. Kubernetes drops the pod from the `projects-api` Service. Bearer-token callers get connection errors from a pod that could serve them.\n\nThe code comments say the rest of the API serves while the login is unready. The `/ready` wiring contradicts that.\n\nAt most the rollout stalls, so the old pods keep serving. A restart, a scale-up or a node move after any later fault leaves the pod out of the Service.\n\n**Fix:** leave `browserLogin.Ready` out of the `checks` slice, or report it as informational. Keep the 503 per request and the log warning. Then only browser requests fail while the login is down."
    }
  ]
}

This review does not gate merge.

@datapointchris datapointchris left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR review — code-architecture

Used: I cloned the repo and checked out PR 18 at 52c3969. I read the full diff and the related code at head: api/, model/project.go, api/wire, api/schemas, the release and validate workflows, release/pin-manifests, deploy/app, web/src and CLAUDE.md. Then I searched the repo for every other site that decides the same project kinds, project statuses, image set and public paths.
Standards: These findings cite api-design.md § "A value the server owns reaches the client from the server", api-design.md § "A client depends on the wire contract, never on the server's types" and code-quality.md § "Ask whatever owns a fact…". The searches for a rule on per-instance release steps and a rule on public paths beside their handler found nothing that applied.
Findings: 0 correctness · 3 design · 0 prose

Design and architecture

code-architecture.1 web/src/api/types.ts:4 · web/src/api/types.ts:15 · web/src/components/projects/AddEditProjectModal.vue:84 · web/src/components/projects/AddEditProjectModal.vue:89 · web/src/views/ProjectsView.vue:517 · web/src/views/ProjectsView.vue:716 · model/project.go:35 · model/project.go:60 · model/project.go:61

Reach: This costs the next project kind or project status. Today that change is a row in model.ProjectKinds/ProjectStatuses, which store/postgres/open.go:52 and store/sqlite/lookup_rows.sql insert. model/project.go:56 already says a new status takes decisions in ProjectIsTerminal and SortProjectsForBoard. types.ts:3 says it outright: "adding one is an insert plus this union". With this PR, the same change also needs the TS union, the kind options and DEFAULT_KIND in AddEditProjectModal.vue, the status filter options at ProjectsView.vue:517 and the terminal-status map at ProjectsView.vue:716. No test compares any of them with model.

The web app keeps its own copy of the project kind and status lists that model owns

model/project.go decides three things:

  • the kinds: ProjectKinds = build, chore, life
  • the default kind: KindLife
  • the statuses: ProjectStatuses = active, someday, completed, dropped, plus ProjectIsTerminal

The PR types each of these again by hand in web/:

  • the ProjectKind and ProjectStatus unions in types.ts
  • the kind options in AddEditProjectModal.vue:84-86
  • DEFAULT_KIND = 'life' at AddEditProjectModal.vue:89, which repeats the default the server already applies to a create with no kind
  • the status filter options at ProjectsView.vue:517-520
  • the completed/dropped terminal map at ProjectsView.vue:716-717

The API schemas do not name these values. api/schemas/project.schema.json types kind and status only as "one of the names in the project_kinds/project_statuses table". So the server suite writes nothing the web suite could check its copy against.

api-design.md § "A value the server owns reaches the client from the server" covers this case: a vocabulary is never a hand-typed copy. The lean-struct rule in api-design.md § "A client depends on the wire contract…" allows hand-written TS shapes. It does not allow hand-written values.

Fix: Take one of the rule's two options.

  • Have the API's wire test write ProjectKinds, ProjectStatuses, KindLife and the terminal set into a document under api/schemas. Then have a Vitest test decode it and require that types.ts and the option lists match it.
  • Or serve the lists from the API, which already has ListProjectKinds in the store, and build the selects from that answer.

Either way, drop DEFAULT_KIND and leave kind unset, so the server's default applies.

code-architecture.2 release/pin-manifests:7 · release/pin-manifests:37 · release/pin-manifests:40 · release/pin-manifests:53 · release/pin-manifests:57 · release/pin-manifests:73 · release/pin-manifests:75 · .github/workflows/release.yml:53 · .github/workflows/release.yml:98 · .github/workflows/release.yml:136 · .github/workflows/release.yml:150 · .github/workflows/release.yml:182 · .github/workflows/release.yml:203 · .github/workflows/release.yml:222 · .github/workflows/release.yml:237 · .github/workflows/validate.yml:449 · deploy/app/kustomization.yaml:22

Reach: This costs the next image deploy/ runs, for example a worker or a migrate image of its own. This PR is that change for the second image. Taking the image set from one to two edited pin-manifests in about 7 places, release.yml in about 10, validate.yml once and the kustomization once. CLAUDE.md:349 now writes the cost into the repo's own instructions: "a new image in deploy/ takes a flag there".

Each image is spelled out separately in three files instead of read from one list

The set of images a release publishes is not stored as a list anywhere. Each member is hand-written separately at every stage:

  • release/pin-manifests: has one named flag per image (--image, --web-image), each with its own required check and its own digest check. Lines 72-75 grep the kustomization for the literal projects-web to decide whether to pin it.
  • release.yml: has a WEB_IMAGE env and build_api/build_web outputs. It runs a separate metadata-action and build-push-action per image, a separate pin call and output per image, and one env line and one argument per image in the manifests, sign, verify and release-notes steps.
  • validate.yml:449: passes one zero-digest flag per image.

The kustomization's images: list is already the one place that says which images a path runs. pin-manifests reads it only to refuse an image no flag pinned.

Fix: Make pin-manifests take repeated --image <name>=<ref> pairs. Have it pin each name the kustomization lists and refuse any listed name without a pair. Then drive release.yml from one list of (name, context) pairs, as a matrix job or a loop in one step. The list feeds decide, build, pin, sign and notes, and the step passes the pairs it built straight through. validate.yml then builds its zero-digest pairs from the same names, and the next image becomes one list entry plus its manifest.

code-architecture.3 api/authentication/browser_login.go:20 · api/authentication/browser_login.go:120 · api/main.go:99 · api/main.go:113

Reach: This costs the next route the browser login serves without a credential, for example a post-logout landing page or a front-channel logout. Two routes like this already exist. The Mount comment at browser_login.go:118-119 says "Protect has to pass them with no credential", yet the paths that comment refers to are only listed in main.go. A third route means editing Mount and main.go:99 together.

Which login routes skip authentication is decided in main.go, not by the code that registers them

BrowserLogin.Mount registers LoginPath and CallbackPath on the mux. Separately, api/main.go:99 adds the same two constants to public, the list Protect lets through with no credential. Two sites decide one fact: the set of routes the login serves before a session exists. code-quality.md § "Ask whatever owns a fact…" covers this. The type that registers the routes is the one that knows which of them must be public.

Fix: Have Mount return the paths it registered with no credential, or add func (b *BrowserLogin) PublicPaths() []string beside Mount. Then main.go appends whatever the login reports, so a new route is a change inside browser_login.go alone.

Findings as JSON
{
  "used": "I cloned the repo and checked out PR 18 at 52c3969. I read the full diff and the related code at head: api/, model/project.go, api/wire, api/schemas, the release and validate workflows, release/pin-manifests, deploy/app, web/src and CLAUDE.md. Then I searched the repo for every other site that decides the same project kinds, project statuses, image set and public paths.",
  "standards": "These findings cite `api-design.md` § \"A value the server owns reaches the client from the server\", `api-design.md` § \"A client depends on the wire contract, never on the server's types\" and `code-quality.md` § \"Ask whatever owns a fact…\". The searches for a rule on per-instance release steps and a rule on public paths beside their handler found nothing that applied.",
  "findings": [
    {
      "number": "code-architecture.1",
      "section": "design",
      "sites": [
        "web/src/api/types.ts:4",
        "web/src/api/types.ts:15",
        "web/src/components/projects/AddEditProjectModal.vue:84",
        "web/src/components/projects/AddEditProjectModal.vue:89",
        "web/src/views/ProjectsView.vue:517",
        "web/src/views/ProjectsView.vue:716",
        "model/project.go:35",
        "model/project.go:60",
        "model/project.go:61"
      ],
      "reach": "This costs the next project kind or project status. Today that change is a row in `model.ProjectKinds`/`ProjectStatuses`, which `store/postgres/open.go:52` and `store/sqlite/lookup_rows.sql` insert. `model/project.go:56` already says a new status takes decisions in `ProjectIsTerminal` and `SortProjectsForBoard`. `types.ts:3` says it outright: \"adding one is an insert plus this union\". With this PR, the same change also needs the TS union, the kind options and `DEFAULT_KIND` in AddEditProjectModal.vue, the status filter options at ProjectsView.vue:517 and the terminal-status map at ProjectsView.vue:716. No test compares any of them with `model`.",
      "body": "### The web app keeps its own copy of the project kind and status lists that `model` owns\n\n`model/project.go` decides three things:\n\n- the kinds: `ProjectKinds` = `build`, `chore`, `life`\n- the default kind: `KindLife`\n- the statuses: `ProjectStatuses` = `active`, `someday`, `completed`, `dropped`, plus `ProjectIsTerminal`\n\nThe PR types each of these again by hand in `web/`:\n\n- the `ProjectKind` and `ProjectStatus` unions in `types.ts`\n- the kind options in AddEditProjectModal.vue:84-86\n- `DEFAULT_KIND = 'life'` at AddEditProjectModal.vue:89, which repeats the default the server already applies to a create with no kind\n- the status filter options at ProjectsView.vue:517-520\n- the `completed`/`dropped` terminal map at ProjectsView.vue:716-717\n\nThe API schemas do not name these values. `api/schemas/project.schema.json` types `kind` and `status` only as \"one of the names in the project_kinds/project_statuses table\". So the server suite writes nothing the web suite could check its copy against.\n\n`api-design.md` § \"A value the server owns reaches the client from the server\" covers this case: a vocabulary is never a hand-typed copy. The lean-struct rule in `api-design.md` § \"A client depends on the wire contract…\" allows hand-written TS *shapes*. It does not allow hand-written *values*.\n\n**Fix:** Take one of the rule's two options.\n\n- Have the API's wire test write `ProjectKinds`, `ProjectStatuses`, `KindLife` and the terminal set into a document under `api/schemas`. Then have a Vitest test decode it and require that `types.ts` and the option lists match it.\n- Or serve the lists from the API, which already has `ListProjectKinds` in the store, and build the selects from that answer.\n\nEither way, drop `DEFAULT_KIND` and leave `kind` unset, so the server's default applies."
    },
    {
      "number": "code-architecture.2",
      "section": "design",
      "sites": [
        "release/pin-manifests:7",
        "release/pin-manifests:37",
        "release/pin-manifests:40",
        "release/pin-manifests:53",
        "release/pin-manifests:57",
        "release/pin-manifests:73",
        "release/pin-manifests:75",
        ".github/workflows/release.yml:53",
        ".github/workflows/release.yml:98",
        ".github/workflows/release.yml:136",
        ".github/workflows/release.yml:150",
        ".github/workflows/release.yml:182",
        ".github/workflows/release.yml:203",
        ".github/workflows/release.yml:222",
        ".github/workflows/release.yml:237",
        ".github/workflows/validate.yml:449",
        "deploy/app/kustomization.yaml:22"
      ],
      "reach": "This costs the next image `deploy/` runs, for example a worker or a migrate image of its own. This PR is that change for the second image. Taking the image set from one to two edited `pin-manifests` in about 7 places, `release.yml` in about 10, `validate.yml` once and the kustomization once. CLAUDE.md:349 now writes the cost into the repo's own instructions: \"a new image in `deploy/` takes a flag there\".",
      "body": "### Each image is spelled out separately in three files instead of read from one list\n\nThe set of images a release publishes is not stored as a list anywhere. Each member is hand-written separately at every stage:\n\n- **`release/pin-manifests`:** has one named flag per image (`--image`, `--web-image`), each with its own required check and its own digest check. Lines 72-75 grep the kustomization for the literal `projects-web` to decide whether to pin it.\n- **`release.yml`:** has a `WEB_IMAGE` env and `build_api`/`build_web` outputs. It runs a separate `metadata-action` and `build-push-action` per image, a separate `pin` call and output per image, and one env line and one argument per image in the manifests, sign, verify and release-notes steps.\n- **`validate.yml:449`:** passes one zero-digest flag per image.\n\nThe kustomization's `images:` list is already the one place that says which images a path runs. `pin-manifests` reads it only to refuse an image no flag pinned.\n\n**Fix:** Make `pin-manifests` take repeated `--image <name>=<ref>` pairs. Have it pin each name the kustomization lists and refuse any listed name without a pair. Then drive `release.yml` from one list of (name, context) pairs, as a matrix job or a loop in one step. The list feeds decide, build, pin, sign and notes, and the step passes the pairs it built straight through. `validate.yml` then builds its zero-digest pairs from the same names, and the next image becomes one list entry plus its manifest."
    },
    {
      "number": "code-architecture.3",
      "section": "design",
      "sites": [
        "api/authentication/browser_login.go:20",
        "api/authentication/browser_login.go:120",
        "api/main.go:99",
        "api/main.go:113"
      ],
      "reach": "This costs the next route the browser login serves without a credential, for example a post-logout landing page or a front-channel logout. Two routes like this already exist. The `Mount` comment at browser_login.go:118-119 says \"Protect has to pass them with no credential\", yet the paths that comment refers to are only listed in main.go. A third route means editing `Mount` and main.go:99 together.",
      "body": "### Which login routes skip authentication is decided in main.go, not by the code that registers them\n\n`BrowserLogin.Mount` registers `LoginPath` and `CallbackPath` on the mux. Separately, `api/main.go:99` adds the same two constants to `public`, the list `Protect` lets through with no credential. Two sites decide one fact: the set of routes the login serves before a session exists. `code-quality.md` § \"Ask whatever owns a fact…\" covers this. The type that registers the routes is the one that knows which of them must be public.\n\n**Fix:** Have `Mount` return the paths it registered with no credential, or add `func (b *BrowserLogin) PublicPaths() []string` beside `Mount`. Then main.go appends whatever the login reports, so a new route is a change inside browser_login.go alone."
    }
  ]
}

This review does not gate merge.

@datapointchris datapointchris left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR review — contracts

Used: Cloned and checked out PR 18 at 52c3969. Ran release/pin-manifests three ways: without --web-image (exit 2, "--web-image is required"), with a tag instead of a digest (exit 2), and with both images by digest. Ran settings.LoadServer through a throwaway main in three env combinations: no secret gives id="projects-web" and on=false; a secret without PUBLIC_URL gives the error naming PUBLIC_URL/api/auth/callback; a secret with PUBLIC_URL gives on=true. Ran a throwaway test of authentication.Protect for a no-credential GET /api/v1/projects, bearer-only against a login that never ran. Read goapiauth v0.2.0 tokenorsession.Require and refusal.Unauthorized. Searched every repo in the registry for these names: api-publish, pin-manifests, projects-web, the - Image: notes line, the manifests artifact, WEB_CLIENT_*, PUBLIC_URL and the /api/auth paths. The only other users are the sibling apps learning and nomad, which use the same names and paths.
Standards: api-design.md § "A status code on a route family keeps the one meaning its clients already read"; api-design.md § "An error answers {"error": ..., "message": ...}"; ci.md § "A deploy keyed on a release workflow's success keys on the job that built what it deploys" was checked, and no receiver keys on the renamed api-publish job.
Findings: 1 correctness · 0 design · 0 prose

Correctness

contracts.1 api/authentication/browser_login.go:110 · api/authentication/browser_login.go:31 · api/authentication/authentication.go:44 · api/main.go:95

Reach: Every request with no credential to a protected route (/api/v1/*) on a deployment with WEB_CLIENT_SECRET set, from start until BrowserLogin.Run succeeds, and for as long as the provider or the session tables stay unreachable. Callers reached: the PR's own web client (web/src/api/client.ts), which goes to the login on a 401 and shows the 503 as an error; and any anonymous probe or client that expects the bearer challenge. Shown by a throwaway test of Protect in the clone; output is in the body.

A request with no credential answers 503 instead of 401 while the browser login starts

With the login on, Protect hands every request without Authorization to tokenorsession.Require, which calls BrowserLogin.Authenticate. Before Run has built the login, Authenticate returns b.Ready(), which is errLoginNotReady. It does this for every request, including one that carries no cookie. tokenorsession treats only gobrowserlogin.ErrNoSession as "no credential". Every other error becomes a 503 session_store_unavailable, logged at ERROR.

I ran a no-credential GET /api/v1/projects through Protect in two configurations:

bearer-only (main): 401 WWW-Authenticate="Bearer" body={"code":"missing_token","error":"Unauthorized","message":"the request carries no bearer token"}
ERROR session store unavailable err="the browser login has not read the identity provider and the session tables yet" path=/api/v1/projects
login on, not ready (PR): 503 WWW-Authenticate="" body={"code":"session_store_unavailable","error":"Service Unavailable","message":"the session store is unavailable"}

This contradicts the contract the change documents for itself. The comment at browser_login.go:31 says "a login and a request carrying only a session answer 503 while the rest of the API serves". The PR body says the same. A request carrying nothing is neither of those.

The change breaks api-design.md § "A status code on a route family keeps the one meaning its clients already read". A 401 with the Bearer challenge means "present a credential". During a provider outage it becomes a 503 that blames a session store the caller never used. It also drops the challenge a client reads to learn the API wants a token. Each anonymous request, such as a scanner or an uptime probe, writes one ERROR line for the whole outage.

Fix: in Authenticate, return gobrowserlogin.ErrNoSession when the request carries no session cookie, before checking readiness. Only a request that holds a cookie should get the not-ready error. Add a test of Protect with a BrowserLogin that has not run, asserting 401 and missing_token for a request with no credential.

Findings as JSON
{
  "used": "Cloned and checked out PR 18 at 52c3969. Ran `release/pin-manifests` three ways: without `--web-image` (exit 2, \"--web-image is required\"), with a tag instead of a digest (exit 2), and with both images by digest. Ran `settings.LoadServer` through a throwaway main in three env combinations: no secret gives id=\"projects-web\" and on=false; a secret without PUBLIC_URL gives the error naming PUBLIC_URL/api/auth/callback; a secret with PUBLIC_URL gives on=true. Ran a throwaway test of `authentication.Protect` for a no-credential GET /api/v1/projects, bearer-only against a login that never ran. Read goapiauth v0.2.0 `tokenorsession.Require` and `refusal.Unauthorized`. Searched every repo in the registry for these names: api-publish, pin-manifests, projects-web, the `- Image:` notes line, the manifests artifact, WEB_CLIENT_*, PUBLIC_URL and the /api/auth paths. The only other users are the sibling apps learning and nomad, which use the same names and paths.",
  "standards": "api-design.md § \"A status code on a route family keeps the one meaning its clients already read\"; api-design.md § \"An error answers {\"error\": ..., \"message\": ...}\"; ci.md § \"A deploy keyed on a release workflow's success keys on the job that built what it deploys\" was checked, and no receiver keys on the renamed `api-publish` job.",
  "findings": [
    {
      "number": "contracts.1",
      "section": "correctness",
      "sites": [
        "api/authentication/browser_login.go:110",
        "api/authentication/browser_login.go:31",
        "api/authentication/authentication.go:44",
        "api/main.go:95"
      ],
      "reach": "Every request with no credential to a protected route (/api/v1/*) on a deployment with WEB_CLIENT_SECRET set, from start until BrowserLogin.Run succeeds, and for as long as the provider or the session tables stay unreachable. Callers reached: the PR's own web client (web/src/api/client.ts), which goes to the login on a 401 and shows the 503 as an error; and any anonymous probe or client that expects the bearer challenge. Shown by a throwaway test of Protect in the clone; output is in the body.",
      "body": "### A request with no credential answers 503 instead of 401 while the browser login starts\n\nWith the login on, `Protect` hands every request without `Authorization` to `tokenorsession.Require`, which calls `BrowserLogin.Authenticate`. Before `Run` has built the login, `Authenticate` returns `b.Ready()`, which is `errLoginNotReady`. It does this for every request, including one that carries no cookie. `tokenorsession` treats only `gobrowserlogin.ErrNoSession` as \"no credential\". Every other error becomes a 503 `session_store_unavailable`, logged at ERROR.\n\nI ran a no-credential `GET /api/v1/projects` through `Protect` in two configurations:\n\n```\nbearer-only (main): 401 WWW-Authenticate=\"Bearer\" body={\"code\":\"missing_token\",\"error\":\"Unauthorized\",\"message\":\"the request carries no bearer token\"}\nERROR session store unavailable err=\"the browser login has not read the identity provider and the session tables yet\" path=/api/v1/projects\nlogin on, not ready (PR): 503 WWW-Authenticate=\"\" body={\"code\":\"session_store_unavailable\",\"error\":\"Service Unavailable\",\"message\":\"the session store is unavailable\"}\n```\n\nThis contradicts the contract the change documents for itself. The comment at `browser_login.go:31` says \"a login and a request carrying only a session answer 503 while the rest of the API serves\". The PR body says the same. A request carrying nothing is neither of those.\n\nThe change breaks api-design.md § \"A status code on a route family keeps the one meaning its clients already read\". A 401 with the `Bearer` challenge means \"present a credential\". During a provider outage it becomes a 503 that blames a session store the caller never used. It also drops the challenge a client reads to learn the API wants a token. Each anonymous request, such as a scanner or an uptime probe, writes one ERROR line for the whole outage.\n\n**Fix:** in `Authenticate`, return `gobrowserlogin.ErrNoSession` when the request carries no session cookie, before checking readiness. Only a request that holds a cookie should get the not-ready error. Add a test of `Protect` with a `BrowserLogin` that has not run, asserting 401 and `missing_token` for a request with no credential."
    }
  ]
}

This review does not gate merge.

@datapointchris datapointchris left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR review — stale-prose

Used: I checked out PR 18 at 52c3969 and read every sentence it adds in these places: the docs (README.md, CLAUDE.md), the config comments (Taskfile, workflows, compose, Caddyfile, Dockerfile, the manifests, the migration), and the comments in the Go and web sources. I checked each against the code at the head and against the pinned goapiauth v0.2.0 and gobrowserlogin v0.1.2 sources in the module cache. I searched with rg to find which tasks, composables, attributes and classes the comments name.
Standards: Rules cited: pull-requests.md/Sentences the diff adds are checked for what goes stale; documentation.md/A count, version or measurement pinned in a comment or doc names what made it true and how it was measured; code-quality.md/A limit is set from the peak of what actually runs through it; documentation.md/A doc points at the source instead of restating its content or constants; documentation.md/A comment states what this repos code enforces, never another repos deployment as fact.
Findings: 0 correctness · 0 design · 4 prose

Prose

stale-prose.1 web/src/assets/sass/abstracts/_breakpoints.scss:2 · web/src/assets/sass/abstracts/_shape.scss:1 · web/src/assets/sass/abstracts/_shape.scss:3 · web/src/assets/sass/abstracts/_colors.scss:19 · web/src/assets/sass/abstracts/_colors.scss:28 · web/src/assets/sass/abstracts/_colors.scss:36 · web/src/assets/sass/abstracts/_colors.scss:37 · web/src/assets/sass/components/_buttons.scss:5 · web/src/assets/sass/base/_base.scss:5 · web/src/assets/sass/components/_add_edit_modal.scss:141 · web/src/assets/sass/components/_add_edit_modal.scss:190

False. The copied stylesheet's comments describe the app it was copied from, and this repo holds none of what they name. The PR body calls the styles "a copy, trimmed to what the page uses", but the comments were not trimmed with them.

  • _breakpoints.scss:2: "base/base publishes it as --phone-max-width, which is how usePhoneViewport reads the same value." rg usePhoneViewport web/src finds only this comment. No composable reads --phone-max-width.
    • Rewrite: "base/base publishes it as --phone-max-width, so a script can read the same value." Delete the sentence instead if nothing is meant to read it.
  • _shape.scss:1-3: "The shape a design style swaps … a design style redefines these under its data-design attribute." rg data-design web/src finds only this comment. The PR says the page is fixed at one style.
    • Rewrite: "Corners, role borders and motion timing, in one place."
  • _buttons.scss:5: "…so a design style can swap every one of them in one place."
    • Rewrite: "The corner a panel, card or code block takes."
  • _colors.scss:19 "overridden by named themes" and _colors.scss:28 "named themes override with palette-matched values". No theme exists here. Delete both clauses.
  • _colors.scss:36-37: "DEPRECATED — aliases for backwards compatibility with durations color data stored in DB. These will be removed after a data migration." Durations is another app. Its data is not in this database, and rg 'clr-accent--' web/src finds no user of these aliases outside the file. Delete the comment and the aliases.
  • _base.scss:5: "The bar holding the menu button on a phone." The page has no menu button and no phone bar. --phone-bar-height is read only by --page-sticky-top, which nothing reads. Delete it with the variables it describes.
  • _add_edit_modal.scss:141 "Tile selector for categories/labels" and :190 "Priority input". No component uses add-edit-modal__tile*, and no field here is a priority. Delete them with their rules. Failing that, write "A radio group drawn as tiles" and "A number input without spinner arrows".

stale-prose.2 web/playwright.config.ts:4

False. The Playwright config names a task that does not exist.

  • web/playwright.config.ts:4: "The smoke runs against the dev stack, which task web:smoke starts and stops".

rg -n 'web:smoke|web:test:e2e' -uu finds web:smoke only in this comment. The Taskfile defines web:test:e2e (Taskfile.yml:92). That task starts the stack and removes it afterwards. CI's web-stack job starts the stack itself and never runs the task.

Rewrite: "The smoke runs against the dev stack in docker-compose.dev.yml: Caddy serving the build, the API, Postgres, and a stand-in identity provider. task web:test:e2e starts a fresh one and removes it afterwards."

stale-prose.3 deploy/app/web.yaml:98 · store/postgres/migrations/00002_browser_sessions.sql:5

Will go stale. Two comments state a number that comes from outside the file, and nothing keeps either level with its source.

  • deploy/app/web.yaml:98: "Caddy holds about 15Mi serving the page." This is a measurement with no command beside it. The PR body measured it with --read-only --cap-drop ALL on the image, but the manifest does not say so. A Caddy bump or a larger build changes the figure, and the 64Mi limit beside it is justified by it. This breaks documentation.md § "A count, version or measurement pinned in a comment or doc names what made it true and how it was measured" and code-quality.md § "A limit is set from the peak of what actually runs through it".
    • Rewrite: "Caddy held 15Mi serving the page as docker stats read it on the dev stack's web container, for caddy:2.11. Measure again after a Caddy bump. The limit equals the request…" Alternatively, drop the number and keep only the reason the limit equals the request.
  • 00002_browser_sessions.sql:5: "a session's row is written on every login and each minute it is used". "Each minute" is gobrowserlogin's private touchInterval = time.Minute (sessions.go:46 at v0.1.2). This repo does not set it, so a library bump can change it silently. This breaks documentation.md § "A comment states what this repos code enforces".
    • Rewrite: "a session's row is written on every login and again as it is used, and no person edits one."

stale-prose.4 CLAUDE.md:388

A value the code holds. CLAUDE.md copies the stack's three ports out of the files that set them.

  • CLAUDE.md:388: "The stack's ports are 8096 for the page and 8097 for the stand-in, and Vite's is 3002."

The ports are set in docker-compose.dev.yml (127.0.0.1:8096:8080, SERVER_PORT: '8097', OIDC_ISSUER: http://localhost:8097/projects) and in web/vite.config.ts (port: 3002). The command block above this bullet already shows the two URLs. A port change leaves this bullet wrong. This breaks documentation.md § "A doc points at the source instead of restating its content or constants".

Rewrite: "The stack's ports are set in docker-compose.dev.yml, and Vite's in web/vite.config.ts." Deleting the bullet also works, because the command block shows the URLs.

Findings as JSON
{
  "used": "I checked out PR 18 at 52c3969 and read every sentence it adds in these places: the docs (`README.md`, `CLAUDE.md`), the config comments (Taskfile, workflows, compose, Caddyfile, Dockerfile, the manifests, the migration), and the comments in the Go and web sources. I checked each against the code at the head and against the pinned goapiauth v0.2.0 and gobrowserlogin v0.1.2 sources in the module cache. I searched with `rg` to find which tasks, composables, attributes and classes the comments name.",
  "standards": "Rules cited: pull-requests.md/Sentences the diff adds are checked for what goes stale; documentation.md/A count, version or measurement pinned in a comment or doc names what made it true and how it was measured; code-quality.md/A limit is set from the peak of what actually runs through it; documentation.md/A doc points at the source instead of restating its content or constants; documentation.md/A comment states what this repos code enforces, never another repos deployment as fact.",
  "findings": [
    {
      "number": "stale-prose.1",
      "section": "prose",
      "sites": [
        "web/src/assets/sass/abstracts/_breakpoints.scss:2",
        "web/src/assets/sass/abstracts/_shape.scss:1",
        "web/src/assets/sass/abstracts/_shape.scss:3",
        "web/src/assets/sass/abstracts/_colors.scss:19",
        "web/src/assets/sass/abstracts/_colors.scss:28",
        "web/src/assets/sass/abstracts/_colors.scss:36",
        "web/src/assets/sass/abstracts/_colors.scss:37",
        "web/src/assets/sass/components/_buttons.scss:5",
        "web/src/assets/sass/base/_base.scss:5",
        "web/src/assets/sass/components/_add_edit_modal.scss:141",
        "web/src/assets/sass/components/_add_edit_modal.scss:190"
      ],
      "reach": "",
      "body": "**False. The copied stylesheet's comments describe the app it was copied from, and this repo holds none of what they name.** The PR body calls the styles \"a copy, trimmed to what the page uses\", but the comments were not trimmed with them.\n\n- `_breakpoints.scss:2`: \"`base/base` publishes it as `--phone-max-width`, which is how `usePhoneViewport` reads the same value.\" `rg usePhoneViewport web/src` finds only this comment. No composable reads `--phone-max-width`.\n  - Rewrite: \"`base/base` publishes it as `--phone-max-width`, so a script can read the same value.\" Delete the sentence instead if nothing is meant to read it.\n- `_shape.scss:1-3`: \"The shape a design style swaps … a design style redefines these under its `data-design` attribute.\" `rg data-design web/src` finds only this comment. The PR says the page is fixed at one style.\n  - Rewrite: \"Corners, role borders and motion timing, in one place.\"\n- `_buttons.scss:5`: \"…so a design style can swap every one of them in one place.\"\n  - Rewrite: \"The corner a panel, card or code block takes.\"\n- `_colors.scss:19` \"overridden by named themes\" and `_colors.scss:28` \"named themes override with palette-matched values\". No theme exists here. Delete both clauses.\n- `_colors.scss:36-37`: \"DEPRECATED — aliases for backwards compatibility with durations color data stored in DB. These will be removed after a data migration.\" Durations is another app. Its data is not in this database, and `rg 'clr-accent--' web/src` finds no user of these aliases outside the file. Delete the comment and the aliases.\n- `_base.scss:5`: \"The bar holding the menu button on a phone.\" The page has no menu button and no phone bar. `--phone-bar-height` is read only by `--page-sticky-top`, which nothing reads. Delete it with the variables it describes.\n- `_add_edit_modal.scss:141` \"Tile selector for categories/labels\" and `:190` \"Priority input\". No component uses `add-edit-modal__tile*`, and no field here is a priority. Delete them with their rules. Failing that, write \"A radio group drawn as tiles\" and \"A number input without spinner arrows\"."
    },
    {
      "number": "stale-prose.2",
      "section": "prose",
      "sites": [
        "web/playwright.config.ts:4"
      ],
      "reach": "",
      "body": "**False. The Playwright config names a task that does not exist.**\n\n- `web/playwright.config.ts:4`: \"The smoke runs against the dev stack, which `task web:smoke` starts and stops\".\n\n`rg -n 'web:smoke|web:test:e2e' -uu` finds `web:smoke` only in this comment. The Taskfile defines `web:test:e2e` (`Taskfile.yml:92`). That task starts the stack and removes it afterwards. CI's `web-stack` job starts the stack itself and never runs the task.\n\nRewrite: \"The smoke runs against the dev stack in `docker-compose.dev.yml`: Caddy serving the build, the API, Postgres, and a stand-in identity provider. `task web:test:e2e` starts a fresh one and removes it afterwards.\""
    },
    {
      "number": "stale-prose.3",
      "section": "prose",
      "sites": [
        "deploy/app/web.yaml:98",
        "store/postgres/migrations/00002_browser_sessions.sql:5"
      ],
      "reach": "",
      "body": "**Will go stale. Two comments state a number that comes from outside the file, and nothing keeps either level with its source.**\n\n- `deploy/app/web.yaml:98`: \"Caddy holds about 15Mi serving the page.\" This is a measurement with no command beside it. The PR body measured it with `--read-only --cap-drop ALL` on the image, but the manifest does not say so. A Caddy bump or a larger build changes the figure, and the 64Mi limit beside it is justified by it. This breaks `documentation.md` § \"A count, version or measurement pinned in a comment or doc names what made it true and how it was measured\" and `code-quality.md` § \"A limit is set from the peak of what actually runs through it\".\n  - Rewrite: \"Caddy held 15Mi serving the page as `docker stats` read it on the dev stack's `web` container, for caddy:2.11. Measure again after a Caddy bump. The limit equals the request…\" Alternatively, drop the number and keep only the reason the limit equals the request.\n- `00002_browser_sessions.sql:5`: \"a session's row is written on every login and each minute it is used\". \"Each minute\" is gobrowserlogin's private `touchInterval = time.Minute` (`sessions.go:46` at v0.1.2). This repo does not set it, so a library bump can change it silently. This breaks `documentation.md` § \"A comment states what this repos code enforces\".\n  - Rewrite: \"a session's row is written on every login and again as it is used, and no person edits one.\""
    },
    {
      "number": "stale-prose.4",
      "section": "prose",
      "sites": [
        "CLAUDE.md:388"
      ],
      "reach": "",
      "body": "**A value the code holds. `CLAUDE.md` copies the stack's three ports out of the files that set them.**\n\n- `CLAUDE.md:388`: \"**The stack's ports are 8096 for the page and 8097 for the stand-in**, and Vite's is 3002.\"\n\nThe ports are set in `docker-compose.dev.yml` (`127.0.0.1:8096:8080`, `SERVER_PORT: '8097'`, `OIDC_ISSUER: http://localhost:8097/projects`) and in `web/vite.config.ts` (`port: 3002`). The command block above this bullet already shows the two URLs. A port change leaves this bullet wrong. This breaks `documentation.md` § \"A doc points at the source instead of restating its content or constants\".\n\nRewrite: \"**The stack's ports are set in `docker-compose.dev.yml`, and Vite's in `web/vite.config.ts`.**\" Deleting the bullet also works, because the command block shows the URLs."
    }
  ]
}

This review does not gate merge.

@datapointchris datapointchris left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR review — useless-test

Used: I cloned PR 18 and checked out its head. I ran npm ci and Vitest in web/ (73 passed). I removed itemBlockers.value = {} from fetchItems (projects.ts:171) and ran the store suite: 29 of 29 passed. I removed the selectedProjectId === null guard from reorderItems (projects.ts:354) and ran its test: it passed. I read every added or edited test beside the code it covers: client.test.ts, projects.test.ts, ProjectsView.test.ts, e2e/projects.spec.ts, and the two edits in routes_test.go and store_test.go. I kept the ProjectsView tests that look for "Loading...", "Select a project to view items", "No items in this project" and "No results found". Each is the only test that pins which pane a v-if shows in that state.
Standards: testing.md § "Test behavior the user cares about, not what this commit happened to change" (the shape "pins a configuration only the fixtures build"); pull-requests.md § "Tests the diff adds are checked for whether they test the program or the change"; testing.md § "Never assert on rendered output — assert the value it was built from".
Findings: 0 correctness · 1 design · 0 prose

Design and architecture

useless-test.1 web/src/stores/__tests__/projects.test.ts:162 · web/src/stores/__tests__/projects.test.ts:241 · web/src/stores/projects.ts:171 · web/src/stores/projects.ts:354

Reach: Both tests run under task test and in CI. Each still passes with the store line it is named for deleted. Neither can fail on a change a user would notice.

Two store tests pass with the code they name deleted

Delete both tests:

  • web/src/stores/__tests__/projects.test.ts › useProjectsStore › clears blockers when fetching new items
  • web/src/stores/__tests__/projects.test.ts › useProjectsStore › does nothing when no project is selected

What I ran. I deleted itemBlockers.value = {} from fetchItems (projects.ts:171) and ran npx vitest run src/stores. All 29 tests passed. I deleted if (selectedProjectId.value === null) return from reorderItems (projects.ts:354) and ran the second test alone. It passed.

Why the blocker test is empty. fetchItems calls fetchItemBlockers(), and that call replaces the whole map with what the API returned. The mock returns no blockers, so the map ends up {} whether or not the clear ran. The test's own comment says the map was refilled by fetchItemBlockers. That refill is already pinned by fetches blockers for all items in project.

The cleared map is a real behavior. While the next project's blockers load, the old ones should not show. If you want that pinned, a replacement test makes /blockers return a promise that never settles. It then calls await store.fetchItems(1) with itemBlockers already filled and asserts store.itemBlockers equals {}. That test fails when line 171 is removed.

Why the guard test is empty, and why the guard is not behavior anyway. store.items is [] in that test, so reorderItems finds nothing to move and sends no PATCH, guard or not. Even a test that set items would be pinning a state that only the test builds. reorderItems has one caller, onItemDragEnd in ProjectsView.vue:607. That draggable list renders only when selectedProjectIds.length === 1. Every store path that leaves one id in that list also sets selectedProjectId: fetchItems at :167 and toggleProjectSelection through fetchItems. The guard exists so TypeScript can narrow the type, and nothing a user does reaches it. testing.md § "Test behavior the user cares about, not what this commit happened to change" names this shape: a nil guard on a configuration that only fixtures build. No replacement test is needed.

Findings as JSON
{
  "used": "I cloned PR 18 and checked out its head. I ran `npm ci` and Vitest in `web/` (73 passed). I removed `itemBlockers.value = {}` from `fetchItems` (projects.ts:171) and ran the store suite: 29 of 29 passed. I removed the `selectedProjectId === null` guard from `reorderItems` (projects.ts:354) and ran its test: it passed. I read every added or edited test beside the code it covers: `client.test.ts`, `projects.test.ts`, `ProjectsView.test.ts`, `e2e/projects.spec.ts`, and the two edits in `routes_test.go` and `store_test.go`. I kept the ProjectsView tests that look for \"Loading...\", \"Select a project to view items\", \"No items in this project\" and \"No results found\". Each is the only test that pins which pane a `v-if` shows in that state.",
  "standards": "testing.md § \"Test behavior the user cares about, not what this commit happened to change\" (the shape \"pins a configuration only the fixtures build\"); pull-requests.md § \"Tests the diff adds are checked for whether they test the program or the change\"; testing.md § \"Never assert on rendered output — assert the value it was built from\".",
  "findings": [
    {
      "number": "useless-test.1",
      "section": "design",
      "sites": [
        "web/src/stores/__tests__/projects.test.ts:162",
        "web/src/stores/__tests__/projects.test.ts:241",
        "web/src/stores/projects.ts:171",
        "web/src/stores/projects.ts:354"
      ],
      "reach": "Both tests run under `task test` and in CI. Each still passes with the store line it is named for deleted. Neither can fail on a change a user would notice.",
      "body": "## Two store tests pass with the code they name deleted\n\nDelete both tests:\n\n- `web/src/stores/__tests__/projects.test.ts` › `useProjectsStore` › **clears blockers when fetching new items**\n- `web/src/stores/__tests__/projects.test.ts` › `useProjectsStore` › **does nothing when no project is selected**\n\n**What I ran.** I deleted `itemBlockers.value = {}` from `fetchItems` (projects.ts:171) and ran `npx vitest run src/stores`. All 29 tests passed. I deleted `if (selectedProjectId.value === null) return` from `reorderItems` (projects.ts:354) and ran the second test alone. It passed.\n\n**Why the blocker test is empty.** `fetchItems` calls `fetchItemBlockers()`, and that call replaces the whole map with what the API returned. The mock returns no blockers, so the map ends up `{}` whether or not the clear ran. The test's own comment says the map was refilled by `fetchItemBlockers`. That refill is already pinned by **fetches blockers for all items in project**.\n\nThe cleared map is a real behavior. While the next project's blockers load, the old ones should not show. If you want that pinned, a replacement test makes `/blockers` return a promise that never settles. It then calls `await store.fetchItems(1)` with `itemBlockers` already filled and asserts `store.itemBlockers` equals `{}`. That test fails when line 171 is removed.\n\n**Why the guard test is empty, and why the guard is not behavior anyway.** `store.items` is `[]` in that test, so `reorderItems` finds nothing to move and sends no PATCH, guard or not. Even a test that set items would be pinning a state that only the test builds. `reorderItems` has one caller, `onItemDragEnd` in ProjectsView.vue:607. That draggable list renders only when `selectedProjectIds.length === 1`. Every store path that leaves one id in that list also sets `selectedProjectId`: `fetchItems` at :167 and `toggleProjectSelection` through `fetchItems`. The guard exists so TypeScript can narrow the type, and nothing a user does reaches it. testing.md § \"Test behavior the user cares about, not what this commit happened to change\" names this shape: a nil guard on a configuration that only fixtures build. No replacement test is needed."
    }
  ]
}

This review does not gate merge.

@datapoints-fleet-automation datapoints-fleet-automation Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR review — fleet-patterns

Used: I cloned the PR head and checked it out. I cloned these siblings for comparison: meso, goapiauth, gobrowserlogin, slate, reusable-workflows, backup-dashboard and ichrisbirch. I diffed the PR's BrowserLogin against goapiauth.Connecting, its pin-manifests and release.yml against slate's, and its Caddyfile and client.ts against meso's. I also diffed the session migration against postgresstore's schema.sql.
Standards: These rules apply: code-quality.md "Ask whatever owns a fact… never duplicate its logic at a second site", go.md "Ask the library to export what you would otherwise copy out of it", frontend.md "A browser front end fails a write on a 401 in place…" and kubernetes.md "An app's release pushes its image and its manifests to the registry…". The search "spa caddy fallback missing asset 404" found no rule on Caddy or SPA serving.
Findings: 0 correctness · 4 design · 0 prose

Design and architecture

fleet-patterns.1 api/authentication/browser_login.go:33 · api/authentication/browser_login.go:63 · api/authentication/browser_login.go:76 · goapiauth:connecting.go:38 · goapiauth:connecting.go:55 · meso:api/main.go:84

Reach: meso is the other gobrowserlogin consumer. When its provider is down at start, meso still exits at api/main.go:85-87. Making meso survive that outage means a third copy of this loop. The next change to the retry, such as reconnecting after a discovery failure, lands in only one copy. The copies already disagree about how a misconfigured provider is logged.

BrowserLogin copies goapiauth.Connecting for gobrowserlogin, and the copy lives in projects.

BrowserLogin reuses goapiauth.Connecting's design line for line:

  • the same firstRetry/lastRetry fields with the same defaults of 1s and 1m
  • atomic.Pointer fields for the built value and the last failure
  • Run with the same doubling select loop
  • Ready returning the last failure
  • an accessor that refuses with a not-ready error

The PR body says it "matches" the verifier, so the copy is intentional.

The copies have already diverged:

  • Connecting.Run logs a misconfigured provider at Error and an unreachable one at Warn (connecting.go:52-56).
  • BrowserLogin.Run logs every failure at Warn (browser_login.go:76). A provider that refuses the client's config shows up here only as a warning.

meso, the other gobrowserlogin consumer, builds the login at start and exits when the provider is down (meso:api/main.go:84-87). The outage handling this PR adds stays in projects only.

go.md § "Ask the library to export what you would otherwise copy out of it" covers this. So does code-quality.md § "Ask whatever owns a fact": across two tools, the owner is a library both import.

Fix: export the background-start wrapper from gobrowserlogin, alongside New. Something like gobrowserlogin.NewConnecting(config, storeFn) with Run, Ready, Authenticate, Login and Callback, the same shape as goapiauth's Connecting. It should log errors as Connecting does, Error for misconfiguration and Warn otherwise. Then projects drops browser_login.go to a constructor call, and meso picks up the same outage behaviour.

fleet-patterns.2 release/pin-manifests:72 · release/pin-manifests:79 · .github/workflows/release.yml:77 · slate:release/pin-manifests:67 · slate:release/pin-manifests:70 · slate:.github/workflows/release.yml:273

Reach: On main, release/pin-manifests is slate's script with only the image name changed, and the publish job matches slate's step for step. This PR adds a guard that refuses a rendered path running an image without a digest (pin-manifests:78-80). slate's copy still runs a bare kustomize build "$path" >/dev/null. So slate ships a manifest with an unpinned image without complaint, and the next fix to either script will again reach one repo only.

projects and slate copy the image-publish pipeline, and this PR widens the gap between them.

Before this PR, projects' release/pin-manifests differed from slate's only in the literal projects/slate. The publish job's "Decide what to publish" step is the same in both repos (release.yml:77 here, slate:.github/workflows/release.yml:273).

This PR adds three things, and only projects gets them:

  • a second image, --web-image
  • a guard that refuses an unpinned image after kustomize build (pin-manifests:78-80)
  • a per-image published check

The guard enforces kubernetes.md § "Name an image by digest". slate does not have it.

projects already calls datapointchris/reusable-workflows/.github/workflows/go-semantic-release.yml@v1 from this same release.yml. So the fleet already has a home for release logic that every repo shares.

Fix: move pin-manifests into reusable-workflows as a reusable publish workflow, together with the decide, build, pin, flux-push and cosign steps. It should take a list of images, each with a name, a build context and a kustomize image name. Then projects passes two entries and slate passes one. The unpinned-image guard then reaches both.

fleet-patterns.3 web/Caddyfile.docker:36 · web/Caddyfile.docker:45 · meso:web/Caddyfile.docker:18 · meso:web/Caddyfile.docker:23 · meso:web/Caddyfile.docker:28

Reach: meso is the only other copy, so the evidence is weak, and the two copies already disagree. meso's Caddyfile still answers a missing /assets/* file with index.html. Its @hashed matcher then sends that page with immutable and a one-year max-age. A tab on an older meso release keeps that page as a script for a year. That is the bug this PR's body names and fixes here only. The next change to the shared SPA image, such as a CSP or header policy, again needs a hand edit in both repos.

The web image's Caddyfile and Dockerfile copy meso's, and only the copy got the fixes.

web/Caddyfile.docker keeps meso's structure:

  • a /health handle
  • the API proxy
  • the try_files {path} /index.html fallback
  • the same max-age=300, must-revalidate and one-year immutable split
  • the same header block

web/Dockerfile likewise copies meso's node:24-alpine to caddy two-stage build.

The PR body gives reasons for two fixes:

  • A missing /assets/* file answers 404 rather than the SPA page cached as immutable.
  • All of /.well-known/* goes to the API, so no document under it gets the page.

Those are real defects, and meso still has both (meso:web/Caddyfile.docker:18-38). This PR's version is better. Fixing it here alone leaves the fleet's two Vue+Go apps serving the SPA differently.

Fix: make one owner for the SPA-serving image, such as a base image or a Caddyfile snippet, that both repos import. Each app then supplies only its upstream (projects-api:8000, api:8088) and its CSP extras. Failing that, port the two fixes to meso in the same round and name the shared origin in both Caddyfiles.

fleet-patterns.4 web/src/api/client.ts:15 · web/src/api/client.ts:50 · web/src/api/client.ts:143 · meso:web/src/api/client.ts:34 · meso:web/src/api/client.ts:80 · gobrowserlogin:sessions.go:18

Reach: The login-loop guard is copied verbatim from meso: LOGIN_WINDOW_MS, LOGIN_STARTS_ALLOWED, loginStarts, sendToLogin and the return_to navigation. The X-CSRF header name hand-copies gobrowserlogin.CSRFHeader. The copies already disagree. projects has the typed-input read half that frontend.md requires (client.ts:143), and meso does not (client.ts:80-85). A change to gobrowserlogin's header or return_to contract silently breaks both clients.

The browser-login client half is copied from meso and hand-copies gobrowserlogin's contract.

sendToLogin, recentLoginStarts, the two-starts-a-minute window, the sessionStorage key and the comment explaining them are meso's text, ported from fetch to axios. The 'X-CSRF' literal at client.ts:15 is a hand copy of gobrowserlogin.CSRFHeader (gobrowserlogin:sessions.go:18). The return_to parameter is gobrowserlogin's Login contract.

frontend.md § "A browser front end fails a write on a 401 in place…" says every product carrying the handler carries both halves. This PR adds the typed-input half (holdsTypedInput). meso, the copy it came from, still lacks that half. That gap is the drift code-quality.md § "Ask whatever owns a fact" predicts.

Fix: have gobrowserlogin's repo publish a small npm package with the client half: the CSRF header constant, sendToLogin with its loop guard, and the 401 policy as a function of method and holdsTypedInput. projects and meso then each import it, whether they use axios or fetch. Without that package, the typed-input half has to be ported to meso by hand, and the next contract change needs edits in three places.

Findings as JSON
{
  "used": "I cloned the PR head and checked it out. I cloned these siblings for comparison: meso, goapiauth, gobrowserlogin, slate, reusable-workflows, backup-dashboard and ichrisbirch. I diffed the PR's BrowserLogin against goapiauth.Connecting, its pin-manifests and release.yml against slate's, and its Caddyfile and client.ts against meso's. I also diffed the session migration against postgresstore's schema.sql.",
  "standards": "These rules apply: code-quality.md \"Ask whatever owns a fact… never duplicate its logic at a second site\", go.md \"Ask the library to export what you would otherwise copy out of it\", frontend.md \"A browser front end fails a write on a 401 in place…\" and kubernetes.md \"An app's release pushes its image and its manifests to the registry…\". The search \"spa caddy fallback missing asset 404\" found no rule on Caddy or SPA serving.",
  "findings": [
    {
      "number": "fleet-patterns.1",
      "section": "design",
      "sites": [
        "api/authentication/browser_login.go:33",
        "api/authentication/browser_login.go:63",
        "api/authentication/browser_login.go:76",
        "goapiauth:connecting.go:38",
        "goapiauth:connecting.go:55",
        "meso:api/main.go:84"
      ],
      "reach": "meso is the other gobrowserlogin consumer. When its provider is down at start, meso still exits at api/main.go:85-87. Making meso survive that outage means a third copy of this loop. The next change to the retry, such as reconnecting after a discovery failure, lands in only one copy. The copies already disagree about how a misconfigured provider is logged.",
      "body": "**BrowserLogin copies goapiauth.Connecting for gobrowserlogin, and the copy lives in projects.**\n\n`BrowserLogin` reuses `goapiauth.Connecting`'s design line for line:\n\n- the same `firstRetry`/`lastRetry` fields with the same defaults of 1s and 1m\n- `atomic.Pointer` fields for the built value and the last failure\n- `Run` with the same doubling `select` loop\n- `Ready` returning the last failure\n- an accessor that refuses with a not-ready error\n\nThe PR body says it \"matches\" the verifier, so the copy is intentional.\n\nThe copies have already diverged:\n\n- `Connecting.Run` logs a misconfigured provider at Error and an unreachable one at Warn (connecting.go:52-56).\n- `BrowserLogin.Run` logs every failure at Warn (browser_login.go:76). A provider that refuses the client's config shows up here only as a warning.\n\nmeso, the other gobrowserlogin consumer, builds the login at start and exits when the provider is down (meso:api/main.go:84-87). The outage handling this PR adds stays in projects only.\n\ngo.md § \"Ask the library to export what you would otherwise copy out of it\" covers this. So does code-quality.md § \"Ask whatever owns a fact\": across two tools, the owner is a library both import.\n\n**Fix:** export the background-start wrapper from gobrowserlogin, alongside `New`. Something like `gobrowserlogin.NewConnecting(config, storeFn)` with `Run`, `Ready`, `Authenticate`, `Login` and `Callback`, the same shape as goapiauth's `Connecting`. It should log errors as Connecting does, Error for misconfiguration and Warn otherwise. Then projects drops browser_login.go to a constructor call, and meso picks up the same outage behaviour."
    },
    {
      "number": "fleet-patterns.2",
      "section": "design",
      "sites": [
        "release/pin-manifests:72",
        "release/pin-manifests:79",
        ".github/workflows/release.yml:77",
        "slate:release/pin-manifests:67",
        "slate:release/pin-manifests:70",
        "slate:.github/workflows/release.yml:273"
      ],
      "reach": "On main, `release/pin-manifests` is slate's script with only the image name changed, and the publish job matches slate's step for step. This PR adds a guard that refuses a rendered path running an image without a digest (pin-manifests:78-80). slate's copy still runs a bare `kustomize build \"$path\" >/dev/null`. So slate ships a manifest with an unpinned image without complaint, and the next fix to either script will again reach one repo only.",
      "body": "**projects and slate copy the image-publish pipeline, and this PR widens the gap between them.**\n\nBefore this PR, projects' `release/pin-manifests` differed from slate's only in the literal `projects`/`slate`. The publish job's \"Decide what to publish\" step is the same in both repos (release.yml:77 here, slate:.github/workflows/release.yml:273).\n\nThis PR adds three things, and only projects gets them:\n\n- a second image, `--web-image`\n- a guard that refuses an unpinned image after `kustomize build` (pin-manifests:78-80)\n- a per-image published check\n\nThe guard enforces kubernetes.md § \"Name an image by digest\". slate does not have it.\n\nprojects already calls `datapointchris/reusable-workflows/.github/workflows/go-semantic-release.yml@v1` from this same release.yml. So the fleet already has a home for release logic that every repo shares.\n\n**Fix:** move pin-manifests into reusable-workflows as a reusable publish workflow, together with the decide, build, pin, flux-push and cosign steps. It should take a list of images, each with a name, a build context and a kustomize image name. Then projects passes two entries and slate passes one. The unpinned-image guard then reaches both."
    },
    {
      "number": "fleet-patterns.3",
      "section": "design",
      "sites": [
        "web/Caddyfile.docker:36",
        "web/Caddyfile.docker:45",
        "meso:web/Caddyfile.docker:18",
        "meso:web/Caddyfile.docker:23",
        "meso:web/Caddyfile.docker:28"
      ],
      "reach": "meso is the only other copy, so the evidence is weak, and the two copies already disagree. meso's Caddyfile still answers a missing `/assets/*` file with index.html. Its `@hashed` matcher then sends that page with `immutable` and a one-year max-age. A tab on an older meso release keeps that page as a script for a year. That is the bug this PR's body names and fixes here only. The next change to the shared SPA image, such as a CSP or header policy, again needs a hand edit in both repos.",
      "body": "**The web image's Caddyfile and Dockerfile copy meso's, and only the copy got the fixes.**\n\n`web/Caddyfile.docker` keeps meso's structure:\n\n- a `/health` handle\n- the API proxy\n- the `try_files {path} /index.html` fallback\n- the same `max-age=300, must-revalidate` and one-year `immutable` split\n- the same header block\n\n`web/Dockerfile` likewise copies meso's `node:24-alpine` to `caddy` two-stage build.\n\nThe PR body gives reasons for two fixes:\n\n- A missing `/assets/*` file answers 404 rather than the SPA page cached as `immutable`.\n- All of `/.well-known/*` goes to the API, so no document under it gets the page.\n\nThose are real defects, and meso still has both (meso:web/Caddyfile.docker:18-38). This PR's version is better. Fixing it here alone leaves the fleet's two Vue+Go apps serving the SPA differently.\n\n**Fix:** make one owner for the SPA-serving image, such as a base image or a Caddyfile snippet, that both repos import. Each app then supplies only its upstream (`projects-api:8000`, `api:8088`) and its CSP extras. Failing that, port the two fixes to meso in the same round and name the shared origin in both Caddyfiles."
    },
    {
      "number": "fleet-patterns.4",
      "section": "design",
      "sites": [
        "web/src/api/client.ts:15",
        "web/src/api/client.ts:50",
        "web/src/api/client.ts:143",
        "meso:web/src/api/client.ts:34",
        "meso:web/src/api/client.ts:80",
        "gobrowserlogin:sessions.go:18"
      ],
      "reach": "The login-loop guard is copied verbatim from meso: `LOGIN_WINDOW_MS`, `LOGIN_STARTS_ALLOWED`, `loginStarts`, `sendToLogin` and the `return_to` navigation. The `X-CSRF` header name hand-copies `gobrowserlogin.CSRFHeader`. The copies already disagree. projects has the typed-input read half that frontend.md requires (client.ts:143), and meso does not (client.ts:80-85). A change to gobrowserlogin's header or `return_to` contract silently breaks both clients.",
      "body": "**The browser-login client half is copied from meso and hand-copies gobrowserlogin's contract.**\n\n`sendToLogin`, `recentLoginStarts`, the two-starts-a-minute window, the `sessionStorage` key and the comment explaining them are meso's text, ported from `fetch` to axios. The `'X-CSRF'` literal at client.ts:15 is a hand copy of `gobrowserlogin.CSRFHeader` (gobrowserlogin:sessions.go:18). The `return_to` parameter is gobrowserlogin's `Login` contract.\n\nfrontend.md § \"A browser front end fails a write on a 401 in place…\" says every product carrying the handler carries both halves. This PR adds the typed-input half (`holdsTypedInput`). meso, the copy it came from, still lacks that half. That gap is the drift code-quality.md § \"Ask whatever owns a fact\" predicts.\n\n**Fix:** have gobrowserlogin's repo publish a small npm package with the client half: the CSRF header constant, `sendToLogin` with its loop guard, and the 401 policy as a function of method and `holdsTypedInput`. projects and meso then each import it, whether they use axios or fetch. Without that package, the typed-input half has to be ported to meso by hand, and the next contract change needs edits in three places."
    }
  ]
}

This review does not gate merge.

@datapoints-fleet-automation datapoints-fleet-automation Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR review — G1

Used: Built the branch with go build ./... and ran go vet and go test ./api/..., all passing. In web/ I ran npm ci, Vitest (73 passing) and vue-tsc. A scratch test drove BrowserLogin and Protect over SQLite-backed session tables with an http PUBLIC_URL. I read gobrowserlogin v0.1.2 and goapiauth/tokenorsession v0.2.0. The compose stack did not run because this pod has no Docker daemon.
Standards: Cited kubernetes.md § "A liveness probe catches only what a restart fixes, and readiness checks what the service needs". Checked against frontend.md § "A browser front end fails a write on a 401 in place, and sends only a read with nothing unsaved to log in", which the client follows. Searches for config refused at load, logout, and documented dev flows found no rule that applies.
Findings: 3 correctness · 0 design · 0 prose

Correctness

G1.1 api/main.go:98 · api/authentication/browser_login.go:60 · api/settings/settings.go:100 · deploy/app/api.yaml:99

Reach: A deployment that sets WEB_CLIENT_SECRET with a configuration gobrowserlogin refuses. One example is a PUBLIC_URL on http at a non-loopback host, which LoadServer accepts and which was valid before this PR. Another is a provider whose discovery omits client_secret_basic. Either case takes every pod out of the projects-api Service on its next restart. The cluster's real PUBLIC_URL and provider would show whether this deployment hits it today.

A browser login that never starts takes bearer clients offline too. The body says a provider still unreachable "keeps /ready false without stopping bearer requests". In the shipped deploy shape that is not true. deploy/app/api.yaml probes readiness on /ready. api/main.go:98 adds browser login to the /ready checks. A pod that fails /ready is dropped from the Service, so the TUI's sync and any script holding a token lose the API as well. The bearer API does not need the browser login. kubernetes.md § "A liveness probe catches only what a restart fixes, and readiness checks what the service needs" says readiness checks what the service needs.

The new cases are config refusals, which the provider check did not already cover. Run retries a refused configuration forever by design (browser_login.go:60).

I ran a scratch test that calls NewBrowserLogin(db, issuer, "projects-web", "secret", "http://projects.lan", log) and runs Run for 300ms of retries. Ready() still returned gobrowserlogin: RedirectURL "http://projects.lan/api/auth/callback" is neither https nor http on a loopback host. A request with no credential sent through Protect got 503 {"code":"session_store_unavailable",...,"message":"the session store is unavailable"}. It also logged at ERROR. A CLI with no token therefore gets a misleading 503 instead of the 401 challenge, for as long as the login is not built.

Fix, in two parts:

  1. Refuse the configuration that can never succeed in settings.LoadServer, as it already refuses a missing PUBLIC_URL. With a browser login on, PUBLIC_URL must be https or http on a loopback host.
  2. Drop browser login from the readiness checks, or report it in /ready's reason without failing the probe. Then /ready fails only on what the bearer API needs. In BrowserLogin.Authenticate, return gobrowserlogin.ErrNoSession while the login is unbuilt and the request carries no session cookie, so the credential-less request still gets a 401.

G1.2 api/authentication/browser_login.go:120 · web/src/App.vue:1

Reach: Every person who logs in through the page. They have no way to end the session. It ends only after 2 hours idle or 12 hours absolute (gobrowserlogin's defaults), or when they clear their cookies by hand.

The login ships without a logout. BrowserLogin.Mount serves only GET /api/auth/login and GET /api/auth/callback. Nothing in api/ or web/src mounts or calls Sessions.Logout. I ran rg -n -i 'logout|sign out' over both directories, and it returns nothing.

gobrowserlogin's doc comment says "Mount Login, Callback and Logout as routes". Its README's setup mounts all three, with POST /api/auth/logout mapping ErrNoCSRFHeader to 403 and success to 204. The cookie is __Host-Http-session, which is HttpOnly, so the page cannot clear it either.

A person on a shared or borrowed machine closes the tab still logged in. For up to two hours idle, the next user of that browser acts as them, with every write a person's token has. They can create and delete projects and items. The session row also stays in browser_sessions.

Fix: mount POST /api/auth/logout in Mount, as the README shows. Add it to the public paths only if needed. Logout checks the CSRF header itself and treats a request with no session as already logged out. Then give the page a control that posts to it with the client's X-CSRF header and reloads.

G1.3 docker-compose.dev.yml:55 · Taskfile.yml:81 · web/vite.config.ts:8

Reach: task web:dev, which README.md and CLAUDE.md document as "the page from source at http://localhost:3002, reloading on each edit". Each login started from the Vite server reaches it.

task web:dev logs the developer into the built image instead of the Vite server. The page at :3002 gets a 401 and navigates to /api/auth/login?return_to=/. Vite proxies that to the stack. The API's RedirectURL is fixed to PUBLIC_URL + /api/auth/callback. The compose file pins that to http://localhost:8096. So the provider sends the browser to :8096/api/auth/callback. Callback then redirects to the attempt's return_to path on that host.

The developer ends on Caddy's built copy at :8096, not on Vite's hot-reloading :3002. Edits then appear not to take effect. The cookie is host-only and cookies ignore the port, so navigating back to :3002 by hand works until the session ends. The next login repeats the detour.

I read this path from the code. I could not run it, because this pod has no Docker daemon.

Fix: make the compose value overridable, as in PUBLIC_URL: ${PROJECTS_PUBLIC_URL:-http://localhost:8096}. Then have web:dev start the stack with PROJECTS_PUBLIC_URL=http://localhost:3002. Vite already proxies /api to the stack, so the callback reaches the API through it. The stand-in provider accepts any redirect URI.

Findings as JSON
{
  "used": "Built the branch with `go build ./...` and ran `go vet` and `go test ./api/...`, all passing. In `web/` I ran `npm ci`, Vitest (73 passing) and `vue-tsc`. A scratch test drove `BrowserLogin` and `Protect` over SQLite-backed session tables with an http `PUBLIC_URL`. I read gobrowserlogin v0.1.2 and goapiauth/tokenorsession v0.2.0. The compose stack did not run because this pod has no Docker daemon.",
  "standards": "Cited `kubernetes.md` § \"A liveness probe catches only what a restart fixes, and readiness checks what the service needs\". Checked against `frontend.md` § \"A browser front end fails a write on a 401 in place, and sends only a read with nothing unsaved to log in\", which the client follows. Searches for config refused at load, logout, and documented dev flows found no rule that applies.",
  "findings": [
    {
      "number": "G1.1",
      "section": "correctness",
      "sites": [
        "api/main.go:98",
        "api/authentication/browser_login.go:60",
        "api/settings/settings.go:100",
        "deploy/app/api.yaml:99"
      ],
      "reach": "A deployment that sets `WEB_CLIENT_SECRET` with a configuration gobrowserlogin refuses. One example is a `PUBLIC_URL` on http at a non-loopback host, which `LoadServer` accepts and which was valid before this PR. Another is a provider whose discovery omits `client_secret_basic`. Either case takes every pod out of the `projects-api` Service on its next restart. The cluster's real `PUBLIC_URL` and provider would show whether this deployment hits it today.",
      "body": "**A browser login that never starts takes bearer clients offline too.** The body says a provider still unreachable \"keeps `/ready` false without stopping bearer requests\". In the shipped deploy shape that is not true. `deploy/app/api.yaml` probes readiness on `/ready`. `api/main.go:98` adds `browser login` to the `/ready` checks. A pod that fails `/ready` is dropped from the Service, so the TUI's sync and any script holding a token lose the API as well. The bearer API does not need the browser login. `kubernetes.md` § \"A liveness probe catches only what a restart fixes, and readiness checks what the service needs\" says readiness checks what the service needs.\n\nThe new cases are config refusals, which the provider check did not already cover. `Run` retries a refused configuration forever by design (`browser_login.go:60`).\n\nI ran a scratch test that calls `NewBrowserLogin(db, issuer, \"projects-web\", \"secret\", \"http://projects.lan\", log)` and runs `Run` for 300ms of retries. `Ready()` still returned `gobrowserlogin: RedirectURL \"http://projects.lan/api/auth/callback\" is neither https nor http on a loopback host`. A request with no credential sent through `Protect` got `503 {\"code\":\"session_store_unavailable\",...,\"message\":\"the session store is unavailable\"}`. It also logged at ERROR. A CLI with no token therefore gets a misleading 503 instead of the 401 challenge, for as long as the login is not built.\n\nFix, in two parts:\n\n1. Refuse the configuration that can never succeed in `settings.LoadServer`, as it already refuses a missing `PUBLIC_URL`. With a browser login on, `PUBLIC_URL` must be https or http on a loopback host.\n2. Drop `browser login` from the readiness checks, or report it in `/ready`'s reason without failing the probe. Then `/ready` fails only on what the bearer API needs. In `BrowserLogin.Authenticate`, return `gobrowserlogin.ErrNoSession` while the login is unbuilt and the request carries no session cookie, so the credential-less request still gets a 401."
    },
    {
      "number": "G1.2",
      "section": "correctness",
      "sites": [
        "api/authentication/browser_login.go:120",
        "web/src/App.vue:1"
      ],
      "reach": "Every person who logs in through the page. They have no way to end the session. It ends only after 2 hours idle or 12 hours absolute (gobrowserlogin's defaults), or when they clear their cookies by hand.",
      "body": "**The login ships without a logout.** `BrowserLogin.Mount` serves only `GET /api/auth/login` and `GET /api/auth/callback`. Nothing in `api/` or `web/src` mounts or calls `Sessions.Logout`. I ran `rg -n -i 'logout|sign out'` over both directories, and it returns nothing.\n\ngobrowserlogin's doc comment says \"Mount Login, Callback and Logout as routes\". Its README's setup mounts all three, with `POST /api/auth/logout` mapping `ErrNoCSRFHeader` to 403 and success to 204. The cookie is `__Host-Http-session`, which is HttpOnly, so the page cannot clear it either.\n\nA person on a shared or borrowed machine closes the tab still logged in. For up to two hours idle, the next user of that browser acts as them, with every write a person's token has. They can create and delete projects and items. The session row also stays in `browser_sessions`.\n\nFix: mount `POST /api/auth/logout` in `Mount`, as the README shows. Add it to the public paths only if needed. `Logout` checks the CSRF header itself and treats a request with no session as already logged out. Then give the page a control that posts to it with the client's `X-CSRF` header and reloads."
    },
    {
      "number": "G1.3",
      "section": "correctness",
      "sites": [
        "docker-compose.dev.yml:55",
        "Taskfile.yml:81",
        "web/vite.config.ts:8"
      ],
      "reach": "`task web:dev`, which README.md and CLAUDE.md document as \"the page from source at http://localhost:3002, reloading on each edit\". Each login started from the Vite server reaches it.",
      "body": "**`task web:dev` logs the developer into the built image instead of the Vite server.** The page at `:3002` gets a 401 and navigates to `/api/auth/login?return_to=/`. Vite proxies that to the stack. The API's `RedirectURL` is fixed to `PUBLIC_URL` + `/api/auth/callback`. The compose file pins that to `http://localhost:8096`. So the provider sends the browser to `:8096/api/auth/callback`. `Callback` then redirects to the attempt's `return_to` path on that host.\n\nThe developer ends on Caddy's built copy at `:8096`, not on Vite's hot-reloading `:3002`. Edits then appear not to take effect. The cookie is host-only and cookies ignore the port, so navigating back to `:3002` by hand works until the session ends. The next login repeats the detour.\n\nI read this path from the code. I could not run it, because this pod has no Docker daemon.\n\nFix: make the compose value overridable, as in `PUBLIC_URL: ${PROJECTS_PUBLIC_URL:-http://localhost:8096}`. Then have `web:dev` start the stack with `PROJECTS_PUBLIC_URL=http://localhost:3002`. Vite already proxies `/api` to the stack, so the callback reaches the API through it. The stand-in provider accepts any redirect URI."
    }
  ]
}

This review does not gate merge.

@datapointchris datapointchris left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR review — synthesis

Used: the 17 findings of round 1, from 8 review(s), and nothing else
Standards: none, because this step reads only the findings
Findings: 0 correctness · 3 design · 0 prose

Design and architecture

synthesis.1 explains G1.1 · failure-paths.1 · contracts.1

An unready optional browser login leaks into the bearer API. The login's not-ready state is wired into /ready (api/main.go) and into Authenticate (browser_login.go). A login that cannot start therefore drops the pod from the Service (G1.1, failure-paths.1). It also turns a credential-less request into a 503 instead of a 401 (G1.1, contracts.1).

synthesis.2 explains fleet-patterns.1 · fleet-patterns.2 · fleet-patterns.3 · fleet-patterns.4

Code is copied from sibling repos (meso, slate, gobrowserlogin, goapiauth) with no shared owner. The copies in projects drift from the originals. The retry loop copies goapiauth's Connecting (fleet-patterns.1). The pin-manifests script and publish job copy slate's (fleet-patterns.2). The Caddyfile and Dockerfile copy meso's (fleet-patterns.3). The browser-login client half copies meso's and gobrowserlogin's contract (fleet-patterns.4).

synthesis.3 explains stale-prose.3 · stale-prose.4

Comments restate a value that another file or library owns. They cite the stack's ports (CLAUDE.md:388), Caddy's memory figure (web.yaml:98) and gobrowserlogin's touch interval (migration 00002). Nothing keeps these level with their source.

One defect, raised more than once

G1.1 · failure-paths.1: The browser login check in /ready (api/main.go) fails the probe when the optional login cannot be built, so the pod leaves the Service and bearer clients lose the API.

Shapes as JSON
{
  "shapes": [
    {
      "number": "synthesis.1",
      "section": "design",
      "explains": [
        "G1.1",
        "failure-paths.1",
        "contracts.1"
      ],
      "body": "An unready optional browser login leaks into the bearer API. The login's not-ready state is wired into `/ready` (api/main.go) and into `Authenticate` (browser_login.go). A login that cannot start therefore drops the pod from the Service (G1.1, failure-paths.1). It also turns a credential-less request into a 503 instead of a 401 (G1.1, contracts.1)."
    },
    {
      "number": "synthesis.2",
      "section": "design",
      "explains": [
        "fleet-patterns.1",
        "fleet-patterns.2",
        "fleet-patterns.3",
        "fleet-patterns.4"
      ],
      "body": "Code is copied from sibling repos (meso, slate, gobrowserlogin, goapiauth) with no shared owner. The copies in projects drift from the originals. The retry loop copies goapiauth's `Connecting` (fleet-patterns.1). The pin-manifests script and publish job copy slate's (fleet-patterns.2). The Caddyfile and Dockerfile copy meso's (fleet-patterns.3). The browser-login client half copies meso's and gobrowserlogin's contract (fleet-patterns.4)."
    },
    {
      "number": "synthesis.3",
      "section": "design",
      "explains": [
        "stale-prose.3",
        "stale-prose.4"
      ],
      "body": "Comments restate a value that another file or library owns. They cite the stack's ports (CLAUDE.md:388), Caddy's memory figure (web.yaml:98) and gobrowserlogin's touch interval (migration 00002). Nothing keeps these level with their source."
    }
  ],
  "duplicates": [
    {
      "defect": "The `browser login` check in `/ready` (api/main.go) fails the probe when the optional login cannot be built, so the pod leaves the Service and bearer clients lose the API.",
      "findings": [
        "G1.1",
        "failure-paths.1"
      ]
    }
  ]
}

This review does not gate merge.

@datapointchris

Copy link
Copy Markdown
Owner Author

PR review round 1

Ran: G1, useless-test, code-architecture, fleet-patterns, system-design, stale-prose, failure-paths, contracts, synthesis
Did not run: none
Rules: 1 proposed, filed into the fleet inbox as proposals.

reviewer model effort duration uncached input cache write cache read output findings review
G1 opus high 7m 45s 64 86,301 1,968,937 17,416 3 review
useless-test opus high 2m 26s 36 48,757 664,782 10,786 1 review
code-architecture opus medium 2m 00s 32 69,914 854,722 9,753 3 review
fleet-patterns opus medium 2m 55s 42 50,601 789,133 9,952 4 review
system-design sonnet medium 0m 33s 12 22,535 118,628 2,556 0 review
stale-prose opus medium 2m 12s 40 70,504 894,812 11,309 4 review
failure-paths sonnet medium 0m 39s 10 34,646 100,554 3,258 1 review
contracts opus medium 2m 00s 38 45,741 666,601 10,127 1 review
synthesis sonnet medium 0m 11s 2 14,359 1,433 1,365 3 review
total 276 443,358 6,059,602 76,522 20
Round record
{
  "grammar": 6,
  "round": 1,
  "repo": "datapointchris/projects",
  "pr": 18,
  "head": "52c3969c5261f2ae8fda9907b44973862230f481",
  "experiment": "none",
  "dispatch": "20261107T221129Z-pr-review",
  "roster": "67aaba2ee4a38bd9a173eff3b1b7fe9b066b2cd4",
  "harness": "9fb899ec802ea87518b69760bc1354bcff4e0c7c",
  "machine": "archlinux",
  "started": "2026-11-07T22:12:28Z",
  "finished": "2026-11-07T22:20:46Z",
  "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": 465,
      "tokens": {
        "input": 64,
        "cache_write": 86301,
        "cache_read": 1968937,
        "output": 17416
      },
      "tokens_from": "result",
      "tokens_by_model": {
        "claude-opus-5-5": {
          "input": 64,
          "cache_write": 86301,
          "cache_read": 1968937,
          "output": 17416
        }
      },
      "session": "01a1b811-906d-759f-a121-5743ce19de48",
      "worklog": "2403.1",
      "worklog_refused": "",
      "claude_code": "2.1.319",
      "review_url": "https://github.com/datapointchris/projects/pull/18#pullrequestreview-5815277249",
      "review_id": 5815277249,
      "findings": {
        "correctness": 3,
        "design": 0,
        "prose": 0
      },
      "numbers": [
        "G1.1",
        "G1.2",
        "G1.3"
      ],
      "rules": {
        "proposed": 1,
        "keys": [
          "rule/frontend/a-browser-login-ships-with-its-logout"
        ],
        "filed": true,
        "why": "",
        "path": "/home/fleet/.cache/run-pr-reviewers/datapointchris-projects-pr18-round1-20261107T221129Z/records/G1.rules.json"
      },
      "head": "52c3969c5261f2ae8fda9907b44973862230f481",
      "kept": false,
      "records": "/home/fleet/.cache/run-pr-reviewers/datapointchris-projects-pr18-round1-20261107T221129Z/records",
      "started": "2026-11-07T22:12:28Z",
      "finished": "2026-11-07T22:20:17Z",
      "account": "fleetizen",
      "api_key_source": "none",
      "machine": "queue"
    },
    {
      "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": 146,
      "tokens": {
        "input": 36,
        "cache_write": 48757,
        "cache_read": 664782,
        "output": 10786
      },
      "tokens_from": "result",
      "tokens_by_model": {
        "claude-opus-5-5": {
          "input": 36,
          "cache_write": 48757,
          "cache_read": 664782,
          "output": 10786
        }
      },
      "session": "01a1b812-8da9-7de7-9bcc-3ecf527e5077",
      "worklog": "2403.1",
      "worklog_refused": "",
      "claude_code": "2.1.319",
      "review_url": "https://github.com/datapointchris/projects/pull/18#pullrequestreview-5815249120",
      "review_id": 5815249120,
      "findings": {
        "correctness": 0,
        "design": 1,
        "prose": 0
      },
      "numbers": [
        "useless-test.1"
      ],
      "rules": {},
      "head": "52c3969c5261f2ae8fda9907b44973862230f481",
      "kept": false,
      "records": "/home/chris/.cache/run-pr-reviewers/datapointchris-projects-pr18-round1-20261107T221129Z/records",
      "started": "2026-11-07T22:13:12Z",
      "finished": "2026-11-07T22:15:40Z",
      "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": 120,
      "tokens": {
        "input": 32,
        "cache_write": 69914,
        "cache_read": 854722,
        "output": 9753
      },
      "tokens_from": "result",
      "tokens_by_model": {
        "claude-opus-5-5": {
          "input": 32,
          "cache_write": 69914,
          "cache_read": 854722,
          "output": 9753
        }
      },
      "session": "01a1b812-3561-7684-9948-8f4afc49575b",
      "worklog": "2403.1",
      "worklog_refused": "",
      "claude_code": "2.1.319",
      "review_url": "https://github.com/datapointchris/projects/pull/18#pullrequestreview-5815244438",
      "review_id": 5815244438,
      "findings": {
        "correctness": 0,
        "design": 3,
        "prose": 0
      },
      "numbers": [
        "code-architecture.1",
        "code-architecture.2",
        "code-architecture.3"
      ],
      "rules": {},
      "head": "52c3969c5261f2ae8fda9907b44973862230f481",
      "kept": false,
      "records": "/home/chris/.cache/run-pr-reviewers/datapointchris-projects-pr18-round1-20261107T221129Z/records",
      "started": "2026-11-07T22:12:49Z",
      "finished": "2026-11-07T22:14:51Z",
      "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": 175,
      "tokens": {
        "input": 42,
        "cache_write": 50601,
        "cache_read": 789133,
        "output": 9952
      },
      "tokens_from": "result",
      "tokens_by_model": {
        "claude-opus-5-5": {
          "input": 42,
          "cache_write": 50601,
          "cache_read": 789133,
          "output": 9952
        }
      },
      "session": "01a1b811-ffc9-7fe6-879c-f1ef9c41929a",
      "worklog": "2403.1",
      "worklog_refused": "",
      "claude_code": "2.1.319",
      "review_url": "https://github.com/datapointchris/projects/pull/18#pullrequestreview-5815250511",
      "review_id": 5815250511,
      "findings": {
        "correctness": 0,
        "design": 4,
        "prose": 0
      },
      "numbers": [
        "fleet-patterns.1",
        "fleet-patterns.2",
        "fleet-patterns.3",
        "fleet-patterns.4"
      ],
      "rules": {},
      "head": "52c3969c5261f2ae8fda9907b44973862230f481",
      "kept": false,
      "records": "/home/fleet/.cache/run-pr-reviewers/datapointchris-projects-pr18-round1-20261107T221129Z/records",
      "started": "2026-11-07T22:12:53Z",
      "finished": "2026-11-07T22:15:54Z",
      "account": "fleetizen",
      "api_key_source": "none",
      "machine": "queue"
    },
    {
      "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": 33,
      "tokens": {
        "input": 12,
        "cache_write": 22535,
        "cache_read": 118628,
        "output": 2556
      },
      "tokens_from": "result",
      "tokens_by_model": {
        "claude-sonnet-5-5": {
          "input": 12,
          "cache_write": 22535,
          "cache_read": 118628,
          "output": 2556
        }
      },
      "session": "01a1b812-b5b3-79f7-9059-3f4ed238201a",
      "worklog": "2403.1",
      "worklog_refused": "",
      "claude_code": "2.1.319",
      "review_url": "https://github.com/datapointchris/projects/pull/18#pullrequestreview-5815241636",
      "review_id": 5815241636,
      "findings": {
        "correctness": 0,
        "design": 0,
        "prose": 0
      },
      "numbers": [],
      "rules": {},
      "head": "52c3969c5261f2ae8fda9907b44973862230f481",
      "kept": false,
      "records": "/home/chris/.cache/run-pr-reviewers/datapointchris-projects-pr18-round1-20261107T221129Z/records",
      "started": "2026-11-07T22:13:18Z",
      "finished": "2026-11-07T22:13:55Z",
      "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": 132,
      "tokens": {
        "input": 40,
        "cache_write": 70504,
        "cache_read": 894812,
        "output": 11309
      },
      "tokens_from": "result",
      "tokens_by_model": {
        "claude-opus-5-5": {
          "input": 40,
          "cache_write": 70504,
          "cache_read": 894812,
          "output": 11309
        }
      },
      "session": "01a1b812-35bf-7312-9c0d-0f95febc1a01",
      "worklog": "2403.1",
      "worklog_refused": "",
      "claude_code": "2.1.319",
      "review_url": "https://github.com/datapointchris/projects/pull/18#pullrequestreview-5815245512",
      "review_id": 5815245512,
      "findings": {
        "correctness": 0,
        "design": 0,
        "prose": 4
      },
      "numbers": [
        "stale-prose.1",
        "stale-prose.2",
        "stale-prose.3",
        "stale-prose.4"
      ],
      "rules": {},
      "head": "52c3969c5261f2ae8fda9907b44973862230f481",
      "kept": false,
      "records": "/home/chris/.cache/run-pr-reviewers/datapointchris-projects-pr18-round1-20261107T221129Z/records",
      "started": "2026-11-07T22:12:50Z",
      "finished": "2026-11-07T22:15:04Z",
      "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": 39,
      "tokens": {
        "input": 10,
        "cache_write": 34646,
        "cache_read": 100554,
        "output": 3258
      },
      "tokens_from": "result",
      "tokens_by_model": {
        "claude-sonnet-5-5": {
          "input": 10,
          "cache_write": 34646,
          "cache_read": 100554,
          "output": 3258
        }
      },
      "session": "01a1b812-aab5-7f5f-b357-0db7ba5f1d90",
      "worklog": "2403.1",
      "worklog_refused": "",
      "claude_code": "2.1.319",
      "review_url": "https://github.com/datapointchris/projects/pull/18#pullrequestreview-5815241979",
      "review_id": 5815241979,
      "findings": {
        "correctness": 1,
        "design": 0,
        "prose": 0
      },
      "numbers": [
        "failure-paths.1"
      ],
      "rules": {},
      "head": "52c3969c5261f2ae8fda9907b44973862230f481",
      "kept": false,
      "records": "/home/chris/.cache/run-pr-reviewers/datapointchris-projects-pr18-round1-20261107T221129Z/records",
      "started": "2026-11-07T22:13:18Z",
      "finished": "2026-11-07T22:14:02Z",
      "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": 120,
      "tokens": {
        "input": 38,
        "cache_write": 45741,
        "cache_read": 666601,
        "output": 10127
      },
      "tokens_from": "result",
      "tokens_by_model": {
        "claude-opus-5-5": {
          "input": 38,
          "cache_write": 45741,
          "cache_read": 666601,
          "output": 10127
        }
      },
      "session": "01a1b812-3638-78ca-9975-6e184c320910",
      "worklog": "2403.1",
      "worklog_refused": "",
      "claude_code": "2.1.319",
      "review_url": "https://github.com/datapointchris/projects/pull/18#pullrequestreview-5815244448",
      "review_id": 5815244448,
      "findings": {
        "correctness": 1,
        "design": 0,
        "prose": 0
      },
      "numbers": [
        "contracts.1"
      ],
      "rules": {},
      "head": "52c3969c5261f2ae8fda9907b44973862230f481",
      "kept": false,
      "records": "/home/chris/.cache/run-pr-reviewers/datapointchris-projects-pr18-round1-20261107T221129Z/records",
      "started": "2026-11-07T22:12:50Z",
      "finished": "2026-11-07T22:14:52Z",
      "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": 1,
      "seconds": 11,
      "tokens": {
        "input": 2,
        "cache_write": 14359,
        "cache_read": 1433,
        "output": 1365
      },
      "tokens_from": "result",
      "tokens_by_model": {
        "claude-sonnet-5-5": {
          "input": 2,
          "cache_write": 14359,
          "cache_read": 1433,
          "output": 1365
        }
      },
      "session": "01a1b819-4294-781e-a442-6647d7437adc",
      "worklog": "2403.1",
      "worklog_refused": "",
      "claude_code": "2.1.319",
      "review_url": "https://github.com/datapointchris/projects/pull/18#pullrequestreview-5815280087",
      "review_id": 5815280087,
      "findings": {
        "correctness": 0,
        "design": 3,
        "prose": 0
      },
      "numbers": [
        "synthesis.1",
        "synthesis.2",
        "synthesis.3"
      ],
      "rules": {},
      "head": "52c3969c5261f2ae8fda9907b44973862230f481",
      "kept": false,
      "records": "/home/chris/.cache/run-pr-reviewers/datapointchris-projects-pr18-round1-20261107T221129Z/records",
      "started": "2026-11-07T22:20:32Z",
      "finished": "2026-11-07T22:20:46Z",
      "account": "fleetizen",
      "api_key_source": "none",
      "machine": "archlinux"
    }
  ],
  "rules": {
    "proposed": 1,
    "keys": [
      "rule/frontend/a-browser-login-ships-with-its-logout"
    ],
    "unfiled": []
  },
  "kept": []
}

The web app reaches the API from the same origin with a session cookie
rather than a token. The API runs the authorization code flow with PKCE
as a confidential client, through gobrowserlogin, and stores each
session and login attempt in two new tables from migration 00002.

The login is on only where WEB_CLIENT_SECRET is set, and settings then
refuse to load without PUBLIC_URL, because the callback is
PUBLIC_URL/api/auth/callback. WEB_CLIENT_ID defaults to projects-web.
Like the token verifier, the login reads the provider in the background,
so the API starts while the provider is unreachable and /ready names
the cause. /api/auth/login and /api/auth/callback pass with no
credential.

A session acts as its person and reaches every route. Service clients
keep their scopes. A session's request logs WEB_CLIENT_ID as its client.

go.mod ignores ./web, the web app's directory, because its npm packages
can ship Go files that ./... would otherwise build and lint.

Agent-Name: claude-code-unattended
Agent-Session: 65c3b3f7-676b-4350-8ae4-8beee7c7bfad
Agent-Run: 01a1b7cb-6a92-7018-a948-2ce91ef4c9e2
Worklog: 2403.1
… the API

web/ is the Projects page in Vue 3, Pinia and Vite, at /, with no
router. Its components and styles are this repo's own copy, so its look
changes here alone. Every request carries X-CSRF: 1. A 401 on a write,
or on the search, fails in place with the reason, so nothing typed is
lost. Any other read sends the browser to /api/auth/login, at most twice
a minute.

The projects-web image is Caddy on port 8080, unprivileged on a
read-only root. It serves the build, answers a missing /assets/ file
with 404, and passes /api and /.well-known to projects-api:8000, so the
browser sees one origin and a %2F reaches the API escaped.

docker-compose.dev.yml runs the stack from the checkout with a stand-in
identity provider, behind task dev-stack:start, web:dev and
web:test:e2e. The Playwright smoke logs in, then creates, edits and
deletes a project named with a slash and an item in it. The web-stack
CI job builds the image and runs that smoke. Regenerating for the vue
component adds the eslint, prettier and typecheck hooks, the vue-web
job and the prettier config, and .nvmrc names the Node major.

Each release builds, signs and pins ghcr.io/datapointchris/projects-web
beside the API's image, under the same version. deploy/app/web.yaml
adds the projects-web ServiceAccount, Service and Deployment, and
release/pin-manifests takes --web-image and refuses a built path running
an image no flag pinned.

Agent-Name: claude-code-unattended
Agent-Session: 65c3b3f7-676b-4350-8ae4-8beee7c7bfad
Agent-Run: 01a1b7cb-6a92-7018-a948-2ce91ef4c9e2
Worklog: 2403.1
…log a browser out

The browser login is gobrowserlogin's Connecting. That library checks the
configuration at start, so a PUBLIC_URL no browser would keep a cookie for
stops the API rather than retrying forever. While it opens the session
tables and reads the provider, a request carrying no session gets the 401
challenge rather than a 503, and /ready leaves the login out, so bearer
clients keep the pod in its Service.

POST /api/auth/logout ends the session, and the page's Log out button posts
it, then shows a logged-out line rather than reading anything, which would
start a login. MountBrowserLogin returns the paths Protect passes with no
credential. task web:dev starts the stack with Vite's address as PUBLIC_URL,
so a login returns to Vite.

The project schemas list each kind and status as an enum, with kind's
default, and a test holds them to model. The page builds its kind and status
choices from constants another test holds to the schema.

Comments copied with the styles that described another app go, with the
variables and rules nothing here uses.

Agent-Name: claude-code-unattended
Agent-Session: 65c3b3f7-676b-4350-8ae4-8beee7c7bfad
Agent-Run: 01a1b811-5fd8-7927-9d2b-aacac4d4e468
Worklog: 2403.1
…quires

Agent-Name: claude-code
Agent-Session: 660f3875-6438-4f68-a538-9a7dea65966f
Worklog: 2388.1
@datapointchris
datapointchris merged commit 0a40ca5 into main Nov 8, 2026
6 checks passed
@datapointchris
datapointchris deleted the projects-web-port branch November 8, 2026 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant