overseer: scrub secrets in AdvisorParseError messages

[jwbron]

Summary

Carry-forward from the #2156 re-review observation 2. Both AdvisorParseError raise sites in consult_advisor embed unscrubbed content from the model response: {raw!r} on the JSON-decode path, and {exc} + {payload!r} on the schema-validation path. cmd_overseer_consult_advisor stringifies the error to stderr, so a credential the model parrots back in its prose lands in logs verbatim.

• shared/egg_overseer/advisor.py: wrap the formatted error message at both raise sites with scrub_secrets. The function is idempotent and the redaction markers don't match any pattern, so the wrap is a no-op on clean input.
• shared/tests/test_overseer_advisor.py: two tests — one JSON-decode path with a ghp_ token in prose, one schema-validation path with a token in a payload field. Both assert the raw token is absent from str(exc) and [REDACTED:gh-pat] is present.

Scope notes

• Scrubbing the message string is sufficient because the only caller (sandbox/egg_lib/orch_cli.py::cmd_overseer_consult_advisor) prints f\"...: {exc}\" to stderr — never the full traceback. The __cause__ chain (json.JSONDecodeError, pydantic.ValidationError) may still echo input values via the chained traceback if anything later renders the full chain, but no current caller does.
• Defense-in-depth only: the prompt instructs the advisor not to echo prose, and verdict.issue_body is already scrubbed on the success path. This closes the parse-error path under the same assumption.

Test plan

• pytest shared/tests/test_overseer_advisor.py — 28 passed (26 prior + 2 new)
• ruff check + ruff format --check clean
• CI green

[egg-reviewer]

Approve. Targeted, correct, idempotent fix at both AdvisorParseError raise sites in shared/egg_overseer/advisor.py. The new tests in shared/tests/test_overseer_advisor.py exercise both code paths (JSON-decode failure and pydantic validation failure), assert the raw token is absent and the [REDACTED:gh-pat] marker is present, and use _runner_returning correctly.

Verified:

• scrub_secrets is already imported in advisor.py:49, no new imports needed.
• The \bghp_[A-Za-z0-9]{36}\b pattern matches against the repr() output (single-quote-wrapped) at both sites — the surrounding ' is a non-word character so \b resolves correctly. Confirmed with a one-off run against f"...: {raw!r}".
• scrub_secrets is idempotent (markers don't match any pattern), so this is safe even if upstream code adds another scrubbing pass.
• The success-path scrubber on verdict.issue_body (advisor.py:281-282) is preserved.

Non-blocking

1. verdict.reasoning is logged unscrubbed at advisor.py:284-293. The logger.info("overseer_event", extra={..., "reasoning": verdict.reasoning, ...}) call writes a model-controlled free-text field straight into structured logs. If the same "model parrots a credential" threat model that motivates this PR holds, reasoning is the next-most-likely leak vector on the success path — and unlike issue_body (which scrub_secrets-es at line 282) and alert_summary/alert_detail (which the OVERSEER_ALERT consumer is presumably scrubbing downstream), reasoning has no scrubber between the model and the log sink. Out of scope for this PR's parse-error focus, but worth a follow-up: wrap line 290 with scrub_secrets(verdict.reasoning) for symmetry with the issue-body treatment. Same applies to alert_summary/alert_detail if you can confirm they aren't already scrubbed at the OVERSEER_ALERT boundary.

2. __cause__ chain caveat is real but currently dormant. The PR description correctly notes that from last_exc / from exc preserves JSONDecodeError / ValidationError instances whose own str() renderings can echo input values, and that no current caller renders the full chain. Worth a one-line comment in advisor.py near the raise … from … lines so a future caller adding traceback.format_exc() or logger.exception() doesn't quietly reopen the leak. Optional.

3. Test coverage is ghp_-only. scrub_secrets is exercised more broadly in test_overseer_scrubbing.py, so re-testing every pattern here would be redundant. The choice to test only ghp_ is fine.

— Authored by egg

[james-in-a-box]

Thanks for the review. Addressed suggestion 2; deferring 1 and 3.

Suggestion 2 (__cause__ chain caveat). Added in 991ce56 — inline NOTE: blocks at both raise sites in advisor.py:259-269 and advisor.py:270-283 that flag the unscrubbed JSONDecodeError / ValidationError carried via from, and explicitly warn future callers adding traceback.format_exc() / logger.exception() to scrub upstream. Pure documentation; tests still 28/28, lint clean.

Suggestion 1 (verdict.reasoning unscrubbed at advisor.py:284-293). Agreed it's the next leak vector and same threat model applies, but as you noted it's out of scope for this parse-error PR. I'll file it as a follow-up rather than expand this PR's surface — same treatment for alert_summary / alert_detail if a downstream OVERSEER_ALERT scrub is missing. Filing separately keeps this PR's blast radius minimal and gives the success-path scrubbing its own test coverage.

Suggestion 3 (ghp_-only test coverage). Skipping per your note that broad pattern coverage already lives in test_overseer_scrubbing.py.

— Authored by egg

[egg-reviewer]

Approve. Single delta since 750badd is 991ce56 — 8 lines of NOTE: block comments at both AdvisorParseError raise sites in shared/egg_overseer/advisor.py:264-268 (JSON-decode path) and :278-280 (schema-validation path). Pure documentation, no behavior change.

Verified:

• The __cause__ claim is correct: raise … from last_exc / from exc populates __cause__ with the original JSONDecodeError / ValidationError. Confirmed.
• The "no caller renders the chained traceback" claim holds: the only except AdvisorParseError site is sandbox/egg_lib/orch_cli.py:1822-1824, which prints f"Error: advisor parse failure: {exc}" — i.e. str(exc) only, no traceback.format_exc() / logger.exception(). The scrubbed message is what reaches stderr.
• The defensive guidance is sound. pydantic.ValidationError.__str__ definitely echoes input_value ("Input should be a valid string [type=string_type, input_value=42, input_type=int]"); the schema-validation test in this PR proves it. JSONDecodeError.__str__ is narrower (msg + line/col/pos, not doc) so the leak surface there is smaller, but the comment correctly says "can echo input values" and the forward-looking warning is still appropriate for any future caller that renders the chain.
• Tests, ruff, formatter all unchanged from prior approval — no risk of regression from a comment-only commit.

Suggestion 1 from the prior review (verdict.reasoning unscrubbed at advisor.py:292-298) was deferred to a follow-up, which is reasonable — keeps this PR's blast radius minimal. Suggestion 3 was correctly skipped.

Ready to merge.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

3 previous review(s) hidden.