Skip to content

Add janitor's HttpApi foundation with the sync group - #130

Merged
tim-smart merged 4 commits into
mainfrom
agent/nelson/a1890d85c55a
Oct 20, 2026
Merged

tim-smart merged 4 commits into
mainfrom
agent/nelson/a1890d85c55a

Conversation

@tim-smart

@tim-smart tim-smart commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Closes EFF-2516

Adds JanitorApi and moves the sync routes onto it as the first group.

Domain

  • @janitor/domain/Api defines JanitorApi with the /api/v1 prefix. Api/Sync.ts has getSummary, requestAll and requestRepository. PUT /repositories/:id/sync is gone.
  • RepositoryNotFound (404) moves from Labeling/Configuration.ts to GitHub/Repository.ts with the same tag, next to the new SynchronizationDisabled (409).

Cluster

  • Ingress/Sync.ts is now one HttpApiBuilder.group(JanitorApi, "sync", …) layer.
  • SyncStatus.requestRepository reads repository_block_reason(). It fails with RepositoryNotFound, RepositoryBlocked { reason } or SynchronizationDisabled. Infrastructure failures die across the whole service, so SyncStatusError is gone.
  • Routes.ts merges HttpApiBuilder.layer(JanitorApi) beside the raw routes and outside ApiRouterLayer. The middleware is provided to the builder layer, as one combine chain: Access, then Membership, then SameOrigin. ApiRouterLayer now serves only Live, Readiness and the webhook.
  • Middleware.ts owns isSameOrigin and both SameOrigin wrappers. SameOriginWritesMiddleware lets GET and HEAD through and checks everything else; Live keeps the unconditional check. AccessKeysUnavailable, a missing Teammates and a failed admit now die, so the response is an empty 500 instead of 401 or 503.
  • Worker.ts provides AccessJwt.layerFrom(...) and FileSystem.layerNoop({}) to makeRoutesLayer.

Web

  • lib/api-client.ts adds ApiClient, built once in entry.ts on FetchHttpClient.layer and added to AppServices.
  • The sync button and the connections page's "Retry sync" call api.sync.* and share one syncRefusalCopy for the three refusal tags. The duplicated SyncSummary schema is gone.
  • routes.ts decodes the repository segment with GitHubRepositoryDatabaseId, so a path segment that isn't an id now matches no route. The route still types repositoryId as a string: typing it with the brand would spread into the wire models (ConnectionCandidate, ConfigurationView, labeling-wire) that later group PRs move. The commands assert the brand with GitHubRepositoryDatabaseId.make before calling the client.

Validation

At 94d3a13:

  • nix develop -c pnpm exec vp check: 0 errors (323 existing warnings).
  • nix develop -c pnpm exec vp test: 1039 of 1043 pass under full-suite load; the four failures are 5-second timeouts in PullRequestReview/{Publish,ReviewAgent,RunEntity}.test.ts, which pass 58/58 when run on their own. The four Routes.test.ts wiring cases pass.
  • Routes.test.ts checks the two wiring mistakes the decisions warn about: with the middleware provided to the group layer, the 401 and 403 cases fail; with the builder inside ApiRouterLayer, all four fail.

Tests only: they fail against the current routes until the sync group,
the method-gated SameOrigin middleware and the domain errors land.
JanitorApi in @janitor/domain/Api carries the sync group: getSummary,
requestAll and requestRepository, with PUT {repo}/sync dropped. The
cluster serves it through HttpApiBuilder beside the raw routes, behind
Access, Membership and a method-gated SameOrigin check, and the web app
calls it through a derived ApiClient.

requestRepository now refuses with RepositoryNotFound, RepositoryBlocked
or SynchronizationDisabled from the domain instead of prose, and missing
Access keys or teammate admission failures die as an empty 500.

@tim-smart tim-smart left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reviewed 9870601d66165ecde9f03f79681003ecee800ec5 against EFF-2516. No actionable correctness or simplicity findings. Verdict: passes review. This is a comment review because the authenticated account is also the PR author.

The builder is outside the raw-route prefix layer, with Access → Membership → SameOrigin middleware provided to the builder rather than the group. The real-route tests cover reachability, missing assertions, cross-site writes versus reads, and malformed path parameters. They guard both known wiring mistakes.

The documented web deviations are acceptable: repository IDs are validated at route parsing and again before typed-client calls despite the route type remaining string; the connections page reuses its existing reason-specific copy. The final test-only commit supplies the missing provider and request services without weakening inferred context types.

Independent validation through Nix:

  • pnpm exec vp check: 0 errors, 323 warnings.
  • pnpm exec vp test: 129 files, 812 tests passed.

The PR description still reports the superseded type error and test timeout; please update its validation section to reflect the final commit.

SyncStatus dies on infrastructure failures everywhere, so SyncStatusError,
its wrap helper and the handlers' orDie go. The sync handlers read the
service with SyncStatus.use. The web's two sync commands share one refusal
copy function and assert the already-validated id instead of re-decoding it.
The test client takes its group names explicitly, and the routes test drops
the duplicated RuntimeContext middleware.

@tim-smart tim-smart left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reviewed the simplification delta 9870601..94d3a13. Verdict: passes review; no actionable findings. This is a comment review because the authenticated GitHub account is also the PR author.

Standards

No correctness or simplicity regressions found. Dispatch still follows successful sync requests and remains best-effort. Moving infrastructure failures into SyncStatus removes redundant wrapping without changing the HTTP failure contract. The shared refusal copy avoids duplicate error mapping.

Spec

The delta preserves EFF-2516 acceptance criteria: typed repository refusals, infrastructure defects, typed-client calls, and real-route wiring coverage. Using .make is safe at the current call sites because route parsing validates the sync button's repository ID and the retry action uses server-issued inventory IDs. Removing the duplicate runtime-context middleware retains request-context provision.

Independent validation at 94d3a13 through Nix:

  • pnpm exec vp check: 0 errors, 323 warnings.
  • pnpm exec vp test: 129 files, 812 tests passed, including the affected cluster and web tests.

All four GitHub CI checks are also green at this head. No code changes made.

@tim-smart
tim-smart merged commit 3c76e70 into main Oct 20, 2026
4 checks passed
@tim-smart
tim-smart deleted the agent/nelson/a1890d85c55a branch October 20, 2026 02:17
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