feat(datastore): dispatch the repository create on kind (S17 Phase 6 plan: 10/42)
Retargeted onto main now that the stacked parent has merged: feat(managementapi): serialize remote settings ... (!1572 - merged) • Hayley Swimelar • 19.3.
Important
This branch carries two steps. Suleimi Ahmed collapsed the stack by merging the remote create enablement, step 24, into it: feat(managementapi): serve the remote repositor... (!1642 - merged) • Hayley Swimelar • 19.4. The sections below describe step 10, the datastore dispatch. Step 24's Why, non-obvious parts, and spec coverage stay in its own description and are not restated here.
Summary
The create path is kind-blind while the merged delete dispatch already gates kinds: RepositoryStore.Create today writes any kind value with no kind-matched child, so a directly-created remote row would violate the both-children invariant every protocol read path relies on (Phase 6 non-hosted tables). Step 10 of the merged Phase 6 plan: createTx keeps the kind-agnostic format child for every kind and dispatches the second, kind-matched child. A remote create routes to the per-format remote creates on the same transaction, so parent, format child, remote child, and collection link commit or roll back as one. A virtual create fails closed with ErrRepositoryCreateVirtualKind until the per-format virtual stores land. Merging after the delete dispatch keeps every created repository deletable from its first insert.
Non-obvious parts:
- The per-format remote creates are store methods carrying audit sinks, so
RepositoryStorecannot construct them.NewRepositoryStoreWithRemoteCreatesinjects them as narrow single-method interfaces; plainNewRepositoryStorekeeps its signature (its six production call sites, across four files, are untouched) and its remote arm fails closed with a typed error. The enablement step that lifts the handler's 422 is the one that wires the creators, and it is now co-resident on this branch, so the window that merge order opened is closed here rather than deferred to a follow-on merge. - Format discrimination is enforced at this layer too: each arm maps only the fields its format defines and refuses a supplied undefined field, so the datastore cannot silently drop a supplied credential or window value.
- The kind, settings-presence, and wiring rejections are pure over the input and fire before any statement is issued, mirroring
repositoryDeleteKindGate.
4007 added lines against 697 removed across both steps, in 45 files: 551 production Go, 2057 unit test, 1034 integration test, 199 OpenAPI, 148 Bruno, 18 docs. Step 10's own production share is the 300 added lines in repositories.go. Splitting is no longer on the table, because the two steps now share one branch, and each half's tests pin the dispatch, its fail-closed arms, and the transactional invariants they exist to prove.
No docs/testing/ scenario added or affected: the e2e catalogs are per-format protocol journeys, their remote scope-outs (proxy and cache journeys, owned by later slices) still hold, and a remote repository's protocol journey answers 501 until the proxy slices land.
The branch sits directly on main. Its 35 commits carry the two steps and the corrections successive review passes asked for: 19 docs, seven test, four style, two refactor, two chore, and one feat.
Governing ADRs
ADR-007 (database schema): the remote child insert joins the create transaction beside the format child, the sibling-rows shape its FK regime expects. ADR-004: the cap check is untouched and counts rows of every kind. The co-resident step's own ADR conformance (ADR-004 cap surfacing, ADR-021 audit actor) is in its description.
Testing
Re-run across both steps at the current head: gofmt, go build ./..., go vet ./..., and go vet -tags=integration ./... all pass.
golangci-lint 2.12 with --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false reports 0 issues in plain mode across cmd/artifact-registry, internal/datastore, internal/managementapi, and internal/remote. In integration mode the same four trees add nothing over main except 29 contextcheck findings in internal/datastore/repositories_integration_test.go, which is that file's own pre-existing pattern (258 already there on main, 4041 across the four trees). internal/managementapi is 0 in integration mode on both sides.
Unit suites pass for all four packages. The internal/managementapi and internal/datastore integration suites pass in full against PostgreSQL 17 and 18, each tuned to max_locks_per_transaction=1024, the setting .pg-service-options gives the CI services. TestEveryStatementIsInstrumented passes. So does TestWireManagementAPI_WiresRemoteCreatorsIntoRepositoryStore across all four formats, which is what proves the wire_management.go conflict resolution kept the remote creators wired rather than reverting to the plain store.
Spec coverage
Spec: docs/specs/S17-rest-management-api.md
Step 10's datastore slice. The HTTP halves of the cited criteria (201/echo bodies, 400/422 mappings) are step 24's, and its own coverage table is in feat(managementapi): serve the remote repositor... (!1642 - merged) • Hayley Swimelar • 19.4. Criteria listed in neither belong to other steps of the merged plan and their MRs.
Acceptance criteria
| # | Criterion (datastore slice) | Tests |
|---|---|---|
| AC-2 | Hosted create inserts parent, format child, and collection link in one transaction that rolls back fully | Pre-existing, unchanged: TestRepositoryStore_Create_EachFormat, TestRepositoryStore_Create_RollsBackOnFailure |
| AC-69a | Remote create of each format inserts parent, kind-agnostic format child, per-format remote child, and collection link in one transaction; the protocol finders resolve the created remote under their kind = remote parent predicate (both-children invariant) |
TestRepositoryStore_Create_RemoteEachFormat |
| AC-69b | The remote create transaction rolls back whole on any failure | TestRepositoryStore_Create_RemoteRollsBackWhole |
| AC-69c | The kind-and-format dispatch is total over the format enumeration and maps only the fields each format defines | TestRemoteChildCreates_CoverEveryFormat, TestMavenRemoteCreateInput, TestContainerRemoteCreateInput, TestNpmRemoteCreateInput, TestRepositoryCreateKindGate |
| AC-69d | Creator wiring: the wiring constructor guards its seams; a plain-built store's remote arm fails closed | TestNewRepositoryStoreWithRemoteCreates, TestRepositoryStore_Create_RemoteUnwiredFailsClosed, TestRepositoryStore_Create_KindDispatchGuards |
| AC-71 | Virtual create succeeds | Owned by the per-format virtual stores (later plan steps). The interim fail-closed contract is pinned: TestRepositoryCreateKindGate, TestRepositoryStore_Create_KindGateFailsClosed |
| AC-72 | The per-format repository cap counts rows of every kind | TestRepositoryStore_Create_CapCountsEveryKind |
| AC-73 | Credentials stored on create; a one-sided pair refused | TestRepositoryStore_Create_RemoteEachFormat (stored, audited once), TestRepositoryStore_Create_RemoteRollsBackWhole (remote.ErrIncompleteCredentials) |
| AC-109 | Delete of an empty remote repository removes all four rows (the deletable-by-construction interlock on rows Create assembles) | TestRepositoryStore_Create_RemoteThenDelete; seeded-row arms pre-existing: TestRepositoryStore_Delete_RemoteKind |
Error cases
| # | Condition (datastore slice) | Tests |
|---|---|---|
| E-1 | Create/Update: invalid url, one-sided credential, out-of-range cache window (400) |
Sentinels surface unswallowed from Create for the handler mapping: TestRepositoryStore_Create_RemoteRollsBackWhole (remote.ErrUpstreamBaseURL, remote.ErrIncompleteCredentials, the three per-format check-violation sentinels) |
| E-2 | Create: remote without settings; settings on a non-remote create (400) |
Store-side defense: TestRepositoryCreateKindGate, TestRepositoryStore_Create_KindDispatchGuards, TestRepositoryStore_Create_KindGateFailsClosed |
| E-3 | Create: name conflict (409); per-format cap (422) | Pre-existing, unchanged: TestRepositoryStore_Create_DuplicateNameConflict, TestRepositoryStore_Create_CapBoundary; cap across kinds: TestRepositoryStore_Create_CapCountsEveryKind |
Security considerations
| # | Concern | Tests |
|---|---|---|
| S-1 | Injection: jet builders with bound parameters | No new SQL: the dispatch composes the per-format creates, whose suites own their statements |
| S-2 | Tenant isolation | Pre-existing, unchanged: TestRepositoryStore_Create_NamespaceIsolation; the per-format parent gates are owned by the per-format create suites |
| S-3 | Upstream credentials never in error bodies | Classified sentinels replace pgconn errors inside the per-format creates (owned by their suites); TestRepositoryStore_Create_RemoteRollsBackWhole pins they arrive unswallowed via errors.Is |
| S-4 | SSRF static covers run at create | TestRepositoryStore_Create_RemoteRollsBackWhole (userinfo-bearing URL refused through Create, nothing stored) |
Two review rounds ran before this MR opened; their hardening rides the same branch: pinned constructor panic values, a partial-wiring falsification test, a reflection net that fails when CreateRemoteRepositorySettings gains a field no builder maps (falsified both ways by mutation), and integration rows driving each format's undefined-field refusal through Create to prove the rollback.
Database Review Evidence
Note
db-review-prep, query mode. Re-verified against the merge base with
main, the branch's target. Named by branch rather than SHA: a pinned
base SHA goes stale on every rebase.
No query changes to review, and no migrations, across either step. The one
hand-written SQL-issuing file either step changes,
internal/datastore/repositories.go, dispatches 16 statements across 15
functions, and every one of those functions is byte-identical to the
merge-base. The co-resident step's production files issue no SQL of their own:
internal/managementapi/create.go, delete.go, handler.go, and audit.go,
cmd/artifact-registry/wire_management.go, remote_audit_sink.go, and
wire_npm.go, and internal/remote/audit.go. Nothing renders different SQL, so there is no plan
to collect and no ephemeral database was started.
The added code orchestrates statements that already exist: the kind gate,
the remoteChildCreates dispatch map, the per-format create-input builders,
and the sentinels. The remote arm's INSERTs run in the per-format stores,
which this MR does not change and which carry their own evidence.
Context for LLM agents
Design rationale:
- Creator seam via a constructor variant rather than changing
NewRepositoryStore: the three remote stores needremote.AuditSink(container also a credential validator), and the six existingNewRepositoryStorecall sites, across four files (wire_management.go,wire_gitlab.go,wire_npm.go,npm_publish_committer.go), have no sink to give. Rejected alternatives: changing the signature (ripples sink plumbing into call sites that cannot serve a remote create anyway) and constructing the stores insideRepositoryStore(hides the audit dependency and would need a sink it does not have). - The remote arm calls the store methods rather than inlining their INSERTs, so URL normalization, the parent gate, error classification, and the credential-write audit stay single-sourced. An integration test asserts the credential-set audit event through the seam, which fails if the dispatch ever bypasses the stores.
- The union settings struct is format-discriminated at the arm, failing closed on undefined fields, because a silently dropped supplied value is the failure mode the spec's per-format field table exists to prevent.
Non-goals, do not raise these in review:
- The HTTP 201/400/422 matrix for create bodies is the create-enablement step's surface. That step is now co-resident on this branch, and its own description carries its coverage table.
- The virtual create contract (child row plus association handling) lands with the per-format virtual stores; the typed fail-closed error is the pinned interim behavior.
- The composition-root rewire arrived with the co-resident enablement step, not with step 10.
TestWireManagementAPI_WiresRemoteCreatorsIntoRepositoryStoreis the test that fails on an unwired store. - This MR carries zero
docs/plans/**changes by design. The plan's Status-table rows are filled by a dedicated table-owner MR, not per-step MRs. - No new SQL statement shapes: the remote arm composes the create statements the per-format stores already ship (each carrying its own review evidence), and the kind gate is pure over the input with no kind read.
Related to #314