diff --git a/Cargo.lock b/Cargo.lock index 845f084..63a1dc3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2,6 +2,15 @@ # It is not intended for manual editing. version = 4 +[[package]] +name = "android_system_properties" +version = "0.1.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ae221649c9976a6f6c56ae1facf410f3ddb33cc661c4b7b61020a912d4237fbc" +dependencies = [ + "libc", +] + [[package]] name = "anstream" version = "1.0.0" @@ -64,6 +73,12 @@ version = "1.1.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1505bd5d3d116872e7271a6d4e16d81d0c8570876c8de68093a09ac269d8aac0" +[[package]] +name = "autocfg" +version = "1.5.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f2032f911046de80f0a198e0901378627c33f59ea0ac00e363d481118bd70a53" + [[package]] name = "base64" version = "0.22.1" @@ -110,6 +125,17 @@ version = "0.2.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f079e83a288787bcd14a6aea84cee5c87a67c5a3e660c30f557a3d24761b3527" +[[package]] +name = "chrono" +version = "0.4.45" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1aa79e62e7697b8e29b513a68abacf485adcd1fe8284a4316c5ae868e6633327" +dependencies = [ + "iana-time-zone", + "num-traits", + "windows-link", +] + [[package]] name = "clap" version = "4.6.6" @@ -180,6 +206,7 @@ name = "commitor-cli" version = "0.4.0" dependencies = [ "anyhow", + "chrono", "clap", "reqwest", "rustyline", @@ -544,6 +571,30 @@ dependencies = [ "windows-registry", ] +[[package]] +name = "iana-time-zone" +version = "0.1.65" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e31bc9ad994ba00e440a8aa5c9ef0ec67d5cb5e5cb0cc7f8b744a35b389cc470" +dependencies = [ + "android_system_properties", + "core-foundation-sys", + "iana-time-zone-haiku", + "js-sys", + "log", + "wasm-bindgen", + "windows-core", +] + +[[package]] +name = "iana-time-zone-haiku" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f31827a206f56af32e590ba56d5d2d085f558508192593743f16b2306495269f" +dependencies = [ + "cc", +] + [[package]] name = "icu_collections" version = "2.3.0" @@ -827,6 +878,15 @@ dependencies = [ "libc", ] +[[package]] +name = "num-traits" +version = "0.2.19" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "071dfc062690e90b734c0b2273ce72ad0ffa95f0c74596bc250dcfd960262841" +dependencies = [ + "autocfg", +] + [[package]] name = "objc2" version = "0.6.4" @@ -1425,9 +1485,21 @@ dependencies = [ "mio", "pin-project-lite", "socket2", + "tokio-macros", "windows-sys 0.61.2", ] +[[package]] +name = "tokio-macros" +version = "2.7.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "78773a2a397f451582ce068015985c33193cf6dea8b74d2a639fe457b2f07b0e" +dependencies = [ + "proc-macro2", + "quote", + "syn 3.0.3", +] + [[package]] name = "tokio-native-tls" version = "0.3.1" @@ -1742,6 +1814,41 @@ dependencies = [ "windows-sys 0.61.2", ] +[[package]] +name = "windows-core" +version = "0.62.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b8e83a14d34d0623b51dce9581199302a221863196a1dde71a7663a4c2be9deb" +dependencies = [ + "windows-implement", + "windows-interface", + "windows-link", + "windows-result", + "windows-strings", +] + +[[package]] +name = "windows-implement" +version = "0.60.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "053e2e040ab57b9dc951b72c264860db7eb3b0200ba345b4e4c3b14f67855ddf" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.119", +] + +[[package]] +name = "windows-interface" +version = "0.59.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3f316c4a2570ba26bbec722032c4099d8c8bc095efccdc15688708623367e358" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.119", +] + [[package]] name = "windows-link" version = "0.2.1" diff --git a/action/README.md b/action/README.md new file mode 100644 index 0000000..7505684 --- /dev/null +++ b/action/README.md @@ -0,0 +1,99 @@ +# commitor-action + +A GitHub Action that runs [Commitor](https://github.com/Commitor-AI/commitor) +against a pull request's diff and posts the result as a PR comment — +updating the previous comment instead of posting a new one on every push. + +Commitor catches unrelated changes bundled into a single commit/PR and +tells you how to split them. This action points the existing `scan` +engine at the PR range (`origin/...HEAD`) and renders the verdict +as compact GitHub-flavored Markdown. + +## Quick start + +Add this to `.github/workflows/commitor.yml`: + +```yaml +on: pull_request + +jobs: + commitor: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + # Full history is required: a shallow clone can't diff against + # the base branch, so `origin/...HEAD` would resolve to + # nothing (or the wrong thing). fetch-depth: 0 prevents that. + fetch-depth: 0 + + - uses: Commitor-AI/commitor-action@v1 + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + commitor-api-key: ${{ secrets.COMMITOR_API_KEY }} +``` + +> If you keep this action as a subdirectory of the Commitor repo, point +> `uses` at it directly instead: `uses: ./action` (or +> `uses: Commitor-AI/commitor/action@v1`). The published form above +> assumes the action lives in its own repo named `commitor-action`. + +## Setup + +1. **Add the API key secret.** Get a key from your Commitor dashboard, + then add it as a repository secret named `COMMITOR_API_KEY` + (`Settings → Secrets and variables → Actions`). The action passes it + to `commitor login --key`. + +2. **Don't forget `fetch-depth: 0`.** The action diffs `HEAD` against + `origin/` using a symmetric range. With the default + shallow checkout (`fetch-depth: 1`), the base branch's history isn't + present, so the diff range comes back empty and Commitor reports + "No changes to analyze." `fetch-depth: 0` (or an explicit + `git fetch` of the base branch) avoids that. + +## Inputs + +| Input | Required | Default | Description | +| ------------------- | -------- | ----------------- | ----------- | +| `github-token` | no | `${{ github.token }}` | Token used to read/update PR comments. | +| `commitor-api-key` | **yes** | — | Commitor API key (from the dashboard), as a repo secret. | +| `strict` | no | `false` | If `true`, fail the check when Commitor reports a mixed PR. | +| `api-url` | no | *(production)* | Override for the Commitor backend URL. | +| `version` | no | `v0.4.0` | **Pinned** release tag for the binary. Review bumps deliberately. | + +The action forwards the PR's **title and description** to `commitor scan +--context`, so the model can weigh the stated intent of the PR against the +files it actually touches (and never trusts a "clean" local heuristic at +PR scale — `--diff-range` scans always escalate to the backend). + +### Strict mode + +When `strict: true`, the action fails (non-zero exit) if Commitor finds a +mixed PR, so it shows up as a failing status check — useful as a CI gate. +Note `commitor scan --strict` only fails on a *mixed* verdict; a clean or +inconclusive result still passes. + +## Output + +The action posts a Markdown comment containing a hidden +`` marker. On subsequent runs it finds that +marker and **updates** the same comment rather than adding a new one, so +a busy PR doesn't accumulate a wall of Commitor comments. + +## Scope (and what's deliberately *not* here — v2) + + +This action is intentionally minimal: it shells out to the existing +`commitor scan` CLI and posts the result via the REST API. The following +are **deliberate v2 scope decisions**, not oversights: + +- **No GitHub App.** It uses the built-in `GITHUB_TOKEN` plus a user + supplied API key, not a first-party App with its own identity/permissions. +- **No webhook listener.** It runs on the `pull_request` event, not a + standalone webhook server. +- **No Marketplace submission.** It's published as a composite action in + this repo, not submitted to the GitHub Marketplace. + +Promoting any of these to v2 should be a conscious, separately-reviewed +change. diff --git a/action/action.yml b/action/action.yml new file mode 100644 index 0000000..7bdb928 --- /dev/null +++ b/action/action.yml @@ -0,0 +1,161 @@ +# Commitor GitHub Action — PR change-analysis bot. +# +# Usage (see README.md for the full workflow): +# - uses: Commitor-AI/commitor-action@v1 # once published as its own repo +# - uses: ./action # or in-repo (this directory) +# +# The action downloads a pinned commitor binary, logs in with the user's +# API key, runs `commitor scan --diff-range` against the PR base, and +# posts (or updates) a Markdown PR comment with the result. +name: Commitor PR Analysis +description: >- + Analyze a pull request's diff with Commitor and post the result as a + PR comment, updating the previous comment instead of spamming new ones. +author: Commitor-AI +inputs: + github-token: + description: >- + Token used to read/update PR comments via the GitHub REST API. + default: ${{ github.token }} + commitor-api-key: + description: >- + Commitor API key (from the Commitor dashboard). Store it as a repo + secret, e.g. secrets.COMMITOR_API_KEY, and pass it here. Required — + without it the scan can't reach the backend. + required: true + strict: + description: >- + If "true", fail the check when Commitor reports a mixed (split- + worthy) PR, so it shows as a failing status. + default: "false" + required: false + api-url: + description: >- + Optional override for the Commitor backend URL. Leave empty to use + the production default. + default: "" + required: false + version: + description: >- + Pinned commitor release tag to download (e.g. v0.4.0). Always pin + this deliberately — never pull "latest" in CI, so upgrades are a + conscious choice you review in the PR that bumps it. + default: v0.4.0 + required: false +runs: + using: composite + steps: + - name: Download commitor + id: download + shell: bash + env: + VERSION: ${{ inputs.version }} + run: | + set -euo pipefail + case "$RUNNER_OS" in + Linux) ARCH_PART="unknown-linux-gnu" ;; + macOS) ARCH_PART="apple-darwin" ;; + Windows) ARCH_PART="pc-windows-msvc" ;; + *) echo "Unsupported runner OS: $RUNNER_OS" >&2; exit 1 ;; + esac + case "$RUNNER_ARCH" in + X64) ARCH="x86_64" ;; + ARM64) ARCH="aarch64" ;; + *) ARCH="$RUNNER_ARCH" ;; + esac + TRIPLE="${ARCH}-${ARCH_PART}" + EXT="" + if [ "$RUNNER_OS" = "Windows" ]; then EXT=".exe"; fi + ASSET="commitor-${TRIPLE}${EXT}" + + BIN_DIR="$RUNNER_TEMP/commitor-bin" + mkdir -p "$BIN_DIR" + echo "Downloading ${ASSET} (${VERSION})" + gh release download "$VERSION" \ + --repo Commitor-AI/commitor \ + --pattern "$ASSET" \ + --dir "$BIN_DIR" + # The asset keeps its triple-bearing name; rename to a stable + # `commitor` so it resolves on PATH in the later steps. + mv "$BIN_DIR/${ASSET}" "$BIN_DIR/commitor${EXT}" + if [ "$RUNNER_OS" != "Windows" ]; then chmod +x "$BIN_DIR/commitor${EXT}"; fi + # Put the downloaded binary on PATH for the later steps. + echo "$BIN_DIR" >> "$GITHUB_PATH" + + - name: Log in to Commitor + shell: bash + run: commitor login --key "${{ inputs.commitor-api-key }}" + + - name: Analyze PR and post comment + if: ${{ github.event_name == 'pull_request' }} + env: + GH_TOKEN: ${{ inputs.github-token }} + COMMITOR_API_URL: ${{ inputs.api-url }} + PR_NUMBER: ${{ github.event.pull_request.number }} + REPO: ${{ github.repository }} + BASE_REF: ${{ github.base_ref }} + STRICT: ${{ inputs.strict }} + PR_TITLE: ${{ github.event.pull_request.title }} + PR_BODY: ${{ github.event.pull_request.body }} + shell: bash + run: | + set -euo pipefail + RANGE="origin/${BASE_REF}...HEAD" + # Make sure the base ref exists locally so the symmetric diff + # (changes on HEAD since it diverged from base) resolves. + git fetch --no-tags origin "${BASE_REF}" || true + + STRICT_FLAG="" + if [ "$STRICT" = "true" ]; then STRICT_FLAG="--strict"; fi + + # Pass the PR's stated intent (title + description) as context so + # the model can weigh it against the files actually touched. + CONTEXT="$PR_TITLE" + if [ -n "${PR_BODY:-}" ]; then + CONTEXT="${PR_TITLE} + +${PR_BODY}" + fi + + OUTFILE="$(mktemp)" + # Capture both output and exit code; we still want to post the + # comment even if the scan (e.g. strict failure) exits non-zero. + set +e + commitor scan --diff-range "$RANGE" --markdown --context "$CONTEXT" $STRICT_FLAG >"$OUTFILE" 2>&1 + SCAN_RC=$? + set -e + + BODY="$(cat "$OUTFILE")" + + # Find an existing Commitor comment (marked) to update in place, + # so we don't post a fresh comment on every push. + MARKER='' + EXISTING_ID="" + if [ -n "$PR_NUMBER" ]; then + COMMENTS="$(curl -sS -H "Authorization: Bearer $GH_TOKEN" \ + -H "Accept: application/vnd.github+json" \ + "https://api.github.com/repos/$REPO/issues/$PR_NUMBER/comments")" + EXISTING_ID="$(printf '%s' "$COMMENTS" | jq -r --arg m "$MARKER" \ + '.[] | select((.body // "") | contains($m)) | .id' | head -n1)" + fi + + if [ -n "$EXISTING_ID" ]; then + echo "Updating existing Commitor comment ($EXISTING_ID)" + curl -sS -X PATCH -H "Authorization: Bearer $GH_TOKEN" \ + -H "Accept: application/vnd.github+json" \ + -H "Content-Type: application/json" \ + -d "$(jq -n --arg b "$BODY" '{body: $b}')" \ + "https://api.github.com/repos/$REPO/issues/comments/$EXISTING_ID" >/dev/null + else + echo "Posting new Commitor comment" + curl -sS -X POST -H "Authorization: Bearer $GH_TOKEN" \ + -H "Accept: application/vnd.github+json" \ + -H "Content-Type: application/json" \ + -d "$(jq -n --arg b "$BODY" '{body: $b}')" \ + "https://api.github.com/repos/$REPO/issues/$PR_NUMBER/comments" >/dev/null + fi + + if [ "$STRICT" = "true" ] && [ "$SCAN_RC" -ne 0 ]; then + echo "::error::Commitor found a mixed PR and strict mode is enabled — failing the check." + exit 1 + fi diff --git a/crates/cli/Cargo.toml b/crates/cli/Cargo.toml index 72bfe65..dd63773 100644 --- a/crates/cli/Cargo.toml +++ b/crates/cli/Cargo.toml @@ -24,8 +24,9 @@ semver = "1" webbrowser = "1" serde = { version = "1", features = ["derive"] } serde_json = "1" -tokio = { version = "1", features = ["rt"] } +tokio = { version = "1", features = ["rt", "net", "macros", "io-util", "io-std"] } toml = "0.8" +chrono = { version = "0.4", default-features = false, features = ["clock", "std"] } rustyline = "18.0.1" [dev-dependencies] diff --git a/crates/cli/src/analysis.rs b/crates/cli/src/analysis.rs index ee02691..c4f6440 100644 --- a/crates/cli/src/analysis.rs +++ b/crates/cli/src/analysis.rs @@ -163,6 +163,26 @@ fn collect_unstaged() -> Result { }) } +/// Collect the diff for an explicit git range (e.g. `origin/main...HEAD`), +/// used by `scan --diff-range` to analyze a PR/branch window instead of +/// the working tree. Never includes untracked files — that matches +/// `scan`'s existing scope, and a fixed range already defines exactly +/// what to look at, so there is no unstaged/staged flavor to pick. +pub fn collect_range(range: &str) -> Result { + let files = git::changed_files_range(range)?; + let patch = if files.is_empty() { + String::new() + } else { + git::diff_range(range)? + }; + Ok(CollectedDiff { + staged_used: false, + files, + patch, + untracked: Vec::new(), + }) +} + /// Build a synthetic unified-diff section presenting an untracked file /// as a brand-new file, so it flows through the normal wire format and /// hunk parser without mutating the git index (`no git add -N`). @@ -227,28 +247,34 @@ impl RateStatus { } } -/// Run the full backend analysis for `patch`. +/// Run the full backend analysis for `patch`, optionally forwarding +/// PR-scoped context (title/description) to the model. /// /// Loads the stored API key (surfacing the standard not-logged-in /// message when absent), applies the size guard, then POSTs to /// `/analyze`. This is the entry point `scan` uses once its local -/// heuristics are inconclusive. +/// heuristics decide to escalate. /// /// Returns the response plus the quota snapshot from the response /// headers, so commands can warn when the user is close to their /// daily limit. -pub fn analyze_patch(patch: &str) -> Result<(AnalyzeResponse, RateStatus), AnalyzeError> { +pub fn analyze_patch_with_context( + patch: &str, + context: Option<&str>, +) -> Result<(AnalyzeResponse, RateStatus), AnalyzeError> { let api_key = auth::load_api_key()?; - analyze_with_key(&api_key, patch) + analyze_with_key(&api_key, patch, context) } -/// Same as [`analyze_patch`] for callers that already loaded the key -/// (e.g. `commit`, which requires auth before touching any state). +/// Same as [`analyze_patch_with_context`] for callers that already +/// loaded the key (e.g. `commit`, which requires auth before touching +/// any state). pub fn analyze_with_key( api_key: &str, patch: &str, + context: Option<&str>, ) -> Result<(AnalyzeResponse, RateStatus), AnalyzeError> { - analyze_with_mode(api_key, patch, "scan") + analyze_with_mode(api_key, patch, "scan", context) } /// Commit passes `mode = "commit"` so the backend never answers with @@ -257,6 +283,7 @@ pub fn analyze_with_mode( api_key: &str, patch: &str, mode: &str, + context: Option<&str>, ) -> Result<(AnalyzeResponse, RateStatus), AnalyzeError> { enforce_size_guard(api_key, patch)?; @@ -264,7 +291,7 @@ pub fn analyze_with_mode( .enable_all() .build() .map_err(|err| AnalyzeError::Other(anyhow!("failed to start async runtime: {err:#}")))?; - runtime.block_on(analyze_request(api_key, patch, mode)) + runtime.block_on(analyze_request(api_key, patch, mode, context)) } /// POST the diff to `{API_BASE_URL}/analyze`. @@ -272,6 +299,7 @@ async fn analyze_request( api_key: &str, patch: &str, mode: &str, + context: Option<&str>, ) -> Result<(AnalyzeResponse, RateStatus), AnalyzeError> { use reqwest::Client; @@ -283,9 +311,9 @@ async fn analyze_request( .map_err(|err| AnalyzeError::Other(anyhow!("failed to set up HTTP client: {err:#}")))?; let request = auth::with_key( - client.post(&url).json(&AnalyzeRequest { + client.post(&url) .json(&AnalyzeRequest { diff: patch, - context: None, + context, mode: Some(mode), }), api_key, diff --git a/crates/cli/src/auth.rs b/crates/cli/src/auth.rs index 33e3ae6..76a4ef1 100644 --- a/crates/cli/src/auth.rs +++ b/crates/cli/src/auth.rs @@ -10,15 +10,13 @@ //! hosted API ships). use std::fs; -use std::io::{Read, Write}; -use std::net::TcpListener; use std::path::PathBuf; -use std::sync::mpsc; -use std::thread; -use anyhow::{bail, Context, Result}; +use anyhow::{anyhow, bail, Context, Result}; use reqwest::blocking::{Client, RequestBuilder, Response}; use serde::{Deserialize, Serialize}; +use tokio::io::{self, AsyncBufReadExt, AsyncReadExt, AsyncWrite, AsyncWriteExt, BufReader}; +use tokio::net::TcpListener; use crate::admin; use crate::config; @@ -302,64 +300,98 @@ fn save_credentials(api_key: &str) -> Result<()> { /// doesn't support the redirect (or the browser flow is interrupted), the /// user can paste an API key from the dashboard instead. pub fn login_interactive() -> Result<()> { + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .map_err(|err| anyhow!("failed to start async runtime: {err:#}"))?; + runtime.block_on(login_interactive_async()) +} + +/// Async heart of [`login_interactive`]: race the browser callback server +/// against a manual key paste and log in with whichever arrives first. +async fn login_interactive_async() -> Result<()> { let redirect = format!("http://127.0.0.1:{CALLBACK_PORT}/callback"); let login_url = format!("{FRONTEND_URL}/login?redirect={redirect}"); println!("Opening your browser to sign in: {login_url}"); let _ = webbrowser::open(&login_url); - let (tx, rx) = mpsc::channel::(); - - let mut have_server = false; - if let Ok(listener) = TcpListener::bind(("127.0.0.1", CALLBACK_PORT)) { - have_server = true; - let server_tx = tx.clone(); - thread::spawn(move || { - for stream in listener.incoming() { - if let Ok(mut stream) = stream { - let mut buf = [0u8; 4096]; - let _ = stream.read(&mut buf); - let req = String::from_utf8_lossy(&buf); - if let Some(key) = extract_key(&req) { - let _ = write_connected(&mut stream); - let _ = server_tx.send(LoginSource::Callback(key)); - break; - } else { - let _ = write_error( - &mut stream, - "No API key was returned. Run `commitor login` again.", - ); - } - } - } - }); - } else { - println!("(Couldn't start a local callback server; you'll need to paste your key.)"); - } + // Stand up the local callback server when the port is free; otherwise + // fall back to paste-only. + let listener = match TcpListener::bind(("127.0.0.1", CALLBACK_PORT)).await { + Ok(listener) => { + println!("If your browser redirects back here automatically, you're all set."); + Some(listener) + } + Err(_) => { + println!("(Couldn't start a local callback server; you'll need to paste your key.)"); + None + } + }; - if have_server { - println!("If your browser redirects back here automatically, you're all set."); - } println!("Otherwise, paste your API key from {DASHBOARD_URL} and press Enter:"); - let stdin_tx = tx.clone(); - thread::spawn(move || { - let mut line = String::new(); - if std::io::stdin().read_line(&mut line).is_ok() { - let key = line.trim().to_string(); - if !key.is_empty() { - let _ = stdin_tx.send(LoginSource::Manual(key)); - } + // `tokio::select!` races the two sources — the first to yield a key + // wins, and the losing branch is dropped automatically. + let source = match listener { + Some(listener) => tokio::select! { + key = accept_callback(listener) => LoginSource::Callback(key), + key = read_manual_key() => LoginSource::Manual(key), + }, + None => LoginSource::Manual(read_manual_key().await), + }; + + let key = match source { + LoginSource::Callback(k) | LoginSource::Manual(k) => k, + }; + + // `login` performs a blocking network call; keep it off the async + // runtime with `spawn_blocking` so it can't stall other tasks. + tokio::task::spawn_blocking(move || login(&key)) + .await + .map_err(|err| anyhow!("login task failed: {err}"))? +} + +/// Accept a single connection on the callback listener, pull the key out +/// of the redirect, and confirm success to the browser. If no usable key +/// arrives the future never resolves, so the manual-paste branch wins the +/// `select!` race instead. +async fn accept_callback(listener: TcpListener) -> String { + let (mut stream, _) = match listener.accept().await { + Ok(pair) => pair, + Err(_) => std::future::pending().await, + }; + + let mut buf = [0u8; 4096]; + let _ = stream.read(&mut buf).await; + let req = String::from_utf8_lossy(&buf); + + match extract_key(&req) { + Some(key) => { + let _ = write_connected(&mut stream).await; + key } - }); - - match rx.recv() { - Ok(source) => { - let key = match source { - LoginSource::Callback(k) | LoginSource::Manual(k) => k, - }; - login(&key) + None => { + let _ = write_error( + &mut stream, + "No API key was returned. Run `commitor login` again.", + ) + .await; + std::future::pending().await } - Err(_) => bail!("Login was cancelled."), + } +} + +/// Read a single line from stdin as the manually pasted API key. +async fn read_manual_key() -> String { + let mut line = String::new(); + if BufReader::new(io::stdin()) + .read_line(&mut line) + .await + .is_ok() + { + line.trim().to_string() + } else { + String::new() } } @@ -517,18 +549,18 @@ fn page_html( } /// Write a self-contained HTML document back to the browser redirect. -fn write_http(stream: &mut impl Write, html: &str) -> std::io::Result<()> { +async fn write_http(stream: &mut (impl AsyncWrite + Unpin), html: &str) -> std::io::Result<()> { let response = format!( "HTTP/1.1 200 OK\r\nContent-Type: text/html; charset=utf-8\r\nContent-Length: {}\ \r\nConnection: close\r\n\r\n{html}", html.len() ); - stream.write_all(response.as_bytes()) + AsyncWriteExt::write_all(stream, response.as_bytes()).await } /// Page shown when the browser redirect delivered a valid API key: /// the user is connected and can return to the terminal. -fn write_connected(stream: &mut impl Write) -> std::io::Result<()> { +async fn write_connected(stream: &mut (impl AsyncWrite + Unpin)) -> std::io::Result<()> { const SVG: &str = r#""#; let html = page_html( @@ -539,13 +571,13 @@ fn write_connected(stream: &mut impl Write) -> std::io::Result<()> { "rgba(198,255,0,.35)", SVG, ); - write_http(stream, &html) + write_http(stream, &html).await } /// Page shown when the redirect did not carry a key (rare — e.g. the user /// navigated to the callback manually). Keeps the same look, in the /// critical (red) accent. -fn write_error(stream: &mut impl Write, msg: &str) -> std::io::Result<()> { +async fn write_error(stream: &mut (impl AsyncWrite + Unpin), msg: &str) -> std::io::Result<()> { const SVG: &str = r#""#; let html = page_html( @@ -556,7 +588,7 @@ fn write_error(stream: &mut impl Write, msg: &str) -> std::io::Result<()> { "rgba(244,63,94,.30)", SVG, ); - write_http(stream, &html) + write_http(stream, &html).await } /// Remove the stored credentials file, if any. diff --git a/crates/cli/src/commit.rs b/crates/cli/src/commit.rs index aed36c1..d192584 100644 --- a/crates/cli/src/commit.rs +++ b/crates/cli/src/commit.rs @@ -61,6 +61,10 @@ pub fn run(flags: CommitFlags) -> Result { return Ok(ExitCode::SUCCESS); } + // Captured before any `-b` branch switch so we can record what the new + // branch was forked from in the session log. + let starting_branch = git::current_branch()?; + // ── 0. Branch selection (optional) ───────────────────────────── // Switch to the chosen branch up front so every resulting commit // lands there. A name is suggested from the diff; uncommitted @@ -76,7 +80,7 @@ pub fn run(flags: CommitFlags) -> Result { suggest_branch_name(&collected) } else { match analysis::load_api_key() { - Ok(key) => match analysis::analyze_with_mode(&key, &collected.patch, "branch") { + Ok(key) => match analysis::analyze_with_mode(&key, &collected.patch, "branch", None) { Ok((resp, _)) => resp .branch_name .map(|name| slugify(&name)) @@ -95,6 +99,14 @@ pub fn run(flags: CommitFlags) -> Result { } } + // The branch the commits will land on is the current one; if `-b` was + // used, the branch we forked from is what we record as `base_branch`. + let base_branch = if flags.branch { + starting_branch + } else { + None + }; + println!( "Analyzing {} changed file(s) ({})…", collected.files.len(), @@ -108,7 +120,7 @@ pub fn run(flags: CommitFlags) -> Result { // Explicit --offline: no account, no backend, no quota needed. if flags.offline { println!("Offline mode — building a basic local plan (no AI)."); - return commit_offline(&collected, &baseline); + return commit_offline(&collected, &baseline, base_branch.as_deref()); } // ── 2. Auth + local hint ──────────────────────────────────────── @@ -126,7 +138,7 @@ pub fn run(flags: CommitFlags) -> Result { let mut attempts = 0; loop { attempts += 1; - match analysis::analyze_with_mode(&api_key, &collected.patch, "commit") { + match analysis::analyze_with_mode(&api_key, &collected.patch, "commit", None) { Ok(ok) => break ok, // Quota gone or AI unreachable: offer to retry, fall back // to an offline plan, or cancel — never block the commit @@ -137,13 +149,13 @@ pub fn run(flags: CommitFlags) -> Result { println!(); println!("AI analysis still unavailable after {attempts} tries — {reason}"); println!("Falling back to an offline plan; rerun later for an AI-crafted message."); - return commit_offline(&collected, &baseline); + return commit_offline(&collected, &baseline, base_branch.as_deref()); } match prompt_retry(&format!("AI analysis unavailable — {reason}"))? { RetryDecision::Retry => continue, RetryDecision::Offline => { println!("Falling back to an offline plan."); - return commit_offline(&collected, &baseline); + return commit_offline(&collected, &baseline, base_branch.as_deref()); } RetryDecision::Cancel => { println!("Commit cancelled, nothing was changed."); @@ -186,14 +198,14 @@ pub fn run(flags: CommitFlags) -> Result { Re-run later (or use `commitor commit --offline`) for an AI split." ); let plan = offline_groups(&collected); - return run_offline_plan(plan, &file_diffs, collected.staged_used, &baseline); + return run_offline_plan(plan, &file_diffs, collected.staged_used, &baseline, base_branch.as_deref()); } match prompt_retry(&format!( "The suggested split was inconsistent ({err}). Retry for a fresh split?" ))? { RetryDecision::Retry => { println!("Re-requesting an analysis…"); - match analysis::analyze_with_mode(&api_key, &collected.patch, "commit") { + match analysis::analyze_with_mode(&api_key, &collected.patch, "commit", None) { Ok(ok) => { response = ok.0; rate = ok.1; @@ -203,14 +215,14 @@ pub fn run(flags: CommitFlags) -> Result { | Err(analysis::AnalyzeError::Unavailable(reason)) => { println!("AI unavailable ({reason}) — falling back to an offline plan."); let plan = offline_groups(&collected); - return run_offline_plan(plan, &file_diffs, collected.staged_used, &baseline); + return run_offline_plan(plan, &file_diffs, collected.staged_used, &baseline, base_branch.as_deref()); } Err(err) => return Err(err.into()), } } RetryDecision::Offline => { let plan = offline_groups(&collected); - return run_offline_plan(plan, &file_diffs, collected.staged_used, &baseline); + return run_offline_plan(plan, &file_diffs, collected.staged_used, &baseline, base_branch.as_deref()); } RetryDecision::Cancel => { println!("Commit cancelled, nothing was changed."); @@ -220,7 +232,7 @@ pub fn run(flags: CommitFlags) -> Result { } let code = if response.groups.len() <= 1 { - commit_single(&mut plan, &file_diffs, collected.staged_used, &baseline)? + commit_single(&mut plan, &file_diffs, collected.staged_used, &baseline, base_branch.as_deref())? } else { commit_split( &response.groups, @@ -229,6 +241,7 @@ pub fn run(flags: CommitFlags) -> Result { &file_diffs, collected.staged_used, &baseline, + base_branch.as_deref(), )? }; @@ -251,11 +264,15 @@ pub fn run(flags: CommitFlags) -> Result { /// unavailable (quota exhausted, backend down). It splits the diff into /// one commit per `(type, scope)` (features, fixes, and the remainder in /// their own commits) using only local heuristics. -fn commit_offline(collected: &analysis::CollectedDiff, baseline: &str) -> Result { +fn commit_offline( + collected: &analysis::CollectedDiff, + baseline: &str, + base_branch: Option<&str>, +) -> Result { ensure_tree_unchanged(baseline)?; let file_diffs = hunks::parse(&collected.patch); let plan = offline_groups(collected); - run_offline_plan(plan, &file_diffs, collected.staged_used, baseline) + run_offline_plan(plan, &file_diffs, collected.staged_used, baseline, base_branch) } /// Validate an offline-derived plan and execute it, committing each @@ -265,6 +282,7 @@ fn run_offline_plan( file_diffs: &[FileDiff], staged_used: bool, baseline: &str, + base_branch: Option<&str>, ) -> Result { if let Err(err) = hunks::validate(file_diffs, &plan, &plan_files(&plan)) { bail!( @@ -273,9 +291,9 @@ fn run_offline_plan( ); } if plan.len() <= 1 { - commit_single(&mut plan, file_diffs, staged_used, baseline) + commit_single(&mut plan, file_diffs, staged_used, baseline, base_branch) } else { - commit_split(&[], None, &mut plan, file_diffs, staged_used, baseline) + commit_split(&[], None, &mut plan, file_diffs, staged_used, baseline, base_branch) } } @@ -798,6 +816,7 @@ fn commit_single( file_diffs: &[FileDiff], staged_used: bool, baseline: &str, + base_branch: Option<&str>, ) -> Result { let Some(group) = plan.first_mut() else { bail!( @@ -839,7 +858,7 @@ fn commit_single( } } - execute_commits(plan, file_diffs, staged_used, baseline) + execute_commits(plan, file_diffs, staged_used, baseline, base_branch) } /// Multiple groups → show the proposed split and commit each group in @@ -851,6 +870,7 @@ fn commit_split( file_diffs: &[FileDiff], staged_used: bool, baseline: &str, + base_branch: Option<&str>, ) -> Result { println!(); println!( @@ -901,7 +921,7 @@ fn commit_split( } } - execute_commits(plan, file_diffs, staged_used, baseline) + execute_commits(plan, file_diffs, staged_used, baseline, base_branch) } /// One line listing a group's contents: whole paths plain, partial @@ -956,6 +976,7 @@ fn execute_commits( file_diffs: &[FileDiff], staged_used: bool, baseline: &str, + base_branch: Option<&str>, ) -> Result { // Final sanity check immediately before mutating anything. ensure_tree_unchanged(baseline)?; @@ -985,6 +1006,8 @@ fn execute_commits( let total = plan.len(); let mut committed: Vec = Vec::new(); + // What we'll record to the session log once every commit lands. + let mut recorded: Vec = Vec::new(); for (index, group) in plan.iter().enumerate() { let number = index + 1; @@ -1018,6 +1041,20 @@ fn execute_commits( } } committed.push(group.message.clone()); + + // Capture the commit we just made for the session log. + let sha = git::head_sha()?; + let mut files: Vec = group.whole.clone(); + for (path, _) in &group.partial { + if !files.contains(path) { + files.push(path.clone()); + } + } + recorded.push(crate::engine::history::SessionCommit { + sha, + message: group.message.clone(), + files, + }); } Err(err) => { eprintln!("error: {err:#}"); @@ -1028,9 +1065,76 @@ fn execute_commits( println!(); println!("Done — created {total} commit(s)."); + + // Record the session only after every commit succeeded, so a partial + // failure never leaves a half-populated entry in the history log. + if !recorded.is_empty() { + record_session(base_branch, recorded)?; + } + + // Offer to push the result upstream; never fatal to the commit itself. + if let Err(err) = maybe_push() { + eprintln!("warning: didn't push — {err:#}"); + } + Ok(ExitCode::SUCCESS) } +/// After a successful commit, ask whether to push the current branch. +/// Skips the prompt entirely when there is no remote to push to. A push +/// failure is reported but does not fail the (already successful) commit. +fn maybe_push() -> Result<()> { + if git::upstream().is_none() && !git::remote_exists("origin") { + return Ok(()); + } + + if !prompt_confirm("Push these commits to the remote? [y/N] ")? { + return Ok(()); + } + + println!("Pushing…"); + git::push_current_branch() +} + +/// Prompt for a yes/no answer; only `y`/`yes` (case-insensitive) returns +/// true, and the default (empty input) is No. +fn prompt_confirm(question: &str) -> Result { + print!("{question}"); + io::stdout().flush()?; + let mut answer = String::new(); + io::stdin().read_line(&mut answer)?; + Ok(matches!(answer.trim().to_lowercase().as_str(), "y" | "yes")) +} + +/// Persist a successful `commitor commit` run to the per-repo session +/// log. All errors here are non-fatal to the commit itself (the user +/// already has their commits) but are surfaced so a broken history dir +/// doesn't fail silently. +fn record_session( + base_branch: Option<&str>, + commits: Vec, +) -> Result<()> { + use crate::engine::history; + + let first_sha = match commits.first() { + Some(c) => c.sha.clone(), + None => return Ok(()), + }; + let branch = git::current_branch()?; + let session = history::Session { + session_id: history::new_session_id(&first_sha), + timestamp: history::now_iso(), + branch, + base_branch: base_branch.map(str::to_string), + commits, + pushed: false, + reverted: false, + reverted_at: None, + }; + history::record_session(&session)?; + Ok(()) +} + /// True when any status entry has a non-space INDEX column (X), i.e. /// something is staged. `-uno` already excludes untracked lines. fn has_other_staged_work() -> Result { diff --git a/crates/cli/src/engine/git.rs b/crates/cli/src/engine/git.rs index 7c7832e..84a5550 100644 --- a/crates/cli/src/engine/git.rs +++ b/crates/cli/src/engine/git.rs @@ -58,6 +58,24 @@ pub fn changed_files(staged: bool) -> Result> { .collect()) } +/// Changed file paths for a git range, one per line +/// (`git diff --name-only`). +pub fn changed_files_range(range: &str) -> Result> { + let out = run_git(&["diff", range, "--name-only"])?; + Ok(out + .lines() + .map(str::trim) + .filter(|line| !line.is_empty()) + .map(str::to_string) + .collect()) +} + +/// The full patch text for a git range (`git diff `), used by +/// `scan --diff-range` to analyze a PR/branch window. +pub fn diff_range(range: &str) -> Result { + run_git(&["diff", range]) +} + /// Stage exactly the given paths (`git add -- `). /// /// The `--` separator keeps paths starting with `-` from being read @@ -219,6 +237,124 @@ pub fn create_branch(branch: &str) -> Result<()> { run_git(&["checkout", "-b", branch]).map(drop) } +/// Full SHA of the current HEAD (`git rev-parse HEAD`). +pub fn head_sha() -> Result { + run_git(&["rev-parse", "HEAD"]).map(|s| s.trim().to_string()) +} + +/// Current branch name, or `None` in a detached HEAD / unborn repo. +pub fn current_branch() -> Result> { + let out = run_git(&["rev-parse", "--abbrev-ref", "HEAD"])?; + let name = out.trim().to_string(); + if name.is_empty() || name == "HEAD" { + Ok(None) + } else { + Ok(Some(name)) + } +} + +/// Absolute path of the repo root (`git rev-parse --show-toplevel`). +pub fn repo_toplevel() -> Result { + run_git(&["rev-parse", "--show-toplevel"]).map(|s| s.trim().to_string()) +} + +/// The `origin` remote's URL, or `None` if the repo has no `origin` or +/// it isn't configured. Used to derive a stable per-repo history id. +pub fn remote_url() -> Result> { + let output = Command::new("git") + .args(["config", "--get", "remote.origin.url"]) + .output() + .context("failed to execute `git` — is git installed and in PATH?")?; + if !output.status.success() { + return Ok(None); + } + let url = String::from_utf8_lossy(&output.stdout).trim().to_string(); + Ok(if url.is_empty() { None } else { Some(url) }) +} + +/// True when the working tree has tracked, uncommitted changes. Untracked +/// files are intentionally ignored: a hard reset leaves them in place, so +/// they are never at risk from a revert/reset. +pub fn has_uncommitted_changes() -> Result { + let out = run_git(&["status", "--porcelain", "-uno"])?; + Ok(!out.trim().is_empty()) +} + +/// True when `sha` has been pushed to a remote-tracking branch (i.e. it +/// shows up in `git branch -r --contains `). Fails clearly when the +/// commit no longer exists — usually a rebase or a `reset --hard` upstream +/// has rewritten history out from under the recorded session. +pub fn check_pushed(sha: &str) -> Result { + let output = Command::new("git") + .args(["branch", "-r", "--contains", sha]) + .output() + .context("failed to execute `git` — is git installed and in PATH?")?; + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + anyhow::bail!( + "commit {} could not be found in this repository — it may have been rewritten \ + or removed by a rebase or a hard reset since `commitor commit` created it. \ + History cannot be reverted safely. ({})", + sha, + stderr.trim() + ); + } + let out = String::from_utf8_lossy(&output.stdout); + Ok(!out.trim().is_empty()) +} + +/// Create a revert commit for `sha` without opening an editor. +pub fn revert_commit(sha: &str) -> Result<()> { + run_git(&["revert", "--no-edit", sha]).map(drop) +} + +/// Move HEAD and the index/working tree to `sha` (`git reset --hard`). +pub fn reset_hard(sha: &str) -> Result<()> { + run_git(&["reset", "--hard", sha]).map(drop) +} + +/// SHA of the parent of `sha` (`git rev-parse ^`). Fails clearly when +/// `sha` is a root commit with no parent — there is nothing before it to +/// reset back to. +pub fn parent_of(sha: &str) -> Result { + let arg = format!("{sha}^"); + run_git(&["rev-parse", &arg]).map(|s| s.trim().to_string()) +} + +/// Name of the current branch's upstream tracking ref (e.g. `origin/main`), +/// or `None` when the branch has no upstream configured. +pub fn upstream() -> Option { + let out = run_git(&["rev-parse", "--abbrev-ref", "--symbolic-full-name", "@{u}"]).ok()?; + let name = out.trim().to_string(); + if name.is_empty() { + None + } else { + Some(name) + } +} + +/// True when a remote with the given name is configured. +pub fn remote_exists(name: &str) -> bool { + Command::new("git") + .args(["remote", "get-url", name]) + .output() + .map(|out| out.status.success()) + .unwrap_or(false) +} + +/// Push the current branch to its remote. Uses the configured upstream when +/// one exists; otherwise pushes `-u origin ` to set one up. Fails +/// clearly on a detached HEAD or when there is no `origin` remote. +pub fn push_current_branch() -> Result<()> { + if upstream().is_some() { + run_git(&["push"]).map(drop) + } else { + let branch = current_branch()? + .context("cannot push: HEAD is detached, so there is no branch to push")?; + run_git(&["push", "-u", "origin", &branch]).map(drop) + } +} + fn run_git(args: &[&str]) -> Result { let output = Command::new("git") .args(args) diff --git a/crates/cli/src/engine/history.rs b/crates/cli/src/engine/history.rs new file mode 100644 index 0000000..a8f7298 --- /dev/null +++ b/crates/cli/src/engine/history.rs @@ -0,0 +1,193 @@ +//! Local, per-repo session log for commits made by `commitor commit`. +//! +//! Each successful `commitor commit` run appends one JSON line (a +//! "session") to `~/.commitor/history/.jsonl`. The repo id is a +//! stable hash of the repo's remote URL (or, failing that, its root path), +//! so history never leaks across unrelated repos on the same machine. +//! +//! The log is append-only for new sessions. Marking a session reverted is +//! the one case that rewrites the file, and it does so atomically (write a +//! temp file, then rename over the original) so a crash mid-write can't +//! corrupt the whole history. + +use std::fs::{self, File, OpenOptions}; +use std::io::{BufRead, BufReader, Write}; +use std::path::PathBuf; + +use anyhow::{Context, Result}; +use chrono::Utc; +use serde::{Deserialize, Serialize}; + +use crate::config; +use crate::engine::git; + +/// One commit produced by a session, enough to show and to revert. +#[derive(Serialize, Deserialize, Debug, Clone)] +pub struct SessionCommit { + pub sha: String, + pub message: String, + pub files: Vec, +} + +/// A single `commitor commit` run. +#[derive(Serialize, Deserialize, Debug, Clone)] +pub struct Session { + pub session_id: String, + /// ISO-8601 UTC timestamp of when the session was recorded. + pub timestamp: String, + /// Branch the commits landed on (`None` in a detached HEAD). + #[serde(default)] + pub branch: Option, + /// Branch the new branch was forked from, only for `commit -b` runs. + #[serde(default)] + pub base_branch: Option, + pub commits: Vec, + /// Whether any commit in the session had been pushed at record time. + /// Out of scope to detect live, so this is always `false` here and + /// `revert` re-checks the remote at run time. + #[serde(default)] + pub pushed: bool, + /// Set once `revert` has undone this session. + #[serde(default)] + pub reverted: bool, + /// ISO-8601 UTC timestamp of the revert, when `reverted` is true. + #[serde(default)] + pub reverted_at: Option, +} + +/// Stable, filesystem-safe identifier for the current repository, derived +/// from its remote URL (preferred) or root path (fallback). +pub fn repo_id() -> Result { + let key = match git::remote_url()? { + Some(remote) if !remote.trim().is_empty() => remote, + _ => git::repo_toplevel()?, + }; + + // FNV-1a 64-bit: cheap, dependency-free, and stable across runs of + // the same binary. We only need a well-distributed, non-reversible + // handle for the on-disk filename, not cryptographic strength. + let mut hash: u64 = 0xcbf29ce484222325; + for byte in key.as_bytes() { + hash ^= *byte as u64; + hash = hash.wrapping_mul(0x100000001b3); + } + Ok(format!("{hash:016x}")) +} + +/// `~/.commitor/history`. +fn history_dir() -> Result { + Ok(config::commitor_dir()?.join("history")) +} + +/// `/.jsonl`. +fn history_path(id: &str) -> Result { + Ok(history_dir()?.join(format!("{id}.jsonl"))) +} + +/// Build a fresh session id from the current time and the session's first +/// commit sha — unique and stable per session without extra dependencies. +pub fn new_session_id(first_sha: &str) -> String { + let nanos = Utc::now().timestamp_nanos_opt().unwrap_or(0); + format!("{nanos:x}-{first_sha}", nanos = nanos, first_sha = &first_sha[..8]) +} + +/// Current UTC time as an ISO-8601 string (e.g. `2025-01-02T03:04:05Z`). +pub fn now_iso() -> String { + Utc::now().format("%Y-%m-%dT%H:%M:%SZ").to_string() +} + +/// Append a new session to the repo's history log. +pub fn record_session(session: &Session) -> Result<()> { + let dir = history_dir()?; + fs::create_dir_all(&dir) + .with_context(|| format!("failed to create {}", dir.display()))?; + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let _ = fs::set_permissions(&dir, fs::Permissions::from_mode(0o700)); + } + + let path = history_path(&repo_id()?)?; + let mut file = OpenOptions::new() + .create(true) + .append(true) + .open(&path) + .with_context(|| format!("failed to open {}", path.display()))?; + let line = serde_json::to_string(session).context("failed to serialize session")?; + writeln!(file, "{line}").context("failed to write session to history")?; + Ok(()) +} + +/// Load every recorded session for the current repo, oldest first. +pub fn load_sessions() -> Result> { + let path = history_path(&repo_id()?)?; + let file = match File::open(&path) { + Ok(f) => f, + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(Vec::new()), + Err(err) => { + return Err(err) + .with_context(|| format!("failed to read {}", path.display())) + } + }; + + let reader = BufReader::new(file); + let mut sessions = Vec::new(); + for (index, line) in reader.lines().enumerate() { + let line = line.with_context(|| format!("failed to read {}", path.display()))?; + let line = line.trim(); + if line.is_empty() { + continue; + } + match serde_json::from_str::(line) { + Ok(session) => sessions.push(session), + Err(err) => { + eprintln!( + "warning: ignoring malformed history line {} in {}: {err}", + index + 1, + path.display() + ); + } + } + } + Ok(sessions) +} + +/// Record that a session has been reverted. Rewrites the whole file +/// atomically; a no-op (beyond a warning-free early return) if the id is +/// not present, since the caller surfaces that as its own error. +pub fn mark_reverted(session_id: &str) -> Result<()> { + let mut sessions = load_sessions()?; + let now = now_iso(); + let mut found = false; + for session in sessions.iter_mut() { + if session.session_id == session_id { + session.reverted = true; + session.reverted_at = Some(now); + found = true; + break; + } + } + if !found { + return Ok(()); + } + + let path = history_path(&repo_id()?)?; + let tmp = path.with_extension("jsonl.tmp"); + { + let mut file = OpenOptions::new() + .create(true) + .write(true) + .truncate(true) + .open(&tmp) + .with_context(|| format!("failed to write {}", tmp.display()))?; + for session in &sessions { + let line = serde_json::to_string(session).context("failed to serialize session")?; + writeln!(file, "{line}").context("failed to write session to history")?; + } + file.flush().context("failed to flush history")?; + } + fs::rename(&tmp, &path).with_context(|| { + format!("failed to replace {} with updated history", path.display()) + })?; + Ok(()) +} diff --git a/crates/cli/src/engine/mod.rs b/crates/cli/src/engine/mod.rs index 90d33d9..289f9eb 100644 --- a/crates/cli/src/engine/mod.rs +++ b/crates/cli/src/engine/mod.rs @@ -4,6 +4,7 @@ #[allow(dead_code)] pub mod gemini; pub mod git; +pub mod history; pub mod hunks; #[allow(dead_code)] pub mod grouping; diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index cd030e0..d1bc609 100644 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -11,6 +11,7 @@ mod commit; mod config; mod engine; mod heuristics; +mod revert; mod scan; use engine::update; @@ -50,6 +51,17 @@ enum Commands { /// Print machine-readable JSON instead of a formatted report #[arg(long)] json: bool, + /// Analyze a fixed git range instead of the working tree + /// (e.g. `origin/main...HEAD`); incompatible with `--all` + #[arg(long, value_name = "RANGE")] + diff_range: Option, + /// Emit GitHub-flavored Markdown suited to a PR comment + #[arg(long)] + markdown: bool, + /// Optional PR context (title/description) forwarded to the model, + /// so it can weigh the stated intent against the files touched + #[arg(long, value_name = "TEXT")] + context: Option, }, /// Analyze the working diff and create the approved git commits Commit { @@ -68,6 +80,22 @@ enum Commands { #[arg(short = 'b')] branch: bool, }, + /// Undo commits made by a previous `commitor commit` run + Revert { + /// Show the last N sessions instead of reverting the most recent + #[arg(long)] + list: bool, + /// Skip the working-tree-dirty safety check (use with care — a + /// reset can discard uncommitted work) + #[arg(long)] + force: bool, + /// Target a specific session by id (or any commit sha it contains) + #[arg(value_name = "SESSION_ID")] + session: Option, + /// Number of sessions to show with --list (default 10) + #[arg(long)] + limit: Option, + }, /// Update commitor to the latest release Update, /// Show or change the local admin role status @@ -138,15 +166,33 @@ fn execute(command: Commands) -> Result> { offline, strict, json, + diff_range, + markdown, + context, } => scan::run(scan::ScanFlags { all, offline, strict, json, + diff_range, + markdown, + context, }).map_err(Some), Commands::Commit { all, offline, branch } => { commit::run(commit::CommitFlags { all, offline, branch }).map_err(Some) } + Commands::Revert { + list, + force, + session, + limit, + } => revert::run(revert::RevertFlags { + list, + force, + session, + limit, + }) + .map_err(Some), Commands::Update => run_update().map(|_| ExitCode::SUCCESS).map_err(Some), Commands::Admin { action } => match action.as_str() { "status" => admin::status().map(|_| ExitCode::SUCCESS).map_err(Some), diff --git a/crates/cli/src/revert.rs b/crates/cli/src/revert.rs new file mode 100644 index 0000000..842278d --- /dev/null +++ b/crates/cli/src/revert.rs @@ -0,0 +1,266 @@ +//! `commitor revert` — undo commits made by a previous `commitor commit`. +//! +//! Every `commitor commit` writes a session to a local per-repo log +//! (see [`crate::engine::history`]). `revert` reads that log, lets the +//! user pick a session (most recent by default, or `--list` to choose an +//! older one), and then undoes it one of two ways: +//! +//! * **hard reset** — when every commit in the session is still local-only +//! (not pushed). This cleanly erases the commits with no history +//! pollution, since nothing shared depends on them yet. +//! * **revert commits** — when any commit has been pushed. Rewriting shared +//! history is unsafe for anyone who has already pulled, so we create +//! proper revert commits instead (newest-to-oldest). +//! +//! Both paths require an explicit `y/N` confirmation, and the command +//! refuses to run with a dirty working tree unless `--force` is given. + +use std::io::{self, Write}; +use std::process::ExitCode; + +use anyhow::{bail, Context, Result}; + +use crate::engine::git; +use crate::engine::history::{self, Session}; + +#[derive(Debug, Default)] +pub struct RevertFlags { + /// Show the last N sessions instead of acting on the most recent. + pub list: bool, + /// Skip the working-tree-dirty safety check. + pub force: bool, + /// Target a specific session by id, or any commit sha it contains. + pub session: Option, + /// How many sessions `--list` should show (default 10). + pub limit: Option, +} + +pub fn run(flags: RevertFlags) -> Result { + let mut sessions = history::load_sessions()?; + if sessions.is_empty() { + println!("No commitor commit history found for this repo"); + return Ok(ExitCode::SUCCESS); + } + // Newest first for display and default targeting. + sessions.reverse(); + + if flags.list { + return list_sessions(&sessions, flags.limit.unwrap_or(10)); + } + + // Pick the target session. + let target = match &flags.session { + Some(id) => find_session(&sessions, id)?, + None => &sessions[0], + }; + + if target.reverted { + println!( + "Session {} was already reverted{} — nothing to do.", + target.session_id, + target + .reverted_at + .as_deref() + .map(|t| format!(" at {t}")) + .unwrap_or_default() + ); + return Ok(ExitCode::SUCCESS); + } + + // Never silently discard uncommitted work. + if !flags.force && git::has_uncommitted_changes()? { + bail!( + "Your working tree has uncommitted changes, so `commitor revert` could destroy or \ + clash with that work (a hard reset would discard it entirely).\n\ + Commit or stash your changes first, or pass --force to proceed anyway." + ); + } + + // Show what the session contained. + show_session(target); + + // Decide reset vs. revert by checking each commit against the remote. + let mut any_pushed = false; + let mut any_local = false; + for commit in &target.commits { + let pushed = git::check_pushed(&commit.sha) + .with_context(|| format!("couldn't check whether {} is pushed", commit.sha))?; + if pushed { + any_pushed = true; + } else { + any_local = true; + } + } + + // A mix within one session is treated as "pushed" for safety: revert, + // don't reset, and say so. + let strategy = if any_pushed { + Strategy::Revert + } else { + Strategy::Reset + }; + + let reset_target = if let Strategy::Reset = strategy { + Some( + git::parent_of(&target.commits[0].sha).with_context(|| { + "the first commit in this session is the repository's root commit — there is \ + nothing before it to reset back to. Revert it manually with \ + `git revert `, or reset the branch ref yourself." + })?, + ) + } else { + None + }; + + // Preview what will happen. + println!(); + match &strategy { + Strategy::Reset => { + let reset_ref = reset_target.as_deref().unwrap(); + println!( + "This will DISCARD the {} commit(s) in this session with a hard reset to {}", + target.commits.len(), + &reset_ref[..7.min(reset_ref.len())] + ); + println!( + "Warning: the commits will be gone, not just unstaged. This cannot be undone \ + for commits that have no other ref pointing at them." + ); + } + Strategy::Revert => { + println!( + "This will create {} new revert commit(s) for the session's commits \ + (newest first).", + target.commits.len() + ); + if any_local { + println!( + "Some commits were already pushed, so a reset would rewrite shared \ + history — `git revert` is used instead to stay safe." + ); + } else { + println!("At least one commit is already on a remote; rewriting history is unsafe."); + } + } + } + + if !confirm("Proceed with the revert? [y/N] ")? { + println!("Aborted — nothing was changed."); + return Ok(ExitCode::SUCCESS); + } + + // Execute. + match &strategy { + Strategy::Reset => { + let target = reset_target.unwrap(); + println!("Resetting to {}…", &target[..7.min(target.len())]); + git::reset_hard(&target)?; + } + Strategy::Revert => { + // Newest-to-oldest so the original ordering is undone cleanly. + for commit in target.commits.iter().rev() { + let summary = commit.message.lines().next().unwrap_or(""); + println!("Reverting {} — {summary}", &commit.sha[..7.min(commit.sha.len())]); + git::revert_commit(&commit.sha)?; + } + } + } + + history::mark_reverted(&target.session_id)?; + println!("Marked session {} as reverted.", target.session_id); + Ok(ExitCode::SUCCESS) +} + +/// Strategy for undoing a session. +enum Strategy { + /// `git reset --hard ` — erases local commits. + Reset, + /// `git revert` each commit — safe for pushed/shared commits. + Revert, +} + +/// Print the most recent `limit` sessions as a pick list. +fn list_sessions(sessions: &[Session], limit: usize) -> Result { + let shown: Vec<&Session> = sessions.iter().take(limit).collect(); + println!("Recent commitor sessions (newest first):\n"); + for (i, session) in shown.iter().enumerate() { + let branch = session + .branch + .as_deref() + .map(|b| format!(" on {b}")) + .unwrap_or_default(); + let reverted = if session.reverted { " (reverted)" } else { "" }; + println!( + "{}. {} — {} commit(s){} at {}{}", + i + 1, + session.session_id, + session.commits.len(), + branch, + session.timestamp, + reverted + ); + if let Some(first) = session.commits.first() { + let summary = first.message.lines().next().unwrap_or(""); + println!( + " first commit: {} {} (use `commitor revert {}` to target this session)", + &first.sha[..7.min(first.sha.len())], + summary, + session.session_id + ); + } + } + println!("\nPass `commitor revert ` to undo a specific session."); + Ok(ExitCode::SUCCESS) +} + +/// Print a single session's commits. +fn show_session(session: &Session) { + println!( + "Session {} — {} commit(s), recorded at {}", + session.session_id, + session.commits.len(), + session.timestamp + ); + if let Some(branch) = &session.branch { + println!("Branch: {branch}"); + } else { + println!("Branch: (detached HEAD)"); + } + if let Some(base) = &session.base_branch { + println!("Forked from: {base}"); + } + for commit in &session.commits { + let summary = commit.message.lines().next().unwrap_or(""); + println!( + " {} {}", + &commit.sha[..7.min(commit.sha.len())], + summary + ); + if !commit.files.is_empty() { + println!(" files: {}", commit.files.join(", ")); + } + } +} + +/// Find a session by its id, or by any commit sha it contains. +fn find_session<'a>(sessions: &'a [Session], id: &str) -> Result<&'a Session> { + let by_id = sessions.iter().find(|s| s.session_id == id); + let by_sha = sessions + .iter() + .find(|s| s.commits.iter().any(|c| c.sha == id || c.sha.starts_with(id))); + by_id + .or(by_sha) + .with_context(|| format!("no commitor session matches `{id}` in this repo's history")) +} + +/// Prompt for a yes/no answer; only `y`/`yes` (case-insensitive) returns +/// true, and the default (empty input) is No. +fn confirm(prompt: &str) -> Result { + print!("{prompt}"); + io::stdout().flush().context("failed to write prompt")?; + let mut answer = String::new(); + io::stdin() + .read_line(&mut answer) + .context("failed to read your answer")?; + Ok(matches!(answer.trim().to_lowercase().as_str(), "y" | "yes")) +} diff --git a/crates/cli/src/scan.rs b/crates/cli/src/scan.rs index f55625a..0b03985 100644 --- a/crates/cli/src/scan.rs +++ b/crates/cli/src/scan.rs @@ -1,8 +1,11 @@ //! `commitor scan` — read-only analysis of the working diff. //! -//! Runs cheap local heuristics first; only escalates to the backend's -//! `/analyze` endpoint when the heuristics cannot confidently call -//! the changeset one logical change. +//! Runs cheap local heuristics first; for a working-tree scan it only +//! escalates to the backend's `/analyze` endpoint when the heuristics +//! cannot confidently call the changeset one logical change. For +//! `--diff-range` (PR-scale) runs the local heuristic is advisory only — +//! the backend is always consulted, because PR diffs mix unrelated areas +//! the path-clustering can't see. //! //! All analysis mechanics (diff collection, wire types, backend call) //! live in [`crate::analysis`]; this module is collection policy and @@ -11,7 +14,7 @@ use std::io::IsTerminal; use std::process::ExitCode; -use anyhow::Result; +use anyhow::{bail, Result}; use crate::analysis; use crate::heuristics::{self, Verdict}; @@ -22,44 +25,100 @@ pub struct ScanFlags { pub offline: bool, pub strict: bool, pub json: bool, + /// Optional explicit git range to analyze (e.g. `origin/main...HEAD`), + /// instead of the working tree. Incompatible with `all`. + pub diff_range: Option, + /// Emit GitHub-flavored Markdown suited to a PR comment. + pub markdown: bool, + /// Optional PR-scoped context (title/description) forwarded to the + /// model. Supplied by the GitHub Action so PR-scale analysis can weigh + /// the stated intent against the files actually touched. + pub context: Option, +} + +/// Whether a scan must escalate to the backend rather than trust the +/// local heuristic verdict. `--diff-range` (PR-scale) scans always +/// escalate — the heuristic is advisory only there. A working-tree scan +/// escalates unless the heuristic confidently called the change clean. +/// Offline scans never reach this (they're handled before any call). +fn scan_escalates(verdict: &Verdict, diff_range: bool) -> bool { + diff_range || !matches!(verdict, Verdict::Clean { .. }) } pub fn run(flags: ScanFlags) -> Result { + // Machine-readable output modes suppress the human progress lines. + let quiet = flags.json || flags.markdown; + // ── 1. Collect the diff ───────────────────────────────────────── - // Scan deliberately ignores untracked files — its scope is the - // tracked working diff (`commit` opts into untracked files). - let collected = analysis::collect_diff(flags.all, false)?; + let collected = if let Some(range) = &flags.diff_range { + if flags.all { + bail!( + "`--diff-range` and `--all` are incompatible — a fixed range already \ + defines exactly what to analyze, so there is no working-tree flavor \ + (staged vs. unstaged) to choose between." + ); + } + // A fixed range is its own scope: skip the staged/unstaged + // fallback warning entirely and never pull in untracked files. + analysis::collect_range(range)? + } else { + // Scan deliberately ignores untracked files — its scope is the + // tracked working diff (`commit` opts into untracked files). + analysis::collect_diff(flags.all, false)? + }; + if collected.files.is_empty() { - println!("Nothing to scan — no staged or unstaged changes."); + if let Some(range) = &flags.diff_range { + if flags.markdown { + println!("\n"); + println!("## 🔍 Commitor Analysis\n"); + println!("_No changes to analyze for range `{range}`._\n"); + print_powered_by(); + } else { + println!("No changes to analyze for range `{range}`."); + } + } else { + println!("Nothing to scan — no staged or unstaged changes."); + } return Ok(ExitCode::SUCCESS); } - if !flags.json { - println!( - "Scanning {} changed file(s) ({}).", - collected.files.len(), - if collected.staged_used { "staged" } else { "unstaged" } - ); - } - - // ── 2. Local heuristics ───────────────────────────────────────── - let verdict = heuristics::evaluate(&collected.files); - - if let Verdict::Clean { summary } = &verdict { - if flags.json { - print_json_local(summary); + if !quiet { + if let Some(range) = &flags.diff_range { + println!( + "Analyzing {} changed file(s) in range `{range}`.", + collected.files.len() + ); } else { - print_success(&format!("Looks like a single logical change — {summary}.")); + println!( + "Scanning {} changed file(s) ({}).", + collected.files.len(), + if collected.staged_used { "staged" } else { "unstaged" } + ); } - return Ok(ExitCode::SUCCESS); } - let Verdict::Inconclusive { reason } = verdict else { - unreachable!("evaluate() only returns Clean or Inconclusive"); + // ── 2. Local heuristics ───────────────────────────────────────── + let verdict = heuristics::evaluate(&collected.files); + let reason = match &verdict { + Verdict::Inconclusive { reason } => reason.clone(), + Verdict::Clean { .. } => String::new(), }; + // `--offline` has no backend to consult: trust a "clean" verdict and + // otherwise report the heuristic's inconclusive reason. if flags.offline { - if !flags.json { + if matches!(verdict, Verdict::Clean { .. }) { + if flags.markdown { + print_markdown_clean("single logical change"); + } else if flags.json { + print_json_local("single logical change"); + } else { + print_success("Looks like a single logical change."); + } + } else if flags.markdown { + print_markdown_offline(&reason); + } else if !flags.json { println!(); println!( "Local heuristics could not confirm one logical change ({reason})." @@ -71,14 +130,49 @@ pub fn run(flags: ScanFlags) -> Result { return Ok(ExitCode::SUCCESS); } + // Online. For `--diff-range` (PR-scale) runs the local heuristic is + // advisory only and we *always* escalate to the backend — PR diffs + // routinely combine unrelated areas the path-clustering can't see + // (most notably a root-level dependency bump, which has *no* + // top-level directory and is invisible to the clustering). A + // working-tree scan only escalates when the heuristic couldn't call + // the change clean, preserving the pre-existing short-circuit. + if !scan_escalates(&verdict, flags.diff_range.is_some()) { + if let Verdict::Clean { summary } = &verdict { + if flags.markdown { + print_markdown_clean(summary); + } else if flags.json { + print_json_local(summary); + } else { + print_success(&format!("Looks like a single logical change — {summary}.")); + } + } + return Ok(ExitCode::SUCCESS); + } + // ── 3. Backend escalation ─────────────────────────────────────── // Credentials and the size guard are handled inside the shared // analysis call; this surfaces the standard not-logged-in message - // before any network attempt. - let (response, rate) = analysis::analyze_patch(&collected.patch)?; + // before any network attempt. PR-scoped context (title/description) + // is forwarded when the caller supplied it (e.g. the GitHub Action). + let (response, rate) = analysis::analyze_patch_with_context( + &collected.patch, + flags.context.as_deref(), + )?; // ── 4./5. Verdict + report ────────────────────────────────────── - if flags.json { + if flags.markdown { + if response.groups.len() <= 1 { + let summary = response + .groups + .first() + .map(|group| group.commit_message.clone()) + .unwrap_or_else(|| "single logical change".to_string()); + print_markdown_clean(&summary); + } else { + print_markdown_mixed(&response, &reason); + } + } else if flags.json { print_json(&response); } else if response.groups.len() <= 1 { let summary = response @@ -91,9 +185,9 @@ pub fn run(flags: ScanFlags) -> Result { print_mixed_report(&response, &reason); } - // Soft quota hint on human output only — JSON and piped output - // must stay machine-clean. - if !flags.json { + // Soft quota hint on human output only — JSON/Markdown and piped + // output must stay machine-clean. + if !flags.json && !flags.markdown { if let Some(message) = rate.low_quota_message() { eprintln!(); eprintln!("{message}"); @@ -179,3 +273,153 @@ fn print_json_local(summary: &str) { }); println!("{payload}"); } + +// ── Markdown (PR-comment) rendering ───────────────────────────────── +// +// Every Markdown variant opens with the hidden `` +// marker so the GitHub Action can find and update a previous comment +// instead of posting a fresh one on each push. Output is deliberately +// compact — PR comments that are walls of text get ignored. Severity is +// derived, not from the model: a single logical change reads "Low", while +// any group that's part of a mixed (split-worthy) PR reads "Medium", and +// the PR as a whole is flagged "High". + +fn print_powered_by() { + println!( + "\nPowered by [Commitor](https://github.com/Commitor-AI/commitor) — \ + catches unrelated changes before they're buried." + ); +} + +fn print_markdown_clean(summary: &str) { + println!("\n"); + println!("## 🔍 Commitor Analysis\n"); + println!("✅ **Looks like a single logical change** — {summary}\n"); + print_powered_by(); +} + +fn print_markdown_offline(reason: &str) { + println!("\n"); + println!("## 🔍 Commitor Analysis\n"); + println!( + "⚠️ Local heuristics couldn't confirm a single logical change ({reason}). \ + Run `commitor login` for a full AI analysis.\n" + ); + print_powered_by(); +} + +fn print_markdown_mixed(response: &analysis::AnalyzeResponse, reason: &str) { + println!("\n"); + println!("## 🔍 Commitor Analysis\n"); + println!("**Severity: High** — this PR bundles multiple unrelated changes.\n"); + println!("⚠️ {reason} Consider splitting it into separate commits:\n"); + + println!("| # | Suggested commit | Severity | Files | Why |"); + println!("|---|---|---|---|---|"); + for (index, group) in response.groups.iter().enumerate() { + let number = index + 1; + let files = if group.files.is_empty() { + group + .partial_files + .iter() + .map(|p| p.path.as_str()) + .collect::>() + .join(", ") + } else { + group.files.join(", ") + }; + let why = if group.rationale.is_empty() { + "—".to_string() + } else { + group.rationale.replace('|', "\\|") + }; + println!( + "| {number} | {} | Medium | {files} | {why} |", + group.commit_message + ); + } + println!(); + println!("Run `commitor commit` to split this into separate commits."); + print_powered_by(); +} + +#[cfg(test)] +mod tests { + use super::*; + use heuristics::Verdict; + + fn paths(values: &[&str]) -> Vec { + values.iter().map(|v| v.to_string()).collect() + } + + // The escalation decision that fixes the PR-scale blind spot. + #[test] + fn working_tree_clean_short_circuits() { + let verdict = Verdict::Clean { + summary: "all changes under src".into(), + }; + assert!(!scan_escalates(&verdict, false)); + } + + #[test] + fn working_tree_inconclusive_escalates() { + let verdict = Verdict::Inconclusive { + reason: "spreads across areas".into(), + }; + assert!(scan_escalates(&verdict, false)); + } + + #[test] + fn diff_range_never_short_circuits_on_clean() { + // The heart of the fix: a PR-scale "clean" verdict must still + // reach the backend, because the path heuristic is advisory only + // at that scale. + let verdict = Verdict::Clean { + summary: "all changes under src".into(), + }; + assert!(scan_escalates(&verdict, true)); + } + + #[test] + fn diff_range_inconclusive_escalates() { + let verdict = Verdict::Inconclusive { + reason: "spreads across areas".into(), + }; + assert!(scan_escalates(&verdict, true)); + } + + // DOCUMENTED BLIND SPOT: the local heuristic calls a root-level + // dependency bump combined with a feature "Clean", because files + // without a top-level directory (Cargo.toml, Cargo.lock, …) are + // invisible to the top-directory clustering. This is exactly the + // class of PR that `--diff-range` must still escalate — which the + // test above guarantees. + #[test] + fn root_dependency_bump_plus_feature_looks_clean_to_heuristic() { + let verdict = heuristics::evaluate(&paths(&[ + "Cargo.toml", + "Cargo.lock", + "src/feature/login.rs", + ])); + assert_eq!( + verdict, + Verdict::Clean { + summary: "all changes under src".to_string() + }, + "heuristic blind spot assumption must hold for the diff-range fix to matter" + ); + } + + #[test] + fn mixed_pr_with_root_bump_is_clean_locally_but_must_escalate() { + let verdict = heuristics::evaluate(&paths(&[ + "Cargo.toml", + "Cargo.lock", + "src/feature/login.rs", + ])); + assert!(matches!(verdict, Verdict::Clean { .. })); + // …yet a --diff-range scan must escalate it to the backend. + assert!(scan_escalates(&verdict, true)); + } +} +