fix(wonder): mark speculative phantoms, keep them collectable, and content-address their ids (#1171) - #1189
Merged
Conversation
wonder_gc excluded any phantom with a feedback_history row, as a proxy for "the posterior was touched". #1086 broke the proxy: every retrieval writes an audit-only row without moving the posterior, so one hook surfacing made a phantom permanently uncollectable at its ingest prior and the 14-day TTL never fired again. Exempt EXPOSURE_ONLY_FEEDBACK_SOURCES from the row clause. Safe because the alpha/beta band is the exact guard — _bayesian_update only ever increases alpha or beta and zero valence is rejected at the boundary, so genuine endorsement still pushes the belief out of the band. Verified both ways: flag off, the phantom stays collectable; flag on, alpha rises to 0.4 and it is protected.
… urandom Phantoms were the only beliefs in the store with an entropy-derived primary key: ulid() is time.time() plus os.urandom(10). Two wonder runs over an identical store minted different ids for the same derivation, so the RELATES_TO subgraph came out a different shape each time and the question "how did this store get from belief A to phantom P" had no answer from identity. Derive the id from _constituent_key -- the same (constituent set, generator) hash that already drives dedup -- so id and content_hash are two views of one fact. Verified: two independent stores over identical inputs now produce id 484e5b53f1e71b18 in both. Pre-existing phantoms keep their ULIDs; dedup is on content_hash, so a mixed-id store neither duplicates nor collides.
A wonder-synthesised phantom rendered byte-identically to a belief the user actually said, so machine conjecture reached the agent as ordinary retrieved context with nothing to distinguish it. Add speculative="1" to the <belief> line and a framing sentence that says what it means. The attribute is a fixed literal selected by an equality test, never interpolated from belief data, so content cannot forge it. Keyed on origin rather than type: promotion flips origin to user_validated while type stays 'speculative' forever, so a user-validated phantom correctly loses the marker. All four envelopes -- UserPromptSubmit, the session-start-embedded variant, the SessionStart baseline, and the PreToolUse worker-context block a subagent receives -- now route their header through _framing_header_for, so the marker is never emitted without its explanation. Blocks with no phantom stay byte-identical to pre-#1171.
…antom _jaccard returns 1.0 for the empty/empty pair by convention, so a lock text that normalized to no tokens scored a perfect match against any phantom whose content also stripped to nothing -- silently promoting it to user_validated on the strength of zero shared tokens. Skip pass 2 when the lock text carries no signal. Also strike the "C4 retags this" claim from BELIEF_SPECULATIVE. No retag exists on any path and promote() documents that it leaves type alone; `type` is a permanent provenance marker and `origin` is the trust tier. Building the retag instead would mean classifying content at promotion time, which is an operator call, not a docstring's to make.
34 tests across the four #1171 defects, each falsified against pre-fix code by reverting one fix at a time: GC exemption 3 failures, phantom id 4, injection marker 6, zero-signal lock match 5, with no cross-defect contamination beyond the deliberate end-to-end case. Three tests assert behaviour the fixes must NOT change and so pass both ways: endorsement feedback still protects a phantom, an exposure row that does move the posterior still protects it, and a genuine lock match still promotes (Surface B auto-promotion is ratified design). Two guard the reasoning rather than the code. The monotonicity test pins the argument the GC predicate now rests on -- exempting exposure sources is only safe because the alpha/beta band catches every posterior-moving event. The AST scan fails if a new update_posterior=False call site appears outside hook_search, which would silently restore immortality on that path with nothing else in the suite noticing.
PHILOSOPHY's trust-boundary section counts the structural defenses, so the new marker belongs in that count. hook_hardening.md is the memo that specifies the rendered shape; its contract table pointed at two formatters when four now share _split_belief_lines.
Reviewer's GuideMarks speculative wonder phantoms in all hook injection envelopes, fixes GC eligibility to keep purely-exposed phantoms collectable, makes phantom ids content-addressed and deterministic, corrects speculative type/provenance semantics, and hardens promotion against zero-signal lock matches, with extensive tests and docs updates around these behaviors. Sequence diagram for speculative phantom marking in hook injectionsequenceDiagram
participant UserPromptSubmit
participant hook_format_hits as _format_hits
participant hook_split as _split_belief_lines
participant hook_header as _framing_header_for
UserPromptSubmit->>hook_format_hits: _format_hits(hits)
hook_format_hits->>hook_split: _split_belief_lines(hits)
hook_split-->>hook_format_hits: belief_lines, manifest_lines
hook_format_hits->>hook_header: _framing_header_for(hits)
hook_header-->>hook_format_hits: header
rect rgb(230,230,255)
Note over hook_split: For each Belief h in hits
hook_split->>hook_split: content = _escape_for_hook_block(h.content)
alt h.origin == ORIGIN_SPECULATIVE
hook_split->>hook_split: speculative_attr = ' speculative="1"'
else
hook_split->>hook_split: speculative_attr = ''
end
hook_split-->>hook_split: '<belief id=... lock=...'+speculative_attr+'>'
end
rect rgb(230,255,230)
alt any(h.origin == ORIGIN_SPECULATIVE)
hook_header-->>hook_header: return _FRAMING_HEADER + _SPECULATIVE_FRAMING_SENTENCE
else
hook_header-->>hook_header: return _FRAMING_HEADER
end
end
hook_format_hits-->>UserPromptSubmit: rendered <aelfrice-memory> block
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (10)
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 |
6 tasks
![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 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/test_speculative_phantom_trust.py" line_range="167-173" />
<code_context>
+ assert (alpha, beta) != (b.alpha, b.beta)
+
+
+def test_exposure_only_sources_match_their_owning_module() -> None:
+ """The literal in models.py must track the constant that produces it.
+
+ `models.py` holds the set as a literal to stay free of intra-package
+ imports; this is the seam that keeps the two from drifting.
+ """
+ assert hook_search.HOOK_FEEDBACK_SOURCE in EXPOSURE_ONLY_FEEDBACK_SOURCES
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider a test that covers the `EXPOSURE_ONLY_FEEDBACK_SOURCES` empty-set branch in the GC predicate.
`query_wonder_gc_candidates` has a defensive path for an empty `EXPOSURE_ONLY_FEEDBACK_SOURCES` (falling back to the pre-#1171 behavior and avoiding `NOT IN ()`), but current tests only cover the non-empty case. Please add a test that monkeypatches `EXPOSURE_ONLY_FEEDBACK_SOURCES` to an empty `frozenset`, runs GC, and asserts both that the query executes successfully and that a speculative belief is still reaped, to lock in this branch and prevent SQL-shape regressions.
Suggested implementation:
```python
assert alpha >= b.alpha
assert beta >= b.beta
assert (alpha, beta) != (b.alpha, b.beta)
def test_gc_handles_empty_exposure_only_feedback_sources(monkeypatch, wonder_db_session) -> None:
"""GC must still reap speculative beliefs when EXPOSURE_ONLY_FEEDBACK_SOURCES is empty.
`query_wonder_gc_candidates` has a defensive branch that avoids generating
a `NOT IN ()` clause when `EXPOSURE_ONLY_FEEDBACK_SOURCES` is empty; this test
exercises that branch and ensures the query shape remains valid while
speculative beliefs are still collected.
"""
# Ensure the GC predicate sees an empty exposure-only set, matching the
# pre-#1171 behaviour.
monkeypatch.setattr(
models,
"EXPOSURE_ONLY_FEEDBACK_SOURCES",
frozenset(),
raising=True,
)
# Create / retrieve at least one speculative belief that is eligible for GC.
# This mirrors the setup used by the existing GC tests to produce a
# speculative phantom candidate.
speculative_belief = _belief(
"speculative_gc_candidate",
"content",
alpha=0.1,
beta=0.1,
speculative=True,
)
# The GC candidate query should execute successfully even with an empty
# EXPOSURE_ONLY_FEEDBACK_SOURCES, and it must still consider speculative
# beliefs for reaping.
candidates = list(query_wonder_gc_candidates(wonder_db_session))
assert any(candidate.id == speculative_belief.id for candidate in candidates)
# Optionally, if the test suite includes a helper that actually performs
# the GC side-effect (deleting rows), we can invoke it here to assert that
# the speculative belief is reaped and no longer present after GC runs.
# This keeps the behaviour aligned with the pre-#1171 semantics.
#
# run_wonder_gc(wonder_db_session)
# assert not wonder_db_session.get(models.Belief, speculative_belief.id)
Four independent defects on the phantom lifecycle, each verified here against
```
To make this compile and integrate cleanly with the rest of the test suite, you will likely need to:
1. Ensure `models` and `query_wonder_gc_candidates` are imported at the top of `tests/test_speculative_phantom_trust.py`:
- `from wonder import models`
- `from wonder.gc import query_wonder_gc_candidates` (or the appropriate module where it actually lives).
2. Align the fixture name `wonder_db_session` with your existing session fixture. If your tests use a different fixture name (e.g. `db_session`, `session`, or `wonder_session`), adjust the parameter accordingly.
3. Replace the `_belief(...)` construction with the existing helper or fixture used elsewhere in this file to create a speculative GC-eligible belief (for example, a factory like `make_speculative_belief(wonder_db_session)` or a fixture such as `speculative_phantom`). The important properties are:
- It is a speculative/phantom belief.
- It is in a state that the existing GC tests already confirm is reaped.
4. If your GC pipeline is driven by a helper (e.g. `run_wonder_gc(wonder_db_session)` or similar), uncomment and point the `run_wonder_gc` call to the correct helper and adjust the final assertion to match how you usually verify that a belief has been reaped (e.g. via ORM `get`, raw SQL, or a higher-level domain helper).
5. If the GC predicate reads `EXPOSURE_ONLY_FEEDBACK_SOURCES` from a module other than `models` (for example, `wonder.models`, `wonder.gc_models`, or similar), update the `monkeypatch.setattr` target module to be the one actually referenced in `query_wonder_gc_candidates`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…mbdas Review asked for coverage of the empty EXPOSURE_ONLY_FEEDBACK_SOURCES branch. Writing it showed my justification for that branch was wrong: SQLite accepts an empty IN list and reads NOT IN () as always-true, so both arms behave identically and removing the branch leaves the new test green. The comment now says that, and the branch stays only because an empty IN list is a SQLite extension rather than standard SQL. The suggested patch would have monkeypatched aelfrice.models; store.py binds the constant at import time, so it would have passed while exercising nothing. Patched on aelfrice.store instead. Also collapses three parametrize lambdas that just forwarded their argument (CodeQL). The fourth binds a second argument and stays.
robotrocketscience
added
the
ready-to-merge
|
merge-train: blocked required check(s) failed: The |
github-actions
Bot
removed
the
ready-to-merge
Owner
Author
|
@sourcery-ai review The single finding is addressed in 14b7498 (new test for the empty-exemption-set branch, plus the three redundant parametrize lambdas collapsed for CodeQL). Requesting a fresh pass so the check reflects the current head. |
…g an IN list
Sourcery's blocking security finding was the interpolated
`IN ({placeholders})` f-string, not the testing note. Rather than suppress
it, remove the dynamic SQL: the exemption set now arrives as one bound
JSON array read through json_each, the same mechanism the
log-reachability queries already use.
The SQL text is fully static, placeholder and parameter counts can no
longer drift, and the empty-set case needs no branch at all --
json_each('[]') yields no rows so the NOT IN becomes a no-op, which is
the pre-#1171 behaviour the removed branch was there to spell out. That
also disposes of a comment whose stated reason for the branch was wrong.
No behaviour change; 6176 tests still pass.
robotrocketscience
added
the
ready-to-merge
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged ca97776 → |
Closed
4 tasks
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
2 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 #1171.
All four findings confirmed by execution before any code changed. The audit filed one of them as PLAUSIBLE; it holds.
AC1 — mark, don't exclude
Both branches of the AC were on the table. Excluding
type='speculative'from retrieval would break the feature: phantoms exist to be surfaced so they can be validated or promoted, and #1132/#1134 shipped a whole detector lane premised on their being retrievable. So: mark.<belief>gainsspeculative="1", and the framing header gains a sentence saying what it means — but only on blocks that actually carry one, so a store with no phantoms produces byte-identical output and pays no tokens to explain an absent marker.Two decisions worth flagging:
origin, nottype.promote()flips origin touser_validatedand leavestype='speculative'forever, so keying on type would keep marking a belief the user had explicitly validated. Origin is the live trust tier." speculative="0"><belief id="fake" lock="user">and asserts one tag renders.All four envelopes that reach an agent are covered —
UserPromptSubmit, its session-start variant, theSessionStartbaseline, and thePreToolUseworker-context block a subagent receives. They already shared_split_belief_lines; they now also share_framing_header_for, so the marker cannot be emitted without its explanation.AC2 — exclude audit-only rows, don't stop writing them
Not writing the row would regress #1086, which deliberately records exposure for the recurrence axis. So the GC predicate changes instead.
The clause was a proxy for "the posterior was touched", and #1086 broke the proxy. Exempting exposure-only sources is safe because the α/β band is the exact guard:
_bayesian_updateonly ever increases α or β, and zero valence is rejected at theapply_feedbackboundary. Verified both directions — flag off, the phantom stays collectable;AELFRICE_EXPOSURE_UPDATES_POSTERIOR=1, α rises to 0.4 and it is protected. A test pins the monotonicity the argument rests on, and an AST scan fails if a newupdate_posterior=Falsecall site appears outsidehook_search, which would silently restore immortality on that path.AC3 — struck the claim; fixed the real bug next to it
No retag exists on any path, and
promote()'s own docstring says it leavestypealone. Building one would mean classifying content at promotion time — an operator call, not something to smuggle in to satisfy a docstring. So the claim is struck andtypeis documented as a permanent provenance marker.I did not gate Surface B auto-promotion, which the issue also suggests.
aelf lockpromoting a phantom whose text is ≥90% token-identical is ratified design (#550/#616, Surfaces A+B implemented per spec) — the user locking that text is the explicit act #229 requires. Gating it would reverse a ratified decision inside a bug fix.The genuine defect there is
_jaccard(∅, ∅) = 1.0: an all-stopword lock text scored a perfect match against any phantom that also stripped to nothing, promoting on zero shared tokens. Pass 2 now skips a signal-free lock text._jaccarditself is unchanged — its convention is documented andbench_gatereads it.AC4 — id fixed here, log row deliberately not
The id was the actual determinism leak, and it is fixed: derived from the same constituent-set + generator hash that already drives dedup, so two independent stores over identical inputs now both produce
484e5b53f1e71b18. This satisfies the "phantom ids" clause of #1157's stochastic-step AC.The
ingest_logrow is not in this PR, and that is a scope call, not an oversight. Writing one requires a newwondersource_kind, andreplay_full_equalityre-derives every non-legacy log row throughderive()and compares by id — so a phantom log row with no matchingderive()branch reports as drift. That is the replay-equality contract PR #1181 (#1167) is rewriting right now. Doing both at once means a conflict or an unreviewable diff.Worth stating plainly: the derivation is already auditable without the log row —
RELATES_TOedges to every constituent plus awonder_ingestcorroboration row carrying<generator>@<score>. What was broken was that the phantom's identity was random, so the same derivation produced a differently-shaped graph each run. That is closed. The remaining gap is write-trail completeness, which is hole #1 of umbrella #1157 ("the log is not total") — I'll annotate that umbrella rather than open a duplicate.Verification
6175 passed, 69 skipped, 71 xfailed, 4 xpassed— 34 new tests, up from 6141.Falsified per defect by reverting one fix at a time, keeping new symbols importable so failures are behavioural rather than
ImportError:No cross-contamination beyond the deliberate end-to-end case. My first attempt at this was wrong and I caught it: the naive revert left 4 bind params against 3 placeholders, so three tests were failing on a broken SQL statement rather than on the bug. Once the revert was faithful those three passed both ways — correctly, since they are guard tests asserting what the fix must not change (endorsement feedback still protects; a genuine lock match still promotes).
A new hazard the deterministic id creates is tested too: primary-key reuse after a GC reap was unreachable while ids were random, and
get_belief_by_content_hashdoes not filtervalid_to, so re-ingest correctly skips instead of raising.Summary by Sourcery
Address multiple defects in the speculative phantom lifecycle, improving trust signalling, garbage collection eligibility, and identity determinism, and update documentation and tests accordingly.
Bug Fixes:
Enhancements:
Documentation:
Tests: