Skip to content

Commit b3b8b18

Browse files
committed
planning: record tee hardening review artifacts
Preserve issue status updates, follow-up issue plans, and adversarial validation artifacts produced while hardening the tee-piped-output workflow.
1 parent e0c4bf5 commit b3b8b18

20 files changed

Lines changed: 514 additions & 2 deletions

File tree

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
{
2+
"status": "open",
3+
"dependencies": []
4+
}
Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
<!--
2+
Copyright (c) 2026 Gili Tzabari. All rights reserved.
3+
Licensed under the CAT Commercial License.
4+
See LICENSE.md in the project root for license terms.
5+
-->
6+
# Issue: clarify-skill-launch-execution-continuation
7+
8+
## Type
9+
bugfix
10+
11+
## Goal
12+
Update execution-model guidance so that after skill launch acknowledgment without completion artifacts, execution continues through first-use instructions rather than returning control to the user.
13+
14+
## Pre-conditions
15+
- [ ] Reproduce the launch-only behavior with a failing regression test
16+
- [ ] Verify the failing test demonstrates missing continuation behavior
17+
18+
## Post-conditions
19+
- [ ] execution-model.md explicitly defines continuation requirements after launch acknowledgment
20+
- [ ] When a skill returns only `Launching skill: ...`, execution proceeds by reading and running that skill's `first-use.md` contract in the same turn
21+
- [ ] Continuation logic includes a deterministic stop condition and does not loop indefinitely on repeated launch-only acknowledgments
22+
- [ ] If launch acknowledgment is returned but `first-use.md` cannot be loaded or is invalid, execution fails explicitly instead of silently succeeding
23+
- [ ] Continuation behavior is consistent for explicit `/skill` invocation and automatic skill invocation
24+
- [ ] Regression coverage verifies both continuation action and absence of “please rerun” fallback on the happy path
25+
- [ ] Regression coverage ensures launch-only responses do not terminate skill execution prematurely
26+
- [ ] `mvn -f client/pom.xml verify -e` passes
Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
# Plan: Document CLI String[] Entry-Point Parameters
2+
3+
## Goal
4+
5+
Update `@param` Javadoc on every legitimate CLI `String[] args` parameter at the boundary between
6+
the JVM and internal Java code, following the new convention in `.claude/rules/java.md`:
7+
8+
> Every positional element must be listed in order with an individual description. Every recognized
9+
> flag must be described separately. No element may be left undescribed or deferred to another method.
10+
11+
The `refactor-instruction-test-runner-string-args` issue converts internal dispatch methods away
12+
from `String[]`. This issue is complementary: it improves the Javadoc on the methods that legitimately
13+
keep `String[]` because they sit at a shell-to-Java CLI boundary.
14+
15+
## Parent Requirements
16+
17+
None — code quality and maintainability.
18+
19+
## Affected Methods
20+
21+
### Hook entry points (`client/src/main/java/io/github/cowwoc/cat/claude/hook/`)
22+
23+
Each hook class follows the pattern `main(String[] args)``run(scope, args, in, out)`.
24+
The `run()` method is the real CLI boundary; `main()` just delegates. Javadoc is needed on
25+
both if they carry any `@param args` description.
26+
27+
| Class | Method |
28+
|-------|--------|
29+
| `HookRunner` | `execute(HookHandlerFactory, String[])` |
30+
| `PreToolUseHook` | `main(String[])`, `run(ClaudeHook, String[], InputStream, PrintStream)` |
31+
| `PreAskHook` | `main(String[])`, `run(ClaudeHook, String[], InputStream, PrintStream)` |
32+
| `PreReadHook` | `main(String[])`, `run(ClaudeHook, String[], InputStream, PrintStream)` |
33+
| `PreWriteHook` | `main(String[])`, `run(ClaudeHook, String[], InputStream, PrintStream)` |
34+
| `PreIssueHook` | `main(String[])`, `run(ClaudeHook, String[], InputStream, PrintStream)` |
35+
| `PostToolUseHook` | `main(String[])`, `run(ClaudeHook, String[], InputStream, PrintStream)` |
36+
| `PostToolUseFailureHook` | `main(String[])`, `run(ClaudeHook, String[], InputStream, PrintStream)` |
37+
| `PostReadHook` | `main(String[])`, `run(ClaudeHook, String[], InputStream, PrintStream)` |
38+
| `SessionStartHook` | `main(String[])`, `run(ClaudeHook, Path, String[], InputStream, PrintStream)` |
39+
| `SessionEndHook` | `main(String[])`, `run(ClaudeHook, String[], InputStream, PrintStream)` |
40+
| `UserPromptSubmitHook` | `main(String[])`, `run(ClaudeHook, String[], InputStream, PrintStream)` |
41+
| `EnforceStatusOutput` | `main(String[])`, `run(ClaudeHook, String[], InputStream, PrintStream)` |
42+
| `TokenCounter` | `main(String[])`, `run(JvmScope, String[], PrintStream)` |
43+
| `SubagentStartHook` | `main(String[])` |
44+
45+
### Skill CLI entry points (`client/src/main/java/io/github/cowwoc/cat/claude/hook/skills/`)
46+
47+
| Class | Method |
48+
|-------|--------|
49+
| `ClaudeRunner` | `run(ClaudeTool, String[], PrintStream)` |
50+
| `GetConfigOutput` | `getOutput(String[])`, `run(ClaudeTool, String[], PrintStream)` |
51+
| `GetStatuslineOutput` | `getOutput(String[])`, `run(ClaudeTool, String[], PrintStream)` |
52+
| `GetIssueCompleteOutput` | `getOutput(String[])`, `run(ClaudeTool, String[], PrintStream)` |
53+
| `GetAddOutput` | `getOutput(String[])`, `run(ClaudeTool, String[], PrintStream)` |
54+
| `SkillComparison` | `getOutput(String[])` |
55+
| `ExtractTurnsContent` | `main(String[])` |
56+
| `InstructionTestRunner` | `main(String[])` (top-level CLI; subcommands are covered by `refactor-instruction-test-runner-string-args`) |
57+
58+
## Approach
59+
60+
For each `String[] args` parameter, read the method body to discover:
61+
1. How many positional arguments it consumes (and from which indices)
62+
2. What each positional argument represents (name, type, valid values, sentinels)
63+
3. Which optional flags it recognizes and what they do
64+
65+
Then write a `@param args` block with:
66+
- An `<ol>` listing positional elements **in order** (item 1 = `args[0]`, item 2 = `args[1]`, ...)
67+
- A `<ul>` for optional flags (if any), each individually described
68+
- Sentinel values called out explicitly (e.g., `{@code "none"}` when no value is available)
69+
70+
`main(String[] args)` methods that simply delegate to `run()` with no additional parsing may
71+
carry a brief `@param args` that refers to the `run()` signature, rather than duplicating it.
72+
73+
## Exclusions
74+
75+
- Methods already covered by `refactor-instruction-test-runner-string-args`: the internal
76+
`String[]` dispatch methods in `InstructionTestRunner`, `SprtStateManager`, and
77+
`SprtIsolationManager` — those will be converted to named parameters, eliminating the `String[]`.
78+
- `AotTraining.main(String[])`: training harness, not a production CLI boundary.
79+
- Test classes.
80+
81+
## Verification
82+
83+
- Every `String[] args` parameter at a CLI boundary has a `@param` block that individually describes
84+
each positional element and each recognized flag.
85+
- No element description defers to another method ("see handler for details").
86+
- `mvn -f client/pom.xml verify -e` passes with no Checkstyle or Javadoc errors.
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
{
2+
"status" : "open"
3+
}
Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
# Plan: Fix SPRT API Error Retry
2+
3+
## Goal
4+
5+
When a claude-runner JSON output contains only API error responses (e.g., "API Error: 503 Service
6+
temporarily unavailable") with an empty `bashCommands[]`, the SPRT runner should detect this as an
7+
infrastructure failure and retry the run rather than recording a FAIL verdict. Transient API outages
8+
must not abort SPRT suites via the early-failure-detection threshold.
9+
10+
## Parent Requirements
11+
12+
None — testing infrastructure reliability fix.
13+
14+
## Current State
15+
16+
`InstructionTestRunner.runSprtLoop` runs each trial via `SprtGrader.gradeTc`, which invokes a
17+
grader agent. The grader sees empty `bashCommands[]` and marks assertions FAIL because no behavior
18+
was demonstrated. The SPRT runner counts these as real failures, triggering early abort after 4
19+
failures.
20+
21+
`SprtGrader.invokeGrader` already retries on grader API errors (the grader process itself failing
22+
with exit code != 0 containing "API Error"). But the subject-under-test run — the claude-runner
23+
process that actually executes the test scenario — has no retry logic for API errors.
24+
25+
## Root Cause
26+
27+
The claude-runner JSON produced for the subject run contains:
28+
- `texts[]`: one or more entries with "API Error: 503 Service temporarily unavailable."
29+
- `bashCommands[]`: empty array
30+
- `toolUses[]`: empty array
31+
32+
This is detectable. The grader correctly marks it FAIL (no behavior observed), but the SPRT loop
33+
should treat this as a retryable infrastructure error, not a behavioral failure.
34+
35+
## Implementation
36+
37+
### 1. Add API-error detection to `ClaudeRunner.ParsedOutput`
38+
39+
In `ParsedOutput`, add a helper method:
40+
41+
```java
42+
public boolean isApiError() {
43+
return bashCommands.isEmpty()
44+
&& toolUses.isEmpty()
45+
&& texts.stream().anyMatch(t -> t.contains("API Error:"));
46+
}
47+
```
48+
49+
### 2. Retry subject runs in `InstructionTestRunner`
50+
51+
In the trial execution loop inside `runSprtLoop` (and any shared helper), after calling
52+
`runner.executeProcess(...)`, check `result.parsed().isApiError()`. If true:
53+
- Log a warning with the TC ID, trial number, and attempt number
54+
- Sleep `attempt * 5` seconds (same backoff as grader retries)
55+
- Retry up to `MAX_API_ERROR_RETRIES = 3` times
56+
- If all retries are exhausted, throw `IOException` (fail the batch, not just the trial)
57+
58+
### 3. Update `SprtGrader.buildGraderPrompt`
59+
60+
Add a note to the grader prompt: if `texts[]` contains only API errors and `bashCommands[]` is
61+
empty, the run is an infrastructure failure — do not grade it, throw an error instead. (This is a
62+
belt-and-suspenders measure in case a retried run still slips through.)
63+
64+
## Test
65+
66+
Run `mvn -f client/pom.xml verify -e` — all existing tests must pass.
67+
68+
Run the full SPRT suite for `plugin/tests/rules/tee-piped-output` after the fix. The 4 false
69+
failures from the 503 outage must not recur for infrastructure reasons.
70+
71+
## Files to Change
72+
73+
- `client/src/main/java/io/github/cowwoc/cat/claude/hook/skills/ClaudeRunner.java` — add
74+
`isApiError()` to `ParsedOutput`
75+
- `client/src/main/java/io/github/cowwoc/cat/claude/hook/skills/InstructionTestRunner.java`
76+
add retry loop around subject run execution
77+
- `client/src/main/java/io/github/cowwoc/cat/claude/hook/skills/SprtGrader.java` — add
78+
belt-and-suspenders note to grader prompt
79+
80+
## Post-conditions
81+
82+
- `ClaudeRunner.ParsedOutput.isApiError()` exists and returns true iff texts contain "API Error:"
83+
and both `bashCommands` and `toolUses` are empty
84+
- Subject runs that return only API errors are retried up to 3 times before failing the batch
85+
- SPRT suites are not aborted by transient 503 outages
86+
- All existing tests pass
Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
{
2-
"status" : "open",
2+
"status" : "closed",
3+
"resolution" : "implemented",
4+
"target_branch" : "v2.1",
35
"dependencies" : [ ],
46
"blocks" : [ ]
57
}

.cat/issues/v2/v2.1/index.json

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,12 @@
2323
"codex-latest-only-release-alignment",
2424
"update-codex-0133-plugin-alignment",
2525
"update-ai-output-license-headers",
26-
"require-javadoc-for-private-methods"
26+
"require-javadoc-for-private-methods",
27+
"fix-sprt-api-error-retry",
28+
"refactor-instruction-test-runner-string-args",
29+
"document-cli-string-array-params",
30+
"normalize-frontmatter-key-casing",
31+
"replace-claude-session-id-with-claude-code-session-id",
32+
"clarify-skill-launch-execution-continuation"
2733
]
2834
}
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"status" : "open",
3+
"resolution" : "",
4+
"target_branch" : "v2.1",
5+
"dependencies" : [ ],
6+
"blocks" : [ ]
7+
}
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
# normalize-frontmatter-key-casing
2+
3+
## Goal
4+
5+
All YAML frontmatter keys must use hyphen-separated lowercase (e.g., `main-agent`, `sub-agents`,
6+
`argument-hint`). Currently many files use camelCase keys (e.g., `mainAgent`, `subAgents`,
7+
`argumentHint`). This issue audits all files with frontmatter and renames camelCase keys to
8+
hyphen-separated lowercase.
9+
10+
## Background
11+
12+
The naming convention was added to `.claude/rules/naming-conventions.md` as part of
13+
`improve-tee-piped-output-rule`. All frontmatter keys in skills, agents, rules, and test files
14+
must follow the new convention going forward.
15+
16+
## Acceptance Criteria
17+
18+
- [ ] All `.md` files with YAML frontmatter in `plugin/` and `.claude/` have hyphen-separated
19+
lowercase keys
20+
- [ ] Any Java code that reads frontmatter keys is updated to match the new key names
21+
- [ ] A migration script is added to `plugin/migrations/` to rename keys in existing files
22+
23+
## Implementation Steps
24+
25+
1. Find all `.md` files with YAML frontmatter:
26+
```bash
27+
grep -rl "^---" plugin/ .claude/ | xargs grep -l "^[a-z][a-zA-Z]*:" 2>/dev/null
28+
```
29+
2. Identify all camelCase keys in use (e.g., `mainAgent`, `subAgents`, `argumentHint`,
30+
`userInvocable`)
31+
3. Rename each key to its hyphen-separated equivalent in all files
32+
4. Update Java code in `client/` that reads these keys (e.g., `SkillMetadataExtractor`,
33+
`GetSkill`, frontmatter parsers)
34+
5. Add migration script to `plugin/migrations/` that renames camelCase frontmatter keys in
35+
all `.md` files
36+
6. Run all tests to verify no breakage
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
{
2+
"status" : "open"
3+
}

0 commit comments

Comments
 (0)