Repository navigation
Latent destructive install hazard: the installer has no extension guard and the only thing keeping the emitted 17-line Cargo.toml off the hand-maintained stage0 manifest is the upstream .rs-only comparison denominator - #9972
Closed
briansrls wants to merge 9 commits into
Conversation
…ed, instead of inheriting the comparator's .rs denominator `install_convergence_stage_with_backend` copies `candidate_src/<basename>` over `stage0_src/<basename>` for whatever roster it is handed, asking nothing about the artifact. Nothing had gone wrong, and nothing declared why. The emitter really produces a bare `Cargo.toml` -- `src/v1/stage0/src/emitted_population.rs`, the emitter's own committed declaration of what it emitted, lists it as the single non-`src/` path. `write_emitted_tree` writes it into the candidate tree verbatim (it skips rustfmt for non-`.rs`), `produce_candidate_manifest` rows it as a `GeneratedSurface` declared by `v1.compiler.emit_rust`, and `admit_candidate_manifest` admits it. Every stage between the emitter and the copy accepts it. The only thing keeping it off the install roster was `is_compared_generated_basename` -- a predicate in a different function, written to denominate the DRIFT COMPARISON. Install admissibility had no authority of its own; it was a consequence of a fact free to move, and the natural widening of that fact (give a non-Rust generated artifact drift coverage) would have turned the copy into an overwrite of authored bytes with no line stopping. DESIGN section 3: one fact with no home. So the boundary asks for itself, over the whole roster before the first byte moves, refusing rather than skipping -- a per-item skip re-exports the same silence one layer in. Three arms: not a bare basename, not a generated Rust surface, hand-maintained mirror. The rung is stated honestly as mechanically preventable with its next-rung trigger, since the roster is still a `Vec<String>`. Also deletes `install_candidate_paths`: a second, equally unguarded installer with ZERO callers anywhere in the tree, kept compiling by a crate-wide dead-code allow. Guarding dead code would be decoration (section 4b); an unguarded operation with no consumer is residue that reads as precedent (section 6). Evidence, both directions executed locally: - guard present: `cargo test -p v1-compiler --lib regen_convergence_host_instrument_tests` -> ok, 3 passed, 0 failed (includes the pre-existing mutating_transaction test) - guard neutered to `let _ = admit_install_target(..)`: FAILED -- "installing Cargo.toml refused with CandidateManifestPopulationMismatch, expected InstallTargetOutsideGeneratedRustPopulation" - `cargo clippy --all-targets -- -D warnings`: clean That RED control is the point: without the guard the call still returns Err, so a test asserting only "it refused" would be permanently green. Each negative arm passes an EMPTY admitted manifest and asserts the CAUSE, which passes only if the install boundary answered before the manifest was consulted at all. Files the class on `gunbc.recurring_failure_mode` as `incidental_denominator_as_wall` per section 4b's census obligation, with DESIGN.md and docs/design-ledgers.md regenerated through main_wet on the local tree (one line added to each projection). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H8YCwmE7ncLG6seK6oe7Sw
…ee arms discriminate
Review of the first cut found that the guard's second arm was the same defect it was
filed against, committed in the repair.
DROP THE EXTENSION ARM. Refusing every non-`.rs` install target reads as a wall and is
the accidental comparison denominator promoted from coincidence to policy, in a second
location. It would also refuse the state the system is trying to REACH: the emitted
Cargo.toml is stage0's OWN package manifest emitted incompletely (17 lines carrying
`[package] name = "v1_compiler"` against the committed 172-line `v1-compiler`), standing
in the same relation to its committed file as the emitted main.rs does -- and
stage0_crate_layout `emitter_produced_divergent_note` fixes that relation: the reachable
end state is the emitter producing the committed bytes with the divergent family EMPTY.
Narrowing the population and calling the narrowing a wall is the dual of widening it.
The two surviving arms are kind-agnostic: a path that cannot address its own destination,
and a hand-authored mirror. What keeps a non-Rust artifact off the roster stays upstream,
and making its absence a typed disposition is the projection-identity subject, not this
boundary's.
CORRECT THE HARM CLAIM, which the first cut transcribed from the brief. It said a widened
population would overwrite the hand-maintained manifest. It would not: destinations are
joined under stage0_src = workspace/src/v1/stage0/src, so a bare basename resolves one
directory BELOW the manifest. Executed, guard neutered, valid manifest row:
SCRATCH result_ok=true
SCRATCH stage0_src/Cargo.toml exists = true
SCRATCH pkgroot/Cargo.toml exists = false
So the install succeeds and lands at src/v1/stage0/src/Cargo.toml; the package root is
never reached. Honest limit: the fixture had no package-root manifest, so this shows the
install does not CREATE one there -- that it cannot OVERWRITE one rests on the join
expression, which cannot address a parent.
THE SURVIVING HARM IS EXEMPTION, NOT DESTRUCTION, and it is the finding: a declared
GeneratedSurface the comparison filter hides is invisible to first_generation_equal and
changed_paths, and unreachable by `emitter_produced_divergent_registrations` -- the one
authority that would force its gap to be counted. The filter did not accidentally PROTECT
the artifact; it accidentally EXEMPTED it.
ALL THREE ARMS NOW SHOWN DISCRIMINATING. The test asserted inside its loop, so it aborted
on the first arm and proved nothing about the other two, which exercise a different
branch. It now accumulates and asserts once. Guard neutered:
installing ../Cargo.toml refused with CandidateManifestPopulationMismatch, expected InstallTargetNotABareBasename
installing nested/mod.rs refused with CandidateManifestPopulationMismatch, expected InstallTargetNotABareBasename
installing cli_run.rs refused with CandidateManifestPopulationMismatch, expected InstallTargetHandMaintained
RED_EXIT=101
Guard restored: test result ok, 3 passed, 0 failed.
The recurring-failure-mode row records the overstatement and the join expression that
decides it, and gains a second recognition rule for the trap that catches the person
FIXING the bug: restating the incidental denominator as the guard's own rule. Check
whether the arm's stated reason forbids the state the system is trying to reach.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H8YCwmE7ncLG6seK6oe7Sw
Roster conflict in gunbc.recurring_failure_mode: both sides appended one class (main's merge_region_excludes_shared_tail, this branch's incidental_denominator_as_wall). Resolved as a union with main's ordering preserved. DESIGN.md and docs/design-ledgers.md are generated projections and were NOT hand-resolved or side-picked -- the merge driver refused them, leaving them unmerged with the ours side verbatim, and they were regenerated from the merged authority via main_wet on the local tree. Both class names appear in both projections. The regen reproduced main's incoming bytes for every other generated artifact it touched byte-identically (no file shows an unstaged modification after the run), so nothing of main's was clobbered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H8YCwmE7ncLG6seK6oe7Sw
…des_shared_tail, and stop generating it Second integration in this session, and this one is a specimen of the class main just landed one commit earlier. REGION 1 WAS THE SHARED-TAIL TRAP. Both sides appended a multi-line RecurringFailureMode block, and the three-way merge factored the identical ` evidence: [],` / `}` suffix OUT of the conflict region -- two half-blocks above the markers, one tail below. The natural resolution (drop the markers, keep both) would have left my row's closing lines belonging to main's row, which per gunbc.recurring_failure_mode merge_region_excludes_shared_tail is not silently wrong but a PARSE REFUSAL at the module index. Resolved by giving each block its own tail. APPLIED THAT ROW'S OWN REPAIR rather than only dodging its trap: "make the appended unit one line, and the shared suffix is a blank separator carrying nothing." incidental_ denominator_as_wall is now a single line, so it stops being a conflict generator on every future integration. main's sealing_property_erases_structure keeps its multi-line shape -- not mine to reformat, and touching it would add conflict surface. Region 2 (the roster list) was an ordinary three-way append: union, main's order first. VERIFIED BY THE COMPILER, NOT BY BRACE COUNTING. I wrote an ad-hoc brace-balance check and it reported the resolved file UNBALANCED at 36/38 -- then reported the same 2-count skew on both known-good parents, so the instrument was broken by string literals containing braces and its verdict carried no information. The oracle that does discriminate is the one the class names: the module has to parse. main_wet resolved the corpus and regenerated the projections at EXIT=0, and all three class names appear in DESIGN.md. Projections regenerated from the merged authority, not side-picked. No file shows an unstaged modification after the run, so the regen reproduced main's incoming bytes exactly for every other generated artifact it touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H8YCwmE7ncLG6seK6oe7Sw
…ditive resolution was correct Third integration in this session. Both sides' new rows were ONE LINE this time -- mine from the previous merge, main's restoration_promise_names_a_route_that_does_not_exist as authored -- so the conflict region held two COMPLETE units with no shared tail factored out below it. The purely additive resolution (keep both, main's order first) is therefore the correct one here, which it was not on the previous merge. That is merge_region_excludes_shared_tail's own prescribed repair working as stated: make the appended unit one line and the shared suffix becomes a blank separator carrying nothing. Recording it because the class row had the repair but no receipt that it holds. Verified by the compiler rather than by inspection: main_wet resolved the corpus and regenerated the projections at EXIT=0, and all four class names appear in DESIGN.md. Projections regenerated from the merged authority; no file shows an unstaged modification after the run, so main's incoming bytes were reproduced exactly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H8YCwmE7ncLG6seK6oe7Sw
…ustification seed_growth_forward_freeze_policy_note requires every PR adding hand-written src/v1 Rust to enumerate the exact item identity, and states plainly that there is no netting deletions against additions. This PR adds one production declaration to required_regen_host and was not enumerated anywhere; deleting the callerless install_candidate_paths does not discharge that. The item extends the existing regen_convergence justification rather than minting a second row: same module, same current_boundary, same owning lane, and the standing trigger -- delete when modeled filesystem mutation, process execution, and executable identity let v2.workflow.regen_convergence_transaction execute directly instead of through required_regen_host -- already covers the install admission boundary, which exists only because that host still copies bytes itself. install_candidate_paths was never rostered, so its deletion leaves no stale justification behind. Test declarations are not enrolled in this roster, so the rewritten admission test needs no entry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H8YCwmE7ncLG6seK6oe7Sw
… where it resolves review 58212 found the lexical admission's containment claim unenforced against filesystem indirection, and it is right. fs::copy follows the DESTINATION link and git tracks symlinks (mode 120000), so a committed stage0 entry that is a symlink puts installed bytes on any path in the tree while emit_path_basename reports the destination as a bare, contained basename. admit_install_destination runs in the same pre-loop, over the whole roster before a byte moves. It reads symlink_metadata and never metadata: metadata FOLLOWS the link and would answer the one question being asked about the wrong file. It refuses symlinks and non-regular destinations, admits NotFound (a first-time emitted artifact legitimately has no committed destination, while a DANGLING symlink still reports Ok/is_symlink and refuses), and refuses on unreadable, because unknown containment answered with "proceed" is the absorbing fallback DESIGN 5 forbids. Rung stated honestly and deliberately OFF the ladder: the filesystem is external reality that no modeled type makes impossible (DESIGN 4b, "outside the modeled guarantee"), so look-and-refuse is the honest arm, not a climb. THE RED PROBE FOUND A DEFECT IN THE EVIDENCE, NOT THE CODE. With the guard neutered, the symlink arm failed at CandidateManifestPopulationMismatch: a bare basename with no admitted manifest row is refused UPSTREAM, so the copy was never attempted and the "outside file survived" assertion was satisfied by that upstream refusal while discriminating nothing about containment (executed_conjunct_discriminates_nothing). Containment now has its own test carrying a real admitted manifest row and real candidate bytes, so every upstream gate passes and this admission is the only thing between the copy and a file outside stage0_src. Its green is self-evidencing: it asserts the refusal IS InstallDestinationNotARegularFile, so an upstream refusal fails it. recurring_failure_mode: the row's own harm correction was an under-enumeration of the same shape it names. It said destruction additionally required the join to become package-root-relative; a destination symlink reaches outside with the join untouched. Amended to state the general form -- enumerate the ROUTES to the destination, since "resolves outside X" is a property of the filesystem and not of the path expression. admit_install_destination is enumerated in gunbc.regen_convergence_seed_growth under the same rule as its predecessor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H8YCwmE7ncLG6seK6oe7Sw
# Conflicts: # DESIGN.md # dag/gunbc/recurring_failure_mode.dag # docs/design-ledgers.md
# Conflicts: # DESIGN.md # dag/gunbc/recurring_failure_mode.dag # docs/design-ledgers.md
briansrls
marked this pull request as ready for review
September 1, 2026 23:04
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18189f6851
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "InstallDestinationNotARegularFile: {basename} exists and is not a regular file, so \ | ||
| the install destination is not a committed generated artifact" | ||
| )), | ||
| Ok(_) => Ok(()), |
There was a problem hiding this comment.
On filesystems supporting hard links, a regular destination may share its inode with a path outside stage0_src, so this arm admits it and the later fs::copy truncates the shared inode, modifying the outside path despite the containment guard. The journal cannot repair that damage because rollback renames a backup over only the stage0 directory entry. Replace the destination atomically via a new temporary file, or explicitly reject multiply linked files before installation.
Useful? React with 👍 / 👎.
Contributor
|
Closing: this is a duplicate of #9920, which already merged. Head 18189f6 is the exact commit squash-merged as 6bdc949. Verified by merge-tree: merging this branch produces tree 3f22d71e61a70574059ba93f53d252edd5bcc29b, byte-identical to current main's tree. Zero delta — there is nothing here to land. The review's substance is accurate and describes content that is already on main: the kind-agnostic admission at the mutation boundary, the deletion of the caller-less install_candidate_paths, the discriminating RED plus positive control, and the honest mechanically-preventable rung with its named capability trigger. No changes are owed against it. For anyone tracing this later: the branch reads 'ahead' permanently because the squash means its landed sha is never an ancestor of main. That is a display artifact, not unlanded work. This is the fourth such duplicate opened on already-merged branches today (#9962, #9963, #9970, #9972). — sent from wise-badger-902 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Auto-opened by session-dashboard for session
nimble-bee-690.Pushing to
session/nimble-bee-690advances this PR.Worker attestation
Before flipping this PR to ready for review, confirm each item:
npm test,cargo test) and the result.Closes #Ndirective.Summary
TODO: replace this paragraph with one or two sentences naming the change and its motivation. Reviewers read this first.
Test plan