From b5bcdb6c482b3a31fbfb80409ccea6d0a7d3f430 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Sat, 1 Aug 2026 20:11:00 +0000 Subject: [PATCH] fix(eval): redact the token from the one failure line that now reaches CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ManagedProcessError.__str__` embeds up to 1000 raw bytes of stderr_tail (process_control.py:72-73), and run_cell printed it verbatim. That line was inert until this branch: nothing ever printed the sweep subprocess's stdout, on success or failure. `echo_stdout` streams it live into the job log, so the print became a sink — and the only one of its kind here that skipped `redact_text`, which results.jsonl and every transcript already apply to exactly this field, for exactly this reason. GitHub masks the registered secret, but masking only catches that literal value; it is not the guarantee the other sinks have. The test drives a ManagedProcessError carrying the token in stderr_tail and asserts it never reaches stdout — verified to fail without the fix. Also from the same pass: bind `result_indexes[0]` once in run_claude, and give the workflow contract test a `findStep` helper instead of five copies of the same `steps.find` predicate. --- eval/tests/test_runner_hardening.py | 27 +++++++++++++++++++ eval/workflow_bench/runner.py | 6 ++++- eval/workflow_bench/runner_sessions.py | 7 ++--- .../unit/skill-evolution-workflow.test.ts | 20 +++++++------- 4 files changed, 47 insertions(+), 13 deletions(-) diff --git a/eval/tests/test_runner_hardening.py b/eval/tests/test_runner_hardening.py index 407bcbe0d..45f543f77 100644 --- a/eval/tests/test_runner_hardening.py +++ b/eval/tests/test_runner_hardening.py @@ -516,6 +516,33 @@ def test_run_cell_records_an_expected_failure_and_still_removes_its_clone(monkey assert removed == [tmp_path / "clone"] +def test_run_cell_redacts_the_auth_token_from_the_failure_it_prints(monkeypatch, tmp_path, capsys): + _stub_cell_dependencies(monkeypatch, tmp_path) + secret = "sk-ant-not-a-real-key" + + def explode(*_args, **_kwargs): + raise ManagedProcessError( + ["claude"], + ManagedProcessResult( + state="exited", + returncode=1, + stdout_tail="", + stderr_tail=f"ANTHROPIC_API_KEY={secret}", + duration_s=1.0, + ), + ) + + monkeypatch.setattr(runner, "run_arm", explode) + context = _cell_context(tmp_path) + context.args.auth_token = secret + # ManagedProcessError stringifies up to 1000 raw bytes of stderr_tail, and + # this line streams live into the CI log now that the sweep's stdout is + # echoed. results.jsonl already redacts the same field. + runner.run_cell(context, 0, "workflow") + + assert secret not in capsys.readouterr().out + + def test_run_cell_lets_an_unexpected_failure_escape_rather_than_scoring_it(monkeypatch, tmp_path): _, removed = _stub_cell_dependencies(monkeypatch, tmp_path) diff --git a/eval/workflow_bench/runner.py b/eval/workflow_bench/runner.py index 71c8eedc0..eaf62fdf1 100644 --- a/eval/workflow_bench/runner.py +++ b/eval/workflow_bench/runner.py @@ -905,7 +905,11 @@ def run_cell(ctx: TaskCellContext, run_idx: int, arm: str) -> dict[str, Any]: # report.md/promotion.json still get written. record = infra_error_record(exc) record["arm"] = arm - print(f"[{task['id']}][{arm}][run {run_idx}] infra-error: {exc}") + # ManagedProcessError carries up to 1000 raw bytes of stderr_tail, and + # this line now streams live into the CI log (run_managed echoes the + # sweep's stdout). Redact it like every other sink this data reaches. + detail = redact_text(str(exc), [args.auth_token or ""]) + print(f"[{task['id']}][{arm}][run {run_idx}] infra-error: {detail}") finally: if worktree is not None and worktree.exists(): try: diff --git a/eval/workflow_bench/runner_sessions.py b/eval/workflow_bench/runner_sessions.py index 1ad7084f3..1b3edff46 100644 --- a/eval/workflow_bench/runner_sessions.py +++ b/eval/workflow_bench/runner_sessions.py @@ -438,19 +438,20 @@ def run_claude( result_indexes = [index for index, event in enumerate(events) if event.get("type") == "result"] if len(result_indexes) != 1: raise ValueError(f"expected exactly one final result event, observed {len(result_indexes)}") - data = events[result_indexes[0]] + result_index = result_indexes[0] + data = events[result_index] # A session that used a background task drains its bookkeeping after # the final result event (`background_tasks_changed`, `task_updated`, # `task_notification` — all `type: "system"`), so the result is last # only among the events that carry evidence. `system` events hold no # tool_use/tool_result/usage payload and so cannot forge skill or cost # evidence; any other event after the result still fails closed. - if any(event.get("type") != "system" for event in events[result_indexes[0] + 1 :]): + if any(event.get("type") != "system" for event in events[result_index + 1 :]): raise ValueError("final result event is not the last event in the captured stream") # Nothing after the result is evidence. Cut the window here so that is # a property of what the evidence readers below can see, rather than an # assumption that a `system` event never carries a tool_use block. - events = events[: result_indexes[0] + 1] + events = events[: result_index + 1] except (UnicodeError, ValueError) as exc: event_stream_error = str(exc) data = {} diff --git a/gitnexus/test/unit/skill-evolution-workflow.test.ts b/gitnexus/test/unit/skill-evolution-workflow.test.ts index 58b8a2b69..864d831bc 100644 --- a/gitnexus/test/unit/skill-evolution-workflow.test.ts +++ b/gitnexus/test/unit/skill-evolution-workflow.test.ts @@ -34,8 +34,14 @@ const workflowDocument = load(workflow) as { const evolveJob = workflowDocument.jobs?.evolve; +type WorkflowStep = NonNullable['steps']>[number]; + +function findStep(stepName: string): WorkflowStep | undefined { + return evolveJob?.steps?.find(({ name }) => name === stepName); +} + function stepRun(stepName: string): string { - const step = evolveJob?.steps?.find(({ name }) => name === stepName); + const step = findStep(stepName); return typeof step?.run === 'string' ? step.run : ''; } @@ -75,9 +81,7 @@ describe('gitnexus skill-evolution workflow contract', () => { // The benchmark sandbox-copies node_modules from all three (tasks.scenarios.yaml). // The root tree was absent on the first real run because only the two subpackage // steps ran, so capture_task_dependency_binding aborted at task binding. - const rootStep = evolveJob?.steps?.find( - ({ name }) => name === 'Install monorepo root dependencies', - ); + const rootStep = findStep('Install monorepo root dependencies'); expect(rootStep).toBeDefined(); expect(rootStep).not.toHaveProperty('working-directory'); // installs at the repo root expect(String(rootStep?.run)).toContain('npm ci'); @@ -104,7 +108,7 @@ describe('gitnexus skill-evolution workflow contract', () => { it('least-privileges the App token and gates the job on a protected Environment', () => { expect(evolveJob?.environment).toBe('gitnexus-evolution'); - const mint = evolveJob?.steps?.find(({ name }) => name === 'Mint GitHub App token'); + const mint = findStep('Mint GitHub App token'); expect(mint?.with).toMatchObject({ 'client-id': expect.any(String), 'permission-contents': 'write', @@ -137,9 +141,7 @@ describe('gitnexus skill-evolution workflow contract', () => { // needs its own, strictly shorter budget: a step timeout only fails that // step, and the always() upload below still ships what it wrote. const jobBudget = evolveJob?.['timeout-minutes']; - const loopStep = evolveJob?.steps?.find( - ({ name }) => name === 'Run the propose → benchmark → gate loop', - ); + const loopStep = findStep('Run the propose → benchmark → gate loop'); const stepBudget = loopStep?.['timeout-minutes']; expect(typeof jobBudget).toBe('number'); expect(typeof stepBudget).toBe('number'); @@ -152,7 +154,7 @@ describe('gitnexus skill-evolution workflow contract', () => { }); it('uploads benchmark evidence unconditionally, on a path it addresses itself', () => { - const upload = evolveJob?.steps?.find(({ name }) => name === 'Upload benchmark evidence'); + const upload = findStep('Upload benchmark evidence'); // The sweep appends results.jsonl and transcripts as it goes, so a killed // generation still holds the evidence explaining why — and a path taken // from the killed step's outputs is exactly what would not be there.