Claim a promised type's canonical rows for the promise file - #518
Conversation
`cargo public-api` renders inherent methods and trait impls at a type's canonical path only, never at the path it is re-exported under, so `api::Launch::run` is rendered `flows::launch::Launch::run` and a filter matching `devlaunch_core::api\b` could not see it. `public-api.api.txt` was 126 rows and not one of them was a `::new(` or a `::run(`: renaming `Launch::run` left the file that exists to catch exactly that byte-identical. The classifier now reads the `api` module's own `pub use crate::...` statements, resolves each to the canonical path it names, and claims that item's rows too. A row's subject is what is claimed, never a mention: promised types are arguments in half the signatures in this crate, and a substring match would drag the binary surface into the promise file. 395 rows move from `public-api.rest.txt` to `public-api.api.txt` and none appear from nowhere. Both were regenerated with nightly and the pinned cargo-public-api 0.52.0, after reproducing the base's three files byte-identically. The rule is reachable now without the toolchain, as `--classify api|rest` over rows on stdin, so the Python guard exercises the real one on rows chosen to be awkward and the Rust guard holds the two files to a fixed point of it rather than restating it in Rust. The assertion that every promised row names `devlaunch_core::api` stopped being true and is what the fixed point replaces. Closes #352
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideThe PR fixes the public-API snapshot classifier so behavior and impl rows for types re-exported by Sequence diagram for public-API snapshot generationsequenceDiagram
participant Script as public-api-snapshots.sh
participant Source as devlaunch_core/src/lib.rs
participant Generator as cargo public-api
participant Classifier as promised_row_pattern()
participant Snapshots as API and rest snapshots
Script->>Source: promised_paths()
Source-->>Script: canonical re-export paths
Script->>Generator: generate public API rows
Generator-->>Script: core API rows
Script->>Classifier: classify rows using resolved paths
Classifier-->>Script: promised and rest rows
Script->>Snapshots: write public-api.api.txt and public-api.rest.txt
Sequence diagram for classifier regression testingsequenceDiagram
participant Test as snapshot tests
participant Script as public-api-snapshots.sh
participant Classifier as --classify api|rest
participant Rows as representative API rows
Test->>Script: invoke --classify api
Script->>Classifier: apply promised_row_pattern()
Classifier->>Rows: inspect row subjects
Rows-->>Classifier: canonical method claimed, argument mention rejected
Classifier-->>Test: classified rows
Test->>Script: invoke --classify rest
Script-->>Test: complementary rows
Flow diagram for canonical public-API row classificationflowchart LR
ApiModule["devlaunch_core::api re-exports"] --> PromisedPaths["promised_paths() resolves canonical paths"]
PromisedPaths --> Pattern["promised_row_pattern()"]
SnapshotRows["cargo public-api rows"] --> Pattern
Pattern --> ApiSnapshot["public-api.api.txt\n promised declarations and behavior"]
Pattern --> RestSnapshot["public-api.rest.txt\n remaining binary surface"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Reviewed the three-dot diff against merge base 9925291 (git diff 9925291...5b237ab), i.e. this PR's own single commit only, not #514's. Note the base branch wayfinder/devlaunch-340 has since advanced to 0c5dea0; the merge base is unchanged, so the diff reviewed is still exactly this PR's.
Every headline claim in the description verified, and two of them understated. Counts: 126→521 and 2755→2360, exactly 395 rows moved. The union is identical as a multiset, not merely by count (a count match would not have ruled out N added / N dropped) — so nothing appeared from nowhere in the strong sense. All 395 moved rows resolve to a promised subject: zero over-claim collateral, checked exhaustively rather than by sample. 521 lines / 378 unique = the 143 duplicates claimed; cross-file overlap is 0, so the partition holds.
The prefix hazard is handled. Launch does not drag in Launched, LaunchAborted or LaunchRefusal — the \b cannot fire mid-identifier, and all three correctly stayed in the rest file.
The fixed-point test is not vacuous. Fed the corpus through four broken classifiers, all four fail the assertion: identity/match-everything (2881 rows), match-nothing (0), the old pre-PR api-path rule (126 — which also reproduces the before-number), and a naive unanchored substring widening (616). It pins the partition against regeneration drift, not just against hand-moved rows.
Standards
1. The new doc guard asserts almost nothing (material). REST_SNAPSHOT = "public-api.rest.txt" is satisfied at all three sites by strings that exist for unrelated reasons: REST_FILE=devlaunch-core/public-api.rest.txt (script line 92, a variable assignment), the split-description sentence at lib.rs:46, and the table row at development.md:53. Proved by mutation — I deleted the entire residual-limit paragraph from lib.rs (the four lines from "What it still does not reach…"): 14 passed. Deleted the equivalent comment block from the script: 14 passed. The docstring says two things "live wherever the promise file is described"; only the canonical half is actually guarded. The old WIDENING_TICKET = "352" was weak but at least appeared only in prose. Suggest asserting a phrase unique to the caveat — "never re-exports" or "hands back".
2. Latent over-claim shapes the anchors miss (minor). The script's comment states a path nested in generics never follows a space. Not true for multi-argument generics, tuples, bounds or where clauses. Run through the real --classify api, all three of these are claimed:
impl core::convert::From<(u8, devlaunch_core::flows::launch::Host)> for devlaunch_core::internal::Bogus
impl<T: devlaunch_core::notices::Notices> devlaunch_core::internal::Sink for devlaunch_core::internal::Bogus<T>
impl devlaunch_core::internal::Sink for devlaunch_core::internal::Bogus where T: devlaunch_core::flows::launch::Host
Zero such rows exist today (0 where rows, and no promised path currently appears in a bound), so this is latent, not live.
3. Latent under-claim: multi-keyword pub rows (minor). ^pub ([a-z]+ )? allows exactly one keyword, so a promised type's pub const fn, pub async fn and pub unsafe fn rows fall to the tripwire file. Confirmed against the real classifier — pub const fn devlaunch_core::flows::launch::Launch::konst() is not claimed. No such rows today, but a const fn constructor on a promised type is an ordinary thing to add. impl X for &devlaunch_core::flows::launch::Host is likewise unclaimed.
4. promised_paths() silently mis-parses as renames (minor). The comment says the parser "refuses anything else rather than guessing, because a form it half-understands is a path it gets wrong". The as form is exactly that, and it does not refuse: pub use crate::domain::spec::WorkspaceSpec as Spec; yields devlaunch_core::domain::spec::WorkspaceSpecasSpec, which passes the identifier-shape validator because the welded result is a valid identifier. Also, /* */ block comments are not stripped (only //), so a commented-out pub use parses as a promised path. Both are caught loudly at generation time by the "every promised path has to claim something" guard, so neither can ship a wrong promise file — but the load-bearing comment is wrong, and --classify has no such guard. Globs are correctly refused; nested groups, multi-line groups and trailing commas all parse correctly.
5. The branch now conflicts with its moved base (minor, stacking). 0c5dea0 edits the very doc-comment paragraph in tests/public_api_snapshots.rs that this PR deletes; git merge-tree reports a content conflict there. Trivial to resolve (take this PR's rewrite), but gh pr view's MERGEABLE is stale. Otherwise stacking hygiene is clean: no re-application or reversal of #514's hunks, and CI's public-api job regenerated and matched on the stacked base.
Spec
Quoting #352: "Widen the classifier so a promised type's canonical-path rows are classified as promise, not as tripwire." Met, and verified exhaustively rather than by assertion.
"make sure the partition stays a partition (every row in exactly one file)" — met; 0 cross-file overlap. The within-file duplicates are tolerated deliberately: the_two_files_share_no_row only compares across files, and the fixed-point test compares Vecs, so the 143 duplicates are pinned in place and in order rather than accidentally ignored.
"Red-first, and the red is the finding itself: rename Launch::run, and public-api.api.txt must change." Met. The old rule yields 126 rows containing none of the four methods, so the_promise_file_carries_the_promised_types_behaviour genuinely fails on the base.
6. The documented residual is one type; the real residual is 38 (material). All three sites name only flows::launch::Launched. Counting the devlaunch_core paths named inside promise-file rows that are not themselves promised and have rows of their own in the tripwire file gives 38 types, ~525 rows — among them:
devlaunch_core::domain::spec::DevcontainerRefError 12 rows in rest
devlaunch_core::domain::metadata::MetadataError 29 rows in rest
devlaunch_core::domain::workspace_id::WorkspaceId 17 rows in rest
devlaunch_core::domain::metadata::Notice 40 rows in rest
DevcontainerRefError is the pointed one: pub fn devlaunch_core::api::resolve_devcontainer_ref(&str) -> Result<DevcontainerPath, DevcontainerRefError> is a row in the promise file at the api path itself, and renaming one of that error's variants breaks every consumer that matches on it while diffing only the file the docs call "regenerate freely". MetadataError is carried by the promised StartupError::Metadata and RecordsNotice::MigrationRefused. A reviewer told the limit is Launched will read those diffs as routine churn. The caveat is accurate about the mechanism and wrong about the scale — worth saying "38 types including the error type of a promised function" rather than naming one, since this is the sentence the whole tripwire file is read against. Finding 1 compounds it: nothing would notice if that sentence were deleted outright.
7. No visible answer to a direct spec instruction (minor). #352 says: "check whether cargo public-api offers this directly before building it by hand". Neither the diff, the commit message nor the PR body records what that check found, and the result is a hand-rolled awk+grep classifier. It may well be the right answer — I could not verify offline what 0.52.0 offers, as it is not installable in this environment — but the check the ticket asked for leaves no trace, so the next person re-opens the question from scratch. One sentence in the script header would close it.
Verdict
Comment — nothing blocking. The change does what #352 asked, and the central mechanism is more solidly evidenced than the description claims: exhaustive zero over-claim across all 395 moved rows, multiset-identical union, and a fixed-point test that rejects four distinct broken classifiers.
Two findings are worth addressing before merge, neither of which is a defect in the classifier itself:
- Finding 6 — the residual caveat understates its scale by a factor of 38 and omits that a promised
api-path function's error type is inside it. - Finding 1 — the guard meant to keep that caveat honest passes with the caveat deleted.
Findings 2, 3 and 4 are latent and cost nothing today; 5 is a rebase.
Three sites described the limit on the promise file as flows::launch::Launched
and nothing else. Measured on the merged tree it is 39 types owning 615 rows in
public-api.rest.txt, DevcontainerRefError among them -- the error type of
api::resolve_devcontainer_ref, whose own row is in the promise file at the api
path, so renaming one of its variants breaks every consumer matching on it and
diffs only the file the docs call routine.
--print-residual computes that list off the checked-in snapshots with no
toolchain, and the doc guard now diffs the figures in all three descriptions
against it, so the number goes red rather than stale.
The old guard was near-vacuous: REST_SNAPSHOT alone is satisfied by a shell
variable assignment, a table row and an unrelated sentence, so the whole caveat
could be deleted from any site and all fourteen tests passed. It now pins the
mechanism ('never re-exports'), the example, and the command. Proven by
deleting the paragraph at each of the three sites in turn: two tests red each
time.
Also from the review: promised_paths() welded 'X as Y' into the identifier
'XasY' while its comment claimed it refused what it could not parse -- it now
resolves the rename to the canonical path, which is what the classifier wants,
and strips block comments as well as line comments. The subject anchor allows
more than one keyword after pub, so a promised type's 'pub const fn' is claimed
(no row moves today). The impl anchor's comment no longer claims generics never
follow a space. And #352's 'check whether cargo public-api offers this
directly' now has its answer written down: 0.52.0 has --omit, --include,
features, target and -p, and no path, module or reachability filter at all.
#524 widened api's re-exports (29 promised paths to 40), so the promise file is 813 rows where it was 521 and the moved-row figure is 631, not 395. The residual is still 39 types, now over six hundred rows. Note main's checked-in public-api.rest.txt is stale by one row (MetadataStorage::look) against main's own tree; regenerating here adds it.
Every documented figure still holds: 182 rows under the old classifier, 813 under this one, 631 moved, 39 residual types.
A bare word-boundary match on 39 was satisfied twice over by things that are not the residual: '395 rows moved' two paragraphs up, and '39 of them' about orphaned Docker volumes five hundred lines away in the same document. Deleting the caveat from docs/development.md left this green. It now matches '39 types' and '39 such types', and deleting the caveat from any of the three sites fails two tests.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
1 participant
public-api.api.txtis meant to be the frozen wf promise, where a diff is a breaking change by definition. It was not.cargo public-apirenders inherent methods and trait impls at a type's canonical path only, never at the path it is re-exported under, soapi::Launch::runis rendereddevlaunch_core::flows::launch::Launch::runand a filter matchingdevlaunch_core::api\bcannot see it. Measured on the merged tree, the old rule kept 182 rows and not one of them contained::new(or::run(.The red
Three of them, and the third is the one the ticket asked for.
A guard over the checked-in file.
the_promise_file_carries_the_promised_types_behaviourasks the promise file forLaunch::new,Launch::run,CommandContext::newandDevcontainerPath::as_str. Before the classifier changed, all four were missing:A guard over the classifier itself. The rule lived inside a script that needs a nightly toolchain and two minutes of rustdoc to reach, so it had never been exercised on a row of anyone's choosing.
--classify api|restis now a filter over rows on stdin, andtest/test_public_api_snapshots_doc.pydrives the real one on rows built to be awkward: a canonical-path method row, which must be claimed, againstBranchManager::adopt(&self, &Host), which must not. That second one is the whole difficulty. Promised types are arguments in half the signatures in this crate, so a substring match would drag the binary surface into the promise file wearing the other hat.The ticket's own red: rename it and look. With the widened classifier,
Launch::runrenamed torun_renamedand the snapshots regenerated by the real tool, the promise file moves. Before, that rename left the file byte-identical.The fix
The classifier reads the
apimodule's ownpub use crate::...statements, resolves each to the canonical path it names (40 of them,--print-promisedwill list them), and claims that item's rows as well as theapi-path ones. A row's subject is what gets claimed, never a mention: the path right afterpuband its item keywords, or, for an impl, a path in the header that follows a space.Row counts, on the merged tree
devlaunch-core/public-api.api.txtdevlaunch-core/public-api.rest.txtdevlaunch-runner/public-api.txt631 rows moved, none appeared from nowhere. Both snapshots were regenerated with a nightly toolchain and the pinned cargo-public-api 0.52.0, and that toolchain reproduces
main's three checked-in files byte-identically first, so the move is the classifier's and nothing else's. The two core files are an exact line partition: feed their concatenation to--classify apiand--classify restand you get each file back, byte for byte, with zero cross-file overlap.One thing found while doing that:
main's checked-inpublic-api.rest.txtis stale by one row againstmain's own tree, missingpub fn devlaunch_core::domain::metadata::MetadataStorage::look(...). Regenerating here adds it.Two things the ticket did not say
The generator emits each promised type's impl rows twice, byte-identical: once under the
apisection and once under the module that owns them. A path match cannot tell the two copies apart, so claiming the canonical rows moves both. That is the honest reading of what the tool emits rather than a compromise. The alternative leaves an identical row in both files, which the partition guard would fail.The classifier's own limit moved rather than closed, and it is not one type. Every description of the promise file used to name
flows::launch::Launchedand stop, which reads as one type. The residual is 39 types owning over six hundred rows inpublic-api.rest.txt, and the pointed one isdomain::spec::DevcontainerRefError:apipromisesresolve_devcontainer_ref, whose own row is in the promise file at theapipath, and it returns that error, so renaming one of its variants breaks every consumer matching on it while diffing only the file the docs call routine.domain::metadata::MetadataErroris the same shape, carried by the promisedStartupError::Metadata.scripts/public-api-snapshots.sh --print-residualcomputes that list off the checked-in snapshots with no toolchain, andtest/test_public_api_snapshots_doc.pyholds the count in all three descriptions to what it prints.Does
cargo public-apido this already?#352 asked the question and it now has a written answer, in the script header where the classifier lives. No, not in the pinned 0.52.0. The tool offers exactly four ways to select what is rendered:
--omit blanket-impls|auto-trait-impls|auto-derived-impls(and the-sshorthands),--include function-parameter-names, the feature/target flags, and-pfor the package. None of them is a path, module or reachability filter, and there is no upstream notion of "the surface reachable from this module", so the choice is a filter over rendered rows or nothing. Re-check when the pin moves.The Rust guard
The one that asserted "every promised row is a
devlaunch_core::apideclaration" stopped being true and could not be patched into truth without a second copy of the classifier in Rust. It is replaced by a fixed point: feed the classifier both files and it must hand back the same two, each row on the side it is already on and in the order it is already in. What that does not pin is where a row sits within its file, since the classifier preserves the order it is fed; only CI's regenerate-and-diff catches that, anddocs/development.mdnow says so.Review findings addressed
docs/development.md,src/lib.rs, the script header, and thepublic-apijob's comment inci.yml), with the mechanism, the real count, andDevcontainerRefErrornamed.REST_SNAPSHOT = "public-api.rest.txt"was satisfied by a shell variable assignment, a table row and an unrelated sentence. It now pins the mechanism (never re-exports), the example (DevcontainerRefError), the command (--print-residual), and the count of types diffed against what that command prints. Proven by deleting the entire caveat paragraph from each of the three sites in turn: two tests red each time, green with it restored.promised_paths()mis-parsedasrenames. It stripped whitespace before looking, weldingWorkspaceSpec as Specinto the valid-looking identifierWorkspaceSpecasSpec. It now resolves the rename to the canonical path, which is what the classifier wants, and strips/* */comments as well as//ones.pubrows.^pub ([a-z]+ )?allowed one keyword, so a promised type'spub const fnfell to the tripwire file. Widened to([a-z]+ )*; no row moves today, and the Python test now carries apub const fnrow.Vec<T>andFrom<T>, false for multi-argument generics, tuples and bounds. The comment now says which way it is wrong and why it is a note rather than a second clause: no row of that shape exists, and the fix is to parse the header rather than match it.Gates
cargo test --workspace,cargo clippy --locked --all-targets -- -D warnings,cargo fmt --check, andpixi run ci(pylint 10.00/10, 635 passed, 6 skipped). CHANGELOG entry under[Unreleased]/### Changed, verified both ways: zero deletion lines againstorigin/main, andscripts/changelog_frozen.pyreports all 65 released sections untouched.Closes #352