feat: pre-push review gate (#46) by sam-phinizy · Pull Request #48 · phin-tech/redpen

sam-phinizy · 2026-03-29T15:18:25Z

Summary

redpen open --pre-push --wait: reads git pre-push stdin, blocks until human approves
redpen open --diff-remote --wait: diffs against upstream tracking branch (for prek/pre-commit which don't forward stdin)
redpen open --diff-base <sha> --wait: diff against any git ref
--no-timeout flag, structured exit codes (0/1/2/3)
Stderr rejection summary with annotation counts and session ID
redpen list --session <id>: list all annotations for a review session
New /rpc/session.annotations server endpoint
Multi-file sessions now fully tracked in ReviewSessions
CI auto-skip (CI, GITHUB_ACTIONS, etc.) + REDPEN_SKIP_GATE=1 escape hatch
prek.toml wired up with --diff-remote hook
README updated with Pre-Push Review Gate section

Known issue

Server discovery (server.json) not available fast enough when app launches cold — needs investigation. Workaround: have the app already running, or use REDPEN_SKIP_GATE=1.

Closes #46

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Address feasibility issues: fix app name, clarify positional args conflict, document server endpoint needed for list --session, add timeout precedence rules, stdout/stderr contract, and edge cases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

- `redpen open --pre-push --wait`: reads git pre-push stdin, computes changed files, opens them in the app, blocks until verdict - `redpen open --diff-base <sha> --wait`: compute changed files from any git ref diff - `--no-timeout`: override the default 600s timeout in --pre-push mode - Structured exit codes: 0=approved, 1=changes_requested, 2=timeout, 3=no app - Stderr summary on rejection with annotation counts and session ID - `redpen list --session <id>`: list all annotations for a session - New server endpoint `/rpc/session.annotations` for session-wide annotation queries - Multi-file sessions now tracked in ReviewSessions (all files indexed) - Auto-launch "Red Pen" app on macOS if server unavailable Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Checks REDPEN_SKIP_GATE env var and well-known CI vars (CI, GITHUB_ACTIONS, GITLAB_CI, CIRCLECI, JENKINS_URL, BUILDKITE, TF_BUILD, CODEBUILD_BUILD_ID). Exits 0 with a message when any are set, so pre-push hooks don't hang in automated pipelines. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Add redpen-review hook to prek.toml and document the pre-push gate, CLI flags, exit codes, CI skip behavior, and agent feedback loop in README. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

prek and pre-commit don't forward pre-push stdin to individual hooks, so --pre-push doesn't work there. --diff-remote diffs against the upstream tracking branch (@{u}, falling back to origin/HEAD) without needing stdin. Update prek.toml and README to use --diff-remote. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Adds a “human-in-the-loop” review gate that can be used as a pre-push hook by extending the redpen CLI to open the changed files in the desktop app and block until a review verdict is recorded, with session-wide annotation retrieval support.

Changes:

Extend redpen open with git-diff based modes (--pre-push, --diff-remote, --diff-base) plus timeout/exit-code behavior for gating pushes.
Add session-wide annotation listing via redpen list --session <id> and a new server RPC endpoint to fetch annotations across session files.
Wire up prek pre-push hook configuration and document the workflow/flags in the README + a design spec.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
`prek.toml`	Adds a `pre-push` hook that runs the redpen review gate via `--diff-remote`.
`docs/superpowers/specs/2026-03-29-pre-push-review-gate-design.md`	New design doc describing CLI flags, exit codes, and server endpoint contract.
`crates/redpen-server/src/lib.rs`	Tracks multi-file sessions and adds `/rpc/session.annotations` endpoint.
`crates/redpen-cli/src/server_client.rs`	Adds session-aware open + typed `review_wait` result and session annotations client call.
`crates/redpen-cli/src/main.rs`	Implements new CLI flags, git diff file discovery, CI skip, rejection summary, and session listing.
`README.md`	Documents pre-push gate setup, flags, and exit codes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-29T15:23:14Z

    pub async fn create_with_id(&self, id: Option<String>, file: String) -> String {
        let session_id = id.unwrap_or_else(|| uuid::Uuid::new_v4().to_string());
        let (tx, rx) = oneshot::channel();
        self.senders
            .lock()
            .await
-            .insert(session_id.clone(), (file, tx));
+            .insert(session_id.clone(), (file.clone(), tx));
        self.receivers.lock().await.insert(session_id.clone(), rx);
+        self.session_files
+            .lock()
+            .await
+            .entry(session_id.clone())
+            .or_default()
+            .push(file);
        session_id
    }

+    /// Add an additional file to a session's file list.
+    pub async fn add_file(&self, session_id: &str, file: String) {
+        self.session_files
+            .lock()
+            .await
+            .entry(session_id.to_string())
+            .or_default()
+            .push(file);
+    }
+
+    /// Get all files associated with a session.
+    pub async fn get_files(&self, session_id: &str) -> Vec<String> {
+        self.session_files
+            .lock()
+            .await
+            .get(session_id)
+            .cloned()
+            .unwrap_or_default()
+    }


session_files is only ever appended to (create_with_id, add_file) and is never removed, so long-running servers will leak memory across sessions. Also, using Vec without deduping can accumulate duplicate entries if the same file is opened multiple times. Consider cleaning up session_files when a session completes and/or using a HashSet (or dedupe on insert).

Copilot · 2026-03-29T15:23:14Z

+| `--diff-remote` | Diff against the remote tracking branch. Implies `--wait`. Use this in prek/pre-commit. |
+| `--pre-push` | Read git pre-push hook stdin to determine changed files. Implies `--wait`. Use for raw git hooks. |
+| `--diff-base <sha>` | Compute changed files by diffing against a specific git ref. |
+| `--timeout <N>` | Timeout in seconds (default: 600 in `--pre-push` mode). |


Docs say --timeout defaults to 600 only in --pre-push mode, but the CLI currently also applies a 600s default for --diff-remote. Please align the README with the actual behavior (or change the code to match the README) so hook configs don’t rely on an undocumented default.

Suggested change

| `--timeout <N>` | Timeout in seconds (default: 600 in `--pre-push` mode). |

| `--timeout <N>` | Timeout in seconds (default: 600 in `--pre-push` or `--diff-remote` mode). |

Copilot · 2026-03-29T15:23:14Z

+Overrides the default timeout. When `--pre-push` is set, the default timeout is 600 seconds (10 minutes). Without `--pre-push`, existing behavior is unchanged (no default timeout).
+
+Precedence: `--no-timeout` > `--timeout <N>` > `--pre-push` default (600s) > existing default (none).


This spec states the only new default timeout is in --pre-push mode, but the current CLI implementation also defaults to 600s for --diff-remote. Update the spec to mention --diff-remote (or adjust the code) so the design doc matches actual behavior.

Suggested change

Overrides the default timeout. When `--pre-push` is set, the default timeout is 600 seconds (10 minutes). Without `--pre-push`, existing behavior is unchanged (no default timeout).

Precedence: `--no-timeout` > `--timeout <N>` > `--pre-push` default (600s) > existing default (none).

Overrides the default timeout. When `--pre-push` or `--diff-remote` is set, the default timeout is 600 seconds (10 minutes). Without these flags, existing behavior is unchanged (no default timeout).

Precedence: `--no-timeout` > `--timeout <N>` > `--pre-push`/`--diff-remote` default (600s) > existing default (none).

Copilot · 2026-03-29T15:23:15Z

+            let status = resp.status();
+            if let Ok(body_str) = resp.into_body().read_to_string() {
+                if status == 504 {
+                    return ReviewWaitResult::Timeout;
+                }
+                if let Ok(parsed) = serde_json::from_str(&body_str) {
+                    return ReviewWaitResult::Ok(parsed);
+                }
+            }
+            ReviewWaitResult::ServerUnavailable
+        }
+        Err(_) => ReviewWaitResult::Timeout,


review_wait can’t reliably distinguish timeout vs server-unavailable with the current ureq handling. ureq returns Err(Status(..)) for non-2xx responses by default, so the status == 504 branch is effectively unreachable, and Err(_) => Timeout will incorrectly treat connection failures and other HTTP errors as timeouts (breaking the documented exit codes). Handle ureq::Error::Status(code, resp) separately (mapping 504 -> Timeout, others -> ServerUnavailable), and only treat transport timeouts as Timeout.

Suggested change

let status = resp.status();

if let Ok(body_str) = resp.into_body().read_to_string() {

if status == 504 {

return ReviewWaitResult::Timeout;

}

if let Ok(parsed) = serde_json::from_str(&body_str) {

return ReviewWaitResult::Ok(parsed);

}

}

ReviewWaitResult::ServerUnavailable

}

Err(_) => ReviewWaitResult::Timeout,

if let Ok(body_str) = resp.into_body().read_to_string() {

if let Ok(parsed) = serde_json::from_str(&body_str) {

return ReviewWaitResult::Ok(parsed);

}

}

ReviewWaitResult::ServerUnavailable

}

Err(ureq::Error::Status(code, resp)) => {

// Non-2xx HTTP response from the server.

if code == 504 {

// Gateway Timeout: the server timed out waiting for a verdict.

ReviewWaitResult::Timeout

} else {

// Consume the body to avoid leaving it unread; ignore parse errors.

let _ = resp.into_body().read_to_string();

ReviewWaitResult::ServerUnavailable

}

}

Err(ureq::Error::Transport(t)) => {

// Distinguish actual transport timeouts from other transport errors.

if t.kind() == std::io::ErrorKind::TimedOut {

ReviewWaitResult::Timeout

} else {

ReviewWaitResult::ServerUnavailable

}

}

Copilot · 2026-03-29T15:23:15Z

+    // Push deletion — nothing to review
+    if local_sha == zero_sha {
+        return Err("Push deletion — nothing to review".into());


Push deletions are meant to be skipped (allow push) per the design, but this returns an error, which will bubble up and block the push in --pre-push mode. Consider returning a sentinel (e.g., Ok(None)) or exiting 0 from the dispatch path when local_sha is all zeros.

Suggested change

// Push deletion — nothing to review

if local_sha == zero_sha {

return Err("Push deletion — nothing to review".into());

// Push deletion — nothing to review; allow push and exit successfully

if local_sha == zero_sha {

eprintln!("Push deletion — nothing to review; skipping review.");

process::exit(0);

Copilot · 2026-03-29T15:23:15Z

+    let mut result = Vec::new();
+    for file in &files {
+        let annotations = state.bridge.get_annotations(file).unwrap_or_default();
+        result.push(serde_json::json!({
+            "file": file,
+            "annotations": annotations,
+        }));


This endpoint silently drops get_annotations errors via unwrap_or_default, which can hide real failures and produce misleading empty annotation sets. Since /rpc/annotations returns 500 on error, consider returning an error (or including per-file error details) here as well for consistency and debuggability.

Copilot · 2026-03-29T15:23:15Z

+
+Lists all annotations across all files in the given review session as JSON. This gives agents and humans a single command to retrieve everything flagged in a review.
+
+Requires a new server RPC endpoint (e.g., `GET /rpc/session.annotations?id=<session-id>`) since the current `list` command is file-based only. The server needs to track which files belong to a session — currently only `primary_file_path` is stored; multi-file sessions need a file index.


The spec suggests a GET /rpc/session.annotations?id=... endpoint, but the server/router and CLI client both implement it as a POST /rpc/session.annotations with a JSON body. Update the spec to match the implemented request shape so clients aren’t built against the wrong contract.

Suggested change

Requires a new server RPC endpoint (e.g., `GET /rpc/session.annotations?id=<session-id>`) since the current `list` command is file-based only. The server needs to track which files belong to a session — currently only `primary_file_path` is stored; multi-file sessions need a file index.

Requires a new server RPC endpoint (e.g., `POST /rpc/session.annotations` with JSON body `{ "session_id": "<session-id>" }`) since the current `list` command is file-based only. The server needs to track which files belong to a session — currently only `primary_file_path` is stored; multi-file sessions need a file index.

Copilot · 2026-03-29T15:23:16Z

+    // Try origin/HEAD first
+    let output = process::Command::new("git")
+        .args(["symbolic-ref", "refs/remotes/origin/HEAD"])
+        .output()?;
+    if output.status.success() {
+        let full_ref = String::from_utf8_lossy(&output.stdout).trim().to_string();
+        if let Some(branch) = full_ref.strip_prefix("refs/remotes/origin/") {
+            return Ok(branch.to_string());
+        }
+    }
+
+    // Fallback: check if main or master exist
+    for branch in ["main", "master"] {
+        let status = process::Command::new("git")
+            .args(["rev-parse", "--verify", &format!("origin/{}", branch)])
+            .stdout(process::Stdio::null())
+            .stderr(process::Stdio::null())
+            .status()?;
+        if status.success() {
+            return Ok(branch.to_string());
+        }


git_default_branch() returns just main/master (or strips origin/ from refs/remotes/origin/HEAD), but parse_pre_push_stdin later calls git merge-base <default_branch> HEAD. In repos without a local main branch (common when only origin/main exists), this merge-base will fail. Return a ref that is guaranteed to exist (e.g. origin/<branch>), or verify the local branch exists before using it.

Copilot · 2026-03-29T15:23:16Z

+    List {
+        /// File to list annotations for
+        file: Option<PathBuf>,
+        /// List all annotations for a review session
+        #[arg(long)]
+        session: Option<String>,
+    },


The CLI currently allows passing both a positional file and --session; the implementation silently prefers --session and ignores file. To avoid ambiguous UX, make these mutually exclusive in clap (e.g., conflicts_with) and/or use an ArgGroup to require exactly one of them.

Copilot · 2026-03-29T15:23:16Z

        #[arg(long)]
        wait: bool,
-        /// Timeout in seconds when using --wait (default: no timeout)
+        /// Timeout in seconds when using --wait (default: no timeout, or 600s with --pre-push)


Help text says the default timeout behavior changes only with --pre-push, but cmd_open_dispatch also applies a 600s default when --diff-remote is used. Either update this help text (and README/spec) to mention --diff-remote, or change the --diff-remote path to preserve the no-default timeout behavior.

Suggested change

/// Timeout in seconds when using --wait (default: no timeout, or 600s with --pre-push)

/// Timeout in seconds when using --wait (default: no timeout, or 600s with --pre-push/--diff-remote)

sam-phinizy and others added 7 commits March 29, 2026 10:13

docs: add pre-push review gate design spec for issue #46

09ab94a

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

style: cargo fmt

fdb748c

feat: enable pre-push review gate in this repo

f58ffb4

Add redpen-review hook to prek.toml and document the pre-push gate, CLI flags, exit codes, CI skip behavior, and agent feedback loop in README. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings March 29, 2026 15:18

Copilot started reviewing on behalf of sam-phinizy March 29, 2026 15:18 View session

Copilot AI reviewed Mar 29, 2026

View reviewed changes

sam-phinizy added 6 commits March 29, 2026 13:37

test: whitespace to trigger review gate

99adbe0

style: cargo fmt

6bc2593

fix: add always_run to redpen-review prek hook

4f9bd55

test: trigger review gate for feedback test

155e809

test: remove test comment

81fb2c3

docs: add doc comments to new pre-push gate functions

45082c6

sam-phinizy merged commit 3c75d8d into main Mar 29, 2026
8 of 15 checks passed

sam-phinizy deleted the worktree-pre-push-review-gate branch March 29, 2026 17:43

Navigation Menu

Uh oh!

feat: pre-push review gate (#46) - #48

feat: pre-push review gate (#46)#48
sam-phinizy merged 13 commits into
mainfrom
worktree-pre-push-review-gate

sam-phinizy commented Mar 29, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 29, 2026

Uh oh!

Copilot AI Mar 29, 2026

Uh oh!

Copilot AI Mar 29, 2026

Uh oh!

Copilot AI Mar 29, 2026

Uh oh!

Copilot AI Mar 29, 2026

Uh oh!

Copilot AI Mar 29, 2026

Uh oh!

Copilot AI Mar 29, 2026

Uh oh!

Copilot AI Mar 29, 2026

Uh oh!

Copilot AI Mar 29, 2026

Uh oh!

Copilot AI Mar 29, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	\| `--timeout <N>` \| Timeout in seconds (default: 600 in `--pre-push` mode). \|
	\| `--timeout <N>` \| Timeout in seconds (default: 600 in `--pre-push` or `--diff-remote` mode). \|

		Overrides the default timeout. When `--pre-push` is set, the default timeout is 600 seconds (10 minutes). Without `--pre-push`, existing behavior is unchanged (no default timeout).

		Precedence: `--no-timeout` > `--timeout <N>` > `--pre-push` default (600s) > existing default (none).


		Lists all annotations across all files in the given review session as JSON. This gives agents and humans a single command to retrieve everything flagged in a review.

		Requires a new server RPC endpoint (e.g., `GET /rpc/session.annotations?id=<session-id>`) since the current `list` command is file-based only. The server needs to track which files belong to a session — currently only `primary_file_path` is stored; multi-file sessions need a file index.

	/// Timeout in seconds when using --wait (default: no timeout, or 600s with --pre-push)
	/// Timeout in seconds when using --wait (default: no timeout, or 600s with --pre-push/--diff-remote)

Uh oh!

Conversation

sam-phinizy commented Mar 29, 2026

Summary

Known issue

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants