Skip to content

refactor: extract text sanitization from gateway/run.py to gateway/text_sanitizer.py - #55190

Closed
Stoltemberg wants to merge 1 commit into
NousResearch:mainfrom
Stoltemberg:refactor/extract-gateway-text-sanitizer
Closed

refactor: extract text sanitization from gateway/run.py to gateway/text_sanitizer.py#55190
Stoltemberg wants to merge 1 commit into
NousResearch:mainfrom
Stoltemberg:refactor/extract-gateway-text-sanitizer

Conversation

@Stoltemberg

@Stoltemberg Stoltemberg commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Extracts ~280 lines of text sanitization, redaction, and error classification code from gateway/run.py (18,844 lines) into a focused gateway/text_sanitizer.py module. This reduces the gateway god-file and makes the sanitization logic independently testable.

Related Issue

Partially addresses #55138 and #54962 (Extract Gateway Platform Routing from gateway/run.py). Also related to #55071 and #54957 — the extracted module centralizes the redaction logic that prevents auth envelope and tool-trace leaks.

Type of Change

  • ♻️ Refactor (non-breaking change that restructures existing code)

Changes Made

  • gateway/text_sanitizer.py: NEW — text sanitization utilities:
    • Platform classification: _gateway_platform_value, _gateway_surface_passes_raw_text
    • Secret redaction: _redact_gateway_user_facing_secrets, _redact_approval_command
    • Provider error classification: _gateway_provider_error_reply, _looks_like_gateway_provider_error
    • Response sanitization: _sanitize_gateway_final_response, render_notice_line
    • Network error classification: _is_transient_network_error
    • Discord metadata: _non_conversational_metadata
    • All regex patterns for error classification and secret detection
  • gateway/run.py: Replace inline definitions with imports from gateway.text_sanitizer. Keep _TELEGRAM_NOISY_STATUS_RE and thin wrapper for _prepare_gateway_status_message.
  • tests/gateway/test_text_sanitizer.py: NEW — 23 tests covering all extracted functions.

How to Test

  1. Run uv run --extra dev python -m pytest tests/gateway/test_text_sanitizer.py -q — all 23 tests should pass
  2. Run uv run --extra dev python -m pytest tests/gateway/test_telegram_noise_filter.py -q — existing tests should still pass
  3. Run uv run --extra dev ruff check . — should pass

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run uv run --extra dev ruff check . and it passes
  • I've run uv run --extra dev python -m pytest tests/gateway/test_text_sanitizer.py -q and all tests pass
  • I've added tests for my changes
  • I've tested on my platform: Windows

Documentation & Housekeeping

  • I've updated relevant documentation — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — uses pathlib.Path, no platform-specific code
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

@Stoltemberg

Copy link
Copy Markdown
Contributor Author

Partially addresses #55138 and #54962 (Extract Gateway Platform Routing from gateway/run.py).

This extracts ~280 lines of text sanitization/redaction/error classification into gateway/text_sanitizer.py. Also related to #55071 and #54957 — the extracted sanitizer module centralizes the redaction logic that prevents auth envelope and tool-trace leaks.

@alt-glitch alt-glitch added type/refactor Code restructuring, no behavior change comp/gateway Gateway runner, session dispatch, delivery sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages P3 Low — cosmetic, nice to have labels Jun 29, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

COMMENT: Well-motivated refactoring extracting text sanitization from gateway/run.py to gateway/text_sanitizer.py. The extraction is clean — 26 regex patterns and 12+ helper functions moved to a focused module. The import chain in gateway/run.py is updated correctly. However, this is a core gateway change with 543 additions across 3 files. The refactoring touches security-sensitive code (secret redaction, provider error sanitization). Recommend verifying that: (1) all imports resolve correctly in production, (2) the extracted functions maintain identical behavior (no subtle signature changes), (3) the test suite covers the new module path. A focused manual review of the extraction boundaries would be prudent.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the focused extraction; this is consistent with the repository's stated goal of breaking up gateway/run.py.

Problems

  • gateway/text_sanitizer.py:293 omits current main's cancellation-metadata guard from gateway/run.py:429-432 (777cfa81f3e89e817a6946369a82b847879a0dc7). After this import replaces the local helper, chat platforms would emit the interrupt-waiting sentinel, contrary to tests/gateway/test_telegram_noise_filter.py:147-153.
  • gateway/text_sanitizer.py:102-109 omits the xapp- fallback redaction pattern added by fdb9620ac492a332084fd7f53f0acc2c396125a4. Keep it when moving _GATEWAY_SECRET_PATTERNS; the local patterns are intentionally retained for the fail-soft path when agent.redact is unavailable.

Suggested changes

  • Salvage the extraction from current main, preserving both safeguards and their existing contract coverage; add a fallback-path test for xapp- masking.
  • Exercise the gateway.run import path in addition to direct module loading so the production re-export and callback wiring are covered.

Automated hermes-sweeper review.

Comment thread gateway/text_sanitizer.py
_GATEWAY_SECRET_PATTERNS = (
re.compile(r"\bsk-[A-Za-z0-9][A-Za-z0-9_\-]{12,}\b"),
re.compile(r"\bgh[pousr]_[A-Za-z0-9_]{20,}\b"),
re.compile(r"\bxox[baprs]-[A-Za-z0-9\-]{20,}\b"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please retain current main's xapp-\d+-… Slack App-Level token pattern here. Commit fdb9620ac492a332084fd7f53f0acc2c396125a4 added it to the gateway fallback redactor; without it, an agent.redact import/call failure can leak an xapp- token.

Comment thread gateway/text_sanitizer.py
if _gateway_surface_passes_raw_text(platform):
return text

redacted = _redact_gateway_user_facing_secrets(str(text))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current main suppresses INTERRUPT_WAITING_FOR_MODEL_PREFIX here before redaction (gateway/run.py:429-432, 777cfa81f3e89e817a6946369a82b847879a0dc7). Preserve that branch in the extracted helper or chat platforms will deliver cancellation metadata; the existing gateway contract test covers this.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@Stoltemberg

Copy link
Copy Markdown
Contributor Author

Closing — the branch is too far behind current main to rebase cleanly. The reviewer identified three issues that would require ground-up work: the benchmark script referenced by the test isn't included in the PR, the subprocess copies the parent's environment (including API keys), and the return code is discarded so a failed import can pass the timing threshold. The TTS voice-feature additions bundled into the same diff are also orthogonal to the benchmark. Closing rather than rebuilding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/refactor Code restructuring, no behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants