[harness] Preserve harness artifact values and write exit codes

[jioffe502]

Summary

• preserve legitimate token-limit settings such as caption_max_tokens and text_chunk_overlap_tokens while continuing to redact API keys, tokens, credentials, and webhook values
• classify run.log and TREC artifact I/O failures through the harness's existing artifact-write error path and exit code 30
• rely on the artifact writer's startup cleanup instead of repeating raw query_results.jsonl unlinks

Why this remains after #2290

#2290 absorbed the broader artifact-safety work: atomic JSON publication, preflight-before-cleanup, portable relative manifests, and exit-30 handling for JSON and session writes. This PR intentionally keeps that contract unchanged.

Two concrete gaps remained on merged main:

1. Redaction matched the substring token, so reproducibility values such as max_tokens were serialized as "<redacted>".
2. Raw text writes could bypass the artifact-write classifier. A TREC/run-log OSError could therefore surface as internal exit code 70 instead of artifact exit code 30.

Scope boundaries

This PR does not change the artifact schema, session layout, output-directory reuse policy, manifest format, Slack reporting, or benchmark execution behavior introduced by #2290. It adds no new writer abstraction.

Validation

• uv run pytest -q tests/test_harness*.py tests/test_beir_evaluation.py — 72 passed
• pre-commit on all four changed files — passed
• real jp20_smoke --dry-run --json artifact check:
  • exit code 0
  • reranker secret absent from stdout and emitted JSON
  • caption_max_tokens: 77 and text_chunk_max_tokens: 123 remained numeric in resolved/planning artifacts

[greptile-apps]

Greptile Summary

This PR fixes two concrete gaps left after #2290: over-aggressive redaction of token-count config keys (e.g. caption_max_tokens) and text-write I/O failures (TREC run, run.log) that could escape the artifact-write error classifier and surface as exit code 70 instead of 30.

• Redaction logic is tightened from a substring match on \"token\" to a whole-word part check: bare \"token\" (singular) is always sensitive, \"tokens\" (plural) is sensitive only when immediately preceded by a credential qualifier (access, auth, oauth, etc.), and count-style keys like caption_max_tokens and text_chunk_overlap_tokens are preserved.
• append_text and _write_trec_run are wrapped in try/except OSError that raises artifact_write_error, routing disk failures through exit code 30; capture_output_to_log's final log-flush block receives the same treatment.
• execution.py adds an early re-raise guard so an EXIT_ARTIFACT_WRITE_FAILURE that propagates out of capture_output_to_log during ingest is not mis-classified as EXIT_INGEST_FAILURE; new tests cover all three paths.

Confidence Score: 4/5

Safe to merge with one fix: the bare tokens key is silently no longer redacted, reversing previous behaviour and creating a potential credential-leakage path in artifact files and Slack posts.

The ingest error re-raise guard, the artifact-write error wrapping for text writes and TREC runs, and the ArtifactWriter-based cleanup removal are all correct. The token-limit preservation logic works as intended for the tested cases. The one concrete issue is that a config key named exactly tokens (plural, no qualifier) was previously redacted and is now passed through, which is a regression in the security boundary the PR is explicitly hardening.

nemo_retriever/src/nemo_retriever/harness/artifact_writer.py — specifically the _is_sensitive_key function and _SENSITIVE_KEY_MARKERS/_TOKEN_CREDENTIAL_QUALIFIERS constants

Security Review

• Credential-redaction regression (artifact_writer.py lines 139–145): A config key named exactly tokens (plural, no qualifying prefix) is no longer redacted. The old substring check \"token\" in normalized covered this case; the new part-equality check requires the singular \"token\" or a credential-qualified plural, so a bare tokens key passes through. Any artifact or Slack post that includes {\"tokens\": [...]} (e.g., an embedded OAuth response) will now expose the value in plaintext.

Important Files Changed

Filename	Overview
nemo_retriever/src/nemo_retriever/harness/artifact_writer.py	Refactors redaction logic to use whole-word part matching for "token"/"tokens" and wraps text-write I/O failures as artifact write errors; standalone plural "tokens" key is no longer redacted (regression)
nemo_retriever/src/nemo_retriever/harness/beir_runner.py	Wraps _write_trec_run I/O in artifact_write_error and removes redundant query_results.jsonl unlinks that are now handled by ArtifactWriter startup cleanup
nemo_retriever/src/nemo_retriever/harness/execution.py	Adds an early re-raise guard for EXIT_ARTIFACT_WRITE_FAILURE inside the ingest exception handler to prevent artifact errors from being misclassified as ingest failures
nemo_retriever/tests/test_harness_artifacts.py	Adds tests for the new token-limit preservation, text-artifact error classification, and run-log write failure propagation

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[capture_output_to_log] -->|append_text start header| B{OSError?}
    B -->|yes| C[raise artifact_write_error EXIT_ARTIFACT_WRITE_FAILURE]
    B -->|no| D[redirect stdout/stderr to buf - yield body]
    D -->|body raises| E[record failure_traceback - re-raise]
    D -->|body ok| F[restore fds]
    E --> F
    F --> G{write buf + traceback to run.log}
    G -->|OSError| C
    G -->|ok| H[write captured to stderr if failed]

    subgraph execution.py
        I[run_ingest_workflow] -->|exception| J{HarnessRunError EXIT_ARTIFACT_WRITE_FAILURE?}
        J -->|yes - NEW| K[re-raise as-is]
        J -->|no| L[wrap as EXIT_INGEST_FAILURE]
    end

    C --> J

    subgraph _is_sensitive_key
        M[normalize key to snake_case] --> N{marker substring in normalized?}
        N -->|yes| O[redact]
        N -->|no| P{token whole part?}
        P -->|yes| O
        P -->|no| Q{tokens with credential qualifier at index-1?}
        Q -->|yes| O
        Q -->|no| R[preserve value]
    end

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[capture_output_to_log] -->|append_text start header| B{OSError?}
    B -->|yes| C[raise artifact_write_error EXIT_ARTIFACT_WRITE_FAILURE]
    B -->|no| D[redirect stdout/stderr to buf - yield body]
    D -->|body raises| E[record failure_traceback - re-raise]
    D -->|body ok| F[restore fds]
    E --> F
    F --> G{write buf + traceback to run.log}
    G -->|OSError| C
    G -->|ok| H[write captured to stderr if failed]

    subgraph execution.py
        I[run_ingest_workflow] -->|exception| J{HarnessRunError EXIT_ARTIFACT_WRITE_FAILURE?}
        J -->|yes - NEW| K[re-raise as-is]
        J -->|no| L[wrap as EXIT_INGEST_FAILURE]
    end

    C --> J

    subgraph _is_sensitive_key
        M[normalize key to snake_case] --> N{marker substring in normalized?}
        N -->|yes| O[redact]
        N -->|no| P{token whole part?}
        P -->|yes| O
        P -->|no| Q{tokens with credential qualifier at index-1?}
        Q -->|yes| O
        Q -->|no| R[preserve value]
    end

Loading

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
nemo_retriever/src/nemo_retriever/harness/artifact_writer.py:139-145
**Bare `tokens` (plural) key no longer redacted — security regression**

The old code checked `"token" in normalized` as a substring, so a key literally named `tokens` (plural, no qualifier) was always redacted. The new code requires either an exact part `"token"` (singular) or `"tokens"` preceded by a credential qualifier at `index - 1`. For a standalone key `tokens`, `parts = ["tokens"]`, `"token" not in parts` is True, and the qualifier check fails because `index 0 > 0` is False — so the key passes through unredacted.

Any config dict with `{"tokens": [...]}` (e.g., an OAuth token list, Slack API response embedded in a config) will now serialize its value in plaintext where it previously would have been `"<redacted>"`. Adding `"tokens"` to `_SENSITIVE_KEY_MARKERS` would restore full coverage while keeping all the new qualifications for count-style keys.

_{Reviews (2): Last reviewed commit: "Merge branch 'main' into codex/retriever..." | Re-trigger Greptile}

[greptile-apps]

The yield after raise is unreachable at runtime but is semantically required: Python classifies a function as a generator function only when it contains at least one yield expression, regardless of reachability. Without the yield, @contextmanager would receive a plain function and would fail when __enter__ calls next(). A brief comment prevents future confusion or spurious linter suppression.

Suggested change

	@contextmanager
	def fail_log_capture(*args, **kwargs):
	raise artifact_write_error(OSError("run log unavailable"))
	yield
	@contextmanager
	def fail_log_capture(*args, **kwargs):
	raise artifact_write_error(OSError("run log unavailable"))
	yield # unreachable; required to make this a generator function for @contextmanager

Prompt To Fix With AI

This is a comment left during a code review.
Path: nemo_retriever/tests/test_harness_artifacts.py
Line: 276-279

Comment:
The `yield` after `raise` is unreachable at runtime but is semantically required: Python classifies a function as a generator function only when it contains at least one `yield` expression, regardless of reachability. Without the `yield`, `@contextmanager` would receive a plain function and would fail when `__enter__` calls `next()`. A brief comment prevents future confusion or spurious linter suppression.

```suggestion
    @contextmanager
    def fail_log_capture(*args, **kwargs):
        raise artifact_write_error(OSError("run log unavailable"))
        yield  # unreachable; required to make this a generator function for @contextmanager
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[charlesbluca]

This only checks the segment immediately before plural tokens, so compound credential keys such as service_account_tokens and oauth_app_tokens remain unredacted; bare tokens does as well. Since these artifacts may be replayed or shared, could we make plural-token handling fail closed while explicitly preserving known budget fields such as max_tokens and overlap_tokens? Please add compound-key and structured-override regressions.

[greptile-apps]

Bare tokens (plural) key no longer redacted — security regression

The old code checked "token" in normalized as a substring, so a key literally named tokens (plural, no qualifier) was always redacted. The new code requires either an exact part "token" (singular) or "tokens" preceded by a credential qualifier at index - 1. For a standalone key tokens, parts = ["tokens"], "token" not in parts is True, and the qualifier check fails because index 0 > 0 is False — so the key passes through unredacted.

Any config dict with {"tokens": [...]} (e.g., an OAuth token list, Slack API response embedded in a config) will now serialize its value in plaintext where it previously would have been "<redacted>". Adding "tokens" to _SENSITIVE_KEY_MARKERS would restore full coverage while keeping all the new qualifications for count-style keys.

Prompt To Fix With AI

This is a comment left during a code review.
Path: nemo_retriever/src/nemo_retriever/harness/artifact_writer.py
Line: 139-145

Comment:
**Bare `tokens` (plural) key no longer redacted — security regression**

The old code checked `"token" in normalized` as a substring, so a key literally named `tokens` (plural, no qualifier) was always redacted. The new code requires either an exact part `"token"` (singular) or `"tokens"` preceded by a credential qualifier at `index - 1`. For a standalone key `tokens`, `parts = ["tokens"]`, `"token" not in parts` is True, and the qualifier check fails because `index 0 > 0` is False — so the key passes through unredacted.

Any config dict with `{"tokens": [...]}` (e.g., an OAuth token list, Slack API response embedded in a config) will now serialize its value in plaintext where it previously would have been `"<redacted>"`. Adding `"tokens"` to `_SENSITIVE_KEY_MARKERS` would restore full coverage while keeping all the new qualifications for count-style keys.

How can I resolve this? If you propose a fix, please make it concise.

[ChrisJar]

lgtm apart from existing comments