Repository navigation
Make gated unattended reviewers reachable: consent on supervised daemons, a withheld diagnostic, and an affirmation-based Gemini gate - #473
Merged
Conversation
… and say why one is withheld Two compounding problems kept the Gemini reviewer unreachable and undiagnosable. A supervised daemon could never be given consent. The two reviewer consent flags are read from the daemon's own environment and have no config-file or profile binding, but the service unit never carried them — and launchd/systemd/a scheduled task inherit nothing from the installing shell. Exporting a flag and reinstalling produced a unit that silently did not have it. ServiceEnvironment now carries both (booleans, not secret-capable, so no platform exclusion applies), and install reports each one it captured: the unit outlives the shell it came from, so freezing a consent decision is something the operator watches happen rather than discovers later. A withheld reviewer never said why. The gate's denial text is precise, but only the launch path threw it — and advertisement is exactly what stops the launch from being attempted, so the one artifact that explains the refusal could never be produced. The only remaining route to the answer was reading the daemon source. Startup now logs one Warning per vendor that is installed and unattended-capable but withheld, carrying that same text. A vendor that simply does not offer unattended hosting is deliberately not reported: warning about a design fact would train operators to ignore the actionable one. Both facts now come from one call. The advertisement flag and the reason are resolved together, because answering spawns the vendor binary to read its version — and startup needs the advertised list, the capability list and the diagnostic. Deriving all three from one classification also removes an existing duplicate probe. That collapses a real duplication on the way: advertisement and the launch boundary were separately maintained copies of the same gate ladder, which is how they could disagree about whether a reviewer was available while only one of them could say why. There is now one function; the launch boundary still evaluates it live, so a build swapped under a running daemon is re-judged rather than read from a startup snapshot. An uncertified vendor build stays refused — that is the fail-closed design working, and clearing it remains a maintainer act. This only makes the refusal legible. Closes #472 AI-1755
PR Summary by QodoCarry reviewer consent flags into service units; log why a reviewer is withheld
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
The withheld-vendor line drops from Warning to Information. A daemon with a gated vendor installed and no opt-in is in a perfectly normal steady state — an operator may run Kiro or Gemini interactively and never intend either unattended — so at Warning this fired on every restart, forever, for a correct configuration, and in most stacks Warning is what reaches an alerting pipeline. The daemon's default minimum level is Information, so the line is still in the log for whoever goes looking, which is all it was ever for. The reason now ends the message. Every reason string is a full sentence ending in a period, so interpolating it mid-sentence rendered "…acceptable. Until that is resolved…", which reads like a truncation artifact. AdvertisedUnattendedVendors is now tested on its own inputs. Since ComputeUnattendedVendors is defined AS that composition, the old test compared a composition to its own definition and could not have failed for either function independently. Feeding statuses directly tests the one thing the helper does, including that it reads Advertised rather than the reason field — verified by mutating the predicate, which the previous version did not catch.
Qodo flagged the new multi-paragraph blocks against the repo's own DosAndDonts-CommentsNotVerbose rule, and it is right that they had grown past what the facts need. Each one keeps the non-obvious claim — why one call rather than two, why the ladder is not cached, why Information rather than Warning, why capture cannot create consent — and drops the restatement around it.
…rsion set The certified set was a maintainer-curated list of gemini builds whose MCP allowlist had been read. It failed in the way that shape fails: the reviewer went offline at 0.54.0, one patch ahead of the certified 0.53.0, and could only come back by shipping a new kcap. Every Gemini release repeats that, against a CLI that moves several versions a week — so in practice the feature was unreachable rather than protected. Kiro already faced this and answered it. Its gate fails closed when the installed build CHANGES and is cleared by the operator — the party who already carries the risk — with no release involved. Gemini now uses the same model and the same command, `kcap daemon reviewer affirm --vendor gemini`. Generalized rather than copied. The store is vendor-keyed and the version comparison lives in one place, so the two reviewers cannot drift apart on what "the operator accepted this build" means; each keeps its own preconditions (Kiro's POSIX requirement) and its own consent text, because what enabling a reviewer grants differs per vendor. Kiro's on-disk filename is deliberately unchanged — renaming it would have made every existing affirmation invisible and silently withheld shipped reviewers on upgrade, which is pinned by a test. What the trade is, stated rather than buried: the certified set asserted a maintainer had read that build's matcher; an affirmation asserts only that the operator accepted the build. That is the weaker claim, made by the party who carries the risk on the machine that carries it. A minimum-version floor was considered and rejected — it would assume the allowlist's semantics can only improve, which is an assumption about someone else's code, and would silently admit a build that changed matching to prefix or flipped empty-list semantics. Gemini's version refusal codes are renamed to match Kiro's (`gemini_reviewer_version_unresolved` / `_unaffirmed`), since the two now share one model. The live certification stays, and now certifies the MECHANISM against a build rather than backing a version list. Also collapses a duplicate probe on the refusal path: the decision and its explanation both need the installed version, and resolving it per consumer spawned the vendor binary twice to produce one refusal.
realtonyyoung
changed the title
This was referenced Oct 1, 2026
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
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.
Closes #472. Linear: AI-1755.
The reported symptom
start_review_flow(vendor: "gemini")fails withreviewer_vendor_unavailableon a daemon where Gemini is installed, on PATH, and hosts interactively fine.The filed diagnosis pointed at
CliResolver.Exists. That was wrong — Gemini resolves and is inSupportedVendors. Advertisement filters on the factory'sSupportsUnattended, which the ACP factory overrides, and for the gated reviewers that override also requires operator consent and an accepted vendor build. Three things were wrong behind it.1. A supervised daemon could never be given consent
KCAP_GEMINI_UNATTENDED_REVIEWER/KCAP_KIRO_UNATTENDED_REVIEWERare read from the daemon's own environment and have no config-file or profile binding.ServiceEnvironmentdid not carry them into a service unit, and launchd/systemd/a scheduled task inherit nothing from the installing shell — so exporting a flag and reinstalling produced a unit that silently did not have it. Same "shipped but unreachable" shape the env bindings themselves were added to fix, just moved from the daemon to the unit, and it lands on the normal way to run a daemon.Both flags are now carried on every platform (booleans an operator typed, not secret-capable, so
GoogleSecretCapableKeys' platform exclusion does not apply). Capture never creates an opt-in — a unit can only end up with a flag the installing environment already had — but it does freeze one, soinstallprints aConsent:line naming each flag it captured, read from the built environment rather than the ambient one.2. A withheld reviewer never said why
DenialReasonis precise, but only the launch path threw it — and advertisement is exactly what stops the launch being attempted. The one artifact that explains the refusal could never be produced; the only route to the answer was reading the daemon source, which is how this was diagnosed.Startup now logs one line per vendor that is installed and unattended-capable but withheld, carrying that same text. A vendor that simply does not offer unattended hosting is deliberately not reported. It logs at
Information, notWarning— a gated vendor installed with no opt-in is a normal steady state, and warning on a correct configuration at every restart is how a diagnostic gets ignored.3. The version gate made the feature unreachable in the first place
Gemini was gated on
CertifiedVersions— a maintainer-curated set ({"0.53.0"}) of builds whose MCP allowlist had been read. The reporting machine ran 0.54.0, one patch ahead, so even with 1 and 2 fixed the reviewer stayed refused until a new kcap shipped. Against a CLI that moves several versions a week that repeats on every release: the gate was not protecting the feature, it was preventing it.Kiro already solved this one commit earlier — fail closed when the installed build changes, cleared by the operator via
kcap daemon reviewer affirm. Gemini now uses that model and that command (--vendor gemini).Generalized rather than copied: the store is vendor-keyed and the version comparison lives in one place, so the two reviewers cannot drift on what "the operator accepted this build" means, while each keeps its own preconditions (Kiro's POSIX requirement) and its own consent text. Kiro's on-disk filename is deliberately unchanged — renaming it would have made every existing affirmation invisible and silently withheld a shipped reviewer on upgrade, which is pinned by a test.
The trade is stated in the type doc rather than buried: the certified set asserted a maintainer had read that build's matcher; an affirmation asserts only that the operator accepted the build. That is the weaker claim, made by the party who carries the risk on the machine that carries it. A minimum-version floor was considered and rejected — it assumes the allowlist's semantics can only improve, which is an assumption about someone else's code, and would silently admit a build that changed matching to prefix or flipped empty-list semantics.
One decision, one probe
The advertisement flag and the reason come from a single
DescribeUnattendedSupport()call, because answering spawns the vendor binary to read its version. Startup needs the advertised list, the capability list and the diagnostic; deriving all three from one classification also removes a duplicate probe that was already there.GateInputsFordoes the same on the refusal path, where the decision and its explanation both needed the installed version and resolved it twice.That collapses a real duplication: advertisement and the launch boundary were separately maintained copies of one gate ladder — which is how they could disagree about whether a reviewer was available while only one could say why. There is now one
ReviewerRefusal, deliberately uncached so the launch boundary re-judges a build swapped under a running daemon.Behaviour changes worth calling out
gemini_unattended_reviewer_version_uncertified/_unresolved→gemini_reviewer_version_unaffirmed/_unresolved, matching Kiro's shape now that the two share a model.gemini_unattended_reviewer_disabledis unchanged — though its text no longer tells operators to setGeminiUnattendedReviewerEnabled, a thing none of them could set.KiroReviewerVersionStore→ReviewerVersionStore(stateDir, vendor). On-disk filename forkirois unchanged.Testing
Suites green:
Gemini*154,Kiro*129,Acp*378,Daemon*231,Service*36,Reviewer*104. Clean AOT publish, no IL warnings.Every new guard was mutation-tested rather than assumed, each mutant reverted by its inverse and re-run green:
ReviewerConsentKeysfrom the unit environmentAdvertisedUnattendedVendorsfilter on the reason instead ofAdvertisedFileNameForto a vendor-independent nameNew tests carry their own negative controls — an advertised vendor must report no reason, a vendor that never offered unattended hosting must not be reported as withheld, and the affirmed build must permit a launch — so a gate that refused everything, or a reason that was always populated, fails.
Reviews
Copilot review flow: clean (two rounds on the consent/diagnostic work, one round on the gate rework). It caught the
Warning-vs-Informationlevel, a sentence seam, and a test that compared a composition to its own definition — all fixed. Qodo: threads addressed and resolved.