fix(guardrails): address code-reviewer findings — 3 CRITICAL + 4 HIGH #146
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -486,16 +486,21 @@ | |||||
| } | ||||||
|
|
||||||
| function parseFindings(raw: string) { | ||||||
| // Match structured finding patterns: "[CRITICAL]", "**CRITICAL**", "CRITICAL:", "- CRITICAL" | ||||||
| // Exclude negations, injected context lines, and prose mentions | ||||||
| const lines = raw.split("\n") | ||||||
| let critical = 0, high = 0, medium = 0, low = 0 | ||||||
| for (const line of lines) { | ||||||
| const trimmed = line.trim() | ||||||
| // Skip negation lines | ||||||
| if (/\b(no|zero|0|none|without|aren't|isn't|not)\b/i.test(trimmed)) continue | ||||||
| // Skip injected guardrail context lines (e.g., "CRITICAL=0 HIGH=0") | ||||||
| if (/CRITICAL=\d|HIGH=\d|checklist|Guardrail mode/i.test(trimmed)) continue | ||||||
| // Match structured patterns: leading marker or bracketed/bold severity | ||||||
|
Comment on lines
+494
to
+499
There was a problem hiding this comment. parseFindings() skips any line containing the word "not" (and other negation tokens). This will incorrectly ignore legitimate findings like "- CRITICAL: do not ..." or "HIGH: not safe ...", causing undercounting of findings and weakening review gates. Consider tightening negation detection to only match explicit summary lines (e.g., lines that start with "No/Zero/None" + severity), or run the structured severity match first and only apply negation filtering when the line is a negated summary. |
||||||
| if (/^[\s\-*]*\[?CRITICAL\]?[\s:*]/i.test(trimmed) || /^\*\*CRITICAL\*\*/i.test(trimmed)) critical++ | ||||||
| if (/^[\s\-*]*\[?HIGH\]?[\s:*]/i.test(trimmed) || /^\*\*HIGH\*\*/i.test(trimmed)) high++ | ||||||
| if (/^[\s\-*]*\[?MEDIUM\]?[\s:*]/i.test(trimmed) || /^\*\*MEDIUM\*\*/i.test(trimmed)) medium++ | ||||||
| if (/^[\s\-*]*\[?LOW\]?[\s:*]/i.test(trimmed) || /^\*\*LOW\*\*/i.test(trimmed)) low++ | ||||||
| } | ||||||
| return { critical, high, medium, low, total: critical + high + medium + low } | ||||||
| } | ||||||
|
|
@@ -526,11 +531,18 @@ | |||||
| return | ||||||
| } | ||||||
| const findings = parseFindings(result.text) | ||||||
| const attempts = num(data.workflow_review_attempts) + 1 | ||||||
| if (attempts >= 3) { | ||||||
|
There was a problem hiding this comment. The review-attempt cap looks off by one: attempts is incremented before the check, and the code blocks when attempts >= 3. That allows only 2 completed review cycles even though the workflow prompt says "max 3 cycles". If the intent is to allow up to 3 attempts, block only when attempts > 3 (or >= 4), and still persist the incremented count consistently.
Suggested change
|
||||||
| await mark({ auto_review_in_progress: false, workflow_phase: "blocked", workflow_review_attempts: attempts }) | ||||||
| await seen("auto_review.max_attempts", { attempts }) | ||||||
| return | ||||||
| } | ||||||
| await mark({ | ||||||
| auto_review_in_progress: false, | ||||||
| auto_review_session: made.data.id, | ||||||
| review_state: "done", | ||||||
| reviewed: true, | ||||||
| workflow_review_attempts: attempts, | ||||||
| review_at: new Date().toISOString(), | ||||||
| edits_since_review: 0, | ||||||
| review_critical_count: findings.critical, | ||||||
|
|
@@ -790,21 +802,21 @@ | |||||
| out: { args: Record<string, unknown> }, | ||||||
| ) => { | ||||||
| const file = pick(out.args ?? item.args) | ||||||
| if (file && (item.tool === "read" || MUTATING_TOOLS.has(item.tool))) { | ||||||
| const err = deny(file, item.tool === "read" ? "read" : "edit") | ||||||
| if (err) { | ||||||
| await mark({ last_block: item.tool, last_file: rel(input.worktree, file), last_reason: err }) | ||||||
| throw new Error(text(err)) | ||||||
| } | ||||||
| } | ||||||
| if (MUTATING_TOOLS.has(item.tool)) { | ||||||
| const err = await version(out.args ?? {}) | ||||||
| if (err) { | ||||||
| await mark({ last_block: item.tool, last_file: file ? rel(input.worktree, file) : "", last_reason: err }) | ||||||
| throw new Error(text(err)) | ||||||
| } | ||||||
| } | ||||||
| if ((MUTATING_TOOLS.has(item.tool)) && file && code(file)) { | ||||||
| const count = await budget() | ||||||
| if (count >= 4) { | ||||||
| const budgetData = await stash(state) | ||||||
|
|
@@ -1528,12 +1540,15 @@ | |||||
| } | ||||||
|
|
||||||
| // Workflow phase transitions (auto-pipeline orchestration) | ||||||
| // Only transition when the pipeline is active — prevents state pollution from manual operations | ||||||
| if (item.tool === "bash") { | ||||||
| const cmd = str(item.args?.command) | ||||||
| const output = str(out.output) | ||||||
| const currentPhase = str(bashData.workflow_phase) | ||||||
|
Check failure on line 1547 in packages/guardrails/profile/plugins/guardrail.ts
|
||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||
| const pipelineActive = currentPhase && currentPhase !== "idle" && currentPhase !== "done" && currentPhase !== "blocked" | ||||||
|
|
||||||
| // PR creation detection — only when implementing | ||||||
| if (pipelineActive && currentPhase === "implementing" && /\bgh\s+pr\s+create\b/.test(cmd) && output.includes("github.com")) { | ||||||
| const prUrl = output.trim().match(/https:\/\/github\.com\/[^\s]+/) | ||||||
| if (prUrl) { | ||||||
| await mark({ workflow_phase: "testing", workflow_pr_url: prUrl[0] }) | ||||||
|
|
@@ -1542,8 +1557,8 @@ | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| // Test execution detection — only when testing | ||||||
| if (pipelineActive && currentPhase === "testing" && /\b(bun\s+test|pytest|go\s+test|npm\s+test|vitest|jest)\b/.test(cmd)) { | ||||||
| const testExit = out.metadata?.exitCode ?? (output.includes("FAIL") ? 1 : 0) | ||||||
| if (testExit === 0 && !output.includes("FAIL")) { | ||||||
| await mark({ workflow_phase: "reviewing", tests_executed: true }) | ||||||
|
|
@@ -1552,19 +1567,17 @@ | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| // Merge detection — only when shipping | ||||||
| if (pipelineActive && currentPhase === "shipping" && /\bgh\s+pr\s+merge\b/.test(cmd) && !output.includes("error") && !output.includes("failed")) { | ||||||
| await mark({ workflow_phase: "done" }) | ||||||
| await seen("workflow.merged", {}) | ||||||
| out.output += "\n\n[WORKFLOW] Merge complete. Create follow-up issues for any discovered problems." | ||||||
| } | ||||||
|
|
||||||
| // CI check detection — any active phase | ||||||
| if (pipelineActive && /\bgh\s+pr\s+checks\b/.test(cmd)) { | ||||||
| const allPass = !output.toLowerCase().includes("fail") && !output.toLowerCase().includes("pending") | ||||||
| await mark({ ci_green: allPass }) | ||||||
| } | ||||||
|
|
||||||
| // Issue creation tracking | ||||||
|
|
@@ -1823,10 +1836,12 @@ | |||||
| `[WORKFLOW] Phase: ${phase}. Pipeline: implement→test→review→fix→ship. Complete autonomously. Do not stop at PR creation.` | ||||||
| ) | ||||||
| } | ||||||
| // Follow-up issue guidance — only during active pipeline | ||||||
| if (phase && phase !== "idle" && phase !== "done") { | ||||||
| out.system.push( | ||||||
| "When you discover problems outside the current scope, create follow-up issues: `gh issue create --title '<desc>' --body '<details>' --label 'tech-debt'`. Do NOT fix out-of-scope problems inline." | ||||||
| ) | ||||||
| } | ||||||
|
Comment on lines
+1839
to
+1844
There was a problem hiding this comment. The comment says follow-up issue guidance is shown only during an "active pipeline", but the condition includes the "blocked" phase (it only excludes idle/done). If blocked should not be treated as active, align this condition with the pipelineActive logic (exclude "blocked" too). |
||||||
| }, | ||||||
| } | ||||||
| } | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The deny rules for
gh pr mergeflags are ineffective as written: permission evaluation uses the last matching rule, and"gh pr merge*": allowcomes after the deny entries, overriding them for--admin/--automerges. Also, the patterns"gh pr merge * --admin*"/"... * --auto*"can be bypassed by putting the flag first (e.g.,gh pr merge --admin 123). Move the deny rules after the allow rule (or narrow the allow rule), and use patterns that match the flag in any position (e.g., without requiring an extra arg before the flag).