Phase 0: model-agnostic actor-critic loop engine core#1
Merged
Conversation
Model-agnostic engine that runs the plan-review and per-task actor-critic loops. No vendor/model names in code; backends plug in via a generic AgentRunner abstraction (CliRunner/HttpRunner/MockRunner). - domain/stateMachine: 8-state task lifecycle with a legal-transition table - schemas: plan, critic JSON contract (severity rubric), task artifact bundle - engine/criticParser: JSON extraction + validation; retry-once policy - engine/mechanicalGate: injectable build/test/lint gate (fail-fast) - engine/redaction: secret/PII redaction before anything leaves the engine - engine/loop: per-task loop + plan-review loop with cycle caps, quota fallback (UNVERIFIED_BY_CRITIC), and rubric enforcement (nits never loop) - adapters: vendor-neutral Actor/Critic roles + scriptable mocks - demo + 34 passing tests (vitest), clean tsc
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughPR initializes a TypeScript Node project and implements Loopwright end to end: schemas/contracts, task state machine, redaction and mechanical gate utilities, critic parsing, task and plan orchestration loops, deterministic mocks, runnable demo flow, and comprehensive Vitest suites. ChangesLoopwright actor-critic engine
Sequence Diagram(s)sequenceDiagram
participant U as DemoCLI
participant PR as runPlanReview
participant AG as MockActor
participant CR as MockCritic
participant TL as runTask
participant MG as runMechanicalGate
U->>PR: submit goal
PR->>AG: draftPlan(goal, feedback)
AG-->>PR: plan draft
PR->>CR: reviewPlan(plan)
CR-->>PR: verdict
alt verdict is changes_required
PR->>AG: draftPlan(goal, blockers)
AG-->>PR: revised plan
PR->>CR: reviewPlan(revised plan)
CR-->>PR: green verdict
end
U->>TL: run each approved task
TL->>AG: build(task, feedback)
AG-->>TL: diff and touched files
TL->>MG: execute verify commands
MG-->>TL: gate result
TL->>CR: reviewTask(artifact bundle)
CR-->>TL: green or changes_required
TL-->>U: task outcome
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/config.ts`:
- Around line 41-47: The env boolean parsing is wrong because z.coerce.boolean()
treats non-empty strings like "false"/"0" as true; update the mechanicalGate and
useWorktrees schemas to use a z.preprocess that converts common string/number
representations to real booleans before validating (e.g., accept boolean,
number, or string and map "false","0","no","off" -> false and
"true","1","yes","on" -> true), then pass the normalized boolean to
z.boolean().default(true) (and .default(true) for mechanicalGate, .default(true)
for useWorktrees) so env strings are interpreted correctly.
In `@src/engine/loop.ts`:
- Around line 96-100: The fallback "unavailable" self-review currently takes all
findings (notes = normalizeReview(parsed.review).review.findings) and later
appends them to accumulatedNits, which can leak blocker-severity findings into
TaskOutcome.nits; change the logic that builds selfReviewNotes and the code that
appends to accumulatedNits to filter out blocker/severity== "blocker" (or
equivalent highest-severity enum) so only non-blocker findings are included in
the fallback path—update the assignment that creates notes and the place where
accumulatedNits is extended (references: normalizeReview(...).review.findings,
notes, accumulatedNits, and the "unavailable" return branch) to apply the
filter.
In `@src/engine/mechanicalGate.ts`:
- Around line 29-53: The defaultExecutor currently can hang and accumulates
unbounded output; modify the CommandExecutor implementation (defaultExecutor) to
enforce a hard timeout (e.g., configurable constant, default ~2–5 minutes) that
kills the child process with child.kill() and resolves with a distinct exitCode
(e.g., 124) and duration, and implement bounded output capture by keeping a
rolling buffer (or appending only until a max size) and periodically applying
redactAndTruncate(output, 8_000) or truncating on append so memory can't grow
unbounded; also ensure the timeout timer is cleared on normal child close/error
and that the resolved output uses the redacted/truncated representation.
In `@src/engine/redaction.ts`:
- Around line 29-31: The "bearer" redaction rule currently matches only exact
"Bearer" casing; update the rule named "bearer" so its pattern is
case-insensitive (add the /i flag) and still captures the token portion (e.g.,
/\bBearer\s+[A-Za-z0-9._-]{8,}/i) and keep the replace using the REDACTED
constant; also update the "home-path" redaction rule to include Windows-style
paths by adding a pattern that matches drive-letter user folders (e.g., patterns
that match C:\Users\<username>\... with proper escaping) and ensure its replace
uses the same REDACTED placeholder so Windows paths like C:\Users\Alice\... are
redacted as well.
In `@src/schemas/plan.ts`:
- Around line 5-7: The schema currently allows verifyCommands to default to an
empty array which, combined with runMechanicalGate treating commands.length ===
0 as success, permits vacuous mechanical passes; change TaskSpecSchema's
verifyCommands definition to require at least one entry (e.g., add "minItems": 1
and/or remove the default empty array) so the validator will reject specs
without verifications, and update runMechanicalGate (the function that checks
commands.length) to no longer treat an empty list as a pass (either treat it as
a failure/error or explicitly require a separate manual-verification mode),
ensuring TaskSpecSchema and runMechanicalGate (and any code that sets a default
verifyCommands) remain consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dc0dbe3c-f7ad-420c-ad8f-4a719bbbeb15
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
.gitignorepackage.jsonsrc/adapters/agents.tssrc/adapters/mocks.tssrc/config.tssrc/demo.tssrc/domain/stateMachine.tssrc/engine/criticParser.tssrc/engine/loop.tssrc/engine/mechanicalGate.tssrc/engine/redaction.tssrc/runners/agentRunner.tssrc/schemas/artifact.tssrc/schemas/critic.tssrc/schemas/plan.tstest/critic.test.tstest/loop.test.tstest/mechanicalGate.test.tstest/redaction.test.tstest/stateMachine.test.tstsconfig.json
Comment on lines
+41
to
+47
| mechanicalGate: z.coerce.boolean().default(true), | ||
| criticFallback: CriticFallbackSchema.default("actor_self_review"), | ||
|
|
||
| // execution | ||
| maxParallel: z.coerce.number().int().min(1).default(2), | ||
| stuckThresholdMs: z.coerce.number().int().min(1000).default(120_000), | ||
| useWorktrees: z.coerce.boolean().default(true), |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Zod 3.23.8, how does z.coerce.boolean() handle string inputs like "false", "0", and "true"?
💡 Result:
In Zod 3.23.8, z.coerce.boolean handles string inputs by passing them directly to the native JavaScript Boolean constructor [1][2]. Consequently, its behavior is determined entirely by JavaScript's truthiness rules [2][3]. For the specific string inputs requested: - "true" is coerced to true [1][2]. - "false" is coerced to true (because it is a non-empty string, and Boolean("false") evaluates to true) [1][2][4]. - "0" is coerced to true (because it is a non-empty string) [1][2]. Because z.coerce.boolean follows standard JavaScript type coercion, any non-empty string—regardless of its content—will be coerced to true [1][2][3]. Only an empty string ("") is coerced to false [1][2]. If you need to parse strings like "false" or "0" as the boolean value false, z.coerce.boolean is not the appropriate tool [2][4]. The Zod documentation recommends using z.stringbool for this purpose, which is designed to handle "boolish" string values logically [5]. Note that while z.stringbool is available in newer versions (Zod 4+), for version 3.23.8, you should use z.preprocess or z.transform to implement custom logic that handles specific string values like "false" or "0" correctly [1][6][7].
Citations:
- 1: https://npmx.dev/package/zod/v/3.23.8
- 2: https://mintlify.com/colinhacks/zod/api/utilities/coerce
- 3: coerce.boolean incorrect colinhacks/zod#3329
- 4: Calling
zod.coerce.boolean()on the string value"false"returnstruebecauseBoolean("false")evaluates totruecolinhacks/zod#5501 - 5: https://zod.dev/api
- 6: Add
z.string().boolean()function to parse a boolean value represented by string logically colinhacks/zod#2989 - 7: Parse a boolean value represented by string logically colinhacks/zod#2985
Fix env boolean parsing: z.coerce.boolean() treats "false"/"0" as true in Zod 3.23.8
In Zod 3.23.8, z.coerce.boolean() coerces string inputs using JavaScript Boolean(...), so "false" and "0" are non-empty and become true—meaning mechanicalGate / useWorktrees may never disable when configured via env strings.
Suggested fix
+const EnvBoolean = z.preprocess((v) => {
+ if (typeof v !== "string") return v;
+ const n = v.trim().toLowerCase();
+ if (["1", "true", "yes", "on"].includes(n)) return true;
+ if (["0", "false", "no", "off"].includes(n)) return false;
+ return v;
+}, z.boolean());
const RawConfigSchema = z.object({
@@
- mechanicalGate: z.coerce.boolean().default(true),
+ mechanicalGate: EnvBoolean.default(true),
@@
- useWorktrees: z.coerce.boolean().default(true),
+ useWorktrees: EnvBoolean.default(true),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| mechanicalGate: z.coerce.boolean().default(true), | |
| criticFallback: CriticFallbackSchema.default("actor_self_review"), | |
| // execution | |
| maxParallel: z.coerce.number().int().min(1).default(2), | |
| stuckThresholdMs: z.coerce.number().int().min(1000).default(120_000), | |
| useWorktrees: z.coerce.boolean().default(true), | |
| const EnvBoolean = z.preprocess((v) => { | |
| if (typeof v !== "string") return v; | |
| const n = v.trim().toLowerCase(); | |
| if (["1", "true", "yes", "on"].includes(n)) return true; | |
| if (["0", "false", "no", "off"].includes(n)) return false; | |
| return v; | |
| }, z.boolean()); | |
| const RawConfigSchema = z.object({ | |
| mechanicalGate: EnvBoolean.default(true), | |
| criticFallback: CriticFallbackSchema.default("actor_self_review"), | |
| // execution | |
| maxParallel: z.coerce.number().int().min(1).default(2), | |
| stuckThresholdMs: z.coerce.number().int().min(1000).default(120_000), | |
| useWorktrees: EnvBoolean.default(true), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/config.ts` around lines 41 - 47, The env boolean parsing is wrong because
z.coerce.boolean() treats non-empty strings like "false"/"0" as true; update the
mechanicalGate and useWorktrees schemas to use a z.preprocess that converts
common string/number representations to real booleans before validating (e.g.,
accept boolean, number, or string and map "false","0","no","off" -> false and
"true","1","yes","on" -> true), then pass the normalized boolean to
z.boolean().default(true) (and .default(true) for mechanicalGate, .default(true)
for useWorktrees) so env strings are interpreted correctly.
Comment on lines
+16
to
+40
| function extractJsonObject(text: string): string | null { | ||
| const start = text.indexOf("{"); | ||
| if (start === -1) return null; | ||
|
|
||
| let depth = 0; | ||
| let inString = false; | ||
| let escaped = false; | ||
|
|
||
| for (let i = start; i < text.length; i++) { | ||
| const ch = text[i]; | ||
| if (inString) { | ||
| if (escaped) escaped = false; | ||
| else if (ch === "\\") escaped = true; | ||
| else if (ch === '"') inString = false; | ||
| continue; | ||
| } | ||
| if (ch === '"') inString = true; | ||
| else if (ch === "{") depth++; | ||
| else if (ch === "}") { | ||
| depth--; | ||
| if (depth === 0) return text.slice(start, i + 1); | ||
| } | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Parser can reject valid responses when a non-JSON {...} appears earlier in text
Line 17 anchors to the first { and Line 42–56 only parse that single extracted object. If the model includes an earlier brace snippet in prose, valid review JSON later in the response is ignored and the task can be incorrectly marked malformed.
Also applies to: 42-56
Comment on lines
+96
to
+100
| const notes = parsed.ok ? normalizeReview(parsed.review).review.findings : []; | ||
| return { | ||
| kind: "unavailable", | ||
| selfReviewNotes: notes, | ||
| reason: "Critic quota exhausted; result self-reviewed by actor (UNVERIFIED).", |
There was a problem hiding this comment.
Fallback self-review can leak blocker-severity findings into TaskOutcome.nits
Line 96 captures all normalized findings (including blockers), and Line 263 appends them to accumulatedNits. In fallback mode this can return nits containing blocker-severity entries.
Suggested fix
- const notes = parsed.ok ? normalizeReview(parsed.review).review.findings : [];
+ const notes = parsed.ok ? nitsOf(normalizeReview(parsed.review).review) : [];Also applies to: 262-264
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/engine/loop.ts` around lines 96 - 100, The fallback "unavailable"
self-review currently takes all findings (notes =
normalizeReview(parsed.review).review.findings) and later appends them to
accumulatedNits, which can leak blocker-severity findings into TaskOutcome.nits;
change the logic that builds selfReviewNotes and the code that appends to
accumulatedNits to filter out blocker/severity== "blocker" (or equivalent
highest-severity enum) so only non-blocker findings are included in the fallback
path—update the assignment that creates notes and the place where
accumulatedNits is extended (references: normalizeReview(...).review.findings,
notes, accumulatedNits, and the "unavailable" return branch) to apply the
filter.
Comment on lines
+29
to
+53
| export const defaultExecutor: CommandExecutor = (command, cwd) => | ||
| new Promise((resolve) => { | ||
| const started = Date.now(); | ||
| const child = spawn(command, { cwd, shell: true }); | ||
| let output = ""; | ||
| const append = (buf: Buffer) => { | ||
| output += buf.toString(); | ||
| }; | ||
| child.stdout.on("data", append); | ||
| child.stderr.on("data", append); | ||
| child.on("error", (err) => { | ||
| resolve({ | ||
| exitCode: 127, | ||
| output: `${output}\n${err.message}`, | ||
| durationMs: Date.now() - started, | ||
| }); | ||
| }); | ||
| child.on("close", (code) => { | ||
| resolve({ | ||
| exitCode: code ?? 1, | ||
| output, | ||
| durationMs: Date.now() - started, | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify absence/presence of timeout and bounded capture safeguards.
rg -n 'spawn\(|setTimeout|child\.kill|output \+=' src/engine/mechanicalGate.tsRepository: inhaq/loopwright
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect mechanicalGate.ts around defaultExecutor
sed -n '1,140p' src/engine/mechanicalGate.ts
# 2) Find all references to defaultExecutor
rg -n "defaultExecutor" src
# 3) Locate CommandExecutor type/contract and any timeout-related options
rg -n "type\s+CommandExecutor|interface\s+CommandExecutor|CommandExecutor" src
rg -n "timeout|MAX_CAPTURE|CAPTURED_CHARS|SIGKILL|kill\(" src/engine srcRepository: inhaq/loopwright
Length of output: 3462
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect redaction/truncation policy
rg -n "function redactAndTruncate|export function redactAndTruncate|redactAndTruncate" -S src/engine
sed -n '1,200p' src/engine/redaction.ts
# Check loop for any overall timeout/cancellation
sed -n '1,220p' src/engine/loop.ts
rg -n "timeout|AbortController|AbortSignal|kill\\(|SIGKILL|SIGTERM|setTimeout" src/engine src/adapters src
# Inspect mock executor behavior
sed -n '1,140p' src/adapters/mocks.tsRepository: inhaq/loopwright
Length of output: 10338
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Global search for any command timeout/cancellation mechanisms
rg -n --hidden --no-ignore-vcs "AbortController|AbortSignal|setTimeout|clearTimeout|SIGKILL|SIGTERM|child\.kill|kill\(" src
# Search for any custom CommandExecutor implementations besides defaultExecutor
rg -n "defaultExecutor|CommandExecutor\s*=|function .*Executor|executor\:" src/adapters src/engine
# See mock executor behavior (may already bound output/timeouts)
sed -n '1,220p' src/adapters/mocks.tsRepository: inhaq/loopwright
Length of output: 42
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the mocks executor implementation
sed -n '1,220p' src/adapters/mocks.ts
# Inspect config for any mechanicalGate/verify command timeout knobs
ls -la src | head
fd -t f "config.*\.(ts|js|mjs|cjs)" src || true
rg -n "mechanicalGate|verifyCommands|executor|timeout|max.*(output|chars)|capture" src/config src -S || true
# Targeted searches for cancellation/kill patterns (no whole-repo dumps)
rg -n "AbortController|AbortSignal|setTimeout|clearTimeout" src || true
rg -n "SIGKILL|SIGTERM|child\.kill|kill\(" src || trueRepository: inhaq/loopwright
Length of output: 8162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/config.ts
nl -ba src/engine/mechanicalGate.ts | sed -n '1,180p'
# Look specifically for timeout/cancellation anywhere relevant
rg -n "timeout|AbortController|AbortSignal|abort\(|signal|kill\\(|SIGKILL|SIGTERM|setTimeout|clearTimeout" src -S
# Also check if any other executor bounds output during capture
rg -n "CommandOutcome|output \+=|toString\\(\\)|redactAndTruncate\\(" src/engine -SRepository: inhaq/loopwright
Length of output: 2697
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print with line numbers using awk (nl isn't available in this sandbox)
awk '{print NR ":" $0}' src/engine/mechanicalGate.ts | sed -n '1,220p'
awk '{print NR ":" $0}' src/engine/loop.ts | sed -n '1,260p'
# Look for stuck threshold usage + any cancellation mechanics
rg -n "stuckThresholdMs" src -S
rg -n "AbortController|AbortSignal|abort\\(|signal|kill\\(|SIGKILL|SIGTERM|clearTimeout|setTimeout" src -SRepository: inhaq/loopwright
Length of output: 12926
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "stuckThresholdMs" src -S
sed -n '1,120p' src/runners/agentRunner.ts
sed -n '120,260p' src/runners/agentRunner.tsRepository: inhaq/loopwright
Length of output: 2143
🏁 Script executed:
#!/bin/bash
# Ensure timeout + bounded capture were added to the default executor
rg -n "defaultExecutor|TIMEOUT_MS|MAX_CAPTURED_CHARS|setTimeout\\(|child\\.kill\\(|output = output\\.slice" src/engine/mechanicalGate.tsRepository: inhaq/loopwright
Length of output: 185
Add a hard command timeout and bounded output capture in defaultExecutor.
A hung verify command blocks runMechanicalGate indefinitely, and output += buf.toString() accumulates unbounded stdout/stderr in memory; truncation/redaction (redactAndTruncate(..., maxChars=8_000)) happens only after the process exits, so memory can spike before that.
🛠️ Suggested guardrails
export const defaultExecutor: CommandExecutor = (command, cwd) =>
new Promise((resolve) => {
+ const TIMEOUT_MS = 5 * 60_000;
+ const MAX_CAPTURED_CHARS = 200_000;
const started = Date.now();
const child = spawn(command, { cwd, shell: true });
let output = "";
const append = (buf: Buffer) => {
- output += buf.toString();
+ output += buf.toString();
+ if (output.length > MAX_CAPTURED_CHARS) {
+ output = output.slice(-MAX_CAPTURED_CHARS);
+ }
};
+ const timer = setTimeout(() => {
+ child.kill("SIGKILL");
+ }, TIMEOUT_MS);
child.stdout.on("data", append);
child.stderr.on("data", append);
child.on("error", (err) => {
+ clearTimeout(timer);
resolve({
exitCode: 127,
output: `${output}\n${err.message}`,
durationMs: Date.now() - started,
});
});
child.on("close", (code) => {
+ clearTimeout(timer);
resolve({
exitCode: code ?? 1,
output,
durationMs: Date.now() - started,
});
});
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/engine/mechanicalGate.ts` around lines 29 - 53, The defaultExecutor
currently can hang and accumulates unbounded output; modify the CommandExecutor
implementation (defaultExecutor) to enforce a hard timeout (e.g., configurable
constant, default ~2–5 minutes) that kills the child process with child.kill()
and resolves with a distinct exitCode (e.g., 124) and duration, and implement
bounded output capture by keeping a rolling buffer (or appending only until a
max size) and periodically applying redactAndTruncate(output, 8_000) or
truncating on append so memory can't grow unbounded; also ensure the timeout
timer is cleared on normal child close/error and that the resolved output uses
the redacted/truncated representation.
Comment on lines
+29
to
+31
| name: "bearer", | ||
| pattern: /\bBearer\s+[A-Za-z0-9._-]{8,}/g, | ||
| replace: () => `Bearer ${REDACTED}`, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current patterns for case-sensitivity and OS-path coverage.
rg -n 'name:\s*"bearer"|pattern:\s*/\\bBearer\\s\+\[A-Za-z0-9._-\]\{8,\}/g' src/engine/redaction.ts
rg -n 'name:\s*"home-path"|Users\|home|windows-home-path' src/engine/redaction.tsRepository: inhaq/loopwright
Length of output: 206
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect surrounding code to ensure the suggested new/modified rules match existing rule shapes (especially `replace` callback signatures).
sed -n '1,120p' src/engine/redaction.tsRepository: inhaq/loopwright
Length of output: 2773
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'pattern:\s*/\\bBearer\\b|name:\s*"bearer"|windows-home-path|home-path' -S .Repository: inhaq/loopwright
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Windows-like home paths / username components
rg -n '(?:[A-Za-z]:\\Users\\|\\Users\\)' -S .
# Any other bearer-token redaction (case variants)
rg -n '\\bBearer\\b|\\bbearer\\b' -S .Repository: inhaq/loopwright
Length of output: 42
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Where "Bearer" appears anywhere in source
rg -n 'Bearer' -S src
# Where "bearer" appears anywhere in source (lowercase)
rg -n 'bearer' -S src
# Windows-style user paths (literal backslashes) in source
rg -n '\\\\Users\\\\' -S src
rg -n '[A-Za-z]:\\\\Users\\\\' -S srcRepository: inhaq/loopwright
Length of output: 715
Harden redaction rules for auth-scheme casing and Windows home paths
src/engine/redaction.tsbearer redaction matches only exactBearercasing (no/i), sobearer/BEARERvariants can leak.home-pathonly redacts/Users/...and/home/...(Unix-like); there’s no Windows equivalent forC:\Users\....
🔒 Suggested hardening
{
name: "bearer",
- pattern: /\bBearer\s+[A-Za-z0-9._-]{8,}/g,
+ pattern: /\bBearer\s+[A-Za-z0-9._-]{8,}/gi,
replace: () => `Bearer ${REDACTED}`,
},
@@
{
name: "home-path",
pattern: /\/(Users|home)\/[^/\s:"']+/g,
replace: (_m, root: string) => `/${root}/[user]`,
},
+ {
+ name: "windows-home-path",
+ pattern: /[A-Za-z]:\\Users\\[^\\\s:"']+/g,
+ replace: (m: string) => m.replace(/\\Users\\[^\\\s:"']+/, "\\Users\\[user]"),
+ },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/engine/redaction.ts` around lines 29 - 31, The "bearer" redaction rule
currently matches only exact "Bearer" casing; update the rule named "bearer" so
its pattern is case-insensitive (add the /i flag) and still captures the token
portion (e.g., /\bBearer\s+[A-Za-z0-9._-]{8,}/i) and keep the replace using the
REDACTED constant; also update the "home-path" redaction rule to include
Windows-style paths by adding a pattern that matches drive-letter user folders
(e.g., patterns that match C:\Users\<username>\... with proper escaping) and
ensure its replace uses the same REDACTED placeholder so Windows paths like
C:\Users\Alice\... are redacted as well.
Comment on lines
+5
to
+7
| * `verifyCommands` is the machine-checkable definition-of-done for the | ||
| * mechanical gate; the task is never GREEN without these passing. | ||
| */ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current bypass path across schema + gate behavior.
rg -n 'verifyCommands: .*default\(\[\]\)' src/schemas/plan.ts
rg -n 'commands.length === 0' src/engine/mechanicalGate.tsRepository: inhaq/loopwright
Length of output: 155
Prevent vacuous mechanical passes when verifyCommands is empty
TaskSpecSchema defaults verifyCommands to [] (src/schemas/plan.ts:13), and runMechanicalGate treats commands.length === 0 as a pass (src/engine/mechanicalGate.ts:73), so tasks can become GREEN with no executable verification. Tighten the schema to require at least one command (or introduce an explicit manual-verification mode).
💡 Suggested schema tightening
export const TaskSpecSchema = z.object({
id: z.string().min(1),
title: z.string().min(1),
description: z.string().default(""),
acceptanceCriteria: z.array(z.string().min(1)).min(1),
- verifyCommands: z.array(z.string().min(1)).default([]),
+ verifyCommands: z.array(z.string().min(1)).min(1),
dependencies: z.array(z.string()).default([]),
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/schemas/plan.ts` around lines 5 - 7, The schema currently allows
verifyCommands to default to an empty array which, combined with
runMechanicalGate treating commands.length === 0 as success, permits vacuous
mechanical passes; change TaskSpecSchema's verifyCommands definition to require
at least one entry (e.g., add "minItems": 1 and/or remove the default empty
array) so the validator will reject specs without verifications, and update
runMechanicalGate (the function that checks commands.length) to no longer treat
an empty list as a pass (either treat it as a failure/error or explicitly
require a separate manual-verification mode), ensuring TaskSpecSchema and
runMechanicalGate (and any code that sets a default verifyCommands) remain
consistent.
- config: env booleans via explicit preprocess (z.coerce.boolean treated "false"/"0" as true, so toggles couldn't be disabled) - criticParser: scan all balanced JSON candidates and prefer the last valid one, so an earlier non-JSON brace in prose no longer fails the parse - mechanicalGate: createShellExecutor with a hard timeout (SIGKILL, exit 124) and bounded output capture to prevent hangs/unbounded memory - redaction: case-insensitive bearer matching + Windows home-path rule - loop: fallback self-review carries only non-blocker findings into nits Skipped: forcing verifyCommands>=1 at the schema level (missing DoD is caught by the critic during plan review; some tasks legitimately have none). Adds 10 tests (44 total); typecheck clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request was created by @kiro-agent on behalf of @inhaq 👻
Comment with /kiro fix to address specific feedback or /kiro all to address everything.
Learn about Kiro Web
Summary
First implementation slice: the headless engine core that runs the plan-review and per-task actor-critic loops. No GUI, no parallel DAG, no real agent backends yet (those come next) — this is the loop logic, fully tested with mocks.
The engine is model-agnostic: there are no vendor or model names in the code. Backends plug in through a generic
AgentRunnerabstraction (CliRunner/HttpRunner/MockRunner), and roles (Actor/Critic) are bound to runners via config.What's included
domain/stateMachine— 8-state task lifecycle with a legal-transition table.schemas/— plan/task specs, the critic JSON contract with the blocker/nit severity rubric, and the task artifact bundle.engine/criticParser— extracts + validates critic JSON; retry-once-then-NEEDS_HUMANon malformed output.engine/mechanicalGate— injectable build/test/lint gate (fail-fast) that runs before the scarce critic is ever called.engine/redaction— secret/PII redaction applied before anything leaves the engine.engine/loop— per-task loop + plan-review loop: cycle caps, quota fallback (UNVERIFIED_BY_CRITIC, never mistaken for a real green), and rubric enforcement so nits never trigger another cycle.adapters/— vendor-neutralActor/Criticroles + scriptable mocks.demo.ts— fake end-to-end run.Testing
npm run typecheck— clean.npm test— 34 tests pass.npm run demo— exercises plan block→revise→approve, per-task fail→fix→block→fix→green, nit-without-loop, and quota fallback.Not in scope (next phases)
Real CLI/HTTP runners, SQLite persistence + checkpoints, parallel DAG scheduler + git worktrees, stuck-detection watchdog, Electron shell.
Summary by CodeRabbit
Release Notes
New Features
Tests