[NagyVikt]

What

A collection of gate-safety, security, and UX fixes for gx claude install, gx pr-review, and the mirror push path, plus a new gx pr respond sub-command for auto-fixing blocking review threads.

Why / Fixes

• False success banner (feat(claude-install,review-gate)): gx claude install printed a green success banner even when ownership was unavailable; now shows an honest warning. Adds lightweight head-poll and actionable mergeStateStatus hints for common blocked states (BLOCKED, DIRTY, BEHIND).
• Gate safety (fix(gate-safety)): Signal-killed hook exits (exit 128+N), push failures in the autofix path, and skipped checks were previously silently treated as success; they now surface as gate failures.
• Review correctness (fix(pr-review-correctness)): Fix prompt was passed on the command line (leaks into ps output); now delivered via stdin. Head-changed guard enforced on non-Lens path.
• Security hardening (fix(security)): Review-bot prompt injection hardened; mirror token no longer exposed in git remote URL or log output.
• Mirror auth (fix(mirror) ×2): Bearer auth is rejected by GitHub HTTPS Git; switched to Basic. Subsequent fix: suppress the mirror token when actions/checkout already persisted its own Authorization header.
• Head-SHA race (fix(pr-review) ×3): Capture head SHA before fetching diff to close stale-review race; abort before diff fetch when SHA capture fails; skip SHA abort when GitHub auth is unavailable (local-only runs).
• New gx pr respond (feat(pr-respond)): Reads open review threads, generates fix commits via Claude, and re-runs the gate.

Tests

• 1 569 passing / 3 known load flakes (pr-respond.test.js passes 30/30 when run in isolation).
• 35 new test cases across test/claude-install.test.js, test/pr-respond.test.js, and test/review-gate.test.js.
• npm run format:check:changed and npx biome lint both clean.

🤖 Generated with Claude Code

[NagyVikt]

🛡️ GitGuardex code-assist

3 finding(s) — 🟠 1 high · 🟡 2 medium

⛔ Merge gate: blocked — 1 blocking finding(s) (blocks on high/critical).

Severity	Location	Finding
🟠 high	src/pr-respond.js:508	Changing a file causes every blocking thread on that file to be resolved without confirming each defect was fixed.
🟡 medium	src/pr-respond.js:123	Filtering reviews to CHANGES_REQUESTED includes historical requests that a reviewer has subsequently approved or dismissed.
🟡 medium	src/pr-respond.js:499	Diagnostic logging corrupts the CLI's JSON output. When re-review throws, this console.log writes a plain-text prefix before cmdRespond

---

Provider codex · commit 02d9d8e · abide: skipped (no rubric (.abide/rubric.json); compile it first)

[NagyVikt]

Warning

🟠 HIGH · correctness

Changing a file causes every blocking thread on that file to be resolved without confirming each defect was fixed. When two threads concern different bugs in the same file, fixing only one clears both conversations; resolution also proceeds when re-review fails. This can remove blocking feedback while the reported defect remains.

[NagyVikt]

Important

🟡 MEDIUM · correctness

Filtering reviews to CHANGES_REQUESTED includes historical requests that a reviewer has subsequently approved or dismissed. The resulting reviewer set does not represent the latest review state, so auto-respond can modify code for feedback that no longer blocks the PR. Fetch the latest review state per author and select only currently active change requests.

[NagyVikt]

Important

🟡 MEDIUM · correctness

Diagnostic logging corrupts the CLI's JSON output. When re-review throws, this console.log writes a plain-text prefix before cmdRespond serializes the result, making --json output invalid; reply and resolution failures have the same problem. Send these diagnostics to stderr at all three logging sites.

[NagyVikt]

🛡️ GitGuardex code-assist

3 finding(s) — 🟠 1 high · 🟡 2 medium

⛔ Merge gate: blocked — 1 blocking finding(s) (blocks on high/critical).

Severity	Location	Finding
🟠 high	src/pr-respond.js:542	Changing a file automatically resolves every blocking thread in that file without confirming each defect was fixed.
🟡 medium	src/pr-respond.js:182	A subsequent COMMENTED or PENDING review incorrectly clears an active change request.
🟡 medium	src/pr-respond.js:127	Review thread coordinates are used without checking which side of the diff they address.

---

Provider codex · commit 68fd91a · abide: skipped (no rubric (.abide/rubric.json); compile it first)

[NagyVikt]

🛡️ GitGuardex code-assist

2 finding(s) — 🟠 1 high · 🟡 1 medium

⛔ Merge gate: blocked — 1 blocking finding(s) (blocks on high/critical).

Severity	Location	Finding
🟠 high	src/pr-respond.js:526	A completed re-review does not prove that a thread's defect was fixed.
🟡 medium	src/pr-respond.js:400	The local worktree is selected without verifying that its HEAD matches the PR head.

---

Provider codex · commit 68fd91a · abide: skipped (no rubric (.abide/rubric.json); compile it first)

[NagyVikt]

Warning

🟠 HIGH · correctness

A completed re-review does not prove that a thread's defect was fixed. This condition permits automatic resolution whenever the review returns any non-null result and the file changed, even if the review still reports the original HIGH/CRITICAL finding or another thread in the same file was the only issue addressed. This can clear blocking conversations while their defects remain.

Why this matters

The added tests do not exercise this resolution path.

[NagyVikt]

Important

🟡 MEDIUM · correctness

The local worktree is selected without verifying that its HEAD matches the PR head. A clean worktree can be behind after another contributor pushes; the responder then applies current review locations to stale code and commits a fix that the subsequent non-fast-forward push rejects. Fetch the PR head SHA and require or safely establish matching local HEAD before running the fix provider.

[NagyVikt]

🛡️ GitGuardex code-assist

2 finding(s) — 🟠 1 high · 🟡 1 medium

⛔ Merge gate: blocked — 1 blocking finding(s) (blocks on high/critical).

Severity	Location	Finding
🟠 high	src/pr-respond.js:572	Human review threads can be resolved without their requested changes being implemented.
🟡 medium	src/pr-respond.js:126	The review-state lookup silently excludes reviewers outside the latest 50 reviews.

---

Provider codex · commit 5654100 · abide: skipped (no rubric (.abide/rubric.json); compile it first)

[NagyVikt]

Warning

🟠 HIGH · correctness

Human review threads can be resolved without their requested changes being implemented. Touching the file and receiving no HIGH/CRITICAL findings from a fresh general review does not verify each original request. If two human threads concern the same file and only one is fixed, both can be automatically resolved, clearing unresolved review conversations.

Why this matters

Require confirmation against each original finding before resolving its thread, and cover partial fixes with a regression test.

[NagyVikt]

Important

🟡 MEDIUM · correctness

The review-state lookup silently excludes reviewers outside the latest 50 reviews. A reviewer with an older active CHANGES_REQUESTED review disappears from humanReviewers after enough subsequent reviews, so their unresolved threads are ignored and the command can incorrectly report no blocking threads. Paginate the review connection or explicitly refuse incomplete review history, as the thread lookup already does.

[NagyVikt]

🛡️ GitGuardex code-assist

2 finding(s) — 🟠 1 high · 🟡 1 medium

⛔ Merge gate: blocked — 1 blocking finding(s) (blocks on high/critical).

Severity	Location	Finding
🟠 high	src/pr-respond.js:439	A failed fetch bypasses the stale-worktree safeguard and still allows automatic edits.
🟡 medium	src/pr-respond.js:201	The review-history truncation check uses the wrong pagination direction.

---

Provider codex · commit 67439ae · abide: skipped (no rubric (.abide/rubric.json); compile it first)

[NagyVikt]

Important

🟡 MEDIUM · correctness

The review-history truncation check uses the wrong pagination direction. Because reviews are fetched with last:50, omitted older reviews are indicated by hasPreviousPage, while hasNextPage remains false. An active CHANGES_REQUESTED reviewer whose later interactions were only comments can therefore disappear from the computed blocking-reviewer set, silently skipping their unresolved threads.

[NagyVikt]

Warning

🟠 HIGH · correctness

A failed fetch bypasses the stale-worktree safeguard and still allows automatic edits. Git fetch can fail independently of the GitHub API, leaving current review coordinates applied to an older local checkout; the later push rejection occurs only after the provider has modified and committed files. Refuse the run when the remote head cannot be verified.

Suggested change

	const fetchResult = gitRun(worktreePath, ['fetch', 'origin', branch], { allowFailure: true });
	if (fetchResult.status !== 0) {
	return {
	status: 'skipped',
	reason: 'could not fetch the PR branch from origin; cannot verify worktree HEAD'
	};
	}
	if (fetchResult.status === 0) {

[NagyVikt]

🛡️ GitGuardex code-assist

1 finding(s) — 🟡 1 medium

✅ Merge gate: pass — no blocking findings (blocks on high/critical).

Severity	Location	Finding
🟡 medium	src/pr-respond.js:453-456	The freshness check reads a tracking ref that the fetch may not update.

---

Provider codex · commit 1068a90 · abide: skipped (no rubric (.abide/rubric.json); compile it first)

[NagyVikt]

Important

🟡 MEDIUM · correctness

The freshness check reads a tracking ref that the fetch may not update. When origin's configured fetch refspec excludes this branch, git fetch origin <branch> updates FETCH_HEAD while an existing origin/ ref remains stale. If the worktree matches that stale ref, this check accepts outdated code and applies review suggestions using coordinates from a newer PR revision.

Suggested change

	).trim();
	const remoteHead = String(
	gitRun(worktreePath, ['rev-parse', `refs/remotes/origin/${branch}`], { allowFailure: true })
	.stdout || ''
	const remoteHead = String(
	gitRun(worktreePath, ['rev-parse', 'FETCH_HEAD'], { allowFailure: true })
	.stdout || ''
	).trim();

[NagyVikt]

🛡️ GitGuardex code-assist

2 finding(s) — 🟠 2 high

⛔ Merge gate: blocked — 2 blocking finding(s) (blocks on high/critical).

Severity	Location	Finding
🟠 high	src/pr-respond.js:50	The responder never verifies that the local commit matches the PR head.
🟠 high	src/pr-respond.js:587	Thread resolution fails open when re-review returns no findings array.

---

Provider codex · commit 92cc97e · abide: skipped (no rubric (.abide/rubric.json); compile it first)

[NagyVikt]

Warning

🟠 HIGH · correctness

The responder never verifies that the local commit matches the PR head. Metadata contains only the branch name, while the later freshness check compares against origin, which can differ from the repository selected by gh. A matching branch in another remote can therefore receive fixes derived from unrelated PR coordinates, and the responder can subsequently resolve threads on the original PR.

Suggested change

	['pr', 'view', String(pr), '--json', 'headRefName,isCrossRepository'],
	['pr', 'view', String(pr), '--json', 'headRefName,isCrossRepository,headRefOid,headRepository'],

[NagyVikt]

Warning

🟠 HIGH · correctness

Thread resolution fails open when re-review returns no findings array. The preceding code treats a missing findings array as zero remaining blockers, but this guard rejects only null. A skipped or otherwise incomplete review result can therefore allow unresolved blocking threads to be automatically marked resolved without confirmation.

Suggested change

	continue;
	if (!reReviewResult || !Array.isArray(reReviewResult.findings)) {

[NagyVikt]

🛡️ GitGuardex code-assist

1 finding(s) — 🟡 1 medium

✅ Merge gate: pass — no blocking findings (blocks on high/critical).

Severity	Location	Finding
🟡 medium	test/pr-respond.test.js:765-768	The fixture cannot create the agent worktree. If checkout succeeds, the branch is already checked out in repoDir, so the subsequent

---

Provider codex · commit cbbb329 · abide: skipped (no rubric (.abide/rubric.json); compile it first)

[NagyVikt]

Important

🟡 MEDIUM · tests

The fixture cannot create the agent worktree. If checkout succeeds, the branch is already checked out in repoDir, so the subsequent worktree add refuses it; if --track fails, the branch does not exist. Both command failures are ignored, and prRespond returns skipped instead of no-threads. The dry-run fixture has the same problem.

Suggested change

	cp.spawnSync('git', ['-C', repoDir, 'checkout', '-b', 'agent/empty-fix', '--track'], {
	encoding: 'utf8',
	env: { ...process.env, GUARDEX_ALLOW_PRIMARY_BRANCH_SWITCH: '1' }
	});
	cp.spawnSync('git', ['-C', repoDir, 'branch', 'agent/empty-fix'], {
	encoding: 'utf8'
	});

[NagyVikt]

🛡️ GitGuardex code-assist

1 finding(s) — 🟡 1 medium

✅ Merge gate: pass — no blocking findings (blocks on high/critical).

Severity	Location	Finding
🟡 medium	templates/scripts/review-bot-watch.sh:451	Unchanged human review threads trigger the fix provider on every poll cycle.

---

Provider codex · commit aa8f085 · abide: skipped (no rubric (.abide/rubric.json); compile it first)

[NagyVikt]

Important

🟡 MEDIUM · performance

Unchanged human review threads trigger the fix provider on every poll cycle. prRespond deliberately leaves human threads unresolved, but fetchBlockingThreads keeps selecting them after a fix and reply. With auto-respond enabled, waiting for reviewer confirmation repeatedly invokes the provider and can produce duplicate replies or additional commits without new reviewer input.

[NagyVikt]

🛡️ GitGuardex code-assist

1 finding(s) — 🟡 1 medium

✅ Merge gate: pass — no blocking findings (blocks on high/critical).

Severity	Location	Finding
🟡 medium	test/pr-respond.test.js:814	The successful auto-fix workflow has no test coverage. These tests stop at refusal, dry-run, or no-threads paths, so they never exercise

---

Provider codex · commit aa8f085 · abide: skipped (no rubric (.abide/rubric.json); compile it first)

[NagyVikt]

Important

🟡 MEDIUM · tests

The successful auto-fix workflow has no test coverage. These tests stop at refusal, dry-run, or no-threads paths, so they never exercise committing, pushing, re-reviewing, or the conditions that automatically resolve GitHub threads. A regression could silently resolve an unfixed blocking thread without failing this suite.

Why this matters

Add workflow tests covering successful resolution, failed re-review, remaining blocking findings, untouched files, and human threads remaining unresolved.

[NagyVikt]

🛡️ GitGuardex code-assist

5 finding(s) — 🟠 2 high · 🟡 1 medium · 🔵 2 low

⛔ Merge gate: blocked — 2 blocking finding(s) (blocks on high/critical).

Severity	Location	Finding
🟠 high	src/pr-respond.js:193	Any GitHub user who submits a CHANGES_REQUESTED review is treated as a trusted human reviewer, so their free-text comment is fed straight
🟠 high	templates/scripts/review-bot-watch.sh:451	Auto-respond runs on every poll cycle even when the head SHA is unchanged, so the same still-open threads are re-fixed over and over.
🟡 medium	test/pr-respond.test.js:883	The riskiest part of prRespond has no tests: the post-fix path (fixed, push-failed, the re-review gating, and the reply/resolve loop).
🔵 low	src/pr-respond.js:26-28	readProtectedBranches and FINDING_MARKER_PREFIX are imported but never used.
🔵 low	src/finish/review-gate.js:606	The BEHIND hint hardcodes 'gx pr sync', while the DIRTY hint right above it uses ${TOOL_NAME}.

---

Provider claude · commit d015626 · abide: skipped (no rubric (.abide/rubric.json); compile it first)

[NagyVikt]

Warning

🟠 HIGH · security

Any GitHub user who submits a CHANGES_REQUESTED review is treated as a trusted human reviewer, so their free-text comment is fed straight into the fix provider's prompt. The provider's output is then committed and pushed automatically. On a public repo, non-collaborators can leave CHANGES_REQUESTED reviews, and with GUARDEX_REVIEW_AUTO_RESPOND=1 the watch loop runs this unattended.

Why this matters

That is a prompt-injection path to arbitrary code being written and pushed to agent/* branches. Fix: add authorAssociation to the reviews{nodes{...}} selection, and only add a reviewer to latestStateByReviewer when the association is OWNER, MEMBER or COLLABORATOR, or when the reviewer has write permission.

[NagyVikt]

Warning

🟠 HIGH · correctness

Auto-respond runs on every poll cycle even when the head SHA is unchanged, so the same still-open threads are re-fixed over and over. Several outcomes leave threads open and eligible again: provider no-op, a thread whose path was not in changedFiles, re-review still flagging the file, a failed resolve, and bot threads (which, unlike human threads, have no 'already replied' check).

Why this matters

Each repeat costs a provider run, and when it does change something it adds a push and a pr-review --post. That push produces a new SHA, so on the next cycle process_one_pr reviews and posts again, duplicating the re-review respond already posted. Without persisted per-thread or per-SHA attempt state, this can loop indefinitely, burning provider quota and piling up commits and review threads. Fix: record which thread IDs and head SHA were last attempted in the state file, and skip them until the SHA or the thread's latest comment changes.

[NagyVikt]

Note

🔵 LOW · maintainability

readProtectedBranches and FINDING_MARKER_PREFIX are imported but never used. They will fail a no-unused-vars lint rule and suggest a protected-branch check that does not actually happen.

Suggested change

	const { gitRun, readProtectedBranches, currentBranchName } = require('./git');
	const { runReviewFix } = require('./review-fix');
	const { runPrReview, MARKER, FINDING_MARKER_PREFIX } = require('./pr-review');
	const { gitRun, currentBranchName } = require('./git');
	const { runReviewFix } = require('./review-fix');
	const { runPrReview, MARKER } = require('./pr-review');

[NagyVikt]

Note

🔵 LOW · maintainability

The BEHIND hint hardcodes 'gx pr sync', while the DIRTY hint right above it uses ${TOOL_NAME}. The two will drift if the CLI name differs or changes.

Suggested change

	BEHIND: 'update the branch to head of base (gx pr sync)',
	BEHIND: `update the branch to head of base (${TOOL_NAME} pr sync)`,

[NagyVikt]

Important

🟡 MEDIUM · tests

The riskiest part of prRespond has no tests: the post-fix path (fixed, push-failed, the re-review gating, and the reply/resolve loop). Nothing checks that human threads are replied to but never resolved. Nothing checks that threads are left open when re-review throws or still reports a blocking finding on the same path.

Why this matters

Nothing checks that threads on files outside changedFiles are skipped, or that a push failure returns fixSha. The local-HEAD vs headRefOid staleness refusal is also untested. Add runner/deps-injected tests that stub runReviewFix/runPrReview results and assert on resolveThread/replyToThread calls.

[NagyVikt]

🛡️ GitGuardex code-assist

3 finding(s) — 🟠 1 high · 🟡 2 medium

⛔ Merge gate: blocked — 1 blocking finding(s) (blocks on high/critical).

Severity	Location	Finding
🟠 high	src/pr-respond.js:198	Any GitHub user who submits a CHANGES_REQUESTED review can make the auto-fix provider edit, commit and push to the agent branch.
🟡 medium	templates/scripts/review-bot-watch.sh:431-435	Any failure of pr respond clears the backoff, so permanent refusals and provider failures re-run on every poll.
🟡 medium	test/pr-respond.test.js:11	The riskiest part of prRespond has no tests: the post-fix reply/resolve path and the HEAD-staleness refusal.

---

Provider claude · commit 1e186ca · abide: skipped (no rubric (.abide/rubric.json); compile it first)

[NagyVikt]

Warning

🟠 HIGH · security

Any GitHub user who submits a CHANGES_REQUESTED review can make the auto-fix provider edit, commit and push to the agent branch. humanReviewers is built from every review author, with no check of the reviewer's relationship to the repository. On a public repo, anyone with read access can submit a 'Request changes' review.

Why this matters

Their inline comment body then becomes a 'high' finding that drives the codex/claude fix provider in the local worktree, and the result is pushed to origin. This also runs unattended every poll when GUARDEX_REVIEW_AUTO_RESPOND=1. The reviewer: prefix in threadToFinding is not a real boundary against prompt injection. Add authorAssociation to the reviews selection in the query (line 133). Then only count reviewers whose association is OWNER, MEMBER or COLLABORATOR (or who have write permission) before treating their threads as actionable.

[NagyVikt]

Important

🟡 MEDIUM · performance

Any failure of pr respond clears the backoff, so permanent refusals and provider failures re-run on every poll. Exit 1 covers more than transient errors. It also covers 'skipped' (no local worktree, dirty worktree, HEAD mismatch, more than 100 threads), 'fix-failed' and 'push-failed'. Because the failure branch resets RESPOND_SKIP_REMAINING to 0, each agent PR re-invokes gx pr respond every interval.

Why this matters

For 'fix-failed' that means paying for a full fix-provider run every 5+ seconds on the same unchanged SHA. Arm the same SHA-keyed backoff on failure. A new push still resets it immediately.

Suggested change

	else
	echo "[review-bot-watch] respond: PR #${pr} skipped or failed (exit=${respond_exit})" >&2
	# Clear backoff on failure so we retry next cycle.
	RESPOND_SKIP_REMAINING["$pr"]=0
	fi
	else
	echo "[review-bot-watch] respond: PR #${pr} skipped or failed (exit=${respond_exit})" >&2
	# Back off on failure too: permanent refusals (no worktree, fork, dirty tree)
	# and provider failures would otherwise re-invoke the fix provider every cycle.
	# A new head SHA still resets the backoff immediately.
	if [[ -n "$current_sha" ]]; then
	LAST_RESPOND_SHA["$pr"]="$current_sha"
	RESPOND_SKIP_REMAINING["$pr"]="$RESPOND_BACKOFF_POLLS"
	fi
	fi

[NagyVikt]

Important

🟡 MEDIUM · tests

The riskiest part of prRespond has no tests: the post-fix reply/resolve path and the HEAD-staleness refusal. The suite covers parsing, filtering, refusals, dry-run and no-threads.

Why this matters

It never exercises the 'fixed' branch, so nothing pins these behaviours: a thread is resolved only when its file changed and the re-review reports no blocking finding on that path; all threads stay open when the re-review throws; human threads get a reply but are never resolved; push-failed returns fixSha. The headRefOid mismatch refusal is also untested. A regression in any of these would silently resolve unfixed threads or push without verification. Add prRespond tests that inject runReviewFix / runPrReview via deps and a runner that records the reply and resolve mutations.

[NagyVikt]

🛡️ GitGuardex code-assist

6 finding(s) — 🟡 5 medium · 🔵 1 low

✅ Merge gate: pass — no blocking findings (blocks on high/critical).

Severity	Location	Finding
🟡 medium	src/pr-respond.js:652	Threads are resolved even when the re-review was never posted to the PR.
🟡 medium	src/pr-respond.js:614	The re-review runs right after git push, without waiting for GitHub's PR head to reach the fix commit.
🟡 medium	src/pr-respond.js:305-307	Follow-up comments on human reviewer threads never reach the fix provider.
🟡 medium	templates/scripts/review-bot-watch.sh:433-434	Every failed respond attempt clears the backoff, so persistent failures re-run on every poll cycle.
🟡 medium	templates/scripts/review-bot-watch.sh:412-415	Auto-respond has no cap on fix rounds per PR. Each successful respond pushes a new SHA.
🔵 low	src/cli/commands/pr.js:470	The skipped-thread summary misstates why threads were left open.

---

Provider claude · commit 2e377a1 · abide: skipped (no rubric (.abide/rubric.json); compile it first)

[NagyVikt]

Important

🟡 MEDIUM · correctness

Threads are resolved even when the re-review was never posted to the PR. Only a thrown error (null result) or a missing findings array keeps threads open. If runPrReview returns posted: false (for example, GitHub auth was unavailable) or a provider returns an empty findings array, the bot still replies and resolves threads with no published evidence that the fixed diff was examined.

Why this matters

review-gate.js explicitly refuses this case in requirePostedReview, because an unposted or empty review looks exactly like a clean pass.

Suggested change

	if (!reReviewResult || !Array.isArray(reReviewResult.findings)) {
	if (!reReviewResult || !reReviewResult.posted || !Array.isArray(reReviewResult.findings)) {

[NagyVikt]

Important

🟡 MEDIUM · correctness

The re-review runs right after git push, without waiting for GitHub's PR head to reach the fix commit. GitHub updates headRefOid asynchronously, so runPrReview({ pr }) can fetch and post a review of the pre-fix diff. That stale review then drives thread resolution. Real fixes may be reported as still blocking, and a stale clean verdict can resolve threads.

Why this matters

review-gate.js guards this same transition with waitForPullRequestHead after every push. Call it, or an equivalent poll against fixSha, before re-reviewing, and treat a timeout as 'leave threads open'.

[NagyVikt]

Important

🟡 MEDIUM · correctness

Follow-up comments on human reviewer threads never reach the fix provider. fetchBlockingThreads re-queues a human thread when the reviewer replies after the bot (for example, 'Actually, also handle the timeout case'), but threadToFinding builds the message only from comments.nodes[0]. The provider re-applies the original request and ignores the follow-up.

Why this matters

The bot then replies 'Applied fix' and stops re-queuing, which buries the reviewer's actual new request. Include the latest comment when its author is the same trusted reviewer. Do not include it for arbitrary repliers, so untrusted text stays out of the prompt.

[NagyVikt]

Important

🟡 MEDIUM · performance

Every failed respond attempt clears the backoff, so persistent failures re-run on every poll cycle. Respond exits 1 for deterministic refusals: no local worktree (common for agent PRs created on another machine), dirty worktree, HEAD mismatch, more than 50 reviews or more than 100 threads. It also exits 1 for fix-failed.

Why this matters

With the backoff cleared, the watcher repeats several gh/GraphQL calls per PR per interval indefinitely. On fix-failed it re-invokes the paid fix provider every cycle on the same SHA. Arm the backoff on failure too. A new SHA still resets it through the else branch above.

Suggested change

	# Clear backoff on failure so we retry next cycle.
	RESPOND_SKIP_REMAINING["$pr"]=0
	# Back off on failure too: deterministic refusals (no local worktree, dirty
	# worktree) and provider failures would otherwise re-run every poll. A new
	# head SHA still resets the backoff above.
	RESPOND_SKIP_REMAINING["$pr"]="$RESPOND_BACKOFF_POLLS"

[NagyVikt]

Important

🟡 MEDIUM · correctness

Auto-respond has no cap on fix rounds per PR. Each successful respond pushes a new SHA. The next poll sees a changed SHA, resets the backoff, re-reviews via process_one_pr, and responds again. If the provider keeps producing fixes that the review still flags, the loop runs unbounded, with provider cost and push noise on every cycle. The review gate bounds this with maxFixRounds.

Why this matters

Track a per-PR respond-round counter here and stop after a configurable limit (for example, GUARDEX_RESPOND_MAX_ROUNDS).

[NagyVikt]

Note

🔵 LOW · maintainability

The skipped-thread summary misstates why threads were left open. skippedReply also includes threads whose file changed but the re-review was unavailable or still blocking, and human-reviewer threads that were replied to but intentionally left unresolved. Labeling all of them 'untouched (path not in changed files)' misleads the operator about what happened.

Suggested change

	logInfo(` ${result.skippedReply.length} thread(s) untouched (path not in changed files).`);
	logInfo(` ${result.skippedReply.length} thread(s) left open (file unchanged, re-review unconfirmed/still blocking, or awaiting human reviewer).`);

[NagyVikt]

🛡️ GitGuardex code-assist

4 finding(s) — 🟡 3 medium · 🔵 1 low

✅ Merge gate: pass — no blocking findings (blocks on high/critical).

Severity	Location	Finding
🟡 medium	templates/scripts/review-bot-watch.sh:388-391	declare -A runs at top level on every start, so the watcher crashes on bash 3.2 (macOS /bin/bash) even when auto-respond is off.
🟡 medium	src/pr-respond.js:614-624	The re-review runs right after git push without waiting for GitHub to report the new PR head, so it can review and post against the stale
🟡 medium	src/pr-respond.js:601	The 'fixed' path has no test coverage: push, re-review, reply, and the rules for resolving or skipping threads.
🔵 low	src/cli/commands/pr.js:470	This summary line gives the wrong reason for most skipped threads.

---

Provider claude · commit 773cbbf · abide: skipped (no rubric (.abide/rubric.json); compile it first)

[NagyVikt]

Important

🟡 MEDIUM · correctness

declare -A runs at top level on every start, so the watcher crashes on bash 3.2 (macOS /bin/bash) even when auto-respond is off. bash 3.2 rejects declare -A with "invalid option". Under the script's set -e, that error kills the watcher for every user, not only those who set GUARDEX_REVIEW_AUTO_RESPOND=1. PR numbers are integers, so plain indexed arrays work here and need no associative arrays.

Why this matters

The comment on lines 388-389 also says the backoff resets on a non-zero exit, but try_respond_pr actually arms a 1-poll backoff.

Suggested change

	# backoff resets immediately when the SHA changes or when respond exits non-zero
	# (so a transient failure is retried on the next cycle).
	declare -A LAST_RESPOND_SHA
	declare -A RESPOND_SKIP_REMAINING
	# backoff resets immediately when the SHA changes; a non-zero respond exit arms a
	# 1-poll mini-backoff so transient failures retry soon without hammering every cycle.
	# PR numbers are integers, so plain indexed arrays work here and stay compatible
	# with bash 3.2 (macOS /bin/bash), which has no `declare -A`.
	LAST_RESPOND_SHA=()
	RESPOND_SKIP_REMAINING=()

[NagyVikt]

Important

🟡 MEDIUM · correctness

The re-review runs right after git push without waiting for GitHub to report the new PR head, so it can review and post against the stale pre-fix diff. review-gate.js waits with waitForPullRequestHead before re-reviewing for exactly this reason ("Refusing to re-review a stale PR diff"). Without that wait, the re-review can post duplicate findings for code that is already fixed.

Why this matters

Thread resolution on lines below is then decided by a review of the wrong commit, which can leave fixed threads open or resolve threads on the wrong evidence. Wait for the PR head to equal the pushed fix SHA, and treat a timeout like a failed re-review, before calling runPrReview.

[NagyVikt]

Important

🟡 MEDIUM · tests

The 'fixed' path has no test coverage: push, re-review, reply, and the rules for resolving or skipping threads. This is the riskiest logic in the PR. It decides when threads get resolved: a bot thread is resolved only if its file changed and the re-review has no blocking finding on that path. Human threads get a reply but are never resolved. A failed re-review leaves all threads open.

Why this matters

A push failure returns 'push-failed' with fixSha. test/pr-respond.test.js covers only the refusal, dry-run, and no-threads paths. Add runner/deps fakes for runReviewFix and runPrReview that assert which thread IDs land in resolved and which land in skippedReply.

[NagyVikt]

Note

🔵 LOW · maintainability

This summary line gives the wrong reason for most skipped threads. skippedReply also holds human-reviewer threads that were fixed and replied to, threads still flagged by the re-review, and threads left open because the re-review failed. Labelling them all "path not in changed files" sends the operator to the wrong cause.

Suggested change

	logInfo(` ${result.skippedReply.length} thread(s) untouched (path not in changed files).`);
	logInfo(` ${result.skippedReply.length} thread(s) left open (file not changed, re-review still blocking or failed, or awaiting human reviewer).`);