Repository navigation
test(engine): make the chain planner's unobserved guards falsifiable - #226
Merged
Merged
Conversation
Four guards in the chain planner had no test that could fail: - `assertAcyclicPlan` was called only to prove it does not throw, so deleting its throw kept the suite green. Now three shapes must raise it, and the fan-in plus 5000-note cases keep the memo honest. - `chainRefused` ANDs three conditions and no single one was falsifiable. Removing each in turn now fails a named test; the one that needed a new fixture is the numbered-layout guard, which only shows up when a vault has numbering and nothing to order. - The roadmap wikilink gate is `palee_roadmap === true`, and nothing said which spellings that admits. Relaxing it to truthiness passed the whole suite, so `palee_roadmap: "no"` would have marked a document as a roadmap and imported it. - The `Skipped (…)` hygiene counts were entirely unasserted. `countFor` unions the scan's drops with the planner's, and the second term is only reachable through an already-adopted note - a fixture nobody had. The C2 dominance test was identity-weak: both of its paths are in the numbered tree, so the filter drops them before planning and the assertion compared the numbered plan with itself. Deleting the filter left it passing. The added case leads the enumeration with an unnumbered note, which is the re-parenting dominance forbids, and it fails when the filter goes. `planTocChain` computed a note's parent directory twice, inline and via `parentOf`; the grouping key and the lookup key are now one function, and an assert says so, matching the file's own `assertBackwardEdges` habit.
Three claims in the Unreleased section were wrong: - "the numbered tree … measured at 0 false edges out of 127". The 127-edge content census is the enumeration tier's; numbering never had that audit, so the strict default rests on listing order being a weak signal rather than on numbering being a proven one. - The C4 refusal bullet quoted a string the CLI no longer prints, and the refusal now has two wordings depending on why nothing was chained. - `assignment.es.md` was listed among names *excluded* as translations. A locale suffix on a README is an exclusion; on any other name it is only a guess about language, and the note is demoted to a leaf instead.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
documentation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The note it excluded was named `template.md`, which Tier-0 hygiene drops on its own, so the test could not say which of the two mechanisms bridged it over - and it read like coverage of the template rule while exercising the pattern filter. Renamed to a note hygiene leaves alone, with the two skip counts asserted so a hygiene drop can no longer pass as an exclusion.
… one The grouping-key assert I added re-derived `parentOf` over the same paths that populated `dirFirstSeen`, so it was a tautology. The dedupe is the real fix — grouping and lookup are one function now — and the comment says what genuinely holds rather than dressing it up as a check. Unlike `assertBackwardEdges`, which inspects data arriving from elsewhere and really can violate it. The spine-length test asserted a fixed 2000ms deadline, which is host-dependent and, worse, too loose to catch the regression it named: with the `done` memo removed, a 5000-node spine stayed well under it. Replaced with an equal-work comparison in the same process — 200 passes over 1000 nodes against 10 passes over 20000, both 2e5 node visits. Measured on the memo removed: 6067ms vs 333439ms, so the shape difference is caught rather than the clock being guessed at.
`idToPath` returns Map<paleeId, path>, so `ids.has('<path>')` was always false
and the assertion proved nothing — the test would have passed with `--exclude`
fully broken. Checked against the map's values and against the note itself, since
an adopted note carries a `palee_id`.
Verified by removing `--exclude` from the command the test runs: it now fails
with "--exclude was ignored and the note was adopted". It passed vacuously before.
…st/pal205-gate-hygiene # Conflicts: # test/cli-adopt-autochain.test.ts
This was referenced Sep 29, 2026
… one (#236) PAL-205 makes inferred position advisory, and `roadmap --auto-chain` synthesizes `depends_on` from the `order` field, so a reader can reasonably ask whether that synthesis is the next thing to demote. It is not: the learner typed the list into a file whose only job is to declare a plan, and an entry's own `depends_on` always overrides it. Nothing asserted the consequence, so a refactor routing synthesized edges through the advisory labels "for consistency" would have passed the suite while quietly turning every roadmap into a suggestion. The test asserts the written note carries no `depends_on_source` and that `palee plan` still withholds the second topic. Proven load-bearing by making `doImport` label synthesized edges `toc`: the test fails, and restoring the file returns it to green.
* fix(engine): stop letting a filename tie gate a note Two notes carrying the same numeric prefix are ordered by how their filenames sort, which is a decision the numbered tree explicitly did not make. That edge was filed under `numbered` and so gated: for `m/02-a.md` and `m/02-b.md`, `palee plan` offered only the first note. That is list position locking a learner out of a note - the defect PAL-205 contains for enumeration order, one level lower. Label those edges `tie` and add `tie` to the advisory set, keeping the fail-closed rule intact: an absent or unrecognized label still gates. The edge is still written, so ranking and cycle detection are unchanged, and `numberedEdgeCount` still includes it, because that count answers whether any order exists rather than whether it gates - dropping it there would have made the honest-refusal line claim a vault states no order. Both guards proven by reverting the deciding predicate: labelling a tie as `numbered` again fails the engine and CLI tests, and removing `tie` from the advisory set fails the readiness test and the same CLI test. * docs(changelog): consolidate the tie note and state its blast radius Two release notes described the same change, and neither said what happens to a vault that adopted before it. A stored tie edge carries `numbered`, which is indistinguishable from a numbering-decided edge on the note alone, and `adopt` never rewrites an already-adopted note - so the fix is forward-only and the notes said otherwise.
Kuldeep2822k
added a commit
that referenced
this pull request
Oct 12, 2026
Three sources carry three numbers (#226's measured 333439/6067 = 55.0×, the test comment's 'about 20x', the assertion's '< small * 5'). Name the measured pair so the claim is independently checkable.
Kuldeep2822k
added a commit
that referenced
this pull request
Oct 12, 2026
Three sources carry three numbers (#226's measured 333439/6067 = 55.0×, the test comment's 'about 20x', the assertion's '< small * 5'). Name the measured pair so the claim is independently checkable.
Kuldeep2822k
added a commit
that referenced
this pull request
Oct 12, 2026
…n ADR-0026 #226's 55× is the corrected-vs-quadratic ratio; the test comment's 'about 20x' is the memo-less large-leg-vs-small ratio. Name both and the 5× bound they clear, so a reader comparing ADR to test does not read a mismatch that is not one.
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.
Closes #225 (criteria 1–5; #225 stays open for the remaining three)
Stacked on #223 — the
chainRefusedand refusal-wording cases only exist there, so this branch targetsfix/palee-gating-policy.What this does
Five guards in the chain planner could not fail a test. Each was proven dead by neutralising its deciding predicate and watching the suite stay green, then given a case that does fail. #225 has the table.
The three worth a reviewer's time:
palee_roadmap === true. Relaxing it toBoolean(...)— which would letpalee_roadmap: "no"declare a document as a roadmap and import it — passed all 26 parser tests. Probed the frontmatter parser to establish what the strict comparison actually admits (true,True; not"true", not"yes", not1), and the test now pins those spellings.Skipped (…)counts. Nothing asserted any line of that block.countForunions notes the scan dropped with notes the planner excluded, and the second term is only reachable through an already-adopted note — a fixture the suite never had. My first version of the test passed, then failed to fail under mutation, which is how the unreachable half was found.planTocChainalso computed a note's parent directory twice, inline and viaparentOf. Grouping and lookup now share one function. The assert I first added beside it re-derived the same key over the same paths and so could not fail; it is removed in 661d6ef. That commit also replaces the spine-length test, whose fixed 2000ms deadline was host-dependent and too loose to catch the regression it named — with thedonememo deleted, the old test still passed. It now compares equal work inside one process (200 passes over 1000 nodes against 10 over 20000); the memo-less build fails reporting 6067ms vs 333439ms.CHANGELOG
Second commit retracts a figure I put in #223's changelog entry: "the numbered tree … measured at 0 false edges out of 127". The 127-edge content census is the enumeration tier's. Numbering never had that audit, so the strict default rests on listing order being a weak signal, not on numbering being a proven one. The same sentence in #223's PR description has been corrected on GitHub. The other two retractions (a refusal string the CLI no longer prints;
assignment.es.mddescribed as excluded when a locale suffix on a non-README name demotes it to a leaf) were checked againstclassifyNoteForChainbefore being rewritten.Verification
Full suite: 1195 tests, 0 failures, 1 pre-existing skip (+9 over #223).
typecheckandlintclean. CI does not run on this base — it is stacked on an unmerged branch, so the local runs are the evidence; every mutation result quoted above came from running the named test against the neutralised predicate, and each guard also has an accepting case so it is not merely over-eager.One process note worth stating: an earlier report in this arc claimed a docstring-coverage gate at 80%. There is no such gate — no jsdoc rules in eslint, no workflow measuring it. #225 records that so the number is not mistaken for a failing check.
The PR appears safe to merge; no outstanding finding or new actionable issue was identified.
Summary
This PR strengthens regression tests for chain-planner guards and removes a redundant computation in TOC directory grouping. It also corrects changelog descriptions of hygiene, refusal wording, and the scope of an earlier edge audit.
Reviews (3) · Last reviewed commit: "Merge remote-tracking branch 'origin/fix..."