fix(tb): scoring tmpfs, redaction, passed flag, cap-drop - #60
Merged
Conversation
Regression guard for the compose long-syntax tmpfs.mode decimal footgun that made /tmp mode 3361 and blocked uid 1000 Terminal-Bench builds.
Compose long-syntax tmpfs.mode: 1777 is decimal (octal 3361). Short syntax /tmp:size=256m,mode=1777 matches the validator compose and yields sticky world-writable /tmp for the master validator process.
…ng secrets Lock both directions: count-dataset-tokens stays verbatim and genuine credential shapes remain scrubbed from public task-event text.
Match secret/token as hyphen-delimited segments and shield the known Terminal-Bench task-id catalog so ids like count-dataset-tokens are not replaced with [REDACTED_SECRET] in public task events.
Contract that score 0.0 serializes passed=false with status completed, score 1.0 serializes passed=true, and terminal event metadata carries the same flag when score is present.
Additive boolean derived from score >= 1.0 (same rule as passed_tasks). Lifecycle status stays completed/failed for compatibility; consumers no longer treat a zero-score completed task as a pass.
Flip isolation invariants so own_runner task containers assert cap-drop is absent while no-new-privileges, pids-limit, and tmpfs remain.
…ucceed Deliberate isolation trade-off already running in prod as a hotpatch: own_runner task guests need capabilities for apt/chmod inside verifiers. Master compose cap_drop ALL is unchanged. Keep no-new-privileges, pids limit, /tmp tmpfs, and writable workspace volume.
Narrow tmpfs options to dict[str, Any] before .get so union-attr is clean.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds derived ChangesPublic task API behavior
Task-container capability configuration
Validator tmpfs configuration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/challenges/agent-challenge/tests/test_public_task_event_redaction.py`:
- Around line 26-27: Update the assertions in
test_public_task_event_redaction.py so the identifier-preservation check cannot
be bypassed by the unconditional “count-dataset-tokens” alternative. After the
existing identifier-presence assertion, require that output equals the expected
message and contains no “[REDACTED_SECRET]”, while preserving any intended
event-specific handling established by the surrounding test.
In `@tests/unit/test_docker_compose_deploy.py`:
- Around line 270-277: Update the /tmp mount filtering in the volume-validation
loop to require mount.get("type") == "tmpfs" before evaluating mode and
numeric_ok. Preserve the existing mode fallback and validation behavior for
qualifying tmpfs mounts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e85542b6-fba5-41a5-b3a8-242c5be4ca6f
📒 Files selected for processing (8)
deploy/compose/docker-compose.ymlpackages/challenges/agent-challenge/src/agent_challenge/api/routes.pypackages/challenges/agent-challenge/src/agent_challenge/evaluation/own_runner/container_builder.pypackages/challenges/agent-challenge/tests/test_frontend_api_contract.pypackages/challenges/agent-challenge/tests/test_own_runner_isolation_invariants.pypackages/challenges/agent-challenge/tests/test_public_task_event_redaction.pypackages/challenges/agent-challenge/tests/test_public_task_passed_flag.pytests/unit/test_docker_compose_deploy.py
Comment on lines
+26
to
+27
| assert "[REDACTED_SECRET]" not in out or "count-dataset-tokens" in out | ||
| assert out == message or "count-dataset-tokens" in out |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the identifier-preservation assertion non-tautological.
After Line [24] proves the identifier is present, Line [26] always passes because its right-hand condition is true. The test can therefore accept preserved task text plus an accidental [REDACTED_SECRET].
Proposed fix
- assert "[REDACTED_SECRET]" not in out or "count-dataset-tokens" in out
- assert out == message or "count-dataset-tokens" in out
+ assert out == message📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| assert "[REDACTED_SECRET]" not in out or "count-dataset-tokens" in out | |
| assert out == message or "count-dataset-tokens" in out | |
| assert out == message |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/challenges/agent-challenge/tests/test_public_task_event_redaction.py`
around lines 26 - 27, Update the assertions in
test_public_task_event_redaction.py so the identifier-preservation check cannot
be bypassed by the unconditional “count-dataset-tokens” alternative. After the
existing identifier-presence assertion, require that output equals the expected
message and contains no “[REDACTED_SECRET]”, while preserving any intended
event-specific handling established by the surrounding test.
Comment on lines
+270
to
+277
| for mount in master.get("volumes") or []: | ||
| if not isinstance(mount, dict) or mount.get("target") != "/tmp": | ||
| continue | ||
| raw_tmpfs = mount.get("tmpfs") | ||
| tmpfs_opts: dict[str, Any] = ( | ||
| raw_tmpfs if isinstance(raw_tmpfs, dict) else {} | ||
| ) | ||
| mode = tmpfs_opts.get("mode", mount.get("mode")) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Require the numeric fallback to verify a tmpfs mount.
numeric_ok accepts any /tmp volume with mode 1023; it never checks type == "tmpfs". A future bind or volume mount could therefore make this regression test pass while /tmp is no longer tmpfs.
Suggested fix
for mount in master.get("volumes") or []:
- if not isinstance(mount, dict) or mount.get("target") != "/tmp":
+ if (
+ not isinstance(mount, dict)
+ or mount.get("type") != "tmpfs"
+ or mount.get("target") != "/tmp"
+ ):
continue📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for mount in master.get("volumes") or []: | |
| if not isinstance(mount, dict) or mount.get("target") != "/tmp": | |
| continue | |
| raw_tmpfs = mount.get("tmpfs") | |
| tmpfs_opts: dict[str, Any] = ( | |
| raw_tmpfs if isinstance(raw_tmpfs, dict) else {} | |
| ) | |
| mode = tmpfs_opts.get("mode", mount.get("mode")) | |
| for mount in master.get("volumes") or []: | |
| if ( | |
| not isinstance(mount, dict) | |
| or mount.get("type") != "tmpfs" | |
| or mount.get("target") != "/tmp" | |
| ): | |
| continue | |
| raw_tmpfs = mount.get("tmpfs") | |
| tmpfs_opts: dict[str, Any] = ( | |
| raw_tmpfs if isinstance(raw_tmpfs, dict) else {} | |
| ) | |
| mode = tmpfs_opts.get("mode", mount.get("mode")) |
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 270-270: Do not hardcode temporary file or directory names
Context: "/tmp"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/test_docker_compose_deploy.py` around lines 270 - 277, Update the
/tmp mount filtering in the volume-validation loop to require mount.get("type")
== "tmpfs" before evaluating mode and numeric_ok. Preserve the existing mode
fallback and validation behavior for qualifying tmpfs mounts.
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
/tmptmpfs mode, public secret redaction, additivepassedflag, and own_runner cap-drop trade-off.Defects and fixes
A — compose tmpfs mode (blocker)
Defect:
deploy/compose/docker-compose.ymlused long-syntaxtmpfs.mode: 1777. Compose sends that as a decimal integer to the Docker API, so runtime mode is3361(octal), producingd-wxrwS--t. The validator runs as uid 1000 and cannot write/tmp. Terminal-Bench’s legacydocker buildneeds a temp context under/tmp→ContainerBuildError→ trial crash.Empirical Docker evidence (prod host):
docker run --mount type=tmpfs,...,tmpfs-mode=1777drwxrwxrwtCORRECT (swarm_backend.pyleft untouched)mode: 1777mode=3361— reproduces prodtmpfs: ["/tmp:size=256m,mode=1777"]WRITE_OKas uid 1000Fix: short syntax matching
docker-compose.validator.yml, with a comment that long-syntaxmode: 1777is a decimal footgun.B — over-broad secret redaction (security-sensitive)
Defect:
_public_task_event_texttreated-as part of the identifier class, socount-dataset-tokenswas fully replaced with[REDACTED_SECRET]in public task events whiletask_idstayed correct.Fix: segment-boundary matching for
secret/token/… plus a shield overTERMINAL_BENCH_2_1_FALLBACK_TASK_IDS. Real credential shapes still redact.Security note: existing contract suites remain green:
test_frontend_api_contract.pytest_submission_status.py(fixtures include
sk-test-secret,Bearer raw-provider-token,broker-token,tb21-platform-sdk-secret,lease-worker-secret,broker-ref-secret,raw-ref-*).C — lifecycle
completedpresented as passedDefect: summary
passed_taskswas correct, but per-task rows only exposed lifecyclestatus: "completed"+ score, so UIs treated score 0 as a pass.Fix: additive boolean
passedon public task results/rows and terminal event metadata when score is present, using the same rule aspassed_tasks(score >= 1.0). Does not renametask.completed, rewrite DB rows, or change lifecyclestatus.D — upstream prod cap-drop hotpatch
Defect: prod scoring depends on a bind-mounted hotpatch that omits
--cap-drop ALLfrom own_runner task guests (apt/chmod need caps). Image recreate without the hotpatch silently breaks scoring again.Fix: port into
hardening_run_args()— omit only--cap-drop ALLfor task guests. Keepno-new-privileges, pids limit,/tmptmpfs, workspace volume, and writable rootfs. Do not touch master composecap_drop: ALL.Isolation trade-off: deliberate and already running in prod as a hotpatch; reviewers should treat this as intentional, not a regression of master hardening.
Test plan
uv run pytest tests/unit/test_docker_compose_deploy.py -quv run --package agent-challenge pytest packages/challenges/agent-challenge/tests/test_frontend_api_contract.py packages/challenges/agent-challenge/tests/test_submission_status.py packages/challenges/agent-challenge/tests/test_own_runner_isolation_invariants.py packages/challenges/agent-challenge/tests/test_public_task_event_redaction.py packages/challenges/agent-challenge/tests/test_public_task_passed_flag.py -quv run ruff check/ruff format --checkon touched Python pathsCI
Notes
src/base/master/swarm_backend.py(verified correct octal path).keyrelease/, prod hosts, or historical evaluation rows.Summary by CodeRabbit
passedflag derived from task scores./tmptmpfs permissions to ensure sticky, world-writable behavior.cap-drop ALLfrom task execution.passedbehavior, redaction, tmpfs configuration, and updated isolation expectations.