Skip to content

[AI-2748] Expose redaction members and kcap eval --baseline-out - #1054

Merged
realtonyyoung merged 5 commits into
mainfrom
tonyyoung/ai-2748-eval-baseline-out
Sep 20, 2026
Merged

realtonyyoung merged 5 commits into
mainfrom
tonyyoung/ai-2748-eval-baseline-out

Conversation

@realtonyyoung

Copy link
Copy Markdown
Collaborator

AI-2748 (no kcap-cli GitHub issue — tracked in Linear)

What & why

Two additive changes the server's eval-baseline harness depends on. First, SecretRedactor.IsSecretKey/RedactValue become public so the server's fixture redactor can reuse the CLI's production secret vocabulary, and EnvVarSecretRegex is anchored so redaction scales to multi-megabyte values without a quadratic blowup. Second, kcap eval --baseline-out <file> writes one machine-readable JSON record per question (route, usage, latency, runner invocations), plus the retrospective and run totals, so a baseline run captures per-question resource use. kcap eval without the flag is unchanged.

Where to look

The EnvVarSecretRegex anchoring is a change to a shipping redaction regex and must stay match-equivalent — a keyword that starts right after a digit (OAUTH2_TOKEN=) must still redact; pinning tests cover that and a 6 MB value. --baseline-out serializes through the source-gen CapacitorJsonContext for AOT.

Verification

dotnet build 0 errors; AOT Release publish no IL3050/IL2026. Focused tests green: Cli.Core eval namespace 142; SecretRedactorTests 138 (incl. digit-preceded keywords and the 6 MB value); EvalBaselineOutArgTests 4.

🤖 Generated with Claude Code

RedactValue is reused to scan multi-megabyte event bodies value-by-value,
outside RedactLine's 64K line cap. The unanchored [A-Z_]* prefix rescanned a
long delimiter-free run once per character (O(n^2)); the lookbehind starts the
match only at a name boundary, finding the same matches.
The name-boundary lookbehind must exclude only [A-Za-z_], not digits: [A-Z_]
stops at a digit, so a keyword run begins right after one, and the digit-
inclusive form skipped that start, leaving OAUTH2_TOKEN=, S3_SECRET_KEY= and
similar values unredacted. Still O(n) on a delimiter-free run.
…rver

The observer's question and retrospective callbacks now carry per-call
token usage, route and wall time; a --baseline-out decorator collects
them into a machine-readable file. kcap eval without the flag is
unchanged — same observer, no routing or judging change.
@linear-code

linear-code Bot commented Sep 19, 2026

Copy link
Copy Markdown

AI-2748

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Add eval baseline output and expose scalable secret redaction APIs

✨ Enhancement 🐞 Bug fix 🕐 40+ Minutes

Grey Divider

AI Description

• Adds optional eval baseline JSON with per-question usage, routing, latency, and totals.
• Exposes reusable secret-key and value redaction APIs for server fixtures.
• Anchors environment-secret scanning to preserve coverage and scale linearly on large values.
Diagram

graph TD
  User["CLI User"] -->|flag| Command["Eval Command"] -->|runs| Service["Eval Service"] -->|metrics| Observer["Baseline Observer"] -->|terminal| Writer["JSON Writer"] -->|writes| File[("Baseline File")]
  Fixture["Server Fixture"] -->|reuses| Redactor["Secret Redactor"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Write baselines inside EvalService
  • ➕ Avoids expanding the observer callback signatures.
  • ➕ Keeps metric collection close to judge execution.
  • ➖ Couples the reusable eval service to filesystem output.
  • ➖ Makes optional output behavior harder to compose or replace.
  • ➖ Mixes evaluation orchestration with persistence concerns.

Recommendation: Keep the observer-decorator approach. It preserves unchanged behavior when the flag is absent, supports partial-run output through terminal callbacks, and isolates persistence from evaluation orchestration despite the observer contract expansion.

Files changed (26) +515 / -46

Enhancement (11) +244 / -24
BaselineObserver.csCollect and persist baseline evaluation metrics +97/-0

Collect and persist baseline evaluation metrics

• Adds an observer decorator that records question and retrospective metrics while forwarding all events. It writes completed or partial baselines on terminal callbacks and reports filesystem failures without masking eval results.

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

BaselineOutput.csDefine the run-level baseline JSON contract +16/-0

Define the run-level baseline JSON contract

• Introduces the snake_case cross-repository contract containing run identity, question records, retrospective data, totals, and elapsed time.

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

BaselineOutputWriter.csAdd AOT-safe baseline JSON serialization +12/-0

Add AOT-safe baseline JSON serialization

• Serializes baseline output through the source-generated CapacitorJsonContext and writes it to the requested path.

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

BaselineQuestionOutput.csDefine per-question baseline records +15/-0

Define per-question baseline records

• Adds the JSON contract for question identity, route, token usage, invocation count, and elapsed time.

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

BaselineRetrospectiveOutput.csDefine retrospective baseline records +12/-0

Define retrospective baseline records

• Adds the JSON contract for retrospective usage, invocation count, and elapsed time.

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

EvalService.csReport route, usage, and timing through eval observers +13/-7

Report route, usage, and timing through eval observers

• Measures question and retrospective execution and sends structured usage, route, latency, and invocation metadata through observer callbacks. The safe observer wrapper is updated to forward the expanded contract.

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

EvalUsage.csModel and aggregate judge resource usage +46/-0

Model and aggregate judge resource usage

• Introduces an AOT-serializable usage record populated from Claude results. It sums token categories and preserves null cost when no invocation reports a cost.

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

IEvalObserver.csExpand eval completion callback metadata +5/-3

Expand eval completion callback metadata

• Extends question and retrospective completion callbacks with structured usage and elapsed time, plus question route and runner invocation count.

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

Models.csRegister baseline contracts for source generation +4/-0

Register baseline contracts for source generation

• Adds baseline output and usage types to CapacitorJsonContext for reflection-free AOT serialization.

src/Capacitor.Cli.Core/Models.cs

EvalCommand.csInstall baseline collection for requested eval runs +14/-5

Install baseline collection for requested eval runs

• Centralizes value-bearing eval flags and wraps the console observer when --baseline-out is supplied. Existing eval behavior remains unchanged without the option.

src/Capacitor.Cli/Commands/EvalCommand.cs

Program.csParse and validate --baseline-out +10/-9

Parse and validate --baseline-out

• Recognizes the new value-bearing option, validates missing values, updates usage text, and passes the path into EvalCommand.

src/Capacitor.Cli/Program.cs

Refactor (1) +2 / -2
EvalRunner.csAdapt daemon observer to enriched eval callbacks +2/-2

Adapt daemon observer to enriched eval callbacks

• Updates daemon observer methods to accept the expanded question and retrospective metadata while preserving existing relay behavior.

src/Capacitor.Cli.Daemon/Services/EvalRunner.cs

Tests (11) +249 / -16
BaselineOutputWriterTests.csVerify the baseline JSON contract and totals +138/-0

Verify the baseline JSON contract and totals

• Tests snake_case serialization, question and retrospective fields, usage aggregation, costs, cache tokens, and null retrospectives.

test/Capacitor.Cli.Core.Tests.Unit/Eval/BaselineOutputWriterTests.cs

EvalCatalogClientTests.csUpdate catalog test observer signatures +2/-2

Update catalog test observer signatures

• Adapts the test observer to the enriched evaluation callback contract.

test/Capacitor.Cli.Core.Tests.Unit/Eval/EvalCatalogClientTests.cs

EvalQuestionCatalogClientTests.csUpdate question catalog test observer signatures +2/-2

Update question catalog test observer signatures

• Updates the test double to accept structured usage, routing, invocation, and timing metadata.

test/Capacitor.Cli.Core.Tests.Unit/Eval/EvalQuestionCatalogClientTests.cs

EvalDaemonObserverProtocol1Tests.csExercise daemon relays with enriched metrics +3/-2

Exercise daemon relays with enriched metrics

• Supplies the new callback metadata while retaining assertions for assessed and unassessed protocol behavior.

test/Capacitor.Cli.Daemon.Tests.Unit/Services/EvalDaemonObserverProtocol1Tests.cs

EvalCatalogFetchTests.csAdapt catalog integration observer callbacks +2/-2

Adapt catalog integration observer callbacks

• Updates the integration test observer for the expanded eval completion signatures.

test/Capacitor.Cli.Tests.Integration/EvalCatalogFetchTests.cs

EvalQuestionsAliasRawTextTests.csAdapt alias integration observer callbacks +2/-2

Adapt alias integration observer callbacks

• Updates the raw-text question alias test observer for structured usage and timing metadata.

test/Capacitor.Cli.Tests.Integration/EvalQuestionsAliasRawTextTests.cs

EvalRunnerV2PostTests.csAdapt V2 eval runner test callbacks +2/-2

Adapt V2 eval runner test callbacks

• Updates the V2 integration observer to implement the enriched completion contract.

test/Capacitor.Cli.Tests.Integration/EvalRunnerV2PostTests.cs

EvalRunnerV3PostTests.csAdapt V3 eval runner test callbacks +2/-2

Adapt V3 eval runner test callbacks

• Updates the V3 integration observer to implement the enriched completion contract.

test/Capacitor.Cli.Tests.Integration/EvalRunnerV3PostTests.cs

EvalRunnerV4PostTests.csAdapt V4 eval runner test callbacks +2/-2

Adapt V4 eval runner test callbacks

• Updates the V4 integration observer to implement the enriched completion contract.

test/Capacitor.Cli.Tests.Integration/EvalRunnerV4PostTests.cs

EvalBaselineOutArgTests.csProtect baseline option session parsing +50/-0

Protect baseline option session parsing

• Verifies the production value-flag list prevents output paths from being mistaken for session IDs across common argument orderings.

test/Capacitor.Cli.Tests.Unit/Commands/EvalBaselineOutArgTests.cs

SecretRedactorTests.csPin public redaction behavior and regex scalability +44/-0

Pin public redaction behavior and regex scalability

• Tests the exposed APIs, digit-preceded secret names, and linear-time scanning of a multi-megabyte value. These cases protect both vocabulary reuse and the optimized regex boundary semantics.

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

Documentation (2) +5 / -0
README.mdDocument the eval baseline output option +1/-0

Document the eval baseline output option

• Adds a command example showing how to write per-question evaluation metrics to JSON.

README.md

help-eval.txtDescribe --baseline-out in eval help +4/-0

Describe --baseline-out in eval help

• Documents the new output path option and the metrics included in its JSON file.

src/Capacitor.Cli.Core/Resources/help-eval.txt

Other (1) +15 / -4
SecretRedactor.csExpose redaction APIs and linearize env-var scanning +15/-4

Expose redaction APIs and linearize env-var scanning

• Makes SecretRedactor, IsSecretKey, and RedactValue public for external reuse. Anchors env-var matching at name boundaries to avoid quadratic scans while retaining secrets whose keyword follows a digit.

src/Capacitor.Cli/SecretRedactor.cs

@qodo-code-review

qodo-code-review Bot commented Sep 19, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (1) 📜 Skill insights (0)

Grey Divider


Action required

1. Server eval tests stop compiling 🔗 Cross-repo conflict ≡ Correctness
Description
IEvalObserver changes both completion callbacks to require usage and timing metadata, but
kcap-server's two test observers retain the old signatures. When the server advances its referenced
CLI submodule to this change, its evaluation test project reaches those implementations and cannot
satisfy the interface.
Code

src/Capacitor.Cli.Core/Eval/IEvalObserver.cs[46]

+    void OnQuestionCompleted(int index, int total, EvalQuestionAssessment assessment, EvalUsage usage, string route, TimeSpan elapsed, int runnerInvocations);
Relevance

●●● Strong

Public interface changes must update downstream implementations; otherwise the referenced server
test project cannot compile.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The PR changes the public observer callbacks, while kcap-server directly references the CLI Core
project and contains two implementations with incompatible completion method signatures.

src/Capacitor.Cli.Core/Eval/IEvalObserver.cs[42-46]
src/Capacitor.Cli.Core/Eval/IEvalObserver.cs[57-58]
External repo: kurrent-io/kcap-server, test/Capacitor.Server.Tests.Evals/Capacitor.Server.Tests.Evals.csproj [10-16]
External repo: kurrent-io/kcap-server, test/Capacitor.Server.Tests.Evals/CliCatalog/CliCatalogIntegrationTests.cs [70-80]
External repo: kurrent-io/kcap-server, test/Capacitor.Server.Tests.Evals/CliCatalog/AliasRawTextLiveTests.cs [59-69]

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

## Issue description
The changed `IEvalObserver` completion signatures are not implemented by kcap-server's evaluation test observers, causing its integrated test project to stop compiling when the CLI submodule is advanced.

## Fix Focus Areas
- src/Capacitor.Cli.Core/Eval/IEvalObserver.cs[42-58]
- test/Capacitor.Server.Tests.Evals/CliCatalog/CliCatalogIntegrationTests.cs[70-80]
- test/Capacitor.Server.Tests.Evals/CliCatalog/AliasRawTextLiveTests.cs[59-69]

## Recommended Fix
Coordinate a kcap-server change that updates both `TestObserver` implementations to the new question and retrospective completion signatures, including `EvalUsage`, route, elapsed time, and invocation count parameters.

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


2. Failed judges disappear from baselines ✓ Resolved 🐞 Bug ≡ Correctness
Description
BaselineObserver.OnQuestionFailed and the retrospective failure path only forward failure reasons,
while _questions and retrospective records are populated exclusively by completion callbacks. When
runner execution fails or a returned response fails ParseVerdict or ParseRetrospectiveV2 and
evaluation continues to finalization, the baseline artifact omits the affected question or
retrospective invocation along with any available route, elapsed-time, token, and cost data from its
records and totals.
Code

src/Capacitor.Cli.Core/Eval/BaselineObserver.cs[R46-47]

+    public void OnQuestionFailed(int index, int total, string category, string questionId, string reason) =>
+        inner.OnQuestionFailed(index, total, category, questionId, reason);
Relevance

●●● Strong

Baseline correctness requires preserving failed invocations and their usage when evaluation
continues; this is a direct data-loss bug.

PR-#134

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The observer appends baseline entries only in successful completion callbacks and merely delegates
failures, while EvalService continues after runner and parsing failures. Both question and
retrospective paths can receive a non-null model result with usage metadata, fail to parse it, and
invoke only the failure callback; because the baseline writer totals only collected entries, these
branches discard resource accounting, and runner failures also leave the promised per-question
output incomplete.

src/Capacitor.Cli.Core/Eval/BaselineObserver.cs[35-62]
src/Capacitor.Cli.Core/Eval/EvalService.cs[575-605]
src/Capacitor.Cli.Core/Eval/EvalService.cs[1594-1622]
src/Capacitor.Cli.Core/Eval/BaselineObserver.cs[74-87]
src/Capacitor.Cli.Core/Eval/BaselineObserver.cs[35-47]
src/Capacitor.Cli.Core/Eval/EvalService.cs[578-604]
src/Capacitor.Cli.Core/Resources/help-eval.txt[27-30]

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

## Issue description
Baseline output promises one record per question, but `BaselineObserver` records only successful completion callbacks and drops failed question and retrospective invocations. Preserve records and available model usage, route, elapsed time, cost, and invocation counts when a returned response cannot be parsed, and represent runner failures even when no result is available.

## Fix Focus Areas
- src/Capacitor.Cli.Core/Eval/BaselineObserver.cs[35-62]
- src/Capacitor.Cli.Core/Eval/IEvalObserver.cs[42-61]
- src/Capacitor.Cli.Core/Eval/EvalService.cs[575-605]
- src/Capacitor.Cli.Core/Eval/EvalService.cs[1594-1622]
- src/Capacitor.Cli.Core/Eval/BaselineQuestionOutput.cs[9-14]

## Recommended Fix
Create a baseline entry for every started or failed question rather than only completed ones, and do the equivalent for failed retrospectives. Extend the failed-question and failed-retrospective observer paths to carry any usage, route, elapsed-time, and cost metadata available when a runner returned a result but parsing failed; update the output contract with an explicit completion status and optional failure details, and use documented defaults when the runner produced no result. Ensure `BaselineObserver` serializes these invocation records and includes their usage in totals. Add tests for unparsable question and retrospective responses with nonzero token usage, as well as resultless runner failures.

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


3. Missing output files still report success ✓ Resolved 🐞 Bug ≡ Correctness
Description
WriteBaseline converts IOException and UnauthorizedAccessException into an informational
callback without exposing the failure to HandleEval. When the destination has a missing parent,
insufficient permissions, or another I/O failure, a successful evaluation still returns exit code
zero despite the requested artifact not existing.
Code

src/Capacitor.Cli.Core/Eval/BaselineObserver.cs[R91-94]

+        } catch (IOException ex) {
+            inner.OnInfo($"failed to write baseline output to '{path}': {ex.Message}");
+        } catch (UnauthorizedAccessException ex) {
+            inner.OnInfo($"failed to write baseline output to '{path}': {ex.Message}");
Relevance

●● Moderate

Write failures should affect command status, but repository precedent on error-reporting semantics
is mixed and context-specific.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The writer directly calls File.WriteAllText, the observer catches common write failures and only
logs them, and the command returns zero whenever the evaluation result itself is non-null. There is
no path by which baseline-write failure changes command success.

src/Capacitor.Cli.Core/Eval/BaselineObserver.cs[64-95]
src/Capacitor.Cli.Core/Eval/BaselineOutputWriter.cs[8-10]
src/Capacitor.Cli/Commands/EvalCommand.cs[55-64]

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

## Issue description
Baseline file creation is an explicitly requested command result, but write failures are reduced to informational output and do not affect the process exit code. Automation can therefore accept a baseline run as successful even though no baseline artifact was produced.

## Fix Focus Areas
- src/Capacitor.Cli.Core/Eval/BaselineObserver.cs[64-95]
- src/Capacitor.Cli.Core/Eval/BaselineOutputWriter.cs[8-10]
- src/Capacitor.Cli/Commands/EvalCommand.cs[55-64]

## Recommended Fix
Track whether the baseline write succeeded and make `HandleEval` return a nonzero status after an otherwise successful evaluation when it failed. Handle invalid-path exceptions as well as I/O and authorization failures, while still forwarding the observer's terminal callback.

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



Remediation recommended

4. One JSON null check bypasses its helper ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
Omitted_retrospective_serializes_as_null compares ValueKind directly with JsonValueKind.Null
instead of using the available IsNull extension. This new assertion establishes the disallowed
direct comparison pattern in the baseline serialization tests.
Code

test/Capacitor.Cli.Core.Tests.Unit/Eval/BaselineOutputWriterTests.cs[136]

+        await Assert.That(doc.RootElement.GetProperty("retrospective").ValueKind).IsEqualTo(JsonValueKind.Null);
Relevance

●●● Strong

Recent precedent accepted replacing direct JsonValueKind comparisons with shared JSON helpers.

PR-#1016

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Compliance rule 2270023 prohibits new direct JsonElement.ValueKind comparisons when an equivalent
extension exists. The added assertion directly compares the retrospective element's kind with
JsonValueKind.Null.

Rule 2270023: Use JsonElementExtensions helpers instead of direct JsonValueKind comparisons
test/Capacitor.Cli.Core.Tests.Unit/Eval/BaselineOutputWriterTests.cs[135-136]

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

## Issue description
The baseline serialization test directly compares a `JsonElement.ValueKind` with `JsonValueKind.Null` despite the available `JsonElementExtensions.IsNull` helper.

## Fix Focus Areas
- test/Capacitor.Cli.Core.Tests.Unit/Eval/BaselineOutputWriterTests.cs[136-136]

## Recommended Fix
Replace the direct `ValueKind` assertion with an assertion on `doc.RootElement.GetProperty("retrospective").IsNull`.

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


5. Two tests manually manage temp folders ✓ Resolved 📘 Rule violation ▣ Testability
Description
Writes_documented_snake_case_keys_with_totals_equal_to_the_sum and
Omitted_retrospective_serializes_as_null each call new TempDir() locally rather than receiving a
public [TempDir] property on BaselineOutputWriterTests. Both test paths therefore manage their
own fixture lifecycle instead of using TUnit's injected directory lifecycle.
Code

test/Capacitor.Cli.Core.Tests.Unit/Eval/BaselineOutputWriterTests.cs[R65-66]

+        using var tmp  = new TempDir();
+        var       path = tmp.PathTo("baseline.json");
Relevance

●●● Strong

The finding directly matches the repository’s explicit TempDir rule and is a trivial test-fixture
migration.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Compliance rule 2808173 explicitly disallows new TempDir() calls in test classes and requires an
injected [TempDir] property. Both newly added test methods manually instantiate and dispose
temporary-directory fixtures.

Rule 2808173: Use injected [TempDir] public required property in test classes instead of manual fields
test/Capacitor.Cli.Core.Tests.Unit/Eval/BaselineOutputWriterTests.cs[65-66]
test/Capacitor.Cli.Core.Tests.Unit/Eval/BaselineOutputWriterTests.cs[130-131]

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

## Issue description
The new test class manually creates two `TempDir` instances, while test classes must use a framework-injected public required `[TempDir]` property.

## Fix Focus Areas
- test/Capacitor.Cli.Core.Tests.Unit/Eval/BaselineOutputWriterTests.cs[6-6]
- test/Capacitor.Cli.Core.Tests.Unit/Eval/BaselineOutputWriterTests.cs[65-66]
- test/Capacitor.Cli.Core.Tests.Unit/Eval/BaselineOutputWriterTests.cs[130-131]

## Recommended Fix
Add one `[TempDir] public required TempDir Tmp { get; init; }` property to the test class, remove both `using var tmp = new TempDir()` declarations, and construct each output path through `Tmp.PathTo(...)`.

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


6. Runner count docs become stale ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
IEvalObserver.OnQuestionCompleted documents runnerInvocations as 1 today, which encodes a
time-dependent implementation count rather than the parameter's durable contract. The wording turns
false as soon as retry behavior yields more than one runner call, even though the surrounding
sentence already explains that retries are included.
Code

src/Capacitor.Cli.Core/Eval/IEvalObserver.cs[R43-45]

+    /// unassessed outcome, which carries no score. <paramref name="route"/> is <c>text</c> or
+    /// <c>tools</c>; <paramref name="runnerInvocations"/> counts the runner calls behind this
+    /// result (retries included — 1 today).</summary>
Relevance

●●● Strong

Recent precedent accepts replacing direct ValueKind checks with durable helpers; removing “today” is
an obvious documentation fix.

PR-#1016

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Compliance rule 2897915 prohibits time-sensitive qualifiers in comments. The added interface
documentation explicitly qualifies the invocation count with today, rather than documenting only
the stable counting semantics.

Rule 2897915: Avoid time-sensitive or process-reference metadata in code comments
src/Capacitor.Cli.Core/Eval/IEvalObserver.cs[43-45]

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

## Issue description
The new observer documentation says the runner invocation count is `1 today`, making the comment dependent on the current implementation and likely to become stale.

## Fix Focus Areas
- src/Capacitor.Cli.Core/Eval/IEvalObserver.cs[43-45]

## Recommended Fix
Remove the `1 today` wording and retain only the durable contract that `runnerInvocations` counts all runner calls, including retries.

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


View medium (1)
7. Requested baseline files are skipped ✓ Resolved 🐞 Bug ≡ Correctness
Description
GetArg returns null when --baseline-out is the final token, but the validation loop only rejects
non-null values beginning with --. With a valid or ambient session identifier, evaluation can
still proceed while HandleEval skips installing BaselineObserver, so the supplied flag is
silently ignored and no requested data is written.
Code

src/Capacitor.Cli/Program.cs[R353-355]

+        foreach (var (flag, value) in new[] { ("--questions", evalQuestions), ("--skip", evalSkip), ("--baseline-out", evalBaselineOut) }) {
            if (value is not null && value.StartsWith("--")) {
                Console.Error.WriteLine($"eval: {flag} requires a value (got '{value}')");
Relevance

●●● Strong

Trailing valued flags must fail validation rather than being silently ignored; recent
argument-parsing findings were accepted.

PR-#383
PR-#196

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The argument helper returns null for a trailing flag, so the changed validation cannot enter its
error branch because it only examines non-null values. Session resolution can then use the provided
session ID or fall back to environment variables, while the baseline decorator condition installs
BaselineObserver only for a non-null path, allowing evaluation to run without baseline output.

src/Capacitor.Cli/Program.cs[341-356]
src/Capacitor.Cli/Program.cs[932-935]
src/Capacitor.Cli/Commands/EvalCommand.cs[35-38]
src/Capacitor.Cli/Program.cs[348-356]
src/Capacitor.Cli/Program.cs[931-936]
src/Capacitor.Cli/ArgParsing.cs[17-31]

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

## Issue description
A trailing `--baseline-out` is treated as if the flag were absent because `GetArg` returns `null`. Reject the command whenever the flag is present but has no following non-flag value, rather than allowing evaluation to continue without producing the requested output file.

## Fix Focus Areas
- src/Capacitor.Cli/Program.cs[348-356]
- src/Capacitor.Cli/Program.cs[931-936]
- src/Capacitor.Cli/ArgParsing.cs[17-31]

## Recommended Fix
Track whether `args` contains `--baseline-out` separately from its parsed value. Before resolving the session or calling `HandleEval`, print the existing requires-a-value message and return exit code 2 when the flag is present and the parsed value is null; retain the existing rejection when the following token is another flag. Add an argument-parsing regression test for a trailing `--baseline-out` after a valid session ID.

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


Grey Divider

Context sources
✅ Compliance rules (platform): 64 rules
✅ Cross-repo context — repo relationships
  Explored: repo: kurrent-io/kcap-server (sha: 2bebd0ee)
Review mode: 🧠 Deep: This PR combines a security-sensitive redaction regex/API change with a new cross-layer baseline-output feature spanning CLI parsing, observer protocols, evaluation accounting, AOT serialization, and failure paths, creating many independent opportunities for subtle defects.

Grey Divider

Tip of the day
💡 Did you know, you can turn on the rule miner and Qodo learns your standards from review history

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread test/Capacitor.Cli.Core.Tests.Unit/Eval/BaselineOutputWriterTests.cs Outdated
Comment thread test/Capacitor.Cli.Core.Tests.Unit/Eval/BaselineOutputWriterTests.cs Outdated
Comment thread src/Capacitor.Cli.Core/Eval/IEvalObserver.cs Outdated
Comment thread src/Capacitor.Cli.Core/Eval/BaselineObserver.cs Outdated
Comment thread src/Capacitor.Cli/Program.cs Outdated
Comment thread src/Capacitor.Cli.Core/Eval/BaselineObserver.cs Outdated
/// unassessed outcome, which carries no score. <paramref name="route"/> is <c>text</c> or
/// <c>tools</c>; <paramref name="runnerInvocations"/> counts the runner calls behind this
/// result (retries included — 1 today).</summary>
void OnQuestionCompleted(int index, int total, EvalQuestionAssessment assessment, EvalUsage usage, string route, TimeSpan elapsed, int runnerInvocations);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

2. Server eval tests stop compiling 🔗 Cross-repo conflict ≡ Correctness

IEvalObserver changes both completion callbacks to require usage and timing metadata, but
kcap-server's two test observers retain the old signatures. When the server advances its referenced
CLI submodule to this change, its evaluation test project reaches those implementations and cannot
satisfy the interface.
Agent Prompt
## Issue description
The changed `IEvalObserver` completion signatures are not implemented by kcap-server's evaluation test observers, causing its integrated test project to stop compiling when the CLI submodule is advanced.

## Fix Focus Areas
- src/Capacitor.Cli.Core/Eval/IEvalObserver.cs[42-58]
- test/Capacitor.Server.Tests.Evals/CliCatalog/CliCatalogIntegrationTests.cs[70-80]
- test/Capacitor.Server.Tests.Evals/CliCatalog/AliasRawTextLiveTests.cs[59-69]

## Recommended Fix
Coordinate a kcap-server change that updates both `TestObserver` implementations to the new question and retrospective completion signatures, including `EvalUsage`, route, elapsed time, and invocation count parameters.

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

A failed judge was dropped from --baseline-out entirely and a failed write still exited 0; the file now records each failed question and the run exits non-zero when it cannot be written. A value flag given with no value is rejected, not silently skipped.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant