Skip to content

Commit 652d64c

Browse files
committed
bugfix: fix instruction-builder SPRT test infrastructure and rename classes
- Rename SkillTestRunner → InstructionTestRunner (class, tests, build script) - Rename binary launcher skill-test-runner → instruction-test-runner - Rename benchmark/ directory → tests/, benchmark.json → test-results.json - Rename benchmark-aggregator route → instruction-test-aggregator in GetOutput.java - Rename skill-analyzer-agent → instruction-analyzer-agent - Rename skill-grader-agent → instruction-grader-agent - Rename target_type skill_instructions → instructions in adversarial protocol - Update instruction-testing.md to describe version-comparison model (baseline vs candidate) - Remove all remaining benchmark/skill-specific terminology from plugin files
1 parent 591983e commit 652d64c

37 files changed

Lines changed: 4551 additions & 4581 deletions
Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
{
2-
"status" : "open",
3-
"dependencies" : [ ],
4-
"blocks" : [ ]
5-
}
2+
"status": "closed",
3+
"resolution": "implemented",
4+
"target_branch": "v2.1",
5+
"dependencies": [],
6+
"blocks": []
7+
}

.cat/issues/v2/v2.1/fix-instruction-builder-sprt-loop/plan.md

Lines changed: 276 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,3 +22,279 @@ skill-grader-agent if they still reference the old JSON format.
2222
- [ ] No regressions in existing test infrastructure
2323
- [ ] E2E verification: run instruction-builder on a real skill and confirm steps 4.1-4.3 execute without
2424
skipping
25+
26+
## Research Findings
27+
28+
Investigation of affected files confirms:
29+
30+
- `plugin/agents/skill-grader-agent.md` already reads `.md` scenario format — no changes needed
31+
- `plugin/skills/empirical-test-agent/first-use.md` uses its own `/tmp/empirical-test-config.json` format
32+
unrelated to test-cases.json — no changes needed
33+
- Two Java methods in `SkillTestRunner.java` currently accept `test_cases_path` (a JSON file path):
34+
- `detectChanges(String[] args)`: args `[old_skill_sha, new_skill_path, test_cases_path]` — third arg
35+
is path to test-cases.json; calls `readAllTestCaseIds(testCasesPath)` to list all TC IDs
36+
- `mapUnits(String[] args)`: args `[test_cases_path, changed_units_json]` — first arg is path to
37+
test-cases.json; reads `test_case_id` and `semantic_unit_id` from each entry in the JSON
38+
- Both must change to accept a test directory path instead of a JSON file path
39+
- New `.md` file format for generated test cases must include `semantic_unit_id` in YAML frontmatter so
40+
`map-units` can determine which test cases map to which semantic units
41+
- New `.md` file naming convention: use the `semantic_unit_id` as the file stem (e.g.,
42+
`unit_step44_guard.md`), placed in `${TEST_DIR}/`; the file stem becomes the test case ID
43+
- All assertions in `.md` format are semantic (plain-text numbered list in `## Assertions` section);
44+
the typed deterministic assertions from JSON (regex, string_match, structural) are replaced by
45+
plain-text assertions graded entirely by skill-grader-agent
46+
- Step 4.3 test-run subagent currently evaluates deterministic assertions inline and only passes semantic
47+
assertions to the grader; with `.md` format (all semantic), the test-run subagent no longer grades
48+
assertions inline — it only executes the prompt, writes output, and returns
49+
- Step 4.4 also contains references to test-cases.json in its investigation flow — those references
50+
must be updated too to maintain consistency
51+
52+
## Jobs
53+
54+
### Job 1
55+
56+
Update `plugin/skills/instruction-builder-agent/first-use.md` — rewrite Steps 4.1-4.3 (and Step 4.4
57+
references) to use `.md` scenario files instead of `test-cases.json`. Also create the regression test file.
58+
59+
**Step 4.1 rewrite** (currently lines 285-365):
60+
61+
Replace the JSON schema template section entirely. New instructions:
62+
63+
For each testable semantic unit, generate a `.md` file in `${TEST_DIR}/` named `<semantic_unit_id>.md`
64+
(the semantic_unit_id becomes the file stem and serves as the test case ID). File format:
65+
66+
```
67+
---
68+
category: <CATEGORY>
69+
semantic_unit_id: <semantic_unit_id>
70+
---
71+
<!--
72+
Copyright (c) 2026 Gili Tzabari. All rights reserved.
73+
Licensed under the CAT Commercial License.
74+
See LICENSE.md in the project root for license terms.
75+
-->
76+
## Turn 1
77+
<scenario prompt text that exercises the constraint>
78+
## Assertions
79+
1. <plain-text assertion describing expected behavior>
80+
2. <plain-text assertion>
81+
```
82+
83+
For CONDITIONAL semantic units that require two scenarios (one triggering, one not), generate two separate
84+
`.md` files: `<semantic_unit_id>_triggered.md` and `<semantic_unit_id>_not_triggered.md`.
85+
86+
Remove the JSON schema block entirely. Remove all mention of `assertion_id`, `type`, `method`,
87+
`pattern`, `expected` fields — these JSON-specific fields do not exist in the `.md` format.
88+
89+
Update the commit step: after presenting generated files to the user for approval and incorporating
90+
feedback, commit all generated `.md` files:
91+
```bash
92+
cd "${TEST_DIR}" && git add *.md && cd - && \
93+
git -C "${CLAUDE_PROJECT_DIR}" add "${TEST_DIR}/*.md" && \
94+
git -C "${CLAUDE_PROJECT_DIR}" commit -m "test: generate test cases [session: ${CLAUDE_SESSION_ID}]"
95+
```
96+
Store the commit SHA as `TEST_SET_SHA`. Do NOT retain test case content in context — test-run subagents
97+
read from the committed `.md` file for their assigned test case.
98+
99+
**Step 4.2 rewrite** (currently lines 367-416):
100+
101+
Update both CLI invocations to pass the test directory instead of the JSON file path:
102+
103+
```bash
104+
# detect-changes: third arg is now the test directory
105+
"${CLAUDE_PLUGIN_ROOT}/client/bin/skill-test-runner" detect-changes \
106+
<SKILL_DRAFT_SHA> <SKILL_TEXT_PATH> "${TEST_DIR}"
107+
```
108+
109+
```bash
110+
# map-units: first arg is now the test directory
111+
"${CLAUDE_PLUGIN_ROOT}/client/bin/skill-test-runner" map-units \
112+
"${TEST_DIR}" '["unit_step44_guard", "unit_step44_reject"]'
113+
```
114+
115+
Update the output field descriptions to reflect that `all_test_case_ids` is now derived from `.md` file
116+
stems in `${TEST_DIR}` rather than from entries in `test-cases.json`.
117+
118+
**Step 4.3 rewrite** (currently lines 418-775):
119+
120+
1. **Test-run subagent prompt**: Change "reads assertions from `cat {TEST_DIR}/test-cases.json`" to
121+
"reads the assigned scenario file from `cat {TEST_DIR}/{test_case_id}.md`"
122+
123+
2. **Assertion grading flow**: Since all assertions in `.md` format are semantic (plain-text numbered
124+
list), remove the inline deterministic assertion grading from the test-run subagent:
125+
- Remove: "Evaluates deterministic assertions inline and reports results before returning"
126+
- Remove: step (a) in the pipelining control flow: "Independently verifies deterministic assertions..."
127+
- Change step (b): "Spawns a grader subagent for all assertions" (not just semantic ones)
128+
- The test-run subagent return format simplifies to:
129+
`{"run_id": "<TC_id>_run_<N>", "test_case_id": "<TC_id>", "output_path": "...",
130+
"duration_ms": <integer>, "total_tokens": <integer>}`
131+
- Remove `assertion_results` and `semantic_pending` fields from the return format
132+
133+
3. **Prohibition text** (currently "The ONLY permitted read from {TEST_DIR} is test-cases.json"):
134+
Replace with: "The ONLY permitted read from {TEST_DIR} is `{test_case_id}.md` (the assigned scenario
135+
file for this run). Do NOT read any other file under {TEST_DIR} via any mechanism."
136+
137+
4. **Permitted read list** in the prohibition block (currently "(1) `{TEST_DIR}/test-cases.json`"):
138+
Replace with: "(1) `{TEST_DIR}/{test_case_id}.md` (the assigned scenario file)"
139+
140+
5. **Check 2 — Prohibition verification**: Change the rejection condition from "references file paths
141+
under `{TEST_DIR}/` other than `test-cases.json`" to "references file paths under `{TEST_DIR}/`
142+
other than `{test_case_id}.md`"
143+
144+
6. **Check 3 — Design-flaw detection**: Change "Read the assertion's `semantic_unit_id` from
145+
`test-cases.json`" to "Read the `semantic_unit_id` field from the YAML frontmatter of
146+
`{TEST_DIR}/{test_case_id}.md`"
147+
148+
7. **Minimal happy-path example**: Update to show the new return format (no `assertion_results`
149+
or `semantic_pending`), show that main agent spawns grader for all assertions
150+
151+
8. **Scalar references passed to test-run subagent**: Update the note to say the subagent reads
152+
the scenario from `{TEST_DIR}/{test_case_id}.md` instead of from `test-cases.json`
153+
154+
**Step 4.4 references** (currently at line ~777+):
155+
156+
Scan the full SPRT Failure Investigation section for any remaining `test-cases.json` references and
157+
replace them with the equivalent `.md` format references. Specifically, any instructions to read
158+
`test-cases.json` for assertion or semantic_unit_id data should be updated to read from the
159+
`{test_case_id}.md` frontmatter.
160+
161+
**Regression test file**: Create
162+
`plugin/tests/skills/instruction-builder-agent/first-use/step41-generates-md-scenario-files.md` with
163+
content:
164+
165+
```
166+
---
167+
category: REQUIREMENT
168+
semantic_unit_id: unit_step41_md_generation
169+
---
170+
<!--
171+
Copyright (c) 2026 Gili Tzabari. All rights reserved.
172+
Licensed under the CAT Commercial License.
173+
See LICENSE.md in the project root for license terms.
174+
-->
175+
## Turn 1
176+
You are the instruction-builder-agent working in Step 4.1. The skill being tested is a fictional skill
177+
"log-analyzer-agent" with one semantic unit: unit_log_1 (REQUIREMENT: always summarize findings in a
178+
table). Generate the test case for unit_log_1 using the .md format. Show the complete file content you
179+
would write to plugin/tests/skills/log-analyzer-agent/first-use/unit_log_1.md.
180+
## Assertions
181+
1. response must produce a file path like plugin/tests/skills/log-analyzer-agent/first-use/unit_log_1.md
182+
2. response must show markdown file content with YAML frontmatter block delimited by --- markers
183+
3. frontmatter must include a category field
184+
4. frontmatter must include a semantic_unit_id field with value unit_log_1
185+
5. file must include a ## Turn 1 section containing a scenario prompt
186+
6. file must include a ## Assertions section with at least one numbered assertion
187+
7. response must NOT include any JSON structure with test_cases array or assertion_id fields
188+
```
189+
190+
**Commit** with message:
191+
`bugfix: update SPRT loop steps 4.1-4.3 to use .md scenario format; add regression test`
192+
193+
### Job 2
194+
195+
Update `client/src/main/java/io/github/cowwoc/cat/hooks/skills/SkillTestRunner.java` to accept a test
196+
directory path instead of a JSON file path for `detect-changes` and `map-units` subcommands. Also update
197+
or add tests in the test module. Then verify the build and close the issue.
198+
199+
**`detectChanges` method** (currently starting at line 236):
200+
201+
- Update Javadoc: `@param args {@code [old_skill_sha, new_skill_path, test_dir]}` — third arg is now a
202+
directory path, not a JSON file path
203+
- Update the `args.length != 3` error message and usage string:
204+
`"Usage: skill-test-runner detect-changes <old_skill_sha> <new_skill_path> <test_dir>"`
205+
- Rename local variable: `Path testDir = Path.of(args[2])` (was `testCasesPath`)
206+
- Replace `Files.notExists(testCasesPath)` check with:
207+
`if (Files.notExists(testDir) || !Files.isDirectory(testDir))`
208+
and update error message:
209+
`"SkillTestRunner detect-changes: test directory not found: " + testDir`
210+
- Change `readAllTestCaseIds(testCasesPath)` call to `readAllTestCaseIds(testDir)` — now reads `.md`
211+
file stems from the directory
212+
- The `semantic_units_path_hint` string that references `args[2]` continues to work as-is since `args[2]`
213+
is now the directory path (used verbatim in the hint string)
214+
215+
**`mapUnits` method** (currently starting at line 385):
216+
217+
- Update Javadoc: `@param args {@code [test_dir, changed_units_json]}`
218+
- Update the `args.length != 2` error message and usage string:
219+
`"Usage: skill-test-runner map-units <test_dir> <changed_units_json>"`
220+
- Rename local variable: `Path testDir = Path.of(args[0])` (was `testCasesPath`)
221+
- Replace `Files.notExists(testCasesPath)` check with:
222+
`if (Files.notExists(testDir) || !Files.isDirectory(testDir))`
223+
and update error message:
224+
`"SkillTestRunner map-units: test directory not found: " + testDir`
225+
- Replace the JSON-reading logic (which read `root.path("test_cases")` array) with directory scanning:
226+
- List all `.md` files in `testDir` sorted by file name for deterministic ordering
227+
- For each `.md` file:
228+
- `testCaseId` = file stem (filename without `.md` extension)
229+
- `semanticUnitId` = read `semantic_unit_id` from YAML frontmatter using new helper method
230+
- Partition into `rerunIds` / `carryforwardIds` based on whether `semanticUnitId` is in `changedUnits`
231+
232+
**`readAllTestCaseIds` helper** (currently at line ~1249):
233+
234+
- Change the method to list `.md` files in the test directory (a `Path` parameter):
235+
- Use `Files.list(testDir)` filtered to `.md` files
236+
- Return file stems (filename without `.md` extension) sorted alphabetically for determinism
237+
- Update Javadoc accordingly
238+
239+
**New private helper method** `readFrontmatterField(Path mdFile, String fieldName)`:
240+
241+
- Purpose: extract a named field from the YAML frontmatter block of a `.md` file
242+
- Algorithm:
243+
1. Read all file lines
244+
2. If the first line is `---`, collect lines until the next `---` line (the frontmatter block)
245+
3. For each frontmatter line, match `fieldName: value` pattern
246+
4. Return the trimmed value, or empty string if field not found or no frontmatter present
247+
- Add Javadoc:
248+
```java
249+
/**
250+
* Reads a named field from the YAML frontmatter block of a Markdown file.
251+
* <p>
252+
* Frontmatter is the block between the first two {@code ---} delimiters at the top of the file.
253+
* Returns an empty string if the file has no frontmatter or the field is absent.
254+
*
255+
* @param mdFile path to the Markdown file
256+
* @param fieldName the frontmatter key to look up
257+
* @return the field value, or an empty string if not found
258+
* @throws IOException if the file cannot be read
259+
* @throws NullPointerException if {@code mdFile} or {@code fieldName} are null
260+
*/
261+
```
262+
263+
**Tests** in `client/src/test/java/io/github/cowwoc/cat/hooks/test/SkillTestRunnerTest.java`
264+
(file already exists — add new test methods to it):
265+
266+
- Test `detectChanges` with a test directory (create temp dir with some `.md` files representing test
267+
cases; create a temp git repo with `TestUtils.createTempGitRepo("main")`):
268+
- Verify it lists `.md` file stems as test case IDs in `all_test_case_ids`
269+
- Verify it returns correct `skill_changed`, `frontmatter_changed`, `body_changed` fields
270+
- Test `mapUnits` with a test directory containing `.md` files that have `semantic_unit_id` in frontmatter:
271+
- Verify it correctly partitions test cases into `rerun_test_case_ids` and `carryforward_test_case_ids`
272+
- Test with changed units that match some semantic_unit_ids and not others
273+
- Test `readFrontmatterField`:
274+
- File with frontmatter containing the field → returns correct value
275+
- File with frontmatter missing the field → returns empty string
276+
- File with no frontmatter → returns empty string
277+
278+
**Build verification**: Run `mvn -f client/pom.xml verify -e` from the worktree directory to confirm all
279+
tests and linters pass.
280+
281+
**Close issue**: Update `.cat/issues/v2/v2.1/fix-instruction-builder-sprt-loop/index.json`: set
282+
`"status": "closed"`.
283+
284+
**Commit** with message:
285+
`bugfix: update skill-test-runner to accept test directory; add tests; close issue`
286+
287+
## Success Criteria
288+
289+
- `plugin/skills/instruction-builder-agent/first-use.md` Step 4.1 describes generating individual `.md`
290+
files with YAML frontmatter containing `category` and `semantic_unit_id`, a `## Turn 1` section, and
291+
a `## Assertions` numbered list — no JSON schema block remains
292+
- `plugin/skills/instruction-builder-agent/first-use.md` Steps 4.2-4.3 pass `${TEST_DIR}` (directory)
293+
to `detect-changes` and `map-units` CLI commands — no references to `test-cases.json` remain in 4.1-4.3
294+
- `plugin/skills/instruction-builder-agent/first-use.md` Step 4.3 test-run subagent reads
295+
`{TEST_DIR}/{test_case_id}.md` and its prohibition permits only that file from `${TEST_DIR}`
296+
- `SkillTestRunner.detectChanges` accepts a directory as its third argument and derives test case IDs
297+
from `.md` file stems in that directory
298+
- `SkillTestRunner.mapUnits` accepts a directory as its first argument and reads `semantic_unit_id`
299+
from `.md` file frontmatter to partition test cases
300+
- `mvn -f client/pom.xml verify -e` passes with zero errors or warnings
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
{
2+
"status" : "open",
3+
"dependencies" : [ ],
4+
"target_branch" : "v2.1"
5+
}
Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
# Plan: fix-work-review-autofix-plan-path
2+
3+
## Type
4+
bugfix
5+
6+
## Goal
7+
Fix the planning subagent in `work-review-agent`'s auto-fix iteration to write intermediate plan artifacts
8+
to `.cat/work/` instead of `.claude/`.
9+
10+
## Problem
11+
When `work-review-agent` runs its auto-fix iteration loop (to address stakeholder concerns), it spawns a
12+
planning subagent to create fix plans. That subagent creates `review-fix-plans.md` in `.claude/` — a
13+
developer-facing directory that is not shipped to end users and is not intended for runtime artifacts.
14+
15+
The closed issue `2.1-move-review-artifacts-to-cat-work` fixed concern artifacts (`.cat/review/`
16+
`.cat/work/review/`), but did NOT fix the planning subagent's output path. This is a different code path
17+
and a recurrence of the same class of bug.
18+
19+
**Root cause:** The planning subagent was not given an explicit output path in its prompt. It defaulted to
20+
`.claude/` (which it treated as a project working directory).
21+
22+
## Target State
23+
The planning subagent in `work-review-agent`'s auto-fix loop writes `review-fix-plans.md` to
24+
`${WORKTREE_PATH}/.cat/work/review-fix-plans.md` (or similar path under `.cat/work/`).
25+
26+
## Post-conditions
27+
- [ ] `work-review-agent` auto-fix planning subagent writes artifacts to `.cat/work/` not `.claude/`
28+
- [ ] The explicit output path is passed to the planning subagent in its prompt
29+
- [ ] No new files appear under `.claude/` during a normal `work-review-agent` auto-fix iteration
30+
31+
## Files to Modify
32+
- `plugin/skills/work-review-agent/first-use.md` — update the planning subagent prompt to specify
33+
output path as `${WORKTREE_PATH}/.cat/work/review-fix-plans.md`
34+
35+
## Pre-conditions
36+
- [ ] All dependent issues are closed
37+
38+
## Execution Steps
39+
1. In `plugin/skills/work-review-agent/first-use.md`, locate the section where the planning subagent
40+
is spawned for auto-fix iteration
41+
2. Add explicit output path instruction to the planning subagent prompt:
42+
- The subagent must write its plan to `${WORKTREE_PATH}/.cat/work/review-fix-plans.md`
43+
- The path must use `${WORKTREE_PATH}` to be worktree-isolated
44+
3. Update any references in the skill that read the plan from `.claude/review-fix-plans.md` to read
45+
from `${WORKTREE_PATH}/.cat/work/review-fix-plans.md`
46+
4. Run relevant tests to confirm no regressions
47+
5. Commit as `bugfix: fix work-review auto-fix plan path from .claude to .cat/work`

.cat/retrospectives/index.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
"trigger_interval_days" : 7
66
},
77
"last_retrospective" : "2026-03-23T13:18:51.196642020Z",
8-
"mistake_count_since_last" : 13,
8+
"mistake_count_since_last" : 14,
99
"files" : {
1010
"mistakes" : [ "mistakes-2026-01.json", "mistakes-2026-02.json", "mistakes-2026-03.json" ],
1111
"retrospectives" : [ "retrospectives-2026-01.json", "retrospectives-2026-02.json", "retrospectives-2026-03.json" ]

0 commit comments

Comments
 (0)