docs(plans): correct the S04-A plan's stale claims and citations
What
A pre-push validation of the S04-A Step 4 implementation checked the plan's
own claims against the tree, and review then extended the check to the two
steps that reuse Step 4's mechanisms. This MR corrects what both found, from a
dedicated docs(plans) branch, because a step MR does not edit the plan file.
| Claim | Correction |
|---|---|
Spec-amendment dependency cites S16:1258 and S04-b:445 |
Now :1341 and :455, re-derived at this branch's base. The bullet also names the amendment MR |
Step 4 Files: maven_remote_repositories_errors.go:22-23 and :73-80 "assert the row carries the plaintext pair" |
Neither line says that. :22-23 gives the PgError.Detail-hygiene reason without naming what the row holds; :73-80 states the pgErrorCarriesRowData discriminator in terms of Detail, InternalQuery and Where. Neither went stale, so that edit lands in the sidecar .md alone |
Step 4 Tests names wire_connection_test_sources_integration_test.go as a seeding suite the step touches |
It seeds only npm remotes (seedConnectionTestNpmRemote, tmp_plaintext_auth_token) and constructs neither Maven store, so no signature change reaches it |
| Two obligations owned by no step | S04-A's stale-NamespaceKey write contract and S13's health-probe arm both belong to Step 4 — the first step with a production credential writer, and the first where the fail-closed probe arm is reachable at all |
Found in review on this MR:
| Claim | Correction |
|---|---|
The maven_remote_repositories_errors.go bullet read as a no-change declaration for that file |
The file is modified by the step: eb965a43e adds execMavenRemoteRepositoryWrite, resolving the FK's 23503 to the sentinel ahead of both mappers. The bullet now separates that from the plaintext-pair finding above it |
| The files carrying the two new obligations were named nowhere | Listed: maven_remote_credential_write.go (Create), credential_leftover_plaintext.go (Create), and internal/remote/healthjob.go (Modify) for the refusal booking |
| The no-retry claim rested on an amendment the plan recorded nowhere | A Dependencies bullet now names !2563 (merged), in the shape of the !2542 (merged) bullet, with the merged S04-A passages it rewrites |
| The spec-amendment bullet said "once it merges Step 4 is free to merge" | An ordering already crossed. Step 4 landed on main as eb965a43e (2026-09-11) ahead of !2542 (merged), so for three days the five spec lines contradicted merged code. !2542 (merged) has since merged (656107304, in main 45b52905d, 2026-09-14) and the fifth round below turns that bullet into a landed record |
| Steps 7 and 10 reuse both new mechanisms, but neither Scope list declared the stale-key half | The mechanism is per-table, so each step now names its own fk_<table>_ns_key_id_ns_enc_keys and its own sentinel, and each Tests line gains the FK-name discriminator case |
Step 7's Files list carried the same errors.go rationale the Step 4 bullet corrects |
Container's :22-23 gives only the hygiene reason too. The plaintext fact stays with the sidecar :29 and container_remote_credentials.go:145, and the bullet now names what the file does gain: Container's stale-key resolution |
Step 10's npm errors.go citations predate Step 9's merge |
Re-derived at main d9929270c: one code site at :68, the sidecar claim at :22, and sidecar pointers at :31, :94 and :98. :33-35 is the pgSQLStateCheckViolation constant now and names no column |
Why the two-obligations row matters
Both were real gaps in the shipped step, not bookkeeping. The health-probe
arm meant a Maven remote that cannot serve kept its previous, possibly
healthy, verdict for as long as the encryption block was absent, because
the credential read sits in ProbeTarget and the sweep logged that error and
skipped. The stale-key contract meant the first production credential writer
had no handling for the ns_key_id FK's 23503 at all: the classifier did not
recognise it. Step 4's implementation now covers both; this MR is the plan
catching up.
The retry is not part of what Step 4 ships, and an earlier revision of
this MR said it was. writeMavenCredentialUnit warn-logs the rejection and
returns ErrMavenRemoteStaleNamespaceKey
(internal/datastore/maven_remote_credential_write.go:29-32), and no caller
retries either: the rejection has already aborted the write's transaction,
and the key cache would serve the retired id again
(internal/datastore/maven_remote_repositories_errors.md:37-79). The owner
is the S17 management write path, which is the owner the spec amendment
!2563 (merged)
names. Corrected in 351f03c0.
Found in the second review round
The branch was rebased onto main 7606fdb59 in 8fda6104, because
c310c9059 landed a parallel 1,820-line amendment to the same file (the
5a/5b, 8a/8b and 11a/11b splits) while this branch sat on ccd1fe9cb. The
merge was clean either way; the point of the rebase is that the 2,768-line
combined file is what ships, so it is the file this MR is reviewed against.
Three regions come through as main's, because this branch edits none of them:
the #513 gate record, the drop steps' file inventories, and Step 4's
nothing touches tmp_plaintext_* sentence.
| Claim | Correction |
|---|---|
| The ADR-007 dependency bullet said the ADR "declares none of the key-wrap machinery" and that the columns, FK and index "all diverge" from it | The amendment landed. The sync 8fa841cf1 (2026-09-06) is an ancestor of main 7606fdb59, and ADR-007 carries wrapped_dek, ns_key_id, ns_key_version, the composite FK, the (namespace_id, ns_key_id) index and namespace_encryption_keys. Steps 2, 6 and 9 have merged, so the gate is met. Step 2's Scope carried the same present-tense claim and moved with it |
Step 10's Scope did not tell npm's write path to null tmp_plaintext_auth_token, so a replaced token stayed readable to a pod on the previous image |
The Scope states the nulling, naming npm_remote_repositories_update.go:331 and :341 and Maven's mavenRemoteCredentialUnitAssignments (:472-486) as the shape. Tests pins it on both arms. Two passages that rested on the missing clause moved with it: the Approach split rationale's "neither entry spells the nulling out", and Step 11's preamble |
assertMavenHasCredentialsParity was cited as ":491 has seven uses", mixing this plan's original basis with main |
:507 at main 7606fdb59, where the seven uses are. The bullet names the symbol and says :491 was the same helper at the old basis, with five call sites |
Step 10's Tests named repositories_integration_test.go:2314 |
:2316 at main 7606fdb59. Step 4's merge added two lines above it; :2314 is the CacheValidityHours assertion |
| The !2563 (merged) bullet said "two merged S04-A lines prescribe the caller retry" | Four: :381, :1450, :1759 and the error-table row at :2180, at main d9929270c. :381 and :2180 stay marked as the two the plan leans on, and the bullet records !2563 (merged)'s own tally of five passages changed for four original claims |
Step 3 justified a doc-comment rewrite with "A Claim Time Will Falsify Names What It Is True At", a name that resolves nowhere under docs/ or in AGENTS.md |
The rule does have a home under another heading: docs/dev/writing-discipline.md's ## A tracking issue describes the world its reader will find, :223-226. The bullet points at that section instead |
Found in the third review round
!2625 (merged)
merged Step 10 part 1/3 (0c1623a8d) while this branch was open, and it moved
most of what the drop steps cite: it folded both formats' report bodies onto a
shared credential_leftover_plaintext.go, registered npm's report arm beside
Maven's, divided the boot read window per registered format, added a fourth
report message, and rebuilt wire_remote_test.go. Most of the findings below
are that move landing on passages written before it. Citations re-derived
against it name main e3878692d rather than the amendment basis, and say so
where they do.
| Claim | Correction |
|---|---|
Step 5b: wire.go's registration is "the only Maven-specific site", wire_remote.go's report block "holds no Maven-specific code at all", and 5b should "remove the whole registration" |
Maven's arm is two entries now: the maven: mavenCredentials field at wire.go:498 and {format: healthSweepSourceMaven, reader: readers.maven} at wire_remote.go:666. 5b removes both and keeps the call, which is what still boots npm's line, the only input Step 11b's clean-log gate reads. Removing the field alone leaves readers.maven nil and panics at :676 |
Step 8 contradicted itself: Files says do not narrow column-encryption.md :137 "to npm alone", Shared seams said 8a does |
Shared seams rewritten to match Files. 8a rewrites the clause for npm's single column or removes it for Step 10 to re-add, never narrows it with the pair wording standing. The runbook sentence is handled differently and the entry now says why |
| The un-bind halves told the runbook to scope the settings-API clear away from moved formats and name operator SQL as the replacement | Inverted. After 5a the API clear still blanks the encrypted unit, which is the copy the moved reader serves upstream; the SQL erases only the interim pair. A responder told the API clear is no longer Maven's remedy leaves the disclosed credential's live copy set. The clear stays prescribed for every format and the SQL becomes the additional step a moved format owes. Seven passages carried the wrong reading |
| Step 5b's shared-machinery bullets presumed the pre-!2625 tree | No //nolint:dupl survives in either file, so that cleanup has no target and is now a check that Step 10's remaining parts have not reintroduced one. npm's arm is registered, so credential_leftover_plaintext.go survives Maven's drop rather than being Modify-or-Delete, and the directive leaves the Approach's machinery list |
Step 5b's report-suite inventory: "nine subtests" over two functions, "eight hard-code "maven"", none table-parameterized over format |
Thirteen leftover-plaintext test functions over :527-1134, eleven "maven" literals in them (a twelfth at :341 is the health-sweep table's). Two things the old map had no room for get their own bullets: the two-format assertions a single arm falsifies rather than retargets, and the maven nil-reader table case at :712 |
Step 5a and 5b name "the 5s budget" as Maven's boot read, and 5b justifies lock_timeout = 10s with "5s is exactly the boot report's own read budget" |
leftoverPlaintextReadTimeout is the whole window the registered formats divide, so Maven's read is bounded by 2.5s with npm registered. The lock_timeout reason is recast onto the window ceiling, which is what the migration actually competes with and does not move as formats are added |
| The report's message set is three, and the Acceptance's confusable causes are enumerated against it | Four. leftoverPlaintextBudgetSpentMsg replaces the read-failure line only on a cut share (cutShort at :733); a deadline on a whole share still emits the ordinary message. The Acceptance keeps three confusable causes and names the cut-share expiry as the one that identifies itself |
Step 5a's maven_remote_credentials.go inventory lists the nulling claims as the bind struct's two plus :575 |
A fourth site: InterimPlaintextWritePathMoved's doc comment says it "answers true because mavenCredentialUnitBindsFor binds the interim pair NULL on both of its arms", the exact mechanism 5a removes while the answer stays true. Nothing pins the literal, so no test reddens. It is also the second half of the leftoverPlaintextReader interface, so it joins 5b's removal list |
Step 10's Scope and Files name neither insertNpmRemoteRepositoryStmt nor InterimPlaintextWritePathMoved, though Step 11a already rests "one statement rather than two" on both |
Both added. The insert stops naming tmp_plaintext_auth_token (npm_remote_repositories.go:1109) and the flag flips with that move, pinned by TestNpmRemoteRepositoryStore_WritePathMovedMatchesTheWritePath. Until the flip npm's boot line stays count-only, which is what 11a's window remedy needs |
No remaining step owns Container's CountRowsForVersion term, and the merged comment points at #417 (closed) as its owner |
#417 (closed) is the work item this plan closes, so the pointer named no future owner, and retire-key-version's pre-flight would report 0 for container rows whose FK then blocks the key delete. Step 7 takes the term and retires the clause, covering both places the comment names, and Step 3 now says so |
Step 5a's credential_table.md bullet describes a :85-93 passage carrying two claims |
The passage moved to ## Why the seeded rows carry the interim plaintext columns (:208-232) and carries three. The added one is a sentence about npm ("its assignment builder nulls the column ... as Maven's does"), so an editor scanning for Maven's own claims skips it. Cited by heading now, since Step 10's remaining parts move it again |
Step 5b: deleting LeftoverPlaintextSampleLimit "leaves a stale comment with nothing failing", unlike its two code-referenced siblings |
Inverted. The constant is code-referenced too, at credential_leftover_plaintext.go:77, so the build catches its deletion. The hand sweep is owed for the comment at wire_remote.go:565, and only in the retire-the-last-arm flow. The sibling citations were stale as well: :625 and :781, not :583 and :648 |
migrations/npm_remote_encrypted_credentials.md :110-112 is cited for the Maven/Container interim CHECK comparison |
:115-117; :110-112 now reads the ADR-007 interim-column paragraph. Six sites carried the stale range, and Step 5a's neighbouring :112 is now :117-118. The Approach gains the find-by-wording instruction the step entries already use |
The second-basis paragraph calls main 85530efe0 "the merge base of the merge request that split Steps 5, 8 and 11" |
It is not. That branch forked from e4d8b5250, and 85530efe0 sits twenty commits behind it. The citations hold there, so the anchor works and nothing derived from it moves; only the label was wrong, and it is now described as the head those citations were derived against |
Found in the fourth review round
Twenty findings, all on the plan's own text. Three groups: the drop gate's reading model, the two deviations the open Step 10 part 3/3 !2627 (merged) declares, and citations or counts that went stale.
| Claim | Correction |
|---|---|
| Step 5b's Acceptance keyed the drop to the reading the engineer shipping 5a takes and records in !2629 (merged) | Reversed. !2629 (merged) and !2630 (merged) both declare that reading cannot serve the drop: a gate reading has to come from a boot postdating that environment's rollout of 5a, so the two cannot be the same read. The reading is the merging engineer's, per environment, with the operator SQL as its remedy. !2633 (closed) carries the same amendment against the pre-split text, so it retargets none of this wording and must be retargeted or folded in before !2630 (merged) leaves Draft. Step 5a's Scope and Acceptance both said 5b's gate reading is taken off Step 4's release and that 5a cannot affect it; the reverse holds, and both moved |
| The boot-line gate has no line to read in production, and no substitute was recorded | !2629 (merged) records production on 2.38.3, below the v2.43.0 release (eb965a43e) that first emits the line. The plan's fallback was written for a failed read, not a never-emitted one. Step 5a's Depends on and Step 5b's Acceptance now carry both halves: a direct count against mavenRemoteLeftoverPlaintextPredicate's two columns where an environment predates the release, and production deploying at or after v2.43.0 before the drop half merges |
| Open Question 3 said the ReplicaSet-series question is why !2630 (merged) stays Draft | !2630 (merged) declares its per-environment readings table, with !2633 (closed) merged first, as the Draft exit. Both gates are named now, and the ReplicaSet answer is owed before 5b's residual check |
| Open Question 2 carried no live state, though its deadline ("before Step 5a opens") has passed | Question 1's live-state sentence added: !2629 (merged) is open, so record the route arranged or that none was and Step 5a's fallback is engaged |
| Step 10's Files said npm's resolution struct becomes "ciphertext plus key metadata" | !2627 (merged) declares the opposite. The credential leaves the resolution entirely and a SQL-computed HasCredentials from npmRemoteHasCredentialsProjection replaces it, with the sealed unit read through Credentials. The resolver builds that value for every kind=2 request, so ciphertext there puts credential material on the hot path to answer a boolean |
Step 10's Files put the fail-closed probe arm in internal/format/npm/remote_health.go, and named no internal/virtual change |
Both from !2627 (merged). The arm landed in npmremote/health_sweep.go, at HealthTargetResolver.ProbeTarget. The internal/virtual arm is behavior-changing and was in no list: probeErrorIsPositional admits remote.ErrCredentialsRefused and the position fails with dataError set. resolve.go and resolve_errors.md join Files, the arm joins Tests, and the "No new scenario" claim now records that npm's credential-never-served row is amended rather than only confirmed |
| The plan closes #417 (closed), and said so twice | !2627 (merged) records "Residual owner: #417 (closed)" for two security-relevant items it does not close: npm's non-atomic URL/credential pairing and the direct read's refusal status. A Dependencies bullet names both and makes the closure a gated action: re-home them onto a named item, or keep #417 (closed) open and say in the closing note that the plan closed its own scope |
11b retired key-rotation.md's operator-SQL supplement unconditionally |
Only the un-bind halves are ordered. Nothing orders 5b, 8b and 11b, and 8b carries its own spec-amendment gate, so 11b landing first deletes the remedy for a format whose interim columns still stand. Conditional at all three sites that carried it: 11b always removes npm's entry, and retires the supplement only once 5b and 8b have landed |
| SQL clears of the interim pair owed no record, while the plan calls the audit trail "the only place a revocation can be reconstructed later" | A SQL clear books nothing. Step 5a's runbook bullet now asks the responder to record the ids cleared, and says the trail cannot show a SQL clear |
Step 3's Shared seams said it alone edits namespace_encryption_keys.go |
Step 7's Files list the same file, and Step 3's own bullet names Step 7 as the second writer. Step 7 is named as the second editor; the no-race conclusion holds on Step 7's Depends on edge instead of on exclusivity |
Step 8's preamble called findContainerRemoteCredentialsStmt and containerRemoteHasCredentialsProjection the read half 8a inherits as the declared read deviation |
Step 7 repoints both onto the encrypted unit. The inherited half is Container's leftover count and page statements, the pair Step 7 adds and 8b removes |
Step 7's Files named no query_names.go, though it creates Container's leftover statements through the shared helper |
Added, with Container's two names. Step 4's and part 1/3's adds both list the file, and both drop halves list it for the removal |
Step 7's column-encryption.md bullet edited only the moved-path clause |
:138 added as an unconditional second edit: the boot-line enumeration reads "for Maven and for npm" and gains Container once this step registers its line. A drop half landing first would rewrite that prose without Container |
| Step 11a's lead told its implementer both shared sentences "go whole" | The next sentence correctly keeps the runbook's API-clear prescription. The lead now names only the moved-path nulling clause |
The Approach said each un-bind half edits "one sentence" in column-encryption.md |
Step 5a's Files says three lines carry the fact and move together. "The moved-path clause" now, with :134 and :136 named and the step entry given as the list |
Step 5b's Files credited Step 10 part 1/3 with giving LeftoverPlaintextSampleLimit a code reference |
Step 4's merge did: selectLeftoverPlaintextPageStmt already ended LIMIT(LeftoverPlaintextSampleLimit) at eb965a43e, before the amendment basis. Part 1/3 moved it from :29 to :77. The line number and compiler claim stand |
| A later paragraph said the cited assert "sits inside the nine report subtests this step retires" | Both halves are pre-part-1/3, and the same subsection rules the retirement out. Replaced by the re-derived inventory: thirteen functions, retargeted because npm's arm survives. The assert is at :603, not :593 |
:712 was listed among eleven "maven" literals to retarget by string swap |
The third bullet in the same entry calls that table case a table edit. Swapping it duplicates the npm nil-reader case and drops the only nil-maven coverage. Ten now, with :712 named as the eleventh and sent to the table bullet, so "a twelfth at :341" still reads |
npm_remote_credential_table.go:88 was cited for npm's flag pass-in |
:88 is the call; the argument is at :93, matching the Maven citation beside it (maven_remote_credentials.go:347) |
column-encryption.md :161-166 was cited with no head, so it resolves to the amendment basis where that range is a different passage |
Now at main e3878692d, where the divided share is |
Found in the fifth review round
!2542 (merged) merged while this branch was open, three days after Step 4. It rewrote
ten files, including four this plan cites, so it both closed the contradiction
the plan recorded as live and moved the line numbers the plan resolves against.
Citations below are re-derived at main 45b52905d, the head it merged at,
with the merge-base or amendment-basis spelling kept beside them.
| Claim | Correction |
|---|---|
| The backfill-amendment bullet recorded !2542 (merged) as drafted and open, and the contradiction as live | It merged. The bullet is a landed record: all five sites prescribe the discard, at S16:58 and :1349, S04-b:455, and S04-c:132 and :185. The merge-base parenthetical also said ccd1fe9cb, which the rebase moved to 7606fdb59 |
| The per-slice re-confirmation duty !2542 (merged) added was assigned to no step | column-encryption.md:12 and S13:106 make the slice that moves a table re-confirm the no-production-credential reading against the environments its move reaches and record it in its own MR. Steps 7 and 10 are the two moves still open and now carry it in Acceptance, with the Go-side-pass branch if a credential is found. Step 4's move predates the duty and is covered by 5a's and 5b's leftover readings |
| The interim-columns amendment was scoped from pre-!2542 text | Re-derived: S13's rows are :96/:97 and the CHECK :101, plus four clauses the rewrite added at :92, :1197, :1232 and :1236. Those four are generic over all three formats, so 5b, 8b and 11b each take a half. S04-a's second ALTER TABLE is :419. "Distinct from the backfill amendment, which covers different files" was false: !2542 (merged) touched ten, four of them these. Different sentences |
| Step 11b was the one drop with no spec-amendment gate | S15 still calls tmp_plaintext_auth_token npm's live credential store at :147, :151-153, :175, :347, :910 and :1106-1108, and S04-a:465-466 says npm's interim table carries no constraint on it. An npm leg is added to Dependencies and to Step 11b's Depends on |
| Research Findings promises each bullet says whether its correction landed; the fourth one did not | It now leads with the landed status and its gate sentence is past tense. The bullet's own numbers stay at the section's declared basis (ee9e5063c), which the text now states, and the residual the amendment does not reach, the work item's own wording, is named |
| Three passages said the drop-gate reading is taken before 5a, which the Acceptance had already reversed | The reading is taken on 5a's image, per environment, after that environment's rollout of 5a and before 5b's image reaches its pods. Fixed in the gate-discrimination paragraph, in the Approach (which had the un-bind half owning the drop gate's reading), and in Step 5b's image-reading passage (which put the leftover readings in !2629 (merged) rather than !2630 (merged)) |
Context said all three formats store credentials as plaintext, credentialTables is composed empty, and the encryption block buys a deployment nothing |
Step 4 falsified all three for Maven. Rewritten to the state at main c136ecec4: Container and npm still plaintext with unmoved write paths, Maven moved and failing closed without the block, and defaultOperatorCredentialTables registering Maven's table and npm's (reencrypt.go:1207-1212). The adjacent #513 sentence is past tense |
The column-encryption.md citations moved with !2542 (merged) and carried no head |
Each now names the head it was read at, with the post-!2542 spelling beside it. They came from two heads: 85530efe0 for the moved-path clause and the Step 5a runs, e3878692d for Step 7's container gap. At 45b52905d: :140 moved-path, :141 boot report, :143 container gap, :183 rollback window, :192-195 five shapes, :198-199 operator SQL, :163 short-timeout bound, :238 npm's lone column. :198 also stopped being Maven-specific, so that reading no longer travels to 8a or 11a |
| Step 5a's runbook edit prescribed only "the operator SQL over the ids the warn line samples" | The sample is capped at LeftoverPlaintextSampleLimit, which is 10, so above ten rows a responder clears part of the set and leaves the rest readable. Both arms are carried in, matching the Shared seams entry: sampled ids at or below the limit, predicate above it. The predicate arm names no rows, so both passages say to capture the ids with a RETURNING as the clear runs |
Related MRs
- !2542 (merged)
is the spec amendment this plan's Step 4 dependency gated on. It merged
2026-09-14 as
656107304(inmain45b52905d) across ten files, after Step 4 had already landed. Most of the fifth round below is that merge landing on passages written while it was open. - !2625 (merged) is Step 10 part 1/3, merged while this branch was open. It is the tree the third-round corrections above are re-derived against.
- !2563 (merged) names the S17 management write path as the stale-key retry's owner in S04-A, and states that the plan needs no new step for it. This MR's Step 4 bullet matches that owner.
Testing
Documentation only, no code. markdownlint-cli2 passes, and lychee
--include-fragments resolves the new S17 link and its anchor.
Related to #417 (closed)