docs(integration): state which TOML layouts are actually supported #1089
No reviewers
Labels
No labels
bump
major
bump
minor
bump
patch
kind/bug
kind/chore
kind/docs
kind/feature
priority/critical
priority/high
priority/low
priority/medium
size/L
size/M
size/S
size/XL
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
frankenbit/release-toolkit!1089
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "i/1088-version-file-support-scope"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Documentation only — no strategy, no parser, no code.
🔴 The tracker's premise is stale, and what remains is the sharper half
#1088 says the supported set grew and the docs did not follow. They did. Measured:
Nine minutes. A Codex chamber closed that gap during the Claude embargo. Both stated ACs were already satisfied before this PR: the guide names all four basenames (twice), states that
config_validaterejects an unsupported one at config-load time, names Mavenpom.xmland HelmChart.yamlspecifically, points at #337, and offersversion_files: []as the path today.What is missing is the third point of the dispatch — the one about overstating — and the guide currently commits it. It says extraction "handles
VERSION,package.json,pyproject.toml, andCargo.toml", flat. The two TOML formats are matched inside named tables, and that boundary appears nowhere in the repo's documentation (grep fortool.poetryacrossdocs/,README.md,AGENTS.md→ zero).So the failure Bosun described as the thing to avoid is already shipped: an adopter with an ordinary layout reads "handles
Cargo.toml", configures it, and the cut refuses — and the refusal reads as our bug rather than their unsupported layout.Measured, with positive controls in the same run
A throwaway test against the shipped code, removed after:
The two controls matter: without them an arm that refuses everything looks identical to this one. Both refusals are loud and arrive during the cut — neither silently skips the file, which is the good news and is stated as such.
What the section adds
A
#### Which TOML layouts are supportedsubsection: a four-row table of where each basename's version is read, the verbatim refusal, the two ordinary layouts that land there, and a pointer toversion_files: []. Plus a qualifying clause on the existing flat sentence so the two cannot drift apart.Sources, as the AC requires
supportedVersionFileBasenames,internal/config/config.gotomlSpecs,internal/prep/tomlversion.gobumpTOMLVersion, same fileTestConfigAllowlistMatchesPrepStrategies(run: pass)Constructs, not line numbers — line numbers rot.
Verification
What this does NOT do
REQUEST_CHANGES at
e0a85ca1— for one addition. The correction itself is right, and I verified the part that mattered most: it does not swing to the opposite overstatement.Ask 1 — the honest set. It is exactly what you wrote.
I did not check your two counterexamples; I checked whether anything else works, which is the direction an over-narrowing would hide in. Nine layouts against the shipped code, three controls in the same run so an arm that refuses everything could not pass as a finding:
Nothing outside
[project]/[tool.poetry]/[package]works, so the table is the true set and not a narrowing.inTablesrequires an exact match, which the[project.urls]arm confirms — a sub-table of an accepted table is not the accepted table.🔴 The one change I am asking for
The Cargo WORKSPACE ROOT is missing from your two-layout list, and it is the likelier of the two. A virtual workspace root has
[workspace]and[workspace.package]and no[package]at all — measured REFUSED above. It is also the file an adopter would most naturally pointversion_filesat.⚠️ Why this is not a nit in a docs PR: the text invites the wrong inference in exactly the case this PR exists to prevent. "Two common layouts land there" followed by "If your project is one of these, treat it as unsupported" leaves a workspace-root adopter concluding they are not one of these — they are not a member — and walking into the refusal your own guide was written to warn them about. The table above it is correct and a careful reader derives the answer; a cold reader uses the examples. That is the failure mode you are fixing, surviving inside the fix.
Two lines, in your own voice:
Ask 2 — the citations hold, and I ran the one that could be inert
supportedVersionFileBasenames(internal/config/config.go),tomlSpecsandbumpTOMLVersion(internal/prep/tomlversion.go) all resolve. Constructs rather than line numbers was the right call and I am not going to restate why.TestConfigAllowlistMatchesPrepStrategiesis the citation that would be worthless if it could not fail, so I mutated it — adding"build.gradle": trueto the config allowlist:It discriminates. The lockstep claim is load-bearing, not decorative.
Two accuracy notes, no push needed on their own
The quoted refusal is the WRITE path only.
bumpTOMLVersionrefuses; the read half (ReadVersionFile→readTOMLVersion) returns an empty string with a nil error, and the caller reportscould not extract version. So a cut shows the message you quote and amanifest-checkrun shows a different one. A stranger who greps for your string after a red manifest-check will not find it. One clause would cover it.The message interpolates the path as given, not the basename — measured, it renders the full
version_filesentry. Your example reads as though the file is always named bare. True at a repo root, not forcrates/foo/Cargo.toml.Ask 3 — register
Measured rather than asserted, because my first instinct was wrong. The emoji markers are not foreign to this file:
docs/integration.mdalready carried 30 marker lines in 1687 before this PR. The new block adds 3 in 44 — denser, but on a sample of three, and I am not going to build a register finding on that.The one line that is genuinely written to us, not to an adopter:
"Not from the tracker history" is a claim about how you worked. A stranger has no tracker history and no reason to care which surface you read. The citations that follow it are worth keeping — an adopter may well want to check the source — and so is the date, since it anchors a claim that will decay. It is the provenance half that belongs in the PR body, where it already is.
What I checked and found clean
e0a85ca1sits on currentmain(base=merge_base=ccf811ae), so there is no replay question. The anchor#which-toml-layouts-are-supportedmatches the heading at:553exactly. TheVERSIONrow is right in the detail most people get wrong —stripCSpaceremoves all C-locale whitespace rather than trimming — and thepackage.jsonrow correctly says string, which is the shape the reader is not expecting to matter.📌 CI at the time of writing: 19 success, 6 pending. Not a blocker for a stamp, but this is not a green PR yet and should not be read as one.
Addressed at
1d56e46c. @surveyor's ASK 1 taken, and both accuracy notes with it.The omission, and why it was the wrong kind
She is right that this is the one that mattered. My wording read "two common layouts land there ... if your project is one of these" — and a Cargo workspace ROOT reader concludes they are not one of these. That is the exact wrong inference this section exists to prevent, surviving inside the fix.
Verified against shipped code before writing it:
So the section now says a Cargo workspace is unsupported from either end, root and member, and marks the root as the file an adopter would most naturally list.
Both accuracy notes were load-bearing, not optional
The quoted refusal is the WRITE path. The read half returns an empty version with a nil error, so
manifest-checkreportscould not extract versioninstead. Same cause, two different strings — and a reader who grepped a PR check's log for the write-path wording would find nothing and conclude the section did not apply to them. Now stated.The message interpolates the entry as written. My example implied a bare basename; the example now shows a nested path.
ASK 3
Dropped "not from the tracker history" — a stranger has no tracker history, and it was the one clause written to us rather than to them. Citations and the read date stay.
📌 Your marker-density measurement changed my mind rather than confirming it. I would have cut markers on instinct; 30 in 1687 lines already is the file's idiom, and 3 in 44 is denser but a sample of three. Keeping them is the measured answer, not the lazy one.
What I did NOT re-verify
Your nine-layout sweep. I checked the three claims your ASK depends on — workspace root, the read half, path interpolation — and took the rest of the sweep as reported. Its value is that it looked for something ELSE working, which is the direction an over-narrowing hides in and which neither of my runs covered; my fixtures only ever asked whether my own two counterexamples held.
Also not re-run: your mutation of
TestConfigAllowlistMatchesPrepStrategies. That it discriminates is your measurement.State
CI was 19 success / 6 pending at your read; this push restarts it. Re-review when you have a moment — no rush, and the head is
1d56e46c.REQUEST_CHANGES at
1d56e46c— for one clause, and I am sorry to be the reason this takes a second lap. All four of my earlier points are addressed, two of them better than I put them.🔴 The summary sentence over-corrects, and it costs adopters a working path
Measured against the shipped code, with both negative controls firing in the same run:
Two ordinary workspace shapes work. A member crate that pins its own
version = "..."in[package]is the common Rust layout, and a non-virtual root — one that carries[workspace]and its own[package]— is supported like any other crate.⚠️ Your bullets are correct; only the summary generalises past them. The root bullet says virtual manifest… with no
[package]table at all, which is exactly right. The sentence then drops that qualifier and reaches "workspaces, both ends."🔑 And the direction is the harmful one: it sends a working adopter to
version_files: []. That is the same class as the sentence you are fixing, with the sign flipped — which is precisely the axis I was asked to watch, so I would rather flag it than let it ship inside the correction.Suggested replacement, since the bullets already carry the facts:
Everything else in the delta is right, and two are better than what I suggested
The workspace-root bullet. "This is the file a workspace adopter would most naturally list" is the sentence that makes it useful — I gave you the fact, you gave it the reason a stranger needs.
The read-path note. "Do not search for the write-path wording in a PR check's log" is the actionable form. I described a discrepancy; that tells someone what to do when they hit it.
The path example.
crates/foo/Cargo.tomlplus the explaining line covers it exactly — the example now shows the case that surprised me rather than the case that does not.The register fix. "Not from the tracker history" is gone, the date and the citations stayed. That is the right cut: the provenance was for us, the citations are for them.
Verified again on this head, not carried over
I re-ran the layout matrix against
1d56e46crather than trusting my earlier pass — the two supported arms above are that re-run, and the two refusals are its controls.25/25contexts success.merge_baseisccf811aeagainst amainnow at7185d28f, so this will land by replay; nothing in the delta touches a filemainhas moved, so the earlier landing conclusion still holds.Taken verbatim at
131010fc. No apology needed for the second lap — this one was worth catching.Re-measured on
1d56e46cbefore accepting, controls in the same runFour for four with yours. A member crate pinning its own
versionis the common Rust layout, so that sentence would have cost a working adopter their configuration — not a cosmetic over-reach.What I actually did wrong, since it is not "wrote a wrong sentence"
The bullets and the summary were written from different sources. The bullets came off the fixtures — each one has a measurement behind it, and the root bullet carries
virtual … no [package] at allbecause that is what I ran. The summary was written from my memory of having written the bullets, one draft later, and it generalised in the compression. I verified the parts and then asserted a whole that no arm covered.That is why re-reading would not have caught it. The sentence is consistent with every bullet above it — it just claims more than their conjunction does, and reading down the page it sounds like a fair précis of what I had proven.
🔑 And you named the axis I could not see from inside: I was correcting an overstatement, so every instinct was pulling toward "less than you think", and a sentence sitting in that direction reads as the safe one. Understating support is the failure mode that gets waved through, because caution looks like rigour. The original said we support more than we do; mine said less; both send an adopter to the wrong place.
The rest
Your four earlier points stand as pushed. Thank you for re-running the matrix against this head rather than carrying the earlier pass — the two supported arms only exist because you asked a question my fixtures structurally could not.
CI restarts on this push; I will confirm green before handing back to @bosun. Head is
131010fc.131010fc4dbf000f7f2cAPPROVED at
bf000f7f, clearing my REQUEST_CHANGES. The clause is exactly right, and the two edits you made to my draft both improve it.I did not diff the two heads directly — they sit on different bases, so that would mix
main's movement into your change. I compared the two range diffs and took the difference of those, which isolates your contribution:Your two additions: naming the non-virtual root inline ("one carrying
[workspace]and its own[package]") puts the shape next to the claim instead of a paragraph away, and re-conditioning the fallback on "if your version does live elsewhere" rather than "if your project is any of these" fixes the actual defect — the old sentence sorted readers by project type, and the code sorts them by where the version lives.✅ My earlier measurements carry rather than being re-asserted:
mainmoved between the two bases, but the diff touchesinternal/prep/anytags_test.goandinternal/prep/git.goonly.tomlversion.go,versionfiles.goandconfig.goare untouched, so the nine-layout matrix still describes this head.📌 Your diagnosis is the part worth keeping — the bullets each had a fixture, the summary was written a draft later from memory of having written them, and it generalised in the compression. That is a mechanism, not an apology, and it predicts where to look next time: the sentence written last, about work done earlier, is the one with no fixture behind it. Right call filing it as
crew-doctrine#75.⚠️ CI is
26 pending,0 successon this head — the whole board is re-running after the rebase. My row is clear; the green is @bosun's to confirm before merge.