feat(cli): aelf core subcommand (#439) - #463
Merged
Merged
Conversation
Reviewer's GuideImplements the new read-only Sequence diagram for the new aelf core CLI commandsequenceDiagram
actor User
participant Shell
participant AelfCLI
participant Argparse
participant CoreCommand
participant Store
participant Qualifier
participant Emitter
User->>Shell: run aelf core [flags]
Shell->>AelfCLI: invoke main()
AelfCLI->>Argparse: parse arguments
Argparse->>CoreCommand: _cmd_core(args, out)
CoreCommand->>Store: _open_store()
alt args.no_locked
CoreCommand->>CoreCommand: locked = []
else not args.no_locked
CoreCommand->>Store: list_locked_beliefs()
Store-->>CoreCommand: locked list
end
alt not args.locked_only
CoreCommand->>Store: list_belief_ids()
Store-->>CoreCommand: belief_ids
loop each belief_id
CoreCommand->>Store: get_belief(belief_id)
Store-->>CoreCommand: belief or None
alt belief is not None and belief.lock_level == none
CoreCommand->>Qualifier: _qualifies_core(belief, args)
Qualifier-->>CoreCommand: qualifies bool
alt qualifies
CoreCommand->>CoreCommand: append belief to candidates
end
end
end
end
CoreCommand->>Store: close()
CoreCommand->>Emitter: _emit_core(locked, candidates, args, out)
Emitter-->>CoreCommand: print formatted results
CoreCommand-->>Argparse: return 0
Argparse-->>AelfCLI: exit code 0
AelfCLI-->>Shell: process exit
Updated class diagram for the aelf core command and related typesclassDiagram
class Belief {
+str id
+str content
+str lock_level
+float alpha
+float beta
+int corroboration_count
}
class BeliefStore {
+list~Belief~ list_locked_beliefs()
+list~str~ list_belief_ids()
+Belief get_belief(belief_id)
+void close()
}
class CoreArgs {
+bool json
+int limit
+int min_corroboration
+float min_posterior
+int min_alpha_beta
+bool locked_only
+bool no_locked
}
class AelfCoreCommand {
+int _CORE_MIN_CORROBORATION
+float _CORE_MIN_POSTERIOR
+int _CORE_MIN_ALPHA_BETA
+bool _qualifies_core(b, args)
+void _emit_core(locked, candidates, args, out)
+int _cmd_core(args, out)
}
AelfCoreCommand --> BeliefStore : uses
AelfCoreCommand --> Belief : filters_formats
AelfCoreCommand --> CoreArgs : reads_thresholds
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 failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR implements the ChangesCore CLI Command Implementation
Sequence DiagramsequenceDiagram
actor User
participant CLI as aelf core
participant Store as Belief Store
participant Filter as Filtering Logic
participant Output as Output Formatter
User->>CLI: aelf core [--flags]
CLI->>Store: Load beliefs
Store-->>CLI: Belief set
CLI->>Filter: Build locked subset
Filter-->>CLI: Locked beliefs
CLI->>Filter: Compute candidates by corroboration/posterior
Filter-->>CLI: Unlocked candidates
CLI->>CLI: Merge & deduplicate
CLI->>CLI: Sort by posterior + tiebreak
CLI->>Output: Format (text or JSON)
Output-->>CLI: Formatted output
CLI-->>User: Display results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Microsoft Presidio Analyzer (2.2.362)docs/COMMANDS.mdMicrosoft Presidio Analyzer failed to scan this file 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In both
_posteriorand the JSON/text signal recomputation paths,alpha / (alpha + beta)can divide by zero if a belief ever hasalpha + beta == 0; consider guarding against this (e.g., skip posterior or treat μ as 0.0) to avoid a hard crash on malformed or edge-case beliefs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In both `_posterior` and the JSON/text signal recomputation paths, `alpha / (alpha + beta)` can divide by zero if a belief ever has `alpha + beta == 0`; consider guarding against this (e.g., skip posterior or treat μ as 0.0) to avoid a hard crash on malformed or edge-case beliefs.
## Individual Comments
### Comment 1
<location path="src/aelfrice/cli.py" line_range="1308-1311" />
<code_context>
+ seen: set[str] = {b.id for b in locked} # type: ignore[attr-defined]
+ unlocked = [b for b in candidates if b.id not in seen] # type: ignore[attr-defined]
+
+ def _posterior(b: object) -> float:
+ a: float = b.alpha # type: ignore[attr-defined]
+ bb: float = b.beta # type: ignore[attr-defined]
+ return a / (a + bb)
+
+ unlocked.sort(key=lambda b: (-_posterior(b), b.id)) # type: ignore[attr-defined]
</code_context>
<issue_to_address>
**issue:** Guard `_posterior` against a zero α+β to avoid a potential ZeroDivisionError.
Beliefs with α=β=0 (i.e., a + bb == 0) will cause a ZeroDivisionError here. Since you already special-case `ab == 0` when computing `posterior_mean`, consider doing the same in `_posterior` (e.g., return 0.0 or another defined default when `a + bb == 0`) so sorting cannot crash on such entries.
</issue_to_address>
### Comment 2
<location path="src/aelfrice/cli.py" line_range="1291" />
<code_context>
+ alpha: float = b.alpha # type: ignore[attr-defined]
+ beta: float = b.beta # type: ignore[attr-defined]
+ corr: int = b.corroboration_count # type: ignore[attr-defined]
+ if corr >= args.min_corroboration:
+ return True
+ ab = alpha + beta
</code_context>
<issue_to_address>
**issue (bug_risk):** `--min-corroboration` "0 disables" semantics don’t match the current check.
`_qualifies_core` checks `if corr >= args.min_corroboration`, so `0` actually enables the signal for all beliefs (`corr >= 0` is always true). If `0` is meant to disable this gate, you likely want something like `if args.min_corroboration and corr >= args.min_corroboration:` (and the same logic wherever you add the `"corroboration"` signal). Otherwise, please update the help text to match the current behavior.
</issue_to_address>
### Comment 3
<location path="tests/test_cli_core.py" line_range="148-152" />
<code_context>
+def test_core_json_parses(isolated_db: Path) -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** Align JSON scenario coverage with the default text scenario by asserting that non-core beliefs are excluded from JSON output as well.
In the text tests we assert that `b-thin` and `b-prior` are excluded from `aelf core` output. Here, the JSON tests only verify structure and the presence of `signals`. Please also assert that `b-thin` and `b-prior` ids are absent from the JSON rows under default thresholds, so both output modes enforce the same core-filter semantics.
```suggestion
def test_core_json_parses(isolated_db: Path) -> None:
b = _seed_store(isolated_db)
_, out = _run("core", "--json")
rows = json.loads(out)
assert isinstance(rows, list)
# core filter should exclude non-core beliefs in JSON mode as well
thin_id = b["b-thin"].id
prior_id = b["b-prior"].id
row_ids = {row["id"] for row in rows}
assert thin_id not in row_ids
assert prior_id not in row_ids
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+148
to
+152
| def test_core_json_parses(isolated_db: Path) -> None: | ||
| _seed_store(isolated_db) | ||
| _, out = _run("core", "--json") | ||
| rows = json.loads(out) | ||
| assert isinstance(rows, list) |
There was a problem hiding this comment.
suggestion (testing): Align JSON scenario coverage with the default text scenario by asserting that non-core beliefs are excluded from JSON output as well.
In the text tests we assert that b-thin and b-prior are excluded from aelf core output. Here, the JSON tests only verify structure and the presence of signals. Please also assert that b-thin and b-prior ids are absent from the JSON rows under default thresholds, so both output modes enforce the same core-filter semantics.
Suggested change
| def test_core_json_parses(isolated_db: Path) -> None: | |
| _seed_store(isolated_db) | |
| _, out = _run("core", "--json") | |
| rows = json.loads(out) | |
| assert isinstance(rows, list) | |
| def test_core_json_parses(isolated_db: Path) -> None: | |
| b = _seed_store(isolated_db) | |
| _, out = _run("core", "--json") | |
| rows = json.loads(out) | |
| assert isinstance(rows, list) | |
| # core filter should exclude non-core beliefs in JSON mode as well | |
| thin_id = b["b-thin"].id | |
| prior_id = b["b-prior"].id | |
| row_ids = {row["id"] for row in rows} | |
| assert thin_id not in row_ids | |
| assert prior_id not in row_ids |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
This PR is now behind Auto-rebase was removed because the bot has no signing key; rebasing as the bot strips author signatures and the |
Collaborator
Author
|
[claim:review:setr:2026-05-07T00:20:09Z] |
Collaborator
Author
|
[claim:review:Gylf:2026-05-07T00:20:32Z] |
Collaborator
Author
|
[release:review:Gylf:2026-05-07T00:20:37Z] |
Collaborator
Author
|
[claim:review:Toug:2026-05-07T00:22:14Z] |
Collaborator
Author
|
[release:review:Toug:2026-05-07T00:22:18Z] |
Collaborator
Author
ReviewReviewed at HEAD Substance — LGTM:
CI: all required checks green at HEAD. Discretion grep: clean. Signatures: all 5 commits show Required change — rebase only:
Releasing review claim. |
Collaborator
Author
|
[release:review:setr:2026-05-07T00:23:54Z] |
robotrocketscience
added a commit
that referenced
this pull request
May 7, 2026
Sort path in _emit_core dereferences alpha/(alpha+beta) on every belief; a malformed belief with alpha=beta=0 would raise ZeroDivisionError and crash 'aelf core'. The JSON path already guarded this case (line 1342: 'round(alpha / ab, 3) if ab else 0.0'); the sort path didn't. Mirror the same fallback (μ=0.0 when α+β==0) in the inner _posterior helper. Add a regression test (test_core_zero_alpha_beta_does_not_crash) that constructs a Belief with alpha=beta=0 and asserts 'aelf core' returns 0 without raising. Found by Sourcery review on PR #463.
robotrocketscience
force-pushed
the
feat/issue-439-aelf-core-impl
branch
from
May 7, 2026 01:53
926b9d3 to
8077883
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/test_cli_core.py (1)
133-136:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUnused variable
b(CodeQL flagged at lines 134 and 140)
_seed_storeis called for its side effects (populating the DB); the returneddictis never used in either test. Replace both assignments with bare calls.🔧 Proposed fix
def test_core_tag_block_corr(isolated_db: Path) -> None: - b = _seed_store(isolated_db) + _seed_store(isolated_db) _, out = _run("core") assert "CORR=3" in out def test_core_tag_block_posterior(isolated_db: Path) -> None: - b = _seed_store(isolated_db) + _seed_store(isolated_db) _, out = _run("core") assert "μ=0.800" in outAlso applies to: 139-143
🤖 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 `@tests/test_cli_core.py` around lines 133 - 136, The test assigns the return value of _seed_store to an unused variable b in test_core_tag_block_corr (and the similar test around lines 139-143); remove the unused assignment and call _seed_store(isolated_db) as a bare call so the DB is populated for side effects only, leaving the rest of the test (calls to _run("core") and assertions) unchanged.
🤖 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/cli.py`:
- Around line 1286-1296: The division by ab (alpha+beta) in multiple places can
raise ZeroDivisionError when ab == 0 and --min-alpha-beta is 0; update the three
sites that compute (alpha / ab) to guard the division the same way _posterior
does by requiring ab > 0 (in addition to the existing threshold check) —
specifically modify the conditional in _qualifies_core and both places inside
_emit_core (JSON and text emission paths) to include ab > 0 before performing
alpha/ab so the division never occurs when ab == 0; also consider adding a
regression test that calls the existing zero-ab belief test with
--min-alpha-beta 0.
---
Duplicate comments:
In `@tests/test_cli_core.py`:
- Around line 133-136: The test assigns the return value of _seed_store to an
unused variable b in test_core_tag_block_corr (and the similar test around lines
139-143); remove the unused assignment and call _seed_store(isolated_db) as a
bare call so the DB is populated for side effects only, leaving the rest of the
test (calls to _run("core") and assertions) unchanged.
🪄 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: 770622e2-15d9-42ad-b64b-85121984c1a6
📒 Files selected for processing (5)
docs/COMMANDS.mdsrc/aelfrice/cli.pysrc/aelfrice/slash_commands/core.mdtests/test_cli_core.pytests/test_slash_commands.py
yoshi280
added
attn:review
Collaborator
Author
|
[claim:review:Kulili:2026-05-07T18:47:28Z] |
|
This PR is now behind Auto-rebase was removed because the bot has no signing key; rebasing as the bot strips author signatures and the |
Collaborator
Author
|
[claim:review:Gylf:2026-05-07T18:49:09Z] |
Collaborator
Author
Review verdict — KuliliSpec conformance (
Code quality: ✅
Discretion grep ( CI: all green (pytest 3.12/3.13, CodeQL, Sourcery, CodeRabbit, Staging Gate full suite, deptry, vulture, typos). Commits: all signed ( Blockers (not author-fixable)
Recommended pathOnce operator clears No diff-level changes requested. Releasing review claim. |
Collaborator
Author
|
[release:review:Kulili:2026-05-07T18:49:50Z] |
Collaborator
Author
|
Review: approve in substance, blocked on rebase. Substance — passImplementation matches
Surface
Blocker — rebase needed
Per branch protection / two-repo workflow rule 6, merge requires a signed FF push. Cannot be done by reviewer — author rebase + force-push of the branch is the path. Once rebased and CI re-passes, this is ready to ship. Releasing the review claim so another session can land it after rebase. |
Collaborator
Author
|
[release:review:Gylf:2026-05-07T18:51:54Z] |
yoshi280
added
attn:merge-conflict
|
This PR is now behind Auto-rebase was removed because the bot has no signing key; rebasing as the bot strips author signatures and the |
Collaborator
Author
|
[claim:review:Toug:2026-05-07T21:31:33Z] |
robotrocketscience
added a commit
that referenced
this pull request
May 7, 2026
Sort path in _emit_core dereferences alpha/(alpha+beta) on every belief; a malformed belief with alpha=beta=0 would raise ZeroDivisionError and crash 'aelf core'. The JSON path already guarded this case (line 1342: 'round(alpha / ab, 3) if ab else 0.0'); the sort path didn't. Mirror the same fallback (μ=0.0 when α+β==0) in the inner _posterior helper. Add a regression test (test_core_zero_alpha_beta_does_not_crash) that constructs a Belief with alpha=beta=0 and asserts 'aelf core' returns 0 without raising. Found by Sourcery review on PR #463.
robotrocketscience
force-pushed
the
feat/issue-439-aelf-core-impl
branch
from
May 7, 2026 21:33
8077883 to
5f6f0f2
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
tests/test_cli_core.py (2)
133-136:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUnused variable
b(also Line 140).
b = _seed_store(isolated_db)is assigned but never referenced; the assertions check raw string matches inout. Both variables can be dropped.🔧 Proposed fix
def test_core_tag_block_corr(isolated_db: Path) -> None: - b = _seed_store(isolated_db) + _seed_store(isolated_db) _, out = _run("core") assert "CORR=3" in outdef test_core_tag_block_posterior(isolated_db: Path) -> None: - b = _seed_store(isolated_db) + _seed_store(isolated_db) _, out = _run("core") assert "μ=0.800" in out🤖 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 `@tests/test_cli_core.py` around lines 133 - 136, The test test_core_tag_block_corr assigns an unused variable b from _seed_store(isolated_db); remove the unnecessary assignment (either drop the call entirely if seeding is not needed, or invoke _seed_store(isolated_db) without assigning its return value to b if you need its side effects). Do the same for the other unused b assignment referenced on Line 140 so both tests no longer assign unused variables; keep the rest of the assertions (the _run("core") and "CORR=3" checks) unchanged.
292-314:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
test_core_zero_alpha_beta_does_not_crashdoes not cover the unguardedalpha / abpath.The test runs with default thresholds (
min_alpha_beta=4), so the zero-ab belief is short-circuited byab >= 4before any division is attempted. TheZeroDivisionErrorat Lines 1294, 1335, and 1360 ofcli.py(when--min-alpha-beta 0is passed with anα+β == 0belief) is still unreachable by this test.A companion variant like the following would provide the regression coverage:
def test_core_zero_alpha_beta_with_min_ab_zero_does_not_crash(isolated_db: Path) -> None: s = MemoryStore(str(isolated_db)) try: s.insert_belief(_make_belief("b0zeroab0000000000", "zero ab", alpha=0.0, beta=0.0)) finally: s.close() code, _ = _run("core", "--min-alpha-beta", "0") assert code == 0🤖 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 `@tests/test_cli_core.py` around lines 292 - 314, Add a companion test that actually exercises the unguarded division path by running the CLI with min-alpha-beta set to 0: create a new test function (e.g. test_core_zero_alpha_beta_with_min_ab_zero_does_not_crash) that uses MemoryStore and _make_belief to insert a belief with alpha=0.0 and beta=0.0, closes the store, calls _run("core", "--min-alpha-beta", "0"), and asserts the exit code is 0; this ensures the code paths in cli.py that compute alpha/ab (referenced by the current test and the _run invocation) do not raise ZeroDivisionError.src/aelfrice/cli.py (1)
1294-1294:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
ZeroDivisionErrorwhen--min-alpha-beta 0with anα+β == 0belief — three sites still unguarded.Lines 1294, 1335, and 1360 all compute
alpha / abguarded only byab >= args.min_alpha_beta. When--min-alpha-beta 0is passed,0 >= 0isTrueand the division executes. The_posteriorinner function at Line 1312 already has theab > 0guard; the three sibling expressions need the same treatment.🤖 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 `@src/aelfrice/cli.py` at line 1294, The condition that computes posterior proportion (alpha / ab) is vulnerable to ZeroDivisionError when ab == 0 and args.min_alpha_beta == 0; update the three places that currently check "ab >= args.min_alpha_beta and (alpha / ab) >= args.min_posterior" (and the two other sibling expressions that perform alpha / ab) to require ab > 0 before performing the division—i.e., change the guard to "ab > 0 and ab >= args.min_alpha_beta and (alpha / ab) >= args.min_posterior" (or equivalent short-circuit ordering) so the division is only executed when ab > 0; mirror the same ab > 0 guard used in the existing _posterior inner function to ensure consistent behavior.
🧹 Nitpick comments (2)
src/aelfrice/cli.py (2)
1328-1328: ⚡ Quick winUse
LOCK_NONEconstant instead of the hardcoded string"none"(Lines 1328, 1356, 1381).
LOCK_NONEis not currently imported incli.py(onlyLOCK_USERis). Three new comparisons use the raw string literal, which silently diverges if the model constant ever changes.♻️ Proposed fix
Add
LOCK_NONEto the existingaelfrice.modelsimport block:from aelfrice.models import ( CORROBORATION_SOURCE_CLI_REMEMBER, ... + LOCK_NONE, LOCK_USER, ... )Then replace the three hardcoded comparisons:
- if b.lock_level != "none": # type: ignore[attr-defined] + if b.lock_level != LOCK_NONE: # type: ignore[attr-defined](apply identically at Lines 1328, 1356, and 1381)
🤖 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 `@src/aelfrice/cli.py` at line 1328, Import the LOCK_NONE constant from aelfrice.models alongside the existing LOCK_USER import, and replace the three occurrences that compare against the hardcoded string ("none")—e.g., expressions like b.lock_level != "none"—with comparisons against LOCK_NONE (e.g., b.lock_level != LOCK_NONE) so the code uses the canonical constant; the replacements appear near the uses of variable b.lock_level and should be applied identically at each occurrence.
1379-1384: ⚖️ Poor tradeoffN+1 store round-trips in
_cmd_corefor large belief stores.
list_belief_ids()+get_belief(bid)per ID is O(N) SQLite lookups. For large stores this could be noticeably slow on the hot loop. The pattern is consistent with_wonder_pick_seed, and the spec explicitly forbids a new store method, but a futurelist_beliefs_with_lock_none()API would cut this to one query.🤖 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 `@src/aelfrice/cli.py` around lines 1379 - 1384, The loop in _cmd_core does N+1 store round-trips by calling store.list_belief_ids() then store.get_belief(bid) for each id; add a new store method list_beliefs_with_lock_none() (or a single-query iterator that yields full belief objects with lock_level == "none") and replace the loop in _cmd_core (and the similar loop in _wonder_pick_seed) to iterate over those belief objects and apply _qualifies_core(b, args) to build candidates, eliminating per-id get_belief calls.
🤖 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.
Duplicate comments:
In `@src/aelfrice/cli.py`:
- Line 1294: The condition that computes posterior proportion (alpha / ab) is
vulnerable to ZeroDivisionError when ab == 0 and args.min_alpha_beta == 0;
update the three places that currently check "ab >= args.min_alpha_beta and
(alpha / ab) >= args.min_posterior" (and the two other sibling expressions that
perform alpha / ab) to require ab > 0 before performing the division—i.e.,
change the guard to "ab > 0 and ab >= args.min_alpha_beta and (alpha / ab) >=
args.min_posterior" (or equivalent short-circuit ordering) so the division is
only executed when ab > 0; mirror the same ab > 0 guard used in the existing
_posterior inner function to ensure consistent behavior.
In `@tests/test_cli_core.py`:
- Around line 133-136: The test test_core_tag_block_corr assigns an unused
variable b from _seed_store(isolated_db); remove the unnecessary assignment
(either drop the call entirely if seeding is not needed, or invoke
_seed_store(isolated_db) without assigning its return value to b if you need its
side effects). Do the same for the other unused b assignment referenced on Line
140 so both tests no longer assign unused variables; keep the rest of the
assertions (the _run("core") and "CORR=3" checks) unchanged.
- Around line 292-314: Add a companion test that actually exercises the
unguarded division path by running the CLI with min-alpha-beta set to 0: create
a new test function (e.g.
test_core_zero_alpha_beta_with_min_ab_zero_does_not_crash) that uses MemoryStore
and _make_belief to insert a belief with alpha=0.0 and beta=0.0, closes the
store, calls _run("core", "--min-alpha-beta", "0"), and asserts the exit code is
0; this ensures the code paths in cli.py that compute alpha/ab (referenced by
the current test and the _run invocation) do not raise ZeroDivisionError.
---
Nitpick comments:
In `@src/aelfrice/cli.py`:
- Line 1328: Import the LOCK_NONE constant from aelfrice.models alongside the
existing LOCK_USER import, and replace the three occurrences that compare
against the hardcoded string ("none")—e.g., expressions like b.lock_level !=
"none"—with comparisons against LOCK_NONE (e.g., b.lock_level != LOCK_NONE) so
the code uses the canonical constant; the replacements appear near the uses of
variable b.lock_level and should be applied identically at each occurrence.
- Around line 1379-1384: The loop in _cmd_core does N+1 store round-trips by
calling store.list_belief_ids() then store.get_belief(bid) for each id; add a
new store method list_beliefs_with_lock_none() (or a single-query iterator that
yields full belief objects with lock_level == "none") and replace the loop in
_cmd_core (and the similar loop in _wonder_pick_seed) to iterate over those
belief objects and apply _qualifies_core(b, args) to build candidates,
eliminating per-id get_belief calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8c7c53e7-3506-40b4-a592-8e033cb49e5a
📒 Files selected for processing (5)
docs/COMMANDS.mdsrc/aelfrice/cli.pysrc/aelfrice/slash_commands/core.mdtests/test_cli_core.pytests/test_slash_commands.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/aelfrice/slash_commands/core.md
- docs/COMMANDS.md
robotrocketscience
added a commit
that referenced
this pull request
May 7, 2026
Sort path in _emit_core dereferences alpha/(alpha+beta) on every belief; a malformed belief with alpha=beta=0 would raise ZeroDivisionError and crash 'aelf core'. The JSON path already guarded this case (line 1342: 'round(alpha / ab, 3) if ab else 0.0'); the sort path didn't. Mirror the same fallback (μ=0.0 when α+β==0) in the inner _posterior helper. Add a regression test (test_core_zero_alpha_beta_does_not_crash) that constructs a Belief with alpha=beta=0 and asserts 'aelf core' returns 0 without raising. Found by Sourcery review on PR #463.
robotrocketscience
force-pushed
the
feat/issue-439-aelf-core-impl
branch
from
May 7, 2026 21:37
5f6f0f2 to
74b0219
Compare
Collaborator
Author
|
[release:review:Toug:2026-05-07T21:37:29Z] |
|
This PR is now behind Auto-rebase was removed because the bot has no signing key; rebasing as the bot strips author signatures and the |
Implements _cmd_core with _qualifies_core and _emit_core helpers. Composition over list_locked_beliefs(), list_belief_ids(), and get_belief() — no new store method. Subparser registered after p_locked with --locked-only/--no-locked as a mutually exclusive group. Spec: docs/feature-aelf-core.md.
Sort path in _emit_core dereferences alpha/(alpha+beta) on every belief; a malformed belief with alpha=beta=0 would raise ZeroDivisionError and crash 'aelf core'. The JSON path already guarded this case (line 1342: 'round(alpha / ab, 3) if ab else 0.0'); the sort path didn't. Mirror the same fallback (μ=0.0 when α+β==0) in the inner _posterior helper. Add a regression test (test_core_zero_alpha_beta_does_not_crash) that constructs a Belief with alpha=beta=0 and asserts 'aelf core' returns 0 without raising. Found by Sourcery review on PR #463.
Sourcery flagged a semantics mismatch: help text said '0 disables' but the implementation reads as 'threshold = 0 admits any non-negative value'. Existing test test_core_disabled_posterior_and_corr_includes_all_nonprior asserts b-thin (posterior-only candidate) is included with --min-posterior 0.0 --min-alpha-beta 0, which only holds under the lowering reading — so the implementation is intentional and the test pins it; only the help text was misleading. Reword help to 'lower to widen lens — 0 admits …' so the documented behavior matches code + existing test. No semantics change. Spec memo (docs/feature-aelf-core.md, PR #456) flag table also says '0 disables'; that is a separate doc-edit follow-up — not flipped here to keep this PR focused on the Sourcery findings.
Sourcery noted that test_core_default_includes_* asserts b-thin and b-prior are excluded from default text output, but test_core_json_parses only checked structure — the JSON path could silently regress and emit non-core rows without the test catching it. Mirror the text-mode exclusion assertion: at default thresholds, the JSON row set must not contain b-thin or b-prior ids.
Replace three hardcoded "none" string literals in _emit_core / _cmd_core with the LOCK_NONE constant from aelfrice.models, matching the existing LOCK_USER usage pattern elsewhere in cli.py. Sourcery nit; no behavior change.
Three sites in _qualifies_core / _emit_core (JSON + text paths) performed alpha/ab guarded only by `ab >= args.min_alpha_beta`. With `--min-alpha-beta 0` (a documented valid value: 'admits any belief that passes --min-alpha-beta'), a belief with α+β==0 satisfies the gate and the division raises ZeroDivisionError. The `_posterior` inner function already had the `ab > 0` guard from #439; its three siblings did not. Add the same guard to all three sites. New regression test `test_core_zero_alpha_beta_with_min_ab_zero_does_not_crash` exercises the unguarded path explicitly — the existing `test_core_zero_alpha_beta_does_not_crash` only covered the sort path because default `min_alpha_beta=4` short-circuits before the division. Refs CodeRabbit + Sourcery review on #463.
`test_core_tag_block_corr` and `test_core_tag_block_posterior` assigned `b = _seed_store(isolated_db)` but never referenced `b`; the assertions match raw substrings in stdout. Replace with bare `_seed_store(isolated_db)` calls to clear the CodeQL py/unused-local-variable warnings.
robotrocketscience
force-pushed
the
feat/issue-439-aelf-core-impl
branch
from
May 8, 2026 04:00
74b0219 to
a89bd81
Compare
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
3 participants
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 #439.
Implements
aelf coreper the merged spec memo (docs/feature-aelf-core.md,PR #456). Surfaces the load-bearing subset of the belief store: locked ∪
{corroboration ≥ 2} ∪ {posterior ≥ 2/3 with α+β ≥ 4}. Read-only.
What's in here
_cmd_core+_qualifies_core+_emit_coreinsrc/aelfrice/cli.py(composition over
list_locked_beliefs/list_belief_ids/get_belief— no new store method, per spec § "Why no new store method").
add_mutually_exclusive_group()for--locked-only/--no-locked(argparse handles exit 2 on conflict).tests/test_cli_core.py— 26 unit tests against the spec's 5-belieffixture matrix; covers all 8 spec test scenarios (default text, JSON
round-trip + signals list,
--locked-only,--no-locked, mutualexclusion,
--limit, empty store, threshold flags).src/aelfrice/slash_commands/core.md(mirrorsunlock.md).coreadded toEXPECTED_COMMANDSintests/test_slash_commands.py.docs/COMMANDS.mdrow inserted betweenlockedandunlock.Notable spec calls
_emit_corerecomputes signal attribution per row from the thresholdargs so the JSON
signalslist and the text tag block both reflect thecurrent gates, not just whether the belief survived
_qualifies_core.list_locked_beliefs(alreadyORDER BY locked_at DESC, id ASC— matches spec).record_corroborationAPIrather than direct SQL.
Out of scope (deferred per spec)
pass first.
aelf core --explain <id>.Test plan
uv run pytest tests/test_cli_core.py tests/test_slash_commands.py— 136 passed.uv run aelf core --help— argparse registration works.AELFRICE_DB=/tmp/empty.db uv run aelf core→no core beliefs, exit 0.sonnet|opus|claude|setr|kulili|gylf|toug|...) onthe diff vs
github/main— clean.Summary by Sourcery
Add a new read-only
coreCLI subcommand and corresponding slash command to surface load-bearing beliefs based on lock status, corroboration, and posterior thresholds.New Features:
aelf coreCLI command to list load-bearing beliefs with JSON and text output modes, result limiting, and configurable corroboration/posterior thresholds.coreas a slash command endpoint that runs the new CLI and returns its output verbatim.Enhancements:
corecommand and its flags in the main commands reference alongside existing belief-management commands.Tests:
aelf corecovering default behavior, JSON output, locking filters, mutual exclusion of flags, limiting, empty-store handling, and threshold configuration.corecommand so the CLI and slash command interfaces stay in sync.Summary by CodeRabbit
New Features
aelf corecommand to surface load-bearing beliefs with locking, corroboration, and posterior/alpha-beta thresholds; supports--locked-only/--no-locked, threshold flags,--limit, and JSON or human-readable output.Documentation
aelf:core.Tests