fix(retrieval): HRR lane lock-starvation and clamp_ghosts false positives (#1374) - #1394
Conversation
|
Warning Review limit reached
Next review available in: 26 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe changes update ghost clamping eligibility and add creation-time filtering. They also modify structural HRR retrieval to account for manifest reference locks while preserving a relevance budget floor. Tests cover both behaviors. ChangesGhost clamping eligibility
Structural HRR retrieval
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ClampCLI
participant clamp_ghost_alphas
participant Database
ClampCLI->>clamp_ghost_alphas: pass created_before
clamp_ghost_alphas->>Database: enumerate eligible ghosts
Database-->>clamp_ghost_alphas: matching rows
clamp_ghost_alphas->>Database: recheck eligibility before mutation
Database-->>clamp_ghost_alphas: mutation result
sequenceDiagram
participant retrieve_v2
participant StructuralRetrieval
participant lock_injection_tokens
retrieve_v2->>StructuralRetrieval: pass manifest_reference_locks
StructuralRetrieval->>lock_injection_tokens: calculate lock cost
lock_injection_tokens-->>StructuralRetrieval: locked_used
StructuralRetrieval->>StructuralRetrieval: reserve relevance budget floor
StructuralRetrieval-->>retrieve_v2: packed structural results
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Reviewer's GuideImplements two targeted fixes: (1) the HRR structural retrieval lane now applies the same relevance-budget floor and lock cost accounting as the main textual lane to avoid lock-only results under lock saturation, and (2) the clamp_ghosts tool’s selector is tightened with explicit user-prior origin exclusion and an optional created_before cutoff, plus tests, so freshly-ingested user beliefs are never misclassified as ghosts. Sequence diagram for HRR structural retrieval with relevance-budget floorsequenceDiagram
actor Client
participant Retrieval as _route_structural_query
participant Store
participant HRR as HRRStructIndex
Client->>Retrieval: retrieve_v2(..., manifest_reference_locks)
Retrieval->>Store: list_locked_beliefs()
Store-->>Retrieval: locked
loop compute_locked_used
Retrieval->>Retrieval: lock_injection_tokens(belief, manifest_reference_locks)
end
Retrieval->>Retrieval: relevance_budget = max(int(budget * RELEVANCE_BUDGET_FLOOR_FRACTION), budget - locked_used)
Retrieval->>Retrieval: tail_cap = locked_used + relevance_budget
Retrieval->>HRR: HRRStructIndex(...).query_structural(...)
HRR-->>Retrieval: hits
loop pack_results
Retrieval->>Store: store.get_belief(belief_id)
Store-->>Retrieval: belief
alt [used + _belief_tokens(belief) <= tail_cap]
Retrieval->>Retrieval: append belief
Retrieval->>Retrieval: used += cost
else [limit reached]
Retrieval->>Client: return locked + HRR_tail
end
end
Retrieval-->>Client: return locked + HRR_tail
Flow diagram for clamp_ghost_alphas eligibility and clampingflowchart TD
A[start clamp_ghost_alphas] --> B[select beliefs b]
B --> C{b.lock_level = 'none'?}
C -->|no| Z[end]
C -->|yes| D{b.origin NOT IN USER_PRIOR_ORIGINS?}
D -->|no| Z
D -->|yes| E{b.alpha > threshold_alpha?}
E -->|no| Z
E -->|yes| F{created_before provided?}
F -->|no| H
F -->|yes| G{b.created_at < created_before?}
G -->|no| Z
G -->|yes| H{no feedback_history rows?}
H -->|no| Z
H -->|yes| I{no belief_corroborations rows?}
I -->|no| Z
I -->|yes| J[under write lock: re-check same predicate]
J --> K{still eligible?}
K -->|no| Z
K -->|yes| L[UPDATE beliefs SET alpha = target_alpha]
L --> M[INSERT feedback_history with source = CLAMP_SOURCE]
M --> Z[end]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
robotrocketscience
added
the
attn:review
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 |
![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 1 security issue, and left some high level feedback:
Security issues:
- Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option. (link)
General comments:
- The
eligibility/created_clauseSQL fragments are manually duplicated between the enumeration query and the under-lock recheck; consider extracting a small helper to build the predicate and params so future changes to the ghost selector cannot accidentally diverge between the two paths. - The
created_beforeargument is currently a raw string passed straight into the SQL predicate; it may be safer and clearer to parse/validate it as adatetimeup front (including enforcing timezone/format) and serialize consistently when binding to the query.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `eligibility`/`created_clause` SQL fragments are manually duplicated between the enumeration query and the under-lock recheck; consider extracting a small helper to build the predicate and params so future changes to the ghost selector cannot accidentally diverge between the two paths.
- The `created_before` argument is currently a raw string passed straight into the SQL predicate; it may be safer and clearer to parse/validate it as a `datetime` up front (including enforcing timezone/format) and serialize consistently when binding to the query.
## Individual Comments
### Comment 1
<location path="src/aelfrice/clamp_ghosts.py" line_range="275-284" />
<code_context>
current = conn.execute(
"SELECT b.alpha AS alpha "
"FROM beliefs b "
"WHERE b.id = ? "
" AND b.lock_level = 'none' "
" AND b.alpha > ? "
+ created_clause
+ eligibility,
recheck_params,
).fetchone()
</code_context>
<issue_to_address>
**security (python.sqlalchemy.security.sqlalchemy-execute-raw-query):** Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
[claim:review:Idnn:2026-08-06T04:46:39Z] |
Review — both fixes are correct. One completeness gap in §12's argument, not in its code.Verified against source rather than the body. §1 reuses the mechanism rather than re-deriving it — confirmedThe floor is character-for-character the shape the textual path uses: relevance_budget = max(int(budget * RELEVANCE_BUDGET_FLOOR_FRACTION),
budget - locked_used) # retrieval.py, HRR lane
relevance_budget = max(int(effective_budget * RELEVANCE_BUDGET_FLOOR_FRACTION),
effective_budget - locked_used) # retrieval.py:4509, textualand Mutation: Non-blocking observation, pre-existing and not introduced here: the HRR lane §12's code is right; the docstring's completeness argument is notThe fix works and the test genuinely distinguishes — the mutation I checked the completeness claim rather than accepting it, because the exclusion For the classifier path the argument holds exactly: the maximum α reachable on a But derive(DerivationInput(..., route_overrides=RouteOverrides(
belief_type="factual", origin=ORIGIN_AGENT_INFERRED,
alpha=9.0, beta=0.5, audit_source=None)))
-> origin agent_inferred (not excluded), alpha 9.0 (over threshold)With So the sentence "Non-user origins get α deflated by I am not asking you to widen the exclusion. An arbitrary router-assigned α State
Nothing blocking — the docstring qualification is a should-fix I'd like before |
|
Took the docstring qualification myself in It now separates the two paths explicitly: complete on the deterministic classifier path (max reachable α there is 1.8), with
Driving this to merge. |
ae8b732 to
faa94b9
Compare
|
[claim:review:Garsecg:2026-08-06T04:55:22Z] |
|
[release:review:Garsecg:2026-08-06T04:55:26Z] |
Blocked on one non-required bot check, and the call is yoursEverything else is done: threads resolved, rebased onto current main, FF, and all
The failure is the SQL-injection finding I resolved as a false positive above To restate the verification: Two ways forward — your call, I did not pick one
I lean (1) — the finding is verified false and contorting the code teaches the Flagging |
robotrocketscience
added
attn:unblock
|
[release:review:Idnn:2026-08-06T04:56:22Z] |
|
[claim:review:Garsecg:2026-08-06T05:01:39Z] |
Review — §1 is clean, §12's fix is right but its safety argument rests on a number the live store contradictsBoth defects are real and the fixes are the right shape. One finding, on the reasoning rather than the code. §1 — verifiedReusing §12 — the fix is right; two things about the justificationThe selector change is correct, and putting a would-have-been-clamped belief in the fixture is the right answer to the "green against a store with nothing clampable proves nothing" trap. The stated deterministic ceiling is wrong. The docstring argues:
Against the live store, the maximum α among rows that satisfy every other arm of the selector — non-user origin, no feedback row, no corroboration row, unlocked — is 3.0, and 260 such rows sit above 1.8: The conclusion still holds today — 0 rows clamp at the shipped threshold — but the margin is 4.0 against an observed 3.0, not against 1.8. That is a headroom of 1.0 rather than 2.2, less than half what the argument claims, and nothing pins it: a future change to Worth finding out what writes 3.0 before deciding what to do about it. It is not corroboration growth — those rows have no Live reach is currently zero. The shipped selector matches 0 rows on the development store at I checked whether the four excluded origins are the right four and they are — but not for the reason the docstring gives. Suggested
HousekeepingThe branch is behind Flagging |
robotrocketscience
added
attn:unblock
|
[release:review:Garsecg:2026-08-06T05:05:06Z] |
|
[claim:review:Idnn:2026-08-06T15:53:10Z] |
faa94b9 to
fe692c6
Compare
Took the three review items, and the escalated merge decision did not need to be onePushed four commits. Summary first, then the part that changes the picture: the α=3.0 rows are not on The Sourcery blocker — decided by precedent, not by a new judgment callThe escalation offered two options: merge past the red non-required check, or restructure. There is a ratified precedent covering exactly this, and it splits on a distinction neither option captured:
The Worth flagging: the review's own suggested restructure would not have worked. Hoisting construction into a local variable is not enough — semgrep's rule carries an explicit assign-then-execute alternative (
Your sharing guarantee survives and is now structural rather than comment-enforced — one module constant spliced into both selectors, with a test asserting each contains it. That was the right instinct in your comment; it just wasn't checkable before. Two things I found mutation-testing my own change, neither of which the existing suite distinguished:
I did independently confirm the finding is a false positive before working around it, by instrumenting Your α ceiling finding — reproduced exactly, then attributedYour three counts reproduce bit-for-bit against the repo-local store ( But the inference "something writes up to 3.0 on those origins" does not hold. Nothing writes it. The 260 sit on an exact lattice: β co-sums with α. That is k copies of the deflated factual prior (3.0×0.2, 1.0) added together — Two consequences:
So your 1.8 number is correct. It is correct about the insert path, and the data corroborates it to the decimal. The defect is scope, not arithmetic: it bounds α by source while the selector excludes by origin, and the docstring never said so. Your margin arithmetic is right too, with that scoping — headroom on the insert path is 2.2, and What I could not confirm, and one thing that is worse than reported
Your instinct that And the mechanism putting high α on it is live, which the docstring denies:
Demonstrated rather than argued: built a v1.0-shaped legacy DB with one unlocked
The
|
Collision, and one thing the branch was still missing —
|
| store | matched at α>4.0 | max α |
|---|---|---|
| development | 0 | 3.0000000000000004 |
| another on this machine | 1,310 | 105.00000000000001 |
Both figures observed directly, read-only. A bare "0 rows match, so this is safe"
is not a property of the code.
On the reviewer's two asks
Both are now answered, though not by me: the 1.8 is correct about
get_source_adjusted_prior and is pinned by a test rather than by prose, and
unknown is named. Corroborating one figure the branch asserts — agent_inferred,
the origin the deflation argument actually governs, caps at exactly 1.8 across
11,243 selector-eligible rows on the development store. The entire excess is
unknown.
Not fixed here, worth someone's attention
The 749 unlocked non-user rows above α=4.0 on the development store are excluded
solely by the NOT EXISTS feedback_history arm. Anything that ever prunes or ages
out feedback_history turns all 749 into matches at once. And the 1,310 rows above
α=100 in the table above sit on agent_inferred — the origin the ceiling argument
governs — so something reaches α=105 there that the insert-path bound does not
explain. I did not chase either; say the word and I will file them.
Verification
- full suite: 7398 passed, 70 skipped, 71 xfailed (with the
archiveextra). tests/test_clamp_ghosts.py: 27 passed.- every number above re-derived first-hand, read-only, not taken from the review.
- discretion grep on added lines vs main: clean. All 7 commits signed.
- pushed as a fast-forward on
fe692c6c— no force, nothing overwritten.
The premise in "Blocked on one non-required bot check" is false — option (1) does not existNot a disagreement about the merge risk. The workflow file contradicts the The comment states:
The merge-train does not verify only the required set. [.check_runs[] | select(.name != "Attempt merge-train FF" and .name != "merge")]
| group_by(.name) | map(max_by(.started_at))and fails=$(... select(.c == "failure" or .c == "timed_out" or .c == "action_required") ...)
if [ -n "${fails}" ]; then
fail_and_unlabel "required check(s) failed: ..."
fi
Consequence for this PR: labelling The false-positive verification is independently confirmed and none of this Operator ruling (2026-08-06 ~15:55Z): unblock this PR via option (2) — Also note, unrelated to the bot: this branch is 4 commits behind main Review claim is not mine; leaving the work with the holder. |
Operator ruling — clear the Sourcery red with
|
Ruling applied —
|
89521ea to
f1f46c7
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: 2
🧹 Nitpick comments (2)
src/aelfrice/clamp_ghosts.py (1)
359-363: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider validating the
created_beforeformat.The cutoff reaches SQLite as an opaque string and is compared lexicographically against
created_at. An operator who passes2026-4-1or04/15/2026gets a silent wrong result rather than an error: the first under-selects, the second matches every row whosecreated_atstarts with a digit greater than0. Under--applythat over-clamps.A
datetime.fromisoformatprobe at this point converts the mistake into aValueError, matching how the function already rejects bad α values.♻️ Proposed refactor
created_before = created_before or None + if created_before is not None: + try: + datetime.fromisoformat(created_before.replace("Z", "+00:00")) + except ValueError as exc: + raise ValueError( + f"created_before must be an ISO-8601 timestamp; " + f"got {created_before!r}" + ) from exc🤖 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/clamp_ghosts.py` around lines 359 - 363, Validate non-None created_before with datetime.fromisoformat at the normalization point before it reaches _ELIGIBILITY_SQL, allowing valid ISO timestamps while raising ValueError for malformed formats. Preserve the existing falsy-to-None behavior so empty values still mean no cutoff, and match the existing invalid-alpha error behavior.tests/test_clamp_ghosts.py (1)
523-533: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a negative-
limitcase to this test.This test pins that
limit=Nonereaches every match through the-1sentinel. It leaves the adjacent case unpinned: an explicit negativelimitfrom a caller also becomes uncapped, becauseint(limit)is forwarded verbatim. See the related comment onsrc/aelfrice/clamp_ghosts.py.If you add the validation guard there, pin it here too.
💚 Proposed test
def test_negative_limit_is_rejected_not_treated_as_uncapped( store: MemoryStore, ) -> None: # A negative LIMIT is uncapped in SQLite, so a caller asking for a # cap must not silently get the whole store under --apply. for i in range(5): store.insert_belief(_mk(f"g{i}", alpha=9.0)) with pytest.raises(ValueError, match="limit"): clamp_ghost_alphas(store, dry_run=False, limit=-1)🤖 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_clamp_ghosts.py` around lines 523 - 533, Add a separate test next to test_none_limit_processes_every_match that inserts matching beliefs and asserts clamp_ghost_alphas with dry_run=False and an explicit negative limit raises ValueError matching “limit”. Preserve the existing limit=None uncapped test, and import pytest if needed.
🤖 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/clamp_ghosts.py`:
- Around line 365-367: Reject negative limit values in the validation logic
alongside the existing threshold_alpha and target_alpha checks, while preserving
None as the no-limit sentinel. Also update the clamp-ghosts CLI --limit argument
to use the existing _positive_int validator so invalid values fail before
opening the store.
- Around line 39-42: Update the two paragraph openers in the explanatory text:
make the line 39 statement explicitly identify get_source_adjusted_prior as the
subject instead of using “It,” and make the line 114 statement explicitly refer
to migrate()’s inability to re-run rather than relying on the earlier claim.
Preserve the existing technical meaning and auditable justification.
---
Nitpick comments:
In `@src/aelfrice/clamp_ghosts.py`:
- Around line 359-363: Validate non-None created_before with
datetime.fromisoformat at the normalization point before it reaches
_ELIGIBILITY_SQL, allowing valid ISO timestamps while raising ValueError for
malformed formats. Preserve the existing falsy-to-None behavior so empty values
still mean no cutoff, and match the existing invalid-alpha error behavior.
In `@tests/test_clamp_ghosts.py`:
- Around line 523-533: Add a separate test next to
test_none_limit_processes_every_match that inserts matching beliefs and asserts
clamp_ghost_alphas with dry_run=False and an explicit negative limit raises
ValueError matching “limit”. Preserve the existing limit=None uncapped test, and
import pytest if needed.
🪄 Autofix
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 Plus
Run ID: 2c6c820b-e9b3-4c43-83e1-bbb150906eaa
📒 Files selected for processing (5)
src/aelfrice/clamp_ghosts.pysrc/aelfrice/cli.pysrc/aelfrice/retrieval.pytests/test_clamp_ghosts.pytests/test_retrieve_v2_hrr_structural.py
Both review threads taken — one was a real data-integrity defect, and it predates this PR
The negative
|
`_route_structural_query` charged locks against the whole token budget with no floor, re-introducing #1014 on a lane that is default-ON and returns early: on a lock-saturated store a structural query returned the locks and nothing else. It now measures lock cost with `lock_injection_tokens` (so a reference-tier lock is charged at its manifest line when the caller renders it that way) and caps the HRR tail at `RELEVANCE_BUDGET_FLOOR_FRACTION` of the budget, the same mechanisms the textual path already uses. `manifest_reference_locks` is threaded down from `retrieve_v2`. Byte-identical packing whenever the locks already leave at least the floor. Refs #1374.
The selector keyed only on lock_level, alpha and an empty audit trail, which a freshly-ingested user belief satisfies by construction: the insert path writes the undeflated TYPE_PRIORS alpha (9.0 for a requirement) onto a new row that has no feedback and no corroboration yet, so the tool clamped it and wrote an audit row attributing the clamp to itself. Rows whose origin is user_stated, user_corrected, user_validated or user_transcript are now excluded, and an optional --created-before cutoff lets an operator confine a one-shot clamp to rows predating the migration. Both predicates apply to the enumeration query and the under-write-lock re-check. Refs #1374.
…path The docstring stated a universal: non-user origins are deflated at insert, so no legitimate insert on them can clear the threshold. That is true of the deterministic classifier path — max reachable alpha there is 1.8 — and not true of the LLM-router path, which bypasses get_source_adjusted_prior and writes the router's (origin, alpha) verbatim. A route with origin=agent_inferred, alpha=9.0 and no audit_source matches every arm of the selector. Not widening the exclusion: an arbitrary router-assigned alpha with no audit trail is the class this tool exists to clamp, and excluding agent_inferred wholesale would gut it. The exception is named instead, because this file's value is that its selector's justification is auditable — the guarantee is "no deterministically derived belief is a false positive", not "no belief is". Refs #1374.
… JSON The selector was composed per call: an `IN (?, ?, …)` placeholder run sized from len(USER_PRIOR_ORIGINS), plus a conditionally-appended created_at clause and a conditionally-appended LIMIT. No caller value ever reached the SQL text, but the shape is the one opengrep's sqlalchemy-execute-raw-query rule matches, and it kept the `Sourcery review` check red — which the merge train blocks on. Both queries are now module constants with no interpolation. The origin exclusion arrives as one bound JSON array read by `json_each`, the same mechanism store.list_stale_speculative_ids adopted for the same reason in #1171; the cutoff is a bound `? IS NULL` disjunction; LIMIT is always bound, using SQLite's negative-means-unbounded reading for the no-cap case. There is no placeholder count left to keep in sync with a parameter count. Behaviour is unchanged. `created_before` is normalised falsy-to-None first, so the empty string keeps meaning "no cutoff" rather than "created before the empty string", which under a bound null-check would match nothing. Sharing the predicate between the enumeration query and the under-the-write-lock re-check was already deliberate and is now structural: one module constant, spliced by one parameter helper, with tests asserting both queries contain it. Four tests cover what the existing suite did not distinguish — the empty-string cutoff, the no-cap LIMIT sentinel, the sorted serialisation (monkeypatched, since the shipped tuple is already alphabetical and cannot exercise it), and the shipped origin list written out literally. Refs #1374.
…hold The selector's safety argument was prose: non-user origins deflate to at most 1.8, the threshold is 4.0, therefore no legitimate insert on a clampable origin is selectable. Nothing failed when a prior moved. Now executable. Asserting only the inequality would not be enough — a deflation factor of 0.4 leaves the maximum at 3.6, still under 4.0, while the stated 1.8 quietly becomes false. So the four shipped constants, the derived ceiling, and the inequality are pinned separately, and the margin is named. Sweeps every type the classifier can emit plus an unmapped string, so the unknown-type fallback is covered; repointing that fallback from factual to requirement raises the real ceiling and is caught. Source labels include case and whitespace variants, since the deflation gate is an exact comparison against "user". Six mutants, all killed: deflation 0.2->0.4, requirement prior 9.0->25.0, threshold 4.0->1.5, deflated floor 0.5->5.0, fallback prior factual->requirement, and a case-insensitive deflation gate. The docstring states the scope so this does not read as a stronger guarantee than it is: it bounds alpha by source, and constrains neither route_overrides nor migration-preserved rows on origin='unknown'. Refs #1374.
…nknown'
Review found 260 rows on the development store satisfying every arm of
the selector but the threshold, at alpha up to 3.0 — against a docstring
claiming 1.8 was the maximum reachable. Three corrections, no selector
change; the tool still matches 0 rows there.
The 1.8 bound is right, and is about the INSERT path. It is now pinned
by a test rather than by this paragraph. What the text failed to say is
that it bounds alpha by source while the selector excludes by origin.
The stated LLM-router exception is wrong in both directions. The
mechanism is real — derive() writes route_overrides' (origin, alpha)
verbatim — but neither shipped producer reaches a clampable origin with
an inflated alpha, and they miss it for different reasons.
llm_classifier is restricted to {agent_inferred, document_recent} and
takes its alpha from get_source_adjusted_prior on the candidate's
doc:/ast:/git: label, so it deflates to 1.8, not 9.0.
claude_memory_reconcile does write the undeflated prior up to 9.0, but
on origin=user_validated, which USER_PRIOR_ORIGINS excludes. The
exposure is a future producer, not a current one.
origin='unknown' is the actual gap and is now named. It is the one
clampable origin carrying alpha above 1.8 in practice: migrate() copies
legacy alpha verbatim, stamps 'unknown' on unlocked non-correction rows,
and copies neither feedback_history nor belief_corroborations, so an
earned alpha arrives indistinguishable from a fabricated ghost. Left
clampable deliberately — it is the target population, not a bystander,
and excluding it would make the tool a no-op; --created-before is the
mitigation.
Also corrects the empirical attribution. The live population above 1.8
sits on an exact k*(0.6, 1.0) lattice — k copies of the deflated factual
prior summed — which is _maybe_consolidate_content_hash_duplicates
(#219), not _read_legacy_beliefs, which copies alpha through unchanged
and cannot produce that shape. That consolidation is marker-gated and
cannot re-run, but migrate() can: it is reachable via 'aelf migrate
--apply' and 'aelf doctor', so "one-shot" is a property of a given
store's existing rows, not of the tool. Re-run after any migration.
Refs #1374.
The attribution to _maybe_consolidate_content_hash_duplicates is right, and it raises a question the paragraph left open: consolidation is not a trail-less writer. It inserts one synthetic consolidation_migration corroboration per consumed duplicate, which would have excluded every row it produced. A reader checking the claim finds that insert and concludes the attribution is wrong. It is not. Zero of those corroborations survive, because _maybe_apply_content_hash_unique ran 82 seconds after the dedup marker and its DROP TABLE beliefs cascaded belief_corroborations away wholesale — #336, since fixed by PRAGMA foreign_keys=OFF around the swap. Verified on the development store: dedup marker 01:38:58Z, unique marker 01:40:20Z, earliest surviving corroboration 01:40:55Z against 6,382 beliefs created before it. That strengthens the section's own conclusion rather than qualifying it. A consolidation running today leaves its trail and yields no candidates, so the existing population is residue of two migrations interacting and cannot regrow the same way. Also warns against quoting a headroom figure without its store: the shipped selector matches 0 rows at alpha>4.0 here and 1,310 rows above alpha=100 on another store on this machine. Same code, both. Refs #1374.
…ts two sites The scanner's sqlalchemy-execute-raw-query rule fires on the concatenation that assembles the ghost selector and its under-write-lock recheck. Both query strings are module constants built from string literals alone, and every variable part — threshold, cutoff, origin set, limit — is a bound parameter, so no untrusted input can reach the SQL text. Suppress at the two execute sites rather than restructuring: the shared _ELIGIBILITY_SQL fragment is what keeps the enumeration and recheck queries in sync, and the finding stays visible on the line it fires on.
… no cap SQLite treats a negative LIMIT as unbounded, so `--limit -1` processed every matching row rather than one — the inverse of the cap the caller asked for, on the path that mutates under --apply. Measured on a five-row fixture: matched=5 clamped=5, every alpha driven to the target. The guard rejects `< 0` rather than falsy values, because `limit=0` means what it says (LIMIT 0 selects nothing) and stays legal; a second test pins that boundary. `_cmd_clamp_ghosts` already maps ValueError to exit 2, so the CLI surfaces it without further wiring. Pre-existing: the same inversion is reachable on main, where the LIMIT clause was appended conditionally. Reported by review on this branch.
"It bounds alpha by source" opened a paragraph whose predecessor ended on a test name, so the pronoun had no referent; the subject is get_source_adjusted_prior. "migrate() can." answered a claim two paragraphs above it about the population not regrowing, with the headroom paragraph in between. Both now carry their own subject.
1c6e12c to
1ab5506
Compare
robotrocketscience
added
the
ready-to-merge
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged 1ab5506 → |
…k-run The wait loop enumerated every check-run on the head SHA and unlabelled on any failure, so Sourcery, CodeRabbit and every other advisory bot were de facto merge-blocking — while the message said 'required check(s) failed', naming a set the workflow never read. PR #1394 sat blocked behind a verified-false SQL-injection finding. The required contexts are now resolved at run time from rules/branches/main, which needs only read access, so the workflow cannot drift from the ruleset. pending is scoped to the same set, so a slow or silent advisory bot no longer holds the train to CHECK_TIMEOUT_SECONDS. A failing advisory check is still reported, labelled as not gating. Fail-closed: resolving zero required contexts aborts rather than merging. An empty set is indistinguishable from a moved ruleset or a token that lost read access, and reading it as 'nothing is required' would be worse than the over-blocking this replaces. The decision moved out of inline jq into scripts/merge_train_gate.py because a gate that cannot be tested is how this survived. #632's per-name latest-run dedup and its cancelled-is-not-failure rule are carried over and pinned, both directions. Closes #1397.
1 participant
Closes #1374. Parent #1158 §1 and §12.
The two still-live #1158 defects that are small and carry no default-ranking risk, so neither is blocked by the standing gold-set hold. Two commits, one per defect.
§1 — the HRR structural lane re-introduced the #1014 lock-starvation bug
The lane charged locks against the full budget with no relevance floor:
Neither
RELEVANCE_BUDGET_FLOOR_FRACTIONnorlock_injection_tokens— the two mechanisms that fixed exactly this on the main path — was applied. In the lock-saturated regime a structural query returned locks only, which is #1014 verbatim.Both mechanisms already existed and are reused rather than re-derived. The lane now reserves the relevance floor the same way the main path does.
No default textual ranking change. This path serves structural marker queries only; it is a no-op fall-through on non-marker queries.
§12 —
clamp_ghostswould clamp legitimately-ingested user beliefsThe selector was
lock_level='none' AND alpha > ? AND NOT EXISTS(feedback_history) AND NOT EXISTS(belief_corroborations)— no origin predicate, nocreated_atcutoff. A freshly-ingested user belief matches all four by construction: the insert path writesTYPE_PRIORSα straight onto the row (α=9.0 for user-sourced types), and a new belief has neither feedback nor corroboration yet.So the tool could not distinguish a fabricated ghost from a legitimate belief that was merely new — while its stated invariant, "every α-mutation path leaves an audit trail", ignores the insert path that writes α=9.0 with no trail. The selector now excludes user-prior origins.
No ranking risk. This is a manually-invoked write tool, not a retrieval path.
Verification
The test for §12 includes a belief that would have been clamped before the fix — a green run against a store with nothing clampable proves nothing, and that is the failure mode the issue called out explicitly. Mutation-checked in both directions.
Full suite green.
Scope
The other #1158 sections are untouched. §2, §4, §6, §9, §10, §11 and §15 either change default retrieval ranking or depend on storing the birth prior, and are parked under the gold-set hold with their triggers named on the parent.
Summary by Sourcery
Fix ghost-belief clamping and HRR structural retrieval lane behavior to avoid misclassifying fresh user beliefs as ghosts and to preserve a relevance floor under lock saturation.
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
--created-beforecutoff for ghost processing, allowing only records created before a specified timestamp to be processed.Bug Fixes