Greptile Summary

This PR fixes a span-leak bug in GuardrailEngine._redact where overlapping redaction rules (e.g. a wide keyword fully containing a narrower keyword) would silently drop the wider rule's match, leaving its prefix and suffix unredacted in the output. The fix replaces the old single-cursor right-to-left walk with a proper interval merge: sort → left-to-right sweep to merge overlapping spans (tracking the largest individual match width to select the representative rule name) → right-to-left application of replacements to preserve string indices.

Key changes:

• apply delegates to a new _redact(text, matches) method, improving testability and separation of concerns.
• The sort key changes from reverse=True on (start, end) to (start ASC, -end) — preserving the "widest-match-first at same start" behavior from PR fix: overlapping keyword redaction and hardcoded investigation loop limit #520 while enabling a correct forward sweep.
• The seen_end cursor and skip logic are replaced with an O(n) interval merge that handles all three overlap classes: containment, partial overlap, and the transitive chain A∩B, B∩C, A⊥C.
• 7 new regression tests cover all relevant sub-cases; the 36 existing guardrail tests are unaffected.

Confidence Score: 5/5

Safe to merge — the interval merge algorithm is correct, well-documented, and backed by 7 targeted regression tests covering all overlap sub-cases, with zero regressions in the existing suite.

The core fix (left-to-right interval merge replacing the buggy seen_end cursor) is algorithmically correct: overlapping spans are collapsed, the widest individual match wins the representative rule name, adjacent spans remain separate, and right-to-left application preserves string indices. All edge cases (containment, partial overlap, transitive chain, disjoint, adjacent, regex+keyword mix) have dedicated tests. Code follows project style conventions. The only finding is a non-blocking P2 style suggestion to use a NamedTuple instead of a raw 4-tuple for the merged interval list.

No files require special attention.

Important Files Changed

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["apply(text)"] --> B["scan(text) → ScanResult"]
    B --> C{any matches?}
    C -- No --> D[return text unchanged]
    C -- Yes --> E[audit each match]
    E --> F{blocked?}
    F -- Yes --> G[raise GuardrailBlockedError]
    F -- No --> H["_redact(text, matches)"]
    H --> I["Filter: action == REDACT\nSort by (start ASC, -end)"]
    I --> J["Left-to-right sweep\nmerge overlapping intervals\ntrack widest individual match\nfor rule-name selection"]
    J --> K["Right-to-left application\nof replacements over\nmerged intervals"]
    K --> L[return redacted text]
    style G fill:#f66,color:#fff
    style L fill:#6c6,color:#fff

_{Reviews (1): Last reviewed commit: "fix(guardrails): redact union of overlap..." | Re-trigger Greptile}

Heads up: the test (ubuntu-latest) CI failure on this PR is a pre-existing bug on main, not introduced here.

tests/nodes/plan_actions/test_node_plan_actions.py::test_node_plan_actions_emits_retrieval_controls raises AttributeError: '_InputStub' object has no attribute 'model_copy' because PR #807 added a model_copy / model_dump round-trip in app/nodes/plan_actions/node.py:55 but did not update the test stub. The same failure reproduces on a clean checkout of main with no other changes.

I opened #929 with the minimal stub repair (14 lines in the test file, no production code touched). Once that lands, I'll rebase this PR — the rest of CI on this branch is green (typecheck, quality, CodeQL, Analyze (python)).

@mayankbharati-ops still trying to understand the context for this change? could you help

@muddlebee — happy to walk through it. The TL;DR is a sensitive-data leak in GuardrailEngine's redaction pass when two rules overlap on the same span — including with the rules we ship in _STARTER_CONFIG. I just re-ran a live repro on main to confirm everything below.

1. The bug, in one screenshot of output

I ran the exact same scenarios through GuardrailEngine.apply on main (current HEAD) and on this branch:

Row 2 is the worst one: with app/guardrails/cli.py:_STARTER_CONFIG (the rules shipped to every user via opensre guardrails init), the literal substring api_key= survives in any text we redact and forward to an LLM — because aws_access_key (matches AKIA… only, span [16:36]) and generic_api_token (matches api_key=AKIA…, span [8:36]) both fire, and main's loop drops the wider one. Same shape with aws_secret_access_key=AKIA…xxxxx….

2. Why main does this — the algorithm

GuardrailEngine.apply had this redact loop on main (engine.py:127-138):

redact_matches = sorted(matches_with_REDACT_action, key=(start, end), reverse=True)
seen_end = len(text)
for match in redact_matches:
    if match.end > seen_end:        # ← drops the wider match
        continue
    redacted = redacted[:match.start] + replacement + redacted[match.end:]
    seen_end = match.start

It walks right-to-left with a single seen_end cursor and silently skips any match whose end exceeds the cursor. PR #520 fixed this for same-start overlaps (longest-keyword-wins) but matches with different start offsets that fully contain a narrower match still slip through — the wider match's prefix and suffix are never written over.

3. The fix — canonical interval merge

engine.py change replaces the cursor walk with the standard merge-intervals algorithm:

1. Sort redact matches by (start ASC, -end) so same-start ties put the widest match first (preserves PR fix: overlapping keyword redaction and hardcoded investigation loop limit #520's behavior).
2. Sweep left-to-right. Merge any match whose start < merged[-1].end. Track the contributing match with the largest original width — that one wins the representative rule_name for the merged span.
3. Apply replacements right-to-left over the merged intervals so string indices stay valid.

Adjacent matches (A.end == B.start) intentionally do not merge — verified by the test_adjacent_matches_not_merged test. Disjoint matches stay independent. Audit logging is unchanged: every individual ScanMatch still emits its own audit entry; we only collapse the output redactions.

(The merged-span list uses a _MergedSpan: NamedTuple — addressing the Greptile P2 nit on the original commit; this is what d26c631 refactor(guardrails): use NamedTuple for merged-span records does.)

4. Test coverage I added on top

Live verification just now: 91/91 tests in tests/test_guardrails/ pass on this branch. CI on the PR is green: test, typecheck, quality, CodeQL, Analyze (python). Approved by @yashksaini-coder on 2026-04-25 with an independent algorithm walkthrough.

5. Why this is worth merging

The class of bug is sensitive-data disclosure to an LLM provider (Anthropic / OpenAI / Bedrock). Anywhere a Tracer user enables guardrails over their default _STARTER_CONFIG, two rules covering the same secret will leak the prefix/suffix bookends into every outgoing prompt, system message, and chat node. This PR makes the redaction step actually idempotent over overlapping rule sets.

Happy to step through any specific scenario on a call or in a screenshare if any part of the merge algorithm itself is the unclear bit — that's the part most worth eyeballing carefully.

@muddlebee — pushed 5f1b3b6 addressing all three review nits. Detail per comment:

1. app/guardrails/engine.py:42 — width field name ambiguity

Renamed _MergedSpan.width → source_width and expanded both the NamedTuple docstring and the _redact algorithm comment to spell out the distinction explicitly. The docstring now reads:

source_width is the width of the single ScanMatch that contributed the current rule_name — i.e. that match's end - start before any merging happened. It is not the merged span's width (end - start of the _MergedSpan); in a chained-overlap scenario where A merges B and then C overlaps the grown span, the rule-name winner is whichever individual source match was the widest, not whichever rule covered the widest post-merge range.

The inline comment in _redact also flags this on the line where source_width is computed, so anyone editing the loop body sees the distinction in context. Good catch — width alone was genuinely ambiguous.

2. tests/test_guardrails/test_llm_integration.py:340 — _FakeMessages / _FakeClient duplication

Right — the four ad-hoc copies were a drift hazard. Extracted into two module-level pytest fixtures:

@pytest.fixture
def anthropic_capture(monkeypatch) -> tuple[Any, dict[str, Any]]:
    """Patch LLMClient (Anthropic surface) to capture messages.create kwargs."""
    ...
    return client, captured

@pytest.fixture
def openai_capture(monkeypatch) -> tuple[Any, dict[str, Any]]:
    """Patch OpenAILLMClient to capture chat.completions.create kwargs."""
    ...
    return client, captured

Each test in TestOverlappingRedactionReachesDownstream is now ~5–10 lines of actual test logic instead of 25+ of mostly-identical fake-class scaffolding. The two response-shape literals (_anthropic_fake_response / _openai_fake_response) are also factored out, so a future SDK change touches one location instead of four.

3. tests/test_guardrails/test_llm_integration.py:382 — system-prompt assertion

Tightened to:

# Subscript (not ``.get``) so a missing ``system`` kwarg surfaces a
# KeyError immediately — the whole point of the test is that the
# client hoisted the system entry out of the messages list.
system_kwarg = captured["system"]
assert system_kwarg, "system kwarg must be non-empty after redaction"
assert "api_key=" not in system_kwarg
assert "[REDACTED:generic_api_token]" in system_kwarg
# System must be a separate kwarg, not smuggled into ``messages``.
for msg in captured.get("messages", []):
    assert msg.get("role") != "system", (
        f"system role leaked into messages list: {msg!r}"
    )

KeyError on a missing system kwarg is now louder than the old .get(...) is not None (which produced an unhelpful AssertionError if the kwarg was absent), and the new structural assertion catches the failure mode you flagged — Anthropic client failing to hoist role: system out of the messages list. The test now actually exercises both "the system kwarg exists and was redacted" and "the system role was not smuggled into messages."

Verification

• pytest tests/test_guardrails/: 91 / 91 pass
• Full unit suite: 3159 pass / 2 skipped / 1 xfailed
• ruff check and ruff format --check clean on both files
• mypy app/guardrails/ clean (the 5 errors mypy reports are pre-existing missing-stub warnings in app/integrations/ — unrelated)
• CI on 5f1b3b6: all 5 active checks SUCCESS (quality, Analyze (python), typecheck, test, CodeQL)

Ready for another look.

Looks good from me. I will wait for additional reviews..

@VaibhavUpreti

🏄 Some PRs rot in review for six weeks. @mayankbharati-ops's said "not today" and merged like it owned the place. 🌊

👋 Join us on Discord - OpenSRE : hang out, contribute, or hunt for features and issues. Everyone's welcome.