-
Notifications
You must be signed in to change notification settings - Fork 0
W1 follow-up: both reviewers' findings on #1120 — admission vs replay, a checked reservation, a reversed doc #1122
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
50c1ceb
e8241c4
cdbf108
7c3f476
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,60 @@ | ||
| ## 2026-08-31 — E-TWO-REVIEWERS-FOUND-THE-SAME-THREE-DEFECTS-AND-ONE-OF-THEM-WAS-MINE-ALONE-1 | ||
|
|
||
| **Status:** FINDING — #1120's full review surface, read after merge. | ||
| **Confidence:** measured — 7 findings across two independent reviewers on the | ||
| same diff; each adjudicated against the code, two remedies rejected with | ||
| evidence. | ||
|
|
||
| #1120 drew **7 findings from two reviewers that never see each other's | ||
| output** — codex (3) and CodeRabbit (4). Their overlap is the interesting | ||
| part, and so is the one place they diverge. | ||
|
|
||
| | line | codex | CodeRabbit | verdict | | ||
| |---|---|---|---| | ||
| | 179 | predicate dropped from the trace | reject an out-of-band ordinal | **same defect, two different remedies** | | ||
| | 183 | overlapping durable coordinates | `cast_seq` overflow at `u64::MAX` | **same field, two distinct bugs — both real** | | ||
| | 199/200 | compare only edges vs the doc's promise | identical finding | **agreed, and both remedies wrong** | | ||
| | board | — | `cast_seq` described as caller-supplied | **CodeRabbit alone; correct** | | ||
|
|
||
| **Independent agreement is evidence; identical remedies are not.** Both | ||
| reviewers proposed comparing whole `ReplayTraceRow` values in | ||
| `first_divergence`. Running it: the same chain replayed from durable base 100 | ||
| vs 900 is declared divergent at step 0 — the same witness at two addresses. | ||
| Two reviewers converging on a fix does not make the fix right, and the cost of | ||
| checking was one test run. | ||
|
|
||
| **The 179 pair is the sharper lesson.** Both saw that the ordinal was | ||
| mishandled; they disagreed about *which* mishandling. Codex: it is dropped | ||
| from the witness (so a recorded program cannot be reconstructed). CodeRabbit: | ||
| it is not validated (so a chain carrying `0xA3`, the SEARCH band, replays as | ||
| if causal). **Both are true and the fixes go in opposite directions** — carry | ||
| it *more* faithfully, and judge it *before* it is carried. Taking either alone | ||
| would have looked complete. | ||
|
|
||
| The split that resolves it: **replay must not refuse history.** A recorded | ||
| chain is a fact; the engine reproduces it, it does not judge it. If the palette | ||
| later drops or renumbers an ordinal, a validating replay starts returning `Err` | ||
| for chains that were valid when recorded — destroying the very property the | ||
| wave exists to hold. So validation is an ADMISSION check (`validate_chain`, | ||
| constant per chain, checked once) and replay stays total over admitted chains, | ||
| while the witness carries the ordinal faithfully. Both findings satisfied, | ||
| neither remedy taken literally. | ||
|
|
||
| **The board finding is the one only a doc-reader caught.** The status row said | ||
| *"`cast_seq` is caller-supplied and durable, never minted"* — reversing the | ||
| contract, since the caller supplies `base_seq` and the planner DERIVES | ||
| `cast_seq`. No code reviewer would flag it; no test could fail on it. It is | ||
| exactly the class of error that survives forever because it lives where nothing | ||
| executes. | ||
|
|
||
| **Consequence for the review posture:** a second reviewer is not redundancy — | ||
| measured here, it contributed one finding neither the first reviewer nor any | ||
| gate could reach, and it disagreed usefully about a defect the first had | ||
| already found. And a finding's *remedy* is a proposal, never a verdict: 3 of 7 | ||
| remedies here were wrong or too literal, while 7 of 7 findings were real. | ||
|
|
||
| --- | ||
|
|
||
| ## 2026-08-31 — E-Q8-THE-SIX-DOES-NO-WORK-A-DEGREE-ABLATION-COLLAPSES-THE-HEX-OVERLAYS-ENTIRE-ADVANTAGE-1 — B passes every pre-registered gate and the pass is unattributable: at degree 1 it scores identically with 5.5x less memory | ||
|
|
||
| **Status:** FINDING [MEASURED] — full entry in | ||
|
|
@@ -21,6 +78,156 @@ nothing; B is a bigram successor table. | |
| Converges with `E-PALETTE256-IS-A-NEEDLE-THE-COLON-IS-THE-DISTRIBUTION-1` by | ||
| a different road: the information is in the PAIR, not the neighbourhood's | ||
| shape. One relation carried everything; five more neighbours added bytes. | ||
| ## 2026-08-31 — E-A-MONITOR-KEYED-ON-THE-PR-HEAD-CAN-CERTIFY-THE-WRONG-COMMIT-1 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: printf '%s\n' '--- applicable knowledge files ---'
find /tmp/coderabbit-repo-knowledge/adaworldapi-lance-graph-8c8fd2e8 -maxdepth 2 -type f -name '*.md' -print
printf '%s\n' '--- scoped convention/learnings ---'
for f in /tmp/coderabbit-repo-knowledge/adaworldapi-lance-graph-8c8fd2e8/{conventions,learnings}/*.md; do
[ -f "$f" ] || continue
case "$f" in
*board*|*epiphan*|*markdown*|*hygiene*) echo "### $f"; cat "$f";;
esac
done
printf '%s\n' '--- targeted entry headers ---'
rg -n '^#{1,6} ' .claude/board/EPIPHANIES.md | head -40
printf '%s\n' '--- surrounding sections ---'
sed -n '65,175p' .claude/board/EPIPHANIES.mdRepository: AdaWorldAPI/lance-graph Length of output: 14577 🏁 Script executed: printf '%s\n' '--- board rule sources ---'
find . -name 'CLAUDE.md' -o -name '.claude' | head -20
for f in CLAUDE.md .claude/CLAUDE.md; do
if [ -f "$f" ]; then
echo "### $f"
rg -n -A12 -B4 'append.only|newest.first|board|ledger|prepend|historical' "$f"
fi
done
printf '%s\n' '--- changed-file status ---'
git status --short -- .claude/board/EPIPHANIES.md
printf '%s\n' '--- zero-context diff ---'
git diff --unified=0 -- .claude/board/EPIPHANIES.md | sed -n '1,260p'
printf '%s\n' '--- first and transition headers ---'
sed -n '1,95p' .claude/board/EPIPHANIES.mdRepository: AdaWorldAPI/lance-graph Length of output: 34951 Prepend the two new epiphany entries.
🤖 Prompt for AI AgentsSource: Learnings |
||
|
|
||
| **Status:** FINDING — the RULE stands; the CAUSE first recorded here was | ||
| **wrong and is corrected below**, same day, before this entry ever merged. | ||
| **Confidence:** measured — GitHub's `pulls/1120` reported head `233ce3f1` for | ||
| >45 minutes while `git/ref/heads/…` (same API, same token, same request) already | ||
| had `216d8f2` and then `81820cc`; `mergeable` stayed `null` throughout. | ||
|
|
||
| > **⊘ CAUSE CORRECTED (same day, on discovering the merge).** This entry first | ||
| > blamed *"GitHub delivered no push event"* — an exotic infrastructure story. | ||
| > The real cause was mundane and one field away: **#1120 had already MERGED**, | ||
| > at 19:45:46, with head `233ce3f1`. A merged PR stops tracking its branch and | ||
| > stops running PR-triggered workflows, so the frozen head, the null | ||
| > `mergeable`, and the silent Actions were all *correct behaviour* for a closed | ||
| > PR — not a delivery stall. | ||
| > | ||
| > **How the wrong cause survived four checks:** an early query printed | ||
| > `pr["state"]` (`open`); every later query printed head, mergeable and | ||
| > check-runs and **dropped `state`**. Each subsequent read was consistent with | ||
| > both hypotheses — "GitHub is stalling" and "the PR is closed" — and I never | ||
| > re-read the one field that separates them. I then wrote an increasingly | ||
| > specific infrastructure diagnosis (webhooks, proxy ref-updates) on top of a | ||
| > premise I had stopped checking. | ||
| > | ||
| > **The transferable rule is the sharper one:** when a subject stops behaving, | ||
| > **re-read its STATE before theorising about its plumbing.** A field you | ||
| > printed once and dropped is not a field you know. The exotic explanation felt | ||
| > earned because each new observation fit it — but they fit the boring one | ||
| > equally, and only the dropped field discriminated. | ||
| > | ||
| > **What survives unchanged:** the rule this entry exists for. Keying a watch | ||
| > on the PR head certified the wrong commit, and it would have done so for a | ||
| > *merged* PR just as surely as for a stalled one — arguably more so, since a | ||
| > merged PR's head is frozen permanently. Pin the SHA; compare it. If anything, | ||
| > a merged-PR head is the commoner way to hit this. | ||
|
|
||
| A CI monitor was armed to poll the PR, emit each check as it landed, and stop | ||
| at "all complete". It reported **ALL GREEN**. The verdict was true — and it was | ||
| a verdict on the **wrong commit**: 7/7 covered the head GitHub still believed | ||
| in, while the two commits carrying that PR's review fixes had **zero** checks. | ||
|
|
||
| ``` | ||
| PR head: 233ce3f1 → 7/7 complete, red=none | ||
| branch ref: 216d8f29 → 0/0 complete | ||
| ``` | ||
|
|
||
| **The failure is in the KEY, not the polling.** A monitor that resolves its | ||
| subject through the PR object inherits whatever the PR object believes, and a | ||
| PR head pointer is *derived state* that can lag the ref it points at. Every | ||
| line the monitor printed was accurate; none of them was about the code under | ||
| review. Silence would have been safer than that green. | ||
|
|
||
| **Rules, both cheap:** | ||
|
|
||
| 1. **Pin the SHA before arming, and assert it.** A watch should be keyed on the | ||
| commit you pushed — read it from `git rev-parse HEAD` — not on whatever the | ||
| PR resolves to at poll time. If the PR's head and your SHA disagree, that | ||
| disagreement is itself the event worth reporting. | ||
| 2. **A green run is a verdict on a SHA, never on a PR.** Before acting on one, | ||
| check `pr.head.sha == <the SHA you pushed>`. Two API calls. | ||
|
|
||
| **Why it was caught:** not by the monitor and not by any gate — by noticing | ||
| that a routine status read showed a head two commits behind a push whose | ||
| output had scrolled past. (The *cause* took longer and was got wrong first — | ||
| see the correction at the top.) The generalization is uncomfortable and worth | ||
| keeping: **an automated check can be simultaneously correct and irrelevant**, | ||
| and nothing inside it can tell the difference, because relevance is a property | ||
| of what it was pointed at. | ||
|
|
||
| Cf. `E-EVERY-DEFECT-IN-A-MEASUREMENT-WAS-IN-ITS-FIXTURE-NOT-ITS-CODE-1` — same | ||
| shape one layer up: there the fixture was wrong and the timing loop was fine; | ||
| here the subject was wrong and the polling was fine. In both, the instrument | ||
| reported faithfully on something that was not the question. | ||
|
|
||
| --- | ||
|
|
||
| ## 2026-08-31 — E-A-WITNESS-THAT-DROPS-THE-RELATION-IS-NOT-A-WITNESS-1 — and the review's own remedy was measurably wrong | ||
|
|
||
| **Status:** FINDING — three codex P1/P2 findings on #1120, all valid; the P1 | ||
| falsified a claim in the module's own doc comment. The P2 remedy was tried | ||
| literally, measured, and rejected in favour of a narrowing. | ||
| **Confidence:** measured — `lance-graph-planner/src/dismech_replay.rs`, | ||
| D-DCR-1 (W1), three disable-runs. | ||
|
|
||
| ### The P1: the trace dropped the predicate, and the doc said otherwise | ||
|
|
||
| `replay_chain` destructured `&(_predicate, weight)` and never carried the | ||
| ordinal into `ReplayTraceRow`. Two chains with identical weights and different | ||
| relations — `causes` vs `protects_against`, near-opposite meanings — replay to | ||
| **byte-identical traces**, and `first_divergence` reports no change. | ||
|
|
||
| For a plan whose keystone is *causality replay*, that is not a gap in a | ||
| convenience field: the witness cannot reconstruct the recorded program. And | ||
| the module doc claimed the opposite — *"carried into the trace's step index so | ||
| a consumer can join a diff back to the palette at the membrane"* — where the | ||
| step index is `i` and the ordinal was carried nowhere. A doc claim with no | ||
| behaviour behind it, in the same PR that added a mirror so the ordinal would | ||
| have a checkable domain. | ||
|
|
||
| **Why every W1 gate passed anyway, and this is the transferable part:** W1's | ||
| arithmetic deliberately does not read the ordinal. A property that does not | ||
| touch a field cannot notice that the field is gone. Determinism, perturbation | ||
| position, sibling isolation, deinterlace — four gates, none of which could | ||
| ever fail on this. **A value carried as WITNESS needs a gate that reads it as | ||
| witness; the gates on the computation will not cover it, by construction.** | ||
|
|
||
| ### The P2 remedy was wrong, and running it is how that was established | ||
|
|
||
| The review's `first_divergence` finding was right about the defect (the name | ||
| promised a whole-row comparison the body did not perform) and its suggested | ||
| fix — *"compare the full `ReplayTraceRow` values"* — was tried verbatim: | ||
|
|
||
| ``` | ||
| addressing_is_not_content_but_the_predicate_is ... FAILED | ||
| left: Some(0) right: None | ||
| ``` | ||
|
|
||
| The same chain, same owner, replayed from durable base 100 vs 900, is declared | ||
| **divergent at step 0**. That is the same causal witness at two addresses, and | ||
| an instrument that calls it divergent is useless for every comparison the plan | ||
| needs. So the fix is the review's *alternative* clause — "narrow the API | ||
| contract explicitly": content is `(predicate, edge)`; `owner`/`cast_seq` are | ||
| addressing and deliberately excluded; `step` is positional and is what the | ||
| return value NAMES. | ||
|
|
||
| **A correct finding does not make its proposed remedy correct.** Both halves | ||
| were verifiable in about a minute each; taking the remedy on trust would have | ||
| shipped a diff instrument that fires on every address change. | ||
|
|
||
| ### The third: a precondition stated as prose is not a precondition | ||
|
|
||
| `base_seq` reserves `[base_seq, base_seq + chain.len())` — one coordinate per | ||
| STEP. A caller advancing by 1 per CHAIN overlaps (bases 10 and 11 over 4 steps | ||
| share 3 of 4 coordinates), which violates `LocalCausalRow::cast_seq`'s | ||
| uniqueness requirement and silently degrades `local_trajectory_of` to scan | ||
| order. The old doc said two chains "never collide" — true only for a caller | ||
| already following a rule the doc did not state. | ||
|
|
||
| The overlap is a property ACROSS calls and cannot be checked inside one, so it | ||
| stays a precondition — but now it is stated, and shipped as code | ||
| (`next_base_seq`) so the correct advance is the easy one. The gate measures | ||
| the collision (`shared == 3`) rather than asserting the rule. | ||
|
|
||
| ### Disable table (all three red-then-green) | ||
|
|
||
| | assertion | disable | observed | | ||
| |---|---|---| | ||
| | predicate is in the witness | `predicate: 0` in the row | RED ×2 | | ||
| | addressing is not content | compare whole rows (the review's literal remedy) | RED — `Some(0)` vs `None` | | ||
| | the reservation is per step | `next_base_seq` → `base + 1` | RED — `left: 11, right: 14` | | ||
|
|
||
| --- | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,8 +5,8 @@ | |
| | D-DCR-0 | W0 measurements: chain-step evals/ms, EvidenceMask branching shrink, palette round-trip; ALU BUY threshold stated | **Shipped (CORRECTED #1118)** — promised kernel (NarsTables::revise + CausalEdge64::forward) **34.7 ns/step**; alloc-free `[u64;64]` mask 61.5 ns ⇒ **MASK dominates 1.77x** (v1's 3.8x superseded: substitute kernel + allocating fixture); KILL does not fire at EITHER the pre-registered 10^5 (13.88 ms) or the real 2,449 arm (0.340 ms), crossover ~53 chains; BUY at >10x the **1.36 ms** oracle arm. Probe `lance-graph-planner/examples/dcr_w0_replay_budget` | | ||
| | D-DCR-0b | "borrow masking from ndarray?" measured (§3d): `mask_and` is a DEAD HEAT (1.08x) because the AND is 11.1 ns of a 65.2 ns half — the scalar POPCOUNT is 5.1x it. R2IL's `CallMask` (`[u64;3]`) needs nothing; jitson is a kernel compiler, not masking. The primitive that would pay (fused `mask_and_popcount`) does not exist and belongs IN ndarray | **Shipped** (measured direction; no wave scheduled) | | ||
| | D-DCR-0a | prior-art reconciliation: `contract::dismech_evidence` + `dismech-causality-v3-v1` §11 arms (2,449 / 4,076 / 361) are W1-W3's falsifier; plan §3a | **Shipped** (E-W0-MEASURED-THE-MASK-HALF-DOMINATES-...-1) | | ||
| | D-DCR-1 | replay core: loco calls under the dismech vocabulary -> CausalEdge64/NarsTruth steps -> temporal.rs trace; determinism + perturbation falsifiers | **In PR** — `lance-graph-planner/src/dismech_replay.rs` (`replay_step` / `replay_chain` / `first_divergence` / `ReplayTraceRow: LocalCausalRow`); 4 gates, 3 disable-verified red-then-green. Palette binds at the membrane (plain `u8` ordinal here); `cast_seq` is caller-supplied and durable, never minted. Membrane half CLOSED: contract `dismech_evidence::DISMECH_PREDICATES` (zero-dep 19-row mirror, floor 0x90, position lookup) + armed-tier fuse `lance_graph_ogar::parity::assert_dismech_palette_parity` against the real `ogar_dismech::RELATIONS`, both directions, 3 more disables verified | | ||
| | D-DCR-2 | Mengenlehre candidate evaluation via `contract::revision::EvidenceMask` (support ∩ / refute ∖, skip-words as ordinals) | Queued | | ||
| | D-DCR-1 | replay core: loco calls under the dismech vocabulary -> CausalEdge64/NarsTruth steps -> temporal.rs trace; determinism + perturbation falsifiers | **Shipped (#1120, merged `cc0046f8`)** + follow-up in PR — `lance-graph-planner/src/dismech_replay.rs` (`replay_step` / `replay_chain` / `first_divergence` / `ReplayTraceRow: LocalCausalRow`); 4 gates, 3 disable-verified red-then-green. Palette binds at the membrane (plain `u8` ordinal here); the caller supplies a durable `base_seq` and the planner DERIVES each row's `cast_seq` from it — nothing here mints a counter. (Wording corrected per CodeRabbit #1120: the earlier phrasing said `cast_seq` was caller-supplied, which reverses the API contract.) Membrane half CLOSED: contract `dismech_evidence::DISMECH_PREDICATES` (zero-dep 19-row mirror, floor 0x90, position lookup) + armed-tier fuse `lance_graph_ogar::parity::assert_dismech_palette_parity` against the real `ogar_dismech::RELATIONS`, both directions, 3 more disables verified. Codex #1120: 3 findings, all valid — `ReplayTraceRow.predicate` now carried as WITNESS (the P1 falsified the module's own doc claim), `first_divergence` contract narrowed to content `(predicate, edge)` (the review's literal whole-row remedy was measured and rejected), `next_base_seq` makes the per-STEP durable reservation explicit. 9 disables total. CodeRabbit #1120 (4 more, read after merge): board wording corrected; `validate_chain` + `UnmintedOrdinal` reject an out-of-band ordinal AT ADMISSION while replay stays total over history; `replay_chain -> Result` with `ReplayError::SequenceExhausted` checks the whole reservation up front (`base_seq + i` panicked in debug / wrapped in release at u64::MAX). 11 disables total; 10 module gates | | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -euo pipefail
git show cc0046f8:.claude/board/STATUS_BOARD.md | rg -n 'D-DCR-[12]'
git diff --unified=0 cc0046f8 -- .claude/board/STATUS_BOARD.mdRepository: AdaWorldAPI/lance-graph Length of output: 4415 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- repository knowledge files ---'
head -5 /tmp/coderabbit-repo-knowledge/adaworldapi-lance-graph-8c8fd2e8/*/*.md
printf '%s\n' '--- board-history rules and learnings ---'
rg -n -i -C 3 'append.only|merged entr|STATUS_BOARD|Status|Confidence' \
/tmp/coderabbit-repo-knowledge/adaworldapi-lance-graph-8c8fd2e8Repository: AdaWorldAPI/lance-graph Length of output: 6160 🏁 Script executed: #!/bin/bash
set -euo pipefail
files=$(fd -a -t f 'CLAUDE.md' .)
if [ -n "$files" ]; then
while IFS= read -r file; do
printf '%s\n' "--- $file ---"
rg -n -i -C 4 'board|append.only|histor|Status|Confidence|newest.first' "$file" || true
done <<< "$files"
else
printf '%s\n' 'No CLAUDE.md file found'
fiRepository: AdaWorldAPI/lance-graph Length of output: 28471 Preserve the merged D-DCR-1 history. D-DCR-1 existed in merged commit 🤖 Prompt for AI AgentsSource: Learnings |
||
| | D-DCR-2 | Mengenlehre candidate evaluation via `contract::revision::EvidenceMask` (support ∩ / refute ∖ over `dismech_evidence::Supports`) | Queued — **spec corrected in preflight**: the refute class is the evidence STANCE (`Supports`, shipped + measured), NOT the graph-construction skip filter the plan first named. The skip filter decides whether an item becomes an edge at all, so a candidate set built from the graph has already excluded it — `∖` would subtract twice. Plan §W2 carries the full correction | | ||
| | D-DCR-3 | counterfactual replay (edge cut through `contract::counterfactual`, Pearl rung 3), two-sided load-bearing/redundant gates | Queued | | ||
| | D-DCR-4 | Σ transport via `jc::ewa_sandwich` + candidate-entropy readout; entropy-surface CONSOLIDATION decision recorded first | Queued | | ||
| | D-DCR-5 | frontier scheduling (info-gain / rung-cost via `EpistemicMode::for_rung`) | **HELD** — operator rung 5-9 table ruling + W0 KILL check | | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.