Skip to content

Kani cycle-canonicalization harnesses (4.2.2)#392

Open
lodyai[bot] wants to merge 11 commits into
mainfrom
4-2-2-kani-harnesses-for-cycle-canonicalization
Open

Kani cycle-canonicalization harnesses (4.2.2)#392
lodyai[bot] wants to merge 11 commits into
mainfrom
4-2-2-kani-harnesses-for-cycle-canonicalization

Conversation

@lodyai

@lodyai lodyai Bot commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Summary

This branch completes roadmap task 4.2.2 — Add Kani harnesses for cycle canonicalization.

It extracts a private production-owned canonicalization kernel in src/ir/cycle.rs, proves that kernel exhaustively with Kani over distinct small u8 cycles, and keeps the existing Utf8PathBuf wrapper as the production adapter. The completed execution plan is docs/execplans/4-2-2-kani-harnesses-for-cycle-canonicalization.md.

The original direct Utf8PathBuf proof route was measured and rejected under the required 8 GiB, five-minute cap. The approved proof claim is now narrower and cleaner: Kani proves the production canonicalization algorithm for N=2, N=3, and N=4 integer cycles; a direct adapter harness and the existing Proptest suite exercise the path-bearing instantiation.

Roadmap task: 4.2.2

Review walkthrough

Validation

  • make check-fmt: passed
  • make lint: passed
  • make test: passed
  • make markdownlint: passed
  • make nixie: passed
  • capped make kani-ir: passed with Complete - 13 successfully verified harnesses, 0 failures, 13 total.
  • focused mutation runs: all three recorded canonicalization mutations failed their matching Kani harnesses, then passed again after reversal
  • coderabbit review --agent: zero findings for the scaffold, kernel, proof-boundary, and final branch reviews

Notes

Direct Utf8PathBuf property proofs for N=2 through N=4 remain a measured non-goal for this branch. They exceeded the agreed resource envelope even after the ID-retaining salvage encoding. The branch avoids a Kani-only duplicate by moving the algorithm into private production code and proving that implementation directly over a tractable u8 instantiation.

Draft the approval-gated execution plan for roadmap item 4.2.2, which adds
Kani harnesses proving the canonicalize_cycle normalisation contract:
preserved length and closed-cycle output, interior-node multiset
preservation, and a stable smallest-first start node.

The plan harnesses the pure canonicalize_cycle end-to-end over fixed-length
closed cycles built from single-byte symbolic node names, reusing the
existing cfg(kani) path_cmp helper, and splits by node count (N in 2..=4).
It adds a rotation strengthening assertion so the suite is non-vacuous
against order-scrambling bugs the three roadmap properties alone would
miss, and records mutation-discipline patches per harness.

Planning used a Plan agent for the harness design, a Firecrawl-backed
research agent for Kani prior art, and a Logisphere community-of-experts
review that drove the rotation strengthening, the unwrap-free symbolic_node,
the unwind-bound reconciliation, and the honest multiset co-proof note.

Status: DRAFT; awaits approval before implementation.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Added approval-gated ExecPlan coverage for cycle canonicalisation and updated the related ADR, roadmap, and developer docs to match the approved proof boundary.

The main implementation change extracts a private generic canonicalize_cycle_by<T> kernel in src/ir/cycle.rs, keeps canonicalize_cycle(Vec<Utf8PathBuf>) as the production adapter, and adds property tests plus Kani harnesses in src/ir/cycle_verification.rs to prove the kernel over small bounded u8 cycles (N=2..4) and the two-node wrapper case.

Also recorded the verification and mutation results in the roadmap/ADR docs, and added the new ExecPlan document docs/execplans/4-2-2-kani-harnesses-for-cycle-canonicalization.md to capture the final proof strategy and acceptance criteria.

Walkthrough

Cycle canonicalisation now uses generic comparator-driven helpers in src/ir/cycle.rs, with unit tests for the new u8-based path. Kani harnesses verify bounded cycle properties and a two-node wrapper equivalence, while the ADR, roadmap, developers guide, and ExecPlan record the proof boundary.

Changes

Cycle canonicalisation Kani coverage

Layer / File(s) Summary
ExecPlan and roadmap records
docs/execplans/4-2-2-kani-harnesses-for-cycle-canonicalization.md
The new ExecPlan records the bounded canonicalisation scope, staged plan, constraints, validation criteria, and revision history for roadmap item 4.2.2.
Generic kernel and unit tests
src/ir/cycle.rs, src/ir/cycle_property_tests.rs
Comparator-driven rotation helpers and canonicalisation now replace the path-specific helpers, and unit tests cover smallest-start rotation and cycle direction preservation.
Kani harnesses and mutation fixtures
src/ir/cycle_verification.rs, docs/verification/mutations/*.patch
The verification module adds bounded harnesses for 2-, 3-, and 4-node cycles plus a two-node wrapper equivalence check, with helpers for symbolic IDs, multiset preservation, smallest-start selection, and rotation matching; the mutation patches target the same contracts.
ADR, roadmap, and guide updates
docs/adr-004-bound-kani-ir-harnesses-to-small-n.md, docs/roadmap.md, docs/developers-guide.md
The ADR, roadmap, and developers guide now describe the bounded kernel proof boundary, the wrapper adapter, the larger Proptest coverage, and the updated harness inventory.

Possibly related PRs

  • leynos/netsuke#163: Shares the src/ir/cycle.rs canonicalisation refactor that this PR extends with bounded Kani coverage and supporting documentation.

Suggested labels

Roadmap

Suggested reviewers

  • codescene-delta-analysis

Poem

A cycle turns with measured grace,
Small proofs find their rightful place.
Kernels, wrappers, docs align,
Bound and tidy, line by line.

🚥 Pre-merge checks | ✅ 20
✅ Passed checks (20 passed)
Check name Status Explanation
Title check ✅ Passed Keep this title; it matches the cycle-canonicalisation harness work and includes roadmap item 4.2.2.
Description check ✅ Passed Keep this description; it clearly matches the cycle-canonicalisation harness changes and proof boundary.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Testing (Overall) ✅ Passed PASS: New tests are substantive—proptests cover idempotence, rotations, smallest-start and closure, and Kani proves the small kernel plus wrapper on symbolic cycles.
User-Facing Documentation ✅ Passed PASS: The new cycle-canonicalisation work is internal-only, and the exec plan explicitly says to keep docs/users-guide.md unchanged.
Developer Documentation ✅ Passed Approve it: the developers' guide, ADR, roadmap, and execplan all record the new canonicalize_cycle_by boundary, and roadmap 4.2.2 is checked off.
Module-Level Documentation ✅ Passed Approve: every touched Rust module has a top-level //! docstring that states its purpose, and the new test/verification modules link back to cycle.rs.
Testing (Unit And Behavioural) ✅ Passed PASS: Property tests cover idempotence, rotations, closure, smallest-start, missing-dependency and repeated-traversal cases; Kani harnesses hit the production contains_cycle/canonicalize_cycle...
Testing (Property / Proof) ✅ Passed PR adds proptest invariants and Kani proofs for cycle canonicalization, with a private kernel, wrapper-adapter check, and mutation tests that break the proof.
Testing (Compile-Time / Ui) ✅ Passed Mark this PASS: the PR adds runtime/Kani harnesses and unit tests for cycle logic, not compile-time or UI output, so trybuild/snapshot coverage is not applicable.
Unit Architecture ✅ Passed PASS — the new canonicalization kernel is private and comparator-injected, and the harnesses/tests stay isolated from I/O, time, network, and ambient state.
Domain Architecture ✅ Passed PASS: canonicalize_cycle_by isolates the ordering kernel from path handling, while BuildGraph already models targets as Utf8PathBuf; the Kani harness stays in *_verification.rs.
Observability ✅ Passed Only algorithmic/docs/test changes landed; no new production logs, metrics, traces, or alerts were added or altered.
Security And Privacy ✅ Passed PASS: The changes only add bounded cycle-canonicalisation harnesses and docs; no secrets, auth/permission changes, unsafe sinks, or sensitive-data exposure appeared.
Performance And Resource Use ✅ Passed PASS: Keep the new loops bounded (2–4 in Kani, small proptest sizes) and retain the existing linear cycle canonicalisation with one owned Vec allocation per detected cycle.
Concurrency And State ✅ Passed No concurrency or shared-state change was introduced; the new Kani harnesses are single-threaded, and CycleDetector already owns and resets its traversal state.
Architectural Complexity And Maintainability ✅ Passed PASS: Keep the new private kernel small and local; it removes duplication, serves the wrapper and Kani harnesses immediately, and the docs state the bound and reuse path.
Rust Compiler Lint Integrity ✅ Passed No dead_code/unused suppressions or artificial refs were added; the new clones are small, intentional, and the Kani module stays narrowly cfg-gated.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 4-2-2-kani-harnesses-for-cycle-canonicalization

Comment @coderabbitai help to get the list of available commands.

@sourcery-ai

sourcery-ai Bot commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Reviewer's Guide

Adds an approval-gated ExecPlan document describing how to introduce Kani harnesses for the private canonicalize_cycle function, without changing any production code, and specifying constraints, risks, stages of work, and harness design details.

Sequence diagram for planned Kani harness execution for canonicalize_cycle

sequenceDiagram
    participant Kani as Kani_driver
    participant Harness as canonicalize_two_node_cycle_is_canonical
    participant Cycle as canonicalize_cycle

    Kani->>Harness: execute_proof_harness()
    Harness->>Harness: closed_cycle_n(2)
    Harness->>Cycle: canonicalize_cycle(input_cycle)
    Cycle-->>Harness: output_cycle
    Harness->>Harness: count_byte(in_interior, sym)
    Harness->>Harness: count_byte(out_interior, sym)
    Harness->>Harness: is_rotation_of(out_interior, in_interior)
    Harness->>Harness: path_cmp(first_node, node)
    Harness->>Kani: assertion_results
Loading

File-Level Changes

Change Details Files
Introduce a detailed ExecPlan for Kani harnesses targeting canonicalize_cycle, including constraints, staged implementation plan, harness design, risks, and mutation-discipline strategy.
  • Document purpose, scope, and success criteria for 4.2.2 Kani canonicalization harnesses, clarifying that only documentation is added and no code changes occur yet.
  • Define strict constraints and tolerances around API stability, file scope, Kani usage, Clippy lint compliance, and build/test gating.
  • Describe risks and mitigations related to Kani modelling (symbolic vectors, strings, unwind bounds) and bounded proof coverage.
  • Lay out a multi-stage plan (Stages A–F) for scaffolding, implementing, refactoring, mutating, validating, and integrating new harnesses into existing tooling and docs.
  • Specify harness design: one harness per node-count N (2–4), using fixed-length closed cycles built from single-byte symbolic nodes, asserting length/closure, interior multiset preservation, stable minimal start node, and an additional rotation-preservation check.
  • Define mutation-discipline patches per harness and how they are applied/reverted to show each harness is load-bearing.
  • Update developer-facing documentation plans (developers-guide harness inventory, ADR-004 extension, roadmap updates) and reference related docs and external Kani resources.
docs/execplans/4-2-2-kani-harnesses-for-cycle-canonicalization.md

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

codescene-delta-analysis[bot]

This comment was marked as outdated.

leynos added 2 commits June 22, 2026 23:52
Add the red-stage harness names for cycle canonicalization so `cargo kani
list` discovers the planned two-, three-, and four-node proofs before their
real assertions are implemented.

Record the Stage A approval, Kani library-path finding, red Kani evidence,
deterministic gate results, and CodeRabbit review outcome in the ExecPlan.
Replace the red canonicalization harness assertions with the first real
length-and-closure proof shape and keep the execplan current with the
resource-capped Kani findings.

Document that N=2 verifies under the user-scope cap, while the direct
N=3 `Vec<Utf8PathBuf>` proof reaches the 8G memory cap. Also record the
rejected concrete-enumeration fallback so the next implementation pass
starts from the measured blocker rather than repeating it.
codescene-delta-analysis[bot]

This comment was marked as outdated.

@leynos

leynos commented Jun 23, 2026

Copy link
Copy Markdown
Owner

@coderabbitai are you satisfied that the introduced proof is substantive, rigorous, and well-founded?

@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== ExecPlan 4.2.2 ==="
cat docs/execplans/4-2-2-kani-harnesses-for-cycle-canonicalization.md

echo ""
echo "=== src/ir/cycle.rs ==="
cat src/ir/cycle.rs

echo ""
echo "=== Predecessor ExecPlan 4.2.1 (for comparison) ==="
cat docs/execplans/4-2-1-kani-harnesses-for-manifest-to-ir-safety-checks.md 2>/dev/null || echo "(not found)"

echo ""
echo "=== cycle_property_tests.rs (if present) ==="
fd -e rs -g '*cycle_property*' . | head -5 | xargs cat 2>/dev/null || echo "(not found)"

Length of output: 173273



@leynos: Here is my full assessment.


Assessment: substantive, rigorous, and well-founded?

Short verdict

The plan is substantive and architecturally well-founded. The rigour is mostly sound but has one unresolved blocking issue and one internal inconsistency that should be corrected before Stage C can close.


Strengths

Target selection. canonicalize_cycle is the strongest narrow proof candidate in the codebase: pure function, no HashMap, no serde, no recursion, no Fluent, allocation-light. Harnessing it end-to-end at its natural contract boundary is the right call, and is consistent with the "narrowest function that still proves the contract" rule from ADR-004.

Property completeness. The three roadmap properties (length+closure, interior multiset, stable smallest-first) directly match the documented contract. The Logisphere-driven addition of the rotation-strengthening assertion (∃k: out[i] == in[(i+k) % N]) is a genuine improvement: length, closure, multiset, and smallest-first together do not pin cyclic order, so an order-scrambling bug could satisfy all three roadmap properties vacuously. The rotation assertion closes that gap.

Honest co-proof documentation. The plan records openly that the interior-multiset property is co-proven rather than independently falsifiable by a small mutation, and assigns a clean falsification path to the rotation assertion instead. This is more intellectually honest than inventing a contrived isolation patch.

Input construction discipline. Building closed cycles by construction (vec![n0, n1, ..., n0]) rather than using kani::assume on a symbolic vector correctly satisfies the debug_assert! precondition without wasted symbolic search, and avoids the classical Kani hazard of assuming a symbolic heap structure into shape.

Input-consumption handling. canonicalize_cycle consumes its argument by value and calls cycle.pop(), so the plan mandates capturing the interior into owned values before the call and passing a clone. This is necessary and correct.

Tie-handling. Admitting repeated node bytes and using != Ordering::Greater for the stable-start assertion correctly matches find_rotation_start's implementation, which retains the first minimum index rather than requiring strict uniqueness.

Mutation discipline. The three patches target three distinct code paths (find_rotation_start, rotate_cycle, rotate_index) and the first patch correctly isolates the stable-start assertion alone — length, closure, multiset, and rotation all still hold under that mutation, so only the smallest-first check fires. This is non-trivial and correct.

Proptest cross-reference. Explicitly delegating N≥5 to cycle_property_tests.rs (which already covers idempotence, rotation-invariance, smallest-first, and closure to N=8–10) is architecturally sound and consistent with the ADR-004 discipline.


Concerns

1. Stage C is blocked — no resolution committed (blocking)

The Progress section records this explicitly: the direct end-to-end canonicalize_cycle proof verifies N=2 in 177 seconds but N=3 exhausts the 8 GiB memory cap under both the N-sized symbolic alphabet and concrete finite enumeration. Four resolution paths are listed in the Surprises section but none has been committed to. The PR is a plan document, so this is not a defect in the plan per se, but Stage C cannot proceed until a decision is made. Recommend adding a Stage A-style approval gate for this decision to the Decision Log before Stage C restarts.

The 177-second N=2 runtime is itself a signal worth flagging: for two symbolic bytes and a deterministic three-node closed vector, that is slow. It suggests the solver is exploring the full Utf8PathBuf allocation model rather than the single-byte abstraction alone. Confirm that the concrete selector approach (e.g. match kani::any() { 0 => path("a"), 1 => path("b"), _ => { kani::assume(false); path("a") } }) is in place in the current cycle_verification.rs — if path still calls Utf8PathBuf::from(String::...) the string model may be leaking through.

2. symbolic_node in the Interfaces section is stale (should be corrected)

The Interfaces section still shows:

fn symbolic_node() -> Utf8PathBuf {
    let b: u8 = kani::any();
    kani::assume(b >= b'a' && b <= b'c');
    Utf8PathBuf::from((b as char).to_string())
}

The Surprises section records that this approach was rejected because Kani explores symbolic character and string encoding, and the implementation was changed to a symbolic selector over concrete path("a") through path("d") values. Update the Interfaces section to reflect the accepted implementation; the stale snippet will mislead any agent or reviewer picking up Stage C.

3. Unwind bounds should be rechecked after selector refactor

The plan derives unwind bounds as max(N, alphabet_len) + 1. With the selector approach the alphabet size is N (one symbol per interior node), so for N=2 the bound is max(2, 2) + 1 = 3, yet the plan proposes unwind(5). That headroom is conservative and safe, but ensure the bound covers the is_rotation_of nested loop (N outer iterations × N inner iterations for the existence check) — the longest sequential loop in that harness is unrolled steps, not N. For N=2 that is still 4, so unwind(5) is correct; for N=4 that would be 16 steps, which may need a higher per-harness #[kani::unwind] override than the plan's proposed unwind(7). Recalculate before Stage C restarts.


Summary table

Criterion Assessment
Target function is the right candidate
Properties match the contract
Rotation strengthening is non-vacuous
Input construction is correct
Mutation isolation is sound
Proptest hand-off is appropriate
N=2 verified, N≥3 tractable ✗ — blocked; decision required
Interfaces section reflects current implementation ✗ — stale symbolic_node snippet
Unwind bounds safe for is_rotation_of ⚠ — recheck for N=4

The plan is worth approving as a design document. Commit a resolution decision for the N≥3 tractability issue and update the Interfaces section before Stage C proceeds.

leynos added 2 commits June 23, 2026 13:38
Align the ExecPlan and pull request description with the committed
harness work and measured tractability blocker.

Add the Stage C approval gate for selecting the proof boundary, update
stale helper and alphabet examples, correct the unwind discussion to use
per-loop bounds, and rewrite acceptance criteria so they describe the
approved boundary rather than assuming every direct `Utf8PathBuf` proof
will pass.
Document the capped N=2 salvage experiment result for cycle
canonicalization. The ID-retaining direct proof shape still exceeds the
8 GiB Kani memory envelope before N=3 can be attempted, so record the
run as a tractability finding rather than a completed proof boundary.
codescene-delta-analysis[bot]

This comment was marked as outdated.

leynos added 4 commits June 23, 2026 16:03
Update the 4.2.2 ExecPlan to reflect the approved production-owned
kernel boundary. Keep the public API closed while allowing private
production refactoring of cycle canonicalization for legibility,
testability, and Kani tractability.
Move cycle rotation and closure into a private generic production helper
and keep `canonicalize_cycle` as the path-bearing wrapper. This gives
Kani a production-owned integer instantiation without adding a public
verification port or a separate model.

Update the canonicalization harnesses to prove length, closure,
interior multiset preservation, smallest-start selection, and rotation
for distinct `u8` cycles at N=2, N=3, and N=4. Add a small direct path
wrapper harness so the `Utf8PathBuf` adapter and comparator remain
covered by Kani.
Document the approved private production-kernel proof boundary for
cycle canonicalization and add mutation patches for the three property
harnesses.

Tighten the Kani closure helper so empty output is not treated as
closed, and record the ADR/developer-guide evidence tying the `u8`
kernel proof back to the path-bearing wrapper.
Mark roadmap item `4.2.2` complete against the approved private
production-kernel boundary.

Record the final deterministic gate evidence and the capped `make kani-ir`
run, which verified 13 IR harnesses with zero failures.
@lodyai lodyai Bot changed the title Add ExecPlan for Kani cycle-canonicalization harnesses (4.2.2) Implement Kani cycle canonicalization harnesses (4.2.2) Jun 23, 2026
Mark the cycle-canonicalization ExecPlan complete after the final
CodeRabbit review returned zero findings and the draft pull request was
updated to describe the implemented proof boundary.
codescene-delta-analysis[bot]

This comment was marked as outdated.

@leynos leynos changed the title Implement Kani cycle canonicalization harnesses (4.2.2) Kani cycle-canonicalization harnesses (4.2.2) Jun 23, 2026
@leynos

leynos commented Jun 23, 2026

Copy link
Copy Markdown
Owner

@coderabbitai following the latest commit, are you satisfied that the introduced proof is substantive, rigorous, and well-founded?

@leynos

leynos commented Jun 23, 2026

Copy link
Copy Markdown
Owner

@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph.

If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced.

src/ir/cycle_verification.rs

Comment on file

//! Kani harnesses for bounded IR cycle-detection properties.
//! Kani harnesses for bounded IR cycle-handling properties.

❌ New issue: Primitive Obsession
In this module, 69.4% of all function arguments are primitive types, threshold = 30.0%

@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== src/ir/cycle.rs ==="
cat src/ir/cycle.rs

echo ""
echo "=== src/ir/cycle_verification.rs ==="
cat src/ir/cycle_verification.rs

echo ""
echo "=== src/ir/cycle_property_tests.rs ==="
cat src/ir/cycle_property_tests.rs

echo ""
echo "=== ExecPlan 4.2.2 ==="
cat docs/execplans/4-2-2-kani-harnesses-for-cycle-canonicalization.md

echo ""
echo "=== ADR-004 ==="
cat docs/adr-004-bound-kani-ir-harnesses-to-small-n.md 2>/dev/null || echo "(not found)"

echo ""
echo "=== Mutation patches ==="
find docs/verification/mutations -type f | sort | while read -r f; do
  echo "--- $f ---"
  cat "$f"
  echo ""
done

Length of output: 134001



@leynos: Yes — I am now satisfied on all three dimensions.


Revised assessment

Resolved concerns from the previous review

N≥3 tractability (was blocking). The private canonicalize_cycle_by<T> kernel extraction is the correct resolution. Kani now verifies N=2 in 4.6 s, N=3 in 7.6 s, and N=4 in 11.8 s — all comfortably within the 5-minute, 8 GiB cap. The production algorithm owns one implementation; no Kani-only duplicate exists.

Stale symbolic_node snippet in the Interfaces section. The section now correctly documents the symbolic_id/name_for_id pattern that is actually in the code. The CycleInput struct and the per-N closed-cycle builders are described accurately.

Unwind bounds for the nested rotation check. The deepest single loop at N=4 is 4 iterations; every loop in the proof path (the outer output_is_rotation, the inner output_matches_rotation, assert_multiset_preserved, count_id, count_output_id, rotate_cycle_by, find_rotation_start_by) needs at most unwind(5). The harnesses carry #[kani::unwind(6)] and the manifest sets default-unwind = "6" — both are above that bound. No harness is under-unwound.


Positive observations on the final state

Five load-bearing assertions per kernel harness. Length, closure, interior-multiset (per-symbol count), smallest-start, and rotation-existential are all present. The rotation assertion closes the order-scrambling gap flagged in the previous review; the multiset assertion is honestly documented as co-proven.

is_closed_id_cycle is correctly strengthened. The pattern-match on (Some(first), Some(last)) with _ => false means an empty canonical output is never silently classified as closed. The closure assertion is independently meaningful during mutation failures.

Pairwise distinctness is complete. N=2 constrains 1 pair, N=3 constrains 3, N=4 constrains all 6. The kani::assume preconditions exactly match the production DFS invariant that interior cycle nodes are distinct.

Adapter harness is sound. name_for_id maps 0→"a", 1→"b", 2→"c", 3→"d" — four single-character paths whose byte order matches compare_u8's integer order under the #[cfg(kani)] single-byte path_cmp. Both two-node orderings are exercised concretely, avoiding symbolic string allocation in the adapter path.

Mutation discipline is clean. The two-node patch breaks find_rotation_start_by and isolates the smallest-start assertion (the other four properties still hold under that mutation). The three-node patch drops the closing push and breaks length plus closure. The four-node patch removes the wrap-around from rotate_index, breaking the rotation, multiset, and length assertions together. All three fail their matching harness; all three restore to pass on reversal.

Thirteen harnesses, zero failures, zero unresolved review findings. The final make kani-ir transcript and the final coderabbit review --agent pass both confirm the branch is clean.


Summary

Criterion Assessment
Target function is the right candidate
Properties match the contract
Rotation strengthening is non-vacuous
Input construction is correct
Pairwise distinctness constraints are complete
Unwind bounds are safe for all loops at N=4
is_closed_id_cycle handles empty output
Adapter harness ordering is sound
Mutation isolation is sound
Proptest hand-off is appropriate
N=2, 3, 4 all verify within the resource cap
Interfaces section reflects current implementation

The proof is substantive, rigorous, and well-founded. No outstanding concerns.

@coderabbitai

This comment was marked as resolved.

codescene-delta-analysis[bot]

This comment was marked as outdated.

Expand the roadmap entry for 4.2.2 with the settled private-kernel proof
boundary, final validation count, mutation evidence, and CodeRabbit review
state.

Record the documentary audit in the ExecPlan so the roadmap and plan agree on
the current implementation status.
@pandalump pandalump marked this pull request as ready for review June 25, 2026 21:39
@coderabbitai coderabbitai Bot added the Roadmap label Jun 25, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
docs/adr-004-bound-kani-ir-harnesses-to-small-n.md (1)

14-16: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Update the ## Date field to match the latest status entry.

The style guide defines the Date field as "the date the ADR was created or last updated." The Status section records an extension on 2026-06-23, so the Date field must be updated accordingly.

📝 Proposed fix
 ## Date
 
-2026-06-12.
+2026-06-23.

Triage: [type:docstyle]

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/adr-004-bound-kani-ir-harnesses-to-small-n.md` around lines 14 - 16, The
ADR’s Date field is out of sync with its latest status update. Update the `##
Date` entry in this document to the most recent ADR update date recorded in the
Status section, using the existing ADR heading and `Date` field as the only
place to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/adr-004-bound-kani-ir-harnesses-to-small-n.md`:
- Line 124: Fix the spelling in the affected documentation sentence by changing
“randomised” to “randomized” to match the required -ize form. Update the text in
the ADR markdown where the phrase about “exercise path-bearing canonicalization
up to the larger randomised bounds” appears, keeping the rest of the sentence
unchanged.

In `@docs/developers-guide.md`:
- Line 200: Add the missing comma before “so” in the sentence in the
documentation table entry, since it joins two independent clauses. Update the
wording in the affected docs row so the phrase around “This complements the
a-first harness” reads with a comma before “so,” keeping the rest of the
`two_node_cycle_reports_cycle_b_first` description unchanged.
- Line 202: The documentation entry uses the British spelling “synthesise” where
the project requires “synthesize”; update the wording in the affected table row
in docs/developers-guide.md and keep the rest of the description unchanged.

In `@docs/execplans/4-2-2-kani-harnesses-for-cycle-canonicalization.md`:
- Line 1124: The section heading currently uses the US spelling “Artifacts”
while the surrounding text uses the required en-GB form “artefacts”; update the
heading in the affected markdown section to “Artefacts and notes” so it matches
the body text and project spelling conventions.
- Around line 14-15: The documentation text uses inconsistent en-GB-oxendict
spelling for the cycle canonicalization description. Update the prose around
canonicalize_cycle to use -ize forms such as “normalization” and “normalize”
instead of “normalisation” and “normalise”, and make the same spelling
correction anywhere else in this section where the same words appear.

---

Outside diff comments:
In `@docs/adr-004-bound-kani-ir-harnesses-to-small-n.md`:
- Around line 14-16: The ADR’s Date field is out of sync with its latest status
update. Update the `## Date` entry in this document to the most recent ADR
update date recorded in the Status section, using the existing ADR heading and
`Date` field as the only place to change.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 8efe8414-479a-476d-a0cf-671935a75ba8

📥 Commits

Reviewing files that changed from the base of the PR and between 41ce67e and 1a6059e.

📒 Files selected for processing (10)
  • docs/adr-004-bound-kani-ir-harnesses-to-small-n.md
  • docs/developers-guide.md
  • docs/execplans/4-2-2-kani-harnesses-for-cycle-canonicalization.md
  • docs/roadmap.md
  • docs/verification/mutations/ir__cycle__verification__canonicalize_four_node_cycle_is_canonical.patch
  • docs/verification/mutations/ir__cycle__verification__canonicalize_three_node_cycle_is_canonical.patch
  • docs/verification/mutations/ir__cycle__verification__canonicalize_two_node_cycle_is_canonical.patch
  • src/ir/cycle.rs
  • src/ir/cycle_property_tests.rs
  • src/ir/cycle_verification.rs

while `canonicalize_cycle(Vec<Utf8PathBuf>)` remains the production wrapper
using the path comparator. A small direct adapter harness checks the
wrapper/kernel connection for two-node path cycles, and Proptest continues to
exercise path-bearing canonicalization up to the larger randomised bounds.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Replace "randomised" with "randomized".

en-GB-oxendict mandates the -ize suffix for this verb.

📝 Proposed fix
-earing canonicalization up to the larger randomised bounds.
+earing canonicalization up to the larger randomized bounds.

Triage: [type:spelling]

🧰 Tools
🪛 LanguageTool

[style] ~124-~124: Would you like to use the Oxford spelling “randomized”? The spelling ‘randomised’ is also correct.
Context: ...aring canonicalization up to the larger randomised bounds. ## Known risks and limitations...

(OXFORD_SPELLING_Z_NOT_S)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/adr-004-bound-kani-ir-harnesses-to-small-n.md` at line 124, Fix the
spelling in the affected documentation sentence by changing “randomised” to
“randomized” to match the required -ize form. Update the text in the ADR
markdown where the phrase about “exercise path-bearing canonicalization up to
the larger randomised bounds” appears, keeping the rest of the sentence
unchanged.

Source: Path instructions

Comment thread docs/developers-guide.md
| `missing_rule_shape_is_rejected` | `src/ir/from_manifest_verification.rs` | A missing single rule reaches `IrGenError::RuleNotFound` and preserves target and rule names. | `#[kani::unwind(6)]` | Drives production `resolve_rule` with symbolic target and rule names and an empty rule map. |
| `self_dependency_reports_cycle` | `src/ir/cycle_verification.rs` | A self-dependency is reported as a cycle by production traversal. | `#[kani::unwind(5)]` | Drives production `contains_cycle`, which reuses `CycleDetector::visit` in boolean mode. |
| `two_node_cycle_reports_cycle_a_first` | `src/ir/cycle_verification.rs` | A two-node cycle is reported when the `a` node is inserted first. | `#[kani::unwind(5)]` | Drives production `contains_cycle`; the separate insertion-order harnesses cover deterministic map-entry traversal under the Kani map. |
| `two_node_cycle_reports_cycle_b_first` | `src/ir/cycle_verification.rs` | A two-node cycle is reported when the `b` node is inserted first. | `#[kani::unwind(5)]` | Drives production `contains_cycle`; this complements the `a`-first harness so the proof is not tied to one insertion order. |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add a comma before "so" — it links two independent clauses.

📝 Proposed fix
-| `two_node_cycle_reports_cycle_b_first`                      | `src/ir/cycle_verification.rs`         | A two-node cycle is reported when the `b` node is inserted first.                                       | `#[kani::unwind(5)]`  | Drives production `contains_cycle`; this complements the `a`-first harness so the proof is not tied to one insertion order.                                               |
+| `two_node_cycle_reports_cycle_b_first`                      | `src/ir/cycle_verification.rs`         | A two-node cycle is reported when the `b` node is inserted first.                                       | `#[kani::unwind(5)]`  | Drives production `contains_cycle`; this complements the `a`-first harness, so the proof is not tied to one insertion order.                                              |

Triage: [type:grammar]

🧰 Tools
🪛 LanguageTool

[uncategorized] ~200-~200: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...; this complements the a-first harness so the proof is not tied to one insertion ...

(COMMA_COMPOUND_SENTENCE_2)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/developers-guide.md` at line 200, Add the missing comma before “so” in
the sentence in the documentation table entry, since it joins two independent
clauses. Update the wording in the affected docs row so the phrase around “This
complements the a-first harness” reads with a comma before “so,” keeping the
rest of the `two_node_cycle_reports_cycle_b_first` description unchanged.

Comment thread docs/developers-guide.md
| `two_node_cycle_reports_cycle_a_first` | `src/ir/cycle_verification.rs` | A two-node cycle is reported when the `a` node is inserted first. | `#[kani::unwind(5)]` | Drives production `contains_cycle`; the separate insertion-order harnesses cover deterministic map-entry traversal under the Kani map. |
| `two_node_cycle_reports_cycle_b_first` | `src/ir/cycle_verification.rs` | A two-node cycle is reported when the `b` node is inserted first. | `#[kani::unwind(5)]` | Drives production `contains_cycle`; this complements the `a`-first harness so the proof is not tied to one insertion order. |
| `direct_missing_dependency_does_not_report_cycle` | `src/ir/cycle_verification.rs` | A single target with an absent dependency is not reported as a cycle. | `#[kani::unwind(6)]` | Drives production `contains_cycle` and proves that a missing direct dependency does not enter the cycle branch. |
| `transitive_missing_dependency_does_not_report_cycle` | `src/ir/cycle_verification.rs` | A two-target chain whose deeper dependency is absent is not reported as a cycle. | `#[kani::unwind(6)]` | Drives production `contains_cycle` and proves that an absent dependency below another target does not synthesise a false cycle. |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Replace "synthesise" with "synthesize".

en-GB-oxendict requires the -ize suffix.

📝 Proposed fix
-| `transitive_missing_dependency_does_not_report_cycle`       | `src/ir/cycle_verification.rs`         | A two-target chain whose deeper dependency is absent is not reported as a cycle.                        | `#[kani::unwind(6)]`  | Drives production `contains_cycle` and proves that an absent dependency below another target does not synthesise a false cycle.                                           |
+| `transitive_missing_dependency_does_not_report_cycle`       | `src/ir/cycle_verification.rs`         | A two-target chain whose deeper dependency is absent is not reported as a cycle.                        | `#[kani::unwind(6)]`  | Drives production `contains_cycle` and proves that an absent dependency below another target does not synthesize a false cycle.                                            |

Triage: [type:spelling]

🧰 Tools
🪛 LanguageTool

[style] ~202-~202: Would you like to use the Oxford spelling “synthesize”? The spelling ‘synthesise’ is also correct.
Context: ...ependency below another target does not synthesise a false cycle. ...

(OXFORD_SPELLING_Z_NOT_S)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/developers-guide.md` at line 202, The documentation entry uses the
British spelling “synthesise” where the project requires “synthesize”; update
the wording in the affected table row in docs/developers-guide.md and keep the
rest of the description unchanged.

Source: Path instructions

Comment on lines +14 to +15
of the pure normalisation function `canonicalize_cycle` in
[`src/ir/cycle.rs`](../../src/ir/cycle.rs). Canonicalization is what turns a

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Replace "-ise" spellings with "-ize" throughout.

Per en-GB-oxendict, words derived from the Greek -izo suffix take the "-ize" ending — "normalisation" and "normalise" must be "normalization" and "normalize". The same applies at lines 779 and 781.

✏️ Suggested corrections
-of the pure normalisation function `canonicalize_cycle` in
+of the pure normalization function `canonicalize_cycle` in

Line 779:

-`canonicalize_cycle` (lines ~440-447) normalises that raw witness so the
+`canonicalize_cycle` (lines ~440-447) normalizes that raw witness so the

Line 781:

-diagnostic is stable by delegating to the private generic
-`canonicalize_cycle_by` kernel with the production path comparator.
+diagnostic is stable by delegating to the private generic normalization
+`canonicalize_cycle_by` kernel with the production path comparator.

Triage: [type:spelling]

As per path instructions: "Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/execplans/4-2-2-kani-harnesses-for-cycle-canonicalization.md` around
lines 14 - 15, The documentation text uses inconsistent en-GB-oxendict spelling
for the cycle canonicalization description. Update the prose around
canonicalize_cycle to use -ize forms such as “normalization” and “normalize”
instead of “normalisation” and “normalise”, and make the same spelling
correction anywhere else in this section where the same words appear.

Source: Path instructions

small and per-property, so any stage can be rolled back with `git revert`
without disturbing the others.

## Artifacts and notes

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Correct "Artifacts" to "Artefacts" in the section heading.

The heading uses US spelling whilst the body text at line 1126 correctly uses the British form "artefacts". Align the heading with the body.

✏️ Suggested correction
-## Artifacts and notes
+## Artefacts and notes

Triage: [type:spelling]

As per path instructions: "Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/execplans/4-2-2-kani-harnesses-for-cycle-canonicalization.md` at line
1124, The section heading currently uses the US spelling “Artifacts” while the
surrounding text uses the required en-GB form “artefacts”; update the heading in
the affected markdown section to “Artefacts and notes” so it matches the body
text and project spelling conventions.

Source: Path instructions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant