Plan: Add Proptest coverage for deterministic Ninja emission (4.3.1) - #700
Draft
leynos wants to merge 4 commits into
Draft
Plan: Add Proptest coverage for deterministic Ninja emission (4.3.1)#700leynos wants to merge 4 commits into
leynos wants to merge 4 commits into
Conversation
Draft the approval-gated execution plan for roadmap item 4.3.1. The plan covers the three roadmap sub-items (insertion-order stability, `default` target ordering, and `path_key` invariance) and additionally discharges the larger-N duplicate-output and cycle-rejection obligations that roadmap 4.2.1 and ADR-004 explicitly deferred to this item. It also settles the determinism contract that `formal-verification-methods-in-netsuke.md` asks for but no roadmap item owns, via a new ADR-021 plus users-guide and README wording, so the property tests verify a stated guarantee rather than incidental behaviour. Eight milestones, nine verification obligations with non-vacuity evidence and mutation patches, and a measured decision on whether the suite needs a light/heavy tier split. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Name each mutation patch after the test it falsifies, matching the existing 18 patches under `docs/verification/mutations/`, and refer to them by short handles so the long filenames do not breach the 80-column Markdown limit. Record that a `find_duplicates` patch already exists from the 4.2.1 Kani work, and that the new one is deliberately more surgical. Add the `always`/`restat` interaction to the insertion-order obligation's non-vacuity coverage; `DisplayEdge` emits `restat` only when `edge.always && !action_restat`, which a naive generator would never vary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`mdtablefix --renumber` read a wrapped line beginning "72." as an ordered-list marker and rewrote it, truncating the sentence before it. No gate catches this: markdownlint and check-fmt both pass on the corrupted text. Rephrase so no bare number can land at the start of a wrapped line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A six-lens review found the first draft structurally unsound. The changes are substantial enough that reviewers should re-read rather than diff. Four findings reshaped the plan. The shared strategy cannot live in `test_support`: a compile probe proved a netsuke-typed value cannot cross into a `src/`-side test, so all strategies move to `src/`. The insertion-order property was not a deterministic function of its Proptest seed, because `RandomState` is outside it; that silently breaks shrinking and makes committed seeds inert, so the obligation is restructured around extracted pure ordering helpers. `docs/verification/mutations/` is governed by an existing contract test that rejects any patch not rooted at `ir`, so the proposed patches were unrepresentable. And `src/ninja_gen/mod.rs` is exactly at the 400-line ceiling, so the first draft breached a constraint on its first commit. Correct three false statements: `path_key` injectivity omitted the empty-path case; `OBL-ACTION` claimed a converse that is SHA-256 collision resistance; and `OBL-E2E`'s rule-permutation claim is false because duplicate rule names are last-writer-wins. Add `OBL-NOHASH` and `OBL-PROCESS`, which cover the stated success criterion and the published guarantee respectively — neither of which any first-draft obligation reached. Move the ADR to the first milestone so the contract is not fitted to the tests. Delete the light/heavy tiering on measured evidence: the suite costs about six seconds of CPU against a forty-five second budget. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
Contributor
Reviewer's GuideThis draft-only PR adds a detailed, approval-gated execution plan—not implementation—for Proptest coverage of deterministic Ninja emission, including the roadmap's ordering properties, inherited larger-N IR checks, mutation-backed evidence, an explicit determinism contract, and staged validation/documentation work. Sequence diagram for planned end-to-end determinism verificationsequenceDiagram
participant Test as Process test
participant Binary as Netsuke binary
participant Lowering as IR lowering
participant Emitter as Ninja emitter
participant FS as Temporary filesystem
Test->>Binary: generate
Binary->>Lowering: from_manifest
Lowering-->>Binary: BuildGraph
Binary->>Emitter: generate_bundle
Emitter-->>Binary: GeneratedNinja
Binary->>FS: write build.ninja and sidecars
Binary-->>Test: generated bytes
Test->>Binary: generate
Binary->>Lowering: from_manifest
Lowering-->>Binary: BuildGraph
Binary->>Emitter: generate_bundle
Emitter-->>Binary: GeneratedNinja
Binary->>FS: write build.ninja and sidecars
Binary-->>Test: generated bytes
Test->>Test: compare generated bytes
Flow diagram for planned Proptest verification milestonesflowchart TD
Approval[Approval gate] --> M0[EP-M0 feasibility spike]
M0 --> M1[EP-M1 determinism contract ADR]
M1 --> M2[EP-M2 refactors and make proptest]
M2 --> M3[EP-M3 shared graph strategy]
M3 --> M4[EP-M4 path_key and no-loss properties]
M4 --> M5[EP-M5 ordering and default properties]
M5 --> M6[EP-M6 hashing declaration order process output]
M6 --> M7[EP-M7 larger-N duplicate and cycle checks]
M7 --> M8[EP-M8 Ninja oracle documentation validation]
M8 --> Gates[All gates and mutation evidence pass]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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
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.
Summary
Draft execution plan for roadmap item 4.3.1, adding Proptest coverage for deterministic Ninja emission. The plan is for review and approval only — no implementation has been done.
Plan:
docs/execplans/4-3-1-proptests-for-deterministic-ninja-emission.mdScope
The three roadmap sub-items (insertion-order stability,
defaulttarget ordering,path_keyinvariance), plus the larger-N duplicate-output and cycle-rejection obligations that roadmap 4.2.1 and ADR-004 both explicitly deferred to this item. The plan also settles the determinism contract thatformal-verification-methods-in-netsuke.mdasks for but no roadmap item owns.Ten verification obligations, nine mutation patches, and eight milestones.
Design review
The first draft was reviewed through six expert lenses and then rewritten. Four findings reshaped it, each verified directly rather than taken on trust:
test_support. Proven by compile probe: passing anetsuke-typed value into asrc/-side#[cfg(test)]module fails with "multiple different versions of cratenetsukein the dependency graph", becausetest_supportdepends on the crate that dev-depends on it. All strategies moved tosrc/.RandomStateis outside Proptest's seed, so shrinking silently discards valid candidates and committed regression seeds are decorative. Restructured around extracted pure ordering helpers over explicitly shuffled vectors.docs/verification/mutations/is already governed by a contract test.tests/kani_mutation_evidence_tests.rsrejects any patch stem not rooted atir, so the proposedninja_genpatches were unrepresentable and would have failedmake test.src/ninja_gen/mod.rsis exactly 400 lines — theAGENTS.mdceiling — so the first draft breached a constraint on its first commit.Three obligation statements were also simply false and are corrected:
path_keyinjectivity omitted the empty-path collision (path_key([]) == path_key([""]));OBL-ACTIONclaimed a converse amounting to SHA-256 collision resistance; andOBL-E2E's rule-permutation claim is false becauseprocess_rulesis last-writer-wins on duplicate rule names.Two obligations were added because nothing in the first draft reached them: a source-shape contract test forbidding unordered-collection iteration in
src/ninja_gen/(the only thing that catches a future map), and a two-runassert_cmdbyte comparison (the guarantee the plan publishes, which nothing in the repository currently tests).Decisions taken with the user
proptestMake target, measure it, and split only if justified. Measured: the whole suite is ~6 s of CPU and 2–3 s wall against a 45-second budget, so no light/heavy split is planned. A standalone CI job would have been ~98% compile.Notes for reviewers
ADR-NNNplaceholder:adr-021is already claimed on four open branches andadr-020on two.EP-M0question 2 (does declaration-order invariance hold today?) gates the ADR's content and should be answered before approval if reviewers prefer.References
🤖 Generated with Claude Code
Summary by Sourcery
Approve a detailed execution plan for adding deterministic Ninja emission coverage and completing deferred larger-N IR verification obligations.
Enhancements:
Documentation:
Tests: