feat: add policy-bounded self-improvement reviews - #3201
Conversation
|
Warning Review limit reachedNext included review available in 32 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (34)
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 |
![protoreview[bot]](https://avatars.githubusercontent.com/in/4233969?s=60&v=4){kind=small}
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head 2f825e56ad17 · formal
The panel's brief could not be read from this round's report (no delimited brief block). The findings below are unaffected.
No findings — the review came back clean.
findings JSON (machine-readable)
[]
2f825e5 to
6784f43
Compare
There was a problem hiding this comment.
QA panel review — FAIL
code-review-structural · head 6784f43852a2 · formal
The PR adds self-improvement config fields and a task-close review dispatch. Overall risk is low-to-moderate: one startup-crash path from a YAML null, and one unguarded call that can turn a successful close into a 500. Fix first: add or {} to the four data.get("self_improvement", {}) calls in graph/config.py. The panel did not disagree on any finding; both were confirmed. Verification corrected finding 2's impact language — the task is closed in the store; the defect is that the API returns a 500 to the client, violating the "curation must never break task closure" contract upheld at the lg_tools.py call site. No structural pass was run; coverage is adequate since both touched files carry findings.
Findings
| Severity | Location | Finding | Verified | |
|---|---|---|---|---|
| 🟠 | major | graph/config.py:1919 |
If a user writes self_improvement: (bare key / YAML null) in their config, data.get("self_improvement", {}) returns None and the chained `.get("enabled",… |
confirmed |
| 🟡 | minor | operator_api/routes.py:390 |
The dispatch_task_review call in _tasks_close is not wrapped in a separate try/except, unlike the identical call in tools/lg_tools.py which is guarded wi… |
confirmed |
findings JSON (machine-readable)
[
{
"file": "graph/config.py",
"line": 1919,
"severity": "major",
"category": "correctness",
"claim": "If a user writes `self_improvement:` (bare key / YAML null) in their config, `data.get(\"self_improvement\", {})` returns `None` and the chained `.get(\"enabled\", ...)` raises `AttributeError`, crashing config load at startup.",
"evidence": "The four new lines all use `data.get(\"self_improvement\", {})` without the `or {}` guard that every other nested-section pattern in this method uses (e.g. `soul = data.get(\"soul\", {}) or {}`). The PR's own test `test_soul_drift_defaults_when_section_absent_or_null` documents this failure mode: \"nested null isn't caught by the top-level normalizer, hence the `or {}`\".",
"verdict": "confirmed",
"note": "Verified in diff: all four lines use `data.get(\"self_improvement\", {}).get(...)` without `or {}`. A bare YAML key parses to None, and None.get() raises AttributeError. The PR's own test docstring confirms this failure mode. Minor overstatement: the `watches` section uses the same unguarded pattern, so 'every other' is slightly inaccurate, but the bug is real."
},
{
"file": "operator_api/routes.py",
"line": 390,
"severity": "minor",
"category": "correctness",
"claim": "The `dispatch_task_review` call in `_tasks_close` is not wrapped in a separate try/except, unlike the identical call in `tools/lg_tools.py` which is guarded with the explicit contract 'curation must never break task closure'; if `dispatch_task_review` raises, the operator API returns a 500 to the client for a successful close.",
"evidence": "operator_api/routes.py: `dispatch_task_review(issue, reason=req.reason or \"\")` \u2014 no try/except. Compare tools/lg_tools.py: `try: from graph.self_improvement import dispatch_task_review; dispatch_task_review(i, session_id=session_id, reason=reason) except Exception: # noqa: BLE001 \u2014 curation must never break task closure`",
"verdict": "confirmed",
"note": "Verified in diff: the routes.py call has no separate try/except (it's inside the outer try that converts to 500), while lg_tools.py has the exact quoted guard. The inconsistency is real. However, 'the task is not closed' is inaccurate \u2014 task_svc.close succeeds before dispatch_task_review is called, so the task IS closed; the actual impact is the API returns 500 to the client for a successful operation."
}
]
|
Addressed both confirmed QA findings in
Local full gate: 5 checks passed; 6,721 passed, 15 skipped. |
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head 5715415c756d · formal
Both prior defects are confirmed fixed in the current head: the or {} guard in graph/config.py neutralises the bare-key AttributeError, and the try/except around dispatch_task_review in operator_api/routes.py matches the contract in lg_tools.py. All four LLM finders returned clean on this round, and the structural pass completed with no findings. No open items remain; the PR is ready to merge.
Prior requests
| Prior finding | Disposition | Why | |
|---|---|---|---|
| ✅ | graph/config.py:1919 |
fixed | the current head adds or {} after data.get("self_improvement", {}), so a bare self_improvement: key (YAML null) no longer propagates None into the chai… |
No findings — the review came back clean.
findings JSON (machine-readable)
[]
Superseded — a later head cleared the QA panel (see the newest verdict).
1 participant
Summary
Safety
Validation
python scripts/gate.py— 5 checks passed; 6,682 passed, 15 skippedRefs #3069