fix(config): thread [relationship_detector] thresholds into ingest - #1323
Conversation
…1299) `ingest.py` resolved `auto_detect` from `.aelfrice.toml` and then called `write_semantic_edges(store, new_belief_ids=...)` with no threshold arguments, so `jaccard_min` / `confidence_min` / `max_candidate_pairs` were honoured by the read-only `aelf doctor --relationships` audits and silently ignored on the one path that mutates the graph. Adjacent keys in one section with opposite reach, and the asymmetry ran the risky direction: the audit was tunable, the writer was pinned at 0.4/0.5/5000. Honoured at ingest after this change: `auto_detect` (as before), `jaccard_min`, `confidence_min`, `max_candidate_pairs`. Still NOT configurable anywhere: `residual_overlap_min` (dataclass field, no TOML parse) and `max_edges_per_belief` (no key by design — the Exp-48 write-gate is a caller kwarg). So the section is still not uniform; it is the three parsing keys that now reach the writer. No second config walk. `is_auto_relationship_detection_enabled()` already falls through to a `.aelfrice.toml` walk, so calling `load_relationship_detector_config()` beside it would double the filesystem probes on a per-turn hot path (#1289/#1298). Instead `auto_detect` is parsed onto `RelationshipDetectorConfig` and handed to the flag resolver as its `explicit` argument, which is decisive before the resolver's own TOML step. Measured `.aelfrice.toml` `is_file` probes during one `ingest_turn`, cwd four levels below the config root: before: 11 probes (no config file present) / 4 probes (config found) after: 11 probes / 4 probes Precedence for `auto_detect` is unchanged (env > TOML > default-off), and default-off keeps a fresh install byte-identical. Distinguishing tests — each mutation was applied to the staged implementation and the named tests failed: 1. Reverted the ingest call to `write_semantic_edges(store, new_belief_ids=inserted)`: test_ingest_honours_toml_jaccard_min, test_ingest_honours_toml_confidence_min and test_ingest_threads_max_candidate_pairs_from_toml FAILED. 2. Changed the resolver to call `is_auto_relationship_detection_enabled(start=start)` without the parsed flag (the second-walk defect): test_resolve_ingest_config_walks_the_tree_once FAILED, 8 probes vs 4. 3. Made the loader stop parsing `auto_detect` (`ad_obj = False`): test_config_loader_reads_auto_detect plus the four above FAILED.
) The section was absent from docs/user/CONFIG.md — `jaccard_min` appeared only in docs/design/dedup.md, and there in the `dedup` (0.8) context, which is a different constant in a different module. Adds the section to the schema block and a keys section with a table stating, per key, which consumer honours it: the ingest write path, the `aelf doctor` audits, or neither. `residual_overlap_min` and `max_edges_per_belief` are recorded as having no TOML key at all, so the doc does not imply a uniformity the section still lacks.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 14 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)
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 |
Reviewer's GuideThreads [relationship_detector] config thresholds through the ingest path while preserving single-pass config resolution and clarifying configuration/documentation semantics. Sequence diagram for ingest threading relationship_detector config thresholdssequenceDiagram
participant Ingest as _ingest_turn_ids
participant RelConfig as resolve_ingest_relationship_config
participant Loader as load_relationship_detector_config
participant Flag as is_auto_relationship_detection_enabled
participant Writer as write_semantic_edges
Ingest->>RelConfig: resolve_ingest_relationship_config()
RelConfig->>Loader: load_relationship_detector_config(start)
Loader-->>RelConfig: RelationshipDetectorConfig
RelConfig->>Flag: is_auto_relationship_detection_enabled(config.auto_detect, start)
Flag-->>RelConfig: enabled
RelConfig-->>Ingest: (enabled, config)
Ingest->>Writer: write_semantic_edges(store, jaccard_min=config.jaccard_min, residual_overlap_min=config.residual_overlap_min, confidence_min=config.confidence_min, max_candidate_pairs=config.max_candidate_pairs, new_belief_ids=inserted)
Note over Ingest,RelConfig: Config and flag resolved in one `.aelfrice.toml` walk
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
robotrocketscience
added
the
attn:review
|
[claim:review:Setr:2026-08-04T04:40:21Z] |
…t off (#1299) resolve_ingest_relationship_config loaded the [relationship_detector] config before consulting AELFRICE_AUTO_RELATIONSHIPS, but the flag-only resolver it replaced checked env first and returned without touching the filesystem. An install that sets the var to a falsy value therefore went from 0 to 11 .aelfrice.toml probes per ingested turn, for a config whose only consumer -- write_semantic_edges -- does not run. That is the #1289/#1298 class the threading was specifically designed to avoid, and the PR's "probe count unchanged" claim was measured env-unset only. Env-off now returns before the walk. Env-on still walks, because the thresholds are then actually used; that cost is the point of the fix. Measured on a 4-deep tree: env=0 is 0 probes before and after; env unset is 11 before and after; env=1 is 0 -> 11. New test asserts both arms (env-off 0, env-unset non-zero) so neither passes vacuously; removing the guard fails it. The existing env-precedence test is updated for the documented contract that the returned config is meaningful only when enabled is True. Also switches the test module's one `import aelfrice.relationship_detector` to the `from` form so the module is not imported both ways, clearing the open CodeQL thread that would otherwise bounce the merge-train.
Review — one defect found and fixed on the branch (
|
| env unset | AELFRICE_AUTO_RELATIONSHIPS=0 |
=1 |
|
|---|---|---|---|
before (is_auto_relationship_detection_enabled()) |
11 probes | 0 | 0 |
| after (this PR, as pushed) | 11 probes | 11 | 11 |
The =1 column is a real and intended cost — the thresholds have to come from somewhere, that is the whole fix. The =0 column is not: it is a full walk to root, on a path that runs every ingested turn, resolving a config whose only consumer is a writer that will not run. That is exactly the #1289/#1298 class the design note in the docstring exists to avoid, and #1304 is open against it right now.
It went unnoticed because the invariant was only ever exercised env-unset — test_resolve_ingest_config_walks_the_tree_once opens with monkeypatch.delenv(ENV_AUTO_RELATIONSHIPS), and the CHANGELOG's "11 with no config file, 4 with one four directories up, before and after" is the same regime. Neither is wrong; they just do not reach the branch that regressed.
What I pushed
resolve_ingest_relationship_configreturns before the walk when env is decisively falsy. Env-on still walks.- New
test_resolve_ingest_config_env_off_probes_nothingasserts both arms — env-off is 0, env-unset over the same tree is non-zero — so it cannot pass vacuously. Removing the guard fails it (and fails the precedence test too). - The returned config is now documented as meaningful only when
enabledis True; on the short-circuit it is the module defaults rather than the file's values.test_resolve_ingest_config_env_still_wins_over_tomlassertedjaccard_min == 0.55on the env-off arm, which was reading a value no caller consumes — updated to assert the default and say why. If you would rather the contract stay "the config is always the file's values", the alternative is to keep the walk and drop the guard, but then the=0row above stands. - CHANGELOG: the probe sentence now states the env-unset measurement as env-unset and gives the env-set numbers separately, so the claim covers the whole precedence rather than half of it.
- Test module imported
aelfrice.relationship_detectorboth asimport xandfrom x import y; switched line 323 to thefromform. That clears the open CodeQL thread, which merge-train counts and would otherwise bounce the label.
Your reviewer note
cli.py:6490 and :6558 reconstruct RelationshipDetectorConfig with keyword arguments only, so the new field takes its default and nothing breaks. The audit path never reads auto_detect, so it is inert as you said. I left it alone — tightening it here would put an unrelated surface in a PR that is already over the size cap, and the docstring you added already scopes the field to the ingest resolver. Worth a follow-up only if a second consumer ever reads the flag off a cli.py-built config.
Verification
Full suite: 6980 passed, 69 skipped, 71 xfailed. Two failures, both ..._when_fastmcp_missing, are a local dependency-state artifact from the mcp bump on main and are green in CI on this same commit. Discretion grep on added lines: clean. Commit is signed.
Not labelling ready-to-merge yet — waiting for CI on ad206de2 and for the CodeQL thread to clear.
robotrocketscience
added
the
ready-to-merge
|
merge-train: merged ad206de → |
github-actions
Bot
removed
the
ready-to-merge
|
[release:review:Setr:2026-08-04T04:51:59Z] |
2 participants
Closes #1299.
[relationship_detector]had a split personality at ingest:auto_detectwas honoured, butjaccard_min— and the other thresholds — were silently ignored, because the ingest call site passed onlynew_belief_idsand let every threshold fall back to its module default. A user who setjaccard_minin.aelfrice.tomlgot the default 0.4 anyway, with no warning.Direction
The issue framed this as a choice: thread the config through, or document the constants as deliberate. Threading was chosen (operator ruling recorded on the issue), because the alternative leaves a documented TOML key that demonstrably does nothing.
The trap this avoids
The naive fix — call
load_relationship_detector_config()inside the enabled-flag branch — is not free on the paths that matter.is_auto_relationship_detection_enabled()already falls through env and kwarg to a TOML read, so a TOML-enabled install pays a filesystem walk; adding a second independentload_relationship_detector_config()call per ingested turn adds another walk on a hot path. That is precisely the cost #1289/#1298 exist to eliminate, and #1304 is open against the same class right now. The config is resolved once and reused; before/after walk counts are in the commit message as counts, not latency, since the perf gate is load-sensitive.Scope, stated so the section is not oversold
This does not make
[relationship_detector]mean one thing everywhere, and the docs now say which keys reach which path:residual_overlap_minhas no TOML parse at all — it is not a key, it is a constant.max_edges_per_beliefhas no key by design (removed deliberately in7040c22a).Only the keys that are actually parsed are now threaded to ingest.
docs/user/CONFIG.mddocuments the section with per-key reach rather than implying uniform coverage.Verification
Reverting the ingest call site to the bare
new_belief_idsform makes the new test fail — the mutation and result are in the commit message. Tests run withAELFRICE_DBpinned to a fresh temp path. CHANGELOG insert-only under[Unreleased].Reviewer note
An audit pass flagged that
cli.py's tworelationships_auditcall sites reconstructRelationshipDetectorConfigwithout the newauto_detectfield, so it defaults toFalseon a config the dataclass docstring now describes as "the resolved[relationship_detector]section". That is pre-existing and inert for the audit path (which does not consultauto_detect), but the docstring is now slightly ahead of the call sites. Worth a look during review; happy to tighten either the docstring or the call sites here rather than in a follow-up.Summary by Sourcery
Thread relationship detector configuration thresholds from
.aelfrice.tomlinto the ingest write path while preserving the single-config-walk behaviour on the ingest hot path.Bug Fixes:
jaccard_min,confidence_min, andmax_candidate_pairsfrom[relationship_detector]are honoured by ingest when auto-detect is enabled.Enhancements:
resolve_ingest_relationship_configto resolve the auto-detect flag and thresholds in a single config read for ingest.auto_detectflag onRelationshipDetectorConfigto unify configuration resolution across consumers.Documentation:
[relationship_detector]section with per-key reach, defaults, and precedence between ingest and audit commands inCONFIG.md.Tests:
.aelfrice.tomlfilesystem walk per turn.