Add auto repo registration feature

[seoes]

#16

• Add new git provider's edit form
• add new middleware for auto repo creation
• separate parsing and verify/load workflow from loadForgejoContext, loadGitLabContext middlewares

[cloudflare-workers-and-pages]

Deploying proval-demo with    Cloudflare Pages

Latest commit:	1323219
Status:	✅  Deploy successful!
Preview URL:	https://3ddcb92c.proval-demo.pages.dev
Branch Preview URL:	https://feat-auto-repository-registr.proval-demo.pages.dev

View logs

[proval-0e6d07]

⚫️ [PROBLEM] Update handler returns 500 on duplicate baseUrl (create returns 409)

⚫ [PROBLEM] updateById on a provider/baseUrl that already hits the unique index (packages/db/src/schema.ts:96) throws a raw UNIQUE constraint error, which falls through to throw e here and produces an unhandled 500. The create handler at apps/api/src/api/access/access.controller.ts:52-55 already maps this exact case to 409, but update does not — and the edit form now lets users freely change baseUrl. Add the same UNIQUE→409 mapping:

if (msg.includes("UNIQUE") || msg.includes("unique")) {
    return c.json({ error: "This access already exists" }, 409);
}

[proval-0e6d07]

🚨 [CRITICAL] Uncaught provider/decrypt errors in auto-create middleware become opaque 500s

🚨 [CRITICAL] The new auto-create branch performs external calls with no error handling: accessService.findForAutoCreate, decrypt(access.defaultWebhookSecret!), decrypt(access.accessToken), and await gitlab.isConnectedAccountProjectMaintainer() all propagate straight out of the middleware. GitLabProvider.fetchUserPermission rethrows non-404 provider errors (a transient GitLab outage or expired token: "GitLab permission lookup failed: …"), and decrypt throws on a bad ciphertext/key. Any of these kills the webhook request as an opaque 500 with no log, so auto-registration silently fails and deliveries are lost. Wrap this branch in try/catch and return 404/503 with logError, matching the pattern used in the API controllers. (Same pattern applies in verifyForgejoWebhook and in loadRepository's create step in apps/api/src/webhook/load-repository.middleware.ts.)

[proval-0e6d07]

Hi, thanks for putting this together — this is a meaningful addition: access connections can now carry a full default config and let webhooks auto-register repositories on GitLab and Forgejo, with a matching client edit flow and provider docs. Solid, well-connected feature.

Overview

Summary

The schema/migration (0040) adds the default* config column suite to the provider access table; GitLabAccessService gains findForAutoCreate with URL normalization and hasAutoCreateDefaultConfig gating; new webhook middleware (gitlab.middleware.ts, forgejo.middleware.ts, and a new shared load-repository.middleware.ts) verify the shared webhook secret via a timing-safe comparison and create the repository row on first delivery if the connection permits auto-creation and the connected account has Maintainer/Collaborator status. The client gets a dedicated /provider/[id]/edit route with an AccessEditForm whose canSubmit mirrors the server contract, plus docs for both self-hosted providers.

🟢 Good points

• 🟢 verifyGitlabToken uses node:crypto.timingSafeEqual instead of string equality for the webhook secret (apps/api/src/webhook/gitlab/gitlab.middleware.ts:19), removing a timing side channel with a guarded length check and an early return for the absent header.
• 🟢 GitLabAccessService.toResponse strips accessToken and defaultWebhookSecret and exposes only hasDefaultWebhookSecret (apps/api/src/api/access/access.service.ts:44), closing a secret-leak path through all list/detail/create/update responses and matching the existing sensitive-response pattern.
• 🟢 loadRepository handles the unique-constraint race on concurrent first webhooks by re-fetching scoped to provider+accessId, so the happy path is idempotent (apps/api/src/webhook/load-repository.middleware.ts:98).
• 🟢 Webhook token verification still runs after findForAutoCreate in both middlewares — auto-creation is gated by a valid secret plus Maintainer/Collaborator checks, no authorization shortcut was introduced.

Main Issues

• 🚨 [CRITICAL] apps/api/src/webhook/gitlab/gitlab.middleware.ts:89 — the auto-create branch calls findForAutoCreate, decrypt, and isConnectedAccountProjectMaintainer() with no try/catch; a provider error, expired token, or decrypt failure escapes as an unhandled 500 with no log, silently losing webhook deliveries. See inline comment. (Same unguarded-external-call pattern applies in forgejo.middleware.ts and inside loadRepository.)
• ⚫️ [PROBLEM] apps/api/src/api/access/access.controller.ts:125 — updateAccessById doesn't map UNIQUE constraint failures to 409 like create does, so editing a connection to a duplicate baseUrl returns an unhandled 500 instead of "This access already exists". See inline comment.

Additional Notes

• apps/api/src/api/access/access.controller.ts:127 — the update catch maps client errors by substring ("required", "auto create"); an unrelated server error whose message contains "required" would be misclassified as a 400 and lose its stack. A dedicated validation-error type would be more robust.
• The new validation branches (updateById's per-field rules, findForAutoCreate's normalization, and load-repository.middleware.ts:72-93's defaultConfigPolicy column-suffix mapping) have no test coverage; the suffix-based key mapping in particular silently breaks mappings if column names drift, so unit tests there would be high value.
• apps/client/src/routes/provider/AccessEditForm.svelte:96 — the $effect syncing ~15 form fields from item re-runs if item identity ever changes, which would wipe in-progress edits; direct $state(item.x) initializers would be safer since the component is recreated per route.
• apps/client/src/routes/provider/AccessEditForm.svelte:135 — switching selectedModelProviderId doesn't reload the cached model list, so the picker can offer models from a different provider until it's reopened.
• Docs note: apps/web/src/content/docs/forgejo.md:51 says Write access is required, but isConnectedAccountCollaborator passes for any permission > 0 (apps/api/src/git-provider/forgejo.ts:59) — either soften the doc or tighten the check to Write level.

[cloudflare-workers-and-pages]

Deploying proval with    Cloudflare Pages

Latest commit:	1323219
Status:	✅  Deploy successful!
Preview URL:	https://8181da6f.proval.pages.dev
Branch Preview URL:	https://feat-auto-repository-registr.proval.pages.dev

View logs

[proval-0e6d07]

⚫️ [PROBLEM] ⚫️ [PROBLEM] New 503 guard skips the existing-repository branch — its decrypt/DB throws stay unlogged 500s

The try/catch added in this push starts only at the auto-create branch (line 73). The existing-repository branch above it — the most common path, since it handles every delivery for an already-registered repository — still runs unguarded: fetchWebhookContextRow(projectId, "gitlab") at line 50 and decrypt(repository.webhookSecret) at line 54. The same gap remains in forgejo.middleware.ts (fetch at line 78, decrypt at line 82) and in load-repository.middleware.ts, whose tail decrypts repository.accessToken, modelProvider.apiKey, and access.accessToken (lines 134, 139-140) outside the new try that ends at line 123.

decrypt throws on an AES-GCM auth-tag failure (ENCRYPTION_KEY mismatch or rotation) and loadEncryptionKey throws when the key is unset or wrong-length (util/encrypt.ts:14-31, 55-66), and there is no app.onError anywhere in apps/api (grep found none), so these failures still surface as Hono's default 500 with no logError output — the exact opaque-failure mode this push just fixed one branch over, now on the sibling branch in the same request.

Could be wrong if you consider provider 5xx retries sufficient, but without a log line operators still have nothing to triage. Either extend the guard to each middleware's full body, or install a single webhookApp.onError in app.ts that logErrors and returns the same 503 shape — that would cover all of these call sites at once.

[proval-0e6d07]

Hi — thanks for the quick follow-up. Both inline threads from the last review are addressed here, and the fixes are clean and well-scoped.

Overview

Summary

This push resolves the two flagged issues: the GitLab and Forgejo auto-create verify branches now wrap findForAutoCreate, decrypt, and the provider maintainer/collaborator checks in a try/catch that logs and returns a consistent 503 "Webhook processing failed" (apps/api/src/webhook/gitlab/gitlab.middleware.ts:73-105, mirrored in forgejo.middleware.ts), and updateAccessById maps duplicate-baseUrl UNIQUE failures to the same friendly 409 that create returns (apps/api/src/api/access/access.controller.ts:134-136). loadRepository's repository-create block got the same 503 guard (apps/api/src/webhook/load-repository.middleware.ts:73-123), and a new logWebhookRequest middleware now logs every ingress delivery on all three webhook routes, including the previously untouched GitHub chain (apps/api/src/webhook/webhook.route.ts:14, 24, 31).

Good points

• 🟢 The duplicate-baseUrl fix reuses create's exact contract — same message and 409 status (apps/api/src/api/access/access.controller.ts:55-56 vs 134-136) — and sits after the existing 404/400 mappings, before throw e, so edit-form users get the same "This access already exists" alert they see on create.
• 🟢 The new 503 guard preserves the intended control flow precisely: negative auth results stay as 401/404 early returns inside the try (only genuine throws become 503), and loadRepository keeps the "row vanished after the create race" case as a direct 404 return inside the catch's reach (apps/api/src/webhook/load-repository.middleware.ts:116-118) instead of swallowing it into 503.
• 🟢 logWebhookRequest runs first in every chain (apps/api/src/webhook/webhook.route.ts:14), so operators get a delivery-count signal before parse (400) or verify (401/404) rejects — which is what makes the new 503 paths diagnosable at all.

Main Issues

• ⚫️ [PROBLEM] apps/api/src/webhook/gitlab/gitlab.middleware.ts:54 — the 503 guard added in this push starts only at the auto-create branch (line 73); the existing-repository branch — the most common path, handling every delivery for an already-registered repository — still runs fetchWebhookContextRow (line 50) and decrypt(repository.webhookSecret) (line 54) unguarded. Same gap in forgejo.middleware.ts (lines 78, 82) and load-repository.middleware.ts (tail decrypts at lines 134, 139-140). decrypt throws on AES-GCM auth-tag failure / key mismatch (apps/api/src/util/encrypt.ts:55-66) and no app.onError exists anywhere in apps/api, so these still surface as opaque 500s with no log — one branch over from the failure mode this push just fixed. See inline comment. Could be wrong if provider 5xx retries are considered sufficient, but without a log line there's nothing to triage.

Additional Notes

• apps/api/src/webhook/load-repository.middleware.ts, line 121 — the new 503 catch calls logError without a provider label, while the sibling catches pass "GitLab" / "Forgejo"; it's derivable from createInput.provider for consistent triage output.
• The new failure semantics (verify throws → 503 vs invalid signature → 401, unique-race re-fetch → fall-through, missing row after re-fetch → 404) have no test coverage; a small Hono-based middleware test would lock in which responses skip await next().

The 409 and 503 fixes match what the prior threads asked for — with the existing-branch guard closed, the webhook chain would be fully covered.

[proval-0e6d07]

Hi — nice follow-up: the per-middleware 503 guards are gone and replaced with a much tidier central handler.

Overview

Summary

This push trades the three duplicated try/catch blocks from the last round for a single webhookApp.onError (apps/api/src/app.ts:9-14) that logs unexpected errors via logError and returns a consistent { error: "Webhook processing failed" } 503, while passing HTTPException responses through untouched. The GitLab and Forgejo verify middleware and loadRepository are unwrapped to plain control flow with no behavior change (verified in apps/api/src/webhook/gitlab/gitlab.middleware.ts, apps/api/src/webhook/forgejo/forgejo.middleware.ts, apps/api/src/webhook/load-repository.middleware.ts). Since webhookApp is the fetch entry with every provider chain mounted under it, this also covers the existing-repository and GitHub branches that the previous review flagged as still unguarded — the gap is now fully closed, and future middleware added to any chain automatically gets the same error surface.

Good points

• 🟢 The central onError keeps intentional responses intact because they are returns, not throws — negative auth results (401) and missing rows (404) never touch the handler, so only genuine decrypt/DB/provider failures become logged 503s (apps/api/src/app.ts:10-12).
• 🟢 The unique-constraint race handling in loadRepository was deliberately preserved: the inner try/catch around repositoryService.create re-fetches the row on failure and only rethrows when none exists, so concurrency tolerance survives the refactor (apps/api/src/webhook/load-repository.middleware.ts:92-100).
• 🟢 Removing the provider-specific catch labels is offset by a single greppable "Webhook processing failed" line with the full stack for every failure class across all three chains — uniform and easier to triage than three different messages.

Main Issues

None. All verification passed.

Additional Notes

• The new onError has no test coverage; a small test driving webhookApp with a throwing middleware (asserting 503 "Webhook processing failed") and one throwing HTTPException would lock in the contract against future refactors (apps/api/src/app.ts:9-14).
• The HTTPException pass-through branch returns error.getResponse() without logging (apps/api/src/app.ts:10-11); nothing currently throws HTTPException, so this is dead-but-defensive — consider logging pass-throughs with status ≥ 500 to preserve diagnosability if that changes.
• parseGitLabWebhook uses await c.req.json() unguarded (apps/api/src/webhook/gitlab/gitlab.middleware.ts:31), so a malformed body now surfaces as a logged 503 instead of the clean 400 "Invalid JSON body" that the Forgejo and GitHub chains return; pre-existing inconsistency, but more visible now that all failures funnel through one handler.

[seoes]

@proval-0e6d07 Could you do a final review of the whole thing?

[proval-0e6d07]

@seoes

Hi — final review done, covering the full 24-file change end to end (schema/migration, types, access service/controller, GitLab & Forgejo webhook chains, central error handling, client edit form, demo fixtures, docs). The two threads from the earlier rounds are resolved, and the central onError from the last push now closes the previously flagged existing-branch gap. What remains is one real defect in the migration and two small polish items.

Overview

Summary

Verified end to end: the three GitLab/Forgejo webhook chains now run logWebhookRequest → parse → verify → loadRepository → logWebhookIngress → handler (apps/api/src/webhook/webhook.route.ts:13-32), with every throwing path funneling into the central webhookApp.onError that logs and returns a uniform 503 ("Webhook processing failed") while passing intentional 400/401/404 returns through untouched (apps/api/src/app.ts:9-14). Auto-registration matches an access by provider + normalized instance origin (findForAutoCreate, apps/api/src/api/access/access.service.ts:77-95), verifies the delivery against that access's defaultWebhookSecret, gates on maintainer/collaborator status with the connection PAT, and persists a repository via loadRepository with the default-config policy mapped onto repository columns (apps/api/src/webhook/load-repository.middleware.ts:60-118). Secrets stay sealed: AccessResponse omits both accessToken and defaultWebhookSecret in favor of a hasDefaultWebhookSecret flag (apps/api/src/api/access/access.service.ts:37-44, packages/types/src/database.ts:50-52), and the previously plaintext GitLab token comparison is now timing-safe (verifyGitlabToken, apps/api/src/webhook/gitlab/gitlab.middleware.ts:9-20).

Good points

• 🟢 The last review's main gap is fully closed: decrypt(repository.webhookSecret) in the existing-repository branches (gitlab.middleware.ts:47, forgejo.middleware.ts:77) and the tail decrypts in loadRepository (load-repository.middleware.ts:120-134) no longer surface as opaque 500s — everything is caught by the central handler, and because every provider chain is mounted under webhookApp (apps/api/src/app.ts:19), the GitHub chain and future middleware get the same 503 surface for free.
• 🟢 The unique-constraint race in loadRepository regressed nothing: on create failure it re-fetches scoped by access.id and only rethrows when no row exists (apps/api/src/webhook/load-repository.middleware.ts:96-106), so the concurrency case still falls through to a successful load while genuine DB/auth/decrypt failures become logged 503s.
• 🟢 The default-config → repository mapping in loadRepository derives repository column keys from the schema (defaultModelProviderId → modelProviderId, etc., load-repository.middleware.ts:71-84), so it stays in sync if new default* columns are added, and hasAutoCreateDefaultConfig (line 28) guarantees no null default ever reaches the insert.
• 🟢 Migration chain is intact and additive: 0040_slimy_queen_noir.sql appends 19 nullable/b defaulted columns with correct DEFAULT false NOT NULL for auto_create_enabled, the journal continues at idx 40 with prevId matching 0039_snapshot.json, and the FK default_model_provider_id → model_provider(id) is present in both schema and snapshot.

Main Issues

• ⚫️ [PROBLEM] packages/db/src/migration/0040_slimy_queen_noir.sql:3 — the default_model_provider_id column is added via plain ALTER TABLE ... ADD ... integer REFERENCES model_provider(id) with no ON DELETE clause, but packages/db/src/schema.ts:79 and packages/db/src/migration/meta/0040_snapshot.json:493 both declare onDelete: "set null". SQLite defaults an inline REFERENCES to NO ACTION, and the API enables PRAGMA foreign_keys = ON (apps/api/src/db/index.ts:6), so on any database created from these migrations, deleting a model provider that is referenced as a default config throws FOREIGN KEY constraint failed instead of nulling the column as the schema promises; a future drizzle migration reconciling the snapshot would also force a table rebuild. Fix: either (a) keep set null and ship the column via a temp-table rebuild in this migration, or (b) accept restrict-like behavior — then update the schema/snapshot to match and have removeModelProvider proactively clear defaultModelProviderId (and optionally auto-create config) before deleting. See inline comment.

• 🔵 [MINOR] apps/api/src/git-provider/forgejo.ts:62 — isConnectedAccountCollaborator returns level > 0, i.e. any collaborator with Read access passes the auto-create gate, while the docs (added in this PR) require Write access in both the manual and automatic paths (apps/web/src/content/docs/forgejo.md:52, 58). Either tighten the check to level >= 3 (write = 3 per forgejoPermissionToLevel, forgejo.ts:986-995) or reword the docs to "collaborator with at least Read access" — currently the documented gate and the enforced gate disagree. The GitLab side is consistent (level >= 4 at gitlab.ts:569 ↔ "Maintainer or above", gitlab.md:96).

Additional Notes

• parseGitLabWebhook still parses with unguarded await c.req.json() (apps/api/src/webhook/gitlab/gitlab.middleware.ts:32) — a malformed body is now a logged 503 instead of Forgejo's clean 400 "Invalid JSON body" (forgejo.middleware.ts:42-44). One-line try/catch would make the three chains no longer funnel inconsistent errors through the new central handler.
• fetchWebhookContextRow matches only (gitProviderRepositoryId, provider) (apps/api/src/webhook/load-repository.middleware.ts:34-46), and the DB uniqueness is (gitProviderRepositoryId, gitProviderAccessId) — a repo id that collides across two same-provider instances (e.g. id 101 on two distinct GitLab hosts) can resolve to the other instance's row, producing a 401 on delivery and suppressing auto-registration for the second instance. Could disambiguate by comparing access.baseUrl origin against the instance URL now available from the payload. Pre-existing behavior, unchanged by this PR — noting it because the auto-create path is otherwise instance-aware.
• The new X-Gitlab-Instance header (gitlab.middleware.ts:66) is undocumented; since payload.project.web_url is the fallback, a sentence in the gitlab.md automatic-registration section would help operators on proxied setups.
• updateById requires autoCreateEnabled at runtime (apps/api/src/api/access/access.service.ts:134-136) while AccessUpdateInput is Partial<…> (packages/types/src/database.ts:70) — type and contract disagree slightly; only affects raw API callers since AccessEditForm.svelte always sends it.

Everything else verified clean: edit-form save path sends autoCreateEnabled + full default config and omits the secret when blank so the stored one is preserved (AccessEditForm.svelte:186-223), client-side canSubmit mirrors the server's required-field checks (line 103), demo fixtures conform to the new AccessResponse shape and demo-mode PUT yields a graceful 403 alert, controller error mappings (400/404/409) catch all validation paths the service throws (access.controller.ts:140-155), and the docs' automatic-registration walkthroughs match the actual middleware behavior.

With the migration FK decision made (issue 1) and the Forgejo gate aligned with the docs (issue 2), this is in good shape to merge.

[proval-0e6d07]

Hi — thanks for pushing this; it directly closes the biggest item from the last review.

Overview

Summary

This push adds packages/db/src/migration/0041_odd_buldozer.sql (tag 0041_odd_bulldozer), a data-preserving table rebuild of git_provider_access that reimplements the table with the full 26-column shape — including the FOREIGN KEY (default_model_provider_id) REFERENCES model_provider(id) ON DELETE set null clause that was missing from migration 0040's inline column definition. The rebuild follows the canonical SQLite pattern (PRAGMA foreign_keys=OFF → create __new_git_provider_access → INSERT INTO ... SELECT copying every column including id → DROP TABLE → RENAME → PRAGMA foreign_keys=ON), with the unique index recreated after the table is dropped.

I verified the key correctness points:

• Columns, types, not-null and DEFAULT false NOT NULL on auto_create_enabled in the rebuild match /packages/db/src/schema.ts (gitProviderAccessTable) and 0041_snapshot.json 1:1, and the FK ON DELETE set null now matches what the schema and 0040 snapshot declare — so the prior review's main issue (migration-created databases throwing FOREIGN KEY constraint failed on model-provider deletion) is resolved for DBs created from these migrations and for existing databases that run 0041 (the rebuild fixes the constraint on the live table too, not just for fresh installs).
• The INSERT INTO ... SELECT copies all 26 columns including id, so row identity that downstream git_provider_access_id foreign keys rely on is preserved.
• Snapshot chaining is intact: 0041_snapshot.json's prevId (line 5) matches 0040_snapshot.json's id (line 4), and _journal.json appends idx 41 with a later timestamp.
• Recreating git_provider_access_provider_baseUrl_unique after the DROP TABLE/RENAME is necessary and correct — dropping the table removes the old index, so this restores the (provider, base_url) uniqueness backstop the API's 409 handling in access.service.ts depends on, with no index-name collision.

No Main Issues this round. One cosmetic note: the SQL file has no trailing newline (\\ No newline at end of file) — harmless, just a small lint/formatting nit if your tooling cares.

I don't see anything blocking here; with the FK repaired, I'd consider this PR ready to merge.