feat: migrate push-approved check from file-based to server-based by sam-phinizy · Pull Request #58 · phin-tech/redpen

sam-phinizy · 2026-04-02T00:52:09Z

Closes #47

Migrates the push-approved signal from a file-based check to querying the Red Pen server.

Changes:

check-git-push.sh: Now queries /rpc/push.check on the Red Pen server (2s timeout) as primary check. Falls back to legacy file signal when server isn't running.
crates/redpen-server/src/lib.rs: New /rpc/push.check endpoint that iterates tracked sessions and checks for an approved verdict via the bridge.
plugin/skills/review-code/SKILL.md: Updated to note server-based approval is primary, file signal is fallback.

🤖 Generated with Claude Code

Add /rpc/push.check endpoint to the redpen server that checks all tracked sessions for an "approved" verdict. Update the git push hook to query the server first, falling back to the legacy file signal for environments where the server isn't running. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Migrates the git push review gate from consuming a per-repo signal file to querying the local Red Pen HTTP server for an approved review state, with the legacy file check kept as a fallback.

Changes:

Update the pre-push hook to call /rpc/push.check with a short timeout before falling back to .redpen/signals/push-approved.
Add a new /rpc/push.check endpoint to redpen-server to report whether an approved verdict exists.
Update the /review-code skill documentation to describe server-based approval as primary and the signal file as fallback.

Reviewed changes

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

File	Description
plugin/skills/review-code/SKILL.md	Documents server-tracked approval as the primary push gate signal (legacy file as fallback).
plugin/hooks/scripts/check-git-push.sh	Adds server RPC query for push approval before checking the legacy signal file.
crates/redpen-server/src/lib.rs	Adds request/response types and a new `/rpc/push.check` handler/route.

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

Copilot · 2026-04-02T00:56:23Z

+  port="${REDPEN_PORT:-8789}"
+
+  # Primary: check if the Red Pen server reports push-approved
+  server_response=$(curl -sf --max-time 2 "http://127.0.0.1:${port}/rpc/push.check" \
+    -X POST -H "Content-Type: application/json" \
+    -d "{\"repo_root\": \"$repo_root\"}" 2>/dev/null || echo "")
+


The hook hardcodes a default port (8789) for the server, but redpen-server binds to an ephemeral port (TcpListener::bind("127.0.0.1:0")) and writes it to ~/.config/redpen/server.json. As-is, the curl will almost always miss the running server and silently fall back to the legacy file gate.

Consider reading the port from the discovery file (same logic as crates/redpen-cli/src/server_client.rs::server_url()), or delegating this check to redpen CLI so the hook stays in sync with server discovery.

Suggested change

port="${REDPEN_PORT:-8789}"

# Primary: check if the Red Pen server reports push-approved

server_response=$(curl -sf --max-time 2 "http://127.0.0.1:${port}/rpc/push.check" \

-X POST -H "Content-Type: application/json" \

-d "{\"repo_root\": \"$repo_root\"}" 2>/dev/null || echo "")

# Determine Red Pen server port:

# 1. Honor REDPEN_PORT if explicitly set.

# 2. Otherwise, read the discovery file written by redpen-server.

if [ -n "${REDPEN_PORT:-}" ]; then

port="$REDPEN_PORT"

else

config_dir="${XDG_CONFIG_HOME:-$HOME/.config}"

discovery_file="$config_dir/redpen/server.json"

if [ -f "$discovery_file" ]; then

port="$(jq -r '.port // empty' "$discovery_file" 2>/dev/null || echo "")"

else

port=""

fi

fi

server_response=""

if [ -n "$port" ]; then

# Primary: check if the Red Pen server reports push-approved

server_response=$(curl -sf --max-time 2 "http://127.0.0.1:${port}/rpc/push.check" \

-X POST -H "Content-Type: application/json" \

-d "{\"repo_root\": \"$repo_root\"}" 2>/dev/null || echo "")

fi

Copilot · 2026-04-02T00:56:24Z

+  server_response=$(curl -sf --max-time 2 "http://127.0.0.1:${port}/rpc/push.check" \
+    -X POST -H "Content-Type: application/json" \
+    -d "{\"repo_root\": \"$repo_root\"}" 2>/dev/null || echo "")


-d "{\"repo_root\": \"$repo_root\"}" does not JSON-escape $repo_root. Repo paths containing quotes/backslashes/newlines will produce invalid JSON (and can change the request payload).

Build the JSON with a proper escaper (e.g., jq -nc --arg repo_root "$repo_root" '{repo_root:$repo_root}') and pass it to curl.

Suggested change

server_response=$(curl -sf --max-time 2 "http://127.0.0.1:${port}/rpc/push.check" \

-X POST -H "Content-Type: application/json" \

-d "{\"repo_root\": \"$repo_root\"}" 2>/dev/null || echo "")

json_payload=$(jq -nc --arg repo_root "$repo_root" '{repo_root:$repo_root}')

server_response=$(curl -sf --max-time 2 "http://127.0.0.1:${port}/rpc/push.check" \

-X POST -H "Content-Type: application/json" \

-d "$json_payload" 2>/dev/null || echo "")

Copilot · 2026-04-02T00:56:24Z

+async fn rpc_push_check(
+    AxumState(state): AxumState<ServerState>,
+    Json(_req): Json<PushCheckRequest>,
+) -> impl IntoResponse {
+    // Check all tracked sessions for a recent approval via the persisted state.
+    let ids = state.sessions.session_ids().await;
+    for session_id in &ids {
+        if let Ok(Some(status)) = state.bridge.review_session_status(session_id) {
+            if status.verdict.as_deref() == Some("approved") {
+                return Json(PushCheckResponse { approved: true });
+            }


PushCheckRequest.repo_root is accepted (and the hook sends it), but the handler ignores it (Json(_req)). That means any approved session tracked by the server will allow pushes for all repos, which is weaker than the legacy per-repo signal file.

Use repo_root to scope approvals (e.g., only treat a session as applicable if its persisted ReviewSessionState.file is under the requested repo_root, or explicitly store repo_root with the session when it’s created).

Copilot · 2026-04-02T00:56:24Z

+/// Check whether any session has an "approved" verdict (used by the git push hook).
+async fn rpc_push_check(
+    AxumState(state): AxumState<ServerState>,
+    Json(_req): Json<PushCheckRequest>,
+) -> impl IntoResponse {
+    // Check all tracked sessions for a recent approval via the persisted state.
+    let ids = state.sessions.session_ids().await;
+    for session_id in &ids {
+        if let Ok(Some(status)) = state.bridge.review_session_status(session_id) {
+            if status.verdict.as_deref() == Some("approved") {
+                return Json(PushCheckResponse { approved: true });
+            }
+        }
+    }
+
+    Json(PushCheckResponse { approved: false })
+}


The legacy gate was one-time use (the signal file is consumed). rpc_push_check returns approved: true for any previously-approved session and never “consumes” that approval or checks recency, so a single approval can effectively allow unlimited future pushes.

If the intent is still “approve the next push”, consider adding consumption/TTL semantics (e.g., record an approval timestamp and require it to be within a short window, or add a separate endpoint to atomically consume an approval token).

Copilot · 2026-04-02T00:56:24Z

+    /// Get all tracked session IDs.
+    pub async fn session_ids(&self) -> Vec<String> {
+        self.session_files.lock().await.keys().cloned().collect()
+    }


session_ids() exposes all keys from session_files, but session_files entries are never removed. With rpc_push_check looping over all session IDs on every push, this can become an unbounded O(n) scan over time.

Consider pruning completed/old sessions from session_files (or tracking only “active/recent” sessions for push checks) to keep the check bounded.

Copilot · 2026-04-02T00:56:25Z

@@ -453,6 +487,7 @@ pub fn build_router(bridge: Arc<dyn AppBridge>, sessions: Arc<ReviewSessions>) -
        .route("/rpc/review", post(rpc_review))
        .route("/rpc/review.pr", post(rpc_review_pr))
        .route("/rpc/session.annotations", post(rpc_session_annotations))
+        .route("/rpc/push.check", post(rpc_push_check))


There are extensive tests for other RPC endpoints in this module, but no test coverage for the new /rpc/push.check behavior. Adding a test would help prevent regressions (e.g., when multiple sessions exist, or when one session is approved).

A minimal test could set up a MockBridge that returns an approved verdict for a known session ID and verify the endpoint returns {approved: true}.

Copilot AI review requested due to automatic review settings April 2, 2026 00:52

sam-phinizy merged commit 9eeda36 into main Apr 2, 2026
7 of 16 checks passed

sam-phinizy deleted the feat/server-push-check branch April 2, 2026 00:52

Copilot started reviewing on behalf of sam-phinizy April 2, 2026 00:52 View session

Copilot AI reviewed Apr 2, 2026

View reviewed changes

Navigation Menu

Uh oh!

feat: migrate push-approved check from file-based to server-based - #58

feat: migrate push-approved check from file-based to server-based#58
sam-phinizy merged 1 commit into
mainfrom
feat/server-push-check

sam-phinizy commented Apr 2, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 2, 2026

Uh oh!

Copilot AI Apr 2, 2026

Uh oh!

Copilot AI Apr 2, 2026

Uh oh!

Copilot AI Apr 2, 2026

Uh oh!

Copilot AI Apr 2, 2026

Uh oh!

Copilot AI Apr 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-  port="${REDPEN_PORT:-8789}"
-  # Primary: check if the Red Pen server reports push-approved
-  server_response=$(curl -sf --max-time 2 "http://127.0.0.1:${port}/rpc/push.check" \
-    -X POST -H "Content-Type: application/json" \
-    -d "{\"repo_root\": \"$repo_root\"}" 2>/dev/null || echo "")
+  # Determine Red Pen server port:
+  # 1. Honor REDPEN_PORT if explicitly set.
+  # 2. Otherwise, read the discovery file written by redpen-server.
+  if [ -n "${REDPEN_PORT:-}" ]; then
+    port="$REDPEN_PORT"
+  else
+    config_dir="${XDG_CONFIG_HOME:-$HOME/.config}"
+    discovery_file="$config_dir/redpen/server.json"
+    if [ -f "$discovery_file" ]; then
+      port="$(jq -r '.port // empty' "$discovery_file" 2>/dev/null || echo "")"
+    else
+      port=""
+    fi
+  fi
+  server_response=""
+  if [ -n "$port" ]; then
+    # Primary: check if the Red Pen server reports push-approved
+    server_response=$(curl -sf --max-time 2 "http://127.0.0.1:${port}/rpc/push.check" \
+      -X POST -H "Content-Type: application/json" \
+      -d "{\"repo_root\": \"$repo_root\"}" 2>/dev/null || echo "")
+  fi

Uh oh!

Conversation

sam-phinizy commented Apr 2, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants