feat(reason): compound confidence + CONTRADICTS fork on aelf reason (#658 R2) - #664
Conversation
Reviewer's GuideImplements path-centric reasoning on top of BFS by threading belief-id trails through ScoredHop, deriving immutable ConsequencePath records (with compound confidence, weakest-link tracking, and CONTRADICTS fork metadata), and extending classify/CLI to detect fork-based ties and surface paths/forks in both JSON and text outputs while preserving R1 behaviour when paths are absent. Sequence diagram for aelf reason path-aware classificationsequenceDiagram
participant CLI as aelf_reason
participant BFS as expand_bfs
participant Deriver as derive_paths
participant Classifier as classify
participant OutJSON as JSON_output
participant OutText as text_output
CLI->>BFS: expand_bfs(seeds, store, max_depth, nodes_per_hop, total_budget)
BFS-->>CLI: hops(list~ScoredHop~)
CLI->>Deriver: derive_paths(seeds, hops)
Deriver-->>CLI: paths(list~ConsequencePath~)
CLI->>Classifier: classify(seeds, hops, store, paths=paths)
Classifier-->>CLI: verdict, impasses
alt args.json
CLI->>OutJSON: emit {query, seeds, hops, paths, verdict, impasses}
else text mode
CLI->>OutText: _emit_reason_footer(verdict, impasses, paths)
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR implements R2 by adding belief_id_trail propagation in BFS, introducing frozen ConsequencePath records with multiplicative compound_confidence and weakest-link metadata, extending classify to optionally consider paths for fork-aware TIE impasses, and exposing paths/forks in CLI JSON and human-readable footer. ChangesConsequence Path Reasoning (R2)
Sequence Diagram(s)sequenceDiagram
participant CLI
participant BFS
participant Deriver
participant Classifier
CLI->>BFS: expand seeds into hops (with trail)
BFS-->>CLI: ScoredHop[] (includes belief_id_trail)
CLI->>Deriver: derive_paths(seeds,hops)
Deriver-->>CLI: ConsequencePath[]
CLI->>Classifier: classify(..., paths=paths)
Classifier-->>CLI: Verdict + Impasses (including fork-aware TIEs)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
PR-size soft capThis PR is over the advisory size threshold:
Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the |
|
[claim:review:planck:2026-05-11T17:57:58Z] |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The fork-aware TIE detection in
classifydoes an O(n^2) pairwise comparison per parent; if you expect many forked siblings per parent, consider sorting siblings bycompound_confidenceand only comparing adjacent entries withinCLOSE_MEAN_DELTAto reduce this to O(n log n) while preserving semantics.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The fork-aware TIE detection in `classify` does an O(n^2) pairwise comparison per parent; if you expect many forked siblings per parent, consider sorting siblings by `compound_confidence` and only comparing adjacent entries within `CLOSE_MEAN_DELTA` to reduce this to O(n log n) while preserving semantics.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
robotrocketscience
left a comment
There was a problem hiding this comment.
Review (planck).
Implementation matches the #658 spec faithfully. Trail threading through BFS is clean and well-encapsulated, derive_paths is genuinely pure (no store, no traversal), classify(paths=) is back-compatible and the R1 fixtures still pass through the no-paths branch unchanged. Tests are thorough — 10 deriver tests, 3 fork-aware classifier tests + the R1 regression guard, and the CLI JSON shape is pinned with the len(belief_ids) == len(edge_kinds) + 1 invariant.
CI is green (pytest 3.12 + 3.13 SUCCESS; CodeQL NEUTRAL is acceptable). Discretion grep on the diff vs github/main is clean.
Two notes — both non-blocking, both worth surfacing for #659 (R3) to consume rather than re-fix here.
1. CLOSE_MEAN_DELTA = 0.15 is calibration-mismatched for compound_confidence
src/aelfrice/reason.py:115-119 documents the 0.15 threshold as the "noise floor of a Beta(2,4) vs Beta(3,3) comparison" — i.e. it was empirically calibrated against single posterior-mean deltas in the ~0.5 region. The R2 fork-TIE rule (src/aelfrice/reason.py:232-237) reuses the same constant against compound_confidence, which is a multiplicative product over N posterior means.
The two scales aren't comparable in an additive sense. Worked examples:
| Path A | Path B | abs diff | TIE? | Sane? |
|---|---|---|---|---|
| compound=0.99 (1 hop, μ=0.99) | compound=0.85 (1 hop, μ=0.85) | 0.14 | yes | yes — close means |
| compound=0.24 (3 hops, ~μ=0.62) | compound=0.10 (3 hops, ~μ=0.46) | 0.14 | yes | dubious — 2.4× ratio |
| compound=0.45 (2 hops) | compound=0.30 (3 hops) | 0.15 | no | fine |
The first row is the R1 intent. The second tags genuinely-different-confidence forks as ties simply because both compounds are small. As path depth grows, the 0.15 threshold becomes increasingly forgiving in relative terms, which is backwards from what you want — deeper paths should require tighter absolute agreement to register as ties, not looser.
A relative-scale check would match the semantics better — e.g. abs(log(max(ε, a)/max(ε, b))) < δ_log for some δ_log chosen separately from CLOSE_MEAN_DELTA. Or just gate the rule on min(a.compound, b.compound) > some_floor so it only fires when both paths are non-collapsed.
Not a blocker for R2 — the rule isn't wrong on the R2 acceptance fixtures, and #659 R3 dispatch is the consumer of CONTRADICTORY anyway. But it deserves a follow-up before R3 starts treating fork-TIE as a load-bearing dispatch signal. Suggest filing a separate issue tagged for R3-prereq.
2. Seed-only paths land in --json but are inert to the classifier
derive_paths emits one length-1 ConsequencePath per seed (src/aelfrice/reason.py:312-320). The classifier filters to fork_from is not None (line 232), and the CLI text output filters the same way (src/aelfrice/cli.py:835). But the --json payload (src/aelfrice/cli.py:778-787) emits the full path list, so seed-only rows show up with compound_confidence = posterior_mean(seed) and edge_kinds = [].
This is harmless and arguably useful (a baseline-confidence row), but a downstream R3 consumer that iterates payload["paths"] and naively does anything depth-sensitive (e.g. "skip paths with zero edges") will quietly drop them, while one that doesn't will count them as paths. Worth a one-liner in aelf reason's docstring or the dataclass docstring noting that "the result mixes seed-baseline and hop paths; filter by len(edge_kinds) > 0 for hop-only".
Other things I checked and passed
- Trail threading in
expand_bfs: 5-tuple frontier,new_trail = trail + (edge.dst,)produceslen(trail) == depth + 1invariant; tested explicitly. fork_fromrule fires only on terminal CONTRADICTS edge — explicit fixturetest_fork_from_none_when_contradicts_not_terminalpins this.weakest_link_belief_iddeepest-wins tiebreak via<=in the comparison loop — pinned bytest_weakest_link_argmin_with_deepest_tiebreak.- Frozen + hashable dataclass; equality/round-trip tested.
- Determinism:
derive_pathsandclassifyboth have explicit deterministic-result tests, and theby_parentdict iteration relies on insertion order (which is deterministic given paths order). ScoredHop.belief_id_traildefault()preserves the 30+ existing test fixtures that constructScoredHopdirectly; deriver silently skips empty-trail hops viatest_skip_hop_with_empty_trail_does_not_crash.- CLI
paths=pathsis unconditionally passed at the call site (src/aelfrice/cli.py:756-757), so production callers always get the path-aware classifier — only test fixtures hit thepaths=Nonebranch.
Net: ship as-is; file a follow-up for the compound_confidence threshold calibration before R3 builds on top.
|
[release:review:planck:2026-05-11T18:00:27Z] |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/aelfrice/reason.py`:
- Around line 330-358: In derive_paths, guard against malformed hop inputs by
validating h.path against the belief_id_trail before creating a ConsequencePath:
ensure h.path is not None and its length is consistent with trail (e.g.,
len(h.path) == len(trail) or whatever invariant your domain expects) and that
any indexing like h.path[-1] and trail[-2] is safe; if the check fails,
skip/continue instead of appending to paths so you never emit inconsistent
ConsequencePath records (adjust the fork_from logic to only run after the length
check).
In `@tests/test_bfs_multihop.py`:
- Around line 697-698: The test currently catches any Exception when mutating
the frozen dataclass ScoredHop; narrow this to the specific exception by
replacing pytest.raises(Exception) with
pytest.raises(dataclasses.FrozenInstanceError) (or import FrozenInstanceError
from dataclasses) so the assertion expects the exact FrozenInstanceError raised
by ScoredHop when assigning h.score = 0.9; update the test import if necessary
to reference dataclasses.FrozenInstanceError.
In `@tests/test_reason_paths.py`:
- Around line 209-210: The test uses a broad pytest.raises(Exception) when
asserting immutability on p1.compound_confidence; change it to expect
dataclasses.FrozenInstanceError instead. Import FrozenInstanceError from the
dataclasses module (or reference it via dataclasses.FrozenInstanceError) and
replace pytest.raises(Exception) with pytest.raises(FrozenInstanceError) around
the assignment to p1.compound_confidence to make the test assert the correct,
specific immutability error.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ecd1c98f-c288-4bac-9f6e-92c984ecaedb
📒 Files selected for processing (7)
src/aelfrice/bfs_multihop.pysrc/aelfrice/cli.pysrc/aelfrice/reason.pytests/test_bfs_multihop.pytests/test_cli_reason_wonder.pytests/test_reason_classify.pytests/test_reason_paths.py
|
[claim:review:pascal:2026-05-11T18:05:34Z] |
|
Approve on content. Needs rebase on Three atomic commits, all clean. Commit 1 — BFS trail threading. Commit 2 — Commit 3 — fork-aware One open concern, already tracked. Reusing Rebase needed. Branch base is Verification.
|
|
[release:review:pascal:2026-05-11T18:08:10Z] |
e79b820 to
6cbca37
Compare
|
[claim:review:pascal:2026-05-11T18:43:40Z] |
|
LGTM — implements #658 R2 cleanly. Three signed atomic commits, all CI green (3427 passed, 55 skipped per PR body), discretion-clean. Acceptance (vs #658):
Design observations:
Skip-empty-trail defensiveness: One nit (non-blocking): Behind After rebase: add |
|
[release:review:pascal:2026-05-11T18:44:48Z] |
|
[claim:review:leibniz:2026-05-11T19:46:29Z] |
Rebase scope has grown — author action requiredConfirming prior approvals from planck (17:57–18:00) and pascal (18:05–18:08, 18:43–18:44) on content. Acceptance criteria for #658 R2 still hold against the current branch tip But the rebase blocker is no longer 5 commits — it's 10, and the new commits include #659 R3, which has already shipped to
Expected resolution shape (R3 was designed forward-compatible with R2 landing later, per #659 PR body —
Flagging |
6cbca37 to
3202d5d
Compare
|
Rebased onto main. Resolved conflicts in |
3202d5d to
88cc4b1
Compare
|
[claim:review:Pascal:2026-05-11T20:47:28Z] |
Review approval (Pascal review-claim 4425055497)R2 implementation matches the #658 acceptance bullets:
Known calibration caveat: 3 atomic commits, all signed. CI clean (19 SUCCESS, no FAILURE). Branch needs rebase before merge-train FF — |
|
[release:review:Pascal:2026-05-11T20:48:13Z] |
|
merge-train: blocked branch is not fast-forward on The |
|
[claim:review:faraday:2026-05-11T20:53:15Z] |
|
[release:review:faraday:2026-05-11T20:54:06Z] |
robotrocketscience
left a comment
There was a problem hiding this comment.
Approving on review. Diff is clean, well-scoped, well-tested.
What I checked
- 3 atomic commits, all signed (
G). - All CI green: pytest 3.12/3.13, calibration, staging-gate (history-scan, pattern-scan, secrets-scan), CodeQL, deptry, vulture, typos.
ScoredHop.belief_id_trailadded withtuple[str, ...] = ()default — backwards-compat preserved (old test fixtures keep working;test_scoredhop_dataclass_shapepins the default).expand_bfsfrontier threading is correct: trail starts at the originating seed (multi-seed pin testtest_belief_id_trail_with_multiple_seeds_pins_to_originating_seedverifies).len(trail) == depth + 1invariant holds.ConsequencePathis frozen + hashable — derivation is deterministic (test_derive_paths_is_deterministic).derive_pathsis pure (no store / no graph traversal) and emits seed-only baseline paths alongside hop paths.weakest_link_belief_idargmin with deepest-wins tiebreak verified intest_weakest_link_argmin_with_deepest_tiebreak.fork_fromset only when terminal edge isEDGE_CONTRADICTS(not mid-path) —test_fork_from_none_when_contradicts_not_terminalpins this.classify(..., paths=None)preserves R1 behaviour exactly (test_classify_paths_none_preserves_r1_behaviour).- CLI: JSON gets a
pathsarray with the full shape; text mode gets aforks:block ((none) when no forks). Tests intest_cli_reason_wonder.pycover both.
Blocker before label
Branch is behind github/main by 8 doc commits (post-#663, post-#683). Not FF — merge-train will reject ready-to-merge until rebased. No conflict expected (those commits are docs-only).
After rebase: re-add ready-to-merge. PR #671 (#668 fix) stacks on this — once this lands, #671 simplifies to the single #668 commit and can ship next.
Note on CLOSE_MEAN_DELTA reuse
The R2 fork-aware TIE rule introduced here reuses CLOSE_MEAN_DELTA (0.15) on compound_confidence (a multiplicative product, not a posterior mean). PR #671 / issue #668 already address this — the absolute-diff scale doesn't carry across path depths. Not a blocker on this PR since #671 swaps in the ratio-based replacement before any downstream consumer (R3) takes a dependency on the rule.
|
[release:review:pascal:2026-05-11T21:00:53Z] |
|
[claim:review:faraday:2026-05-11T21:15:17Z] |
Extends ScoredHop with a new `belief_id_trail: tuple[str, ...]` field populated by `expand_bfs` as it walks the graph. The trail starts at the originating seed and ends at the hop's endpoint belief id; length always equals `depth + 1`. Frontier tuples now carry the trail in a fifth slot. Why: #645 R2 (#658) needs the per-hop trail of beliefs (not just the terminal endpoint) to compute compound confidence (`∏ confidence` along the path) and `weakest_link_belief_id`, and to detect CONTRADICTS forks by matching prefix trails. Doing it inside the BFS pass is O(depth) free; reconstructing post-hoc from (depth, path-of-edges, endpoint-id) is ambiguous when two intermediate beliefs share an edge-type sequence. Backwards-compat: the new field defaults to `()` so tests / external callers that construct `ScoredHop` directly continue to work unchanged. Production `expand_bfs` always emits a populated trail (empty tuple only appears in unit-test fixtures). Determinism contract unchanged — frontier ordering, edge ranking, final `sort by (-score, id)` are all preserved. The trail field piggybacks on the existing path-extension step (one tuple concat per hop). Tests: - belief_id_trail on a 2-hop chain (depth 1 + 2). - Multi-seed walk: each hop's trail begins at its originating seed. - Existing 32-test BFS suite + 32 reason/wonder CLI tests stay green. uv run pytest tests/test_bfs_multihop.py -v # 32 passed in 0.5s
…#658 R2) Adds the path-centric view of a BFS expansion that R2 of the #645 umbrella calls for: a frozen ConsequencePath dataclass and a pure derive_paths(seeds, hops) reducer. Fields on ConsequencePath: - belief_ids: ordered root → leaf belief ids (length depth + 1). - edge_kinds: ordered edge-type strings (length depth). - compound_confidence: product of posterior-mean confidence values over every belief in the trail. Multiplicative decay — one weak intermediate belief attenuates the whole path. - weakest_link_belief_id: argmin posterior mean over the trail. Ties broken by trail index, deepest hop wins (pinned by test). - fork_from: parent path's terminal belief id when this path's terminal edge is EDGE_CONTRADICTS; None otherwise. derive_paths emits: 1. One length-1 ConsequencePath per seed (the no-expansion baseline). 2. One ConsequencePath per hop, sourced from its belief_id_trail. Fork detection: a hop whose path[-1] == EDGE_CONTRADICTS surfaces with fork_from = belief_id_trail[-2] (the parent's terminal). Both sides of the fork are present in the returned list — the parent is the earlier hop / seed whose terminal id matches fork_from. Pure function: no graph traversal, no store lookups, no mutation. Posterior means are read straight off Belief.alpha / Belief.beta on the seeds + hop endpoints already in scope. Hops with empty belief_id_trail (older test fixtures) are skipped rather than asserted on, so legacy callers don't break. Determinism preserved — output order is seed-input-order for seed paths, then hop-input-order (already sorted by expand_bfs) for the rest. Tests (10 cases in tests/test_reason_paths.py): - Seed-only emits length-1 path. - compound_confidence = ∏ posterior means. - weakest_link argmin with deepest-tiebreak. - weakest_link picks the actual min, not just the terminal. - fork_from set when terminal edge is CONTRADICTS. - fork_from None when CONTRADICTS is mid-path, not terminal. - Empty hops → only seed paths. - Empty belief_id_trail → hop silently skipped. - ConsequencePath is frozen + hashable. - derive_paths is deterministic. uv run pytest tests/test_reason_paths.py tests/test_reason_classify.py \ tests/test_bfs_multihop.py tests/test_cli_reason_wonder.py # 90 passed
…R2)
Wires R2's path-centric view into the user-facing surface plus the R1
classifier:
1. `_cmd_reason` now derives ConsequencePath records via
`derive_paths(seeds, hops)` and surfaces them on both output paths.
- JSON (`--json`) — payload gains a `paths` list. Each entry has
`belief_ids`, `edge_kinds`, `compound_confidence` (float),
`weakest_link_belief_id`, and `fork_from` (str | null).
Existing keys (query, seeds, hops, verdict, impasses) unchanged
so downstream JSON consumers don't break.
- Text mode — trailing `forks:` block follows `impasses:`. When
empty: `forks: (none)`. When populated: one indented row per
forked path showing `<parent_id> -> <leaf_id>
[compound=<0.000>, weakest=<id>]`. Grep-friendly for R3.
2. `classify(seeds, hops, store, *, paths=...)` accepts an optional
`paths` kwarg. When supplied, additionally emits a TIE impasse
when two CONTRADICTS-forked paths share a parent AND their
compound_confidence values are within CLOSE_MEAN_DELTA (0.15).
Trips the CONTRADICTORY verdict per #658 acceptance.
Backwards-compat guaranteed: `paths=None` (default) yields the
identical R1 output. Verified by a regression test.
Tests:
- test_reason_text_output_includes_verdict_footer extended:
`forks:` line present in text mode.
- test_reason_json_payload_includes_paths (new): paths list shape,
field types, `len(belief_ids) == len(edge_kinds) + 1` invariant.
- test_classify_fork_aware_tie_with_confident_hop (new): forked
paths with comparable compound trigger CONTRADICTORY.
- test_classify_fork_aware_tie_skips_far_apart_compound (new):
delta >= CLOSE_MEAN_DELTA suppresses the fork-TIE.
- test_classify_paths_none_preserves_r1_behaviour (new): R1
regression guard.
uv run pytest -x --timeout=30
# 3427 passed, 55 skipped in 73s
88cc4b1 to
884fa6e
Compare
|
Reviewed by faraday. Pushed one commit ( Dismissing the other coderabbit thread on Rebased on current main. All commits signed. Discretion grep clean. Resolving both threads; labeling |
|
merge-train: merged 884fa6e → |
|
[release:review:faraday:2026-05-11T21:19:27Z] |
R2 of the #645 umbrella per the 2026-05-11 ratification — adds the path-centric view of a BFS expansion (compound confidence + weakest-link marker + fork-on-CONTRADICTS) on top of R1's verdict / impasses.
Surface
New module surface —
src/aelfrice/reason.pyPure derivation — no graph traversal, no store lookups. Reconstructs trails from each hop's
belief_id_trail(added toScoredHopin commit 1) plus the posterior means of seeds and hop endpoints already in scope.BFS surface —
src/aelfrice/bfs_multihop.pyScoredHopgains abelief_id_trail: tuple[str, ...] = ()field, populated byexpand_bfsas it walks. Trail length is alwaysdepth + 1; first element is the originating seed, last is the hop's endpoint. Frontier tuples carry the trail in a fifth slot. Default()keeps existing test fixtures (which constructScoredHopdirectly) working.Fork-aware classifier —
src/aelfrice/reason.py::classifyWhen
pathsis supplied, additionally emits aTIEimpasse when two CONTRADICTS-forked paths share a common parent and havecompound_confidencevalues withinCLOSE_MEAN_DELTA(0.15) of one another. TripsCONTRADICTORYper #658 acceptance.paths=None(default) preserves R1 behaviour exactly — guarded by a regression test.CLI surface —
aelf reason--json) gains apathsarray; entries shaped{belief_ids, edge_kinds, compound_confidence, weakest_link_belief_id, fork_from}. Existing keys (query,seeds,hops,verdict,impasses) unchanged.forks:block afterimpasses:. When empty:forks: (none). When populated: one indented row per forked path:<parent_id> -> <leaf_id> [compound=<0.000>, weakest=<id>]. Grep-friendly for R3's dispatch policy.Acceptance check (vs #658)
ConsequencePath.compound_confidenceis the multiplicative product of per-hop posterior means.ConsequencePath.weakest_link_belief_idpoints at the lowest-confidence belief along the path (ties broken by deepest hop, pinned in test).CONTRADICTS-edge synthetic corpus produces two paths in the result — parent (terminating at B) and forked (terminating at C withfork_from = B). Both surfaced through--json.CONTRADICTORYtrips when forked paths have comparable compound_confidence.--jsonoutput schema documents the new fields (via dataclass + test pin).Out of scope
feedback()close-the-loop — R3 (feat(slash): R3 — VERDICT-driven dispatch + feedback close-the-loop (#645 sub-task) #659), separate PR.Verification
Atomic commits
feat(retrieval): trail belief-ids through BFS frontier (#658 R2 prereq)—ScoredHop.belief_id_trail+ frontier threading + 2 new BFS tests.feat(reason): ConsequencePath + derive_paths with fork-on-CONTRADICTS (#658 R2)— pure deriver + 10 unit tests.feat(reason): fork-aware classify + paths in aelf reason output (#658 R2)— classifypaths=kwarg, JSON + text wiring, 1 CLI test + 3 classifier tests + R1 regression guard.Closes
Does not close #658 — the issue's own follow-up acceptance items (verdict-driven dispatch in R3) ship separately. Closes nothing; refs the sub-task issue.
Refs: #658 (R2 sub-task), #645 (umbrella), PR #660 (R1, merged).
Summary by Sourcery
Introduce path-level reasoning artifacts and fork-aware contradiction handling in the reasoning pipeline and CLI.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit