feat(detectors): pin and version the thresholds behind the 2.8% non-spine edges - #1362
Conversation
Reviewer's GuideAdds a new detector thresholds manifest module that pins and versions constants affecting non-spine edges, plus tests that verify pinned values, enforce version/digest coupling, and ensure all non-spine edge writers are either covered or explicitly excluded, with a brief changelog note documenting the behavior. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 52 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 (3)
📝 WalkthroughWalkthroughThe PR adds a versioned detector-threshold manifest, canonical digest tracking, writer coverage metadata, and tests that detect threshold drift, invalid manifest changes, and unclassified edge writers. ChangesDetector threshold reproducibility
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 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 |
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 issue, and left some high level feedback:
- In
test_manifest_digest_is_pinned, consider including the currentmanifest_digest()value in the assertion failure message (or a small helper/CLI to print it) so that updatingMANIFEST_DIGESTis less error-prone and doesn’t require recomputing the digest out-of-band. - In
test_scalar_entries_pin_a_literal_not_a_digest, the kind strings ("numeric_cutoff","cap", etc.) are duplicated instead of using theKIND_*constants; reusing the existing constants would reduce the chance of typos or drift between the manifest and its validation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_manifest_digest_is_pinned`, consider including the current `manifest_digest()` value in the assertion failure message (or a small helper/CLI to print it) so that updating `MANIFEST_DIGEST` is less error-prone and doesn’t require recomputing the digest out-of-band.
- In `test_scalar_entries_pin_a_literal_not_a_digest`, the kind strings (`"numeric_cutoff"`, `"cap"`, etc.) are duplicated instead of using the `KIND_*` constants; reusing the existing constants would reduce the chance of typos or drift between the manifest and its validation.
## Individual Comments
### Comment 1
<location path="src/aelfrice/detector_thresholds.py" line_range="152-162" />
<code_context>
+ return json.dumps(obj, sort_keys=True, separators=(",", ":"))
+
+
+def pin_value(obj: Any) -> str:
+ """Return the pinned form of a live constant.
+
+ Scalars pin as their literal, so the manifest stays readable and a
+ reviewer can check ``0.4`` against the source line by eye. Collections
+ pin as a digest of their canonical form — a stopword set is not
+ reviewable inline, but its digest changes the moment a token is added
+ or removed, which is the property the manifest needs.
+ """
+ if isinstance(obj, bool) or not isinstance(obj, (int, float, str)):
+ return "sha256:" + hashlib.sha256(
+ _dumps(_canonical(obj)).encode("utf-8")
</code_context>
<issue_to_address>
**suggestion:** Clarify or align the special handling of booleans in `pin_value` with the docstring.
The current implementation treats booleans as non-scalars and hashes them, which conflicts with the docstring’s claim that scalars pin as literals. If this is intentional (e.g. to distinguish True/False from 1/0), please either update the docstring to document the special-case behaviour for booleans, or adjust the condition so booleans are treated like other numeric scalars. Otherwise, the behaviour is surprising for anyone relying on the docstring.
```suggestion
Scalars (including booleans) pin as their literal, so the manifest
stays readable and a reviewer can check ``0.4`` or ``True`` against
the source line by eye. Collections pin as a digest of their canonical
form — a stopword set is not reviewable inline, but its digest changes
the moment a token is added or removed, which is the property the
manifest needs.
"""
if not isinstance(obj, (int, float, str)):
return "sha256:" + hashlib.sha256(
_dumps(_canonical(obj)).encode("utf-8")
).hexdigest()
return _dumps(obj)
```
</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}
|
[claim:review:Garsecg:2026-08-05T23:42:02Z] |
|
[claim:review:Idnn:2026-08-05T23:42:59Z] |
|
[release:review:Idnn:2026-08-05T23:43:04Z] |
|
[claim:review:Idnn:2026-08-05T23:58:50Z] |
|
[release:review:Idnn:2026-08-05T23:58:55Z] |
1a13c31 to
0c86989
Compare
Review — the mechanism is sound, five of the 22
|
| commit | what |
|---|---|
aae3c41c |
pin_value and size_of disagreed on whether a bool is a scalar. size_of(True) → None (scalar); pin_value(True) → a sha256: digest. A pinned boolean would carry size=None and a digest — the exact pair test_scalar_entries_pin_a_literal_not_a_digest rejects — so no boolean could be added without a spurious failure. The exclusion bought nothing: json.dumps already renders True as true and 1 as 1. Test added, mutation-checked. This is the Sourcery thread, but the consequence is the inconsistency rather than the docstring. |
48f4ca1f |
CodeQL is right about Sequence — referenced only inside a string-literal cast, so never evaluated. Moved under TYPE_CHECKING; pyright still resolves the cast, 0 errors. |
3fae2a6e |
Two evasions in the coverage sweep. _RAW_SQL_RE matched only bare/OR-qualified INSERT INTO edges, so REPLACE INTO edges walked past both arms — and REPLACE is what a new writer reaches for first, since insert_edge is a bare INSERT against a PRIMARY KEY (src,dst,type) and raises on re-write. INSERT INTO "edges", [edges], main.edges also evaded, and the qualified form is house style (INSERT INTO temp.fts, store.py:3752). Widened; still exactly ["aelfrice.store"] on the live tree, and a trailing \b drops a false positive the old pattern had on edges_backup. Also: the digest covers version + THRESHOLDS but not the coverage lists, so a module can move covered → excluded with no digest and no version movement, silently voiding test_covered_modules_all_have_entries for it while its entries stay in THRESHOLDS. Asserted. Both mutation-verified: green before, red after. |
8f77cf97 |
The "Two limits" paragraph sat unindented between two list items, which terminates the list — it rendered under ### Added rather than under its entry, and restarted the list below. Only such paragraph across all five changelog files. |
0c86989b |
22 constants across 9 modules, not 8. |
Full suite green on the rebase; pyright 0 errors; discretion grep on added lines clean.
Not for this PR — aelf spine clear silently reverses prose-derived edges
Falling out of finding 2's last item, and filed separately since it is a live data path, not a manifest question:
ingest "deployment phase two succeeds deployment phase one"
TEMPORAL_NEXT: d74b6a6b -> 0f0f6740 (successor -> predecessor, per triple_extractor.py:148)
clear_temporal_spine() removed 1
backfill_temporal_spine() wrote 1
TEMPORAL_NEXT: 0f0f6740 -> d74b6a6b reversed
clear_temporal_spine deletes by type (temporal_spine.py:343), so it takes the triple-extractor rows with it; the backfill then rebuilds a session-chronological chain pointing the other way. One edge before, one after — no count-based check sees it — and clear_temporal_spine's docstring claims "a later backfill rebuilds them byte-identically (the G5 determinism property)", which does not hold for these rows. Reachable from aelf spine clear (cli.py:6012). May also account for part of #1356's shipped - recomputed bucket.
robotrocketscience
added
attn:unblock
|
[release:review:Garsecg:2026-08-06T00:10:51Z] |
All four accepted, three commits pushed. You were right about the headline claim.Reproduced every finding against live source before acting on it. Nothing here is disputed. 1. The digest did not force a version bump — fixedYou're right, and it's the worst kind of wrong: the module was named after the property it didn't have, and arm 2's own failure message instructed the repair that defeats it. A lone My mutation table row Took the first option, since #1355's criterion is "forces": DIGEST_HISTORY: Final[dict[int, str]] = {1: "ffaaca91…"}
MANIFEST_DIGEST: Final[str] = DIGEST_HISTORY.get(DETECTOR_THRESHOLDS_VERSION, "")
Your repro, re-run with
All four sites that stated it falsely are rewritten, including The 2. Five
|
d7ad9d3 to
cadfd5d
Compare
robotrocketscience
added
attn:review
|
Unblocked. The only outstanding thread was CodeQL alert 557 (unused Fixed at Rebased onto main (12 commits, all signed). CI green, 0 unresolved threads. Returning to |
robotrocketscience
added
attn:unblock
robotrocketscience
added
attn:review
Review — two fixes pushed, two body numbers correctedReviewed at Pushed to your branch —
|
db212b0 to
0b6af03
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 (1)
tests/test_detector_thresholds_manifest_1355.py (1)
138-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSelect scalars by the imported kind constants, and assert the selector is not empty.
Line 139 hardcodes the kind strings.
KIND_CUTOFF,KIND_CAP,KIND_WEIGHTandKIND_LITERALare already exported by the module under test. If a kind value is renamed, this set matches no entry and the loop body never runs, so the test passes while asserting nothing.test_entries_are_wellformed_and_uniquestays green in that case because it checks membership in the importedKINDS. That is the same "test that cannot fail" shape this file's docstring targets.Import the constants and add a floor so the selector cannot go empty.
♻️ Proposed change
from aelfrice.detector_thresholds import ( COVERED_WRITER_MODULES, DETECTOR_THRESHOLDS_VERSION, EXCLUDED_WRITERS, + KIND_CAP, + KIND_CUTOFF, + KIND_LITERAL, + KIND_WEIGHT, KINDS,+ _SCALAR_KINDS = {KIND_CUTOFF, KIND_CAP, KIND_WEIGHT, KIND_LITERAL} + checked = 0 for entry in THRESHOLDS: - if entry.size is None and entry.kind in {"numeric_cutoff", "cap", "weight", "literal"}: + if entry.size is None and entry.kind in _SCALAR_KINDS: + checked += 1 assert not entry.value.startswith("sha256:"), ( f"{entry.module}.{entry.name} is a scalar and must pin its " f"literal, not a digest" ) + assert checked, "no scalar entry was examined — the selector matched nothing"Note:
KIND_CAP,KIND_CUTOFF,KIND_LITERALandKIND_WEIGHTare not in the module's__all__. Add them there, or keep the strings and add the floor assertion alone.🤖 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_detector_thresholds_manifest_1355.py` around lines 138 - 143, Update the scalar selector in the threshold test to use the exported KIND_CUTOFF, KIND_CAP, KIND_WEIGHT, and KIND_LITERAL constants instead of hardcoded strings; add these constants to the module’s __all__ if required for import, and assert that at least one THRESHOLDS entry matches the selector so the test cannot pass vacuously.
🤖 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/detector_thresholds.py`:
- Around line 742-754: Reorder the entries in the module-level __all__ list into
isort order, moving "DIGEST_HISTORY" before "EXCLUDED_WRITERS" and preserving
all existing exports unchanged.
In `@tests/test_detector_thresholds_manifest_1355.py`:
- Around line 310-315: Correct the comment near the manifest consistency
assertion to state that manifest_digest() covers only THRESHOLDS, not the
version, while preserving the existing explanation about coverage-list moves and
the test_covered_modules_all_have_entries assertion.
---
Nitpick comments:
In `@tests/test_detector_thresholds_manifest_1355.py`:
- Around line 138-143: Update the scalar selector in the threshold test to use
the exported KIND_CUTOFF, KIND_CAP, KIND_WEIGHT, and KIND_LITERAL constants
instead of hardcoded strings; add these constants to the module’s __all__ if
required for import, and assert that at least one THRESHOLDS entry matches the
selector so the test cannot pass vacuously.
🪄 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: 58f38134-8890-4139-ae3c-9eb453df2460
📒 Files selected for processing (3)
CHANGELOG/v4.mdsrc/aelfrice/detector_thresholds.pytests/test_detector_thresholds_manifest_1355.py
0b6af03 to
db212b0
Compare
…edges #1283 restated AC2 in two halves. The recompute half shipped in #1336; this is the other one. Edges that are neither TEMPORAL_NEXT nor DERIVED_FROM are a function of the belief set AND of detector thresholds, so "edges are recomputable" holds for them only if those thresholds are pinned and versioned. They were bare module constants with no guard. detector_thresholds records 22 constants across 8 modules behind DETECTOR_THRESHOLDS_VERSION: the relationship_detector cutoffs, caps and vocabularies, the contradiction precedence ladder, the triple_extractor phrase-to-edge-type patterns, the value_compare slot gate, and the constants on the two paths that decide which phantoms reach the RELATES_TO writer. The module holds hand-written literals and imports nothing from aelfrice. That is deliberate: importing the constants it describes would make it tautological in exactly the way the tests it replaces were, and would drag store/bm25 into the import graph of any reader. Scalars pin as literals so a reviewer can check them by eye; collections pin as a digest of a canonical form that includes regex flags, since dropping re.IGNORECASE changes which triples match without changing any pattern text. Entries were checked for reachability rather than assumed. The wonder bake-off constants are NOT pinned -- the package docstring states the strategies are research-only and their sole importer builds against an in-memory store, so they decide no edge a user holds. The value_compare entries are pinned but labelled dormant: nothing shipped passes use_value_comparison=True. Three upstream suppliers the call-site sweep cannot see (bm25._TOKEN_PATTERN, models.ANCHOR_TEXT_MAX_LEN, wonder_consolidation._TOKENIZER_DROP) are pinned by hand, and the two suppliers left unpinned are named so their absence is a decision. Forward-only. The edges table has no version and no created_at, so this does not make a historical edge attributable to the thresholds that produced it; adding those columns is the migration that bricked stores in #1161, and historical reproduction stays out of scope.
…or unpinned writers Three independent failure modes, three arms that fail independently. Arm 1 re-derives each pinned value from the live constant by importing it, so the comparison is manifest-literal against source rather than symbol against itself. This is the defect the issue was filed on: test_config_loader_overrides_and_falls_back asserts cfg.jaccard_min == DEFAULT_JACCARD_MIN, and changing the constant from 0.4 to 0.9 leaves it green -- measured, not asserted. Arm 2 pins a digest taken over the manifest INCLUDING its version, which is what makes "changing a value forces a version bump" mechanical rather than conventional: a value edit fails twice, and the only route back to green moves MANIFEST_DIGEST next to the version. Arm 3 sweeps insert_edge call sites out of source text rather than comparing against a hand-list, so a writer landing later fails here instead of sitting silently outside the manifest. A companion arm asserts only the store writes the edges table directly, so the sweep cannot be evaded with raw SQL. Covered-ness matches on the exact module, not a package prefix -- prefix matching is what let wonder.lifecycle look covered by wonder.evaluator, a research-only module that writes to no live store. Verified by mutation, eight cases: five source-side (a cutoff, a token added to a negation set -- red twice, since the stopword set is a union of it -- dropping re.IGNORECASE from a triple pattern, widening the bm25 token pattern, and shortening the anchor cap) and three manifest-side (a version bump alone, relaxing a pinned value to match a drift, and a comment-only edit that correctly stays green because the digest covers data rather than source text).
`pin_value` excluded bool from its scalar branch and digested it, while `size_of` reports None (scalar) for a bool. A pinned boolean constant would therefore have carried `size=None` and a `sha256:` value at once — exactly the combination `test_scalar_entries_pin_a_literal_not_a_digest` rejects — so no boolean could be added to the manifest without a spurious failure. The exclusion bought nothing: json renders True as `true` and 1 as `1`, so the two were never ambiguous. Asserted directly, since no boolean is pinned today.
`Sequence` is referenced only inside a string-literal `cast`, so the name is never evaluated at runtime and the unconditional import was dead — CodeQL flagged it. Moving it under TYPE_CHECKING keeps pyright resolving the cast while removing the runtime import.
The paragraph sat unindented between two list items, which terminates the list in Markdown: it rendered as a standalone paragraph belonging to `### Added` rather than to its entry, and restarted the list below it. It is the only such paragraph in the five changelog files. Two spaces makes it a continuation of the bullet it belongs to; no text changed.
The PR body claims the sweep "cannot be evaded with raw SQL" and that keeping the raw-SQL arm "means arm 3 cannot be quietly defeated". Neither held. `_RAW_SQL_RE` matched only a bare or `OR`-qualified `INSERT INTO edges`, so `REPLACE INTO edges` evaded it — and REPLACE is the spelling a new writer is most likely to reach for, since `insert_edge` issues a bare INSERT against a `PRIMARY KEY (src, dst, type)` table and raises on re-write. `INSERT INTO "edges"`, `[edges]` and `main.edges` evaded it too, and the qualified form is already house style (`INSERT INTO temp.fts` in store.py). Such a module is invisible to both arms at once: no `.insert_edge(` for the call-site sweep, no match here. Verified by adding a `REPLACE INTO edges` module — green before, red after — and the widened pattern still resolves to exactly ["aelfrice.store"] on the live tree. A trailing `\b` also drops a false positive the old pattern had on `edges_backup`. Separately, `manifest_digest()` covers the version and THRESHOLDS but not the two coverage lists, so moving a module from covered to excluded moves no digest and no version. That silently voids `test_covered_modules_all_have_entries` for the module while its entries stay in THRESHOLDS claiming to gate edges the exclusion says it does not decide. Asserted directly; mutation-verified.
`{t.module for t in THRESHOLDS}` is 9: bm25, contradiction, models,
relationship_detector, triple_extractor, value_compare, wonder.dispatch,
wonder.lifecycle, wonder_consolidation. The count is prose on both sides and
nothing gates it; MANIFEST_DIGEST already goes red on any entry change, so
this is a correction rather than a new assertion.
…cannot be absorbed Review found the module's headline claim false, and it was: a lone MANIFEST_DIGEST literal sitting beside the thing it digests forces nothing. Edit a constant, edit its manifest entry, edit the digest, and the suite is green with DETECTOR_THRESHOLDS_VERSION untouched -- two different edge-producing behaviours shipping as version 1. That is verbatim failure mode 2 in the test file's own docstring, and the arm's failure message instructed exactly the repair that causes it. Replaces the literal with DIGEST_HISTORY, keyed by version, and digests content only (the version is now the key, so folding it into the payload would move the digest for reasons unrelated to what the manifest says). The cheap repair is gone: the ways back to green are to revert, or to bump and APPEND a row. A second arm pins the history contiguous over 1..VERSION with no duplicate digests, so a bump cannot skip a row. MANIFEST_DIGEST is derived via .get rather than a subscript -- bumping without appending is a contract breach the tests should name, and an import-time KeyError would take the module down and surface as a collection error in every unrelated test that imports it. Stated rather than overclaimed: overwriting a historical row still works. Only a merge-base check in CI is fully mechanical, and that is not bundled here. Verified by mutation with caches cleared between runs (same-length digests are otherwise served from a stale .pyc): the reported repro -- source 0.4 -> 0.9 plus the matching manifest edit -- previously passed 31/31 and now fails; bumping without appending fails two arms cleanly; bumping and appending is green.
…usion `gates` is the manifest's substance -- "a constant that cannot answer this does not belong here" -- and nothing tests these strings, so five wrong ones shipped. All five were reproduced against live source before being rewritten. - SUPERSEDES_WEIGHT claimed "propagation arithmetic". There is none: Edge.weight is read by a BFS sort key, a clustering floor and persistence. Propagation is EDGE_VALENCE, keyed on edge TYPE, which this constant does not touch. The real effect is sharper -- at 1.0 it clears DEFAULT_CLUSTER_EDGE_FLOOR (0.4), and any value below that silently drops every SUPERSEDES edge out of candidate clustering. - UNCERTAINTY_THRESHOLD claimed to filter the anchor tuple. It does not: `anchors` is built from the unfiltered `known_beliefs`. It selects high_uncertainty_beliefs, which decides whether an uncertainty_deep_dive axis is emitted -- still a real path to RELATES_TO, but not the stated one. - QUANT_AXIS said the score is half the axis distance. It is a quarter: q_term halves the distance and the score halves it again. `always` vs `sometimes` is 1.0 apart and scores 0.25, so it lands as POTENTIALLY_STALE rather than CONTRADICTS -- the wrong side of the very split DEFAULT_CONFIDENCE_MIN exists to record. Sizing an edit with the old prose picks the wrong value. - DEFAULT_JACCARD_MIN claimed lowering it "can only add edges". The candidate pool is monotonic; the written set is not, because DEFAULT_MAX_EDGES_PER_BELIEF is spent in sorted pair order, so a newly-admitted pair can evict a previously written one. - _PATTERNS omitted TEMPORAL_NEXT from edge_types while four of its 25 patterns mint exactly that type (`follows`, `comes after`, `is after`, `succeeds`). That last one has a consequence outside the manifest, so the exclusion entry now carries it: EXCLUDED_WRITERS said temporal_spine "writes TEMPORAL_NEXT only", which reads as the spine accounting for the whole TEMPORAL_NEXT population. It does not -- triple_extractor is a second, prose-driven producer the #1336 spine recompute does not cover. Also records two limits the manifest was silent on. `--axes-budget` (default 24) caps the anchor tuple and so how many RELATES_TO edges each persisted phantom writes -- a bigger lever than several pinned constants, but a signature default rather than a module constant, out of reach of the (module, name) scheme for the same reason the inline weights are. And belief ARRIVAL ORDER is a third input beside the belief set and these thresholds: the per-belief cap is spent on whichever pairs arrived first, so full-store and incremental runs can disagree on identical beliefs, and re-deriving edges from beliefs plus this manifest gives a false mismatch on any incrementally built store -- which is every real one.
The string-literal cast to Sequence kept a TYPE_CHECKING import alive that CodeQL reads as dead (alert 557). The branch is already narrowed to tuple|list by the isinstance guard above it, so casting to that union is both stricter and self-contained.
…e one assert MANIFEST_DIGEST == DIGEST_HISTORY[VERSION] compared DIGEST_HISTORY.get(VERSION, "") against DIGEST_HISTORY[VERSION] — true by definition of .get whenever the key exists, which the contiguity test already guarantees. It could never fail. The case the .get fallback exists for — a version bump with no appended row — reached the preceding subscript first and reported as a KeyError, the crash the fallback was added to avoid. Assert the constant resolved before subscripting anything, and compare against it thereafter. Mutation-checked both directions: bumping the version without a row now names the missing row; changing a pinned value without bumping still names the digest mismatch.
…filter (#1355) Two fixes in the manifest test. The comment above the covered/excluded overlap assert said `manifest_digest()` 'covers the version and THRESHOLDS'. It does not -- the payload is content-only by design and `DETECTOR_THRESHOLDS_VERSION` does not appear in it. The version bump is forced by `DIGEST_HISTORY` being keyed by version. The comment's actual conclusion still holds, so only its stated reason was wrong -- wrong in a way that reads as verified. The scalar filter selected on the raw strings {'numeric_cutoff', 'cap', 'weight', 'literal'} while KINDS was already imported and the KIND_* constants exist. Renaming any KIND_* value made the filter match fewer entries and the guard go quietly vacuous rather than red: with KIND_CUTOFF renamed, the raw-string filter selects 6 of the 11 scalar entries it should. Selecting through the constants keeps all 11.
db212b0 to
cff7be8
Compare
`DIGEST_HISTORY` sat after `KINDS`. No behaviour change -- `__all__` only governs star-imports and the module has no star-importer. Taken on convention, not on the reason the review gave: there is no ruff configuration and no lint job in this repo, so RUF022 is not enforced and no gate was failing. 19 of the 22 modules on main that declare a multi-entry `__all__` keep it sorted, which is reason enough on its own.
robotrocketscience
added
the
ready-to-merge
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged ec25ee6 → |
robotrocketscience
added
the
ready-to-merge
|
[release:review:Garsecg:2026-08-06T01:10:39Z] |
2 participants
Closes #1355. Parent: #1157, carved from #1283 on the operator ruling of 2026-08-05.
What this is
The funded half of #1283 AC2 that was never started: a frozen, versioned record of the constants that decide the non-spine edge set, so "edges are recomputable" is a checkable claim for the 2.8% rather than an assertion.
aelfrice.detector_thresholdspins 22 constants across 9 modules behindDETECTOR_THRESHOLDS_VERSION = 1.The defect being fixed could not fail
The issue calls the existing coverage tautological. It is, and this PR measures it rather than restating it:
assert cfg.jaccard_min == DEFAULT_JACCARD_MINcompares the constant to itself, so it survives any change to what the symbol resolves to. Same class as the #1353 synth-exclusion pin.Design — why the manifest imports nothing
The manifest holds hand-written literals and imports nothing from
aelfrice. If it imported the constants it describes, it would reproduce the exact defect it exists to close. The test does the importing and re-derives each pinned form from the live object; the asymmetry is the mechanism.0.4against the source line by eye. A separate arm rejects "repairing" a red scalar by converting it to a digest, which would otherwise satisfy arm 1 while destroying the reviewability the issue asked for.re.IGNORECASEfrom the triple patterns changes which triples match without changing one character of pattern text, and it goes red.DIGEST_HISTORY[version],cfe1054d). An earlier revision folded the version into the digested content, which let a value change be absorbed by editing the digest literal with the version untouched — two behaviours shipping as version 1. Keyed by version, the ways back to green are to revert or to bump-and-append.Coverage is swept, not hand-listed
The third checkbox is the easy one to under-deliver: pinning
relationship_detectoralone would look complete. Arm 3 extracts everyinsert_edgecall site from source text and requires each module to be either pinned or excluded with a stated reason — 4 covered, 6 excluded (the spine, the twoDERIVED_FROMpaths, the cross-store copier, two synthetic-fixture builders). A companion arm asserts only the store writes theedgestable directly, so the sweep cannot be evaded with raw SQL.Two writers surfaced this way that a
relationship_detector-shaped reading would have missed:contradiction.CLASS_NAMESis keyed on thePRECEDENCE_*integers, and_pick_winnercompares those to choose the winner — which becomes the edge'ssrc. Reordering them does not resize the edge set, it reverses edges already in it. No count-based check would notice.wonder.lifecycle._CONSTITUENT_KEY_VERSIONis the phantom idempotency-key prefix. Bumping it makes every existing phantom miss its own dedup guard and re-ingest, minting a second full set ofRELATES_TOedges.Verified by mutation
The issue asks for this explicitly. Eight cases, each restored and re-baselined between runs:
DEFAULT_JACCARD_MIN0.4 → 0.9 (source)_NEGATION_TOKENS(source)_STOPWORDSis a union of it — coupling documentedre.IGNORECASEfrom a triple pattern[\w-]+ANCHOR_TEXT_MAX_LEN1000 → 500manifest_digest_is_pinned+digest_history_is_contiguous_and_completeReachability was checked, not assumed
Two entries in a first pass would have shipped
gatestext that was false, and finding that is most of the value here.wonder.{evaluator,strategies}constants as the gate onRELATES_TO.wonder/__init__.pysays outright that those strategies are research-only and do not write to a live store, and their sole importer (wonder/runner.py) builds againstMemoryStore(":memory:"). They decide no edge in any user's store, so they are dropped rather than shipped as padding. The paths that do decide it are pinned instead: BFS hops ranked bywonder_consolidation.scorethen sliced by--top, and the dispatch-seeded persist-docs path.value_compareis dormant, and now says so. No shipped caller passesuse_value_comparison=True— it defaultsFalseandrelationships_auditdoes not thread it — so the slot gate mints nothing today. The four entries are kept and relabelled: pinning now makes flipping that flag a one-line change against a known baseline rather than a change to four unpinned constants at once.insert_edge, so a module that supplies the decision is structurally undetectable. Three were added by hand —bm25._TOKEN_PATTERN(the token universe for every Jaccard and membership test in the detector, so it outranks any single cutoff),models.ANCHOR_TEXT_MAX_LEN,wonder_consolidation._TOKENIZER_DROP. The two left unpinned (dedup's prefilter semantics,config_discovery's file resolution) are behavioural surfaces rather than constants and are named in the module docstring so their absence is a recorded decision.Covered-ness now matches on the exact module, not a package prefix — prefix matching is precisely what let
wonder.lifecyclelook covered bywonder.evaluator.Limits, stated rather than papered over
.aelfrice.toml[relationship_detector]section can override, so the manifest pins the shipped default, not what a given store ran with. Every entry names its override mechanism so the two are distinguishable.edgestable has no version and nocreated_at, so this does not make a historical edge attributable to the thresholds that produced it. Adding those columns is theedges-table migration that left stores unopenable-forever in [Umbrella] Deployment and operational hardening #1161. Historical reproduction stays explicitly out of scope, per the issue.(module, name). Three writers stampweight=1.0inline in theEdge(...)constructor;contradictionis the one that names it (SUPERSEDES_WEIGHT), which is why it is pinned and the others are not. Naming those three is a behaviour-preserving refactor that would close the gap, deliberately not bundled here. Transitively-reached constants are covered —_QUANTIFIER_TOKENSderives fromQUANT_AXIS, and the noun-phrase fragments compile into_PATTERNS.Verification
pytest: 7263 passed, 70 skipped, 71 xfailed. New file: 32 passed.pyright(strict, coverssrc/aelfriceandtests): 0 errors on both new files.github/main: clean.Summary by Sourcery
Pin and version detector threshold constants that determine non-spine edges and enforce coverage via a manifest-backed test suite.
New Features:
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
Documentation
Reliability
Maintenance