From 750badd623e1bbbfcbe385ac7ba8e4df5dbb0f96 Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Mon, 27 Apr 2026 13:38:14 -0700 Subject: [PATCH 1/2] overseer: scrub secrets in AdvisorParseError messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. The error gets stringified to stderr by `cmd_overseer_consult_advisor`, so a credential the model parrots back in its prose lands in logs verbatim. Apply `scrub_secrets` to the formatted error message at both sites. The function is idempotent and the markers don't match any pattern, so wrapping is safe and a no-op when the input is clean. Adds two tests: one for the JSON-decode path with a `ghp_` token in prose, one for the schema-validation path with a token in a payload field. Both assert the raw token is gone and the redaction marker is present in `str(exc)`. --- shared/egg_overseer/advisor.py | 12 ++++++-- shared/tests/test_overseer_advisor.py | 41 +++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 3 deletions(-) diff --git a/shared/egg_overseer/advisor.py b/shared/egg_overseer/advisor.py index 9a41e0e95b..f87c642614 100644 --- a/shared/egg_overseer/advisor.py +++ b/shared/egg_overseer/advisor.py @@ -259,16 +259,22 @@ async def _default_runner(p: str, m: str) -> str: if payload is None: # No `{` ever found → no useful inner exception; otherwise # chain the most recent ``raw_decode`` failure for context. + # Scrub before raising: ``raw`` is the unparsed model output + # and ends up in stderr via ``cmd_overseer_consult_advisor``. raise AdvisorParseError( - f"consult_advisor: SDK response is not valid JSON: {raw!r}" + scrub_secrets(f"consult_advisor: SDK response is not valid JSON: {raw!r}") ) from last_exc try: verdict = AdvisorVerdict.model_validate(payload) except Exception as exc: + # Scrub: ``payload`` and the pydantic error both echo input + # values that may include credentials the model parroted back. raise AdvisorParseError( - f"consult_advisor: SDK response failed AdvisorVerdict " - f"validation: {exc}; payload={payload!r}" + scrub_secrets( + f"consult_advisor: SDK response failed AdvisorVerdict " + f"validation: {exc}; payload={payload!r}" + ) ) from exc # Defense-in-depth: scrub the body before it leaves this function. diff --git a/shared/tests/test_overseer_advisor.py b/shared/tests/test_overseer_advisor.py index e8db1380e7..a4820c09b9 100644 --- a/shared/tests/test_overseer_advisor.py +++ b/shared/tests/test_overseer_advisor.py @@ -339,6 +339,47 @@ def test_schema_failure_raises_parse_error(self) -> None: ) ) + def test_invalid_json_error_message_is_scrubbed(self) -> None: + # Raw model output is embedded in the AdvisorParseError message + # and ends up in stderr via cmd_overseer_consult_advisor. If the + # model parrots a credential in its prose, the error must not + # surface it verbatim. + runner = self._runner_returning(f"oops, leaked {_GH_PAT} not json") + with pytest.raises(AdvisorParseError) as excinfo: + asyncio.run( + consult_advisor( + classification={}, + health_alerts=[], + progress_events=[], + recent_log_lines=[], + _agent_runner=runner, + ) + ) + message = str(excinfo.value) + assert _GH_PAT not in message + assert "[REDACTED:gh-pat]" in message + + def test_validation_error_message_is_scrubbed(self) -> None: + # Same concern on the schema-validation path: the payload repr + # and pydantic error both echo input values back into the + # message string. + runner = self._runner_returning( + {"decision": "alert", "alert_summary": f"saw token {_GH_PAT}"} + ) + with pytest.raises(AdvisorParseError) as excinfo: + asyncio.run( + consult_advisor( + classification={}, + health_alerts=[], + progress_events=[], + recent_log_lines=[], + _agent_runner=runner, + ) + ) + message = str(excinfo.value) + assert _GH_PAT not in message + assert "[REDACTED:gh-pat]" in message + def test_default_model_used_when_config_none(self) -> None: seen: dict[str, str] = {} From 991ce56fb346c5b02529d615d4b8f27dc0018564 Mon Sep 17 00:00:00 2001 From: "egg-reviewer[bot]" <261018737+egg-reviewer[bot]@users.noreply.github.com> Date: Mon, 27 Apr 2026 20:50:56 +0000 Subject: [PATCH 2/2] overseer: comment __cause__ chain caveat at AdvisorParseError raises MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewer follow-up on #2163: add an inline NOTE at both raise sites flagging that __cause__ (JSONDecodeError / ValidationError) preserves unscrubbed input, so any future caller that renders the chained traceback (traceback.format_exc, logger.exception) must scrub there too. Pure documentation — no behavior change. --- shared/egg_overseer/advisor.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/shared/egg_overseer/advisor.py b/shared/egg_overseer/advisor.py index f87c642614..97e81aeb07 100644 --- a/shared/egg_overseer/advisor.py +++ b/shared/egg_overseer/advisor.py @@ -261,6 +261,11 @@ async def _default_runner(p: str, m: str) -> str: # chain the most recent ``raw_decode`` failure for context. # Scrub before raising: ``raw`` is the unparsed model output # and ends up in stderr via ``cmd_overseer_consult_advisor``. + # NOTE: ``__cause__`` (last_exc) preserves the original + # ``JSONDecodeError`` whose ``str()`` can echo input values. + # Safe today because no caller renders the chained traceback + # — if you add ``traceback.format_exc()`` or + # ``logger.exception()`` upstream, scrub there too. raise AdvisorParseError( scrub_secrets(f"consult_advisor: SDK response is not valid JSON: {raw!r}") ) from last_exc @@ -270,6 +275,9 @@ async def _default_runner(p: str, m: str) -> str: except Exception as exc: # Scrub: ``payload`` and the pydantic error both echo input # values that may include credentials the model parroted back. + # Same ``__cause__`` caveat as the JSON-decode path above: + # ``ValidationError`` carries unscrubbed input; safe only while + # callers don't render the chained traceback. raise AdvisorParseError( scrub_secrets( f"consult_advisor: SDK response failed AdvisorVerdict "