Repository navigation
[AI-1677] flows.reviewer_vendor preference + vendor-less definition fallback - #433
Merged
Merged
Conversation
A vendor-less catalog start whose definition names no reviewer is refused by the server with the structured reviewer_vendor_required code. Retry that once with the saved flows.reviewer_vendor preference, injected as an explicit vendor so the request is indistinguishable from a named one — the echo check asserts against it, and a mismatch still closes the run defensively. The trigger is the safety boundary: a start is non-idempotent, so it fires only for the structured code on a non-dynamic, non-model-bearing, vendor-less start. A model-bearing v3 start is excluded by shape (it returns the same code for a blank vendor, where a preference would shadow the vendor the pinned model belongs to), and no uncoded body, other code, or transport ambiguity can reach it. The retry is terminal either way: a stale preference gets the re-ask-and-re-save remedy, anything else surfaces as itself. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The retry was opening a second full 3-minute settlement window, so a tool call could spend 3m + 3m plus the poll cap — past the harness MCP tool timeout the deadline was derived against, and in the worst shape available: the retry POST succeeds, a paid reviewer launches, the harness has already given up, and the driver starts the flow again. SendWithSettlementRetryAsync gains an optional budgetStartedAt; the start arm takes one timestamp before the first send and threads it into the retry, so both sends share a single window and a spent one returns before posting at all. Every other caller is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The README's start_flow tool doc and the "restart after upgrade" callout still described the removed Flows:Review:DefaultVendor server setting. Rewrite both to the landed vendor-resolution ladder (explicit -> the definition's authored vendor -> the CLI's saved flows.reviewer_vendor preference, applied via one automatic retry -> coded reviewer_vendor_required), matching the wording already landed in the review-flows/agent-flows skills. Also teach kcap config's help text about the new unset subcommand and the flows.reviewer_vendor key.
PR Summary by QodoAdd saved reviewer-vendor fallback for vendor-less review-flow starts
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5eb50878e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+702
to
+703
| static async Task<string?> LoadReviewerVendorPreferenceAsync() => | ||
| (await AppConfig.GetActiveProfileAsync())?.EffectiveReviewerVendorPreference(); |
There was a problem hiding this comment.
In a long-lived kcap mcp flows process, ResolveServerUrl has already populated AppConfig.ResolvedProfile, so GetActiveProfileAsync() returns that cached Profile without rereading config.json. If the user follows the new guidance and sets, changes, or unsets flows.reviewer_vendor while the harness session remains open, subsequent vendor-less starts continue using the startup value (or keep reporting no preference) until the MCP server is restarted. Resolve the selected profile name once, but reload that profile's current settings when the fallback is triggered.
Useful? React with 👍 / 👎.
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+125
to
+126
| "flows.reviewer_vendor" => profile with { Flows = (profile.Flows ?? new FlowsSettings()) with { ReviewerVendor = null } }, | ||
| _ => throw new ArgumentException($"Unknown or non-unsettable config key: {key}") |
There was a problem hiding this comment.
2. Unset logs as crash 🐞 Bug ☼ Reliability
kcap config unset throws on unsupported keys and the exception bubbles to the top-level crash handler, producing crash.log entries for normal user mistakes (e.g., unset server_url). This misclassifies user input errors as crashes and pollutes crash telemetry.
Agent Prompt
### Issue description
`kcap config unset <key>` currently throws `ArgumentException` for unknown/non-unsettable keys, which escapes `ConfigCommand` and is handled by the CLI-wide crash handler. This causes normal user errors to be logged as crashes (and written to crash.log), which is misleading and noisy.
### Issue Context
- `ConfigCommand.HandleAsync()` dispatches directly to `Unset()`.
- `Unset()` calls `ApplyUnset()`, whose default arm throws.
- `Program` has a catch-all that calls `CrashReporter.Record()`, so these user errors become crash records.
### Fix Focus Areas
- src/Capacitor.Cli/Commands/ConfigCommand.cs[15-25]
- src/Capacitor.Cli/Commands/ConfigCommand.cs[72-86]
- src/Capacitor.Cli/Commands/ConfigCommand.cs[123-127]
### Implementation notes
- Catch `ArgumentException` inside `Unset()` (or in `HandleAsync()` around the subcommand dispatch), print a normal user-facing error (and optionally `UnsetUsage()`), and return exit code 1.
- Keep `ApplyUnset()` throwing (useful as a pure function + tests), but prevent exceptions from reaching the global crash reporter.
- Add/adjust a unit test that exercises `Unset()`/`HandleAsync()` for an unsupported key and asserts it returns 1 and does not throw.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…file The lookup went through the profile deserialized at process start, but `kcap mcp flows` is long-lived and the `kcap config set` its own guidance asks for is another process writing that file — so refuse → ask → save → retry could never close inside a session. It now re-reads from disk on every consultation, using the profile this process resolved. Both fallback messages also name that profile. `kcap config set` writes to the ACTIVE profile while the flows lane reads the repo/URL/env-resolved one; when they differ a dutiful save lands where nothing reads it, and without the name the symptom is a preference that silently never works. Changing the write target is deliberately out of scope here — it affects every key. Also: one shared vendor-token list behind the flows guidance and the config warning, the config echo prints the normalized token it stored, and help-config.txt regains three keys it had drifted away from. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The public-repo lint forbids Linear issue IDs in C# source; the rationale stays, the token goes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three commits deliver a local fallback for the case a review-flow catalog definition names no reviewer vendor:
flows.reviewer_vendorconfig key +kcap config unset— a new per-profile preference (ProfileConfig/FlowsSettings.ReviewerVendor), set viakcap config set flows.reviewer_vendor <vendor>(warns, but does not reject, an unrecognized vendor token — the server has the authoritative list) and removed via the newkcap config unset <key>subcommand (currently only supports this key).kcap mcp flows— when a vendor-less, non-dynamic, non-model-bearingstart_review_flow/start_flowis refused by the server with the codedreviewer_vendor_required, the CLI retries exactly once with the savedflows.reviewer_vendorpreference injected as an explicitvendorargument — indistinguishable from a caller-supplied one, so the existing vendor-echo check still asserts against it and defensively closes the run on a mismatch. The trigger (ShouldPreferenceRetry) is deliberately narrow: a start is non-idempotent, so it fires only for the structured code on a start that never named a vendor and never carried a model ordefinition_yaml— a model-bearing v3 start returns the same code for a blank vendor but must never have a preference silently shadow the vendor its pinned model belongs to, and no uncoded body, other code, or transport ambiguity can reach the retry. With nothing saved, the tool result asks the driver to prompt the user and offer to save the answer; a stale preference (reviewer_vendor_unavailable/unknown_vendoron the retry itself) gets a re-ask-and-re-save remedy instead of blaming an unrelated fault.SendWithSettlementRetryAsyncnow takes an optionalbudgetStartedAt; the start arm takes one timestamp before the first send and threads it into the retry so both sends share one window, and an already-spent budget returns before posting at all. Every other caller is unchanged.agent-flowsandreview-flowsSKILL.md now teach the definition-authored-vendor → saved-preference →reviewer_vendor_requiredchain (replacing the old "server default" framing), with matching updates to the anti-hallucination "do not claim a named reviewer ran" guidance and the tool-parameter tables;FlowsDriverSchemaConformanceTestspins the new wording.Flows:Review:DefaultVendorreferences (thestart_flowtool doc and the "restart your harness after upgrading" callout) to the landed vendor-resolution ladder, and addskcap config unset+ theflows.reviewer_vendorkey tohelp-config.txt.Compatibility
reviewer_vendor_requiredfor an unconfigured/unnamed reviewer) — no server-side change is required for the fallback to work.reviewer_vendor_required) instead of a server-wide configured default; this CLI's fallback composes with that unchanged.kcap-flows) after upgrading to see any tool-description wording change from this PR.Test plan
dotnet build Capacitor.slnx— 0 errors (pre-existingScriban.SignedNuGet advisory warnings only, unrelated to this branch)Capacitor.Cli.Tests.Unit) — 5161 total, 47 failed, 5090 passed, 24 skipped. The 47 failures are the known pre-existing baseline (Codex TOML / Uninstall / BorrowedReviewAuthBroker sandbox artifacts, reproduced at branch base) — zero new failures.ConfigCommandTests(26/26) andReviewerVendorFallbackTests(42/42) — this PR's own coverage — pass fully.Server PR: https://github.com/kurrent-io/kcap-server/pull/1272
🤖 Generated with Claude Code