Skip to content

fix(guardrails): PR #90 review findings + 14 test cases #139

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
terisuke merged 2 commits into from
Apr 8, 2026
Merged

fix(guardrails): PR #90 review findings + 14 test cases #139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5bf4596
fix(guardrails): address PR #90 review findings + add 14 test cases
terisuke Apr 8, 2026
b4e6a11
fix(guardrails): address review HIGH-1 — kill orphaned git fetch on t…
terisuke Apr 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 31 additions & 10 deletions packages/guardrails/profile/plugins/guardrail.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,12 +160,12 @@ async function git(dir: string, args: string[]) {
stdout: "pipe",
stderr: "pipe",
})
const [stdout, stderr] = await Promise.all([
const [stdout, stderr, code] = await Promise.all([
new Response(proc.stdout).text(),
new Response(proc.stderr).text(),
proc.exited,
])
return { stdout, stderr }
return { stdout, stderr, code }
}

function free(data: {
Expand Down Expand Up @@ -549,8 +549,22 @@ export default async function guardrail(input: {
if (flag(data.git_freshness_checked)) return
await mark({ git_freshness_checked: true })
try {
const fetchCheck = await git(input.worktree, ["fetch", "--dry-run"])
if (fetchCheck.stdout.trim() || fetchCheck.stderr.includes("From")) {
const proc = Bun.spawn(["git", "-C", input.worktree, "fetch", "--dry-run"], {
stdout: "pipe",
stderr: "pipe",
})
const fetchResult = await Promise.race([
Promise.all([
new Response(proc.stdout).text(),
new Response(proc.stderr).text(),
proc.exited,
]).then(([stdout, stderr, code]) => ({ stdout, stderr, code })),
Bun.sleep(5000).then(() => {
proc.kill()
return null
}),
])
if (fetchResult && fetchResult.code === 0 && (fetchResult.stdout.trim() || fetchResult.stderr.includes("From"))) {
Comment on lines +556 to +567
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Promise.race timeout prevents blocking the chat pipeline, but the underlying git fetch --dry-run process is not cancelled when the sleep wins. This can leave a long-running git process (and open pipes) alive for the rest of the session, and it can still fail later outside the awaited control-flow. Consider spawning git fetch inline here and explicitly killing the process on timeout (or using a Bun/process-level timeout mechanism) so the timeout actually bounds resource usage.

Copilot uses AI. Check for mistakes.
out.parts.push({
id: crypto.randomUUID(),
sessionID: out.message.sessionID,
Expand All @@ -565,7 +579,7 @@ export default async function guardrail(input: {
// Branch hygiene: surface stored branch warning from session.created
const branchWarn = str(data.branch_warning)
if (branchWarn) {
const statusCheck = await git(input.worktree, ["status", "--porcelain"]).catch(() => ({ stdout: "", stderr: "" }))
const statusCheck = await git(input.worktree, ["status", "--porcelain"]).catch(() => ({ stdout: "", stderr: "", code: 1 }))
const dirty = statusCheck.stdout.trim().length > 0 && !statusCheck.stderr.trim()
out.parts.push({
id: crypto.randomUUID(),
Expand Down Expand Up @@ -630,7 +644,7 @@ export default async function guardrail(input: {
throw new Error(text("shell access to protected files"))
}
// [W9] pre-merge: tier-aware gate + CRITICAL/HIGH block (consolidated)
if (/\b(git\s+merge|gh\s+pr\s+merge)\b/i.test(cmd)) {
if (/\bgit\s+merge(\s|$)/i.test(cmd) || /\bgh\s+pr\s+merge(\s|$)/i.test(cmd)) {
// Check CRITICAL/HIGH first (applies to all tiers)
const criticalCount = num(bashData.review_critical_count)
const highCount = num(bashData.review_high_count)
Expand Down Expand Up @@ -791,7 +805,7 @@ export default async function guardrail(input: {
throw new Error(text("branch rename/force-move blocked: prevents commit guard bypass"))
}
// Enforce soak time: develop→main merge requires half-day minimum
if (/\b(git\s+merge|gh\s+pr\s+merge)\b/i.test(cmd)) {
if (/\bgit\s+merge(\s|$)/i.test(cmd) || /\bgh\s+pr\s+merge(\s|$)/i.test(cmd)) {
const lastMerge = str(bashData.last_merge_at)
if (lastMerge) {
const elapsed = Date.now() - new Date(lastMerge).getTime()
Expand Down Expand Up @@ -1017,9 +1031,16 @@ export default async function guardrail(input: {
edit_count_since_check: 0,
})
}
// Reset review_state on mutating bash commands (sed -i, redirects, etc.)
if (bash(cmd)) {
await mark({
edits_since_review: num(data.edits_since_review) + 1,
review_state: "",
})
}
}

if ((item.tool === "edit" || item.tool === "write") && file) {
if ((item.tool === "edit" || item.tool === "write" || item.tool === "apply_patch") && file) {
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apply_patch is treated like an edit in the tool.execute.after hook (it increments edit counters and resets review_state), but it’s not handled in tool.execute.before. That means apply_patch can bypass the existing preflight protections (secret/config deny checks, version baseline regression check, context-budget gate), allowing policy-protected files or version downgrades to slip through via patches. Consider extending the tool.execute.before logic to include apply_patch anywhere edit/write are treated as mutating operations (deny/version/budget checks).

Suggested change
if ((item.tool === "edit" || item.tool === "write" || item.tool === "apply_patch") && file) {
const isMutatingFileTool = item.tool === "edit" || item.tool === "write" || item.tool === "apply_patch"
if (isMutatingFileTool && file) {

Copilot uses AI. Check for mistakes.
const seen = list(data.edited_files)
const next = seen.includes(rel(input.worktree, file)) ? seen : [...seen, rel(input.worktree, file)]
const nextEditCount = num(data.edit_count) + 1
Expand All @@ -1032,7 +1053,7 @@ export default async function guardrail(input: {
review_state: "",
})

if (/\.(test|spec)\.(ts|tsx|js|jsx)$|^test_.*\.py$|_test\.go$/.test(rel(input.worktree, file))) {
if (/\.(test|spec)\.(ts|tsx|js|jsx)$|(^|\/)test_.*\.py$|_test\.go$/.test(rel(input.worktree, file))) {
out.output += "\n\n🧪 Test file modified. Verify this test actually FAILS without the fix (test falsifiability)."
}

Expand Down Expand Up @@ -1191,7 +1212,7 @@ export default async function guardrail(input: {
}
}
// Soak time advisory: surface warning set during tool.execute.before
if (item.tool === "bash" && /\b(git\s+merge|gh\s+pr\s+merge)\b/i.test(str(item.args?.command))) {
if (item.tool === "bash" && (/\bgit\s+merge(\s|$)/i.test(str(item.args?.command)) || /\bgh\s+pr\s+merge(\s|$)/i.test(str(item.args?.command)))) {
const freshData = await stash(state)
if (flag(freshData.soak_time_warning)) {
const hours = num(freshData.soak_time_elapsed_h)
Expand Down
Loading
Loading