[AI-1899] Parse retain_fact object shape + carry applies_to_* on the judge-fact wire

[realtonyyoung]

AI-1899 (kcap-cli) — parse the widened retain_fact contract

The daemon eval runner's judge may now return retain_fact as either the plain string (unchanged) or an object {"fact": "...", "applies_to_vendors": [...], "applies_to_session_kinds": [...]} declaring which vendors / session kinds a fact is specific to. This is the kcap-cli half; the kcap-server prompt widening + payload-tolerance ship first.

Changes

• EvalService.ExtractRetainFact returns a RetainedFact struct (fact + optional AppliesToVendors/AppliesToSessionKinds), parsing both shapes:
  • string → fact only (no applicability);
  • object → fact (required; missing/empty ⇒ no fact, never throws) + each axis read as an array of non-empty strings, or null when absent/empty/malformed (a non-string element drops the whole axis, keeping the fact and the other axis — matching the server's whole-axis-discard).
• Verdict JSON schema widened: retain_fact is now string | object | null.
• JudgeFactPayload carries the two optional arrays (omitted from the wire when null, so older servers ignore them).

Rollout safety

Server-first: an older server tolerates the extra JSON fields, and an older CLI already degrades an object-shaped retain_fact to no fact (its parse returns null for non-string), so no combination breaks.

Tests

EvalServiceTests ExtractRetainFact suite: 14 cases (string shape has no applicability; object both-axes / one-axis / empty-array-axis→null / malformed-axis-drops-that-axis / object-without-fact→null; plus the existing null/empty/fence/malformed cases).

[linear-code]

AI-1899

[qodo-code-review]

PR Summary by Qodo

Parse widened retain_fact contract and forward applicability on judge-fact posts

✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

AI Description

• Accept retain_fact as either string or object with vendor/session-kind applicability.
• Forward optional applies_to_* arrays when posting retained facts to the server.
• Expand unit coverage for all new retain_fact parsing edge cases.

Diagram

graph TD
  A["Eval runner"] --> B["Judge JSON"] --> C["ExtractRetainFact"] --> D["RetainedFact"] --> E["PostJudgeFactAsync"] --> F{{"kcap-server API"}}
  C --> G["Unit tests"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Typed DTO + JsonSerializer (union via custom converter)

• ➕ Moves shape-handling into a reusable converter and reduces manual JsonElement branching
• ➕ Easier to share parsing logic if other components also ingest retain_fact
• ➖ More machinery (converter registration, potential AOT/source-gen considerations)
• ➖ Harder to express the ‘drop whole axis on any invalid element’ rule cleanly

2. Schema-driven validation before extraction

• ➕ Explicit contract checks and clearer diagnostics if the judge output drifts
• ➕ Could unify with verdict parsing/validation flow
• ➖ Heavier runtime cost and more code paths for a best-effort extraction feature
• ➖ Conflicts with the stated goal of ExtractRetainFact being independent and fail-open

Recommendation: Keep the PR’s current manual parsing approach: it’s intentionally fail-open, independent from verdict parsing, and matches the server’s axis-discard behavior. A custom converter/DTO could be considered later if multiple call sites need the same union parsing, but it’s not necessary for this localized extraction + forwarding change.

Files changed (3) +121 / -23

Enhancement (2) +67 / -21

EvalService.csWiden retain_fact schema, parse object shape, and forward applicability +57/-21

Widen retain_fact schema, parse object shape, and forward applicability

• Widens the verdict JSON schema to allow retain_fact as string | object | null. Replaces string return with a RetainedFact struct and implements tolerant parsing for both shapes, including whole-axis discard on malformed arrays. Threads applicability through PostJudgeFactAsync so it can be sent to the server.

src/Capacitor.Cli.Core/Eval/EvalService.cs

Models.csExtend JudgeFactPayload with applies_to_* fields (null-omitting) +10/-0

Extend JudgeFactPayload with applies_to_* fields (null-omitting)

• Adds AppliesToVendors and AppliesToSessionKinds to the judge-fact payload model. Uses WhenWritingNull to omit fields from the wire for backward-compatible rollout.

src/Capacitor.Cli.Core/Models.cs

Tests (1) +54 / -2

EvalServiceTests.csExpand ExtractRetainFact tests for string/object shapes and malformed axes +54/-2

Expand ExtractRetainFact tests for string/object shapes and malformed axes

• Updates existing assertions to accommodate RetainedFact return type. Adds coverage for object-shaped retain_fact, missing fact behavior, empty arrays mapping to null, and malformed axes being dropped while preserving other valid data.

test/Capacitor.Cli.Tests.Unit/EvalServiceTests.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Manual ValueKind checks added ✗ Dismissed 📘 Rule violation ⚙ Maintainability

Description

New JSON parsing branches directly on JsonElement.ValueKind instead of using the project’s
JsonElementExtensions helpers, reducing consistency and hardening across JSON handling. This
violates the standardization requirement for JSON inspection/parsing.

Code

src/Capacitor.Cli.Core/Eval/EvalService.cs[R1031-1034]

+            switch (prop.ValueKind) {
+                case JsonValueKind.String: {
+                    var text = prop.GetString()?.Trim();
+                    return string.IsNullOrEmpty(text) ? null : new RetainedFact(text, null, null);

Evidence

PR Compliance ID 2 requires using JsonElementExtensions rather than direct JsonValueKind
branching. The updated ExtractRetainFact and ReadStringArrayOrNull implementations add explicit
ValueKind checks (switch (prop.ValueKind), arr.ValueKind != JsonValueKind.Array,
item.ValueKind != JsonValueKind.String) instead of the available helper APIs defined in
JsonElementExtensions.

CLAUDE.md: Use JsonElementExtensions instead of manual JsonValueKind checks
src/Capacitor.Cli.Core/Eval/EvalService.cs[1031-1040]
src/Capacitor.Cli.Core/Eval/EvalService.cs[1059-1064]
src/Capacitor.Cli.Core/JsonElementExtensions.cs[5-18]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New/modified code is checking `JsonElement.ValueKind` directly (e.g., `switch (prop.ValueKind)`, `arr.ValueKind != JsonValueKind.Array`) instead of using the project-provided `JsonElementExtensions` helpers.

## Issue Context
The repo has shared `JsonElementExtensions` (e.g., `IsString`, `IsObject`, `IsArray`, `Str(...)`, `Arr(...)`) intended to standardize and harden JSON parsing.

## Fix Focus Areas
- src/Capacitor.Cli.Core/Eval/EvalService.cs[1031-1040]
- src/Capacitor.Cli.Core/Eval/EvalService.cs[1059-1064]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Unbounded applicability arrays ✓ Resolved 🐞 Bug ☼ Reliability ⭐ New

Description

ReadStringArrayOrNull parses applies_to_* arrays without any length limits, and PostJudgeFactAsync
forwards the full arrays to the server when present. Because VerdictJsonSchema also lacks maxItems
bounds for these arrays, a judge can emit very large applicability lists that bloat structured
output and outgoing POST payloads, increasing latency and risk of request-size failures.

Code

src/Capacitor.Cli.Core/Eval/EvalService.cs[R1059-1064]

+        if (!prop.TryGetProperty(name, out var arr) || arr.ValueKind != JsonValueKind.Array) return null;
+
+        var list = new List<string>();
+        foreach (var item in arr.EnumerateArray()) {
+            if (item.ValueKind != JsonValueKind.String) return null; // malformed axis → drop it entirely.
+            var v = item.GetString();

Evidence

The schema defines applicability arrays with only type: array/items: string (no maxItems), the
parser enumerates all elements with no cap, and the POST payload includes these arrays when
non-null—so large judge outputs can directly translate into larger structured_output and outgoing
request bodies.

src/Capacitor.Cli.Core/Eval/EvalService.cs[27-43]
src/Capacitor.Cli.Core/Eval/EvalService.cs[1055-1069]
src/Capacitor.Cli.Core/Eval/EvalService.cs[1302-1320]
src/Capacitor.Cli.Core/Models.cs[501-520]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`applies_to_vendors` / `applies_to_session_kinds` are currently unbounded in both the JSON schema (StructuredOutput constraint) and runtime parsing. This allows unexpectedly large applicability arrays to be accepted and then forwarded to the `/judge-facts` endpoint, which can inflate model output and produce oversized HTTP requests.

### Issue Context
- `VerdictJsonSchema` now allows an object-shaped `retain_fact` with applicability arrays but doesn’t constrain their lengths.
- `ReadStringArrayOrNull` enumerates the full JSON array and returns all non-empty strings.
- `PostJudgeFactAsync` serializes whatever arrays were parsed into `JudgeFactPayload`.

### Fix Focus Areas
- src/Capacitor.Cli.Core/Eval/EvalService.cs[27-43]
- src/Capacitor.Cli.Core/Eval/EvalService.cs[1055-1069]
- src/Capacitor.Cli.Core/Eval/EvalService.cs[1302-1320]

### Concrete fix
1. Update `VerdictJsonSchema` so `retain_fact.applies_to_vendors` and `retain_fact.applies_to_session_kinds` include a reasonable `maxItems` (matching the prompt’s documented caps, if any), and ideally add `additionalProperties:false` for the `retain_fact` object as well.
2. Add a defensive runtime cap in `ReadStringArrayOrNull` (e.g., if `arr.GetArrayLength()` exceeds the max, either truncate to max or drop the axis to null) so even non-schema-constrained responses can’t create oversized payloads.
3. (Optional) Consider a max length per string value to avoid very large identifiers being forwarded.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Schema permits factless object ✓ Resolved 🐞 Bug ≡ Correctness

Description

VerdictJsonSchema allows retain_fact to be an object without requiring a "fact" property, so the
judge can emit an applicability-only object that passes schema validation but is then ignored by
ExtractRetainFact (returns null), silently preventing fact retention.

Code

src/Capacitor.Cli.Core/Eval/EvalService.cs[28]

+        {"type":"object","properties":{"category":{"type":"string"},"question_id":{"type":"string"},"score":{"type":"integer","minimum":1,"maximum":5},"verdict":{"type":"string","enum":["pass","warn","fail"]},"finding":{"type":"string"},"evidence":{"type":["string","null"]},"recommendation":{"type":["string","null"]},"retain_fact":{"type":["string","object","null"],"properties":{"fact":{"type":"string"},"applies_to_vendors":{"type":"array","items":{"type":"string"}},"applies_to_session_kinds":{"type":"array","items":{"type":"string"}}}}},"required":["category","question_id","score","verdict","finding","evidence","recommendation","retain_fact"],"additionalProperties":false}

Evidence

The schema is used to constrain/validate the judge output, but it does not require retain_fact.fact
when retain_fact is an object; meanwhile the extractor explicitly treats object-without-fact as “no
fact”. This mismatch makes an applicability-only object schema-valid but operationally ignored.

src/Capacitor.Cli.Core/Eval/EvalService.cs[27-29]
src/Capacitor.Cli.Core/Eval/EvalService.cs[499-509]
src/Capacitor.Cli.Core/Eval/EvalService.cs[518-529]
src/Capacitor.Cli.Core/Eval/EvalService.cs[1037-1041]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`VerdictJsonSchema` currently allows `retain_fact` to be an `object` with no required properties. That means `{}` or `{ "applies_to_vendors": ["codex"] }` is schema-valid, but `ExtractRetainFact` intentionally returns `null` for an object with no usable `fact`, so the system silently drops the retained fact.

## Issue Context
This schema is passed to `ClaudeCliRunner.RunAsync` as `jsonSchema`, so it shapes and validates the judge output. Tightening the schema reduces silent no-op outputs and aligns validation with `ExtractRetainFact` behavior.

## Fix Focus Areas
- src/Capacitor.Cli.Core/Eval/EvalService.cs[27-29]

### Suggested change
Update the `retain_fact` sub-schema to require `fact` when the value is an object (and ideally disallow extra nested fields). For example, refactor to a `oneOf`:
- `{"type":"string"}`
- `{"type":"object","properties":{...},"required":["fact"],"additionalProperties":false}`
- `{"type":"null"}`

Optionally also constrain axis arrays to non-empty strings (e.g., `items: {"type":"string","minLength":1}` and/or `minItems: 1`) to better match the parser’s “empty array => null/omit” semantics.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Applicability not trimmed ✓ Resolved 🐞 Bug ≡ Correctness

Description

ReadStringArrayOrNull preserves leading/trailing whitespace in applies_to_* values, so identifiers
like "codex " are forwarded onto the wire and may not match server-side exact filters for
vendor/session kind.

Code

src/Capacitor.Cli.Core/Eval/EvalService.cs[R1063-1066]

+            if (item.ValueKind != JsonValueKind.String) return null; // malformed axis → drop it entirely.
+            var v = item.GetString();
+            if (!string.IsNullOrWhiteSpace(v)) list.Add(v);
+        }

Evidence

Fact text is trimmed during extraction, but axis values are not; the untrimmed arrays are then
placed directly on JudgeFactPayload for JSON serialization, so whitespace can survive onto the wire.

src/Capacitor.Cli.Core/Eval/EvalService.cs[1033-1045]
src/Capacitor.Cli.Core/Eval/EvalService.cs[1058-1066]
src/Capacitor.Cli.Core/Eval/EvalService.cs[1314-1320]
src/Capacitor.Cli.Core/Models.cs[501-520]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ReadStringArrayOrNull` filters out whitespace-only entries but does not `Trim()` string values before adding them to the applicability arrays. This can preserve leading/trailing spaces and produce identifiers that won’t match expected vendor/session-kind strings.

## Issue Context
The main fact text is already trimmed, so normalizing applicability strings the same way is consistent and reduces accidental mismatches.

## Fix Focus Areas
- src/Capacitor.Cli.Core/Eval/EvalService.cs[1058-1066]
- test/Capacitor.Cli.Tests.Unit/EvalServiceTests.cs[456-506]

### Suggested change
In `ReadStringArrayOrNull`, change to something like:
```csharp
var v = item.GetString()?.Trim();
if (!string.IsNullOrEmpty(v)) list.Add(v);
```

Add a unit test demonstrating that values like `" codex "` become `"codex"`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous review results

Review updated until commit b5edb92

Results up to commit 527dc56 ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Manual ValueKind checks added ✗ Dismissed 📘 Rule violation ⚙ Maintainability

Description

New JSON parsing branches directly on JsonElement.ValueKind instead of using the project’s
JsonElementExtensions helpers, reducing consistency and hardening across JSON handling. This
violates the standardization requirement for JSON inspection/parsing.

Code

src/Capacitor.Cli.Core/Eval/EvalService.cs[R1031-1034]

+            switch (prop.ValueKind) {
+                case JsonValueKind.String: {
+                    var text = prop.GetString()?.Trim();
+                    return string.IsNullOrEmpty(text) ? null : new RetainedFact(text, null, null);

Evidence

PR Compliance ID 2 requires using JsonElementExtensions rather than direct JsonValueKind
branching. The updated ExtractRetainFact and ReadStringArrayOrNull implementations add explicit
ValueKind checks (switch (prop.ValueKind), arr.ValueKind != JsonValueKind.Array,
item.ValueKind != JsonValueKind.String) instead of the available helper APIs defined in
JsonElementExtensions.

CLAUDE.md: Use JsonElementExtensions instead of manual JsonValueKind checks
src/Capacitor.Cli.Core/Eval/EvalService.cs[1031-1040]
src/Capacitor.Cli.Core/Eval/EvalService.cs[1059-1064]
src/Capacitor.Cli.Core/JsonElementExtensions.cs[5-18]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New/modified code is checking `JsonElement.ValueKind` directly (e.g., `switch (prop.ValueKind)`, `arr.ValueKind != JsonValueKind.Array`) instead of using the project-provided `JsonElementExtensions` helpers.

## Issue Context
The repo has shared `JsonElementExtensions` (e.g., `IsString`, `IsObject`, `IsArray`, `Str(...)`, `Arr(...)`) intended to standardize and harden JSON parsing.

## Fix Focus Areas
- src/Capacitor.Cli.Core/Eval/EvalService.cs[1031-1040]
- src/Capacitor.Cli.Core/Eval/EvalService.cs[1059-1064]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Schema permits factless object ✓ Resolved 🐞 Bug ≡ Correctness

Description

VerdictJsonSchema allows retain_fact to be an object without requiring a "fact" property, so the
judge can emit an applicability-only object that passes schema validation but is then ignored by
ExtractRetainFact (returns null), silently preventing fact retention.

Code

src/Capacitor.Cli.Core/Eval/EvalService.cs[28]

+        {"type":"object","properties":{"category":{"type":"string"},"question_id":{"type":"string"},"score":{"type":"integer","minimum":1,"maximum":5},"verdict":{"type":"string","enum":["pass","warn","fail"]},"finding":{"type":"string"},"evidence":{"type":["string","null"]},"recommendation":{"type":["string","null"]},"retain_fact":{"type":["string","object","null"],"properties":{"fact":{"type":"string"},"applies_to_vendors":{"type":"array","items":{"type":"string"}},"applies_to_session_kinds":{"type":"array","items":{"type":"string"}}}}},"required":["category","question_id","score","verdict","finding","evidence","recommendation","retain_fact"],"additionalProperties":false}

Evidence

The schema is used to constrain/validate the judge output, but it does not require retain_fact.fact
when retain_fact is an object; meanwhile the extractor explicitly treats object-without-fact as “no
fact”. This mismatch makes an applicability-only object schema-valid but operationally ignored.

src/Capacitor.Cli.Core/Eval/EvalService.cs[27-29]
src/Capacitor.Cli.Core/Eval/EvalService.cs[499-509]
src/Capacitor.Cli.Core/Eval/EvalService.cs[518-529]
src/Capacitor.Cli.Core/Eval/EvalService.cs[1037-1041]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`VerdictJsonSchema` currently allows `retain_fact` to be an `object` with no required properties. That means `{}` or `{ "applies_to_vendors": ["codex"] }` is schema-valid, but `ExtractRetainFact` intentionally returns `null` for an object with no usable `fact`, so the system silently drops the retained fact.

## Issue Context
This schema is passed to `ClaudeCliRunner.RunAsync` as `jsonSchema`, so it shapes and validates the judge output. Tightening the schema reduces silent no-op outputs and aligns validation with `ExtractRetainFact` behavior.

## Fix Focus Areas
- src/Capacitor.Cli.Core/Eval/EvalService.cs[27-29]

### Suggested change
Update the `retain_fact` sub-schema to require `fact` when the value is an object (and ideally disallow extra nested fields). For example, refactor to a `oneOf`:
- `{"type":"string"}`
- `{"type":"object","properties":{...},"required":["fact"],"additionalProperties":false}`
- `{"type":"null"}`

Optionally also constrain axis arrays to non-empty strings (e.g., `items: {"type":"string","minLength":1}` and/or `minItems: 1`) to better match the parser’s “empty array => null/omit” semantics.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Applicability not trimmed ✓ Resolved 🐞 Bug ≡ Correctness

Description

ReadStringArrayOrNull preserves leading/trailing whitespace in applies_to_* values, so identifiers
like "codex " are forwarded onto the wire and may not match server-side exact filters for
vendor/session kind.

Code

src/Capacitor.Cli.Core/Eval/EvalService.cs[R1063-1066]

+            if (item.ValueKind != JsonValueKind.String) return null; // malformed axis → drop it entirely.
+            var v = item.GetString();
+            if (!string.IsNullOrWhiteSpace(v)) list.Add(v);
+        }

Evidence

Fact text is trimmed during extraction, but axis values are not; the untrimmed arrays are then
placed directly on JudgeFactPayload for JSON serialization, so whitespace can survive onto the wire.

src/Capacitor.Cli.Core/Eval/EvalService.cs[1033-1045]
src/Capacitor.Cli.Core/Eval/EvalService.cs[1058-1066]
src/Capacitor.Cli.Core/Eval/EvalService.cs[1314-1320]
src/Capacitor.Cli.Core/Models.cs[501-520]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ReadStringArrayOrNull` filters out whitespace-only entries but does not `Trim()` string values before adding them to the applicability arrays. This can preserve leading/trailing spaces and produce identifiers that won’t match expected vendor/session-kind strings.

## Issue Context
The main fact text is already trimmed, so normalizing applicability strings the same way is consistent and reduces accidental mismatches.

## Fix Focus Areas
- src/Capacitor.Cli.Core/Eval/EvalService.cs[1058-1066]
- test/Capacitor.Cli.Tests.Unit/EvalServiceTests.cs[456-506]

### Suggested change
In `ReadStringArrayOrNull`, change to something like:
```csharp
var v = item.GetString()?.Trim();
if (!string.IsNullOrEmpty(v)) list.Add(v);
```

Add a unit test demonstrating that values like `" codex "` become `"codex"`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[realtonyyoung]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 527dc56