Repository navigation
fix(guards): two assertions that could not hold the property they name (#17673, #17650) - #17678
Conversation
#17673, #17650) Both are the #15826 class: a claim about structure enforced by something that cannot express structure. #17673 -- the versioning redaction assertion was the sibling of the one #17667 replaced, and survived because CI only flagged the one my change broke. Its property is NOT the runner path's: that one is positional within a function, this one is aliasing. `_apply_version_to_fact` receives `target_version` from its caller and must write the redacted value back through a subscript on that dict, because `revert_to_version` reads the same dict afterwards to record the revert. A local-only redaction reads as correct and leaves the second write raw -- the regression #13708's review found. So two assertions rather than a mirror of the runner's one: the write goes through the caller's dict, AND the apply call precedes the record call. Mutation table, new guard vs the substring it replaces: local-only redaction (the real regression) new FAIL old FAIL redaction removed new FAIL old FAIL record before applying (order swapped) new FAIL old PASS equivalent rewrite via a temp variable new PASS old FAIL Wrong in both directions on two of four -- blind to a regression it names, loud about a refactor that preserved the property. #17650 -- `test_the_uncovered_record_only_shrinks` pins equality. The equality is correct and documented: RATCHET_BASELINES.md holds exact congruence in both directions up as the one exemplar satisfying staleness AND blindness, and headroom under a ceiling is room for a new bypass to appear with nothing failing. The missing half is the rest of the stated convention -- "and only ever goes down" -- which was prose. Equality makes MAX track the count in EITHER direction, so a new bypass is made green by raising the number: 27 -> 38 -> 39 -> 41 -> 42 -> 45. Added the direction check with the escape #17650's own criterion asks for (justified, not forbidden): MAX may not exceed the base revision's value unless `ACKNOWLEDGED_RAISE` names exactly the current value and cites an issue. Checked in both directions -- a record that no longer matches MAX fails as stale, so lowering MAX cannot leave a standing permission behind it. Verified across five states: bare raise fails, matching record passes, stale record fails, record without an issue reference fails, restored passes. Neither RATCHET_BASELINES.md nor ARCHITECTURE_EXCEPTIONS.md defines an allowance path for a baseline raise, so this test carries the record itself. The preferred route is still the one the ratchet exists to force: widen the filter until the read is covered, as #16442 did. A git-based check that cannot resolve the base SKIPS with a stated reason rather than passing -- building a guard that reports clean having examined nothing, inside the fix for a guard that could not hold its own direction, would be a pointed own goal. `kb_content_redaction_chokepoint_guard_test.py` reached 629 lines, over the 600 ceiling, so the placement helpers moved to `repo_tests/_redaction_placement.py` rather than the ceiling moving: 629 -> 488, plus a 176-line module. Refs #17673, #17650, #15826, #17667, #13708, #16442
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughThe changes add AST-based checks for redaction placement in extract and version-revert flows. They also add a test that compares the uncovered-read limit with its value on a configured base revision and validates any recorded acknowledgement. ChangesRedaction guard checks
Uncovered-read limit guard
Priority: ➖ Normal Merge Risk: 🔵 Low · up to Production code is unchanged. However, the new guard tests can pass in situations they are meant to catch, so later regressions in secret redaction or an increase in the uncovered-read limit could go unnoticed. Tightening these checks is recommended, either before merge or as a prompt follow-up. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The changes strengthen security checks without changing production behavior. The new limit check can be skipped when a base revision is unavailable, so its protection is not assured in every build environment. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❓ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The implementation satisfies the main coding objectives. For ✨ 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 |
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: a135081b-d787-4124-8d9d-72a105ac3035
📒 Files selected for processing (4)
repo_tests/_redaction_placement.pyrepo_tests/kb_content_redaction_chokepoint_guard_test.pyrepo_tests/python_filter_covers_its_guards_test.pyrepo_tests/python_filter_uncovered_reads.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
| for node in ast.walk(fn): | ||
| if not isinstance(node, ast.Assign) or not isinstance(node.value, ast.Call): | ||
| continue | ||
| callee = _callee_name(node.value.func) | ||
| if callee not in REDACTING_CALLS: | ||
| continue | ||
| targets = [t for tgt in node.targets for t in (tgt.elts if isinstance(tgt, ast.Tuple) else [tgt])] | ||
| if not any(isinstance(t, ast.Name) and t.id == "input_data" for t in targets): | ||
| continue | ||
| if node.lineno < first_loop: | ||
| return True, "" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Exclude uncalled nested functions from the extract-path check.
ast.walk(fn) also visits assignments inside nested functions. If the only matching assignment is in an uncalled nested function before the extract loop, this helper returns True although the extract tasks receive unsanitised input_data. Limit the check to statements on the extract path, and add that mutation as a negative fixture. As per path instructions, “Every detector needs a contrast pair: a fixture that SHOULD trip it and one that should not.”
Source: Path instructions
| if _callee_name(node.value.func) not in REDACTING_CALLS: | ||
| continue | ||
| for target in node.targets: | ||
| if ( | ||
| isinstance(target, ast.Subscript) | ||
| and isinstance(target.value, ast.Name) | ||
| and target.value.id == "target_version" | ||
| and isinstance(target.slice, ast.Constant) | ||
| and target.slice.value == "content" | ||
| ): | ||
| return True, "" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Check the value passed to the redacting call.
The target and callee checks accept target_version["content"] = redact_content(other_content). The helper then reports in-place redaction even though it replaced the version content with a value from another source. Check that the redacting call receives target_version["content"], or explicitly recognise an equivalent data flow.
| for target in node.targets: | ||
| if ( | ||
| isinstance(target, ast.Subscript) | ||
| and isinstance(target.value, ast.Name) | ||
| and target.value.id == "target_version" | ||
| and isinstance(target.slice, ast.Constant) | ||
| and target.slice.value == "content" | ||
| ): | ||
| return True, "" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Require redaction before the Redis write.
This helper returns True if the assignment moves below the hset call in autobot-backend/knowledge/versioning.py. In that order, hset writes the raw target_version["content"] to the live fact before the assignment runs. Check that the redacting assignment precedes the consuming write, not just that the assignment exists.
| calls = [(c.lineno, _callee_name(c.func)) for c in ast.walk(fn) if isinstance(c, ast.Call)] | ||
| applies = [ln for ln, name in calls if name == "_apply_version_to_fact"] | ||
| records = [ln for ln, name in calls if name == "create_version"] | ||
| if not applies: | ||
| return False, "revert_to_version no longer calls _apply_version_to_fact at all" | ||
| if not records: | ||
| return False, "revert_to_version no longer calls create_version -- re-derive this guard" | ||
| if min(applies) < min(records): | ||
| return True, "" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require the apply call to be awaited before recording.
Removing await from revert_to_version leaves both call line numbers unchanged, so this helper returns True. _apply_version_to_fact then produces a coroutine instead of mutating target_version, and create_version can record the raw content. Check that the relevant apply call is awaited before the record call.
| out = subprocess.run( | ||
| ["git", "show", f"{ref}:{rel}"], |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Compare with the branch’s merge-base revision.
git show origin/main:… reads the current branch tip of origin/main, not the revision where this branch diverged. If this branch raises MAX_UNCOVERED_READS from 45 to 46 and origin/main later rises to 47, the test accepts 46 without an acknowledgement. Resolve the merge-base commit and read the value from that commit instead.
🧰 Tools
🪛 ast-grep (0.45.3)
[error] 295-302: Command coming from incoming request
Context: subprocess.run(
["git", "show", f"{ref}:{rel}"],
cwd=repo_root(),
capture_output=True,
text=True,
env=scrubbed_git_env(),
timeout=30,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.16.6)
[error] 296-296: subprocess call: check for execution of untrusted input
(S603)
[warning] 296-296: subprocess.run without explicit check argument
Add explicit check=False
(PLW1510)
[error] 297-297: Starting a process with a partial executable path
(S607)
| base, why = _max_on_base() | ||
| if base is None: | ||
| pytest.skip(f"cannot compare against the base revision, so direction is UNCHECKED: {why}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not skip the direction guard when the base is unavailable.
If neither configured reference resolves, pytest.skip leaves a raised MAX_UNCOVERED_READS unchecked. This can occur in a shallow checkout without a local main reference. Make the gating test fail when it cannot read a base value, and arrange for the checkout to supply the required history.
| assert issue.strip().startswith("#"), f"ACKNOWLEDGED_RAISE must cite an issue, got {issue!r}" | ||
| return |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require an issue identifier in ACKNOWLEDGED_RAISE.
issue.strip().startswith("#") accepts "#" and "#not-an-issue". With either value, the test returns before the direction assertion, so a raise passes without the required issue reference. Validate the whole reference, for example with re.fullmatch(r"#\d+", issue.strip()).
1 participant
Thinking Path
Both issues are the #15826 class — a claim about structure enforced by something that cannot
express structure — and both were found by fixing a sibling and looking one function further.
#17673. #17667 replaced an order-blind substring assertion on the runner path. The identical
shape survived twelve lines up, on the versioning path, because CI only flagged the one my change
happened to break. That is the mechanism by which #15826's population persists: a class gets
fixed at whichever instance fails, and its siblings inherit nothing.
The instruction I was given was to mirror the runner's positional assertion. Checking first showed
that would assert the wrong thing. The runner's property is positional within one function —
redact before the extract loop. This one is aliasing:
_apply_version_to_factreceivestarget_versionfrom its caller and must write the redacted value back through a subscript onthat dict, because
revert_to_versionreads the same dict afterwards to record the revert. Alocal-only redaction satisfies every reasonable reading of "it redacts" and leaves the second write
raw — the regression #13708's review actually found. So two assertions, not a mirror of one.
#17650. The brief said to change
len(uncovered) == MAX_UNCOVERED_READSto<=. Reading theissue and
RATCHET_BASELINES.mdshowed that would be a regression: the doc holds exact congruencein both directions up as "the only one of the five satisfying staleness and blindness", and
the test's own message gives the reason — "headroom under a ceiling is room for a new bypass to
appear with nothing failing". The issue never asked for
<=either; it says "the equality pin isgenuinely good and should stay".
The real defect is the other half of the convention stated at
python_filter_uncovered_reads.py:23: "and only ever goes down". That half was prose. Equalitymakes MAX track the count in either direction, so a new bypass is made green by raising the
number — which is how it went 27 → 38 → 39 → 41 → 42 → 45.
What Changed
#17673 —
repo_tests/kb_content_redaction_chokepoint_guard_test.pyReplaced
assert 'target_version["content"] = redact_content(...)' in sourcewith two ASTassertions: the redacting call's result is assigned back through a subscript on
target_version(not a local), and
revert_to_versioncalls_apply_version_to_factbeforecreate_version.Wrong in both directions on two of four. It passed the order swap it was named for, and failed
an equivalent rewrite that preserved the property. A guard that fails a correct rewrite does not
merely under-detect — it punishes the right change, which is the argument for replacing it
rather than widening it.
#17650 —
repo_tests/python_filter_covers_its_guards_test.py+python_filter_uncovered_reads.pyKept
==. Addedtest_the_ceiling_may_not_rise_above_the_base_revision: MAX may not exceed itsvalue on the base revision, unless
ACKNOWLEDGED_RAISEnames exactly the current value andcites an issue. That is the escape #17650's own AC2 asks for — "the criterion is that the
justification is recorded, not that raising is forbidden" — and it is checked in both directions, so
a record that no longer matches MAX fails as stale and lowering MAX cannot leave a standing
permission behind it.
Verified across five states: bare raise fails · matching record passes · stale record fails · record
without an issue reference fails · restored passes.
Neither
RATCHET_BASELINES.md(no exception mechanism; its checklist has no "how to raise" entry)nor
ARCHITECTURE_EXCEPTIONS.md(which records code-structure deviations, not baseline movements)defines an allowance path, so the test carries the record itself. The preferred route is still the
one the ratchet exists to force — widen the filter until the read is covered, as #16442 did
("filter entry instead of a raised MAX").
A git-based check that cannot resolve the base SKIPS with a stated reason rather than passing.
Building a guard that reports clean having examined nothing, inside the fix for a guard that could
not hold its own direction, would be a pointed own goal.
The file split is payback, not a ceiling move.
kb_content_redaction_chokepoint_guard_test.pyreached 629 lines, over the 600 ceiling, so the placement helpers moved to the new
repo_tests/_redaction_placement.py: 629 → 488, plus a 176-line module. No ceiling was raisedand no baseline entry was added —
repo_tests/is not inEXCLUDED_PREFIXESand the file is not inKNOWN_LARGE.Verification
repo_tests/suite against the committed state: 4704 passed, 26 skipped, 3 xfailed, 0failed (9m35s). The baseline before this change was 4703; the delta is the new direction check.
reverted, with the file restored and re-verified green afterwards.
black,isort,flake8clean on all four touched files.and the
ACKNOWLEDGED_RAISEwork, so its result described a tree that no longer existed.Local counts are labelled local; CI is the authority.
One note for the next handover
A delta patch applied to this branch failed on first attempt, and the reusable form is: the patch
was built against
c08bf1b45band the worktree was not atc08bf1b45b. Pre-commit'send-of-file fixer had trimmed a trailing blank line after the earlier commit staged the file, so
the working tree diverged from its own HEAD by exactly the byte the patch used as trailing context.
Any handover into a worktree carrying an unnoticed fixer edit hits this.
Model Used
Claude Opus 5 (1M context)
Closes #17673
Closes #17650
Refs #15826, #17667, #13708, #16442, #17353
Summary by CodeRabbit