[harness] Preserve harness artifact values and write exit codes#2286
Open
jioffe502 wants to merge 1 commit into
Open
[harness] Preserve harness artifact values and write exit codes#2286jioffe502 wants to merge 1 commit into
jioffe502 wants to merge 1 commit into
Conversation
jioffe502
changed the title
jioffe502
force-pushed
the
codex/retriever-harness-followup
branch
from
c2168c1 to
03e901d
Compare
jioffe502
requested review from
edknv and
nkmcalli
and removed request for
nkmcalli
Contributor
Greptile SummaryThis PR closes two concrete gaps left after #2290: (1) the old substring-based
|
| Filename | Overview |
|---|---|
| nemo_retriever/src/nemo_retriever/harness/artifact_writer.py | Wraps append_text and the binary log-flush inside capture_output_to_log with OSError → artifact_write_error classification; rewrites _is_sensitive_key to allow plural token-limit keys like caption_max_tokens while keeping true credential tokens redacted. The qualifier check for tokens (plural) only looks at the immediately preceding word, which could miss compound credential keys like service_account_tokens. |
| nemo_retriever/src/nemo_retriever/harness/beir_runner.py | Wraps _write_trec_run with the same OSError → artifact_write_error pattern so TREC file write failures surface as exit code 30; removes redundant pre-loop query_results.jsonl unlinks, delegating cleanup to ArtifactWriter.__init__ which already handles this. |
| nemo_retriever/src/nemo_retriever/harness/execution.py | Adds a passthrough guard in the ingest except Exception handler so that a HarnessRunError(EXIT_ARTIFACT_WRITE_FAILURE) raised by capture_output_to_log is re-raised instead of being re-wrapped as EXIT_INGEST_FAILURE. Correctly scoped to ingest only; the query-evaluate phase sits outside any inner try/except so its errors already flow directly to the outer HarnessRunError handler. |
| nemo_retriever/tests/test_harness_artifacts.py | Adds three new tests: token-limit preservation in redact, artifact-write classification for append_text/_write_trec_run, and the ingest-phase passthrough of run-log write failures. Coverage is well-targeted; the yield after raise in the helper context manager is functionally necessary but undocumented. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[run_benchmark] --> B[ArtifactWriter init\ncleans stale artifacts]
B --> C{dry_run?}
C -- no --> D["capture_output_to_log\n(ingest)"]
D -- OSError on log write --> E["artifact_write_error →\nHarnessRunError EXIT_30"]
E --> F{"inner except\nExc.exit_code == 30?"}
F -- yes --> G[re-raise ✓]
F -- no --> H[wrap as EXIT_INGEST_FAILURE]
D -- success --> I[run_ingest_workflow]
I -- failure --> H
I -- success --> J{beir mode?}
J -- yes --> K["capture_output_to_log\n(query_evaluate)"]
K -- OSError on log write --> L["artifact_write_error →\nHarnessRunError EXIT_30"]
K -- success --> M[run_beir_queries]
M --> N[_write_trec_run\nOSError → EXIT_30]
G --> O[outer except HarnessRunError\n→ _failure_outcome with exit_code]
L --> O
H --> O
N --> O
C -- yes --> P[write_artifacts phase]
M --> P
P --> Q[RunOutcome EXIT_0]
%%{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[run_benchmark] --> B[ArtifactWriter init\ncleans stale artifacts]
B --> C{dry_run?}
C -- no --> D["capture_output_to_log\n(ingest)"]
D -- OSError on log write --> E["artifact_write_error →\nHarnessRunError EXIT_30"]
E --> F{"inner except\nExc.exit_code == 30?"}
F -- yes --> G[re-raise ✓]
F -- no --> H[wrap as EXIT_INGEST_FAILURE]
D -- success --> I[run_ingest_workflow]
I -- failure --> H
I -- success --> J{beir mode?}
J -- yes --> K["capture_output_to_log\n(query_evaluate)"]
K -- OSError on log write --> L["artifact_write_error →\nHarnessRunError EXIT_30"]
K -- success --> M[run_beir_queries]
M --> N[_write_trec_run\nOSError → EXIT_30]
G --> O[outer except HarnessRunError\n→ _failure_outcome with exit_code]
L --> O
H --> O
N --> O
C -- yes --> P[write_artifacts phase]
M --> P
P --> Q[RunOutcome EXIT_0]
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
nemo_retriever/src/nemo_retriever/harness/artifact_writer.py:142-145
**Token-plural qualifier checks only the immediately preceding word**
The check `parts[index - 1] in _TOKEN_CREDENTIAL_QUALIFIERS` only examines the word at `index - 1`. A key like `service_account_tokens` would not be redacted because "account" is the immediate predecessor of "tokens", even though "service" (a qualifier) appears at `index - 2`. Similarly `oauth_app_tokens`, `api_service_tokens`, etc. would slip through. The fix is to check any part between position 0 and `index - 1` for a qualifier, rather than only `index - 1`.
### Issue 2 of 2
nemo_retriever/tests/test_harness_artifacts.py:276-279
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
```
Reviews (1): Last reviewed commit: "fix(harness): preserve artifact values a..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+142
to
+145
| return any( | ||
| part == "tokens" and index > 0 and parts[index - 1] in _TOKEN_CREDENTIAL_QUALIFIERS | ||
| for index, part in enumerate(parts) | ||
| ) |
Contributor
There was a problem hiding this comment.
The check parts[index - 1] in _TOKEN_CREDENTIAL_QUALIFIERS only examines the word at index - 1. A key like service_account_tokens would not be redacted because "account" is the immediate predecessor of "tokens", even though "service" (a qualifier) appears at index - 2. Similarly oauth_app_tokens, api_service_tokens, etc. would slip through. The fix is to check any part between position 0 and index - 1 for a qualifier, rather than only index - 1.
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: 142-145
Comment:
**Token-plural qualifier checks only the immediately preceding word**
The check `parts[index - 1] in _TOKEN_CREDENTIAL_QUALIFIERS` only examines the word at `index - 1`. A key like `service_account_tokens` would not be redacted because "account" is the immediate predecessor of "tokens", even though "service" (a qualifier) appears at `index - 2`. Similarly `oauth_app_tokens`, `api_service_tokens`, etc. would slip through. The fix is to check any part between position 0 and `index - 1` for a qualifier, rather than only `index - 1`.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+276
to
+279
| @contextmanager | ||
| def fail_log_capture(*args, **kwargs): | ||
| raise artifact_write_error(OSError("run log unavailable")) | ||
| yield |
Contributor
There was a problem hiding this comment.
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!
jioffe502
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
caption_max_tokensandtext_chunk_overlap_tokenswhile continuing to redact API keys, tokens, credentials, and webhook valuesrun.logand TREC artifact I/O failures through the harness's existing artifact-write error path and exit code 30query_results.jsonlunlinksWhy 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:token, so reproducibility values such asmax_tokenswere serialized as"<redacted>".OSErrorcould 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 passedjp20_smoke --dry-run --jsonartifact check:caption_max_tokens: 77andtext_chunk_max_tokens: 123remained numeric in resolved/planning artifacts