chore: close the npm remote mount review follow-ups

What

Closes the eleven review discussions from !1821 (merged), tracked in #796 (closed), and #785 (closed).

Twelve commits, in six groups, kept apart so each fix is reviewable against the change that needed it:

Commits What they close
db52872c, d746d8a8, 24069b19 #796 (closed)
24fcb0f1, 4f473465, 0b0b8fdb the review of those three
48cde6aa the second review round (six findings)
ddc526b7, 642c664c the third review round (see below)
e6601ec3, 5f0ed887 the fourth review round (two claims of scope)
0af3e821 the rebase onto the maven remote driver work (see below)

The branch has been rebased three times, so every id above is from the current tip. The first rebase, after the third round, reworded 24fcb0f1 and 48cde6aa from fix(skills): to the chore(skills): the prefix table in AGENTS.md mandates for .claude/skills/**: messages only, with the tree at the tip byte-identical to what it was before the reword.

The second rebase put the branch back on main after refactor(remote): one standalone read state machine for both callers landed. That commit rewrote the same writeRemoteProxyError doc paragraph this branch rewrites, which is where its one conflict was. Both sides were correcting the stale "the tarball proxy will do the same when it lands" sentence, so the resolution keeps this branch's more precise wording, that the tarball route reaches writeRemoteProxyError on the upstream-refusal arm alone, and renames the function it cites from serveOnFetchError to the RemoteTarballHandler.writeReadFailure that refactor replaced it with.

The third rebase followed chore(skills): drive the maven remote read arms from the run driver, which added a ### Driving the Maven remote proxy section to the same SKILL.md region this branch rewrites, and smoke checks to the same cmd_smoke. driver.sh merged clean. In SKILL.md both sides had rewritten the paragraph describing $D smoke, so the resolution keeps this branch's version, which is the one that documents the recorded port mode, and keeps the new Maven section after it.

Combining the two then falsified two of this branch's own sentences, which is what 0af3e821 fixes rather than leaving for a reviewer to catch: the check count was measured at 48 before the Maven arms existed and is now 71, and the npm checks answered by npmjs.org were described as the only ones in the run leaving loopback, which the two Maven Central checks also do. 71 is the 73 check sites in driver.sh, all inside cmd_smoke, less the two in whichever Maven arm is not taken; it is still invariant across all four combinations, because every other branch there picks an expected value rather than whether to assert.

No other content changed in either rebase.

No production behavior changes. The only non-test Go edits are comments.

The recipe defect (chore(skills))

cmd_smoke derived the npm remote metadata expectation from the current shell's AR_ADDRESS. The documented fixed-port flow sets AR_ADDRESS inline on up alone and then presents a plain $D smoke as the verification, so smoke re-derived the ephemeral answer and failed two checks expected 503, got 500 against a healthy fixed-port server.

up now records the npm.public_registry_url it booted with in ports.env, and every subcommand reads that instead of re-deriving one. Also in that commit: the re-seed hint is scoped to one slug's row instead of repointing every remote npm repository on the database; an empty seed read-back is no longer reported as a no-op re-seed; the checks that depend on a live npmjs.org say so in their descriptions; the usage() Env list is completed; and the ## Gotchas status claim gains the fixed-plus-unresolvable case it omitted.

What the review of the first three commits found

Worth reading, because two findings were in the spec text this MR adds.

The coalescing row contradicted the code it described. The row added to S15's Error Cases table said a coalesced follower carries Retry-After: 1, unqualified. S15 governs all three npm remote reads and the tarball route does not do that — remoteTarballBackpressure answers ahead of the shared mapping with the sweep interval, and this branch's own boot test pins 1 for dist-tags and 30 for the tarball fifteen lines apart. The table now carries a row per route, and the open question goes to #825 (closed) with the argument on both sides.

The replacement rule was under-broad. Splitting the paragraph left the cap breach and the unusable-status 503 under no rule, and the Go comment described two arms where remoteReadPolicy.retryAfterSeconds enumerates three. The old sentence was over-broad; that was the mirror-image error.

A stated reason was false. "Which arrives with no interval of its own" is wrong about upstream 503s — they routinely send Retry-After, and the reason this service cannot forward it is the closed header allowlist. Now cited to #687.

The new health assertions could not fail. They targeted RemoteTarballHandlerDeps.Health, which the fixture wired to an in-memory fake, so a request-path health write would have landed nowhere and the assertions would still have passed. The fixture now passes the production store.

AR_API_TIMEOUT was documented as bounding $D api. cmd_api's curl carries no -m; only smoke's probes do.

The third review round (ddc526b7, 642c664c)

The "pass green" claim was inverted. Both the smoke section of SKILL.md and the cmd_smoke comment it was paraphrased from said that branching on NPM_UPSTREAM_URL instead of the seeded row would pass green. Whenever the two disagree the run goes red on a healthy server, in both directions. The comment is pre-existing (3cde426f, on main) and is corrected at its source rather than only in the copy.

A cross-format claim widened with the paragraph under it. "Maven and Container answer 1 on the same condition" sat two sentences after this MR extended S15's coalescing set with ErrFillAbandoned, so it read as all three formats agreeing. maven.mapTransportFailure and oci.mapRemoteFetchFailure match the two S13 sentinels only and answer 500 on the third. The sentence is scoped and the divergence stated; the table row above it named Maven alone and now names Container too.

The fourth review round (e6601ec3, 5f0ed887)

Both findings are the same defect in two documents: a claim written wider than the thing it describes.

AR_API_TIMEOUT was scoped to "each status and header probe smoke issues". The third round narrowed this claim off $D api; it was still wider than the code. Only api_code and api_header read the variable. anon_code, api_body and the curl calls cmd_smoke makes inline pass no timeout at all, and the readiness, liveness and metrics checks hardcode -m 5. All three sites that carry the claim now name the two helpers and list the exemptions: the SKILL.md paragraph, the usage() Env entry, and the comment on API_TIMEOUT itself. That comment said "bounds one api_code call" and left api_header out; 3cde426f added the comment and both -m flags in one commit, so it has been wrong since the line was written.

S15's Retry-After paragraph said "every 503 in the table above". The two coalescing rows this MR adds put an exception in the table, and the sentence above them was not re-read afterwards. It now excepts the coalescing row for the packument and dist-tags routes, which answers 1. The tarball coalescing row answers the sweep interval, so it stays under the rule rather than under the exception, and the scope names one row rather than both.

The rebase

!1750 (merged) merged while this was open, so the "conflict risk" section of the original description is obsolete. This branch is rebased onto it. git merged it without a conflict, which was the hazard rather than the relief: !1750 (merged) gave seed_npm_remote repo and upstream parameters while leaving the read-back hardcoded on npm-remote-repo and the NPM_UPSTREAM_URL global, so seed-npm-remote demo npm-remote <url> seeded one repository and reported on another. That bug is on main and predates this branch; it is fixed here because this branch rewrites the same block. Main had also moved the smoke count to 48 while leaving one SKILL.md site at 43 — the site this MR rewrites, which would have re-authored the stale number.

Verification

Run, not reasoned about:

  • driver.sh smoke: 48 passed, 0 failed, measured on the rebased branch, and 48 again with the shell's AR_ADDRESS deliberately disagreeing with the booted server. Reverting only the predicate reproduces the reported 2 failures on that same healthy server.
  • The scoped UPDATE, pasted verbatim from what seed prints, against a database holding four remotes across three slugs including two under one slug: UPDATE 1, other rows untouched. Fed a slug and an upstream carrying a quote, $(...), a backtick and ; DROP TABLE npm_remote_repositories; --, the same paste created no file, dropped no table, and matched no row. The statement rides psql -v on stdin rather than -c, because psql performs no variable interpolation on a -c string.
  • Both seed paths with two remotes on one slug: each now reports the repository it actually wrote.
  • The health assertion A/B: injected a SetHealthStatus on the degraded serve path; green with the fake, red with the production store. Same method for the retention anchors.
  • The ports.env round trip before and after %q: $HOME:5050 came back expanded before, intact after.
  • The variable/row divergence, constructed on a running fixed-port server: a slug seeded with the unresolvable default and then re-seeded with AR_NPM_UPSTREAM_URL pointed at the public registry keeps the dead row, and its packument, dist-tags and tarball reads all answer 503. Branching on the variable would have expected 200 for all three, which is the red run the corrected wording now describes.
  • driver.sh smoke again after the third round: 48 passed, 0 failed, and 48 again under AR_NPM_UPSTREAM_URL=https://registry.npmjs.org, where the three proxy checks answer 200 from the live registry.
  • The fourth round changes no executable line. driver.sh gains comment and usage() text only; the two Markdown files are prose. bash -n, shellcheck -x and the full pre-commit hook set are clean on both commits, and usage() renders. The exemption list was read off the source rather than recalled: -m "${API_TIMEOUT}" appears at driver.sh:656 and :681 and nowhere else.
  • go test -tags=integration ./internal/format/npm/... and TestMountSlugAnchoredFormats_NpmRemote* green. Integration-tagged golangci-lint with the repeat caps lifted: no findings in any file this MR touches. Default lint 0 issues. bash -n and shellcheck clean.

Testing and docs guardrails

No e2e scenario is added or affected: nothing in the request path changes. No configuration schema or OpenAPI operation is touched, so the configuration reference and the Bruno collection are unaffected.

Closes #796 (closed) Closes #785 (closed)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading