Skip to content

fix(agent): replace assert with runtime guard in secret_sources command parser - #72539

Open
JonthanaHanh wants to merge 1 commit into
NousResearch:mainfrom
JonthanaHanh:fix/secret-sources-assert-guard
Open

JonthanaHanh wants to merge 1 commit into
NousResearch:mainfrom
JonthanaHanh:fix/secret-sources-assert-guard

Conversation

@JonthanaHanh

Copy link
Copy Markdown
Contributor

Summary

Replace assert m is not None with a defensive if m is None: continue guard in agent/secret_sources/command.py:116.

Problem

The assert statement at agent/secret_sources/command.py:116 is stripped by python -O (optimized mode), silently removing the invariant check. While the regex match is logically guaranteed by the upstream filter in the list comprehension, relying on assert for production code is a well-known anti-pattern:

  • Under python -O, the assert is removed entirely
  • If the invariant were ever violated (e.g. regex engine change), the subsequent m.group(1) would raise an unhelpful AttributeError: 'NoneType' object has no attribute 'group'

Fix

Replace:

assert m is not None  # filtered above

With:

if m is None:
    continue  # pragma: no cover — filtered above, defensive guard

This is a no-op in the normal case (the regex always matches lines that already passed the filter) but provides a graceful degradation path instead of an AssertionError crash.

Test Plan

  • Syntax check passes (ast.parse())
  • Existing tests pass
  • python -O -c "import agent.secret_sources.command" — no AssertionError

Notes

This assert was not covered by prior assert-fixing PRs (#56866, #62659, #64818). The agent/secret_sources/ module was recently added and was not included in the earlier assert sweeps.

…nd parser

The `assert m is not None` at agent/secret_sources/command.py:116 is
stripped by `python -O`, silently removing the invariant check. If the
regex match were to fail (e.g. due to a future regex engine change),
the subsequent `m.group(1)` would raise an unhelpful AttributeError
instead of gracefully skipping the line.

Replace with `if m is None: continue` — a defensive guard that preserves
the intent (skip non-matching lines) without relying on assert.

Fixes a production assert not covered by prior PRs (NousResearch#56866, NousResearch#62659,
NousResearch#64818).
@alt-glitch alt-glitch added type/refactor Code restructuring, no behavior change P3 Low — cosmetic, nice to have comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint labels Jul 27, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the defensive cleanup. The target assertion is still present on current main, but the claimed optimized-mode failure is not established by the current parser flow.

Problems

  • agent/secret_sources/command.py:109-113 only appends lines for which _ENV_LINE.match(line) succeeds; agent/secret_sources/command.py:115 immediately repeats that same match before the assertion at line 116. Under python -O, removing the assertion does not itself make m nullable or change normal behavior.
  • The diff adds no regression test for the new branch; existing parsing coverage at tests/test_command_secret_source.py:88-91 covers the base64 fallback only.

Suggested changes

  • Please provide a reachable production condition where the second match can fail, together with a behavior-level test. If none exists, reframe this as a defensive cleanup rather than an optimized-mode bug fix.

Automated hermes-sweeper review.

for line in dotenv_lines:
m = _ENV_LINE.match(line)
assert m is not None # filtered above
if m is None:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The preceding comprehension retains only lines for which this same _ENV_LINE.match succeeded. Under python -O, removing the assertion does not itself make this second match fail; please provide a reachable condition and regression test for this guard, or reframe it as defensive cleanup.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 30, 2026

This branch has not been deployed

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

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users 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.

3 participants