Honor server-provided repository head overrides - #13718
Conversation
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR adds pinned repository baseline support for eval runs, including hidden CLI parsing, environment baseline validation, exact shallow detached checkouts, baseline reporting, remote removal after setup, and snapshot patching relative to pinned commits.
Concerns
- No blocking correctness, security, or spec-alignment concerns found in the annotated diff.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR adds pinned repository baselines to agent environment setup, wires baseline reporting through the harness-support client, and changes end-of-run snapshots to diff pinned repos against their server-provided baseline.
Concerns
- Adding
report_repository_baselinesas a requiredHarnessSupportClientmethod leaves existing test client implementations outside this diff without the new method, so the test build will fail. report_repository_baselinesis enabled for every task-backed run, but environment prep treats report failures as fatal; that makes ordinary unpinned environments depend on a new endpoint before setup commands can run.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| /// artifacts already reported via `report_artifact`. | ||
| async fn finish_task(&self, success: bool, summary: &str) -> Result<()>; | ||
| /// Report verified initial repository state before environment setup begins. | ||
| async fn report_repository_baselines(&self, repositories: &[RepositoryBaseline]) -> Result<()>; |
There was a problem hiding this comment.
HarnessSupportClient test impls outside this diff, such as FailingUploadTargetsClient and RecordingClient in checkpoint_coordinator_tests.rs, without an implementation, so cargo test/clippy with tests will fail with E0046.
| cloud_providers: Vec::new(), | ||
| environment: None, | ||
| repository_baselines: args.repository_baselines.clone(), | ||
| report_repository_baselines: args.task_id.is_some(), |
There was a problem hiding this comment.
prepare_environment_impl returns ReportRepositoryBaselines on any reporting failure, ordinary runs now abort before setup if the new endpoint is unavailable or transiently fails. Gate this to pinned runs or make unpinned reporting best-effort.
38b06ec to
cdbb5f5
Compare
| fi | ||
| printf '%s\n' "Cloning repository $repo_name..." | ||
| git clone --filter=tree:0 "$repo_url" "$target" | ||
| } |
There was a problem hiding this comment.
[smell] clone_repo() here is duplicated verbatim from build_parallel_clone_command's clone_repo() (environment.rs:575-585). Consider sharing one implementation instead of copy-pasting.
| }) | ||
| .await | ||
| .unwrap_or(ShellType::Bash) | ||
| } |
There was a problem hiding this comment.
[complexity] Nice new helper, but the two pre-existing inline duplicates of this exact logic in clone_repos (~line 646-649) and clone_repo (~line 693-696) weren't refactored to call it. Since this file is already being touched, worth collapsing all three.
| pub repo_name: String, | ||
| pub commit_sha: String, | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub branch: Option<String>, |
There was a problem hiding this comment.
[correctness] branch is parsed/validated but never consumed — checkout_pinned_repo (environment.rs:423) always fetches by raw commit_sha only. Is this reserved for a follow-up, or should it feed the fetch/checkout logic?
| match entry.kind { | ||
| EntryKind::Repo => { | ||
| repo_index += 1; | ||
| let baseline_sha = repository_baselines.get(Path::new(&entry.path)); |
There was a problem hiding this comment.
[correctness] This matches by exact PathBuf equality against a map keyed by working_dir.join(&baseline.repo_name) (driver.rs), but entry.path comes from the external snapshot-declarations.sh script (warp-agent-docker, not this repo). Any path-normalization difference between the two silently drops the baseline (falls back to git diff HEAD) with no warning logged. Worth confirming the two are guaranteed to match, or adding a fallback/log on miss.
| } | ||
|
|
||
| #[test] | ||
| fn pinned_prepare_command_rejects_shallow_repo_with_history_ref() { |
There was a problem hiding this comment.
[tests] The real-execution tests here (wrong-HEAD, non-shallow, exposed-history) all cover rejection paths. Worth adding one success-path test (correct shallow+detached+single-commit checkout) confirming valid pinned state doesn't false-fail — a trimmed version of this test with one commit + a .git/shallow write, asserting success instead.
cephalonaut
left a comment
There was a problem hiding this comment.
Posted some comments inline, overall looks great!
| format!("sh -c '{escaped_script}'") | ||
| } | ||
|
|
||
| async fn remove_pinned_remotes( |
There was a problem hiding this comment.
This is aggressive, but seems right for benchmarking. I guess we could make it optional if we wanted to make pinning a more generic capability some day.
cdbb5f5 to
25b439b
Compare
25b439b to
d21e62f
Compare
Co-Authored-By: Warp <agent@warp.dev>
d21e62f to
a1a56dc
Compare
Description
Stack 1 of 2. The resolved repository state/reporting follow-up remains #15274 and will be restacked separately.
Accept sparse server-provided repository head overrides for task-backed agent runs and apply them while preparing environment repositories:
COMMIT_SHAresolves to an exact shallow detached checkout.BRANCHperforms a filtered, shallow, single-branch clone of the requested branch.Each supplied repository head override must be unique and declared by the environment. Remotes are removed from overridden repositories after setup so the run cannot expand beyond the prepared repository state. This PR intentionally does not model or report resolved repository baselines.
The hidden CLI contract is
--repository-head-override-json, backed byRepositoryHeadOverrideandRepositoryHeadRef.Linked Issue
N/A
Testing
./script/formatcargo test -p warp_cli(201 passed)cargo test -p warp --lib repository_head_override(2 passed)cargo test -p warp --lib commit_override(5 passed)cargo test -p warp --lib branch_override(1 passed)cargo clippy --workspace --exclude warp_completer --all-targets --tests -- -D warningscargo clippy -p warp_completer --all-targets --tests -- -D warningsI have manually tested my changes locally with
./script/run(not applicable to this headless environment setup path)Agent Mode
CHANGELOG-NONE
Conversation: https://staging.warp.dev/conversation/68b43c0f-5ef2-4aa8-b911-115291bdb7a2
Co-Authored-By: Warp agent@warp.dev