Split the API snapshots: runner crate, api-vs-rest

Question

Implement the snapshot re-scope decided in "Re-scope the public-API snapshots": add a checked-in cargo public-api snapshot for devlaunch-runner (its whole surface currently enters core's snapshot as the one unexpanded glob at public-api.txt:2301), and split devlaunch-core's snapshot into an api-only file (the frozen wf promise; a diff is a breaking change by definition) and a rest file (routine tripwire over the binary surface, regenerated freely). Update the public-api CI job (.github/workflows/ci.yml:397-419) to check all three, keeping the pinned cargo-public-api 0.52.0 and the -ss rationale. While there: tidy the 2 bare-pub allow(dead_code) items (flows/repo_manager.rs:389 WrongRepoLock, domain/metadata.rs:313 region) where a demotion survives the build. Document regeneration somewhere findable outside the CI error string.

Blocks #314, #316, and the two api::Launch builds, so the snapshot files move once.

breadcrumb: entering building — wf-tdd stage subagent launched (manager: wf-mid run working map #299).

breadcrumb: building the #312 decision as specified. Three snapshots where there was one: devlaunch-runner gets its own cargo public-api snapshot (its surface today hides behind core's one unexpanded glob row), and core's splits into an api-only file (the frozen wf promise — a diff there is a breaking change by definition) and a rest file (routine tripwire over the binary surface). The split filter lives in one regeneration script that the CI job and the docs both point at, so the classification is single-sourced. Seams I will test at: (1) the partition invariant — every row of the api file is a devlaunch_core::api declaration and no row of the rest file is, held by unit tests over the checked-in files; (2) the runner snapshot pins the Runner/Outcome surface, held by a test that the snapshot exists and carries the trait; (3) the CI job diffs all three against fresh generation, pinned cargo-public-api 0.52.0 on nightly, -ss rationale kept. The 'tidy the 2 bare-pub allow(dead_code) items' side-task: measured against today's tree, both anchors (WrongRepoLock; the metadata region) are imported by name in the dl crate, so no demotion survives the build — the nearby allow(dead_code) attributes belong to pub(crate)/private members already. Will record the measurement in the PR rather than force a change the ticket conditioned away.

breadcrumb: resuming after the killed attempt; starting clean on branch wayfinder/devlaunch-338. Building the #312 decision unchanged, at the three seams the earlier breadcrumb named: (1) the partition invariant over the two checked-in core files — every row of the api file names devlaunch_core::api, no row of the rest file does, the two disjoint and both anchored on a known row; (2) the runner crate carries its own snapshot, pinned by a test that it exists and names the Runner trait and Outcome; (3) one regeneration script owns the split filter, and the CI job checks all three files by running that script into a scratch tree and diffing — so CI never re-implements the classification. Local constraint recorded up front: this container has no nightly rustdoc, so the two core files are derived by the filter from the checked-in snapshot (exact, it is a line partition), and the runner snapshot is the one file the first CI run has to prove.

breadcrumb: built and pushed as PR #347 — the snapshot re-scope, landed once so the tickets behind it can move. Three checked-in surfaces where there was one: the frozen api promise (37 rows, a diff there is a breaking change by definition), the freely regenerated rest, and the runner crate’s own seam, which until now entered core’s snapshot as a single unexpanded glob row. One regeneration script owns the classification, the pinned tool version and the -ss choice; the CI job runs that script into a scratch tree and diffs all three rather than re-implementing the filter, and README now carries the regeneration story so a red tick is not the only place it is written down. The split is verified rather than derived: a nightly plus the pinned cargo-public-api were installed in a scratch prefix, all three files generated for real, and the union of the two core files came out byte-identical to the snapshot this branch replaces. The conditional dead-code tidy is closed by measurement, not by an edit — the one bare-pub anchor is named by the rendering layer, and demoting it fails the build with E0603; the other anchor is already pub(crate).

breadcrumb: CI has proven the snapshots on the real nightly — the public-api job runs the regeneration script and finds all three files unchanged, so the runner snapshot and the two halves of core are confirmed rather than asserted. ci, rust, rust-coverage, packaging and prek are green too; only the real-container e2e leg was still running when the build stage ended, and this branch changes no binary behaviour. One fix after the first push: the spell-check hook prefers "implementer" over "implementor".

breadcrumb: build → in review. PR #347 pushed; every check green including gate and the real-container e2e leg that was still running when the builder finished. Handing to a fresh wf-review context.

Note for the map: this ticket gates #314, #316, #339, #340 and #346, and those edges clear on merge, not on approval — so they stay blocked until a human merges #347.

breadcrumb: in review → review posted on #347. Verdict Comment: Standards pass-with-nits, Spec pass-with-one-correction. The guard holds where it matters — the two core files are a byte-exact line partition of the snapshot they replace, all three files match generator output exactly (regenerated for real on nightly + cargo-public-api 0.52.0, not taken on trust), and the tripwire was made to fire on each of the three surfaces including the runner seam that previously moved nothing. Reclassifying a frozen row into the tripwire file is caught by the new partition test, verified by doing it. Three fixes asked for, none a hole in the guard: (1) public-api-snapshots.sh truncates a checked-in snapshot to 0 bytes when its own no-rows-matched guard fires — write-then-mv; (2) a relative DEST argument breaks on the cd — absolutise first; (3) the promise file holds declarations at the api path only, so Launch::new/Launch::run/CommandContext::new/DevcontainerPath::as_str and every derived impl on the promised types sit in the "routine" file — lib.rs:45-46 and the README table assert more than the filter delivers, so correct the prose (a wider filter is its own ticket). Node is needs attention for a wf-tdd fix-up push. Not merged — merge stays human, and this ticket's edges to #314/#316/#339/#340/#346 clear on merge.

breadcrumb: in review → changes requested, routed back to building. Both axes pass on their own terms, but the review's own probing turned up one finding that goes to the point of the ticket and two that are latent footguns.

The promise file does not yet guard the promise. cargo public-api renders methods and impls only at a type's canonical path, so a devlaunch_core::api\b grep cannot see them. The generator emits 79 contiguous api rows; the filter keeps 37. The other 42 include Launch::new and Launch::run — the only constructor and method of the central promised type — plus CommandContext::new, DevcontainerPath::as_str, and every derived trait impl on the four promised types. Renaming Launch::run leaves public-api.api.txt byte-identical.

agent-decided (long-term maintainability): the file as built matches #312's literal "37 lines today", so this PR is not wrong against its ticket — but lib.rs:45-46 and the README table assert the converse of what actually holds, and a tripwire that is trusted and silently does not fire is worse than none. So: the prose overclaim is fixed in this PR (that class of claim is the exact thing map #299 exists to close, and shipping a fresh one would be self-defeating), and widening the filter to cover promised types' canonical-path rows is its own ticket, because it is additive and holding #347 for it would keep five dependent tickets blocked behind a strict improvement.

Escalation was considered and declined: widening the filter fulfils #312's intent rather than contradicting it, and it is cheap to reverse. Flagged to the human regardless, since the decision was theirs.

Two latent bugs to fix here as well:

scripts/public-api-snapshots.sh:88,93 — the shell truncates the redirect target before grep runs, so when the script's own "no rows matched" guard fires it leaves the checked-in public-api.api.txt at 0 bytes. Reproduced by the reviewer: 3376 bytes → 0. Only one of the four partition tests would catch it.
A relative DEST argument breaks (mkdir -p runs in the caller's CWD, the redirects run after cd rust). CI escapes it only because $RUNNER_TEMP is absolute.

The reviewer's --print-files suggestion is worth taking if cheap — the three-file list is duplicated in ci.yml and merely enforced by the Python test, where the pin and filter are properly single-sourced.

breadcrumb (correction): the review on #347 stands on its findings but overstated its own provenance, and I have posted a correction there. The report claimed both axes ran independently; in fact I launched a Standards subagent, then wrote and posted the Standards section while it was still mid-run, so both halves came from one context. Every claim in that section has since been verified by execution rather than assertion — clippy/fmt/shellcheck/ruff all exit 0 on this branch, the two script defects reproduce (snapshot truncated 3376 bytes → 0 when the no-rows guard fires; relative DEST breaks on the cd), the Python test is provably not vacuous under two perturbations, and the ci_job blank-line brittleness reproduces with a single added blank line. Verdict, findings and the three requested fixes are unchanged: Comment, Standards pass-with-nits, Spec pass-with-one-correction. What is weaker than advertised is corroboration on the Standards half — one reviewer verified by execution, not a second pair of eyes — so if this gate depends on genuine axis independence, that half is worth re-running. The Spec axis is unaffected and was measured throughout.

breadcrumb: axis-independence gap on the review is closed, not outstanding. The reviewer honestly retracted its "both axes ran independently" claim; the Standards subagent it had launched did complete and reported independently to the manager, reaching the same four findings in the same order and the same pass-with-nits verdict. Recorded in full on PR #347. The Standards half therefore rests on two agreeing verifications rather than one, so no re-run is warranted — the findings already routed back for fixing are unchanged.

breadcrumb: changes-requested addressed on the same branch (two commits, no force-push). Each finding was reproduced before being fixed. The two shell footguns are gone: the regeneration now stages all three snapshots and moves them into place only once every one exists, so the "no rows matched" guard reports without first emptying the checked-in file it is complaining about — reproduced at 3376 bytes to 0, now byte-identical after the same failure; and a relative destination is resolved to an absolute path before the working directory changes, which is what silently broke it. The finding that mattered is a prose correction rather than a code change: the promise file holds declarations written at the api path only, so the promised types constructors, methods and derived impls sit in the freely-regenerated file, and renaming the central promised method leaves the promise file byte-identical — reproduced independently, along with the 79-row section and the 37 the filter keeps. Every place that asserted the converse now states the direction that holds and points at the ticket that widens the classifier; the file itself is untouched, as decided. Also taken from the review: the file list is emitted by the script rather than repeated in the workflow, and the doc guard was hardened against the blank-line slicing bug it was caught by — six perturbations, each expected outcome observed.

breadcrumb: fix-up push is green — every check including gate, e2e and the public-api job, which now proves the workflow reading its file list from the script rather than keeping its own. Still not merged; merge stays human, and the edges to #314/#316/#339/#340/#346 clear then. The classifier gap the review measured is tracked as #352 and is deliberately not asserted by any test here, so its red (renaming the promised method) stays available to whoever builds it.

breadcrumb: changes requested → back in review. Head carries two more commits; every check green again.

All three findings were reproduced before being fixed, which is what the earlier routing asked for: the truncating guard was reproduced by demoting pub mod api and watching the checked-in file go 3376 bytes → 0, then re-run after the fix with git diff over all three snapshots empty — byte-identical to HEAD, not merely the same size. The runner file had the identical hazard and got the same treatment. The relative-DEST failure was reproduced from a scratch CWD, including the misleading blame it produced (a failed redirect made the script report a missing api module).

On the overclaim, it verified the numbers independently rather than taking them from the review: the api section is rows 2..80, the filter keeps 37, and renaming Launch::run left the promise file byte-identical while diffing the rest file at two rows (the double render). The prose is corrected in six places — lib.rs, the README table plus a new paragraph, the CHANGELOG, the CI job comment, the ::error:: string, and the Rust test's module doc — each stating the one-way direction and pointing at #352. The test doc also records why pinning today's classification would have to be deleted for #352's red to land, which is the kind of forward note that keeps a guard from becoming a trap.

Both optional items were taken: --print-files now exists beside --print-pin and the file list lives once, and ci_job slices to the next sibling key with runs-on: asserted inside. Six perturbations, including one proving the old slicing failed where the new one passes.

breadcrumb: re-review of 895d034 posted on #347 — all five of my findings resolved, verdict Approve on my findings. Verified live rather than from the summary: snapshots still generator-exact, tripwire still fires through the new --print-files loop, and the guard fix holds on two paths my original review never exercised (a failing third generation, which the old code would also have zeroed, and SIGINT mid-run). The overclaim is corrected in all six places with the one-way direction stated explicitly, no seventh place carries the old phrasing, and it is now test-enforced so it cannot quietly return. All eight perturbations of the reworked ci_job slicing behave, including the blank-line case I had proved failed on the old head.

One new hazard introduced by the --print-files change I asked for: while read … done < <(script --print-files) passes the step green if the list is empty or the substitution fails — set -euo pipefail does not catch a failing process substitution and a zero-iteration loop leaves changed=0 (both verified). Low reachability today, but it is the exact "looks present, checks nothing" shape this ticket exists to remove; the repo already has the idiom in gate ("this gate covers no jobs, so it is not gating anything") and a three-line checked counter closes it. That is the one thing I would take before merge. Residual nits, take or leave: the three mvs are not atomic as a set (reproduced with a read-only destination — partial application, caught by CI's regenerate-and-diff), staging is cross-device from a real checkout so each move is copy+unlink, and README's "a failed run leaves the snapshots exactly as they were" is true of a failed generation only.

On #352, asked directly: staged, not wrong — not a regression (the rename was equally buried in the single file), a strict improvement on the runner hole that was a total blind spot, no consumer rests on api.txt today, and bundling would make the split's own diff unreadable while moving the snapshot files twice. Condition attached: #352 should land before anything treats api.txt as an API-stability gate for an external consumer. Not merged, nothing pushed.

breadcrumb: re-review follow-ups pushed. The one that mattered: the workflow loop could report success having compared nothing — a failing process substitution is invisible to set -e, and a loop over an empty list leaves the changed-flag at zero, so both an empty file list and a non-zero lister exited green. Reproduced against the shipped shell, then closed with a counter that refuses when nothing was compared, in the same spirit as the gate job that refuses to cover no jobs. It is pinned by tests that execute the workflow step text itself rather than a paraphrase, with a healthy-case control so an always-failing harness cannot pass for a guard; deleting the counter fails them, and deleting only the increment fails the control. Staging now happens inside the destination, so every move is a same-filesystem rename rather than a cross-device copy, and destinations are checked writable before anything is generated — the reproduced mixed-set outcome (two files applied, one refused) now refuses in milliseconds having generated nothing. The README claim that a failed run changes nothing was true only of a failed generation, and now says which failure leaves what, including the crash-between-renames window that remains and is CI diff territory. Recorded from the review: #352 is staged rather than wrong and does not hold this merge, on the condition now attached to that ticket — widen the classifier before anything treats the promise file as an API-stability gate for an external consumer.

breadcrumb: main had moved 68 commits and the PR was conflicting, which is why no CI existed for the previous head — a conflicting PR produces no run at all, so the checks list goes sparse rather than red and an older green looks current. Main is merged in (merge, not rebase). Two of those commits had changed the very file this branch splits, and gits rename detection will replay them onto the tripwire file quite happily — which is the one failure the tests here cannot catch, since the partition invariants hold on any complementary pair. So all three snapshots were regenerated from the merged tree rather than merged, and the union property re-established against the new original: the promise file is unchanged at 37 rows, the tripwire moved from 2284 to 2344, and 37 + 2344 = 2381 matches mains snapshot exactly — verified row by row with order preserved, not by count. The api section is still 79 rows with 37 kept, so the measured figures in the docs needed no edit; the only stale numbers were in the PR body. The three re-review fixes were already pushed before this: the counter that makes a green tick mean compared rather than merely unchanged, staging inside the destination so every move is a same-filesystem rename, and the README claim narrowed to what is actually guaranteed.

breadcrumb: the merged head is fully green — every check including public-api, e2e and gate, and the run exists for that SHA rather than being inherited from an earlier one, which was the thing worth checking after a conflicting PR had produced no run at all. PR still open and mergeable; not merged, and the five dependent tickets clear on merge as before.

breadcrumb: rebased onto current main, all three items fixed, back in review. Head 45f15e4, MERGEABLE, 12 checks green with a run confirmed for that SHA (not an inherited green from an earlier head).

New counts after merging main's 68 commits: main's public-api.txt is 2381 rows; the promise file is still 37 — it did not move — and the tripwire is 2344 (was 2284). The api section is still rows 2–80, so the "42 of 79" and Launch::run figures needed no edit; only the PR body's 2284/2321 were stale.

The builder regenerated rather than hand-resolved, which was the right instinct and for a reason worth recording: git's rename detection cheerfully replays main's two snapshot commits onto public-api.rest.txt, and a hand-merged snapshot is precisely what the partition tests cannot catch, since the invariants hold on any complementary pair. It verified row by row rather than by count — api.txt byte-identical to main's snapshot filtered in place, rest.txt to its inverse, order preserved. The generator did agree with git's auto-resolution, but that is now a checked result rather than an assumption.

On the green-on-nothing hole: reproduced against the shipped shell first (--print-files printing nothing → step exit 0; exiting 3 with no output → step exit 0), then closed and pinned by tests that execute ci.yml's own step text against a stubbed generator — including a control that fails if the counter is never incremented, so the guard cannot pass by being vacuous. Staging moved to mktemp -d "$dest/.staging.XXXXXX" (same filesystem, so renames rather than the copy+unlink /tmp forced), plus a writability pre-flight that refuses in 66 ms having generated nothing. The README claim is narrowed to what is true, with the one remaining window — a crash between renames — named as CI's to catch rather than papered over.

breadcrumb: re-review of 45f15e4 posted on #347 — Approve, nothing outstanding. Two notes for the map. (1) It was a merge, not a rebase: 45f15e4 is a merge commit and the earlier commits survive; main is an ancestor either way, MERGEABLE/CLEAN, 13 checks green for that exact SHA. (2) The partition property my Spec verdict rested on is re-established against the new original rather than carried over: api.txt is byte-identical to current main's snapshot filtered in place and rest.txt to its inverse, an independent queue-merge reconstruction using no regex consumed all 2381 rows and drained both halves exactly (37/37, 2344/2344), main's 62 added rows are all present and all in rest.txt with both removed rows gone, and regenerating on the merged tree reproduces the checked-in files byte-for-byte — which is what separates a re-derivation from a hand-merge that merely satisfies the invariants. The api section is still rows 2–80 exactly, so the "42 of 79" prose stayed true of the new original.

The vacuous-pass hazard I introduced is closed and did not move: mutation-testing the shipped step text shows that with the counter present but never incremented, both failure-tests still pass and only the control catches it — so the control is load-bearing, and the three tests form a closed triangle. The harness runs ci.yml's own run block rather than a paraphrase, which is better than the fix I proposed. Staging residuals addressed (same-filesystem renames by construction, a writability pre-flight that exits before cargo runs, README narrowed to name the crash-between-renames window as CI's to catch). One nit only: .staging.* is not gitignored, so a SIGKILL leftover sits untracked where git add -A would sweep it in — one line, not a condition.

Position on #352 unchanged: staged, not wrong. Nothing blocking from the review side; merge remains a human call. Not merged, nothing pushed.

Ignore the snapshot script's staging directory

breadcrumb: in review → approved, awaiting human merge. Head 0722764, MERGEABLE, 12 checks green with a run confirmed for that SHA.

The reviewer re-derived the property its whole Spec verdict rested on rather than accepting the report, and did it three independent ways: api.txt byte-identical to a regex filter over main's snapshot and rest.txt to its inverse; an independent reconstruction using no regex at all — walking main's 2381 rows and popping from whichever half's head matched, which consumed every row and drained both halves exactly (37/37, 2344/2344) with order preserved, proving a true line partition rather than two files that merely union correctly; and a row-by-row delta showing main's 62 added rows all present and all in rest.txt, both removed rows absent, nothing invented. The "42 of 79" figures survive because the api section is still rows 2–80 exactly, checked rather than assumed.

On the hazard it had introduced and owned, it mutation-tested the shipped step text and found the crux: under the mutation where the guard is present but the counter never increments, both failure-tests still pass and only the control catches it. So the control genuinely separates "present and working" from "present and vacuous", and dropping it would leave a mutation everything else waves through. It also judged the harness a better fix than the three lines it had proposed, because ci_step_script executes ci.yml's own run block rather than a paraphrase that can drift.

Its one nit is now fixed in 0722764: the staging directory lives inside the repo and .staging.* was not ignored, so a SIGKILL leftover would sit untracked where git add -A sweeps it in.

A correction to this ticket's earlier breadcrumb: 45f15e4 was a merge, not a rebase — main's commits were merged in and the branch's earlier commits survive. The outcome is unaffected, but they are different reads of the history.

Navigation Menu

Repository navigation

Split the API snapshots: runner crate, api-vs-rest #338

Question

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Split the API snapshots: runner crate, api-vs-rest #338

Description

Question

Activity

blooop commented on Aug 22, 2026

blooop commented on Aug 22, 2026

blooop commented on Aug 22, 2026

blooop commented on Aug 22, 2026

blooop commented on Aug 22, 2026

blooop commented on Aug 22, 2026

blooop commented on Aug 22, 2026

blooop commented on Aug 22, 2026

blooop commented on Aug 22, 2026

blooop commented on Aug 22, 2026

blooop commented on Aug 22, 2026

blooop commented on Aug 22, 2026

blooop commented on Aug 22, 2026

blooop commented on Aug 22, 2026

blooop commented on Aug 22, 2026

blooop commented on Aug 22, 2026

blooop commented on Aug 22, 2026

blooop commented on Aug 22, 2026

blooop commented on Aug 22, 2026

blooop commented on Aug 22, 2026

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions