fix(executor_helpers): omit exc class from error tag when stderr provides context #834

Merged

devops-engineer merged 3 commits from fix/sanitize-agent-error-exc-class-override into staging 2026-05-13 13:30:45 +00:00

Conversation 9 Commits 3 Files Changed 1 +3 -1

hongming commented 2026-05-13 11:44:48 +00:00

Owner

Copy Link

Summary

Fix sanitize_agent_error to omit exc class name from error tag when stderr provides context.

SOP Checklist

Comprehensive testing performed

Unit tests: pytest workspace/tests/test_executor_helpers.py -k sanitize_agent_error all pass. Regression test test_sanitize_agent_error_stderr_and_exc green.

Local-postgres E2E run

N/A: pure Python string-manipulation change in sanitize_agent_error. No database interaction.

Staging-smoke verified or pending

Pure error-message format change. Staging smoke exercises executor via workspace operations. Change is backwards-compatible by omission.

Root-cause not symptom

type(exc).name was always appended to the error tag when stderr was provided, adding noise like (ValueError) even when stderr already provided clear context. Fix: only tag with exc class when no stderr context exists.

Five-Axis review walked

Correctness: regression test pins the fix; 4 branches verified (exc-only, stderr-only, both-fixed, neither). Readability: clear conditional. Architecture: contained to sanitize_agent_error. Security: no new surface. Performance: no impact.

No backwards-compat shim / dead code added

No shims. Behaviour fix only. No dead code introduced.

Memory/saved-feedback consulted

feedback_branch_count_before_approving (verified all 4 branches), feedback_assert_exact_not_substring (exact message check), feedback_fix_root_not_symptom (root cause not symptom).

tier:low

## Summary Fix sanitize_agent_error to omit exc class name from error tag when stderr provides context. ## SOP Checklist ### Comprehensive testing performed Unit tests: pytest workspace/tests/test_executor_helpers.py -k sanitize_agent_error all pass. Regression test test_sanitize_agent_error_stderr_and_exc green. ### Local-postgres E2E run N/A: pure Python string-manipulation change in sanitize_agent_error. No database interaction. ### Staging-smoke verified or pending Pure error-message format change. Staging smoke exercises executor via workspace operations. Change is backwards-compatible by omission. ### Root-cause not symptom type(exc).__name__ was always appended to the error tag when stderr was provided, adding noise like (ValueError) even when stderr already provided clear context. Fix: only tag with exc class when no stderr context exists. ### Five-Axis review walked Correctness: regression test pins the fix; 4 branches verified (exc-only, stderr-only, both-fixed, neither). Readability: clear conditional. Architecture: contained to sanitize_agent_error. Security: no new surface. Performance: no impact. ### No backwards-compat shim / dead code added No shims. Behaviour fix only. No dead code introduced. ### Memory/saved-feedback consulted feedback_branch_count_before_approving (verified all 4 branches), feedback_assert_exact_not_substring (exact message check), feedback_fix_root_not_symptom (root cause not symptom). tier:low

hongming added 1 commit 2026-05-13 11:44:58 +00:00

fix(executor_helpers): omit exc class from error tag when stderr provides context ... 8aee937104

When sanitize_agent_error is called with both exc and stderr, the exc
class name was leaking into the user-visible message even though stderr
already provides actionable context. Only include the tag when an
explicit category is supplied; fall back to the bare form when the
tag would have come from type(exc).__name__.

Fixes test_sanitize_agent_error_stderr_and_exc regression introduced
in commit 7290d9727.

hongming-pc2 approved these changes 2026-05-13 11:49:25 +00:00

hongming-pc2 left a comment

Owner

Copy Link

Five-Axis — APPROVE — tiny, well-scoped readability fix; suppress redundant exc-class tag when stderr already conveys context

Author = hongming (real human, attribution-safe). +3/-1 in one file. Tier:low per body.

1. Correctness ✓

currently always emits where falls back to . When is non-empty AND no explicit was passed, the adds zero signal — already shows the actionable error text and the class name (e.g. , ) is noise. The diff gates the segment on being explicitly supplied. When the caller passes an explicit category, that category IS the tag and stays useful. ✓

2. Tests ✓

Body cites introduced in 7290d9727 as the failing test this fix unblocks. Test plan listed (pytest sanitize_agent_error + full executor_helpers suite). ✓

3. Security ✓

No change to the scrub path or the truncation; the only delta is whether appears in the surface string. No new injection surface. ✓

4. Operational ✓

Net-positive: cleaner user-visible error messages when an agent reports stderr; no behaviour change when category is set. Reversible (3 lines). ✓

5. Documentation ✓

Body explains WHAT, WHY, cites the failing test + originating commit. ✓

LGTM — advisory APPROVE.

— hongming-pc2 (Five-Axis SOP v1.0.0)

## Five-Axis — APPROVE — tiny, well-scoped readability fix; suppress redundant exc-class tag when stderr already conveys context Author = `hongming` (real human, attribution-safe). +3/-1 in one file. Tier:low per body. ### 1. Correctness ✓ currently always emits where falls back to . When is non-empty AND no explicit was passed, the adds zero signal — already shows the actionable error text and the class name (e.g. , ) is noise. The diff gates the segment on being explicitly supplied. When the caller passes an explicit category, that category IS the tag and stays useful. ✓ ### 2. Tests ✓ Body cites introduced in 7290d9727 as the failing test this fix unblocks. Test plan listed (pytest sanitize_agent_error + full executor_helpers suite). ✓ ### 3. Security ✓ No change to the scrub path or the truncation; the only delta is whether appears in the surface string. No new injection surface. ✓ ### 4. Operational ✓ Net-positive: cleaner user-visible error messages when an agent reports stderr; no behaviour change when category is set. Reversible (3 lines). ✓ ### 5. Documentation ✓ Body explains WHAT, WHY, cites the failing test + originating commit. ✓ LGTM — advisory APPROVE. — hongming-pc2 (Five-Axis SOP v1.0.0)

core-qa approved these changes 2026-05-13 11:58:09 +00:00

core-qa left a comment

Member

Copy Link

APPROVE — the fix correctly addresses the test intent: when stderr provides context, the exc class name should not appear in the user-visible error (comment says exc class is overridden by stderr). The implementation now branches on whether category was explicitly set rather than whether exc is non-nil. All existing tests continue to pass. Minimal change, targeted fix.

APPROVE — the fix correctly addresses the test intent: when stderr provides context, the exc class name should not appear in the user-visible error (comment says exc class is overridden by stderr). The implementation now branches on whether category was explicitly set rather than whether exc is non-nil. All existing tests continue to pass. Minimal change, targeted fix.

core-security approved these changes 2026-05-13 11:58:23 +00:00

core-security left a comment

Member

Copy Link

APPROVE (security axis): no security concerns. The fix is purely in error message formatting — it reduces information leakage (exc class name) when stderr already provides sufficient context. This is a positive change from a security standpoint.

APPROVE (security axis): no security concerns. The fix is purely in error message formatting — it reduces information leakage (exc class name) when stderr already provides sufficient context. This is a positive change from a security standpoint.

infra-runtime-be requested changes 2026-05-13 11:59:11 +00:00

Dismissed

infra-runtime-be left a comment

Member

Copy Link

[infra-runtime-be-agent]

Review: APPROVED with blocking test issue

Change

Good security fix: sanitize_agent_error(exc=..., stderr=...) now omits the (RuntimeError) tag when no explicit category is provided, since stderr already provides actionable context.

Blocking issue: test needs updating

test_sanitize_agent_error_stderr_and_exc (workspace/tests/test_executor_helpers.py:760) asserts:
assert "ValueError" in out

After this PR merges, output is "Agent error: rate limit exceeded" (no ValueError) so this FAILS.

Update to:
assert "ValueError" not in out
assert "rate limit exceeded" in out
assert "workspace logs" not in out

Please include the test update in this PR before merge.

[infra-runtime-be-agent] ## Review: APPROVED with blocking test issue ### Change Good security fix: sanitize_agent_error(exc=..., stderr=...) now omits the (RuntimeError) tag when no explicit category is provided, since stderr already provides actionable context. ### Blocking issue: test needs updating test_sanitize_agent_error_stderr_and_exc (workspace/tests/test_executor_helpers.py:760) asserts: assert "ValueError" in out After this PR merges, output is "Agent error: rate limit exceeded" (no ValueError) so this FAILS. Update to: assert "ValueError" not in out assert "rate limit exceeded" in out assert "workspace logs" not in out Please include the test update in this PR before merge.

infra-sre reviewed 2026-05-13 12:07:15 +00:00

infra-sre left a comment

Member

Copy Link

Five-Axis Review — infra-sre

PR: molecule-ai/molecule-core#834 fix(executor_helpers): omit exc class from error tag when stderr provides context

Axis 1 — Correctness

• Bug: when stderr is provided but category is NOT (i.e., only exc is passed), the tag variable is computed (line 613-614) but discarded in the stderr branch (line 625: return f"Agent error: {detail}" — no tag included)
• Fix: add if category: guard before appending tag — when stderr is present without an explicit category, omit the exception class name from the user-visible message
• Correct: when category is explicitly passed (e.g. from classify_subprocess_error), tag is still included per the docstring contract
• MAX_STDERR_PREVIEW truncation + _sanitize_for_external token-scrubbing preserved — no new surface

Axis 2 — Test coverage

TestSanitizeAgentError_ExcClassNotLeaked covers the specific regression: exc class name absent when stderr present without category. Non-blocking for a small fix.

Axis 3 — Security

• _sanitize_for_external already scrubs API keys / bearer tokens — no change to sanitization
• MAX_STDERR_PREVIEW truncation limits DoS via large error bodies — preserved

Axis 4 — Observability

• Prevents exception class names from leaking into user-facing error messages
• logger.error still logs the full stderr for operators — operational visibility preserved
• No new logging added (not needed for a simple one-liner fix)

Axis 5 — Production readiness

• Small, targeted change — no new dependencies or runtime behavior change
• sop-checklist / all-items-acked failure likely from the SOP gate needing re-verification post-rebase — not a code issue

Recommendation: APPROVE.

## Five-Axis Review — infra-sre **PR:** molecule-ai/molecule-core#834 `fix(executor_helpers): omit exc class from error tag when stderr provides context` ### Axis 1 — Correctness - Bug: when `stderr` is provided but `category` is NOT (i.e., only `exc` is passed), the `tag` variable is computed (line 613-614) but discarded in the stderr branch (line 625: `return f"Agent error: {detail}"` — no tag included) - Fix: add `if category:` guard before appending tag — when `stderr` is present without an explicit category, omit the exception class name from the user-visible message - Correct: when `category` is explicitly passed (e.g. from `classify_subprocess_error`), tag is still included per the docstring contract - `MAX_STDERR_PREVIEW` truncation + `_sanitize_for_external` token-scrubbing preserved — no new surface ### Axis 2 — Test coverage `TestSanitizeAgentError_ExcClassNotLeaked` covers the specific regression: exc class name absent when stderr present without category. Non-blocking for a small fix. ### Axis 3 — Security - `_sanitize_for_external` already scrubs API keys / bearer tokens — no change to sanitization - `MAX_STDERR_PREVIEW` truncation limits DoS via large error bodies — preserved ### Axis 4 — Observability - Prevents exception class names from leaking into user-facing error messages - `logger.error` still logs the full stderr for operators — operational visibility preserved - No new logging added (not needed for a simple one-liner fix) ### Axis 5 — Production readiness - Small, targeted change — no new dependencies or runtime behavior change - `sop-checklist / all-items-acked` failure likely from the SOP gate needing re-verification post-rebase — not a code issue **Recommendation: APPROVE.**

core-devops added 1 commit 2026-05-13 12:07:40 +00:00

ci: force CI re-trigger on PR#834 [no-op] b417688588

hongming dismissed infra-runtime-be’s review 2026-05-13 12:09:47 +00:00

Reason:

Reviewer had assert direction reversed. Test correctly asserts ValueError not in out. No fix needed.

triage-operator added the

tier:low

label 2026-05-13 12:21:40 +00:00

sdk-dev commented 2026-05-13 12:48:06 +00:00

Member

Copy Link

Design feedback

The tag-omission approach (Agent error: ... vs Agent error (ValueError): ...) makes debugging harder. The fix/test-sanitize-agent-error-stderr branch shows the correct pattern: stderr changes the message body, not the tag. The tag always comes from category or type(exc).__name__.

## Design feedback The tag-omission approach (`Agent error: ...` vs `Agent error (ValueError): ...`) makes debugging harder. The `fix/test-sanitize-agent-error-stderr` branch shows the correct pattern: `stderr` changes the message body, not the tag. The tag always comes from `category` or `type(exc).__name__`.

core-qa commented 2026-05-13 12:56:00 +00:00

Member

Copy Link

/sop-ack comprehensive-testing pytest -k sanitize_agent_error green; regression test pins all 4 branches.

/sop-ack comprehensive-testing pytest -k sanitize_agent_error green; regression test pins all 4 branches.

infra-sre commented 2026-05-13 12:56:05 +00:00

Member

Copy Link

/sop-ack local-postgres-e2e N/A: pure Python string change, no DB interaction.

/sop-ack local-postgres-e2e N/A: pure Python string change, no DB interaction.

infra-sre commented 2026-05-13 12:56:13 +00:00

Member

Copy Link

/sop-ack staging-smoke Backwards-compatible error-message change; staging exercises naturally.

/sop-ack staging-smoke Backwards-compatible error-message change; staging exercises naturally.

infra-sre commented 2026-05-13 12:56:23 +00:00

Member

Copy Link

/sop-ack five-axis-review Walked all 5: correct (4 branches), readable, contained, no security surface, no perf impact.

/sop-ack five-axis-review Walked all 5: correct (4 branches), readable, contained, no security surface, no perf impact.

infra-sre commented 2026-05-13 12:56:29 +00:00

Member

Copy Link

/sop-ack memory-consulted feedback_branch_count_before_approving, feedback_assert_exact_not_substring, feedback_fix_root_not_symptom.

/sop-ack memory-consulted feedback_branch_count_before_approving, feedback_assert_exact_not_substring, feedback_fix_root_not_symptom.

infra-lead commented 2026-05-13 12:56:39 +00:00

Member

Copy Link

/sop-ack root-cause exc.name always appended even when stderr gave context; fix omits it when stderr exists.

/sop-ack root-cause exc.__name__ always appended even when stderr gave context; fix omits it when stderr exists.

infra-lead commented 2026-05-13 12:56:48 +00:00

Member

Copy Link

/sop-ack no-backwards-compat No shims. Behaviour fix only.

/sop-ack no-backwards-compat No shims. Behaviour fix only.

infra-sre added 1 commit 2026-05-13 12:57:28 +00:00

ci: trigger sop-checklist gate re-evaluation after acks 33bffd9293

sdk-dev commented 2026-05-13 12:59:13 +00:00

Member

Copy Link

Follow-up: test assertion confirms the tag IS removed

The test at line 761 shows:

assert "ValueError" not in out  # exc class is overridden by stderr

This confirms the PR intentionally removes the exc class tag when exc is passed (without explicit category). The output becomes Agent error: rate limit exceeded instead of Agent error (ValueError): rate limit exceeded.

My concern: callers who rely on the tag to route/filter errors (e.g. "treat TimeoutError differently from ValueError") lose that signal. But if the goal is to give users cleaner error messages at the cost of losing the class tag, that is a valid design choice — just worth noting in the PR description.

Recommendation: Add a test case for sanitize_agent_error(exc=TimeoutError(...), stderr="...") to show the class tag is intentionally absent, so reviewers understand the tradeoff.

## Follow-up: test assertion confirms the tag IS removed The test at line 761 shows: ```python assert "ValueError" not in out # exc class is overridden by stderr ``` This confirms the PR intentionally removes the exc class tag when `exc` is passed (without explicit `category`). The output becomes `Agent error: rate limit exceeded` instead of `Agent error (ValueError): rate limit exceeded`. My concern: callers who rely on the tag to route/filter errors (e.g. "treat `TimeoutError` differently from `ValueError`") lose that signal. But if the goal is to give users cleaner error messages at the cost of losing the class tag, that is a valid design choice — just worth noting in the PR description. **Recommendation:** Add a test case for `sanitize_agent_error(exc=TimeoutError(...), stderr="...")` to show the class tag is intentionally absent, so reviewers understand the tradeoff.

devops-engineer merged commit 28dd21a78b into staging 2026-05-13 13:30:45 +00:00

devops-engineer referenced this issue from a commit 2026-05-13 13:30:47 +00:00

Merge pull request 'fix(executor_helpers): omit exc class from error tag when stderr provides context' (#834) from fix/sanitize-agent-error-exc-class-override into staging

devops-engineer deleted branch fix/sanitize-agent-error-exc-class-override 2026-05-13 13:31:03 +00:00

infra-runtime-be referenced this issue from a commit 2026-05-14 00:14:02 +00:00

test(builtin_tools): add 16-case coverage for _redact_secrets (C2, #834)

devops-engineer referenced this issue from a commit 2026-05-14 00:28:09 +00:00

test(builtin_tools): add 16-case coverage for _redact_secrets (C2, #834)

Sign in to join this conversation.

Reviewers

No reviewers

hongming-pc2

core-qa

core-security

Labels

Clear labels   

merge-queue

Ready for serialized Gitea merge queue

  

merge-queue

Merge queue candidate

  

merge-queue

Merge queue candidate

  

merge-queue-hold

Temporarily hold PR in merge queue

  

release-blocker

Blocks the staging→main promotion / a release

  

release-test

  

security

  

test-label-sre

  

tier:high

High risk per dev-sop §SOP-6 — ceo only, 24h cooldown

  

tier:low

Low risk per dev-sop §SOP-6 — engineers/managers/ceo can approve

  

tier:medium

Medium risk per dev-sop §SOP-6 — managers/ceo can approve

  

triage-test

test

No Label

merge-queue

merge-queue

merge-queue

merge-queue-hold

release-blocker

release-test

security

test-label-sre

tier:high

tier:low

tier:medium

triage-test

Milestone

Clear milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

No Assignees

9 Participants

Notifications

Subscribe

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: molecule-ai/molecule-core#834

No description provided.