feature: add version-aware model ID resolution to test results schema

Author: cowwoc

.cat/issues/v2/v2.1/add-model-id-to-test-results-schema/index.json

@@ -1,3 +1,5 @@
 {
-  "status" : "open"
+  "status" : "closed",
+  "resolution" : "implemented",
+  "target_branch" : "v2.1"
 }

.cat/issues/v2/v2.1/add-model-id-to-test-results-schema/plan.md

@@ -9,6 +9,108 @@ model changes.
 
 (none)
 
+## Research Findings
+
+### Current State
+
+1. **Skills results.json** (`plugin/tests/skills/*/first-use/results.json`): Has a `model` field but stores
+   short names like `"haiku"` instead of fully-qualified IDs like `"claude-haiku-4-5-20251001"`. The schema
+   documentation (`plugin/concepts/skill-test-results.md`) already specifies `"claude-sonnet-4-6"` as the
+   expected format, but the code does not comply.
+
+2. **Instruction-test.json** (`plugin/skills/*/instruction-test/instruction-test.json`): Written by
+   `InstructionTestRunner.persistArtifacts()`. Contains `session_id`, `phase`, `timestamp`, `skill` (path +
+   sha256), `test_cases` (path + sha256) — but NO model field at all.
+
+3. **Rules test-results.json** (`plugin/skills/*/test/test-results.json` and
+   `plugin/tests/rules/*/test-results.json`): Contains `sprt.session_id` and `sprt.test_cases[]` with SPRT
+   data — but NO model field at all.
+
+4. **Model extraction**: `InstructionTestRunner.extractModel()` reads the `model:` field from SKILL.md
+   frontmatter and returns short names (`"haiku"`, `"sonnet"`, `"opus"`). Falls back to `"haiku"` if absent.
+
+5. **No staleness detection**: No existing logic compares stored model against current model to detect stale
+   cached results.
+
+### Approach: Model ID Resolution
+
+Short model names from SKILL.md frontmatter must be resolved to fully-qualified model IDs. Create a
+`ModelIdResolver` utility class in Java that maps short names to their fully-qualified equivalents:
+- `haiku` → `claude-haiku-4-5-20251001`
+- `sonnet` → `claude-sonnet-4-6`
+- `opus` → `claude-opus-4-6`
+
+This mapping is maintained as a static map in Java, easily updated when model versions change.
+
+## Risk Assessment
+
+- **Risk Level:** MEDIUM
+- **Concerns:** Existing cached results.json files will have short model names; changing to fully-qualified
+  IDs means all existing cached results become stale (model mismatch). This is the desired behavior — it
+  forces re-validation after model changes.
+- **Mitigation:** The migration is intentional. Existing results with short names will naturally trigger
+  re-validation when compared against the new fully-qualified format.
+
+## Files to Modify
+
+- `client/src/main/java/io/github/cowwoc/cat/hooks/skills/ModelIdResolver.java` — NEW: model name resolution utility
+- `client/src/main/java/io/github/cowwoc/cat/hooks/skills/InstructionTestRunner.java` — update `extractModel()` to return fully-qualified IDs; update `persistArtifacts()` to write model_id; update `mergeResults()` and `init-sprt` to handle model staleness
+- `client/src/test/java/io/github/cowwoc/cat/hooks/test/ModelIdResolverTest.java` — NEW: tests for model resolution
+- `client/src/test/java/io/github/cowwoc/cat/hooks/test/InstructionTestRunnerModelIdTest.java` — NEW: tests for model ID in results
+- `plugin/concepts/skill-test-results.md` — update schema documentation to include model_id field and staleness semantics
+
+## Jobs
+
+### Job 1
+
+- Create `ModelIdResolver.java` in `client/src/main/java/io/github/cowwoc/cat/hooks/skills/`:
+  - A final class with a static method `resolve(String shortName)` that maps short model names to fully-qualified model IDs
+  - Mapping: `haiku` → `claude-haiku-4-5-20251001`, `sonnet` → `claude-sonnet-4-6`, `opus` → `claude-opus-4-6`
+  - Case-insensitive matching on the short name
+  - Throws `IllegalArgumentException` for unknown model names
+  - Include proper Javadoc, license header, and validation per project conventions
+  - Files: `client/src/main/java/io/github/cowwoc/cat/hooks/skills/ModelIdResolver.java`
+
+- Update `InstructionTestRunner.extractModel()` method to resolve the short model name to a fully-qualified model ID by calling `ModelIdResolver.resolve()` on the value read from SKILL.md frontmatter
+  - Files: `client/src/main/java/io/github/cowwoc/cat/hooks/skills/InstructionTestRunner.java`
+
+- Update `InstructionTestRunner.persistArtifacts()` to include a `"model_id"` field in the instruction-test.json output:
+  - Add a new argument to `persistArtifacts()` for the model_id, OR extract it from the skill path (read SKILL.md frontmatter and resolve)
+  - Write `root.put("model_id", modelId)` in the JSON output alongside `session_id`, `phase`, `timestamp`
+  - Files: `client/src/main/java/io/github/cowwoc/cat/hooks/skills/InstructionTestRunner.java`
+
+- Update `InstructionTestRunner.mergeResults()` to check model staleness:
+  - When reading prior instruction-test.json, extract the `model_id` field
+  - Compare against the current model_id (resolved from skill frontmatter)
+  - If model_id differs (or is absent in prior results), treat ALL prior results as stale — do not carry forward any SPRT states
+  - Log a message: "Model changed from {prior} to {current}, invalidating cached SPRT results"
+  - Files: `client/src/main/java/io/github/cowwoc/cat/hooks/skills/InstructionTestRunner.java`
+
+- Update `InstructionTestRunner` init-sprt command to check model staleness:
+  - When reading prior instruction-test results for carry-forward, check model_id
+  - If prior model_id differs from current, skip carry-forward entirely (start fresh SPRT)
+  - Files: `client/src/main/java/io/github/cowwoc/cat/hooks/skills/InstructionTestRunner.java`
+
+- Create `ModelIdResolverTest.java` in `client/src/test/java/io/github/cowwoc/cat/hooks/test/`:
+  - Test that `resolve("haiku")` returns `"claude-haiku-4-5-20251001"`
+  - Test that `resolve("sonnet")` returns `"claude-sonnet-4-6"`
+  - Test that `resolve("opus")` returns `"claude-opus-4-6"`
+  - Test case-insensitive matching: `resolve("HAIKU")` returns `"claude-haiku-4-5-20251001"`
+  - Test that unknown names throw `IllegalArgumentException`
+  - Files: `client/src/test/java/io/github/cowwoc/cat/hooks/test/ModelIdResolverTest.java`
+
+- Update `plugin/concepts/skill-test-results.md` schema documentation:
+  - Add `model_id` field to the top-level fields table (fully-qualified model identifier, e.g., `claude-haiku-4-5-20251001`)
+  - Document the `model` field as the short name from SKILL.md frontmatter (for human reference)
+  - Add a "Staleness Detection" section explaining that when `model_id` in cached results differs from the current model's fully-qualified ID, all cached SPRT results are invalidated
+  - Update the example JSON to include a `model_id` field
+  - Files: `plugin/concepts/skill-test-results.md`
+
+- Update index.json to status: closed, progress: 100%
+  - Files: `.cat/issues/v2/v2.1/add-model-id-to-test-results-schema/index.json`
+
+- Run `mvn -f client/pom.xml verify -e` to verify all tests pass
+
 ## Post-conditions
 
 - [ ] Both skills results.json and rules test-results.json schemas include a fully-qualified model ID field (e.g., claude-haiku-4-5-20251001)

client/src/main/java/io/github/cowwoc/cat/hooks/skills/InstructionTestRunner.java

@@ -109,17 +109,22 @@ public final class InstructionTestRunner
 
   private final Logger log = LoggerFactory.getLogger(InstructionTestRunner.class);
   private final JvmScope scope;
+  private final String claudeCodeVersion;
 
   /**
    * Creates a new InstructionTestRunner.
    *
-   * @param scope the JVM scope providing shared services
-   * @throws NullPointerException if {@code scope} is null
+   * @param scope             the JVM scope providing shared services
+   * @param claudeCodeVersion the Claude Code version string (e.g., {@code "2.1.87"})
+   * @throws NullPointerException     if {@code scope} or {@code claudeCodeVersion} are null
+   * @throws IllegalArgumentException if {@code claudeCodeVersion} is blank
    */
-  public InstructionTestRunner(JvmScope scope)
+  public InstructionTestRunner(JvmScope scope, String claudeCodeVersion)
   {
     requireThat(scope, "scope").isNotNull();
+    requireThat(claudeCodeVersion, "claudeCodeVersion").isNotBlank();
     this.scope = scope;
+    this.claudeCodeVersion = claudeCodeVersion;
   }
 
   /**
@@ -191,11 +196,12 @@ public String extractUnits(String[] args) throws IOException
   /**
    * Implements the {@code extract-model} command.
    * <p>
-   * Reads the YAML frontmatter of the skill and returns the value of the {@code model:} field,
-   * falling back to {@code "haiku"} when the field is absent.
+   * Reads the YAML frontmatter of the skill and returns the fully-qualified model identifier.
+   * The short name from the {@code model:} field is resolved via {@link ModelIdResolver}.
+   * Falls back to {@code "haiku"} (resolved to its fully-qualified ID) when the field is absent.
    *
    * @param args {@code [skill_path]}
-   * @return the model name
+   * @return the fully-qualified model identifier
    * @throws IllegalArgumentException if the argument count is wrong or the file is not found
    * @throws IOException              if the file cannot be read
    */
@@ -215,11 +221,11 @@ public String extractModel(String[] args) throws IOException
     String model = SkillDiscovery.extractField(parsed.frontmatter(), "model");
     if (model.isBlank())
     {
-      log.warn("InstructionTestRunner extract-model: no 'model:' field in frontmatter of {}; falling back to 'haiku'",
-        skillPath);
-      return "haiku";
+      throw new IllegalArgumentException(
+        "InstructionTestRunner extract-model: no 'model:' field in frontmatter of " +
+        skillPath + ". Every skill must declare a model.");
     }
-    return model;
+    return ModelIdResolver.resolve(claudeCodeVersion, model);
   }
 
   /**
@@ -525,11 +531,23 @@ public void persistArtifacts(String[] args, PrintStream out) throws IOException
 
     String timestamp = ISO_UTC.format(Instant.now());
 
+    // Resolve model_id from skill frontmatter
+    ParsedSkill parsed = parseSkill(absSkillPath);
+    String model = SkillDiscovery.extractField(parsed.frontmatter(), "model");
+    if (model.isBlank())
+    {
+      throw new IllegalArgumentException(
+        "InstructionTestRunner persist-artifacts: no 'model:' field in frontmatter of " +
+        absSkillPath + ". Every skill must declare a model.");
+    }
+    String modelId = ModelIdResolver.resolve(claudeCodeVersion, model);
+
     // Write instruction-test.json using Jackson to ensure proper escaping and formatting
     Path instructionTestJsonPath = instructionTestDir.resolve("instruction-test.json");
     JsonMapper mapper = scope.getJsonMapper();
     ObjectNode root = mapper.createObjectNode();
     root.put("session_id", sessionId);
+    root.put("model_id", modelId);
     root.put("phase", phase);
     root.put("timestamp", timestamp);
     ObjectNode skillNode = root.putObject("skill");
@@ -596,25 +614,29 @@ public void persistArtifacts(String[] args, PrintStream out) throws IOException
    * Implements the {@code init-sprt} command.
    * <p>
    * Initialises per-test-case SPRT state: fresh state for re-run cases, and carry-forward state
-   * from the prior instruction-test for unchanged cases.
+   * from the prior instruction-test for unchanged cases. When the prior instruction-test was produced
+   * by a different model (detected via the {@code model_id} field), all prior results are treated as
+   * stale and carry-forward is skipped entirely.
    *
-   * @param args {@code [rerun_tc_ids_json, prior_instruction_test_json_path, (--prior-boost)?]}
+   * @param args {@code [rerun_tc_ids_json, prior_instruction_test_json_path, current_model_id, (--prior-boost)?]}
    * @return a JSON object containing the {@code sprt_state} map
    * @throws IllegalArgumentException if arguments are missing or the prior file is not found
    * @throws IOException              if the prior file cannot be read
    */
   public String initSprt(String[] args) throws IOException
   {
     requireThat(args, "args").isNotNull();
-    if (args.length < 2)
+    if (args.length < 3)
       throw new IllegalArgumentException(
-        "InstructionTestRunner init-sprt: expected at least 2 arguments, got " + args.length + ".\n" +
-        "Usage: skill-test-runner init-sprt <rerun_tc_ids_json> <prior_instruction_test_json_path> [--prior-boost]");
+        "InstructionTestRunner init-sprt: expected at least 3 arguments, got " + args.length + ".\n" +
+        "Usage: skill-test-runner init-sprt <rerun_tc_ids_json> <prior_instruction_test_json_path> " +
+        "<current_model_id> [--prior-boost]");
 
     String rerunJson = args[0];
     String priorPath = args[1];
+    String currentModelId = args[2];
     boolean usePriorBoost = false;
-    for (int i = 2; i < args.length; ++i)
+    for (int i = 3; i < args.length; ++i)
     {
       if (args[i].equals("--prior-boost"))
         usePriorBoost = true;
@@ -641,18 +663,32 @@ public String initSprt(String[] args) throws IOException
 
     // Read prior instruction-test (if any), building a lookup map in a single pass over test_cases
     // so priorIds can be derived from the map's key set without a second traversal.
+    // When the prior model_id differs from the current model, invalidate all cached results.
     Map<String, JsonNode> priorByTestCaseId = new HashMap<>();
     if (hasPrior)
     {
       JsonNode priorRoot = mapper.readTree(Path.of(priorPath).toFile());
-      JsonNode priorTestCases = priorRoot.path("test_cases");
-      if (priorTestCases.isArray())
+      String priorModelId = priorRoot.path("model_id").asString("");
+      // Only carry forward prior results when model_id matches the current model
+      boolean modelMatches = !priorModelId.isBlank() && priorModelId.equals(currentModelId);
+      if (!priorModelId.isBlank() && !priorModelId.equals(currentModelId))
       {
-        for (JsonNode tc : priorTestCases)
+        log.warn("Model changed from {} to {}, invalidating cached SPRT results",
+          priorModelId, currentModelId);
+      }
+      else if (priorModelId.isBlank())
+        log.warn("Prior instruction-test has no model_id, invalidating cached SPRT results");
+      if (modelMatches)
+      {
+        JsonNode priorTestCases = priorRoot.path("test_cases");
+        if (priorTestCases.isArray())
         {
-          String tcId = tc.path("test_case_id").asString("");
-          if (!tcId.isBlank())
-            priorByTestCaseId.put(tcId, tc);
+          for (JsonNode tc : priorTestCases)
+          {
+            String tcId = tc.path("test_case_id").asString("");
+            if (!tcId.isBlank())
+              priorByTestCaseId.put(tcId, tc);
+          }
         }
       }
     }
@@ -937,25 +973,27 @@ public String smokeStatus(String[] args) throws IOException
    * Implements the {@code merge-results} command.
    * <p>
    * Merges new SPRT decisions with carried-forward results to produce a complete instruction-test.json
-   * summary ready for committing.
+   * summary ready for committing. The {@code model_id} parameter is included in the output to enable
+   * staleness detection on subsequent runs.
    *
-   * @param args {@code [new_sprt_state_path, prior_instruction_test_json_path, carryforward_ids_json]}
-   * @return a JSON object with overall_decision, timestamp, incremental flag, and test_cases
+   * @param args {@code [new_sprt_state_path, prior_instruction_test_json_path, carryforward_ids_json, model_id]}
+   * @return a JSON object with model_id, overall_decision, timestamp, incremental flag, and test_cases
    * @throws IllegalArgumentException if arguments are invalid or the state file is not found
    * @throws IOException              if files cannot be read
    */
   public String mergeResults(String[] args) throws IOException
   {
     requireThat(args, "args").isNotNull();
-    if (args.length != 3)
+    if (args.length != 4)
       throw new IllegalArgumentException(
-        "InstructionTestRunner merge-results: expected 3 arguments, got " + args.length + ".\n" +
+        "InstructionTestRunner merge-results: expected 4 arguments, got " + args.length + ".\n" +
         "Usage: skill-test-runner merge-results <new_sprt_state_path> " +
-        "<prior_instruction_test_json_path> <carryforward_ids_json>");
+        "<prior_instruction_test_json_path> <carryforward_ids_json> <model_id>");
 
     Path statePath = Path.of(args[0]);
     String priorInstructionTestPath = args[1];
     String carryforwardIdsJson = args[2];
+    String modelId = args[3];
 
     if (Files.notExists(statePath))
       throw new IllegalArgumentException(
@@ -1062,6 +1100,7 @@ else if (decision.equals("INCONCLUSIVE") && !overallDecision.equals("REJECT"))
 
     String timestamp = ISO_UTC.format(Instant.now());
     ObjectNode result = mapper.createObjectNode();
+    result.put("model_id", modelId);
     result.put("timestamp", timestamp);
     result.put("overall_decision", overallDecision);
     result.put("incremental", true);
@@ -1359,6 +1398,7 @@ public static void run(JvmScope scope, String[] args, PrintStream out) throws IO
     requireThat(scope, "scope").isNotNull();
     requireThat(args, "args").isNotNull();
     requireThat(out, "out").isNotNull();
-    new InstructionTestRunner(scope).run(args, out);
+    String claudeCodeVersion = ModelIdResolver.detectClaudeCodeVersion();
+    new InstructionTestRunner(scope, claudeCodeVersion).run(args, out);
   }
 }