Repository navigation
refactor(redaction): name the redaction boundary and pin the concept census (#16688) - #17335
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe changes document redactor ownership, update credential-key matching to use a shared vocabulary, widen JWT payload matching, and add a repository test that checks the census of detector-owning modules. ChangesRedaction boundary and detection
Priority: ➖ Normal 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
Full details: Linked Issues checkExplanation
Resolution Migrate at least one directly affected module or caller onto the selected redactor. Add the required grep-based guard, or record an approved change to that acceptance criterion. Update the boundary document and tests to reflect the completed migration and guard. Full details: Out of Scope Changes checkExplanation
✨ Finishing Touches 💡 1📝 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 |
|
| anchor | was | now |
|---|---|---|
redact_text |
:97 | :102 |
redact_mapping |
:109 | :114 |
redact_cloud_identifiers |
:126 | :131 |
redact_provider_error |
:140 | :145 |
Only redact_provider_error was reported; redact_text sits on the preceding line of the doc and would not have been caught by checking the reported line alone.
Two anchors that look broken and are not
A definition-only probe also flagged import_module (:550) and spec_from_file_location (:619) in loader.py. Those are stdlib call sites, and the test has an explicit branch for anchors with no definition in the file — both are named in its comment. The probe was wrong, not the doc; "fixing" them would have broken two correct anchors.
Scope check
This PR adds a docstring pointer to eight modules, so the sweep covered all 38 anchors rather than only redaction.py. The other seven are unaffected — either unanchored or anchored above the insertion point.
Verification
repo_tests/threat_model_anchors_resolve_test.py ... 41 passed
THREAT_MODEL.md stays at 140 lines, which is its own ceiling (test_document_exists_and_is_short_enough_to_read_every_review asserts <= 140) — this change edits numbers in place and adds none.
Root-caused by autobot-ai-f8 while sweeping, including the base-vs-branch attribution check. The widening from one anchor to four is this commit.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 5d8aa3b3-7a76-46a7-9faa-40f5bcf85364
📒 Files selected for processing (14)
CLAUDE.mdautobot-backend/a2a/pii_pipeline.pyautobot-backend/chat_workflow/cot_events.pyautobot-backend/llm_shared/credential_redaction.pyautobot-backend/llm_shared/tests/test_credential_redaction.pyautobot-backend/security/chat_message_safety.pyautobot-backend/services/config_revision_service.pyautobot_shared/secret_redaction.pyautobot_shared/secret_redaction_test.pyautobot_shared/security/redaction.pydocs/developer/REDACTION_BOUNDARY.mddocs/developer/THREAT_MODEL.mdrepo_tests/audio_extension_allowlist_test.pyrepo_tests/redaction_concept_census_test.py
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
| _AUTHORIZATION_KEYS = frozenset({"bearer", "auth", "authorization"}) | ||
|
|
||
| SENSITIVE_KEYS = frozenset(CREDENTIAL_SUFFIXES) | _AUTHORIZATION_KEYS |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '35,120p' autobot-backend/llm_shared/credential_redaction.py
sed -n '138,205p' autobot-backend/llm_shared/tests/test_credential_redaction.py
sed -n '65,110p' autobot_shared/secret_redaction.pyRepository: mrveiss/AutoBot-AI
Length of output: 7692
Separate legacy fragments from suffix matching.
redact_dict currently uses substring matching for every SENSITIVE_KEYS entry. Since key is now in CREDENTIAL_SUFFIXES, ordinary keys such as keyboard, keyframe, and monkey can have their values redacted. Apply suffix matching only to newly introduced suffixes. Retain the legacy literal matching so auth_header, apikey, and authtoken remain covered.
Suggested fix
+_LEGACY_SENSITIVE_KEYS = frozenset(
+ {"apikey", "token", "bearer", "password", "secret", "credential", "auth", "authorization"}
+)
+_SUFFIX_SENSITIVE_KEYS = frozenset(CREDENTIAL_SUFFIXES) - _LEGACY_SENSITIVE_KEYS
+
_AUTHORIZATION_KEYS = frozenset({"bearer", "auth", "authorization"})
-SENSITIVE_KEYS = frozenset(CREDENTIAL_SUFFIXES) | _AUTHORIZATION_KEYS
+SENSITIVE_KEYS = (
+ frozenset(CREDENTIAL_SUFFIXES)
+ | _LEGACY_SENSITIVE_KEYS
+ | _AUTHORIZATION_KEYS
+)
...
- key_lower = key.lower().replace("_", "").replace("-", "")
- if any(sensitive in key_lower for sensitive in SENSITIVE_KEYS):
+ key_lower = key.lower()
+ key_normalized = key_lower.replace("_", "").replace("-", "")
+ if (
+ any(sensitive in key_normalized for sensitive in _LEGACY_SENSITIVE_KEYS | _AUTHORIZATION_KEYS)
+ or any(
+ key_lower == sensitive or key_lower.endswith(f"_{sensitive}")
+ for sensitive in _SUFFIX_SENSITIVE_KEYS
+ )
+ ):Also add regression cases for keyboard, keyframe, and monkey, while retaining coverage for apikey and authtoken.
| * a new **credential** pattern or noun → `secret_redaction.py` (reaches | ||
| `credential_redaction` and every `redact_content` caller automatically) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Inspect the vocabulary and free-text pattern declarations without running repository code.
rg -n -C 5 'CREDENTIAL_SUFFIXES|_CREDENTIAL_PHRASE_RE|_KNOWN_PREFIX_RE|def scan_content_for_credentials|def redact_content' autobot_shared/secret_redaction.pyRepository: mrveiss/AutoBot-AI
Length of output: 5178
Separate field-name and free-text redaction updates.
CREDENTIAL_SUFFIXES feeds is_credential_field, but scan_content_for_credentials uses fixed regular expressions, including _CREDENTIAL_PHRASE_RE. A new noun added only to CREDENTIAL_SUFFIXES can therefore reach credential_redaction without making redact_content recognise that noun in free text. Update the guidance to distinguish these changes.
Suggested documentation fix
-* a new **credential** pattern or noun → `secret_redaction.py` (reaches
- `credential_redaction` and every `redact_content` caller automatically)
+* a new **credential field-name** pattern or noun → `secret_redaction.py`
+ (reaches `credential_redaction` automatically); update the free-text
+ patterns separately when `redact_content` must recognise the noun📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * a new **credential** pattern or noun → `secret_redaction.py` (reaches | |
| `credential_redaction` and every `redact_content` caller automatically) | |
| * a new **credential field-name** pattern or noun → `secret_redaction.py` | |
| (reaches `credential_redaction` automatically); update the free-text | |
| patterns separately when `redact_content` must recognise the noun |
| A detector added anywhere else is an eighth implementation, and | ||
| `repo_tests/redaction_concept_census_test.py` fails on it. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Limit the census guarantee to new detector-owning modules.
The census definition on Lines 21-25 counts modules, not detectors. Adding a detector to an existing counted module does not create an eighth module or necessarily fail the census. State that limit here so reviewers do not treat a passing census as proof that every new detector was placed in the correct module.
| #: | ||
| #: Measured on THIS rebased tree, twice and by two methods, not carried from | ||
| #: another session's message: `git ls-files "*.py"` = 6151 and | ||
| #: `git ls-tree -r HEAD` = 6151 (origin/main = 6150 both ways). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '90,170p' repo_tests/audio_extension_allowlist_test.py
git ls-tree -r HEAD --name-only | wc -l
git ls-tree -r HEAD --name-only -- '*.py' | wc -lRepository: mrveiss/AutoBot-AI
Length of output: 4316
🏁 Script executed:
set -eu
printf '%s\n' '--- current comment and declaration ---'
sed -n '126,160p' repo_tests/audio_extension_allowlist_test.py
printf '%s\n' '--- correctly scoped tree counts ---'
printf 'all paths: '
git ls-tree -r --name-only HEAD | wc -l
printf 'python paths: '
git ls-tree -r --name-only HEAD -- '*.py' | wc -l
printf '%s\n' '--- changed lines against the stated PR base ---'
git diff --unified=3 31310392dfe359bcdd4738ef97953407781e11ef -- repo_tests/audio_extension_allowlist_test.py | sed -n '/6151/,+20p'Repository: mrveiss/AutoBot-AI
Length of output: 3219
🏁 Script executed:
set -eu
printf '%s\n' '--- representative HEAD tree paths ---'
git ls-tree -r --name-only HEAD | sed -n '1,5p'
printf '%s\n' '--- representative tracked Python paths ---'
git ls-files '*.py' | sed -n '1,5p'
printf '%s\n' '--- Python count from unfiltered HEAD tree ---'
git ls-tree -r --name-only HEAD | grep -E '\.py$' | wc -l
printf '%s\n' '--- pathspec variants ---'
printf 'HEAD then pathspec: '
git ls-tree -r --name-only HEAD -- '*.py' | wc -l
printf 'pathspec before HEAD: '
git ls-tree -r --name-only -- '*.py' HEAD | wc -l
printf 'globstar pathspec: '
git ls-tree -r --name-only HEAD -- '**/*.py' | wc -lRepository: mrveiss/AutoBot-AI
Length of output: 751
Filter the ls-tree cross-check to Python files.
git ls-tree -r HEAD lists 10,761 paths. It does not support the stated count of 6,151 Python files. Record the filtered command instead.
Suggested fix
-#: `git ls-files "*.py"` = 6151 and
-#: `git ls-tree -r HEAD` = 6151 (origin/main = 6150 both ways).
+#: `git ls-files "*.py"` = 6151 and
+#: `git ls-tree -r --name-only HEAD | grep -E '\.py$' | wc -l` = 6151
+#: (origin/main = 6150 both ways).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #: `git ls-tree -r HEAD` = 6151 (origin/main = 6150 both ways). | |
| #: `git ls-tree -r --name-only HEAD | grep -E '\.py$' | wc -l` = 6151 | |
| #: (origin/main = 6150 both ways). |
| single literal first argument. | ||
| """ | ||
| for call in ast.walk(node): | ||
| if not (isinstance(call, ast.Call) and isinstance(call.func, ast.Attribute) and call.func.attr == "compile"): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Recognise directly imported regex compilers.
from re import compile as regex_compile produces an ast.Name call, not the ast.Attribute required here. A module that defines PASSWORD_PATTERN = regex_compile(r"password=\S+") and redact_values is reported as having no detector. Resolve imports of re.compile, including aliases, before excluding name-style calls. Add a fixture for this import form.
| continue | ||
| for arg in call.args: | ||
| for const in ast.walk(arg): | ||
| if isinstance(const, ast.Constant) and isinstance(const.value, str) and _mentions_secret(const.value): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Detect PII patterns named outside their regex text.
_compiles_secret_pattern checks the regex text but not its assignment name. A new module with EMAIL_PATTERN = re.compile(r"[\w.+-]+@[\w.-]+\.[A-Za-z]{2,}") and redact_email therefore escapes the census. The regex contains none of _SECRET_NOUNS. Check detector names or equivalent PII evidence, and add an email-only fixture. The documented census includes PII implementations.
| if not isinstance(coll, (ast.Tuple, ast.List, ast.Set)): | ||
| continue | ||
| strings = [e.value for e in coll.elts if isinstance(e, ast.Constant) and isinstance(e.value, str)] | ||
| if len(strings) >= 3 and sum(1 for s in strings if _mentions_secret(s)) >= 3: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Include dictionary-based secret vocabularies.
A module can define SENSITIVE_FIELDS = {"password": True, "token": True, "api_key": True} and use it in redact_mapping. This walker excludes ast.Dict, so the new implementation passes the census. Inspect string keys in dictionary literals and add positive and negative dictionary fixtures.
| for node in tree.body: | ||
| if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)): | ||
| lowered = node.name.lower() | ||
| if any(verb in lowered for verb in _REDACTION_VERBS): | ||
| return True |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Recognise redaction methods on classes with other names.
A module with a secret vocabulary and class CredentialFilter exposing def redact(self, value) defines a redaction API. _defines_redaction_api checks only the top-level class name, so the module passes the census. Inspect methods on top-level classes and add a fixture for this class shape.
|
|
||
| # A detector vocabulary with no redaction entry point — e.g. an audit rule list. | ||
| _PLANTED_DETECTOR_ONLY = """ | ||
| INTERESTING = ("password", "token") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '160,255p' repo_tests/redaction_concept_census_test.py
sed -n '330,390p' repo_tests/redaction_concept_census_test.py
rg -n '_declares_secret_vocabulary|_declares_secret_enum|_PLANTED_DETECTOR_ONLY|contrast pair|Every detector needs' repo_tests CLAUDE.mdRepository: mrveiss/AutoBot-AI
Length of output: 8945
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- relevant files ---'
rg -l '_declares_secret_vocabulary|_declares_secret_enum|is_redaction_implementation|PLANTED_.*(DETECTOR|ENUM|COLLECTION)|secret vocabulary|secret/PII|detector' repo_tests --glob '*.py' | sort
printf '%s\n' '--- relevant symbols and assertions ---'
rg -n '_declares_secret_vocabulary|_declares_secret_enum|is_redaction_implementation|PLANTED_DETECTOR_ONLY|PLANTED_.*(ENUM|COLLECTION)|expected|assert .*redaction|assert .*detector|secret vocabulary|secret/PII' repo_tests --glob '*.py'
printf '%s\n' '--- current module outline and focused source ---'
ast-grep outline repo_tests/redaction_concept_census_test.py
sed -n '1,170p' repo_tests/redaction_concept_census_test.py
sed -n '255,390p' repo_tests/redaction_concept_census_test.py
printf '%s\n' '--- base/head status ---'
git show --stat --oneline f58793659b2fa282b7deac8e8bbec4ea09486a31 -- repo_tests/redaction_concept_census_test.py
git diff --stat 31310392dfe359bcdd4738ef97953407781e11ef f58793659b2fa282b7deac8e8bbec4ea09486a31 -- repo_tests/redaction_concept_census_test.pyRepository: mrveiss/AutoBot-AI
Length of output: 42043
Make the detector-only fixture exercise the collection detector.
_PLANTED_DETECTOR_ONLY contains only two secret strings, but _declares_secret_vocabulary requires at least three. Add a third secret noun and assert _declares_secret_vocabulary(ast.parse(_PLANTED_DETECTOR_ONLY)) is True before asserting that is_redaction_implementation returns False. Add separate positive and negative contrast fixtures for the collection and enum detector paths.
…rebased tree (#17331, #17332) CI failed shard 12/12 on reach_declarations_test for audio-extension-allowlist and hooks-path-override. Both floors sat 2 below their lower bound, on a branch whose own contribution is 2 counting files -- main had spent the rest of the allowance before this branch existed. #17142's shape, for the sixteenth time on one of these. Measured on THIS tree after rebasing onto current main, not copied from any other branch or message: audio-extension-allowlist population 6152 completed 5765 verify_floor needs floor >= 6152-(400+300) = 5452 completed needs floor <= 5765 window [5452, 5765]; was 5450; now 5700 hooks-path-override population 6939 completed 6938 verify_floor needs floor >= 6939-(400+1) = 6538 completed needs floor <= 6938 window [6538, 6938]; was 6536; now 6800 A floor has TWO bounds and only the first has been checked at every previous re-pin. `verify_floor` fails when population-floor exceeds growth+skips; `completed` fails when the count the guard actually FINISHED processing drops below the floor. `completed` is smaller than population and shrinks as repo_tests grows, so a floor pinned from population alone can pass one check and fail the other. Every previous re-pin took the lower bound as the value, which is why there have been fifteen: at the minimum, the next file main adds is another red. These take the middle of the window -- 452 of 700 allowance and 65 files of completed headroom for audio, 139 and 138 for hooks. Both are true claims: 5765 and 6938 files really are opened. 5700 is the same value e5 pins in #17335, deliberately rather than inventing a third independent number for one measurement. This does not fix #17142. A 400-file growth allowance is 5.8% of a 6939-file tree and this tree spends it in under a week; the allowance is the thing that is wrong, not the floors. Sixteenth data point. Verified: 209 tests pass across reach_declarations_test, audio_extension_allowlist_test and hooks_path_override_15961_test on this tree. Local passes are not CI (#17144) -- this red came from CI and the re-measurement is what answers it. Refs #17142
0c9174f to
d26c9b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: bfe65c7d-60c6-4b6a-841b-51f6b724b531
📒 Files selected for processing (11)
autobot-backend/a2a/pii_pipeline.pyautobot-backend/chat_workflow/cot_events.pyautobot-backend/llm_shared/credential_redaction.pyautobot-backend/security/chat_message_safety.pyautobot-backend/services/config_revision_service.pyautobot_shared/secret_redaction.pyautobot_shared/secret_redaction_test.pyautobot_shared/security/redaction.pyrepo_tests/audio_extension_allowlist_test.pyrepo_tests/hooks_path_override_15961_test.pyrepo_tests/redaction_concept_census_test.py
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
| for node in tree.body: | ||
| if isinstance(node, (ast.Assign, ast.AnnAssign)): | ||
| if node.value is not None and ( | ||
| _compiles_secret_pattern(node.value) or _declares_secret_vocabulary(node.value) | ||
| ): | ||
| return True | ||
| elif isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): | ||
| if _compiles_secret_pattern(node) or _declares_secret_vocabulary(node): | ||
| return True | ||
| return _declares_secret_enum(tree) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Scan class bodies for detectors, not only module-level statements.
_declares_own_detector checks only module-level Assign, AnnAssign, FunctionDef and AsyncFunctionDef nodes. For a ClassDef, it runs only _declares_secret_enum. That check accepts plain string constants only. It does not find compiled patterns or collection literals.
A class-based redactor therefore passes the census. Example:
import re
class SecretRedactor:
_PATTERNS = [re.compile(r"password=\S+"), re.compile(r"bearer\s+\S+")]
def apply(self, text):
..._defines_redaction_api matches the class name SecretRedactor. _declares_own_detector returns False, because the class-body assignment and the method bodies are never scanned. A class that holds its patterns is a common shape for a new redactor. This gap lets an eighth implementation land without any test failure.
The same gap applies to module-level if/try blocks, for example a try: import … fallback that defines patterns.
Scan ClassDef bodies and their methods with the same two checks. Add a positive fixture for the class-held pattern. Also add a negative fixture: a class that holds a non-secret regex. The path instruction requires a contrast pair for every detector.
🐛 Proposed fix
for node in tree.body:
if isinstance(node, (ast.Assign, ast.AnnAssign)):
if node.value is not None and (
_compiles_secret_pattern(node.value) or _declares_secret_vocabulary(node.value)
):
return True
- elif isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
+ elif isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef, ast.If, ast.Try)):
if _compiles_secret_pattern(node) or _declares_secret_vocabulary(node):
return True
return _declares_secret_enum(tree)As per path instructions: "Every detector needs a contrast pair: a fixture that SHOULD trip it and one that should not."
🧰 Tools
🪛 Ruff (0.16.6)
[warning] 229-230: Use a single if statement instead of nested if statements
(SIM102)
Source: Path instructions
59268da to
ee2668a
Compare
Review: approveReviewed at head 1. if any(sensitive in key_lower for sensitive in SENSITIVE_KEYS):so 2. The The census being MEASURED rather than transcribed is the part worth keeping. #16688 and #17312 both said four; the rule found seven, and the three nobody had counted ( One note, not blocking: Overlap check: this touches |
…census (#16688) The issue asks which of four parallel redaction implementations is canonical. Measuring first changed the question twice. There are seven, not four. A census rule run over all 3,392 tracked production sources finds three more modules that independently decide what a secret looks like and that no issue had counted: chat_workflow/cot_events.py (_SENSITIVE_KEY_FRAGMENTS, 13 nouns), llc/services/portability.py (_SECRET_LIKE_KEYS, 13) and services/config_revision_service.py (_SECRET_SUBSTRINGS, 5). A fifth, autobot_shared/security/redaction.py, also declares itself the one canonical redactor (#12242). And one of the four is not an implementation at all. security/chat_message_safety.py declares no detector; it composes pii_pipeline and the injection detector. It is the shape the other seven should converge on, so the guard must not count it. There is therefore no single canonical redactor and there should not be one. What was missing is the boundary: one module per SHAPE of the problem. docs/developer/REDACTION_BOUNDARY.md states it, with a which-one-do-I-call table, and each of the modules points at it -- except portability.py, which sits on its grandfathered 936-line ceiling and would be pushed over by the note. Two behaviour changes, both strictly widening: - secret_redaction's JWT pattern required "eyJ" on the payload as well as the header, making it narrower than a2a/pii_pipeline.py's. A payload of {} encodes to e30 and was matched there and missed here -- in the module llm_shared.credential_redaction delegates log redaction to. Anchored on the header alone, as pii_pipeline already was. - credential_redaction.SENSITIVE_KEYS no longer hand-copies the credential nouns; it derives from secret_redaction.CREDENTIAL_SUFFIXES. Deliberately a UNION: that vocabulary carries no authorization terms, so a straight replacement would have stopped redacting Authorization, auth_header and x_auth. A redaction merge that narrows is a silent under-redaction, which is why both directions are now tested. repo_tests/redaction_concept_census_test.py pins the seven. It counts implementations of a concept rather than duplicated text, because jscpd finds zero clone lines across these modules -- they are forks, and a fork shares no 8-line run, so MAX_DUP_LINES neither rises when an eighth lands nor falls when one is collapsed (#17312). The rule is proven on a planted fork that shares no text with any real module, and on the two shapes that must NOT count: pure composition, and a vocabulary with no redaction entry point. The two autobot_shared redactors disagree on 7 of 10 sampled field names -- suffix matching vs substring, each a recorded ruling in its own file. Merging them would reverse one, so this documents the split and leaves it open. Refs #16688, #17312
…les size (#16688) The pre-push suite caught the previous commit's fixture asserting the wrong thing. `{}` base64url-encodes to `e30`, three characters -- below the `{8,}` this scanner requires for a segment and below the `{10,}` a2a/pii_pipeline.py requires, so NEITHER rule matched it. The test passed its "something matched" sibling only because the high-entropy rule claimed the span; the precise assertion (`pattern == "jwt"`) is what surfaced it, which is the argument for writing it that way. The divergence itself is real; the example was not. A payload starts "eyJ" only when its JSON begins exactly '{"'. A serializer emitting a space after the brace gives "eyAi" and one padding before it gives "IHsi" -- both long enough, both matched by pii_pipeline, both missed by this scanner before the widening. Fixture replaced with such a token, decoding to { "sub": "1234567890", "name": "A" }, verified matched by the new rule and unmatched by the old one. The source comment carried the same wrong example and is corrected with it. Refs #16688
…om on main (#16688, #16724) origin/main sits at EXACTLY zero headroom on this floor: population 6150, floor 5450, and 6150 - 5450 = 700 = skips 300 + growth 400. Any branch adding one tracked .py file turns it red. This branch adds one test file, measures 6151, slack 701 -- one over. The floor is stale, not the branch. Measured on this rebased tree, two methods, not carried from another session: git ls-files "*.py" = 6151 (origin/main 6150) git ls-tree -r HEAD = 6151 (origin/main 6150) Re-pinned to 5700, NOT the 5751 that population - growth gives. The binding constraint is completed(), not verify_floor(): the guard's loop continues past the canonical file and everything under repo_tests/ before reading anything, so completed = 6151 - 386 - 1 = 5764. A 5751 floor leaves 13 files of margin against a number that FALLS as repo_tests/ grows -- and this branch grows it. 5700 leaves 64, comparable to the 47 the 5450 pin carried against its measured completed of 5497, which held from #16702 until now. verify_floor: 6151 - 5700 = 451 <= 300 + 400 = 700 ok completed: 5764 >= 5700, margin 64 ok skips and growth unchanged: neither is short, the floor was stale. #16724 already tracks re-measuring this from CI's own log rather than from a tree walk. Checked for the duplicated `floor=` keyword a conflict resolution produced elsewhere today: one occurrence. Also measured, not changed: hooks-path-override is green here but at ZERO headroom (population 6937 incl. 31 extensionless shell scripts, floor 6536, slack 401, allowance 401); origin/main has exactly 1. The next tracked .sh/.py/.yml/.yaml file anywhere turns it red. Left alone deliberately -- it passes, it is a hub file four sessions are touching tonight, and #17142 already tracks it. Refs #16688, #16724, #17142
…6688) python-suite shards 5, 6, 8 and 10 were red on threat_model_anchors_resolve_test. The cause is in this branch's own diff, not in base: the boundary pointer added to autobot_shared/security/redaction.py's module docstring inserted five lines above every definition in the file, and the citations in docs/developer/THREAT_MODEL.md still pointed at the old numbers. Base agrees with itself, checked before assuming: git show origin/main:autobot_shared/security/redaction.py \ | grep -n "def redact_provider_error" -> 140 git show origin/main:docs/developer/THREAT_MODEL.md \ | grep -n redact_provider_error -> 116: ... (:140) FOUR anchors moved, not one. The failing test names only the anchor its shard reached first, so fixing that one would have left three and produced another red run. Every citation in the document was re-resolved with the test's own rule (AST definition lookup, not a line regex) rather than only the reported case: redact_text :97 -> :102 redact_mapping :109 -> :114 redact_cloud_identifiers :126 -> :131 redact_provider_error :140 -> :145 The two loader.py anchors that a definition-only check flags are NOT broken: import_module and spec_from_file_location are stdlib CALL sites, and the test has an explicit branch for anchors with no definition in the file. A first pass listed them as failures; that was the probe being wrong, not the doc. The other seven modules this branch adds a docstring pointer to are unaffected -- the sweep covered all 38 anchors, not just redaction.py, and they are either unanchored or anchored above the insertion point. Verified: repo_tests/threat_model_anchors_resolve_test.py -- 41 passed. THREAT_MODEL.md stays at 140 lines, which is its own ceiling (test_document_exists_and_is_short_enough_to_read_every_review asserts <= 140); this change edits line numbers in place and adds none. Root-caused by autobot-ai-f8 while sweeping, including the base-vs-branch attribution check; the widening from one anchor to four is this commit. Refs #16688
…re-empt a zero-headroom floor (#16688) Both changes are pre-emptive: this branch was GREEN when they were made. It was green against a base that has since moved, which is the whole point. THE CENSUS GUARD WAS A SECOND FORK OF THE ENUMERATION. redaction_concept_census_test shelled out to `git ls-files` for itself, exactly as enum_hand_copy_guard did on the #14881 branch. one_git_enumeration_15926_test::test_the_direct_invocation_count_only_shrinks caught that one in CI; rebasing this branch onto origin/main 3f0d4b3 reproduces it here -- 34 invocations against a MAX of 33 -- because main reached 33 after this branch's last run. Found by predicting it and checking, not by a red. A census guard forking the repo's enumeration, in a PR whose subject is forks, is the joke writing itself. Routed through tools.lint._scan_helpers.tracked_paths, whose `exclude` becomes a git `:(exclude)` pathspec so git does the matching -- rather than the second Python-side matcher this had, which is how #15510 fired an exclusion against absolute paths and matched every file. subprocess and scrubbed_git_env are no longer imported. HOOKS-PATH-OVERRIDE 6536 -> 6800 WHILE PASSING. Measured on this rebased tree: population 6937 (glob hits for *.sh/*.py/*.yml/*.yaml plus 31 extensionless files whose shebang says shell), completed 6936, slack 401 against growth 400 + skips 1 = exactly 401. That is not "passing", it is "passing until the next file anyone lands", and four sessions moved main in the hour this branch sat in review. The cost of pre-empting is one line on a hub file three other PRs already change to the same value; the cost of not is an hour of CI and a round trip on a predictable red. 6800 is mid-window of [6536, 6936], not the minimum -- fifteen previous re-pins took the bottom, which is why there were fifteen. It is deliberately the same value as autobot-ai-58's #17338 and this session's #17342 and #17346: one measurement should not produce four numbers, and identical lines cannot conflict. First to land wins, the rest adopt on rebase. The other three floors were measured here and left alone, because this branch does not move their populations: audio-extension-allowlist 6151/5764 against the 5700 pinned earlier on this branch (margin 64), prompt-injection-detector-strict-mode 3256 (no new backend non-test module), import-hermeticity 662 (no new module under api/ or autobot_shared/). Two of those sit at exactly zero headroom on main and will need re-pinning by whichever branch next adds to their roots -- see #17142. Verified on this tree: one_git_enumeration + redaction_concept_census + threat_model_anchors + prompt_injection_detector_strict_mode + reach_declarations ... 274 passed reach_declarations_test is run with the prompt-injection module in the same session on purpose: its declaration reaches the sweep only when something else has imported it, so repo_tests/ alone silently checks 35 of 36 floors. Refs #16688, #15926, #17142
…ts own (#16688) Empty commit: the change is in the rebase conflict resolutions, but the commits carrying them still SAY 6800 and 5700 in their messages because that is what they proposed when written. Stating it once beats rewriting four messages. Rebased onto origin/main 19006b3. Both floors adopted, first-to-land wins: hooks-path-override proposed 6800 -> adopted main's 6736 (#17318) audio-extension-allowlist proposed 5700 -> adopted main's 5752 (#17317) Re-derived on this tree rather than taken on trust: audio pop 6153 completed 5766 pin 5752 slack 401/700 margin 14 hooks pop 6939 completed 6938 pin 6736 slack 203/401 margin 202 prompt-inj pop 3256 pin 2956 slack 300/300 ZERO headroom import-herm pop 662 pin 602 slack 60/60 ZERO headroom The pre-emptive 6800 on this branch was still the right call when made -- it sat at slack 401 against an allowance of 401, one file from red, while passing -- but main moved first and one measurement should not produce four numbers. FOR #17142, the measurement that took three branches to produce. audio's pin is population - growth, the GAP bound, while the bound that fires on this guard is completed(): its loop continues past the canonical file and all of repo_tests/ before reading anything, so completed sits ~387 below population and falls every time anyone adds a guard. Main pinned 5752 with an apparent 402 of headroom. The real figure, measured on this session's three branches: issue-13049-lazy-import margin 16 issue-14881-enum-hand-copies margin 15 issue-16688-fork-collapse margin 14 Three ordinary branches, each adding one or two repo_tests guards, took it from 16 to 14. That is the allowance being wrong, not the floors, and it is why this guard has a re-pin history at all. Verified on this tree: prompt_injection_detector_strict_mode + reach_declarations + one_git_enumeration + redaction_concept_census + threat_model_anchors ................................... 274 passed Refs #16688, #17142, #17317, #17318
…arried (#16688, #15928) reach_floor_migration_test::test_no_new_hand_rolled_floor_remains_unmigrated failed: redaction_concept_census_test declared `_MIN_SOURCE_FILES` as a hand-rolled reach-sized floor. #15928's rule is that a floor >=100 which binds a tree sweep goes through _reach.declare(), because a number chosen by feel decays silently. IT HAD ALREADY DECAYED, IN THE FOUR HOURS SINCE I WROTE IT. The constant was 1500. Re-measured with a throwaway script, as #15928 requires rather than carrying the old value across: the real population is 3,393 tracked production .py files (excluding .worktrees/, docs/, repo_tests/ and test files). 1500 sat 56% BELOW the tree it claimed to bind -- it would have passed over a sweep that had lost more than half the codebase and still reported "no new redaction implementations". Which is the argument #15928 makes, demonstrated on a guard whose own docstring is about not confusing "nothing found" with "did not look". I wrote the number by feel in a file about measurement discipline. declare("redaction-concept-census", floor=3093, growth=300, what="tracked production python files") growth=300 matches the sibling declaration in prompt_injection_detector_strict_mode_test over a population of similar size. Also wired REACH.completed(parsed), which the hand-rolled version had no way to express: the floor now binds what the census actually PARSED, not merely what it listed. Without it the sweep could enumerate 3,393 files, fail to parse 3,000 of them, and report the same clean as a tree with nothing in it -- the exact distinction this guard exists to make. discover() takes a root and returns [] on EmptyEnumeration rather than letting it escape: reach_declarations_test drives every declaration against an empty repository and demands ReachFloorError specifically, so letting EmptyEnumeration through means the declaration fails loudly while saying nothing about whether its floor binds. Found by autobot-ai-f8 reading the shard-8 log on the sibling #17342 branch, whose enum_hand_copy_guard_test.py carries the identical defect. Confirmed here by running the migration guard rather than assuming the sibling's diagnosis transferred. Verified on this tree: reach_floor_migration + redaction_concept_census + reach_declarations ...................................... 206 passed Refs #16688, #15928, #17312
…he minimum (#16688, #17142) Rebased onto origin/main e8c7bf9. Main's floors adopted where they hold; the one this branch created did not. audio-extension-allowlist adopted main's 5757 (NOT 5752 -- superseded) hooks-path-override adopted main's 6736 prompt-injection-... adopted main's 2958, green here import-hermeticity adopted main's 604, green here redaction-concept-census 3093 -> 3247, MINE, and it was red THE CENSUS FLOOR WAS MY OWN MINIMUM PIN, AND IT IS THE SECOND ONE TONIGHT. I created that declaration earlier in this branch, migrating a hand-rolled floor under #15928, and pinned it at 3093 -- population 3393 minus growth 300, which is the MINIMUM OF THE WINDOW. It went red on this rebase: population reached 3397, slack 304 against an allowance of 300. Four files. The sibling declaration on the #14881 branch (enum-hand-copy-census, pinned 5755 at population 6155) went red the same way at 6165. Ten files. Both were committed AFTER I filed the comment on #17142 arguing that `population - growth` is not a floor but a snapshot with zero tolerance by construction. So: wrote the argument, made the mistake twice, noticed neither time until reach_declarations_test failed. That is the case for the check being mechanical rather than a review item, and it is self-reported on #17142 with timestamps because "people who understand the rule will follow it" is the objection it removes. Re-measured on this tree rather than adjusted: 3,397 tracked production .py files using the guard's own filter. Window [3097, 3397]; 3247 is mid-window and buys ~150 files of headroom instead of four. Measured after the re-pin: audio pop 6163 completed 5776 pin 5757 slack 406/700 margin 19 hooks pop 6949 pin 6736 slack 213/401 prompt-inj pop 3258 pin 2958 slack 300/300 ZERO headroom import-herm pop 664 pin 604 slack 60/60 ZERO headroom census pop 3397 pin 3247 slack 150/300 The two at zero headroom are adopted unchanged because this branch adds nothing to their roots. #17346 carries prompt-injection at 3100 and #17342 carries import-hermeticity at 633 -- those are the branches that do. audio is 5757, not the 5752 quoted to me during review: #17317's value was superseded while that message was in flight. Read by AST at e8c7bf9 rather than taken from the message, which is the same discipline this session has been applying all night -- a value quoted by name is already a snapshot. Verified on this tree: reach_declarations + reach_floor_migration + redaction_concept_census + threat_model_anchors .......... 247 passed Refs #16688, #17142, #15928
… not ship (#16688) Third instance of the same defect, found by checking rather than by review. autobot-ai-f8 caught it on #17342 (fixed as acbfbe3) and #17346 (67f977f); this branch is the third of the three that re-pinned this declaration tonight, so it was checked before anyone had to report it. git diff origin/main...HEAD -- repo_tests/audio_extension_allowlist_test.py \ | grep -c 'floor=' -> 0 the file ships -> floor=5757 the block this branch ADDED said -> "Ratcheted 5450 -> 5700 (#16688)" An earlier revision of this branch did propose 5700 and the block described it correctly then. #17317 landed 5752, later 5757, and the rebase adopted main's value; the `floor=` hunk was resolved and the prose, living in a different commit, survived as a pure addition. The branch ended up ADDING a precise narrative about a change it no longer makes. Deleted rather than corrected, matching the #17346 fix: main's own comment sits immediately below explaining 5757, so a correction block would leave two voices on one floor -- which is the defect, not its remedy. The diff no longer touches this file. Note this differs from the #17342 fix, which rewrote its block instead. That was the right call there and the wrong one to repeat: at the time I had not seen that main already carried a true account a few lines down, so the correction became a second voice too. It is deleted there as well. WHY THIS FILE AND NO OTHER, which is f8's observation: three branches re-pinned this one declaration tonight. A floor whose comment block is append-only across concurrent branches accumulates contradictory narratives even when every individual re-pin is correct, because the rebase resolves the value and nothing resolves the sentence. The durable record of the mechanism belongs in commit messages and on #17142 -- not as competing prose in a hub file. Refs #16688, #17142, #17356
…ments from prose (#16688) Two distinct problems, both found by grepping this branch's own diff for prose naming a measurement -- which is the check autobot-ai-f8 asked for after the orphaned-narrative round. 1. THIS BRANCH WAS SHIPPING TWO STALE FLOORS AND TWO SUPERSEDED COPIES OF MAIN'S OWN COMMENTS. audio-extension-allowlist carried 5757 with main's older narrative while main is at 5764 with a rewritten one; prompt-injection-detector-strict-mode carried 2958's text while main is at 2960. Neither needs a branch-specific value -- measured on this tree, main's floors pass here -- so both files are now byte-identical to origin/main. That is worse than an orphaned note of my own: a superseded copy of ANOTHER branch's comment silently misattributes a stale account to them, and reads as authoritative because it is written in their voice. 2. EVERY MEASUREMENT IS OUT OF THE PROSE THIS BRANCH ADDS. The census declaration's migration note, its re-pin note, the hooks adoption note and one line of REDACTION_BOUNDARY.md all cited a population or a slack figure. Each was correct when written and stale one rebase later. The last one is the instructive case: "enumerate 3,393 files, fail to parse 3,000 of them" was an ILLUSTRATION, not a claim -- and an illustration carrying a real population goes stale exactly like a measurement while reading as more authoritative for being precise. Now "enumerate the whole tree, fail to parse most of it", which stays true. Prose that cites no measurement cannot go stale. The arithmetic lives in commit messages, which cannot drift from the tree they describe because they are not in it. Verified on this tree: reach_declarations + reach_floor_migration + redaction_concept_census ................................ 206 passed Refs #16688, #17142, #17356
ee2668a to
df4273e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 032323a2-cbb2-43b0-863b-87c8dc1a83b6
📒 Files selected for processing (10)
autobot-backend/a2a/pii_pipeline.pyautobot-backend/chat_workflow/cot_events.pyautobot-backend/llm_shared/credential_redaction.pyautobot-backend/security/chat_message_safety.pyautobot-backend/services/config_revision_service.pyautobot_shared/secret_redaction.pyautobot_shared/secret_redaction_test.pyautobot_shared/security/redaction.pydocs/developer/REDACTION_BOUNDARY.mdrepo_tests/redaction_concept_census_test.py
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
| ``llm_shared/credential_redaction.py`` IS in the census, because it still | ||
| declares ``API_KEY_PATTERNS`` of its own on top of the shared vocabulary it now | ||
| derives ``SENSITIVE_KEYS`` from (#16688). If those patterns are folded into | ||
| ``secret_redaction`` later, drop it from ``_CENSUS`` and the count falls to 3. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the projected census count.
_CENSUS has seven entries. Removing credential_redaction.py would leave six, not three. Correct the count so the consolidation guidance matches the pinned census.
| elif isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): | ||
| if _compiles_secret_pattern(node) or _declares_secret_vocabulary(node): |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Detect inline regex redactors.
If a new module defines redact_text using re.sub(r"password=\S+", "[redacted]", text), this branch reports no detector. The regex is not compiled, and the function declares no collection vocabulary. The new implementation therefore passes the census. Recognise inline regex calls and add one fixture that trips this detector and one that does not.
As per path instructions: “Every detector needs a contrast pair: a fixture that SHOULD trip it and one that should not.”
🧰 Tools
🪛 Ruff (0.16.6)
[warning] 229-230: Use a single if statement instead of nested if statements
(SIM102)
Source: Path instructions
Review currency: the approval was given at a different head, and still applies — checked rather than assumedThis PR was reviewed and approved at Both heads' own patches, each taken against its own merge base (a two-dot diff between the two tips would show One file left the patch and nothing else changed: Every one of the remaining 13 files is byte-identical between the two patches — compared hunk-by-hunk, not by counting files: Why that file left, and why its departure is the reviewer's own recommendation
Resolving that conflict toward The approval therefore still covers what would merge. Not because heads are close, but because the delta is one removal in the direction the review asked for. Floors re-verified on the current headMeasured, not carried:
That last one is tight and worth naming: Outstanding: |
1 participant
Thinking Path
#16688 asks which of four parallel redaction implementations is canonical. I measured before choosing, and the measurement changed the question twice.
There are seven, not four. A census rule run over all 3,392 tracked production sources found three modules that independently decide what a secret looks like and that no issue had counted:
chat_workflow/cot_events.py_SENSITIVE_KEY_FRAGMENTSllc/services/portability.py_SECRET_LIKE_KEYSservices/config_revision_service.py_SECRET_SUBSTRINGSA fourth uncounted one,
autobot_shared/security/redaction.py, also declares itself the one canonical redactor (#12242, "a redactor is a security control, so there is exactly ONE implementation of it"). Two modules inautobot_shared/each claiming to be the single canonical redactor is the fork problem in its sharpest form.And one of the original four is not an implementation at all.
security/chat_message_safety.pydeclares no detector — it composespii_pipeline.scrub_outboundand the shared injection detector. It is the shape the other seven should converge on, so the guard must not count it. #17312's table scored it at "5 sensitive categories" by counting keyword mentions in its prose; that file itself flags the method as indicative rather than proof, and reading the source is where it comes apart.So there is no single canonical redactor, and there should not be one. What was missing was never a winner — it was the boundary: one module per shape of the problem. Six independent answers to one question ("is this key name sensitive?") with word lists of 19, 13, 13, 8, 5 and 8 meant which module a caller imported decided whether
client_secretorbearer_tokenreached a log.The decision I did not make
secret_redaction.is_credential_fieldmatches by suffix;security/redaction.redact_mappingmatches by substring. On a 10-name sample they disagree on 7 — substring fires onmonkey_patch,tls_key_path,password_hash_algorithm,private_ip,passenger_count,certainty_score,tokenizers_parallelism. Each rule is a documented ruling in its own file, and each is right for its call sites: over-masking is free in a log line and destroys the diagnostic in SSOT config's__repr__. Merging them reverses one recorded ruling and silently changes behaviour at the other's call sites, so this PR documents the split and leaves it open rather than guessing. Filed separately.What Changed
New —
docs/developer/REDACTION_BOUNDARY.md(AC 1). The census table, a which-one-do-I-call table keyed by what you're holding, where to add a new detector, and why the duplication guard cannot see any of this. Indexed fromCLAUDE.md; every module in the census points at it exceptportability.py, which sits on its grandfathered 936-line ceiling (#5060) and would be pushed over by the note — recorded in the doc rather than raising the ceiling.New —
repo_tests/redaction_concept_census_test.py(AC 3). Pins the seven.Changed — two behaviours, both strictly widening. A redaction merge that narrows is a silent under-redaction, so direction was the constraint throughout:
secret_redaction._JWT_RErequiredeyJon the payload as well as the header, making it narrower thana2a/pii_pipeline.py, which anchors the header alone. A payload startseyJonly when its JSON begins exactly{"; a serializer emitting a space after the brace giveseyAi. Such tokens were blocked on the A2A path and passed through here — in the modulellm_shared.credential_redactiondelegates its log redaction to. Now header-anchored, matchingpii_pipeline.credential_redaction.SENSITIVE_KEYSno longer hand-copies the credential nouns; it derives fromsecret_redaction.CREDENTIAL_SUFFIXES. As a union, not a replacement — that vocabulary carries no authorization terms, and a straight swap would have stopped redactingAuthorization,auth_headerandx_auth. Verified: 0 lost, 7 gained.Verification
Pre-push suite, green (this is what gates the push;
[pre-push OK] pytest: all relevant tests pass):The census rule, executed against the real tree (its own functions loaded from the guard file, not a paraphrase):
The rule fires on drift, not on similarity (#17312 AC 4 — a planted fork sharing no 8-line run with any real module):
The union direction, measured:
A failure worth reporting. The first version of the JWT test asserted
pattern == "jwt"on a fixture using{}→e30. That is 3 characters — below the{8,}this scanner requires and belowpii_pipeline's{10,}, so neither rule matched it andhigh_entropyclaimed the span. The pre-push suite caught it. The divergence is real, the example was not; the fixture is now a token verified matched by the new rule and unmatched by the old one. The loose assertion (assert matches) would have passed throughout — the precise one is what surfaced it, and the source comment carried the same wrong example and is corrected with it.Why
MAX_DUP_LINESwill not move.jscpd -k 70 -l 8finds zero clone lines across these modules: they are forks, and a fork shares no 8-line run. The pin neither rises when an eighth lands nor falls when one is collapsed — that blind spot is #17312, and the census is the answer to it for this concept.Not verified locally: this machine sits below the declared dependency floors (104 of 209), so the full CI suite is the authority on anything the pre-push subset does not cover.
Model Used
Claude Opus 5 (1M context)
All three acceptance criteria delivered, with the AC-3 deviation stated above.
Closes #16688
Refs #17312
Refs #12242
Summary by CodeRabbit
Bug Fixes
Documentation