Skip to content

feat(onboard): rejection ledger so re-runs don't re-classify rejected sentences (#801) - #806

Merged
github-actions[bot] merged 6 commits into
mainfrom
feat/issue-801-onboard-rejection-ledger
May 14, 2026
Merged

github-actions[bot] merged 6 commits into
mainfrom
feat/issue-801-onboard-rejection-ledger

Conversation

@robotrocketscience

Copy link
Copy Markdown
Owner

Summary

Closes #801. aelf onboard <path> historically re-classified every
sentence the host had previously rejected with persist=False, because
rejected sentences are never stored as beliefs and the dedup filter in
start_onboard_session only consults beliefs.id. The recorded repro
in the issue: pass 1 emits 2276 candidates → 723 rejected; pass 2 emits
726 candidates ≈ the 723 rejects + a few new files; pass 3 the same
again. Each pass burns one classification round-trip per re-rejected
sentence.

This PR persists (belief_id, text, source, rejected_at) rows in a new
onboard_rejections table from accept_classifications whenever the
host returns persist=False, and start_onboard_session +
check_onboard_candidates filter against that table the same way they
already filter against beliefs. aelf onboard --force bypasses the
ledger so the operator can re-roll a prior pass they think was too
aggressive.

Behavior change

Pass before after
1 (fresh, all rejected) 2276 → 723 rejected unchanged
2 (no file changes) 726 candidates 0 candidates
2 with --force (no such flag) re-emits the 723
2 after pass-1 accepted everything 0 candidates unchanged
2 with --force after pass-1 accept (no such flag) 0 candidates (already-present still filtered)

The slash command's n_new == 0 short-circuit (onboard.md step 2)
keeps working because rejected sentences move out of n_new into the
new n_already_rejected bucket — no slash-command change needed.

Files changed

  • src/aelfrice/store.pyonboard_rejections table in _SCHEMA
    (forward-compat via CREATE TABLE IF NOT EXISTS); five new
    MemoryStore methods (insert_onboard_rejection,
    delete_onboard_rejection, is_onboard_rejected,
    list_onboard_rejection_ids, count_onboard_rejections).
  • src/aelfrice/classification.pystart_onboard_session and
    check_onboard_candidates gain a force: bool = False kwarg, bucket
    candidates into already_present / already_rejected / new, and
    return a new n_already_rejected field on both result dataclasses
    (default 0, additive for existing callers). accept_classifications
    writes a ledger row on persist=False and deletes any prior row on
    persist=True so a rejected-then-accepted sentence leaves the ledger.
  • src/aelfrice/cli.pyaelf onboard --force flag; --check output
    gains an already rejected: line; --emit-candidates JSON gains
    n_already_rejected.
  • tests/test_onboard_rejection_ledger.py — 16 new tests covering
    store CRUD, accept-time ledger writes/deletes, emit + check filter
    behaviour, the force=True bypass, and the recorded repro.
  • tests/test_cli_onboard_handshake.py — 4 new CLI cases for the
    flag and the new output fields.
  • CHANGELOG.md — Unreleased / Fixed entry.

Design notes

belief_id derivation is shared between the rejection ledger and the
existing dedup path (_derive_belief_id(text, source) =
sha256(source\0text)[:16]), so the filter key matches the write key
without waiting for run_worker to assign a canonical id. The table
is per-project (lives in the same brain.db as beliefs), so the
"per-project rejection memory" semantics fall out for free.

--force only re-opens the noise lane. Already-present beliefs stay
filtered, by design — the issue's --force proposal is about
re-rolling a prior classification verdict, not duplicating stored
content. Verified by a dedicated test
(test_already_present_still_filtered_under_force).

Test plan

  • uv run pytest -x -q → 4183 passed, 62 skipped, 75 xfailed.
  • End-to-end smoke: reject all → re-emit returns 0 sentences; reject
    all → re-emit --force returns the original N; reject then
    --force accept → ledger drops to 0.
  • Discretion grep on full diff vs github/main clean.

Alternatives considered

  • Reuse ingest_log with a rejected=1 flag. Rejected sentences
    are not ingests — they were never written to the belief substrate —
    so a separate table keeps the ingest-log audit trail clean.
  • Project-level "already onboarded" flag (the issue's first
    alternative). Rejected because it blocks the legitimate "I added new
    docs, re-onboard the directory" case unless --force is used; the
    ledger handles incremental onboarding naturally.
  • Classifier-version tracking on rejection rows. Considered, then
    dropped — --force already covers "I want to re-roll because the
    prior pass was too aggressive". Adding a version column means
    arguing about what counts as a version change; the simple flag is
    enough.

Out of scope

  • No migration backfill — the table starts empty on existing stores
    and fills as new rejections happen. No retroactive promotion of
    already-rejected content from prior runs (those are lost; they will
    be re-rejected on the next pass and then stick).
  • Rejection-ledger GC / expiry. The ledger grows monotonically on
    the rejected lane until --force accepts a row. If this becomes a
    size issue, a follow-up can add aelf onboard --clear-rejections
    or an age-based GC. Today the bound is "everything that ever got a
    persist=False verdict", which is finite per repo.

Adds a per-DB ledger keyed on the same `belief_id` SHA used by
`beliefs.id`. Rejections and accepted beliefs share a key space so a
later persist=True moves the row out of this table and into `beliefs`.
Store methods (insert/delete/is_/list_/count_) follow the existing
onboard_sessions surface. Forward-compat: CREATE TABLE IF NOT EXISTS
leaves prior schemas untouched and the table starts empty on existing
stores.
On host verdict persist=False, record `(belief_id, text, source, ts)`
into onboard_rejections so the next emit does not re-classify the
same sentence. On persist=True, delete any prior ledger row for that
belief_id — a rejected-then-accepted sentence (e.g. via --force) leaves
the ledger when its canonical belief lands in `beliefs`.

belief_id is computed with the same _derive_belief_id() the dedup path
in start_onboard_session uses, so the rejection key matches the filter
key without waiting for the worker to assign a canonical id.
start_onboard_session and check_onboard_candidates now load the
rejection-ledger id set once per call and bucket candidates into
already_present / already_rejected / new. Both result dataclasses
carry a new `n_already_rejected` field (default 0, additive for
existing callers).

`force=True` (kwarg, default False) bypasses the ledger so a re-run
re-emits previously rejected sentences. Already-present beliefs stay
filtered either way — force only opts back in to noise the classifier
previously discarded, not to content the store already holds.
`aelf onboard <path> --force` (works with --emit-candidates and
--check) bypasses the rejection ledger so previously rejected
candidates re-emit. Default stays ledger-on, so the common case is the
new no-op behavior the issue asks for.

`--check` output gains an `already rejected: <N> candidates` line
above `new since last onboard:`. The slash command's `n_new == 0`
short-circuit (onboard.md step 2) still works as-is — rejected
sentences just move from `new` into the new bucket.

`--emit-candidates` JSON gains `n_already_rejected`. Existing keys
unchanged.
New tests/test_onboard_rejection_ledger.py (16 cases) covers store
CRUD, accept_classifications writes/deletes ledger rows, emit + check
honor the ledger, force=True bypasses it, and the recorded repro
(pass-2 emits 0 candidates after pass-1 rejects all).

tests/test_cli_onboard_handshake.py gains four cases for the CLI
surface: emit-candidates JSON exposes n_already_rejected, --check
reports the new bucket, --force re-emits, and --check --force notes
the ledger bypass.
@robotrocketscience robotrocketscience added the author-prince Authored by parallel session: prince label May 14, 2026

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@coderabbitai

coderabbitai Bot commented May 14, 2026

Copy link
Copy Markdown

Warning

Rate limit exceeded

@robotrocketscience has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 41 minutes and 57 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: acc1d32a-7645-4dbf-bf53-804efdbf15d7

📥 Commits

Reviewing files that changed from the base of the PR and between e52d93e and 4b22eeb.

⛔ Files ignored due to path filters (1)
  • CHANGELOG.md is excluded by !**/CHANGELOG.md
📒 Files selected for processing (5)
  • src/aelfrice/classification.py
  • src/aelfrice/cli.py
  • src/aelfrice/store.py
  • tests/test_cli_onboard_handshake.py
  • tests/test_onboard_rejection_ledger.py
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/issue-801-onboard-rejection-ledger

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.

❤️ Share

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

@robotrocketscience robotrocketscience added the attn:review Needs review (PR open, awaiting reviewer) label May 14, 2026
@github-actions

github-actions Bot commented May 14, 2026

Copy link
Copy Markdown

PR-size soft cap

This PR is over the advisory size threshold:

  • 508 changed lines (limit: 200)
  • 6 changed files (limit: 3)

Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated attn:merge-conflict cycles (see #602). When practical, split into smaller PRs that each touch a focused surface.

This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the size:override label and this comment will be removed on the next push.

@robotrocketscience

Copy link
Copy Markdown
Owner Author

[claim:review:sankara:2026-05-14T18:52:27Z]

@robotrocketscience

Copy link
Copy Markdown
Owner Author

LGTM. Six atomic signed commits, all CI green, mergeable, discretion-clean, 24 new tests including the recorded repro (pass-2 → 0 candidates after rejecting pass-1). Ledger keyed on the same sha256(source\0text)[:16] as beliefs.id so the filter matches writes without round-tripping through run_worker. Forward-compat via CREATE TABLE IF NOT EXISTS, additive dataclass fields default to 0, and the rejected-then-accepted-under---force path deletes the stale row so the ledger doesn't accumulate ghosts.

Two non-blocking observations, both follow-up-shaped:

  1. --force is silently no-op on the top-level aelf onboard <path> entry. The flag is only consulted in _cmd_onboard_emit_candidates / _cmd_onboard_check. If the user runs aelf onboard . --force without --check or --emit-candidates, the call falls through to _run_regex_onboard / _run_llm_onboard and --force is ignored. Help text already says "Apply on the --emit-candidates or --check entry", so it's at least documented, but an argparse warning (or routing --force through scan_repo too) would be a nicer UX. Out of scope here.

  2. Same bug pattern survives in scan_repo. The in-process LLM path (aelf onboard . --llm-classify, no handshake) hits _run_llm_onboardscan_repo, whose skipped_non_persisting branches (src/aelfrice/scanner.py:268-271, 317) drop persist=False verdicts on the floor with no ledger write. A second LLM-classify run will re-classify the same rejects. The PR's framing is "the host classifier" (handshake terminology), and the recorded repro is Haiku-subagents = handshake, so the scope is intentional — but the same fix shape (store.insert_onboard_rejection on not route.persist, and consult the ledger in scan_repo's candidate loop) would close the LLM-classify side too. Follow-up issue worth filing if the in-process LLM path is still a supported surface.

Approving and adding ready-to-merge.

@robotrocketscience robotrocketscience added ready-to-merge Trigger merge-train: FF main to this PR's head and removed attn:review Needs review (PR open, awaiting reviewer) labels May 14, 2026
@robotrocketscience

Copy link
Copy Markdown
Owner Author

[release:review:sankara:2026-05-14T18:55:05Z]

@github-actions
github-actions Bot merged commit 4b22eeb into main May 14, 2026
32 of 35 checks passed
@github-actions github-actions Bot removed the ready-to-merge Trigger merge-train: FF main to this PR's head label May 14, 2026
@github-actions

Copy link
Copy Markdown

merge-train: merged 4b22eebmain via FF push.

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

Labels

author-prince Authored by parallel session: prince

Projects

None yet

Development

Successfully merging this pull request may close these issues.

aelf onboard <dir> is not idempotent — re-runs re-classify previously-rejected sentences

1 participant