Repository navigation
[eval][cli] fix: harden source digest, rubric parsing, and secret redaction - #458
JoyboyBrian wants to merge 4 commits into
Conversation
…action
- source_digest length-prefixes extra, paths, and contents, so trees such as {"ab": "c"} and {"a": "bc"} no longer share a digest. Digest values change: the Harbor bundle cache misses once, and named local eval runs created by an earlier SDK refuse to resume.
- Rubric datasets skip non-dict entries in "messages" and report a CLIError with the line number instead of raising AttributeError.
- Secret redaction replaces the whole string when the "[REDACTED]" marker would join its neighbours into the secret again.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
github-actions
Bot
added
bug
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
View guided diff | Turn on auto-fix | Re-trigger cubic
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
A secret that is part of "[REDACTED]" (for example "RE") survives any marker-based replacement, so the fallback returns an empty string in that case.
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
View guided diff | Turn on auto-fix | Re-trigger cubic
Redacting one secret at a time let a later secret's marker rebuild an earlier one (secrets "]x" and "y" turned "yx" into "[REDACTED]x"). Each string is now replaced for all secrets and then checked against all of them before the fallback applies.
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
View guided diff | Turn on auto-fix | Re-trigger cubic
Values under "value", "secret" and "secret_value" were set to a bare "[REDACTED]", so a secret contained in the marker (for example "RE") still appeared there. They now go through the same check as other redacted strings.
What
source_digest(osmosis_ai/source_scan.py) length-prefixesextra, each relative path, and each file's contents before hashing, so different source trees can no longer hash the same byte stream (previously{"ab": b"c"}and{"a": b"bc"}produced the same digest).extract_assistant_content(osmosis_ai/eval/rubric/dataset.py) skips non-dict entries inmessages, so a row like{"messages": [1]}reports the usual line-numberedCLIErrorinstead of raisingAttributeErrorwith a traceback.osmosis_ai/platform/cli/secret_redact.py) routes string values, dict keys, andredact_secret_textthrough one helper that replaces the whole string with[REDACTED]when the marker would join its neighbours into the secret again (for example"]]]"with the secret"]]"previously became"[REDACTED]]"), and with an empty string when the secret is part of the marker itself (for example"RE").redact_provided_secretsnow replaces all secrets in each string and checks the result against all of them, so one secret's marker cannot rebuild another ("yx"with secrets"]x"and"y"previously became"[REDACTED]x").Why
These came out of writing "fuzz test as spec" style properties for pure-Python trust-boundary functions: the digest should be injective over source trees, rubric loading should fail only with
CLIError, and redacted output should never contain the secret. Each property was violated by a small, concrete input.The digest collision matters most: the same digest keys the Harbor bundle wheel cache, pins the resume lock of named local eval runs, and is recorded as
source_digestin uploaded eval manifests, so a collision could reuse a stale bundle or let a run resume across a code change.Compatibility: digest values change for every project. The Harbor bundle cache misses once and rebuilds. A named local eval run created by an earlier SDK refuses to resume after upgrading, reporting that the rollout source changed. Newly uploaded manifests carry the new digest values. No public API changes; I did not mark this
[BREAKING], but the release notes should mention the resume impact.How to Test
uv run pytest tests/unit/eval/local tests/unit/test_packaging.py tests/unit/test_rubric_cli_command.py tests/unit/platform/cli/test_secret_redact.py tests/unit/rollout/test_harbor_backend.py→ 754 passed.uv run pytest tests/unit/platform/cli tests/unit/cli→ 1528 passed.uv run ruff check .anduv run ruff format --check .→ pass.uv run ty check --error-on-warning osmosis_ai/→ pass.git diff --check origin/main...HEAD→ clean.pytest -n 4 --dist worksteallocally: 3733 passed, 11 failed, all intests/unit/test_dependency_security.py::test_wheel_verifier_checks_allowed_versions. My local environment has stale installedosmosis-aimetadata from before [misc] fix: require patched certifi and uv trust stores #457 (uv<0.13,>=0.9.7), which the wheel verifier reads; unrelated to this diff, CI builds fresh metadata.test_a_second_interrupt_during_server_shutdown_cuts_its_grace_shortfailed once under xdist and passed on rerun.uv run --no-editable pyright --verifytypes osmosis_ai --ignoreexternal(no public API changes); left to thetypecheck-pyrightCI job.Checklist
[module] type: descriptionformat(labels are derived from it automatically — no need to add them by hand)
ruff check .andruff format --check .passty check --error-on-warning osmosis_ai/passes;uv run --no-editable pyright --verifytypes osmosis_ai --ignoreexternalhas no unexpected errors (seeCONTRIBUTING.mdfor the Harbor baseline)pytestpasses (new tests added if applicable)