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'sAR_ADDRESSdeliberately 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 whatseedprints, 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 ridespsql -von stdin rather than-c, because psql performs no variable interpolation on a-cstring. - Both seed paths with two remotes on one slug: each now reports the repository it actually wrote.
- The health assertion A/B: injected a
SetHealthStatuson the degraded serve path; green with the fake, red with the production store. Same method for the retention anchors. - The
ports.envround trip before and after%q:$HOME:5050came 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_URLpointed at the public registry keeps the dead row, and its packument, dist-tags and tarball reads all answer503. Branching on the variable would have expected200for all three, which is the red run the corrected wording now describes. driver.sh smokeagain after the third round: 48 passed, 0 failed, and 48 again underAR_NPM_UPSTREAM_URL=https://registry.npmjs.org, where the three proxy checks answer200from the live registry.- The fourth round changes no executable line.
driver.shgains comment andusage()text only; the two Markdown files are prose.bash -n,shellcheck -xand the full pre-commit hook set are clean on both commits, andusage()renders. The exemption list was read off the source rather than recalled:-m "${API_TIMEOUT}"appears atdriver.sh:656and:681and nowhere else. go test -tags=integration ./internal/format/npm/...andTestMountSlugAnchoredFormats_NpmRemote*green. Integration-taggedgolangci-lintwith the repeat caps lifted: no findings in any file this MR touches. Default lint 0 issues.bash -nandshellcheckclean.
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)