The following discussions from !1821 should be addressed: - [x] @mkhalifa3 started a [discussion](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/1821#note_3716600339): > Smoke derives the port mode from the shell's `AR_ADDRESS`, not the booted server's — the documented fixed-port real-registry flow fails 2 checks red on a healthy server > > `cmd_smoke` computes the metadata expectation from the *current shell's* `AR_ADDRESS`: the default `127.0.0.1:0` takes the ephemeral branch and expects `500`. But the recipe sets `AR_ADDRESS=127.0.0.1:5050` **inline on `up` only** (SKILL.md:247) and then presents `$D smoke` as the follow-up verification (SKILL.md:259). The server is fixed-port, so the packument and dist-tags reads answer `503` (hermetic row) or `200` (public row) — deterministic `expected 500, got 503/200` FAILs against a healthy server, in exactly the flow this MR's recipe rewrite documents. > > This is the safe direction (fails loud, never false-green), but it makes the documented verification path unusable as written and reads as a regression to any agent or reviewer who follows it. Fix options: read the port mode from what `up` persisted (the same config file/`ports.env` other subcommands read), or document repeating `AR_ADDRESS=127.0.0.1:5050 $D smoke`. - [x] @mkhalifa3 started a [discussion](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/1821#note_3716600360): > The re-seed hint prints an unscoped UPDATE — following it repoints every `npm_remote_repositories` row, not the slug's > > The mismatch log printed by `seed_npm_remote` suggests `UPDATE npm_remote_repositories SET url = '...'` with no `WHERE` clause (:726). Each smoke run leaves its slug's row behind, so a dev DB carries several rows; executing the printed statement silently repoints all of them, including a second slug the recipe told the reader to seed with a different upstream — inverting the row-vs-variable care the rest of this change is built around. > > Fix: scope the printed `UPDATE` to the slug's own row (reuse `psql_run -v slug=` with the namespace/repository join and `r.name = 'npm-remote-repo'`). - [x] @mkhalifa3 started a [discussion](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/1821#note_3716600370): > Real-registry smoke: a transient npmjs blip (429/5xx) fails checks named "dispatches to the proxy", which reads as a wiring regression > > The expectations are correct, but in the public-registry branch the check descriptions still blame dispatch while the answer depends on the upstream's live health. A 429 mid-run fails "dispatches to the proxy (not the hosted 404 or the interim 501)". Rename the real-branch descriptions to name the upstream dependence, or document the false-red condition in SKILL.md. - [x] @mkhalifa3 started a [discussion](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/1821#note_3716600383): > Empty seed read-back logs "already existed; it still proxies , not …" — a first-seed failure and a no-op re-seed are indistinguishable > > When the guarded inserts wrote nothing because of an error (no row exists), `seeded` is empty and the mismatch branch claims the repository "already existed". Branch on the empty read-back first and log that the inserts wrote nothing. - [x] @mkhalifa3 started a [discussion](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/1821#note_3716600390): > `AR_API_TIMEOUT` is the first user-facing env knob missing from the `usage()` Env list and SKILL.md > > `API_TIMEOUT="${AR_API_TIMEOUT:-45}"` (:525) — every other `AR_*` knob is documented in `usage()` and/or SKILL.md. One line in each. - [x] @mkhalifa3 started a [discussion](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/1821#note_3716600399): > Pre-existing spec sentence contradicts the coalescing `Retry-After` value the new boot test pins > > `docs/specs/S15-npm-remote.md:848` says every `503 upstream_unavailable` carries a `Retry-After` derived from `health_check.scheduled_interval`. The metadata routes' coalescing failure answers a fixed `1` (`remoteCoalescingRetryAfterSeconds`), now pinned by the boot test's two follower assertions (dist-tags `1` vs tarball `30`). Add the coalescing carve-out to that paragraph, or a spec reader will "correct" one of the two. - [x] @mkhalifa3 started a [discussion](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/1821#note_3716600411): > `npmRemoteBootGet`'s rationale still says the shared-registry test "reads both routes" through one namespace > > The comment at `wire_npm_remote_boot_integration_test.go:562` was not updated with the other count fixes — the shared-registry test now drives all three routes (tarball through `npmRemoteTarballBootGet`). Locally true for the metadata pair only, but it reads stale next to the "all three routes" comments above and below. - [x] @mkhalifa3 started a [discussion](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/1821#note_3716600435): > Spec rewrite introduces a bare positional reference: "except where the sections below say otherwise" > > `docs/specs/S15-npm-remote.md:604` — the repo doc style asks to point at a section by its name. The next two sentences already enumerate the departures with anchors, so name them in the pointer sentence or drop it. - [x] @mkhalifa3 started a [discussion](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/1821#note_3716600456): > File-header bullet overstates the 304 proof: "answered 304 without reading storage at all" > > `remote_tarball_integration_test.go:24` — the tally covers the blob-store surface (OpenBlob/BlobInfo) and proves no *blob-store* read; the 304 arm still runs the cache-row lookup for the address. Tighten the bullet to "without reading the blob store at all" so readers don't infer the cache-row lookup is skipped. - [x] @mkhalifa3 started a [discussion](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/1821#note_3716600466): > "mutates nothing" on the fallback arm is evidenced on `npm_remote_files` only — retention anchors and health columns are unwatched > > `TestRemoteTarballIntegration_UnreachableUpstreamServesTheStaleRow` (:1158) asserts the files row's four fields plus the files count. Two mutation classes can slip past: a retention bump on the degraded serve (the anchors test guards only fresh-hit GET/HEAD), and a request-path health write on `npm_remote_repositories` (sweep-only today, but unpinned). Reuse the fixture's anchors helper before/after the fallback request. - [x] @mkhalifa3 started a [discussion](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/1821#note_3716600477): > `SKILL.md` smoke paragraph: "The metadata reads answer 200 on a fixed port and 500 on an ephemeral one" omits the fixed+unresolvable case > > `SKILL.md:264` — against the unresolvable default on a fixed port the metadata reads answer `503`, not `200`. Qualify the sentence with the upstream condition so "names exact statuses in both modes" holds in both modes. --- 🤖 **Triage note:** This issue has been classified as `type::maintenance`. It is a **follow-up tracking issue** collecting review discussions from !1821, covering tooling correctness fixes (smoke script, seed helper), test coverage improvements, spec/doc clarifications, and comment accuracy. All items are non-functional improvements — no new user-facing features and no regressions being fixed — which fits `type::maintenance`. The following labels have been applied: `type::maintenance`, `Category:Artifact Registry`, `devops::package`, `group::package registry`. Note: the previous `group::container registry` label was incorrect for this project and has been replaced with `group::package registry`. If this classification doesn't look right to you, please update the labels and let the team know. 🙏