diff --git a/Cargo.lock b/Cargo.lock index 7d5e80b..8ee5d08 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1780,7 +1780,7 @@ dependencies = [ [[package]] name = "proofline-cli" -version = "0.1.6" +version = "0.1.7" dependencies = [ "anyhow", "axum", diff --git a/Cargo.toml b/Cargo.toml index 9531048..60e3b72 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "proofline-cli" -version = "0.1.6" +version = "0.1.7" edition = "2024" rust-version = "1.95" description = "Proofline command-line client for the review API" diff --git a/README.md b/README.md index 917af23..f14c3a3 100644 --- a/README.md +++ b/README.md @@ -5,6 +5,13 @@ The command-line client for the [Proofline review API](https://proofline.sh/docs Install with `curl -fsSL https://proofline.sh/cli | sh`, then run `proofline login`. See the [CLI documentation](https://proofline.sh/docs/cli/) for commands. +`proofline review --path src/main.rs` reviews only that file's local changes +against the normal merge-base. The path is literal and repository-relative; +directories, globs and `..` are refused. Combine it with `--staged` to read +the index, or `--include-untracked` to select a new, non-ignored file. The +command preserves the real index and applies the usual secret preflight. +It cannot be combined with pushed-commit, range or pull-request review. + Read all recorded runs with `proofline history --repo owner/name --pr 7`. `proofline rerun REVIEW_ID` requests the original immutable subject and waits; `--no-wait` returns its link. Reruns require `reviews:rerun`. diff --git a/src/git.rs b/src/git.rs index 6df5fb8..9f700e4 100644 --- a/src/git.rs +++ b/src/git.rs @@ -41,6 +41,8 @@ pub enum GitError { NotRange(String), #[error("name between 1 and 20 exact repository-relative file paths")] FileCount, + #[error("{0:?} is not one exact repository-relative tracked file; no review was requested")] + InvalidLocalPath(String), #[error("{0:?} is not a regular tracked file at commit {1}; no review was requested")] NotTrackedFile(String, String), } @@ -254,6 +256,7 @@ pub async fn local_changes( dir: &Path, remote: &str, selection: LocalSelection, + path: Option<&str>, ) -> Result { let root = line_in(dir, &["rev-parse", "--show-toplevel"]).await?; let root = Path::new(&root); @@ -289,7 +292,46 @@ pub async fn local_changes( ) .await?; } - diff(root, Some(&index), base_commit_sha, None).await + if let Some(path) = path { + if path.contains('\0') || path.split('/').any(|part| matches!(part, "" | "." | "..")) { + return Err(GitError::InvalidLocalPath(path.to_owned())); + } + let indexed = git_in( + root, + Some(&index), + &["--literal-pathspecs", "ls-files", "-z", "--", path], + ) + .await?; + let base_paths = git_in( + root, + None, + &[ + "--literal-pathspecs", + "ls-tree", + "-r", + "--name-only", + "-z", + &base_commit_sha, + "--", + path, + ], + ) + .await?; + let exact = |entries: &[u8]| { + entries + .split(|byte| *byte == 0) + .any(|entry| entry == path.as_bytes()) + }; + let prefix = format!("{path}/"); + let descendants = indexed + .split(|byte| *byte == 0) + .chain(base_paths.split(|byte| *byte == 0)) + .any(|entry| entry.starts_with(prefix.as_bytes())); + if descendants || (!exact(&indexed) && !exact(&base_paths)) { + return Err(GitError::InvalidLocalPath(path.to_owned())); + } + } + diff(root, Some(&index), base_commit_sha, None, path).await } /// The commits a push sends: `head` against `base`, under the same fixed @@ -298,7 +340,7 @@ pub async fn local_changes( pub async fn range_changes(dir: &Path, base: &str, head: &str) -> Result { let root = line_in(dir, &["rev-parse", "--show-toplevel"]).await?; let head = commit_sha(head)?; - diff(Path::new(&root), None, commit_sha(base)?, Some(&head)).await + diff(Path::new(&root), None, commit_sha(base)?, Some(&head), None).await } /// The base of a branch the remote does not have yet: its merge-base with @@ -315,8 +357,10 @@ async fn diff( index: Option<&Path>, base_commit_sha: String, head: Option<&str>, + path: Option<&str>, ) -> Result { let mut args = vec![ + "--literal-pathspecs", "-c", "core.quotePath=false", // The server accepts renames and refuses copies, whatever the @@ -347,6 +391,7 @@ async fn diff( ]); args.extend(head); args.push("--"); + args.extend(path); let output = git_in(root, index, &args).await?; let (paths, patch) = diff_paths(&output)?; if patch.len() > MAX_PATCH_BYTES { @@ -628,7 +673,7 @@ mod tests { ], ); - let unchanged = local_changes(dir, "origin", LocalSelection::Tracked) + let unchanged = local_changes(dir, "origin", LocalSelection::Tracked, None) .await .unwrap(); assert_eq!( @@ -668,7 +713,7 @@ mod tests { ], ), ] { - let changes = local_changes(dir, "origin", selection).await.unwrap(); + let changes = local_changes(dir, "origin", selection, None).await.unwrap(); assert_eq!(changes.base_commit_sha, base); let mut expected = vec![ "diff --git a/committed.txt b/committed.txt", diff --git a/src/hooks.rs b/src/hooks.rs index df17a55..3c79631 100644 --- a/src/hooks.rs +++ b/src/hooks.rs @@ -375,7 +375,7 @@ async fn subjects(hook: Hook, here: &Path, remote: &str) -> anyhow::Result<(Vec< Hook::PreCommit => Ok(( vec![Subject { label: "the staged changes".to_owned(), - changes: git::local_changes(here, remote, LocalSelection::Staged).await, + changes: git::local_changes(here, remote, LocalSelection::Staged, None).await, }], 0, )), diff --git a/src/lib.rs b/src/lib.rs index 352f36f..6096711 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -267,6 +267,9 @@ struct ReviewArgs { conflicts_with_all = ["pr", "staged", "include_untracked", "allow_path"] )] range: Option, + /// Review only this literal repository-relative file in the local patch. + #[arg(long, value_name = "PATH", conflicts_with_all = ["pr", "commit", "range", "files"])] + path: Option, /// Re-verify these open findings of one pull request against the local /// changes, and print each one's outcome. The outcome is local: the pull /// request's findings are unchanged. Needs an API token. @@ -804,6 +807,7 @@ async fn review_local(api: &Api, args: &ReviewArgs) -> anyhow::Result { std::path::Path::new("."), &args.repository.remote, selection, + args.path.as_deref(), ) .await?; let files = changes.files(); diff --git a/tests/local_patch.rs b/tests/local_patch.rs index 6704439..5c704a7 100644 --- a/tests/local_patch.rs +++ b/tests/local_patch.rs @@ -786,3 +786,111 @@ async fn a_local_override_still_requires_repository_authorization() { assert_eq!(checkout.index().await, index); server.close().await; } + +#[tokio::test] +async fn local_path_selects_one_literal_file_and_preserves_the_index() { + let checkout = Checkout::new(&[("[file].txt", "base\n"), ("f.txt", "other\n")]).await; + checkout.write("[file].txt", "staged\n").await; + checkout.git(&["add", "--all"]).await; + checkout.write("[file].txt", "working\n").await; + checkout.write("f.txt", "excluded\n").await; + let index = checkout.index().await; + for (flags, expected) in [ + (vec!["--path", "[file].txt"], "+working"), + (vec!["--path", "[file].txt", "--staged"], "+staged"), + ] { + let server = Server::start(true).await; + let output = checkout.review(&server, &[], &flags).await; + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + { + let uploads = server.fixture.uploads.lock().unwrap(); + assert_eq!(uploads.len(), 1); + assert_eq!(uploads[0]["base_commit_sha"], checkout.base); + let patch = uploads[0]["patch"].as_str().unwrap(); + assert!(patch.contains(expected), "{patch}"); + assert!(!patch.contains("excluded"), "{patch}"); + assert_eq!(patch.matches("diff --git ").count(), 1); + } + assert_eq!(checkout.index().await, index); + server.close().await; + } +} + +#[tokio::test] +async fn local_path_refuses_directory_glob_and_traversal_before_http() { + let checkout = Checkout::new(&[("nested/file.txt", "base\n")]).await; + checkout.write("nested/file.txt", "changed\n").await; + let server = Server::start(true).await; + for path in [ + "nested", + "nested/*.txt", + "../file.txt", + "nested/../nested/file.txt", + "missing.txt", + ] { + let output = checkout.review(&server, &[], &["--path", path]).await; + assert!(!output.status.success(), "{path}"); + assert!( + String::from_utf8_lossy(&output.stderr) + .contains("exact repository-relative tracked file") + ); + } + server.assert_no_requests(); + server.close().await; +} + +#[tokio::test] +async fn local_path_keeps_secret_preflight_and_can_select_a_deleted_file() { + let checkout = Checkout::new(&[("selected.txt", "base\n"), ("other.txt", "base\n")]).await; + let server = Server::start(true).await; + let secret = "-----BEGIN PRIVATE KEY-----"; + checkout.write("selected.txt", secret).await; + let refused = checkout + .review(&server, &[], &["--path", "selected.txt"]) + .await; + assert!(!refused.status.success()); + assert!(!String::from_utf8_lossy(&refused.stderr).contains(secret)); + server.assert_no_requests(); + tokio::fs::remove_file(checkout.root().join("selected.txt")) + .await + .unwrap(); + checkout.write("other.txt", "excluded\n").await; + let result = checkout + .review(&server, &[], &["--path", "selected.txt"]) + .await; + assert!( + result.status.success(), + "{}", + String::from_utf8_lossy(&result.stderr) + ); + { + let uploads = server.fixture.uploads.lock().unwrap(); + let patch = uploads[0]["patch"].as_str().unwrap(); + assert!(patch.contains("deleted file mode")); + assert!(!patch.contains("excluded")); + } + server.close().await; +} + +#[tokio::test] +async fn local_path_never_expands_a_file_replaced_by_a_directory() { + let checkout = Checkout::new(&[("selected", "base\n")]).await; + tokio::fs::remove_file(checkout.root().join("selected")) + .await + .unwrap(); + checkout + .write("selected/other.txt", "must not leave the checkout\n") + .await; + checkout.git(&["add", "--all"]).await; + let index = checkout.index().await; + let server = Server::start(true).await; + let output = checkout.review(&server, &[], &["--path", "selected"]).await; + assert!(!output.status.success()); + server.assert_no_requests(); + assert_eq!(checkout.index().await, index); + server.close().await; +}