feat(relationship_detector): #387 POTENTIALLY_STALE writer via aelf doctor --detect-stale - #444
Conversation
Reviewer's GuideImplements the producer-side POTENTIALLY_STALE edge writer and wires it into Sequence diagram for aelf doctor --detect-stale executionsequenceDiagram
actor User
participant CLI as aelf_doctor_CLI
participant Doctor as _cmd_doctor
participant DetectStale as _cmd_doctor_detect_stale
participant RelConfig as RelationshipDetectorConfig
participant Store as MemoryStore
participant Writer as write_potentially_stale_edges
User->>CLI: aelf doctor --detect-stale [flags]
CLI->>Doctor: _cmd_doctor(args, out)
Doctor->>Doctor: check args.relationships
Doctor->>Doctor: check args.detect_stale
Doctor->>DetectStale: _cmd_doctor_detect_stale(args, out)
DetectStale->>RelConfig: load_relationship_detector_config()
DetectStale->>RelConfig: apply jaccard/confidence/max_pairs overrides
DetectStale->>Store: _open_store()
activate Store
DetectStale->>Writer: write_potentially_stale_edges(store, jaccard_min, residual_overlap_min, confidence_min, max_candidate_pairs)
activate Writer
Writer->>Writer: relationships_audit(store, thresholds)
Writer->>Writer: filter LABEL_CONTRADICTS pairs
Writer->>Writer: select pairs with score < confidence_min
loop for each sub_confidence pair
Writer->>Store: get_belief(a_id), get_belief(b_id)
Writer->>Writer: choose src,dst by created_at then id
Writer->>Store: get_edge(src, dst, EDGE_POTENTIALLY_STALE)
alt edge exists
Writer->>Writer: n_edges_skipped_existing++
else edge missing
Writer->>Store: insert_edge(Edge(src,dst,POTENTIALLY_STALE))
Writer->>Writer: n_edges_written++
end
end
Writer-->>DetectStale: PotentiallyStaleWriteReport
deactivate Writer
DetectStale->>Store: close()
deactivate Store
DetectStale->>DetectStale: format_write_report(report)
DetectStale->>CLI: print report to out
DetectStale-->>Doctor: exit code 0 or 1
Doctor-->>User: process exit
Class diagram for PotentiallyStaleWriteReport and related writer utilitiesclassDiagram
class PotentiallyStaleWriteReport {
+int n_pairs_audited
+int n_sub_confidence
+int n_edges_written
+int n_edges_skipped_existing
+int n_edges_skipped_self_pair
}
class RelationshipDetectorConfig {
+float jaccard_min
+float residual_overlap_min
+float confidence_min
+int max_candidate_pairs
}
class MemoryStore {
+get_belief(belief_id)
+get_edge(src_id, dst_id, edge_type)
+insert_edge(edge)
+close()
}
class Edge {
+str src
+str dst
+str type
+float weight
}
class RelationshipDetectorModule {
+write_potentially_stale_edges(store, jaccard_min, residual_overlap_min, confidence_min, max_candidate_pairs) PotentiallyStaleWriteReport
+format_write_report(report) str
+relationships_audit(store, jaccard_min, residual_overlap_min, confidence_min, max_candidate_pairs)
}
RelationshipDetectorModule ..> PotentiallyStaleWriteReport : returns
RelationshipDetectorModule ..> MemoryStore : uses
RelationshipDetectorModule ..> Edge : inserts
RelationshipDetectorModule ..> RelationshipDetectorConfig : thresholds
MemoryStore "*" o-- Edge : contains_edges
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR implements the ChangesPOTENTIALLY_STALE Edge Writer Implementation
Sequence DiagramsequenceDiagram
actor User
participant CLI as aelf doctor CLI
participant Store as MemoryStore
participant Detector as relationship_detector
participant Graph as Edge Graph
User->>CLI: aelf doctor --detect-stale
CLI->>Store: load store (with --relationships-* overrides)
CLI->>Detector: write_potentially_stale_edges(store, ...)
Detector->>Detector: relationships_audit() → contradicts pairs
Detector->>Detector: filter pairs where score < confidence_min
Detector->>Detector: sort by created_at (newer → src)
loop Each contradicting pair
Detector->>Store: check if POTENTIALLY_STALE edge exists
alt Edge not idempotent
Detector->>Store: insert (src, dst, POTENTIALLY_STALE, weight=1.0)
Detector->>Graph: edge written
else Already exists
Detector->>Detector: increment skipped count
end
end
Detector->>CLI: return PotentiallyStaleWriteReport
CLI->>User: format_write_report() → output (edges_written, skipped, etc.)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
| from aelfrice.relationship_detector import ( | ||
| LABEL_CONTRADICTS, | ||
| write_potentially_stale_edges, | ||
| ) |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
_cmd_doctor,--relationshipsis checked before--detect-stale, so if a user passes both flags only the relationships path runs; consider either enforcing mutual exclusivity at argparse level or documenting the precedence explicitly to avoid confusion. - In
_cmd_doctor_detect_stale, the CLI overrides forrelationships_jaccard,relationships_confidence, andrelationships_max_pairsare cast directly to float/int without error handling; wrapping these conversions in a small validator that surfaces a cleanValueError(similar to the threshold errors you already handle) would provide a friendlier CLI failure mode for bad inputs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_cmd_doctor`, `--relationships` is checked before `--detect-stale`, so if a user passes both flags only the relationships path runs; consider either enforcing mutual exclusivity at argparse level or documenting the precedence explicitly to avoid confusion.
- In `_cmd_doctor_detect_stale`, the CLI overrides for `relationships_jaccard`, `relationships_confidence`, and `relationships_max_pairs` are cast directly to float/int without error handling; wrapping these conversions in a small validator that surfaces a clean `ValueError` (similar to the threshold errors you already handle) would provide a friendlier CLI failure mode for bad inputs.
## Individual Comments
### Comment 1
<location path="src/aelfrice/cli.py" line_range="2694-2703" />
<code_context>
+ j_override = getattr(args, "relationships_jaccard", None)
+ c_override = getattr(args, "relationships_confidence", None)
+ mp_override = getattr(args, "relationships_max_pairs", None)
+ config = RelationshipDetectorConfig(
+ jaccard_min=(
+ float(j_override) if j_override is not None else config.jaccard_min
+ ),
+ residual_overlap_min=config.residual_overlap_min,
+ confidence_min=(
+ float(c_override)
+ if c_override is not None
+ else config.confidence_min
+ ),
+ max_candidate_pairs=(
+ int(mp_override)
+ if mp_override is not None
+ else config.max_candidate_pairs
+ ),
+ )
</code_context>
<issue_to_address>
**issue:** Threshold overrides can raise ValueError before the try/except, contradicting the documented exit semantics
Because `float(j_override)`, `float(c_override)`, and `int(mp_override)` run before the `try`, malformed CLI values (e.g. `--relationships-jaccard=foo`) raise `ValueError` that bypasses your handler and crash the command instead of exiting with 1 as documented. Please move the parsing / `RelationshipDetectorConfig` construction into the existing `try` or add a dedicated `try/except ValueError` that logs the same `aelf doctor --detect-stale: ...` message and returns 1.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| config = RelationshipDetectorConfig( | ||
| jaccard_min=( | ||
| float(j_override) if j_override is not None else config.jaccard_min | ||
| ), | ||
| residual_overlap_min=config.residual_overlap_min, | ||
| confidence_min=( | ||
| float(c_override) | ||
| if c_override is not None | ||
| else config.confidence_min | ||
| ), |
There was a problem hiding this comment.
issue: Threshold overrides can raise ValueError before the try/except, contradicting the documented exit semantics
Because float(j_override), float(c_override), and int(mp_override) run before the try, malformed CLI values (e.g. --relationships-jaccard=foo) raise ValueError that bypasses your handler and crash the command instead of exiting with 1 as documented. Please move the parsing / RelationshipDetectorConfig construction into the existing try or add a dedicated try/except ValueError that logs the same aelf doctor --detect-stale: ... message and returns 1.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/aelfrice/cli.py (1)
2690-2709: ⚡ Quick winExtract the shared config-resolution block into a helper to avoid drift.
The 20-line
load_relationship_detector_config()+RelationshipDetectorConfig(...)override block in_cmd_doctor_detect_stale(lines 2690–2709) is a verbatim copy of the identical block in_cmd_doctor_relationships(lines 2622–2641). If a new--relationships-*flag is introduced (e.g., aresidual_overlapoverride), it must be updated in both places.♻️ Proposed extraction
+def _resolve_relationship_detector_config( + args: argparse.Namespace, +) -> "RelationshipDetectorConfig": + from aelfrice.relationship_detector import ( + RelationshipDetectorConfig, + load_relationship_detector_config, + ) + config = load_relationship_detector_config() + j_override = getattr(args, "relationships_jaccard", None) + c_override = getattr(args, "relationships_confidence", None) + mp_override = getattr(args, "relationships_max_pairs", None) + return RelationshipDetectorConfig( + jaccard_min=( + float(j_override) if j_override is not None else config.jaccard_min + ), + residual_overlap_min=config.residual_overlap_min, + confidence_min=( + float(c_override) + if c_override is not None + else config.confidence_min + ), + max_candidate_pairs=( + int(mp_override) + if mp_override is not None + else config.max_candidate_pairs + ), + )Then in both handlers:
- config = load_relationship_detector_config() - j_override = getattr(args, "relationships_jaccard", None) - c_override = getattr(args, "relationships_confidence", None) - mp_override = getattr(args, "relationships_max_pairs", None) - config = RelationshipDetectorConfig( - jaccard_min=( - float(j_override) if j_override is not None else config.jaccard_min - ), - residual_overlap_min=config.residual_overlap_min, - confidence_min=( - float(c_override) - if c_override is not None - else config.confidence_min - ), - max_candidate_pairs=( - int(mp_override) - if mp_override is not None - else config.max_candidate_pairs - ), - ) + config = _resolve_relationship_detector_config(args)🤖 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/cli.py` around lines 2690 - 2709, The shared config-resolution code for relationship detector overrides is duplicated between _cmd_doctor_detect_stale and _cmd_doctor_relationships; extract that logic into a helper (e.g., a new function like resolve_relationship_detector_config) that calls load_relationship_detector_config and applies overrides from args (relationships_jaccard, relationships_confidence, relationships_max_pairs, etc.) returning a RelationshipDetectorConfig, then replace the inline blocks in both handlers to call this helper so future flags only need one update.
🤖 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 `@tests/test_relationship_detector_stale_writer.py`:
- Around line 185-217: The current tautological assertion in
test_writer_returns_report_counts should be replaced with a concrete equality:
assert that report.n_pairs_audited equals the expected total contradicting pairs
from the setup (one sub-confidence + one high-confidence = 2). In the test
function test_writer_returns_report_counts, replace the tautology involving
report.n_sub_confidence with a direct check assert report.n_pairs_audited == 2
(and keep or remove the existing >= check as optional) so the test actually
verifies the intended total audited count.
---
Nitpick comments:
In `@src/aelfrice/cli.py`:
- Around line 2690-2709: The shared config-resolution code for relationship
detector overrides is duplicated between _cmd_doctor_detect_stale and
_cmd_doctor_relationships; extract that logic into a helper (e.g., a new
function like resolve_relationship_detector_config) that calls
load_relationship_detector_config and applies overrides from args
(relationships_jaccard, relationships_confidence, relationships_max_pairs, etc.)
returning a RelationshipDetectorConfig, then replace the inline blocks in both
handlers to call this helper so future flags only need one update.
🪄 Autofix (Beta)
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
Run ID: e7657785-a59c-4bbf-8f77-9a80f7b131cd
📒 Files selected for processing (4)
docs/edge_rerank.mdsrc/aelfrice/cli.pysrc/aelfrice/relationship_detector.pytests/test_relationship_detector_stale_writer.py
| def test_writer_returns_report_counts(store: MemoryStore) -> None: | ||
| """Mixed store: report counts add up correctly. | ||
|
|
||
| Store contains: | ||
| - one sub-confidence contradicting pair → 1 edge written | ||
| - one high-confidence contradicting pair → 0 edges (excluded) | ||
| - one unrelated pair → 0 edges | ||
| """ | ||
| # Sub-confidence pair (score 0.4). | ||
| _make_belief(store, belief_id="s1", content=_CONTENT_A, | ||
| created_at="2026-01-01T00:00:00Z") | ||
| _make_belief(store, belief_id="s2", content=_CONTENT_B, | ||
| created_at="2026-02-01T00:00:00Z") | ||
| # High-confidence pair (score 0.5 — NOT sub-confidence). | ||
| _make_belief(store, belief_id="h1", content=_CONTENT_HI_A) | ||
| _make_belief(store, belief_id="h2", content=_CONTENT_HI_B) | ||
| # Unrelated pair. | ||
| _make_belief(store, belief_id="u1", content="sphinx fts indexing latency") | ||
| _make_belief(store, belief_id="u2", content="compression ratio on disk") | ||
|
|
||
| report = write_potentially_stale_edges(store) | ||
|
|
||
| # Exactly 1 sub-confidence contradicts pair qualifies. | ||
| assert report.n_sub_confidence == 1 | ||
| assert report.n_edges_written == 1 | ||
| assert report.n_edges_skipped_existing == 0 | ||
| assert report.n_edges_skipped_self_pair == 0 | ||
| # Total contradicts audited = sub + high. | ||
| assert report.n_pairs_audited == report.n_sub_confidence + ( | ||
| report.n_pairs_audited - report.n_sub_confidence | ||
| ) | ||
| # n_pairs_audited must be >= n_sub_confidence. | ||
| assert report.n_pairs_audited >= report.n_sub_confidence |
There was a problem hiding this comment.
Tautological assertion tests nothing — replace with the intended concrete check.
Lines 213–215:
assert report.n_pairs_audited == report.n_sub_confidence + (
report.n_pairs_audited - report.n_sub_confidence
)This reduces to X == X + (X − X) → X == X, which is unconditionally true and provides zero test coverage. The comment above it ("Total contradicts audited = sub + high") reveals the intent: verify the total contradicting-pair count equals 2 (one sub-confidence + one high-confidence pair from the test setup). The second assertion on line 217 (n_pairs_audited >= n_sub_confidence) is similarly always true by construction.
🐛 Proposed fix
- # Total contradicts audited = sub + high.
- assert report.n_pairs_audited == report.n_sub_confidence + (
- report.n_pairs_audited - report.n_sub_confidence
- )
- # n_pairs_audited must be >= n_sub_confidence.
- assert report.n_pairs_audited >= report.n_sub_confidence
+ # 1 sub-confidence + 1 high-confidence (score==0.5 NOT < 0.5) = 2 total.
+ assert report.n_pairs_audited == 2
+ assert report.n_sub_confidence == 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_relationship_detector_stale_writer.py` around lines 185 - 217, The
current tautological assertion in test_writer_returns_report_counts should be
replaced with a concrete equality: assert that report.n_pairs_audited equals the
expected total contradicting pairs from the setup (one sub-confidence + one
high-confidence = 2). In the test function test_writer_returns_report_counts,
replace the tautology involving report.n_sub_confidence with a direct check
assert report.n_pairs_audited == 2 (and keep or remove the existing >= check as
optional) so the test actually verifies the intended total audited count.
|
This PR is now behind Auto-rebase was removed because the bot has no signing key; rebasing as the bot strips author signatures and the |
|
[claim:review:Toug:2026-05-05T17:37:45Z] |
…onfidence contradicts (#387) Add PotentiallyStaleWriteReport dataclass and write_potentially_stale_edges() function. For each contradicts pair with score < confidence_min, one POTENTIALLY_STALE edge is emitted src=newer→dst=older (created_at lex order, id tiebreak). Pre-insert get_edge check keeps the call idempotent. Also add format_write_report() helper. Update module docstring to reflect that the POTENTIALLY_STALE writer ships here; CONTRADICTS write-path remains R2-deferred.
Add --detect-stale argparse flag to aelf doctor. Dispatch routes to the new _cmd_doctor_detect_stale handler, which opens the store and calls write_potentially_stale_edges with the same --relationships-jaccard / --relationships-confidence / --relationships-max-pairs overrides shared with --relationships. Prints format_write_report output on success. Update _cmd_doctor_relationships docstring to note the CONTRADICTS write-path remains R2-deferred while POTENTIALLY_STALE ships via #387.
…ort counts (#387) Seven unit tests against a real MemoryStore(:memory:): sub-confidence pairs emit edges, high-confidence pairs are excluded, writer is idempotent, newer→older direction is correct, created_at tiebreak on lex id order, refines/unrelated pairs produce zero edges, report count fields are internally consistent.
Expand the Producer side section to name the CLI entry point (aelf doctor --detect-stale), describe the staleness signal (sub-confidence contradicting pairs, score < confidence_min), the edge direction rule (newer belief → older belief, created_at lex order + id tiebreak), idempotency guarantee, and the shared tuning flags.
9f0295a to
68dafc0
Compare
|
[release:review:Toug:2026-05-05T17:41:36Z] |
Closes #387.
Summary
Wires the
POTENTIALLY_STALEedge writer intoaelf doctor, closingthe missing acceptance criterion #2 from #387. The substrate (#421)
shipped the edge-type constant, BFS skip-during-expansion pin, the
edge-type-keyed rerank consumer, and the bench-gate stub at
tests/bench_gate/test_edge_rerank_potentially_stale.py. This PRadds the producer side.
Acceptance check (#387)
EDGE_POTENTIALLY_STALEinmodels.py, BFS pin atBFS_EDGE_WEIGHTS[POTENTIALLY_STALE] = 0.0)aelf doctor --detect-staletests/bench_gate/test_edge_rerank_potentially_stale.pyfrom #421 runs lab-side (skips whenAELFRICE_CORPUS_ROOTunset, per directory-of-origin). Public CI verifies the stub's collection + skip path; lab-side run records the +1pp drop number in a follow-up comment on #387.docs/edge_rerank.md§ Producer sideDesign
Picked the staleness signal that the codebase had already pre-decided
(
relationship_detector.py:22-26,cli.py:2606-2608pre-PR docstrings):Staleness signal: sub-confidence (
score < confidence_min)LABEL_CONTRADICTSpairs fromrelationships_audit(). High-confidencecontradicting pairs are explicitly NOT touched — those belong to the
deferred
CONTRADICTSwrite-path hook (R2 in #201, still bench-gated).Edge direction: for each qualifying pair
(a, b), onePOTENTIALLY_STALE edge is emitted with
src = newer belief, dst = older belief. "Newer" is the belief whose(created_at, id)tuple islexicographically greater (ISO timestamps, tie-break on lex id order).
Semantics: the newer belief casts doubt on the older one.
Idempotent: a pre-insert
get_edge(src, dst, POTENTIALLY_STALE)check skips pairs whose edge already exists. Repeated invocations are
safe.
Stdlib only. No new dependencies. Determinism is preserved end-to-end:
relationships_auditalready sorts pairs by(belief_a_id, belief_b_id),and the writer iterates in that order.
CLI surface
The tuning flags share semantics with
--relationships. Models thewrite-mode shape after
--classify-orphans(the existing precedentfor write-mode doctor flags).
Sample output:
Test plan
7 new unit tests in
tests/test_relationship_detector_stale_writer.py:test_writer_emits_edges_for_sub_confidence_pairs— sub-confidencepair (always/rarely, score 0.4) yields exactly one edge.
test_writer_skips_high_confidence_pairs— negation-only pair(score exactly 0.5 ==
confidence_min) yields zero edges. Thefilter is strict less-than (
< confidence_min), so 0.5 ishigh-confidence. This boundary behaviour is intentional and the
test pins it.
test_writer_idempotent— second call writes 0 edges,n_edges_skipped_existingreflects the prior insertion.test_writer_direction_newer_to_older— newer (latercreated_at)is
src, older isdst. Reverse direction does NOT exist.test_writer_direction_tiebreak_on_id— identicalcreated_at,lex-greater id wins as
src.test_writer_skips_refines_and_unrelated— onlyLABEL_CONTRADICTSpairs are eligible.
test_writer_returns_report_counts— mixed store; counts add up.Local:
uv run pytest tests/ --ignore=tests/e2e -q→ 2508 passed, 41 skipped.Out of scope
CONTRADICTSwrite-path hook is unchanged (stillR2-deferred per Semantic contradiction detector (relationship_detector) — v2.0 evaluation #201).
aelf doctordoes not auto-run--detect-stale; the operator opts in.+1pp drop number is recorded by the lab-side run.
Files changed
src/aelfrice/relationship_detector.py—+150(newwrite_potentially_stale_edges,PotentiallyStaleWriteReport,format_write_report,__all__extension, docstring reconciliation).src/aelfrice/cli.py—+85(new_cmd_doctor_detect_stale,argparse flag, dispatch in
_cmd_doctor, docstring update).tests/test_relationship_detector_stale_writer.py—+217(new file).docs/edge_rerank.md—+24(Producer-side § expanded).Summary by Sourcery
Add a POTENTIALLY_STALE edge writer and expose it via the
aelf doctor --detect-staleCLI to emit edges for sub-confidence contradicting belief pairs.New Features:
write_potentially_stale_edgesand reporting helpers to generate POTENTIALLY_STALE edges from low-confidence contradicting relationships.aelf doctor --detect-stalecommand-line option to scan for stale beliefs and write POTENTIALLY_STALE edges with shared tuning flags from--relationships.Enhancements:
Tests:
Summary by CodeRabbit
New Features
aelf doctor --detect-stalecommand now identifies contradicting belief pairs and emits stale edge markersDocumentation
Tests