fix(research): isolate prediction recovery artifacts - #75
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe harness adds symlink-resistant directory validation, tempfile-based atomic publication, isolated snapshot compilation and extraction per retry, empty-results preconditions, and atomic persistence of compiler and runner evidence. Documentation records the retry and resume requirements. ChangesRetry-safe artifact handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PredictionRunner
participant SnapshotCompiler
participant TemporarySnapshot
participant ArtifactStore
PredictionRunner->>PredictionRunner: validate workspace and empty results
PredictionRunner->>TemporarySnapshot: extract snapshot into fresh private directory
SnapshotCompiler->>TemporarySnapshot: compile isolated snapshot
SnapshotCompiler->>ArtifactStore: persist compiler evidence atomically
PredictionRunner->>ArtifactStore: persist runner evidence atomically
🚥 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 |
| fn create_bundle_replaces_a_stale_symlink_without_following_it() { | ||
| use std::os::unix::fs::symlink; | ||
|
|
||
| let root = std::env::temp_dir().join(format!( |
| fn publish_result_rejects_a_symlinked_parent_directory() { | ||
| use std::os::unix::fs::symlink; | ||
|
|
||
| let root = std::env::temp_dir().join(format!( |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@rust_hft/alpha-harness/app/src/data_mission.rs`:
- Around line 334-380: The path validation in ensure_real_directory_at and
temporary_output_file is vulnerable to ancestor replacement after validation.
Replace path-based create_dir, tempfile_in, and persistence operations with held
directory handles and descriptor-relative, no-follow creation/rename semantics,
preserving the validated-tree boundary; add a deterministic counterexample test
that swaps an ancestor with a symlink between validation and use.
In `@rust_hft/alpha-harness/app/src/mission_runner.rs`:
- Line 405: Update the create_new result-publishing flow to stage and sync the
bundle under a private temporary name, then atomically publish it to the final
destination with no-overwrite semantics. Ensure interruptions during copying
never expose the final path, while preserving retryability after failure. Add a
targeted interruption counterexample test asserting the final destination
remains absent and a subsequent retry succeeds.
In `@rust_hft/alpha-harness/README.md`:
- Around line 131-133: Update the README description of harness verification to
state that resumed execution verifies three hashes: mission, snapshot, and
resume-bundle. Replace the inaccurate “both outer hashes” wording while
preserving the surrounding behavior and retry/artifact details.
🪄 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
Run ID: e1a9f79b-0b0a-4afc-97c7-64546ec5fa13
⛔ Files ignored due to path filters (1)
rust_hft/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
rust_hft/alpha-harness/README.mdrust_hft/alpha-harness/app/Cargo.tomlrust_hft/alpha-harness/app/src/data_mission.rsrust_hft/alpha-harness/app/src/mission_runner.rsrust_hft/alpha-harness/app/src/prediction_runner.rsrust_hft/alpha-harness/app/src/prediction_snapshot.rs
Comment on lines
+334
to
+380
| if let Some(parent) = path.parent().filter(|parent| *parent != path) { | ||
| ensure_real_directory_at(parent, label)?; | ||
| } | ||
| let temporary = path.with_extension("tmp"); | ||
| std::fs::write(&temporary, serde_json::to_vec_pretty(value)?)?; | ||
| std::fs::rename(temporary, path)?; | ||
| let metadata = match std::fs::symlink_metadata(path) { | ||
| Ok(metadata) => metadata, | ||
| Err(error) if error.kind() == std::io::ErrorKind::NotFound => { | ||
| match std::fs::create_dir(path) { | ||
| Ok(()) => {} | ||
| Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => {} | ||
| Err(error) => { | ||
| return Err(error) | ||
| .with_context(|| format!("create {label} directory {}", path.display())) | ||
| } | ||
| } | ||
| std::fs::symlink_metadata(path) | ||
| .with_context(|| format!("inspect {label} directory {}", path.display()))? | ||
| } | ||
| Err(error) => { | ||
| return Err(error) | ||
| .with_context(|| format!("inspect {label} directory {}", path.display())) | ||
| } | ||
| }; | ||
| if metadata.file_type().is_symlink() { | ||
| bail!( | ||
| "{label} directory cannot be a symbolic link: {}", | ||
| path.display() | ||
| ); | ||
| } | ||
| if !metadata.is_dir() { | ||
| bail!("{label} path must be a directory: {}", path.display()); | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| pub(crate) fn temporary_output_file( | ||
| path: &Path, | ||
| prefix: &str, | ||
| ) -> anyhow::Result<tempfile::NamedTempFile> { | ||
| let parent = path | ||
| .parent() | ||
| .filter(|parent| !parent.as_os_str().is_empty()) | ||
| .unwrap_or_else(|| Path::new(".")); | ||
| ensure_real_directory(parent, "temporary output parent")?; | ||
| tempfile::Builder::new() | ||
| .prefix(prefix) | ||
| .tempfile_in(parent) | ||
| .with_context(|| format!("create private temporary output in {}", parent.display())) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Close the symlink check/use race.
The directory walk validates paths, then later performs create_dir, tempfile_in, and persistence through those paths. A writable ancestor can be replaced with a symlink between these operations, redirecting publication outside the validated tree.
Use held directory handles with descriptor-relative, no-follow creation/rename semantics, and add a deterministic ancestor-swap counterexample test.
As per coding guidelines, “Every safety boundary requires a targeted counterexample test.”
Also applies to: 394-399, 443-467
🤖 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 `@rust_hft/alpha-harness/app/src/data_mission.rs` around lines 334 - 380, The
path validation in ensure_real_directory_at and temporary_output_file is
vulnerable to ancestor replacement after validation. Replace path-based
create_dir, tempfile_in, and persistence operations with held directory handles
and descriptor-relative, no-follow creation/rename semantics, preserving the
validated-tree boundary; add a deterministic counterexample test that swaps an
ancestor with a symlink between validation and use.
Source: Coding guidelines
proerror77
force-pushed
the
codex/prediction-recovery-isolation
branch
from
July 16, 2026 16:30
5a18d40 to
b67a8e2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rust_hft/alpha-harness/app/src/mission_runner.rs (1)
509-514: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
tempfile::tempdir()for secure temporary directory creation.Using
std::env::temp_dir()with a predictably formatted name triggers static analysis warnings due to potential predictable temp path vulnerabilities in shared environments. More importantly, it requires manual cleanup which can leak the directory if a test panics. Both sites can be refactored to usetempfile::tempdir()which securely creates a uniquely named directory and automatically cleans it up via RAII (as is already done inpublish_result_does_not_leave_a_destination_when_the_bundle_is_missing).
rust_hft/alpha-harness/app/src/mission_runner.rs#L509-L514: Replace the manual root path andcreate_dir_allwithlet root = tempfile::tempdir().unwrap();(and update subsequent path usages toroot.path().join(...), removing the manualremove_dir_allat the end).rust_hft/alpha-harness/app/src/mission_runner.rs#L557-L562: Apply the same refactoring for the parent symlink test.🤖 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 `@rust_hft/alpha-harness/app/src/mission_runner.rs` around lines 509 - 514, Replace manual temporary-directory creation in both mission_runner.rs sites (lines 509-514 and 557-562) with tempfile::tempdir(), update subsequent joins to use root.path(), and remove the corresponding manual remove_dir_all cleanup so RAII handles cleanup on test failure or panic.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@rust_hft/alpha-harness/app/src/mission_runner.rs`:
- Around line 509-514: Replace manual temporary-directory creation in both
mission_runner.rs sites (lines 509-514 and 557-562) with tempfile::tempdir(),
update subsequent joins to use root.path(), and remove the corresponding manual
remove_dir_all cleanup so RAII handles cleanup on test failure or panic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: de408b4c-0cf9-4e1b-a6e0-1248ecc83555
⛔ Files ignored due to path filters (1)
rust_hft/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
rust_hft/alpha-harness/README.mdrust_hft/alpha-harness/app/Cargo.tomlrust_hft/alpha-harness/app/src/data_mission.rsrust_hft/alpha-harness/app/src/mission_runner.rsrust_hft/alpha-harness/app/src/prediction_runner.rsrust_hft/alpha-harness/app/src/prediction_snapshot.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- rust_hft/alpha-harness/app/Cargo.toml
- rust_hft/alpha-harness/app/src/prediction_runner.rs
- rust_hft/alpha-harness/README.md
- rust_hft/alpha-harness/app/src/data_mission.rs
- rust_hft/alpha-harness/app/src/prediction_snapshot.rs
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
2 participants
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.
Change contract
Prediction snapshot and runner retries never trust prior writable extraction or output state. Symlinked artifact leaves are rejected before a compiler or runner starts; JSON evidence rechecks its output leaf at publication. Local file results are staged, synced, and atomically published without overwrite.
Out of scope
Dependency / merge order
None. Rebased on current
mainat1ec9add1.Focused validation
cargo test -p alpha-harness -- --test-threads=1(62 passed)cargo clippy -p alpha-harness --all-targets -- -D warningscargo fmt --check --package alpha-harnessgit diff --checkRollout / rollback
Research-only CLI and artifact-path behavior. Roll back this one commit to restore the former retry/output behavior; no live execution path changes.
Summary by CodeRabbit