fix(review): record escaping workspace symlinks as evidence exclusions - #1130
bingran-you wants to merge 2 commits into
Conversation
A symlink that resolves outside the solver workspace, such as codex-acp's /app/apply_patch helper link, made evidence capture raise. The trial then ended in a scoring error with rewards null although the agent and verifier had finished (FrontierPhysics PR #192). Capture now keeps a link only when both its resolution and its target text stay inside the workspace, the rule host extraction and reviewer admission already enforce. Any other link, including a loop or a link whose text climbs out with .., is recorded as a symlink_escape exclusion with its link_target, and no outside file is copied. The host check and the stdlib admission check both reject a manifest that captures and excludes the same path.
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}
There was a problem hiding this comment.
Devin Review found 1 potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| if not kept_link(path, target): | ||
| record(path, "symlink_escape", link_target=target) |
There was a problem hiding this comment.
🟡 Task evidence entry limit bypass
When escaping links accompany external artifacts, max_entries permits more evidence records than configured. capture_task_evidence omits exclusions when budgeting artifacts, so large exclusion sets bypass the task-wide limit.
Learn more
Each capture_workspace call counts entries and exclusions against max_entries. The enclosing task capture then computes the artifact budget using only len(manifest.entries). Every symlink_escape recorded here therefore disappears from the cumulative budget, letting later external artifacts consume those slots again. The published task bundle can exceed its configured maximum even though each individual capture passed.
Example: With max_entries=3, a workspace containing one file and one escaping symlink consumes two slots. The artifact budget is incorrectly set to two, so an external two-file artifact succeeds and publishes four total entry/exclusion records.
Recommended fix: In capture_task_evidence, subtract both len(manifest.entries) and len(manifest.exclusions) from remaining_entries. Apply the same subtraction after every external artifact capture, and add a regression test combining exclusions with external artifacts at the boundary.
Was this helpful? React with 👍 or 👎 to provide feedback.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9581599945
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "task_exclude", | ||
| ] | ||
| # The link's own text, recorded for a symlink the bundle cannot keep. | ||
| link_target: str | None = None |
There was a problem hiding this comment.
When a new build records any existing exclusion type such as credential, sandbox_runtime, special_file, or task_exclude, Pydantic now serializes it with "link_target": null. Pre-change readers define EvidenceExclusion with extra="forbid", so an older bench eval score rejects these manifests even when they contain no symlink_escape record. This makes the compatibility break broader than intended and changes capture output for ordinary exclusions; omit the field when it is None or version the schema accordingly.
Useful? React with 👍 / 👎.
1 participant
Problem
The in-run rubric review from #1126 captures the solver workspace with
_CAPTURE_SCRIPT(review/evidence.py). Before this PR the script raisedworkspace symlink escapes evidenceon any symlink that resolved outside the workspace root. That madecapture_terminal_workspaceset_export_error, andfinish_reviewthen refused to review. The trial ended in a scoring error withrewards: null, even though the agent and verifier had finished.This happened on FrontierPhysics PR #192 (2026-09-15): GPT-6 Astra on codex-acp left
/app/apply_patchlinked to codex's helper outside/app, and capture aborted on it. The FIFO variant (/root/.daytona/sessions/.../input.pipe) and.daytonaruntime state are already handled by #1128, which merged as 6614f92; this PR builds on it.A second, related gap: a link whose resolution stays inside the root but whose target text climbs out (
/app/sub/../answer.txt, or../app/x) passed capture and then failed host extraction._link_targetrejects any..left after normalization, so these also discarded the review.Change
_extract_archive,validate_workspaceand the reviewer's_UNPACK_SCRIPTand_ADMISSION_SCRIPTalready enforce. Any other link, including a loop, becomes asymlink_escapeexclusion that records the link's ownlink_target. Dangling links that point inside are still kept, and no outside file is ever copied.EvidenceExclusion.reasongainssymlink_escape, and there is a new optionallink_target. A validator requireslink_targetexactly forsymlink_escape.validate_workspaceand_ADMISSION_SCRIPTboth reject a manifest that captures and excludes the same path. The admission script also requireslink_targeton, and only on,symlink_escaperecords, so the stdlib check applies the same exclusion rules as the host.docs/rubric-review.mdlists the new reason and the link rule. The CHANGELOG entry sits next to fix(review): keep Daytona session state and special files out of evidence capture #1128's under 0.7.8 (untagged); fix(review): keep Daytona session state and special files out of evidence capture #1128's line saying escaping symlinks still fail closed is corrected.Should a capture failure fall back to reviewing the rollout copy?
I recommend not doing that silently, so this PR does not implement it.
/evidence/trialalone has no manifest-verified workspace. Blockers that must inspect the submitted files (fabrication, artifacts present, implementation) would be judged from the trajectory and verifier copies, and the result would still be acompletescore.ScoringResulthas no field to record that weaker evidence mode (extra="forbid", schema version 1). Those scores would not be comparable with normally captured ones.evidence: rollout_copyin a v2 scoring block, which reports can exclude from pass@1.bench eval scorecannot retry after the solver sandbox is gone, because the workspace cannot be recaptured. That makes robust capture the more important fix.Compatibility
link_targetdefaults toNone.symlink_escapewould reject it (for example,bench eval scorerun with a pre-fix build on a rollout captured by this one).Tests
test_escaping_links_and_fifos_are_exclusions_through_reviewer_admission. One workspace holds a FIFO, an absolute outside link, a relative../link, a dangling outside link, a link through an escaping link, a text-detour link that resolves inside, a loop, a kept internal link and a kept dangling link. It checks capture,validate_workspace, packaging,_UNPACK_SCRIPTand_ADMISSION_SCRIPT.test_host_and_reviewer_admission_reject_inconsistent_exclusions. It checks the overlap rule and the link-target rule on the host and in the admission script.test_agent_helper_link_outside_workspace_keeps_terminal_evidence_scorable. Terminal capture reproduces the PR 14 infrastructure bugs found in SkillsBench Trial 1 audit — verifier, permissions, provisioning #192apply_patchcase:_export_errorstaysNoneand the manifest records the exclusion.test_external_symlink_never_imports_outside_fileskeeps its PR feat(review): rubric.json post-verify agentic review #942 guard, that no outside bytes enter the bundle, while asserting the exclusion instead of a failure.test_external_artifact_failure_does_not_publish_partial_task_evidencenow triggers the failure with an over-budget artifact, because its escaping link is no longer a failure. The atomicity assertion is unchanged.The four new or updated escaping-link tests fail on #1128's head and pass here.
Validation
uv run python -m pytest tests/: 6447 passed, 60 skipped (rebased on 6614f92).ruff check .,ruff format --check src tests toolsandty check src/pass.