Repository navigation
feat(afk): judge patch_apply edits in the rule hook - #36
Merged
Merged
Conversation
agent-afk's patch_apply tool writes several files atomically, and the AFK
rule hook only matched edit_file and write_file, so those edits landed
unjudged and never reached the Stop sweep.
agent-afk hands a plugin command hook tool_name "patch_apply" with the raw
{ changes, dry_run } input (src/agent/hooks/command-executor.ts
buildStdinPayload), and tests a /.../ matcher against that name alone; the
MultiEdit alias in src/agent/hooks/matcher.ts applies only to bare-name
matchers. The matcher now lists patch_apply.
Each file in the patch is judged in its own Jev call, in parallel, against
the rules scoped to that file, under one 12 s budget inside the hook's
15 s timeout, and each check writes its own log row. One file at >= 0.80
blocks the whole call and nothing is recorded for the Stop sweep; a clean
or flagged patch records every file. A dry_run call is not judged. The
input parsing moves to adapters/afk/src/shared/edit-targets.ts.
No version bump; left to the maintainer.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="adapters/afk/src/shared/edit-targets.ts">
<violation number="1" location="adapters/afk/src/shared/edit-targets.ts:51">
P3: The `edits` loop in `editHunks` dereferences `e.old_string`/`e.new_string` before confirming `e` is an object, so one malformed element (e.g. `null`) throws inside `editTargets`. The top-level `try/catch` in `rules.ts` then swallows the event silently, so that edit lands unjudged and is never recorded for the Stop sweep — while `changeHunk` in the same parser guards elements with `isJsonObject(e)`. Apply the same guard here so the shared parser consistently skips malformed inputs as its contract states.</violation>
<violation number="2" location="adapters/afk/src/shared/edit-targets.ts:73">
P2: `changeHunk` accepts non-string runtime `old`/`new` values and turns them into hunks, so malformed patch inputs can be judged, logged, or blocked instead of being skipped. Validate both fields as strings before calling `pairHunk`.</violation>
</file>
<file name="adapters/afk/src/rules.ts">
<violation number="1" location="adapters/afk/src/rules.ts:195">
P3: Budget exhaustion is logged as kind `rules-error`, the same row type used for actual Jev API failures (`stats.py` and the README describe it as "Jev call failed"). Files skipped here were never attempted and still land unjudged via the fail-open path, so the row misrepresents a scheduled skip as an API failure. Drop the row (it still needs no block output) or use a dedicated skip reason.</violation>
<violation number="2" location="adapters/afk/src/rules.ts:252">
P2: A rejected per-file assessment is silently treated as if it did not exist, so a malformed Jev answer can let that patch file through without a `rules-error` row or a judgment. Log the rejected target as a check error before continuing fail-open.</violation>
</file>
<file name="adapters/afk/README.md">
<violation number="1" location="adapters/afk/README.md:14">
P2: This row overstates the patch guarantee: after the same rule/file reaches the two-block session limit, a third >= 0.80 hit does not block the atomic `patch_apply` call. Qualify the statement with that existing per-rule/file block-limit exception.</violation>
</file>
<file name="adapters/afk/test/patch-apply.test.ts">
<violation number="1" location="adapters/afk/test/patch-apply.test.ts:78">
P2: The mock answers every judgment question with noul 0.97 or 0.02, so no answer can ever land in the 0.50–0.80 flag band. The patch flag path — `hookSpecificOutput.additionalContext` plus recording all affected files for the Stop sweep (explicitly claimed in the PR description) — is never exercised by these tests. Make the mock answer configurable in the flag band (e.g. a per-state noul value) and add a test asserting a flagged patch writes `additionalContext` and still records every file.</violation>
<violation number="2" location="adapters/afk/test/patch-apply.test.ts:291">
P3: `maxInFlight > 1` only proves two judgments overlapped; it never verifies the eight-way batching (`MAX_PARALLEL` waves) that the patch judge is meant to perform, and with only three files the assertion would pass even if a max-parallel limit were removed entirely. Feed more than eight files in the clean patch and assert bounded concurrency (e.g. `1 < maxInFlight && maxInFlight <= 8`).</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
Comment on lines
+252
to
254
| for (const r of settled) { | ||
| if (r.status === "fulfilled" && r.value !== null) out.push(r.value); | ||
| } |
There was a problem hiding this comment.
P2: A rejected per-file assessment is silently treated as if it did not exist, so a malformed Jev answer can let that patch file through without a rules-error row or a judgment. Log the rejected target as a check error before continuing fail-open.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At adapters/afk/src/rules.ts, line 252:
<comment>A rejected per-file assessment is silently treated as if it did not exist, so a malformed Jev answer can let that patch file through without a `rules-error` row or a judgment. Log the rejected target as a check error before continuing fail-open.</comment>
<file context>
@@ -242,84 +231,187 @@ async function judge(
- if (h.band === "act" && (state.blocks[blockKey] ?? 0) < MAX_BLOCKS) {
- state.blocks[blockKey] = (state.blocks[blockKey] ?? 0) + 1;
- acting.push(h);
+ for (const r of settled) {
+ if (r.status === "fulfilled" && r.value !== null) out.push(r.value);
}
</file context>
Suggested change
| for (const r of settled) { | |
| if (r.status === "fulfilled" && r.value !== null) out.push(r.value); | |
| } | |
| for (const [i, r] of settled.entries()) { | |
| if (r.status === "fulfilled" && r.value !== null) { | |
| out.push(r.value); | |
| } else if (r.status === "rejected") { | |
| logCheckError(ctxFor(wave[i]!), String(r.reason), 0); | |
| } | |
| } |
| | `prompt-router.ts` | `UserPromptSubmit` | Classifies each prompt as chat / lookup / fix / feature / ops and injects a routing hint when confidence >= 0.75. Interactive REPL only. | | ||
| | `subagent-router.ts` | `PreToolUse` (`agent`) | Recommends a model tier when the spawn names no `agent_type`, and flags a brief that changes files but leaves out paths, acceptance criteria, verification, or commit policy. Advisory only, and AFK does not deliver it yet (see Known gaps). | | ||
| | `rules.ts` | `PreToolUse` (`edit_file`, `write_file`) | Loads rules from instruction files, classifies them, and judges each edit before it is written. **Blocks at >= 0.80**, so the edit never lands (agent cannot override). Flags 0.50-0.80 as advisory context, which AFK does not deliver yet. | | ||
| | `rules.ts` | `PreToolUse` (`edit_file`, `write_file`, `patch_apply`) | Loads rules from instruction files, classifies them, and judges each edit before it is written. **Blocks at >= 0.80**, so the edit never lands (agent cannot override). A `patch_apply` call is judged file by file, and one file at >= 0.80 blocks the whole call, so none of its files land; a `dry_run` call is not judged. Flags 0.50-0.80 as advisory context, which AFK does not deliver yet. | |
There was a problem hiding this comment.
P2: This row overstates the patch guarantee: after the same rule/file reaches the two-block session limit, a third >= 0.80 hit does not block the atomic patch_apply call. Qualify the statement with that existing per-rule/file block-limit exception.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At adapters/afk/README.md, line 14:
<comment>This row overstates the patch guarantee: after the same rule/file reaches the two-block session limit, a third >= 0.80 hit does not block the atomic `patch_apply` call. Qualify the statement with that existing per-rule/file block-limit exception.</comment>
<file context>
@@ -11,7 +11,7 @@ Jev judgment hooks for [agent-afk](https://github.com/griffinwork40/agent-afk).
| `prompt-router.ts` | `UserPromptSubmit` | Classifies each prompt as chat / lookup / fix / feature / ops and injects a routing hint when confidence >= 0.75. Interactive REPL only. |
| `subagent-router.ts` | `PreToolUse` (`agent`) | Recommends a model tier when the spawn names no `agent_type`, and flags a brief that changes files but leaves out paths, acceptance criteria, verification, or commit policy. Advisory only, and AFK does not deliver it yet (see Known gaps). |
-| `rules.ts` | `PreToolUse` (`edit_file`, `write_file`) | Loads rules from instruction files, classifies them, and judges each edit before it is written. **Blocks at >= 0.80**, so the edit never lands (agent cannot override). Flags 0.50-0.80 as advisory context, which AFK does not deliver yet. |
+| `rules.ts` | `PreToolUse` (`edit_file`, `write_file`, `patch_apply`) | Loads rules from instruction files, classifies them, and judges each edit before it is written. **Blocks at >= 0.80**, so the edit never lands (agent cannot override). A `patch_apply` call is judged file by file, and one file at >= 0.80 blocks the whole call, so none of its files land; a `dry_run` call is not judged. Flags 0.50-0.80 as advisory context, which AFK does not deliver yet. |
| `stop-sweep.ts` | `Stop` | Judges the edits made since its last completed judgment against turn-scope rules (scope creep, cross-file patterns), and hands what it finds to the next turn. Interactive REPL only. |
</file context>
| for (const e of change.edits) { | ||
| if (!isJsonObject(e)) continue; | ||
|
|
||
| const hunk = pairHunk(e.old, e.new); |
There was a problem hiding this comment.
P2: changeHunk accepts non-string runtime old/new values and turns them into hunks, so malformed patch inputs can be judged, logged, or blocked instead of being skipped. Validate both fields as strings before calling pairHunk.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At adapters/afk/src/shared/edit-targets.ts, line 73:
<comment>`changeHunk` accepts non-string runtime `old`/`new` values and turns them into hunks, so malformed patch inputs can be judged, logged, or blocked instead of being skipped. Validate both fields as strings before calling `pairHunk`.</comment>
<file context>
@@ -0,0 +1,133 @@
+ for (const e of change.edits) {
+ if (!isJsonObject(e)) continue;
+
+ const hunk = pairHunk(e.old, e.new);
+
+ if (hunk) parts.push(hunk);
</file context>
| ruleStates.push(state); | ||
|
|
||
| for (const key of keys) { | ||
| answers[key] = { noul: state.includes("console.log") ? 0.97 : 0.02 }; |
There was a problem hiding this comment.
P2: The mock answers every judgment question with noul 0.97 or 0.02, so no answer can ever land in the 0.50–0.80 flag band. The patch flag path — hookSpecificOutput.additionalContext plus recording all affected files for the Stop sweep (explicitly claimed in the PR description) — is never exercised by these tests. Make the mock answer configurable in the flag band (e.g. a per-state noul value) and add a test asserting a flagged patch writes additionalContext and still records every file.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At adapters/afk/test/patch-apply.test.ts, line 78:
<comment>The mock answers every judgment question with noul 0.97 or 0.02, so no answer can ever land in the 0.50–0.80 flag band. The patch flag path — `hookSpecificOutput.additionalContext` plus recording all affected files for the Stop sweep (explicitly claimed in the PR description) — is never exercised by these tests. Make the mock answer configurable in the flag band (e.g. a per-state noul value) and add a test asserting a flagged patch writes `additionalContext` and still records every file.</comment>
<file context>
@@ -0,0 +1,411 @@
+ ruleStates.push(state);
+
+ for (const key of keys) {
+ answers[key] = { noul: state.includes("console.log") ? 0.97 : 0.02 };
+ }
+
</file context>
| const parts: string[] = []; | ||
|
|
||
| for (const e of inp.edits) { | ||
| const hunk = pairHunk(e.old_string, e.new_string); |
There was a problem hiding this comment.
P3: The edits loop in editHunks dereferences e.old_string/e.new_string before confirming e is an object, so one malformed element (e.g. null) throws inside editTargets. The top-level try/catch in rules.ts then swallows the event silently, so that edit lands unjudged and is never recorded for the Stop sweep — while changeHunk in the same parser guards elements with isJsonObject(e). Apply the same guard here so the shared parser consistently skips malformed inputs as its contract states.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At adapters/afk/src/shared/edit-targets.ts, line 51:
<comment>The `edits` loop in `editHunks` dereferences `e.old_string`/`e.new_string` before confirming `e` is an object, so one malformed element (e.g. `null`) throws inside `editTargets`. The top-level `try/catch` in `rules.ts` then swallows the event silently, so that edit lands unjudged and is never recorded for the Stop sweep — while `changeHunk` in the same parser guards elements with `isJsonObject(e)`. Apply the same guard here so the shared parser consistently skips malformed inputs as its contract states.</comment>
<file context>
@@ -0,0 +1,133 @@
+ const parts: string[] = [];
+
+ for (const e of inp.edits) {
+ const hunk = pairHunk(e.old_string, e.new_string);
+
+ if (hunk) parts.push(hunk);
</file context>
Suggested change
| const hunk = pairHunk(e.old_string, e.new_string); | |
| if (!isJsonObject(e)) continue; | |
| const hunk = pairHunk(e.old_string, e.new_string); |
| const timeoutMs = Math.min(DEFAULT_TIMEOUT_MS, deadline - performance.now()); | ||
|
|
||
| if (timeoutMs < MIN_CALL_MS) { | ||
| logCheckError(ctx, "hook budget spent before this file was judged", 0); |
There was a problem hiding this comment.
P3: Budget exhaustion is logged as kind rules-error, the same row type used for actual Jev API failures (stats.py and the README describe it as "Jev call failed"). Files skipped here were never attempted and still land unjudged via the fail-open path, so the row misrepresents a scheduled skip as an API failure. Drop the row (it still needs no block output) or use a dedicated skip reason.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At adapters/afk/src/rules.ts, line 195:
<comment>Budget exhaustion is logged as kind `rules-error`, the same row type used for actual Jev API failures (`stats.py` and the README describe it as "Jev call failed"). Files skipped here were never attempted and still land unjudged via the fail-open path, so the row misrepresents a scheduled skip as an API failure. Drop the row (it still needs no block output) or use a dedicated skip reason.</comment>
<file context>
@@ -180,48 +174,43 @@ async function judge(
+ const timeoutMs = Math.min(DEFAULT_TIMEOUT_MS, deadline - performance.now());
+
+ if (timeoutMs < MIN_CALL_MS) {
+ logCheckError(ctx, "hook budget spent before this file was judged", 0);
+
+ return null;
</file context>
|
|
||
| assert.equal(out, ""); | ||
| assert.equal(ruleStates.length, 3); | ||
| assert.equal(maxInFlight > 1, true, `max in flight ${maxInFlight}`); |
There was a problem hiding this comment.
P3: maxInFlight > 1 only proves two judgments overlapped; it never verifies the eight-way batching (MAX_PARALLEL waves) that the patch judge is meant to perform, and with only three files the assertion would pass even if a max-parallel limit were removed entirely. Feed more than eight files in the clean patch and assert bounded concurrency (e.g. 1 < maxInFlight && maxInFlight <= 8).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At adapters/afk/test/patch-apply.test.ts, line 291:
<comment>`maxInFlight > 1` only proves two judgments overlapped; it never verifies the eight-way batching (`MAX_PARALLEL` waves) that the patch judge is meant to perform, and with only three files the assertion would pass even if a max-parallel limit were removed entirely. Feed more than eight files in the clean patch and assert bounded concurrency (e.g. `1 < maxInFlight && maxInFlight <= 8`).</comment>
<file context>
@@ -0,0 +1,411 @@
+
+ assert.equal(out, "");
+ assert.equal(ruleStates.length, 3);
+ assert.equal(maxInFlight > 1, true, `max in flight ${maxInFlight}`);
+ assert.deepEqual(
+ ruleStates.map((s) => s.split("\n")[0]).sort(),
</file context>
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
2 participants
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.
Why
agent-afk has a
patch_applytool for atomic multi-file edits. The AFK rule hook (adapters/afk/src/rules.ts) only matchededit_fileandwrite_file, so edits made throughpatch_applylanded unjudged and were never recorded for the Stop sweep. The adapter README listed this as a known gap.What agent-afk sends (checked in agent-afk 5.286.1 source)
src/agent/hooks/command-executor.tsbuildStdinPayload: aPreToolUsecommand hook getstool_name: context.toolName(the AFK name,"patch_apply") andtool_input: context.input, the raw tool input{ changes: [{ path, edits?: [{ old, new }], content?, expected_hash? }], dry_run? }. Nofile_path, noold_string/new_string.src/agent/hooks/matcher.tscompileMatcher: a/pattern/matcher is tested against the AFK tool name only. TheCLAUDE_CODE_ALIASEStable (patch_apply: ['MultiEdit']) is used only for bare-name and anchored-regex matchers, so/^(edit_file|write_file)$/never fired forpatch_apply. Listingpatch_applyin the regex is what makes it fire. TheMultiEditalias does not matter here: the hook reads AFK'schangesinput, not Claude Code'sMultiEditinput.src/agent/tools/handlers/patch-apply.ts: relativepaths resolve against the session cwd (the samecwdthe hook receives), anddry_rundefaults to false.What
hooks.json: matcher becomes/^(edit_file|write_file|patch_apply)$/.adapters/afk/src/shared/edit-targets.ts(new) turns any of the three inputs into a list of{ filePath, rel, hunk }.edit_fileandwrite_fileproduce the same hunk as before. Eachpatch_applychange becomes the sameREMOVED:/ADDED:hunk anedit_filecall would, or its fullcontent. Two changes to one path merge. Entries without a path or content are skipped.dry_run: trueproduces nothing.rules.ts:rules-errorand not judged (fail open).stats.pysees a file-level check exactly as it does foredit_file.patch_applyKnown gaps bullet. No other README lines changed, because a parallel PR refreshes the rest.[Unreleased]bullet.Not changed: the AFK hook has no out-of-project skip today (
isOutsideis used only by the rootsrc/rules.ts).patch_applyfiles are treated the same asedit_fileones. Adding that skip would changeedit_filebehavior too, so it should be a separate PR.No version bump. Other AFK adapter PRs are open at the same time, and bumping in each would conflict. I left it to you.
Verified
node --experimental-strip-types --test adapters/afk/test/*.test.ts: 13 pass, 0 fail. The newadapters/afk/test/patch-apply.test.tsruns the real hook as a subprocess against a local HTTP server passed in through the existingJEV_BASE_URL, the same wayclassify-failure.test.tsdoes. That server stands in for the Jev endpoint, so these are wiring tests, not a measure of Jev's judgments. They cover:/…/, matchingpatch_applyand notMultiEditeditTargetsparsing for all three toolsdry_run: no Jev call, no log, no recordedit_fileblock message unchangedpatch_applyinput exits 0 with no outputnpx tsc --noEmit(root, which includes the adapter tests) andadapters/afktsc --noEmit: clean.npx oxlint: 0 warnings, 0 errors.check_no_comments.py,check_no_stubs.py: ok.echo 'not json' | node --experimental-strip-types adapters/afk/src/rules.tsexits 0. A no-keypatch_applyevent exits 0 with no stdout and logsrules-error.Not verified live. I did not run a real agent-afk session with a real key against
patch_apply.Summary by cubic
patch_applyedits from agent-afk now go through the AFK rule hook, instead of landing unjudged and never reaching the Stop sweep.patch_applyand parses its{ changes, dry_run }input into the same per-file hunksedit_fileuses, via the newadapters/afk/src/shared/edit-targets.ts.dry_runcalls entirely.patch_applyknown-gap bullet from the README (the merge from main kept the other gaps) and adds a CHANGELOG entry.Notes
isOutside) is still not applied topatch_apply; adding it would changeedit_filebehavior too, so it stays out of this PR.Written for commit ad131f0. Summary will update on new commits.