[IAmJSD]

Root cause

Nothing checked internal/api/openapi.yaml against the routes in router.go, so routes were added without spec entries and documented operations drifted from their handlers. On main, 49 registered routes had no operation. Because TestOpenAPI403OnPermissionGatedRoutes only checks routes the spec lists, those routes were also exempt from the 403 guard.

Merged PRs #772 and #778 mention "#262", but they mean cloud-commitments-go#262 and neither touches the spec. Some items in the issue were already fixed on main and are not redone here: /api/recommendations/freshness and POST /api/purchases/revoke/{id} are documented, and approve/cancel are POST-only with the body token documented (#911).

Fix

1. New guard, TestOpenAPIDocumentsEveryRoute (internal/api/openapi_route_coverage_test.go). For every spec operation it fills each {param} with a sample value and runs it through the live router's first-match logic (Router.matches over registerRoutes(), in order). It fails in two directions:

   • a route that no documented operation reaches;
   • a documented operation that reaches no route.

   Multi-segment parameters and prefix/suffix ordering therefore behave exactly as in production. There are two exemption lists, and each entry must give a reason. specOnlyOperations holds GET /ready, which internal/server/http.go serves before the router. undocumentedRoutes is empty: docs(api): document existing insurance contracts #1080 documented the two insurance routes it used to hold.

   An exemption that is no longer needed also fails the test, so the lists can't go stale. TestRouteCoverageGapsDetectsDrift and TestPartitionExemptReportsStaleEntries check that the checker itself catches drift in both directions.

   Limits, also stated in the test's header comment:

   • Only registerRoutes() is walked. Paths the server mux handles before the API router (/oidc/*, /api/scheduled/* in internal/server/http.go) are outside the guard. fix(api): openapi.yaml omits ~30 routed paths and mis-describes documented operations #262 scopes the work to router.go.
   • Path parameter names and segment counts under a prefix route are not checked. Renaming {provider}/{service} back to {service} still reaches the same route.

2. Backfill. All 49 operations, read from the handlers: parameters actually read, request/response shapes, and the status codes the handler can return. New tags: Accounts, Registrations, Federation, Inventory, Ladder, Notifications. RIExchange was already used but never declared, so it is now declared. Docs get HEAD operations and the /docs/openapi.yaml alias the Swagger page fetches.

3. Corrected operations from the issue:

   • The shared StartDate/EndDate parameters are now start/end. Every handler reads those names, and the frontend sends them; start_date/end_date were never read.
   • /api/history: add provider, account_ids and limit; drop interval; start/end are YYYY-MM-DD.
   • /api/history/analytics: add interval and provider, plus the 400/404/503 responses.
   • /api/history/breakdown: add account to the dimension enum.
   • /api/dashboard/summary: drop the unread date range; add account_ids.
   • /api/recommendations: add account_ids, min_savings_usd and min_savings_pct; drop the unread account_id.
   • /api/recommendations/refresh: returns 200 RefreshResponse and can return 409. The unused CollectResult schema is removed.
   • PUT /api/config and the service config PUT return {"status":"updated"}.
   • /api/config/service/{service} is now /api/config/service/{provider}/{service}. GET returns {} rather than 404.
   • setup-admin gets 409/503, login gets 503, and reset-password gets 409/429/503.
   • The spec is served as application/yaml.
   • /api/history response: PurchaseHistoryRecord lists all 39 fields the struct emits (it had 16 and lacked status), and monthly_cost is nullable. HistorySummary lists all 10 fields (it had 4).
   • GET /api/auth/me/permissions declares bearerAuth or apiKeyAuth, since the handler accepts user and admin API keys.

4. 403 guard fixes (openapi_403_test.go):

   • specPathsForRoute now maps each operation to the route the router actually dispatches it to, not to every route whose prefix matches.
   • requirePermission mentions on whole-line comments are ignored. Only whole lines are skipped, so a // inside a string literal cannot hide a real call.
   • A length check makes sure the source parse of router.go matches the live route table.

   Without these changes, documenting GET /api/purchases/revoke/{id} (the public HTML form) wrongly demanded a 403 from the GET /api/purchases/{id} handler. GET /api/auth/me/permissions was also flagged, because of a comment.

Verification

Unit/fixture evidence only, re-run on the tree merged with main at 2fe9db4. No server was run and no cloud call was made. Tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, because this machine's default AWS profile is invalid.

• Fail-before. The new guard against main's spec fails with 49 undocumented routes: recommendation detail, plan accounts, approve details, revoke GET/retry/revoke/calculate/marketplace, trends, me/permissions, reset-password/status, 4x MFA, 11x accounts, 2x inventory, 6x ladder, 2x unsubscribe, register and 5x registrations, federation/iac, /version, and HEAD on /api/docs and /docs.
• Pass-after. go test ./internal/api/ -run 'TestOpenAPI|TestRouteCoverage|TestPartitionExempt' -count=1 passes.
• Old 403 matcher against the new spec. Main's openapi_403_test.go fails with exactly the two false positives above (GET /api/purchases/revoke/{id}, GET /api/auth/me/permissions). The updated test passes.
• Package tests and guards. go test ./internal/api/ -count=1 and go test ./internal/mocks/ -count=1 pass. go build ./... and go vet ./internal/api/ (with and without -tags integration) are clean.
• Lint. golangci-lint v2.10.1 run ./internal/api/ reports 0 issues, with and without --build-tags integration.
• Complexity. gocyclo -over 10 -ignore "_test\.go" reports nothing. Only test files changed; the two pre-existing hits in openapi_403_test.go are unchanged functions.
• Secrets. git-secrets --scan on the changed files, run in an isolated repo with the CI patterns and .gitallowed, is clean. A planted fake key in the same scan is flagged. scripts/test-git-secrets-allowlist.sh gives 137 PASS, 0 FAIL.
• OpenAPI validation. redocly lint (@redocly/cli 1.25.0, recommended rules): valid with 0 errors, 42 warnings versus 25 on main. The added warnings are tag-description warnings and no-ambiguous-paths (for example /api/purchases/approve/{id} vs /api/purchases/{id}/revoke), which is inherent to the router's prefix/suffix URL scheme, and operation-4xx-response on the docs, version and HEAD operations.
• Reading the diff. GitHub's default diff makes the YAML look heavily rewritten. git diff --diff-algorithm=histogram origin/main shows 42 deleted lines, all intended edits; the rest are additions.

Out of scope (found while reading handlers, not changed here)

• Unsupported analytics values return 500. interval/dimension reach the store unvalidated and come back as 500 instead of 400.
• PATCH skips CSRF validation. The CSRF middleware only checks POST/PUT/DELETE, so PATCH /api/ladder/tranches/{id} (and PATCH /api/plans/{id}) are never CSRF-validated. The spec still lists the header, by convention.
• Registration approve can leave an orphan. It marks the registration approved before creating the account, so a 409/500 on create leaves an approved registration with no account.
• PUT /api/accounts/{id} drops the provider. It validates but does not persist provider, yet echoes it back.
• Possible Azure revoke 500 (not verified). POST /api/purchases/{id}/revoke with an Azure ARM purchase_id probably fails the UUID execution_id lookup with a 500.
• The email revoke form's POST always returns 401. It sends no Authorization or CSRF header.
• Stale code comments. "202" in the refresh handler, "207" for RECONCILE_PENDING (the API sends 200), and the old /revoke/calculate/{id} path.
• PUT /api/ladder/configs can return 500 for a bad account ID. The spec documents cloud_account_id as format: uuid and 400 for invalid fields, but upsertLadderConfig (handler_ladder.go) never validates the UUID, so a non-UUID value reaches Postgres and comes back as 500. Code bug, from reading the code only (not run).
• Audit finding A02-012 is not addressed. /api/api-keys/{id} and /api/users/{id} returning 500 where the spec says 404 is a code bug, not spec drift.
• Expected conflicts with open PRs.
  • fix(api): add opt-in in-memory cursor pagination to GET /api/recommendations #974 also edits the GET /api/recommendations parameters.
  • fix(api): report unrestricted account access on /auth/me/permissions #1155 adds unrestricted_account_access to the /api/auth/me/permissions response. This PR adds the UserPermissionsResponse schema, so whichever of the two merges second adds that field to it.

Maintainer: please mirror the issue's triage labels onto this PR: priority/p2, severity/medium, type/chore, triaged, urgency/this-quarter, impact/few, effort/m.

Closes #262

🤖 Generated with Claude Code

[coderabbitai]

Caution

Review failed

The pull request is closed.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

VERDICT: REQUEST_CHANGES 46734e7

Reviewed at head 46734e7. The PR has 1 commit on base 8caed39, and current origin/main is b6a0649.

Blocking

1. Merge conflict. internal/api/openapi.yaml:39-44 (the tags list) conflicts with origin/main. docs(api): document existing insurance contracts #1080 (d5f9cfa) added - name: Insurance plus a description in the same spot. git merge-tree reports CONFLICT (content) in this file only. Rebase, and keep both tag sets.
2. The guard fails on the rebased tree. internal/api/openapi_route_coverage_test.go:42-43 still exempt GET /api/insurance/{comparison,status} "documented by PR docs(api): document existing insurance contracts #1080". docs(api): document existing insurance contracts #1080 has merged, so on the merged tree TestOpenAPIDocumentsEveryRoute fails at :155 with "undocumentedRoutes entries that are now documented ... GET /api/insurance/comparison, GET /api/insurance/status". The PR body predicted this. Delete both entries, and with them the now-empty-map handling if you no longer need it.

Non-blocking

The spec still disagrees with handlers in places this PR did not touch:

• openapi.yaml PurchaseHistoryRecord (head :6362) vs internal/config/types.go PurchaseHistoryRecord (merged tree :942-1040):
  • The spec lists 16 properties; the struct emits 39 JSON fields. Among the missing ones is status, so a generated client cannot tell pending, failed, completed and expired rows apart.
  • HistorySummary (head :6346) lists 4 of the 11 fields the handler returns.
  • monthly_cost is a *float64 with no omitempty, so it can be null, but the spec does not mark it nullable.
  • All of this predates the PR (the schema is unchanged from main). It is the "response shapes" part of fix(api): openapi.yaml omits ~30 routed paths and mis-describes documented operations #262 for /api/history, which this PR otherwise corrects. Fix it here or file a follow-up.
• GET /api/auth/me/permissions (head ~:2745): the description says the endpoint works for user and admin API keys (handler_auth.go:130-152). The operation has no security override, though, so it inherits bearerAuth only and does not list apiKeyAuth.
• PUT /api/ladder/configs (head :4276) documents 400 for invalid fields, and cloud_account_id as format: uuid. However, handler_ladder.go:88 never validates the UUID, so a non-UUID value reaches Postgres and comes back as 500.
  • This is a code bug that predates the PR, not spec drift.
  • It comes from reading the code; I did not run it live.
  • Add it to the PR's out-of-scope list.
• nit: the server mux routes (/oidc/*, /api/scheduled/*, internal/server/http.go:45,54) are outside the guard, which only walks registerRoutes(). fix(api): openapi.yaml omits ~30 routed paths and mis-describes documented operations #262 scopes the work to router.go, so this is acceptable, but "every routed path" overstates it.
• nit (process): the single commit changes 8565 lines (6458+/2107-). With the histogram diff it is 4485 (4418+/67-). Either way it is far over the 400-line cap. Non-blocking.

Attacked

Guard mutation probes. I ran these on the merged tree, with the conflict resolved by union and the stale exemptions removed. Each probe was reverted afterwards. Every one made TestOpenAPIDocumentsEveryRoute FAIL and named the route:

• P1: a new ExactPath route.
• P2: a new PathPrefix+PathSuffix route.
• P3: a new method-less route (reported as ANY).
• P4: flipping a route's method (reported in both directions).
• P5: inserting a shadowing prefix route first, which leaves /api/config/service/* uncovered and shows that first-match is respected.
• P6: renaming a spec path (reported in both directions).

Router registration styles. internal/api has a single registerRoutes() slice (router.go:98-99). Every Route uses only ExactPath, PathPrefix, PathSuffix and Method, and those are the only fields matches (router.go:440-462) reads. The guard calls the real matches, so all styles are covered.

403 guard. Reverting openapi_403_test.go to main's version against the new spec reproduces exactly the two false positives the PR claims (GET /api/purchases/revoke/{id}, GET /api/auth/me/permissions). The claim about the comment-only requirePermission mention is confirmed at handler_auth.go:146.

Lint. redocly lint (@redocly/cli 1.25.0, recommended rules) on the merged spec: valid, 0 errors, 42 warnings, versus 25 on main. The new warnings are tag-description, no-ambiguous-paths (from the router's prefix scheme) and operation-4xx-response on the docs, version and health HEAD/GET operations. None is a problem.

Schema vs handler, 10 routes. I spot-checked these against their handlers:

• /api/recommendations/{id}/detail
• /api/plans/{id}/accounts (GET and PUT)
• /api/purchases/retry/{id}
• /api/history
• /api/analytics/trends
• /api/auth/me/permissions
• /api/auth/reset-password/status
• /api/accounts (POST) and /api/accounts/{id} (PUT)
• /api/register
• /api/inventory/coverage and /api/ladder/configs

Parameters, bodies, auth level and status codes match. The exceptions are the items listed above, plus undocumented 500s on a few operations (nit).

The start/end rename is confirmed: handler_analytics.go:52,161,204 and handler_history.go:748 read start and end, and no handler reads start_date or end_date as a query parameter.

Local results

All runs were on the merged tree with the conflict and exemptions resolved:

• go vet ./internal/api/: clean.
• go test -count=1 ./internal/api/: 3942 passed.
• Without the exemption fix: TestOpenAPIDocumentsEveryRoute FAILs (blocking item 2).

[cristim]

Independent review 2 (claude-fleet, cold): REQUEST_CHANGES 46734e7

The changes themselves look right. The two blocking items come only from main having moved under the PR.

Findings:

• [blocking] internal/api/openapi.yaml:39 (tags list) - The PR does not merge with origin/main. git merge-tree --write-tree origin/main <head> reports a content conflict here.
  • Main's docs(api): document existing insurance contracts #1080 (d5f9cfa) added an Insurance tag at the same spot where this PR adds the Accounts/Registrations/Federation/Inventory/Ladder/Notifications tags. Keep both blocks.
  • No other hunks conflict.
• [blocking] internal/api/openapi_route_coverage_test.go:41-44 - Once the conflict is resolved, TestOpenAPIDocumentsEveryRoute fails on the merged tree: "undocumentedRoutes entries that are now documented ... GET /api/insurance/comparison, GET /api/insurance/status".
  • docs(api): document existing insurance contracts #1080 documented those routes, so the guard correctly flags the two exemptions as stale. The PR body predicts this.
  • Fix: rebase, keep both tag blocks and delete both entries. With that done on a scratch export of the merged tree, the guard passes.
• [non-blocking, gap] Spec path parameter names are not checked.
  • Probe M4 renamed /api/config/service/{provider}/{service} back to /api/config/service/{service}, and every test passed.
  • Each {param} becomes "x" and the router matches on prefix, so the guard catches missing or extra operations and wrong methods, but not wrong parameter names or segment counts under a prefix route.
  • Fine for this issue's scope, but worth a comment.
• [non-blocking] openapi_403_test.go:236 - strings.Cut(line, "//") also cuts at // inside a string literal such as a URL. No current line triggers this, so it is not a practical risk today.

Attacked:

• Issue items against the handlers:
  • start/end are read by parseDateRange (handler_analytics.go:50,162,210,306), and its defaults (7 days, at most 366) match the spec.
  • getServiceConfig splits provider/service and returns {} (handler_config.go:288-305).
  • listAccounts gates on view:accounts, and the spec lists its 403.
• Router/test parity: every router Method: is an uppercase literal, so dropping ToUpper is safe. The new require.Len check holds.
• Mutation probes on openapi.yaml, each restored and confirmed identical with cmp:
  • M1, /version deleted: the guard fails on GET /version. Caught.
  • M2, listAccounts get changed to patch: fails as both undocumented and unrouted. Caught.
  • M3, listAccounts' 403 removed: TestOpenAPI403OnPermissionGatedRoutes FAILS. Caught.
  • M4, path param renamed: passes. This is the gap above.
• Merged spec: yaml.v3 rejects duplicate keys and parses the merged spec cleanly, so no path is duplicated between main's edits and this PR's.

Verified locally, on git archive exports with no cloud credentials, under the heavy lock:

• Head:
  • gofmt -l on both test files: clean.
  • go build ./... and go vet ./internal/api/: OK.
  • go test -count=1 ./internal/api/ ./internal/mocks/: OK.
• Fail-before:
  • The new tests on base fail with the same 49 undocumented routes the PR body lists.
  • Base's 403 test against the head spec fails with exactly the two false positives the PR claims. The PR's version of the test passes.
• Merged tree, with the tags block resolved to keep both sides:
  • Build and vet: OK.
  • internal/api has one failure, the stale exemptions above. internal/mocks passes.
  • With the two exemptions removed, -run 'TestOpenAPI|TestRouteCoverage|TestPartitionExempt' passes.
• No migration.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[IAmJSD]

Addressed both reviews of 46734e7. New head: 2fe9db4. I merged origin/main rather than rebasing (merge commit d727905), so the earlier commit is unchanged.

Blocking

1. Tags conflict (internal/api/openapi.yaml:39-46). Resolved by keeping both blocks: this PR's Accounts/Registrations/Federation/Inventory/Ladder/Notifications tags, then main's Insurance tag with its description. That was the only conflict. Every block added by base→main and by base→head is present in the merged file, and main's fix: six small scheduled-auth, CI, OpenAPI and IaC fixes #1127 (exclusiveMinimum, last_collection_partial) and fix(api): refuse a planned purchase cancel whose plan the caller cannot disable #1158 (plan_disabled, 409) edits are intact. The histogram diff against origin/main still shows 42 deleted lines.
2. Stale exemptions (internal/api/openapi_route_coverage_test.go:53). Deleted both insurance entries. undocumentedRoutes is now an empty map. There was no special empty-map handling to remove, and I kept the map so the stale-entry mechanism stays available. main changed no router.go routes since the base, and the guard flags nothing else.

Non-blocking

• PurchaseHistoryRecord / HistorySummary (openapi.yaml:6638-6800). Fixed here.
  • PurchaseHistoryRecord now lists all 39 JSON fields of config.PurchaseHistoryRecord, including status with its values. monthly_cost is nullable with a note that aggregations skip nulls.
  • HistorySummary lists all of its fields. That is 10, not 11: the per-state counters total_completed, total_pending, total_in_progress, total_failed, total_expired and total_revoked were missing.
• GET /api/auth/me/permissions security (openapi.yaml:2856-2858). Added security: [bearerAuth, apiKeyAuth]. The UserPermissionsResponse schema is new in this PR, and fix(api): report unrestricted account access on /auth/me/permissions #1155 adds unrestricted_account_access to that response, so whichever PR merges second adds the field.
• Ladder UUID → 500. Left as is: it is a code bug. Added it to the PR body's out-of-scope list.
• nit: "every routed path" Reworded the title and body to "every router.go route". Added a scope note to the test header (openapi_route_coverage_test.go:29-38) and the body: server-mux paths (/oidc/*, /api/scheduled/*) are outside the guard.
• Param-name gap (probe M4) (openapi_route_coverage_test.go:34-38). Documented in the same header comment. No check added.
• strings.Cut(line, "//") (openapi_403_test.go:233-238). The check now skips only whole-line comments, so a // in a string literal can't hide a call. Mutation probe: with comment skipping disabled, TestOpenAPI403OnPermissionGatedRoutes fails on GET /api/auth/me/permissions. It passes with the fix.
• nit: diff size. Left as is. The spec backfill is one unit; the histogram diff is the readable view.

Verification (merged tree, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null):

• go build ./... OK.
• go vet ./internal/api/ is clean, with and without -tags integration.
• go test -count=1 ./internal/api/ OK.
• go test -count=1 ./internal/mocks/ OK.
• golangci-lint v2.10.1 on ./internal/api/ reports 0 issues, with and without --build-tags integration.
• gocyclo: no non-test Go files changed.
• git-secrets --scan in an isolated repo with the changed files and .gitallowed tracked is clean, and a planted fake key is flagged.
• scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.
• redocly lint (@redocly/cli 1.25.0, recommended rules): valid, 0 errors, 42 warnings (main has 25).

The PR stays a draft.

[cristim]

Independent review 2 (claude-fleet, cold), re-review: REQUEST_CHANGES 2fe9db4

Both earlier blockers are fixed against the old base. Against current origin/main (6af022f), the PR conflicts again, and the guard fails on the merged tree because a route that landed on main has no spec entry.

Findings:

• [blocking] Conflicts with current main. git merge-tree --write-tree origin/main 2fe9db4 reports two conflicting files. d727905 only merged c8bdd46, and main has gained 119 commits since then.
  • internal/api/openapi.yaml, HistorySummary: 3 hunks against feat(api): scope-aware SQL totals for the History summary cards #1200 (503eb2e).
    • Both sides declare the same 10 fields with the same types. Only the descriptions differ.
    • The PR's per-field descriptions match main's counting code (handler_history.go:1176-1190 on main).
    • Resolution: keep main's top-level description and the PR's field descriptions.
  • internal/api/openapi_403_test.go: test: make the slowest unit and integration tests fast without weakening them #1403 (f5ad8c6) refactored the scanner into scanHandlerFileForPermission.
    • The PR's whole-line-comment skip has to move into that function, around main's line 253.
    • The new specPathsForRoute and require.Len changes merge cleanly.
• [blocking] POST /api/ri-exchange/resolve/{id} has no spec entry.
  • fix(exchange): alarm on stale processing RI exchanges and let an operator resolve them #1109 (f9ae766) added this route on main without one.
  • On the resolved merged tree, TestOpenAPIDocumentsEveryRoute fails with "routes registered in router.go with no operation in openapi.yaml: POST /api/ri-exchange/resolve/*". That is the guard doing its job.
  • The handler calls requirePermission(..., "execute", "ri-exchange") and can return 403 and 503 (handler_ri_exchange_resolve.go:67,74,92 on main). The new entry must list 403, or the 403 guard fails next.
• [non-blocking] Path parameter names (M4) are now a documented limit (openapi_route_coverage_test.go:34-38). The probe still passes, as expected.
• The "//" finding is fixed by the whole-line skip at 2fe9db4. It needs porting as described above.
• [non-blocking] Merge vs rebase. The branch was updated by merging (d727905), not rebasing. A squash merge makes this moot.

Why the diff grew: openapi.yaml was not regenerated or reformatted.

• The default Myers diff exaggerates the change. git diff --diff-algorithm=histogram c8bdd460 2fe9db4 shows +4531/-67 across the 3 files.
• The growth over v1 is the newer base plus about 102 lines of real new YAML in 2fe9db4:
  • all 39 PurchaseHistoryRecord fields;
  • the HistorySummary counters;
  • me/permissions security.
• No file moved.

Attacked:

• Normalised parse (yq to JSON). Every operation, path-item, component and top-level key was compared structurally.
  • Base vs head: 141 additions. The only removals are the intended ones, the /api/config/service/{service} GET/PUT pair (renamed to {provider}/{service}) and CollectResult. Every changed operation matches a claim in the PR body, and no tags are lost.
  • Base vs main: no new operations, and 11 changed keys.
  • Three-way, on the resolved merged spec: nothing from head or main is lost, and nothing spurious appears. HistorySummary is the only key both sides changed.
• Mutation probes on a scratch copy, each restored and confirmed identical with cmp:
  • M1, /version deleted: the guard FAILS.
  • M2, listAccounts get changed to patch: FAILS in both directions.
  • M3, listAccounts' 403 removed: the 403 guard FAILS.
  • M4, {provider}/{service} renamed to {service}: passes. This is the documented limit.

Verified locally, with GOPROXY=off and no cloud credentials, under the heavy lock:

• Head (go1.26.9, matching its go.mod): gofmt, build and vet are clean, and go test -count=1 ./internal/api/ ./internal/mocks/ is ok.
• Merged tree, resolved by hand as above, with GOTOOLCHAIN=go1.26.10:
  • Build and vet pass.
  • TestOpenAPI403OnPermissionGatedRoutes, TestRouteCoverageGapsDetectsDrift and TestPartitionExemptReportsStaleEntries pass.
  • TestOpenAPIDocumentsEveryRoute fails on the resolve route only.
  • The full internal/api run has only that failure, and internal/mocks is ok.
• No migration.

Same reviewer as the earlier review, re-reviewing the new head. Review only: nothing was pushed, and the GitHub review state is unchanged.

[IAmJSD]

Merged origin/main into the branch to clear the conflicts. New head: 29ec73bc24fca1364663d1abff9f9fad0b39969c. It has a merge commit (502a6158) and one fallout fix (29ec73bc).

Resolution:

• openapi.yaml, GET /api/recommendations parameters: kept this PR's account_ids, min_savings_usd and min_savings_pct, and main's limit and cursor (fix(api): add opt-in in-memory cursor pagination to GET /api/recommendations #974). The AccountID (account_id) entry that main still carried stays dropped, because the handler never reads it.
• openapi.yaml, recommendations 400: kept main's detailed description (fix(api): add opt-in in-memory cursor pagination to GET /api/recommendations #974). It already covers this PR's filters.
• openapi.yaml, HistorySummary: kept main's schema description from feat(api): scope-aware SQL totals for the History summary cards #1200 and added a sentence on revoked rows. Also kept this PR's field descriptions for total_pending, total_in_progress and total_revoked, which match addHistoryRowToSummary on main.
• openapi_403_test.go: main (test: make the slowest unit and integration tests fast without weakening them #1403) replaced the per-route file scan with a single permissionGatedHandlers index built by scanHandlerFileForPermission. I kept main's structure and moved this PR's whole-line comment skip into scanHandlerFileForPermission. The first-match specPathsForRoute and the route-table length check merged cleanly on top.
• Semantic fallout: main added POST /api/ri-exchange/resolve/{id} (fix(exchange): alarm on stale processing RI exchanges and let an operator resolve them #1109), so TestOpenAPIDocumentsEveryRoute failed on the merged tree. 29ec73bc documents that operation from handler_ri_exchange_resolve.go: the request body, the response, and 400/403/404/409/503.
• I checked that every line main added to the spec since the merge base is still there, and that none of the lines main removed came back.

Checks:

• go build and go vet (default, integration and e2e tags) pass.
• golangci-lint v2.10.1 on ./internal/api/ reports 0 issues, with and without the integration tag. gocyclo has nothing to report, since no non-test Go files changed.
• go test -race -short and go test -race -tags integration on ./internal/api/ both pass.
• Secrets scan is clean and test-git-secrets-allowlist.sh reports 137 PASS.

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 29ec73bc24fca1364663d1abff9f9fad0b39969c

Blocking:

• none

Non-blocking:

• internal/api/openapi_403_test.go:315: the new require.Len only checks that the source parse of router.go and the live route table have the same length. specPathsForRoute replays first-match dispatch by index over the parsed entries, so if the two lists are equally long but in a different order, routes get mapped to the wrong spec operations without any failure. Comparing (exact, prefix, suffix, method) per index would close that, and it's a few lines.
• Commit 46734e7 is about 8.5k changed lines (mostly YAML), far over the 400-line-per-commit convention. Nothing enforces it, but nobody can review that commit line by line. I spot-checked the operations rather than verifying all 49.
• Merge-order conflict: feat(api): page the History view with a keyset cursor (has_more, next_cursor) #1277 (History keyset cursor) also edits /api/history parameters and the response schema in openapi.yaml, so whichever PR merges second has to reconcile. Whoever merges second should also re-run TestOpenAPIDocumentsEveryRoute, because any route added on main in the meantime now fails CI until it is documented. That is the guard working as intended, but it will surprise other open PRs that add routes.
• The guard's documented limits (parameter names not checked; /oidc and /api/scheduled out of scope) are acceptable for fix(api): openapi.yaml omits ~30 routed paths and mis-describes documented operations #262.

Attacked:

• Ran go build ./..., go vet ./internal/api/ (with and without -tags integration), go test -race -short -count=1 ./internal/api/ (ok) and golangci-lint run ./internal/api/ (0 issues). I also merged current origin/main (f6553c2) into a scratch copy and re-ran the OpenAPI/route-coverage tests: ok.
• Fail-before: I swapped in main's openapi.yaml and ran TestOpenAPIDocumentsEveryRoute. It fails and lists 50 gaps (the 49 routes plus the unrouted operation from the old /api/config/service/{service} shape).
• 403 guard coverage of newly documented routes: I deleted the '403' response from the new GET /api/accounts operation, and TestOpenAPI403OnPermissionGatedRoutes fails on it, naming listAccounts. Restored.
• Spot-checked the handler parity claims: the start/end params are read by handler_analytics.go and handler_history.go (start_date is read nowhere), and limit is read in handler_history.go. The spec parses (107 paths). The frontend only copies the YAML (webpack) and CI has no YAML linter that the new redocly warnings could trip.

Body-stale:

• none

_{Reviewer: rB11. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Local verification at 29ec73bc24fca1364663d1abff9f9fad0b39969c merged onto main (51957bcf), together with #1100, #1108, #1147, #1149, #1276, #1292 in that order (combined tree bc575de3, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 1 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 1 approving independent review(s) on 29ec73bc, green CI, and the combined-tree verification above. Squash-merged at exactly 29ec73bc24fca1364663d1abff9f9fad0b39969c.