docs(specs): reconcile the S02 client seams to what shipped

Narendranrequested to merge
narendran/s02-reconcile-client-seams into main

What does this MR do and why?

The S02 Step 1 client MR (gitlab-org/gitlab!249808 (merged)) landed a smaller client than the plan and spec describe, and Steps 2 and 3 (!250031, !250045) added the provisioning and disable/enable methods on top. This reconciles both S02 documents, the S10 passages that assert the same seams, and the two downstream documents that depend on the S02 text — S03-graphql-foundation.md and the unconfigured-base-URL plan — so someone reviewing the stack reads the client that exists rather than the one that was planned.

Every claim below is stated against the merged client (ee/lib/artifact_registry/client.rb) and its ErrorReporter (ee/lib/artifact_registry/error_reporter.rb) on master.

Three injected constructor inputs were removed during Step 1 review, because nothing consumed them:

  • correlation_id: - the client reads Labkit::Correlation::CorrelationId.current_id at request time, in resolve_correlation_id (called from perform_request). No caller ever supplied an injected value.
  • error_callback: - there is no injected callback. The client keeps an internal per-attempt callback (error_callback_options binds report_attempt) and reports terminally through raise_terminal; both delegate to the ErrorReporter collaborator, which owns the log_exception call and redaction.
  • user_agent: - the USER_AGENT constant, byte-identical to what the client has always sent.

Contracts corrected to what the code actually enforces:

  • Base URL validation. validate_base_url! requires an http/https scheme and a host and refuses userinfo, query, fragment, and a path (a bare / aside), raising ArgumentError. It accepts plain http on any host at construction; HTTPS is not gated by host. HTTPS is enforced separately by guard_service_transport!, on the service-credential path and only in production, so a per-user http request works in every environment. The reconcile removes an invented "HTTPS-except-loopback" policy the client never shipped (no IPAddr loopback test, no localhost/127.0.0.0/8/::1 classification, no bracketed-IPv6 handling), and the stored origin, canonical-origin comparison, and IPv6-spelling normalization that also never existed.
  • Containment no longer holds. The merged constructor is stricter than the S01 configuration predicate — it refuses query, fragment, and path where the predicate admits them — so a UI-gated value can pass the route gate yet fail construction and reach S03's service-unavailable row. The deleted "constructor refusals stay a subset of the predicate's" sentence is restored in its corrected form, and S03's error-mapping row that cited it is updated to match.
  • ConfigurationError is a target contract, kept. The shipped client raises ArgumentError for a bad base URL. ConfigurationError is the distinct class the unconfigured-base-URL plan introduces and S03-graphql-foundation.md:201 maps; that plan has no MR yet, so its text is live prescription. The class is retained across S02, S03, and that plan, each noting the shipped client raises ArgumentError until Step 1 lands. (An earlier revision of this MR wrongly deleted it as "never existed".)
  • The uuid guard is two guards. #namespace, #disable_namespace, and #enable_namespace guard their uuid: path segment with guard_segments! (blank and bare dot). #provision_namespace guards its entity_id: body field with guard_uuid!, a full canonical-UUID match (UUID_REGEX).

Reporting corrected against ErrorReporter:

  • Terminal context keys. The report is sliced to ALLOWED_CONTEXT_KEYS = %i[url method status code request_id slug uuid body_class correlation_id] and nil-compacted, so the keys present depend on the outcome: url and method on every outcome; correlation_id when the thread-local resolves; status on any HTTP outcome; request_id and code on the 429/5xx arm, request_id read from the envelope with a fallback to the X-Request-ID response header (raise_error resolves the pair once above its case); body_class on an unexpected-success outcome; slug or uuid as attribution. A #namespace 503 with an envelope carries seven keys inside a correlation block and six outside one, where correlation_id compacts away. The earlier text described a closed four-to-five-key list that wrongly forbade code, uuid, and body_class.
  • cause. The raised UnavailableError carries the Faraday exception as its cause on a transport failure — including the Faraday::ParsingError an unreadable body raises on any status the rescue does not divert — and nil on an unexpected success body, or on a 429 or 5xx whose body parses. The Faraday::ParsingError rescue diverts only 404, 401 and 403, so the two statuses are symmetric here. The cause distinguishes the transport failures from one another, not an unavailability from an authorization failure (separate classes). "Malformed body" is dropped from the distinguishing list, since that path carries no cause.
  • 404-log context. Drawn from five keys (base url, uuid, correlation_id, request_id, status), nil-compacted, resolving the earlier "nothing else" contradiction.

monolith/S01 is left to a follow-up, per review. Because the merged constructor is stricter than the S01 configuration predicate, a value the predicate admits and the constructor refuses renders the entry points and then fails the first AR call, which falsifies S01's twice-stated "an operator fault is not a user-visible state". Rather than widen this MR into S01, the S02 containment paragraph names S01's two rows as still out of step and says to read them as scoped to the predicate's refusals, and S02's ## Follow-ups carries the amendment with what closes it.

No prescription changes for unimplemented steps beyond the seams they restated. The 08-14 plan's Research findings are reconciled to validate_base_url! ahead of its own Step 1, since it restates the same seams this MR corrects elsewhere. Its construction-site bullet named an Organization#artifact_registry_service_client that origin/master does not define; ArtifactRegistry::CachesClient#artifact_registry_client is the only site there, and the bullet now says which site exists where so it holds once gitlab-org/gitlab!250657 (merged) adds the second, as CachesClient#artifact_registry_service_client in that same concern rather than as a method on Organization. That second name is an entry point rather than a construction site: it delegates to #artifact_registry_client with current_user: nil, and !250657 has since merged, so the plan states it against origin/master. There are two Client.new sites on master, the concern and ProvisionNamespaceService#client, and the plan now names both plus a search that matches both.

S02's request_id absolute is qualified here, together with the two dependants in this file list (monolith/S03 and monolith/S10). The same absolute sits in S04-repositories-list.md, S05-repository-detail.md and S14-version-list.md, which this MR does not otherwise touch; those three are knowingly left, on the same scope grounds as the documents below.

Two documents outside this MR's file list are knowingly left. monolith/S01 is one, above. The other is docs/plans/monolith/2026-07-28-repositories-list.md, which still names the gem-era Gitlab::ArtifactRegistry::Client and the ee/gems/ path gem, and whose Status table leaves 8 of 10 steps without an MR, so that text is live prescription rather than a record. The defect predates this branch. Correcting a ten-step plan is the same widening review declined for S01, so it takes the same answer and a separate MR.

References

  • Shipped client: gitlab-org/gitlab!249808 (merged) (Step 1), with !250031 and !250045 stacked on it
  • Files reconciled: docs/specs/monolith/S02-ar-ruby-client.md, docs/specs/monolith/S10-activation-and-deactivation.md, docs/specs/monolith/S03-graphql-foundation.md, docs/plans/monolith/2026-08-06-s02-gitlab-api-namespace-client.md, docs/plans/monolith/2026-08-04-activation-and-deactivation.md, docs/plans/monolith/2026-08-14-unconfigured-ar-base-url.md, docs/plans/monolith/2026-07-02-ar-ruby-client.md

Screenshots or screen recordings

N/A. Documentation only.

How to set up and validate locally

Documentation only, so validation is a consistency read rather than a script.

  1. Confirm no document still names a removed input or a policy the client never shipped (patterns written to survive a line wrap and the hyphenated spelling):

    grep -rnE "injected error.?callback|error-callback seam|injected correlation|HTTPS except.*loopback|IPAddr\.new|raise_unavailable|ERROR_CALLBACK_OPTIONS|narrows .url. to the origin|keys are exactly" \
  docs/specs/monolith/S02-ar-ruby-client.md \
  docs/specs/monolith/S10-activation-and-deactivation.md \
  docs/specs/monolith/S03-graphql-foundation.md \
  docs/plans/monolith/2026-08-06-s02-gitlab-api-namespace-client.md \
  docs/plans/monolith/2026-08-04-activation-and-deactivation.md \
  docs/plans/monolith/2026-08-14-unconfigured-ar-base-url.md

    Expect no output. Multi-word claims can wrap across lines, so also run the whitespace-normalized form, which is what a line-oriented grep misses:

    for f in docs/specs/monolith/S02-ar-ruby-client.md docs/specs/monolith/S10-activation-and-deactivation.md docs/specs/monolith/S03-graphql-foundation.md docs/plans/monolith/2026-08-06-s02-gitlab-api-namespace-client.md docs/plans/monolith/2026-08-04-activation-and-deactivation.md docs/plans/monolith/2026-08-14-unconfigured-ar-base-url.md docs/plans/monolith/2026-07-02-ar-ruby-client.md; do
  tr '\n' ' ' < "$f" | tr -s ' ' | grep -oiE "namespace-exempt|frozen Hash with exactly|hashes coincide here|exact-key assertion on the context|subset of the constructor.s refusals|fires exactly .retry_options\[:max\] \+ 1 times" && echo "  ^ $f"
done

    Expect no output.

  2. Confirm the terminal context claim matches the shipped allowlist:

    grep -nE "ALLOWED_CONTEXT_KEYS|%i\[url method" docs/specs/monolith/S02-ar-ruby-client.md \
  docs/plans/monolith/2026-08-06-s02-gitlab-api-namespace-client.md

    The documented allowlist must equal %i[url method status code request_id slug uuid body_class correlation_id] in ee/lib/artifact_registry/error_reporter.rb. S02 spells the allowlist as the bare %i[...] literal and never names the constant, which is why the pattern matches both.

  3. Lint gates:

    npx markdownlint-cli2 docs/plans/monolith/*.md docs/specs/monolith/*.md
vale docs/specs/monolith/S01-vue-app-shell-and-composition-surfaces.md docs/specs/monolith/S02-ar-ruby-client.md docs/plans/monolith/2026-08-06-s02-gitlab-api-namespace-client.md
lychee --offline --include-fragments '**/*.md'

    markdownlint clean; vale error-free on the changed files. lychee is the third command docs:lint runs, and it is what catches a relative link or an anchor that does not resolve; run it from the repository root.

Database changes

None. Documentation only.

MR acceptance checklist

  • The S01/S02/S03/S10 specs and the 08-04, 08-06, and 08-14 plans are consistent with each other and with the shipped client and its ErrorReporter
  • markdownlint clean and vale error-free on the changed files
  • ConfigurationError retained as the target contract S03 and the 08-14 plan depend on, with the shipped ArgumentError state noted
Edited by Narendran

Merge request reports