feat(managementapi): serve the remote settings PATCH (S17 Phase 6 plan: 23/42)

Was stacked on chore(datastore): add the maven remote row-lock... (!1585 - merged) • Hayley Swimelar • 19.4, which has merged, so this targets main directly.

Summary

PATCH /api/v1/:slug/repositories/:repository_name accepted the fields that apply to every kind and rejected settings outright, so a platform engineer who created a remote repository could never change its cache windows or rotate its upstream credentials without deleting and recreating it. This adds the settings object to the update surface for remote repositories: a field-wise merge over the cache windows and the per-format flags, and the credential tri-state (replace, clear with credentials: null, keep by omission), each write audited.

url stays out of the published update schema. Re-pointing an upstream also has to clear the credentials scoped to the old host, discard the health verdict measured against it, and evict the cached rows filled from it, and none of that is reachable from this arm, so a settings.url in a PATCH answers the same undefined-field 400 as any other field the schema does not declare. Step 34 of the plan publishes the field together with those obligations.

Four things a reviewer would not read off the diff:

Three transaction composers, not one. The per-format update stores take a caller's *sql.Tx and internal/managementapi held no transaction seam, so this is the first handler step that needs one. internal/datastore/remote_repository_updaters.go follows the package's precedent (container_manifest_deleter.go): the composer owns the BeginTx/Commit/Rollback envelope, and the handler consumes it through an interface whose signature hides the transaction. Three composers, one per family, mirroring the three details-reader seams the read side already has, which keeps sibling-step edits in this plan disjoint.

The credential actor is a required parameter, not an optional field. remote.RecordCredentialWrite now takes the actor positionally, so a caller cannot forget it. The store emits the credential event and the handler emits the repository-update event, and both name the same actor because auditActor is a pure function of the request context, so a single PATCH cannot land two differently attributed audit entries. Under the bootstrap-token validator that actor is the empty string on every request, because it authenticates everything as anonymous. The change reaches into internal/datastore/{maven,container,npm}_remote_repositories_update.go and internal/remote/{audit,credentials}.go, files the plan assigned to earlier steps. See the plan-contradiction note below.

A credential value carrying a control byte is a 400, for two reasons rather than one. LabKit's pool runs pgx in simple-protocol mode, so a credential reaches PostgreSQL as interpolated query text and a NUL truncates the wire query and fails the whole statement. This route is the first client-reachable write to those columns, so without that half a malformed value would answer a logged 500. The rest of C0 and DEL store fine. They are refused because a stored credential becomes an Authorization header value and net/http will not carry a control byte, so the failure would move to every outbound fetch. validContainerCursorName is the precedent for the first half only, and says so itself.

The request union is anyOf, deliberately permissive. A field-wise PATCH carrying only the fields all three families share satisfies every branch, so no request-side oneOf can hold. Per-format field admission belongs to the strict decode, which answers the undefined-field 400. The response side keeps its closed oneOf.

Governing ADRs

ADR-009 (API design): conformant. ADR-009:105 is the third bullet of a list introduced by "The repository detail response is polymorphic", and the two bullets above it are about the response. On a narrow reading the bullet does not bind the request schemas, so there is no deviation here. Everything else in ADR-009 holds: settings stays a single nested object rather than optional top-level keys, the immutable trio is a 422, and the error envelope is unchanged.

Two steps complete the surface, not one: Step 24 gives CreateRepositoryRequest its settings, and Step 34 adds url to the per-format update branches. Step 34 carries Depends on: Step 23.

ADR-007 (database schema): no schema change. No column, index, or constraint is added or altered. The credential columns remain tmp_plaintext_* rather than the encrypted_* that ADR-007:29 mandates, which is the pre-existing GA-blocking interim tracked by gitlab-org/ops/artifact-registry#68+s and recorded in the S14 spec. This branch neither widens nor narrows it.

ADR-021 (authorization): unchanged pre-existing gap. ADR-021:96 maps repository-settings updates to the update_repository action, and no management-API path calls authz today. That gap is systemic across the whole management API and is the accepted closed-beta posture recorded in the S17 spec, not something this step opens or closes.

ADR-023 (code structure): conformant. The composer lives in internal/datastore/, one of the locked paths, and no path is renamed. The consumer-side interfaces sit in internal/managementapi/handler.go, which imports no format package.

ADR-004 (limits): not engaged. The 2048-character credential bound this step enforces comes from the S17 spec, not from ADR-004, which sets no credential or settings limit.

Diff size: 5368 reviewable LOC

Over the 500-LOC bar docs/dev/development-model.md sets. Measured as added and deleted lines, not churn, against main, and re-measured on every rebase. The figures held through the chore(datastore): add the maven remote row-lock... (!1585 - merged) • Hayley Swimelar • 19.4 squash. They moved by one line when main split requireArtifactWriteDeps out of wireManagementAPIWithDeps, because this branch's guard call now sits beside that helper's call rather than ahead of the comment block it absorbed. Review round 1 moved them again, by the six commits at the head of the series.

File group	Files	Added	Deleted
Production Go	18	1526	262
Test Go	15	3166	25
OpenAPI contract	1	229	53
Bruno collection	2	64	1
Docs (observability.md)	1	23	19
Total	37	5008	360

Two thirds of it is tests, and splitting does not help. The three composers, the three handler dispatch arms, and the three store inputs are one mechanism expressed three times: a split by format would ship a handler arm whose seam is unwired, or a wired seam no route reaches. A split along the handler-and-datastore line would ship composers with no caller, the shape the project's wiring guardrail exists to prevent.

The production half is 1526 added lines across 18 files, concentrated in three: update.go (+571), the new remote_repository_updaters.go (+293), and settings.go (+222). The other 15 average 29. Of those 1526 lines, 649 are code and 738 are comments (139 blank), so the Go a reviewer has to trace is closer to 649 lines than to 1526.

Guardrails

• e2e catalogs (13): no scenario added or affected. docs/testing/e2e/docker.md, maven.md, npm.md, and oci.md each put virtual and remote (proxy/cache) repositories out of scope "until the capability ships" in their ## Scope section. The catalogs gain scenarios when the remote capability itself is delivered, not per management-API step.
• Conformance (12): not applicable. This changes no Maven, npm, or OCI protocol surface.
• Configuration reference (17): untouched. No schema, loader, or config.example.yaml change.
• Bruno (18): update-remote-settings.bru lands with the OpenAPI change, and delete-repository.bru moves to seq: 9 so a folder run reaches the settings PATCH before the delete.
• Run recipe (22): unchanged, and re-run. No new route, build step, or boot path: the PATCH route already existed and only the body it accepts widened. Four dependencies did become boot-required though (the three updaters and Audit), so a missed wire now panics the service instead of failing a request. driver.sh smoke reports 24 passed, 0 failed on this branch.

Depends on

The pg.NULL wrap sweep, and it has landed. go-jet's pg.StringExp and pg.TimestampzExp call setRoot on what they wrap, so two concurrent callers mutate the shared pg.NULL and race. This branch adds no such site and converts none, but it is what makes the credential-clear arm reachable: before it no production path called any Update*RemoteRepository method at all, so a concurrently served PATCH carrying {"settings":{"credentials":null}} on a docker, oci, or npm remote is the first request that can reach one.

Three of the bare sites sat in that arm, two in container_remote_repositories_update.go and one in npm_remote_repositories_update.go. The requirement was to merge the sweep first so the window never opens, rather than to convert them here, where the edit would collide with it. fix(datastore): stop nullable binds from wrappi... (!1710 - merged) • João Pereira • 19.4 is that sweep and merged on 2026-08-20, so this branch now sits on a main where every site is converted and the blocking lint:jet-null-wrap job guards them. That job passes here, and git grep finds no wrapper over pg.NULL anywhere in the tree. The ordering requirement is met rather than outstanding, and pg.NULL singleton mutation races across remote ... (#696 - closed) • Hayley Swimelar • 19.4 is what tracks whatever remains of the class.

Two things to know before deploying this

The audit trail's level gap closed on main while this was open, and the actor gap did not. Inject the application logger into the remote c... (#556 - closed) • Dzmitry (Dima) Meshcharakou asks for the credential trail's logging gaps to be closed before a credential-writing consumer ships, and this route is that consumer. Both gaps were logging-threshold problems: the trail rode Info through the application logger, so log.level: warn discarded it and log.level: error discarded the boot breadcrumb reporting that, and an off-request GITLAB_LOG_LEVEL governed the emitters that write outside a request. main has since given remoteAuditLogSink its own injected logger, floored at levelCredentialAudit by newRemoteAuditLogSink, so no configured level discards the trail and the breadcrumb is retired along with the blind spot it could not cover. This branch rebases onto that and carries none of the earlier wording.

What remains is the actor, and it is certain rather than conditional. auditActor renders whatever the request identity resolved to, and the bootstrap-token validator authenticates every request as anonymous, so every credential record this route emits carries audit_actor: "". No write is lost, the field is filled the moment a validator resolves a real identity, and nothing here introduces it. docs/dev/observability.md records it, scoped.

Rolling deploys serve the new request field unevenly for one window. During the rollout an old pod answers a settings body with the undefined-field 400 while a new pod answers 200, so a client retrying across pods sees both. The window closes when the rollout finishes, and no partial write is possible because each request is served entirely by one pod. Worth knowing when reading 400 rates during the deploy.

Plan contradictions

Three places where this branch's research contradicts docs/plans/2026-08-13-s17-phase6-virtual-remote-repositories.md. Recorded here rather than in the plan, which is a historical document.

1. Step 23's Files list is incomplete. It names internal/managementapi/update.go, api/openapi/v1.yaml, and api/bruno/management-api/ only. Its own Scope sentence requires a dispatch to the per-format update stores, and those stores take a caller's transaction, so the step necessarily also adds internal/datastore/remote_repository_updaters.go and touches internal/managementapi/handler.go and cmd/artifact-registry/wire_management.go, the two wiring files every prior handler step in this plan named. New files: internal/datastore/remote_repository_updaters.go, internal/datastore/remote_repository_updaters_test.go, internal/datastore/remote_repository_updaters_integration_test.go, internal/managementapi/update_settings_test.go, internal/managementapi/fake_remote_updaters_test.go, cmd/artifact-registry/wire_management_remote_updater_stubs_test.go, and api/bruno/management-api/repositories/update-remote-settings.bru.

2. The credential actor could not be filled from the seam the plan describes. Step 3 states that "the per-format create and update store inputs carry an actor field - Steps 4-7 declare it from day one, Step 8 adds npm's ... and the handler steps fill it from this seam's resolved actor." The merged inputs declare no such field, so there was nothing to fill. This branch adds it, which is why the diff reaches into three files Steps 5, 7, and 8 own. Without it, every credential write from this route would be audited with an empty actor, which is the actor slice of AC 79.

3. The LOC estimate is off by roughly 7x. The plan budgets ~220 / ~550 / ~770 (production / test / total) for Step 23. Measured: 1526 / 3166 / 5008 added lines, 6.9x on production and 5.8x on tests. The three composers the Files list omitted are most of the gap.

The Status table row for Step 23 is deliberately empty. Guardrail 4 asks the opening MR to fill it in its own change set. This step is one branch of a wave whose branches all edit the same table, so a single writer fills every row in one follow-up commit rather than having each branch collide on it.

Reading the commit series

Three of this branch's rebases are worth reading. First onto 5e16350cd, the parent's rewritten tip, when chore(datastore): add the maven remote row-lock... (!1585 - merged) • Hayley Swimelar • 19.4 force-pushed its review round. Then onto main when that MR squash-merged, deleting the branch this one targeted and orphaning the base. Then the review round 1 rebase, onto a main that had absorbed fix(datastore): stop nullable binds from wrappi... (!1710 - merged) • João Pereira • 19.4 and rewritten the audit sink: the conflicts were in wire_management.go, remote_audit_sink.go, and observability.md, and every one resolved to main's side, because main had generalized this branch's own Audit: wiring into a parameter and closed the level-suppression gap this branch had only described. It also staled a statement pin the same way the first rebase did, and left the npm twin passing on a substring. Both are fixed at the head of the series. Other rebases replayed the series onto a moving main, resolving what it had absorbed, and were clean. The first was textually clean and still broke two things, both fixed in place. logKeyRepositoryID ended up declared twice, here in update.go and in the parent's new bulk_container_worker.go, in different files so no conflict marker appeared. This branch's declaration went and update.go reads the parent's, which puts one spelling of repository_id in the package at the cost of a constant no longer sitting beside every emission site the way docs/dev/logging.md asks. The alternative was two declarations of one wire key, which is the drift that rule exists to prevent. And the parent replaced the maven update's shared pg.NULL with pg.CAST(pg.NULL).AS_TEXT(), which renders NULL::text, staling a statement pin this branch had added. The pin now expects the parent's render.

Two statement pins are red mid-series, and the ranges are different. The maven fix could only land in the later of its two candidate commits, so the eight commits from feat(managementapi): serve the remote settings PATCH through docs(remote): name the obligations the credential arm inherits fail TestUpdateMavenRemoteRepositoryStmt. The container fix is newer than any of them, because main converted container's clear arm only when fix(datastore): stop nullable binds from wrappi... (!1710 - merged) • João Pereira • 19.4 merged, so TestUpdateContainerRemoteRepositoryStmt fails from that same feat(managementapi) commit through style: drop the semicolons from this round's prose, twenty-four commits, and goes green at fix(datastore): pin the cast the credential clear arms now render, the first of round 1's commits.

Both are test-only, both were measured per commit rather than reasoned about, and every one of those commits builds. The test-first authorship commit ahead of them is red across the suite by design. The final tree is green on go build ./..., go test ./..., and go test -tags=integration over internal/managementapi, internal/datastore, internal/remote, and cmd/artifact-registry.

Testing

• go test ./internal/managementapi/ ./internal/datastore/ ./internal/remote/... ./cmd/artifact-registry/
• go test -tags=integration ./internal/managementapi/ ./internal/datastore/ ./internal/remote/... ./cmd/artifact-registry/ (needs max_locks_per_transaction=4096, because the stock local value fails ~95 tests with SQLSTATE 53200, pre-existing Local testcontainers Postgres needs max_locks_p... (#657) • Unassigned)
• golangci-lint run --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false ./internal/managementapi/ ./internal/datastore/ ./cmd/artifact-registry/ (zero findings on this branch's own files. The package totals are the pre-existing never-linted baseline CI lint cannot see.)
• Contract: internal/managementapi/contract_settings_test.go validates every new request and response body against api/openapi/v1.yaml.
• .claude/skills/run-artifact-registry/driver.sh smoke: 24 passed, 0 failed.

Two notes for a reviewer working from the spec or from guardrail 7:

• The 400 table at docs/specs/S17-rest-management-api.md:1345 lists "a one-sided or over-long credential value, an out-of-range cache window" and names neither the empty-value nor the control-character class this step rejects. Both are additions the spec does not yet carry, documented in api/openapi/v1.yaml and the Bruno request instead of amended into the spec here. Separately, spec:856 illustrates the request union's shared field as "a bare url", which this step's union deliberately omits, the same Step 34 scoping the ADR-009 note covers.

• internal/managementapi/settings_internal_test.go keeps eleven //nolint:thelper directives that suppress nothing, and the branch adds 320 lines to that file and deletes 11. Guardrail 7 asks a file you touch to come out consistent with the rule, so keeping them is a deliberate deviation: the directives are pre-existing, the trim is Trim the //nolint:thelper tokens that suppress ... (#682) • Pawel Rozlach • 19.4, and sibling step branches in this plan edit the same file. The three directives this branch itself introduced are gone, and the tests it adds carry none.

  Why they suppress nothing is worth recording for that issue, because it is structural rather than incidental. Measured on the pinned golangci-lint 2.12.2: thelper reports zero findings in the file with all eleven present and zero with all eleven stripped. A control file in the same package pins what it does flag, which is helper functions, declared or assigned to a variable: a missing t.Helper(), a *testing.T that is not first, and a helper whose parameter is named tt. It does not flag the parameter of a function literal passed to t.Run, with or without tt.Helper(). The five live thelper findings in these packages are all the helper shape, none the subtest shape. So docs/dev/go-testing.md's tt rule, which is about subtest closures, never collides with thelper at all, and the guide says as much itself: "no linter enforces it today". Every one of the eleven sits on a TestXxx function or on a helper already named t, which thelper does not flag under any circumstance. The trim is therefore removing directives whose cited conflict does not exist, and the directive is still owed wherever a helper takes tt.

Manually, against a local instance:

• PATCH a remote repository's cache_validity_hours alone. url and has_credentials are unchanged in the 200 body.
• PATCH credentials: null. has_credentials flips to false with no credentials_cleared marker.
• PATCH settings.url. The 400 reads settings has unknown field "url".

Context for LLM agents

Design rationale

Why the url rejection is an unknown-field 400 rather than a 422. The alternative considered was declaring url on the update decode target as readOnly, which would render a 422. Rejected: it publishes a field the schema does not have, and the ordering would then put the url verdict after readOnly.reject(), so {"url": "not-a-url"} would answer an invalid-URL 400 instead of the undefined-field 400. The rejection has to be byte-identical to what settingsDecodeFailure renders for a genuinely unknown field, and it has to land during strictDecodeSettings, before any value validation. Both properties are pinned by tests in settings_internal_test.go. The admission is gated per mode, because decodeRemoteSettingsCreate delegates to decodeRemoteSettingsUpdate and requires url.

Why the two writes are two transactions. The repositories-row updater (datastore.RepositoryStore.Update) takes no transaction handle, so nothing can compose the metadata write with the settings write. Every client-shape rejection runs before either, so no malformed body half-applies. What remains is a settings write failing after the metadata write committed: the response is 404 or 500, the metadata half is durable, and no update audit event is recorded, because the event fires only once both halves are in. The residue is a description or visibility change, visible on the next read, and re-applying the same PATCH converges. An unaudited committed change is the accepted cost of the missing shared transaction. It is named in updateRepositoryHandler's doc comment and pinned by TestUpdateHandler_Settings_MetadataHalfSurvivesSettingsFailure. Composing both writes in one transaction would mean giving RepositoryStore.Update a transaction-taking variant, a change to a store three other routes consume.

Why the composer owns the rollback. The rollback is not what enables the 4xx. Every refusal writeSettingsStoreError renders as a 4xx returns either before any statement runs or from a zero-row SELECT, neither of which aborts the transaction. The rollback is there for atomicity and for releasing the row lock and the pooled connection, which is what the integration test proves.

What the rollback does not reach: the credential audit record. The stores emit remote.CredentialWriteEvent as soon as their UPDATE succeeds, so a failure after that point rolls the columns back with the record already out. Two such paths are reachable here: the gated details read answering ErrNotFound when a parent tombstone commits after the locked read, and Commit itself failing on cancellation. For a clear that is the unsafe direction, and NpmRemoteRepositoryStore.UpdateNpmRemoteRepository names it: a reader auditing a revocation is told it happened while the row still holds the credential. These composers are the first production callers to make it reachable. Converging the emission onto a tx-threaded seam is refactor: converge npm credential writes onto t... (#487) • Unassigned, not this step, because it changes the store contract three other write paths share. The composers' own doc bullet now says so.

Why the write arm's ErrNotFound is a 404 where the read arm's is a 500. The locked read folds its parent-active gate into one statement, so its ErrNotFound covers a live parent whose child settings row is absent as well as a genuine mid-request delete. remoteRowSettings sends that first shape to a logged 500, calling it inconsistent data. writeSettingsStoreError answers 404 for both: a settings PATCH against a row with no settings row cannot be served whatever the cause. The cost is that the write arm records no signal of the inconsistency, and routing it through remoteRowVanished the way the read arm does is the alternative, at one extra FindByName on the error path. Named in the function's doc rather than changed here, because the choice is a reviewer's.

Non-goals

• AC 76 (URL-change credential clear and credentials_cleared), AC 77 (health reset), AC 78 (eviction enqueue and the 503): Step 34's. The stores already implement that behavior internally, and this step never sends a URL change, so settingsResponse leaves CredentialsCleared false.
• The update event's previous-and-new-URL payload: Step 34's. This step emits the non-URL form.
• A second credential audit event from the handler: the store owns that emission. The handler's whole part is putting the actor on the store input.
• Authorization: covered under Governing ADRs. No management-API path calls authz, and this step does not change that.
• Encrypted credential columns: covered under Governing ADRs. Pre-existing interim, gitlab-org/ops/artifact-registry#68+s.
• Auditing a hosted or virtual repository write: the emission is gated on the remote kind, so a visibility change on a hosted row goes through this handler and is recorded by nothing. S17's acceptance criterion 79 asks for the event on remote repositories only, so the gate matches the spec as written. Widening it is Audit repository writes on hosted and virtual r... (#718) • Unassigned.
• Evicting the container upstream token cache: a credential replace or clear has to evict it, per oci.RemoteTokenCache.EvictRemote, and no caller does on either arm. Harmless while the container read path reaches no token cache, and owed by whichever change wires one: Evict the container upstream token cache on a c... (#717 - closed) • Unassigned.

Related to #314