Repository navigation
[AI-899] Host Gemini CLI as an ACP hosted agent - #421
Merged
Merged
Conversation
Rev 1 + the nine round-1 spec-review findings. Three claims withdrawn rather than defended: that GOOGLE_CLOUD_PROJECT is required on every auth path (it was measured on one), that --skip-trust adds no risk surface (the probe never established what the trust gate was gating), and that the eight Google variables are safe to serialise because of what their values usually are (a semantic label is not validation, and the Windows .cmd wrapper turns an embedded quote into command execution). Two answers came from reading code the reviewer could not see: service units are written 0600 owner-only with a directory permission check, which bounds the credential-path disclosure question; and SupportsMcpServers is a blanket switch on the whole mcpServers array, not a per-transport one. Round 2 returned seven further findings — NOT yet addressed. See the Linear issue. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Seven findings; three were shape-changing, and two of those got smaller once I read the code instead of arguing. Validation belongs at the serialisation SINK, not at ServiceEnvironment.Build. Not a theoretical bypass: DaemonCommands builds the env from Capture(profile) and then adds KCAP_DAEMON_SUPERVISED to the dictionary, so a value already reaches the writers without passing Build today. The Windows writer now validates its own input for every key, which closes the pre-existing PATH/KCAP_URL exposure rather than merely not widening it, and makes the drop-recording contract unnecessary: service install fails with the key named. The earlier writer table was also wrong about systemd — EnvAssignment quotes and escapes properly; what it actually does is silently rewrite CR/LF. The 0600 measurement answers Unix ONLY. Every permission path in ServiceFiles begins with an IsWindows early return, commented "ACL-governed, inherited from the user profile". So the secret-capable values — the credential path and the two base URLs, which can carry userinfo or a query token — are captured off-Windows and EXCLUDED on Windows, following the KCAP_COPILOT_TOKEN_CMD precedent. Stated as a principle so future additions are not classified by variable name: persist only when there is no non-persistent alternative AND the platform gives an owner-only guarantee this code enforces. The status predicate contradicted the spec's own capture table. "Project configuration present" is now the OR of the two project variables — and that is the vendor's definition, quoted from Gemini's own error text, not an inference from reference counts. The alternate-only case gets a test. The trust probe has a decision rule instead of "clamp or document": any repository-controlled facility that can cause process execution before an explicit user-approved action MUST be clamped; non-executable settings may be documented. If it can execute and cannot be clamped, hosted Gemini does not ship. The pricing outcome is UNKNOWN, not "no cost by one of two mechanisms" — the disjunction was not exhaustive, since a miss on the sentinel could still match a pricing entry directly. The characterisation test is now gating, with a three-way outcome table, and must not be written to expect one of them. The approved diagnostic wording is in the spec, so the golden test pins that rather than whatever the implementer writes first; plus the additive-wire constraint, since a structured kind crosses a shape an older server consumes. Two AI-1413 prescriptions relabelled as questions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rev 4. The gating probe from rev 3 is measured, with a positive control, and the answer is worse than the question: --skip-trust does NOT grant workspace-configuration trust. So the flag was never the risk, and the earlier draft's worry was aimed at the wrong thing. But trust INHERITS from a trusted parent, and --skip-trust cannot undo it. With a parent marked TRUST_PARENT and no entry for the worktree at all, a repo-authored MCP server AND a repo-authored SessionStart hook both executed while the flag was passed. Passing --skip-trust is not containment and must never be described as one. This is live for kcap. WorktreePathResolver and WorktreeManager put daemon worktrees at <repo>/.capacitor/worktrees/agent-..., inside the operator's repository. An operator who trusted their own repo — the normal thing to do, and the measuring machine has a real TRUST_PARENT entry — gets every hosted Gemini agent inheriting that trust and auto-executing whatever .gemini/settings.json the checked-out branch carries, under the daemon user, with no prompt, before the model acts. That branch may be PR-authored. By §3.1's own rule that is a MUST-clamp. MCP servers are clampable and the flag is now in the descriptor argv; hooks are NOT clampable by any launch flag — `gemini hooks` is a management subcommand, there is no --no-hooks, and the hook fired under every trusted configuration. So hosted Gemini cannot ship on --skip-trust alone. Four options are tabled with a recommendation (refuse to launch when the worktree resolves as trusted, plus the MCP clamp, with worktree relocation as the follow-up that restores availability). That is a decision, not an implementer's choice. Also recorded because it is a verification trap: --allowed-mcp-server-names "" crashes config load before session start, so a probe using it reports no markers for entirely the wrong reason.
…ntained The gating probe was run with gemini --prompt, the print/CLI path. kcap hosts over ACP. Re-measured there with inherited trust: print/CLI : repo MCP EXECUTED, repo hook EXECUTED ACP : repo MCP EXECUTED, repo hook BLOCKED ACP + --allowed-mcp-server-names <injected names> : both blocked So 3.1a was wrong in both directions. Hooks do not execute on the ACP path, so 'hooks are unclampable, therefore hosted Gemini cannot ship' is withdrawn. Repo-authored MCP servers do execute on ACP under inherited trust, and the allowlist flag already in the descriptor contains it — injected server loads, repo-authored one blocked. The containment decision is no longer needed: no worktree relocation, no launch refusal, no .gemini/ stripping. The real correction was the allowlist CONTENTS — rev 4 wrote a non-matching sentinel, which would have blocked our own injected servers and shipped hosted Gemini with MCP silently broken. Spec is implementation-ready. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Interactive hosting only; the unattended reviewer stays its own issue, as with Kiro. Argv is `--experimental-acp --skip-trust --allowed-mcp-server-names kcap-none`. `--skip-trust` is required and is NOT containment. Gemini refuses a headless turn in an untrusted directory (exit 55, before any model call) so without it every launch fails — but trust INHERITS from a trusted parent, and a daemon worktree lives inside the operator's repository, so an operator who trusted their own repo gives every hosted agent inherited trust over the checked-out branch's configuration and the flag does not withdraw it. Measured. Which is why the MCP allowlist is here. Under inherited trust a repo-authored .gemini/settings.json MCP server WAS observed starting on the ACP path — repo-controlled process execution under the daemon user, before the model acts, on a branch that may be contributor-authored. Reducing the allowlist to a single non-matching name blocks it. Repo-authored HOOKS were separately measured not to run on the ACP path, though they do on the --prompt path; the two paths differ and neither predicts the other, which is why the spec's earlier conclusion (that hooks were the unclampable blocker) was wrong. The allowlist contents are COUPLED to SupportsMcpServers. A non-matching name permits nothing, which is correct only while nothing is injected; the day the stdio call-level probe flips that flag, the list must become the injected server names in the same change or hosted Gemini ships with MCP silently broken. A test asserts the coupling in both directions, and it is mutation-proven: flipping the flag alone fails exactly that test plus the constants test. NoOpModelSelector for the same reason as Kiro — session/new returns models so the read half fits, but the write half is unverified and that selector fails SILENTLY. --approval-mode is deliberately not passed: hosting should behave as the user's own session does. The launch-failure hint is worded as a possibility, not a verdict, and that is a requirement. Gemini reports a missing project with a message naming a tier problem — thrown by throwIneligibleOrProjectIdError, the same text for both causes — and reproducing that confidently-wrong attribution would be worse than saying nothing. Asserted clause by clause rather than by keyword scan. Still to come on this issue: the ServiceEnvironment capture of the Google/Vertex configuration with its platform split, the kcap status warning, and the README section.
…unit
A supervised daemon inherits nothing from an interactive shell — launchd passes
no shell environment and a non-interactive shell never reads a profile — so a
project exported in .zshrc is invisible to a hosted Gemini agent. Gemini then
reports the absence as a TIER problem (IneligibleTierError, thrown by
throwIneligibleOrProjectIdError, which emits the same text for a missing project
id), which sends people to entirely the wrong place. ServiceEnvironment now
captures the configuration so that cannot happen.
Both project spellings are carried because Gemini honours both; its own error
says "The GOOGLE_CLOUD_PROJECT (or GOOGLE_CLOUD_PROJECT_ID) environment variable
must be set".
Platform split, stated as a principle rather than per variable: persist a value
into a unit only when there is no non-persistent alternative AND the platform
gives an owner-only guarantee this code actually enforces.
everywhere : the two project spellings, location, and the two USE_* booleans
Unix only : GOOGLE_APPLICATION_CREDENTIALS and the two base URLs — a
credential PATH discloses where the credential is, and a URL can
carry userinfo or a query token. Bounded on Unix by ServiceFiles
writing 0600, re-checking the mode on the open handle (umask
filters UnixCreateMode, so the request is not the result) and
refusing a group/other-writable directory. Every one of those
paths returns early on Windows — "ACL-governed, inherited from
the user profile" — so there is no equivalent guarantee and those
three are excluded there, exactly as KCAP_COPILOT_TOKEN_CMD is.
never : GOOGLE_API_KEY, GOOGLE_CREDENTIALS. Named in code rather than
merely absent, so adding a GOOGLE_* key forces a decision.
Also closes a pre-existing hole rather than only declining to widen it. The
Windows wrapper emits `set "K=V"` with only % escaped, so an embedded quote
CLOSES the assignment and &, |, <, >, ^ become live batch metacharacters in a
file the service executes. WindowsTaskUnit now validates its own input for every
key — PATH and KCAP_URL included — and fails the install naming the key.
The check is at the sink deliberately, not at ServiceEnvironment.Build: Build is
not a boundary, because DaemonCommands adds KCAP_DAEMON_SUPERVISED to the
dictionary AFTER Capture returns, so a value already reaches the writer without
passing through it. The tests call the writer directly for the same reason —
routing through Build would pass even if the writer were defenceless — and
include a value arriving by that exact bypass route.
Both guards mutation-proven: removing the sink check fails 8 of 13 (the 5
survivors are the positive controls, which is what stops the suite passing on a
writer that rejects everything); adding GOOGLE_API_KEY to the allowlist fails
exactly the two never-captured cases.
README documents the daemon-not-your-shell requirement, the misattributed error,
the platform table, the install-time capture footgun, and which auth method was
actually verified.
PR Summary by QodoHost Gemini CLI as an ACP hosted agent (interactive-only)
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}
Comment on lines
+280
to
+315
| /// <summary>Google Gemini CLI as an ACP hosted agent (<c>gemini --experimental-acp</c>). | ||
| /// Interactive hosting only; the unattended reviewer is its own issue, as with Kiro. | ||
| /// | ||
| /// <para><b><c>--skip-trust</c> is required, and is NOT a containment measure.</b> Gemini refuses a | ||
| /// headless turn in an untrusted directory outright — <c>exit 55</c> before any model call — and a | ||
| /// daemon-created worktree cannot be assumed pre-trusted, so without the flag every launch fails. | ||
| /// What it does NOT do is protect anything: trust INHERITS from a trusted parent directory, and a | ||
| /// daemon worktree lives at <c><repo>/.capacitor/worktrees/agent-…</c>, INSIDE the operator's | ||
| /// repository. An operator who has trusted their own repo therefore gives every hosted Gemini agent | ||
| /// inherited trust over the checked-out branch's configuration — and passing <c>--skip-trust</c> does | ||
| /// not withdraw it. Measured, not assumed.</para> | ||
| /// | ||
| /// <para><b>Which is why the MCP allowlist is here.</b> Under inherited trust, a repository-authored | ||
| /// <c>.gemini/settings.json</c> MCP server WAS observed starting on the ACP path — repo-controlled | ||
| /// process execution under the daemon user, before the model acts, on a branch that may be | ||
| /// contributor-authored. <see cref="GeminiNoMcpSentinel"/> reduces the allowlist to nothing, which | ||
| /// blocks it. Repo-authored <i>hooks</i> were separately measured NOT to run on the ACP path (they do | ||
| /// on the <c>--prompt</c> path — the two paths differ, and neither predicts the other).</para> | ||
| /// | ||
| /// <para><b>The sentinel is only correct while <see cref="AcpVendorDescriptor.SupportsMcpServers"/> is | ||
| /// false.</b> It permits nothing, so with nothing injected it costs nothing. The day the stdio | ||
| /// call-level probe flips that flag, this list must become the injected server names in the SAME | ||
| /// change, or hosted Gemini ships with MCP silently broken. <c>AcpVendorDescriptorTests</c> asserts | ||
| /// the coupling so the two cannot drift apart.</para> | ||
| /// | ||
| /// <para><see cref="NoOpModelSelector"/> for the same reason as Kiro: <c>session/new</c> does return a | ||
| /// <c>models</c> object, so <see cref="ConfigOptionModelSelector"/>'s read half would fit, but its | ||
| /// write half is unverified on Gemini and that selector fails SILENTLY — a session that reports the | ||
| /// requested model while running another. <c>ResolveDefaultModel: null</c> alone is not enough, | ||
| /// because <c>ResolveRequestedModel</c> prioritises a per-launch model and would reach a live | ||
| /// selector anyway.</para> | ||
| /// | ||
| /// <para><c>--approval-mode</c> is deliberately not passed: interactive hosting should behave as the | ||
| /// user's own session does, and pinning <c>plan</c> would silently make hosted Gemini read-only. | ||
| /// <c>DiagnosticBinary</c> needs no branch — the vendor key and the binary name are both | ||
| /// <c>gemini</c>.</para></summary> |
There was a problem hiding this comment.
1. Verbose gemini descriptor comments 📘 Rule violation ⚙ Maintainability
The PR adds very long explanatory XML doc comments (multi-paragraph rationale and operational guidance) directly in production code, reducing readability and increasing maintenance burden. This violates the guideline to keep comments minimal and prefer self-explanatory code with concise, necessary comments.
Agent Prompt
## Issue description
New production-code comments are overly verbose, embedding extensive design/spec narrative where concise documentation should suffice.
## Issue Context
Long rationale blocks make code harder to scan and keep correct over time; the deeper reasoning can live in the spec/README, while code should retain only the minimal invariants required for correct maintenance.
## Fix Focus Areas
- src/Capacitor.Cli.Daemon/Acp/AcpVendorDescriptor.cs[280-315]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Collaborator
Author
There was a problem hiding this comment.
Declined, with reasons — leaving this thread open for a maintainer to overrule. The density matches the sibling descriptors (Kiro explains why SupportsMcpServers: true where Copilot is false on the same advertised shape), and each paragraph records something measured that the code cannot show — two of them are why this change is correct rather than dangerous: --skip-trust is required and is not containment (trust inherits from a parent; the flag does not withdraw it), and the deny-all name must be unguessable — which is the bug you found in comment 3690901406. Deleting that explanation is how it comes back. Full rationale: #421 (comment)
🤖 Addressed by Claude Code
Comment on lines
+1
to
+8
| # AI-899 — Gemini CLI as an ACP hosted agent | ||
|
|
||
| **Status:** rev 5, 2026-07-31, against `gemini 0.53.0` and `origin/main` (`e2b2821`). | ||
| **Implementation-ready.** §3.1a's gating trust probe has been run, re-run on the correct code path, and its | ||
| finding is contained by a flag already in the descriptor. | ||
| **Repository:** kurrent-io/kcap-cli | ||
| **Parent:** AI-1399 (multi-vendor ACP hosted agents) | ||
| **Template:** the shipped Kiro child (AI-1404) and the Copilot child (AI-1403). Read those, not the |
There was a problem hiding this comment.
2. Linear ids in spec doc 📘 Rule violation ⚙ Maintainability
The new spec document includes Linear issue identifiers (e.g., AI-899) in repository text, coupling documentation to Linear-specific numbering. This violates the requirement to avoid Linear issue keys in comments/docs and use GitHub issue references instead when needed.
Agent Prompt
## Issue description
Repository documentation includes Linear issue keys (e.g., `AI-899`), which should not be referenced in repo comments/docs.
## Issue Context
Compliance requires avoiding Linear-specific identifiers; if an issue reference is necessary, use a GitHub issue/PR link or number.
## Fix Focus Areas
- docs/superpowers/specs/2026-07-31-ai899-gemini-acp-hosted-agent-design.md[1-8]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Collaborator
Author
There was a problem hiding this comment.
Declined — I think the rule is being over-applied here; leaving this thread open to adjudicate. The cited rule is about code comments, and this repo enforces it with scripts/check-linear-ids.sh, which greps exactly :(glob)src/**/*.cs and :(glob)test/**/*.cs. It passes on this branch, and I did fix a real violation earlier in this PR (an AI-899 I had put in a test comment). Design specs are the opposite case — every spec in docs/superpowers/specs/ is named and titled for its issue (…ai1404-kiro-acp-hosted-agent-design.md, …ai1410-…, …ai1463-…), and a spec whose whole job is to be the durable record of an issue should be findable from it. Changing that is a repo-wide convention decision across ~30 files, not a call for this PR.
🤖 Addressed by Claude Code
Codex round 1, finding 1, and it was the whole clamp. The descriptor passed the
literal `kcap-none` to --allowed-mcp-server-names and a comment asserted it was
"a name no MCP server will ever have". A contributor controls the reviewed
branch's .gemini/settings.json and can simply name their server `kcap-none`.
MEASURED: it executes. The clamp was bypassed completely, and the comment was
asserting a property nothing enforced — the same semantic-label-is-not-validation
error this PR already fixes elsewhere.
The descriptor now carries a PLACEHOLDER and the factory substitutes an
unguessable `kcap-deny-{guid}` per launch, so two concurrent agents do not even
share one. Verified against the worst case: a repo naming its server the
placeholder itself is BLOCKED. The substitution is deliberately generic — any
vendor whose containment is "allow a name that cannot exist" needs it, and a
per-vendor copy of the reasoning is how one of them ends up with a guessable
literal again.
Finding 3, also real: the KEY is interpolated into the same `set "K=V"` line and
is exactly as dangerous as the value, but only the value was checked. The stated
rationale for validating at the sink applies to both sides equally. Keys
containing a quote, newline or `=`, and empty keys, are now rejected, with tests.
Finding 2: the coupling test was vacuous two ways. With the option ABSENT,
IndexOf returns -1 and `argv[IndexOf + 1]` read argv[0] — non-empty and unequal
to the sentinel, so the coupling "held" with no allowlist at all. And a second
occurrence, whose value may win at argument parsing, was invisible. Now asserts
exactly one occurrence, a non-negative index, and a following value.
Finding 4: the auth-hint test asserted a few required phrases and two forbidden
ones, which cannot establish "does not diagnose" — a hint could keep both hedges
and append a categorical verdict. Now a golden equality test against the approved
wording held independently in the test.
AcpVendorDescriptorTests 19/19 · WindowsTaskUnitRepresentabilityTests 17/17 ·
ServiceEnvironmentTests 10/10 · AcpHostedAgentRuntimeFactoryTests 68/68.
…t Windows
Codex round 2, and systemd was the worst of the three. A key shaped like
SAFE=ok\nExecStartPre=/bin/touch /tmp/pwned\n#
with a perfectly ordinary value renders a valid `Environment=SAFE=ok` line, then
an attacker-chosen ExecStartPre= directive that the service runs on EVERY
restart, then comments out the remainder. Value escaping cannot help:
SystemdValue normalises the value and EnvAssignment decides quoting from the
value — neither looks at the name. Round 1 fixed exactly this at the Windows
wrapper and I left the same primitive live in a service definition that executes.
The check is now one shared rule, ServiceText.RequireValidEnvName, called by all
three writers. Deliberately an ALLOWLIST — the POSIX name grammar
[A-Za-z_][A-Za-z0-9_]* — rather than a blacklist of the characters each format
happens to fear: a blacklist has to be right once per format and again for the
next writer, while a grammar for what a legitimate name can contain is right by
construction. Nothing on any capture allowlist is excluded, so a rejection means
a caller composed the dictionary directly, which is the path that bypasses
ServiceEnvironment in the first place.
launchd is included even though XML escaping already contains the name
structurally: a control character is not legal XML 1.0, so the plist would be
silently unparseable rather than injected. Same grammar, every sink.
Tests call each writer directly rather than testing the helper once — the point
of a sink check is that every sink has it, and a shared-helper test would pass
while a writer forgot to call it. Positive control at all three sinks over the
names kcap actually captures, so the suite cannot pass on a writer that rejects
everything.
Mutation-proven: removing the systemd guard fails exactly its 6 hostile-name
cases and nothing else.
UnitWriterEnvNameTests 19/19 · LaunchdUnitTests 6/6 · ServiceFilesTests 12/12 ·
ServiceEnvironmentTests 10/10 · WindowsTaskUnitRepresentabilityTests 17/17 ·
AcpVendorDescriptorTests 19/19.
Codex round 3. The name check landed but the value still went straight to ServiceText.Xml, and entity escaping does not help: U+0001 has no legal XML 1.0 form, escaped or not, while POSIX permits any byte except NUL in a value. So a captured or directly composed value could produce a plist launchctl refuses to load. The consequence is availability rather than injection — but the failure mode is bad: the service silently does not exist. Failing at write time names the variable instead. Tab, LF and CR are legal XML 1.0 and are explicitly still accepted; the check is about what XML cannot carry, not tidiness. Both directions tested. Written as a plain scan rather than FirstOrDefault: that returns '\0' for "no match", which is itself an illegal character here, so the sentinel and a real hit are indistinguishable without a second pass. The first version did exactly that and worked, but subtlety in a security-adjacent guard is not worth the brevity. UnitWriterEnvNameTests 24/24.
Collaborator
Author
Qodo review — both bugs resolved, both rule flags declined with reasons🐞 Gemini MCP clamp bypass — ✓ Resolved (
|
… parsing Both writers interpolate values into a format that expands them BEFORE reading them as data, and each had the escape applied unevenly: - systemd expands %-specifiers in Environment=, Description= AND ExecStart=. SystemdValue escaped newlines but not `%`, and QuoteArg escaped nothing, so a path under /home/50%off rendered an ExecStart systemd rewrites or refuses to load. BinaryFromUnit reverses the doubling so `daemon doctor` still resolves the real path. - cmd.exe expands %VAR% in a batch file. CmdValue was applied to the env keys, the env values and the binary path — but not to the log path or extra args on that same exec line. `%` is a legal Windows filename character, so the log path could be rewritten from the daemon's own environment. Also replaces the plist value guard's char.IsControl with XmlConvert's XML 1.0 predicate. The two are not equivalent in either direction: IsControl accepted U+FFFE/U+FFFF and lone surrogates (which no encoder can represent, so the value is corrupted or throws) and rejected U+007F-U+009F (which XML 1.0 permits, so an install failed over a representable value). The scan now consumes a valid surrogate pair as one character, so a value containing an emoji is accepted. Found by the code-review flow, which reasoned from the submitted description rather than the source and so also raised the already-closed cmd env-value case; checking that claim against CmdValue is what surfaced the unescaped neighbours on the same line. 35 new tests: preservation and round-trip for both formats, the structural %PAYLOAD% case, and XML boundary cases (U+0085, U+FFFD, U+FFFE, U+FFFF, lone and reversed surrogates, a trailing high surrogate, consecutive pairs).
…environment Same asymmetry as the previous commit, at the third sink. Plist() guarded environment values with RequireXmlRepresentableValue but sent the service label, ProgramArguments and StandardOut/ErrorPath straight to ServiceText.Xml. SecurityElement.Escape handles the five markup characters and nothing else, so a C0 control, U+FFFE or a lone surrogate passed through unaltered into a plist launchctl then silently refuses to load. ProgramArguments carries the binary path, the log path and the extra args, and macOS filenames may contain any byte except / and NUL — so this was reachable through an ordinary directory name. Every interpolated string now goes through one guard-then-escape helper. 5 tests: a control character in the binary path, the log path, an extra arg, and (regression) an environment value, plus a positive control proving a plist whose values are all legal — including an emoji in the log path — still renders.
…idate --max-agents
Percent doubling handled expansion but not the surrounding grammar, and the
quote-when-it-has-a-space rule left the injection class open on both platforms.
cmd treats & | < > ( ) ^ as metacharacters OUTSIDE double quotes, so an argument
like `8&calc.exe` has neither a space nor a percent, rendered bare, and cmd ran
the tail as a second command in a file the OS executes at every logon. Every
exec-line value is now guarded and quoted unconditionally; a quote or newline is
rejected rather than escaped (neither can be represented there); and a trailing
backslash run is doubled, since under the Windows argv rules it would otherwise
escape the closing quote and merge the argument with the next.
systemd expands $NAME/${NAME} in ExecStart as well as % specifiers, so a path
containing ${HOME} was replaced from the daemon's own environment -- in the
executable path. Both are now doubled and reversed by BinaryFromUnit. NOT applied
to Environment=, where systemd expands specifiers but not variables, so doubling
$ would corrupt the value; the two sinks differ, so they get different escapes.
A bare `;` ExecStart argument is refused: it is systemd's command separator, and
refusing is provably safe where relying on this code's reading of the documented
\; escape (unverifiable on the build machines) is not. A line break is refused
for the same reason quoting cannot fix it -- the directive ends at the newline.
Also narrows the source: --max-agents is the only caller-supplied ExtraArgs
entry, arrives unparsed off the command line, and is numeric -- so it is now
validated as a positive integer instead of only escaped where it lands. The two
defences are independent.
29 tests: six cmd metacharacters, three unrepresentable arguments, an
unrepresentable log path, trailing vs interior backslashes, ${HOME} and $HOME in
both the binary path and an argument, both round-trips, the Environment= arm
proving $ is NOT doubled there, a bare vs embedded semicolon, CR and LF, and
seven rejected --max-agents values plus two accepted.
…exer gaps Delayed expansion: the wrapper began with only `@echo off` and the action was a plain `cmd /c`. Delayed expansion is off by default but can be enabled for every cmd session via the Command Processor registry key, and `!NAME!` expands INSIDE double quotes -- so a value like `!PAYLOAD!` passed both the quoting and CmdValue (which doubles %, not !) and could expand after serialization. The mode is now part of the artifact: `setlocal DisableDelayedExpansion` as the wrapper's first statement, and /v:off on the action so it is fixed before the file is opened. cmd /c quoting: the action was `/c "<wrapper>"`, which relies on cmd's conditional quote-stripping. A path containing & -- legal in a Windows account name, and this path runs through the config directory -- fails those conditions, the quotes come off, and the metacharacter is parsed as command syntax. Now `/d /s /v:off /c ""<wrapper>""`: /s with a command both starting and ending in a quote strips exactly the outer pair and takes the rest verbatim, /d skips AutoRun so a per-user AutoRun value cannot inject ahead of the daemon. A % in the wrapper path is refused rather than escaped -- %% is a batch-file construct with no command-line equivalent, so refusal is the only correct answer at that sink, while the wrapper BODY still escapes. Same character, two sinks. systemd: NeedsQuote covered the double quote and backslash but not the apostrophe, which is equally structural to systemd's word lexer. Bare, an unpaired one opens a single-quoted string that never closes (unit unloadable); a paired one is stripped (value silently changed). `O'Reilly` in a home directory is the ordinary case. 14 tests: the setlocal and its position, a bang-shaped value, the switch set, the nested-quote form, a path with a space and an ampersand, a %USERNAME%-shaped component, two structurally-unrepresentable paths, and apostrophes across ExecStart, Environment= and the round-trip -- paired as well as unpaired.
…e Task XML Two more instances of the two patterns this review has been finding. systemd rejected CR/LF at ExecStart but emitted every other control character verbatim, and SystemdValue rewrote CR/LF in an Environment= value to a space. A POSIX environment value, a filename and an argv string may all legally carry U+0001, a backspace or a vertical tab; quoting makes none of them valid unit syntax and this writer has no encoder for them. One shared guard now refuses C0 and DEL at both Environment= and ExecStart=. systemd does document C-style and hex escapes, but nothing here can exercise a real systemd parser, so this refuses the input rather than shipping an escape whose behaviour it has only read about -- the same call already made for the bare ';' separator. That supersedes the newline-to-space normalisation, which is the better outcome independently: silently rewriting a caller's value leaves a service running with a value nobody chose, which is harder to diagnose than an install that failed and named the variable. Task XML: TaskXml interpolated the wrapper path and the service id through ServiceText.Xml with no representability check -- the plist's fix, one sink over. Composition does not imply XML validity: the path carries KCAP_CONFIG_DIR verbatim, Windows paths are native UTF-16, and U+FFFE/U+FFFF or a malformed surrogate unit is outside XML 1.0 while SecurityElement.Escape passes all of them through. Both values now route through a guard-then-escape helper mirroring the plist's, including the sanitized service id, so the invariant holds at every interpolation rather than only where it is currently needed. 17 tests: six control characters in an environment value, four at ExecStart, one in an extra arg, a positive control that an ordinary unit still renders, four XML-illegal wrapper paths, and a positive control that a supplementary character in the path is accepted. The line-break case keeps its own test and now asserts the code point the shared guard names.
Collaborator
Author
|
/review |
PR Reviewer Guide 🔍Warning
Here are some key observations to aid the review process:
|
Rev 5 documented two things that turned out to be defects, so the spec is now wrong in a way that would mislead the next reader of these sinks. 3.1d: the descriptor block still showed the literal `kcap-none` deny-all MCP name with a comment calling it "a name no MCP server will ever have". Nothing enforced that, and a repo naming its server `kcap-none` was measured to execute. Records the measurement, the fix (placeholder plus per-launch unguessable name), and that the clamp is now verified in BOTH directions rather than only the negative one. 4.2b: 4.2 reasoned about environment values one writer at a time, which was the wrong unit. Each writer interpolates several values into the same artifact, and in every one of them some were guarded and their neighbours were not — six sinks, tabulated. Also records the second error: escaping the one expansion a format performs without checking what else it expands (cmd's metacharacters outside quotes, delayed expansion inside them, conditional /c quote-stripping; systemd's $-substitution in ExecStart and the apostrophe in its word lexer), the same-character-two-sinks rule that falls out of it, and the escape-where-verifiable / refuse-otherwise policy. Supersedes 4.2's decision to leave the CR/LF rewrite in place: a silent rewrite is worse than a refusal. Also reorders 4.2b after 4.2a, and notes the mutation-restore trap (verify by content, not by git diff — a diff can hide a bad restore through alignment).
Collaborator
Author
|
/agentic_review |
Comment on lines
+22
to
+139
| /// <summary> | ||
| /// Escape a systemd <c>Environment=</c>/<c>Description=</c> value: literal percent signs doubled. | ||
| /// | ||
| /// <para><b>Why the doubling.</b> systemd expands <i>specifiers</i> — <c>%n</c>, <c>%h</c>, <c>%i</c> and | ||
| /// friends — in both of those directives, so a value carrying a literal <c>%</c> does not survive: a | ||
| /// <c>PATH</c> containing <c>%n</c> silently becomes the unit name, and a percent sequence systemd does | ||
| /// not recognise makes it refuse to load the unit at all. <c>%%</c> is systemd's own escape for one | ||
| /// literal percent, so this is a faithful round-trip rather than a mangling.</para> | ||
| /// | ||
| /// <para>This is the same primitive <see cref="CmdValue"/> applies for cmd.exe, for the same reason, and | ||
| /// it was missing here while present there — the asymmetry is what review caught.</para> | ||
| /// | ||
| /// <para>It no longer normalises CR/LF to spaces: <see cref="RequireNoControlCharacters"/> refuses them | ||
| /// at the sink first, so the replacement was unreachable — and silently rewriting a caller's value was | ||
| /// the wrong behaviour anyway.</para> | ||
| /// </summary> | ||
| public static string SystemdValue(string value) => value.Replace("%", "%%"); | ||
|
|
||
| /// <summary> | ||
| /// Rejects a VALUE that XML 1.0 cannot represent, for the plist writer. | ||
| /// | ||
| /// <para>Entity escaping does not help here, and that is the point: <c>SecurityElement.Escape</c> turns | ||
| /// <c>&</c> and <c><</c> into entities, but U+0001 has no legal XML 1.0 representation at all — | ||
| /// escaped or not. XML 1.0 permits only #x9, #xA, #xD and #x20 upwards. POSIX environment values, by | ||
| /// contrast, may contain any byte except NUL, so a captured or directly composed value really can carry | ||
| /// one.</para> | ||
| /// | ||
| /// <para>The consequence is availability rather than injection: the plist becomes unparseable, so | ||
| /// <c>launchctl</c> refuses to load it and the service cannot install or restart. Failing at write time | ||
| /// names the variable; failing at load time produces a service that silently does not exist.</para> | ||
| /// </summary> | ||
| public static void RequireXmlRepresentableValue(string name, string value) { | ||
| // The predicate is XmlConvert's, not char.IsControl — the two are NOT equivalent, and review caught | ||
| // both directions of the gap. char.IsControl accepts U+FFFE, U+FFFF and lone surrogates, none of | ||
| // which XML can carry (a lone surrogate is not even a scalar value, so the encoder either throws or | ||
| // substitutes), and it rejects U+007F–U+009F, which XML 1.0 permits. Deferring to the platform | ||
| // predicate makes the code match the doc comment above instead of approximating it. | ||
| // | ||
| // Scanned by index rather than foreach so a valid surrogate PAIR can be recognised as the one | ||
| // supplementary character it encodes; checking `char` in isolation would reject every emoji. | ||
| for (var i = 0; i < value.Length; i++) { | ||
| var c = value[i]; | ||
|
|
||
| if (char.IsHighSurrogate(c) && i + 1 < value.Length | ||
| && XmlConvert.IsXmlSurrogatePair(value[i + 1], c)) { | ||
| i++; // consumed the low half too | ||
| continue; | ||
| } | ||
|
|
||
| if (XmlConvert.IsXmlChar(c)) continue; | ||
|
|
||
| throw new InvalidOperationException( | ||
| $"Refusing to write a service unit: the value of '{name}' contains U+{(int)c:X4}, which " | ||
| + "XML 1.0 cannot represent even escaped — the resulting plist would not load. Unset or " | ||
| + "correct that variable, then re-run `kcap daemon service install`."); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Rejects an environment-variable NAME that no unit writer can carry safely. Shared by all three | ||
| /// sinks, because every one of them interpolates the name into a line whose structure the name can | ||
| /// break — and each format then hands the attacker something different. | ||
| /// | ||
| /// <para><b>systemd is the worst of the three, which is why this is central rather than per-writer.</b> | ||
| /// A key shaped like <c>SAFE=ok\nExecStartPre=/bin/touch /tmp/pwned\n#</c> with an ordinary value | ||
| /// renders a valid <c>Environment=SAFE=ok</c> line, then an attacker-chosen <c>ExecStartPre=</c> | ||
| /// directive, then comments out the remainder — a command the service runs on every restart. Value | ||
| /// escaping does not help: <c>SystemdValue</c> normalises the VALUE, and <c>EnvAssignment</c> decides | ||
| /// quoting from the VALUE. Neither looks at the name.</para> | ||
| /// | ||
| /// <para>The rule is the POSIX environment-name grammar (<c>[A-Za-z_][A-Za-z0-9_]*</c>) rather than a | ||
| /// blacklist of the characters each format happens to fear. A blacklist has to be right three times, | ||
| /// once per format, and a fourth writer would need it extended; an allowlist of what a legitimate name | ||
| /// can contain is right by construction. Nothing on any capture allowlist is excluded by it, so a | ||
| /// rejection means a caller composed the dictionary directly — which is exactly the path that bypasses | ||
| /// <see cref="ServiceEnvironment"/>.</para> | ||
| /// </summary> | ||
| public static void RequireValidEnvName(string name) { | ||
| var ok = name.Length > 0 | ||
| && (char.IsAsciiLetter(name[0]) || name[0] == '_') | ||
| && name.All(c => char.IsAsciiLetterOrDigit(c) || c == '_'); | ||
|
|
||
| if (ok) return; | ||
|
|
||
| throw new InvalidOperationException( | ||
| $"Refusing to write a service unit: '{name}' is not a valid environment variable name " | ||
| + "([A-Za-z_][A-Za-z0-9_]*). A name outside that grammar can break the unit's own line " | ||
| + "structure — in a systemd unit it can inject an ExecStartPre= directive that runs on every " | ||
| + "restart. No name from the capture allowlist is affected, so this one was added directly."); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Rejects a value carrying a C0 control character or DEL, for the systemd writer. | ||
| /// | ||
| /// <para>Quoting does not make a raw control byte valid unit syntax, and this writer has no encoder for | ||
| /// one: <see cref="SystemdValue"/> handles expansion, <c>Esc</c> handles the backslash and the double | ||
| /// quote, and neither touches U+0001, a backspace or a vertical tab — all of which a POSIX environment | ||
| /// value, a filename or an argv string may legally contain. systemd does document C-style and hex | ||
| /// escapes, but this code has no way to exercise a real systemd parser, so it refuses the input rather | ||
| /// than shipping an escape whose behaviour it has only read about. That is the same call already made for | ||
| /// the bare <c>;</c> separator, and the same reason.</para> | ||
| /// | ||
| /// <para>CR and LF are included, which supersedes the old newline-to-space normalisation in | ||
| /// <see cref="SystemdValue"/>: silently rewriting a value the caller supplied is worse than declining to | ||
| /// write it, because a service that runs with a value nobody chose is harder to diagnose than an install | ||
| /// that failed and said which variable was at fault.</para> | ||
| /// </summary> | ||
| public static void RequireNoControlCharacters(string name, string value) { | ||
| foreach (var c in value) { | ||
| if (!char.IsControl(c) || c > '\u007F') continue; | ||
|
|
||
| throw new InvalidOperationException( | ||
| $"Refusing to write a systemd unit: the value of '{name}' contains the control character " | ||
| + $"U+{(int)c:X4}. A unit directive has no encoding for one that this code can verify, and " | ||
| + "quoting does not make it valid syntax — systemd would refuse to load the unit. Unset or " | ||
| + "correct that variable, then re-run `kcap daemon service install`."); | ||
| } | ||
| } |
There was a problem hiding this comment.
1. Verbose servicetext doc comments 📘 Rule violation ⚙ Maintainability
The PR introduces multi-paragraph explanatory comments in ServiceText that significantly exceed concise documentation and duplicate information that could be captured via clearer naming/factoring. This increases maintenance burden and reduces readability, violating the “keep comments minimal” requirement.
Agent Prompt
## Issue description
`ServiceText` contains very long, multi-paragraph comments that reduce readability and increase future maintenance cost.
## Issue Context
PR Compliance requires keeping comments minimal and preferring self-explanatory code; these comments are substantially longer than necessary for API docs.
## Fix Focus Areas
- src/Capacitor.Cli/Services/ServiceText.cs[22-139]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Collaborator
Author
There was a problem hiding this comment.
Declined, consistently with the same flag on AcpVendorDescriptor — leaving this thread open for a maintainer to overrule.
ServiceText is the file where this PR's security reasoning lives, and the comments are load-bearing in a specific way: each records a measured fact about a target parser that the code cannot express, and in several cases why the obvious simpler thing is wrong. RequireXmlRepresentableValue explains why the predicate is XmlConvert's and not char.IsControl — the two differ in both directions, which is a bug review caught. SystemdValue explains why % is doubled but $ is deliberately not, while QuoteArg doubles both — an asymmetry that looks like an oversight and is not. RequireNoControlCharacters records that the escape exists but is refused because nothing here can exercise a real systemd parser.
Delete those and the next reader's reasonable simplification reintroduces the defect. Naming and factoring cannot carry "systemd expands specifiers in Environment= but not variables."
🤖 Addressed by Claude Code
|
Code review by qodo was updated up to the latest commit a2dec46 |
Inserting a new member above an existing one detached the existing member's doc comment, leaving two back-to-back <summary> elements that the compiler concatenates into a single doc comment on whichever member follows. So `Guarded` carried RequireSafeWrapperPath's rationale and RequireSafeWrapperPath carried none; likewise SubstituteUnmatchableNames and ResolveRequestedModel. Mis-attributed docs in IDE tooltips and anything generating XML output. Found by Qodo. The same slip had already been caught once in ServiceText during this branch, so this adds a repo-wide check rather than a third one-off fix: grepping for `/// </summary>` immediately followed by `/// <summary>` finds every instance. Five remain in ImportCommand and CodexHookCommand, both untouched by this PR and left alone deliberately rather than widening a review-clean diff for a cosmetic issue in unrelated files. Also merges the two AcpVendorDescriptorTests blocks, which described the same golden test, into one summary.
This was referenced Sep 29, 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
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.
Gemini CLI as the fifth ACP hosted-agent vendor, after Cursor, Copilot, Claude/Codex and Kiro. Interactive hosting only — the unattended reviewer is AI-1413, withheld here for the same structural reason it was for Kiro.
Spec:
docs/superpowers/specs/2026-07-31-ai899-gemini-acp-hosted-agent-design.md(rev 6, in this branch). Two codex spec-review rounds (9 findings, then 7) plus a gating security probe before implementation; then eight codex code-review rounds, clean at round 8. Every finding was real, and rev 6 of the spec records the two where rev 5's design was wrong rather than its prose — §3.1d and §4.2b.What ships
--experimental-acp --skip-trust --allowed-mcp-server-names <per-launch unguessable>,SupportsUnattended: false,NoOpModelSelector,SupportsMcpServers: falseAcpHostedAgentRuntimeFactoryline inDaemonRunnerDiagnosticAuthHintthat suggests rather than diagnoses--skip-trustis required, and it is NOT containmentGemini refuses a headless turn in an untrusted directory outright —
exit 55, before any model call — and a daemon worktree cannot be assumed pre-trusted, so without the flag every launch fails.What it does not do is protect anything. Measured: trust INHERITS from a trusted parent directory, and daemon worktrees live at
<repo>/.capacitor/worktrees/agent-…, inside the operator's repository. An operator who has trusted their own repo gives every hosted Gemini agent inherited trust over the checked-out branch's configuration, and passing--skip-trustdoes not withdraw it.Under that inherited trust, a repository-authored
.gemini/settings.jsonMCP server was observed starting on the ACP path: repo-controlled process execution under the daemon user, before the model acts, on a branch that may be contributor-authored. That is why the MCP allowlist is in the argv — reduced to a single non-matching name, it permits nothing and blocks it. Verified: repo-authored server blocked, an injected server still loads.The name must be generated, not written down (§3.1d). The first version used the literal
kcap-nonewith a comment calling it "a name no MCP server will ever have" — nothing enforced that, and measured, a repository naming its serverkcap-noneexecutes. This is a public repo, so the constant is the bypass. The descriptor now carries a placeholder thatAcpHostedAgentRuntimeFactoryreplaces per launch withkcap-deny-{guid}. Verified in both directions: a repo naming its server the placeholder itself is blocked, and an ACP-injected server still loads.Repo-authored hooks were separately measured not to run on the ACP path, though they do on the
--promptpath. The two paths differ and neither predicts the other — an earlier revision of this spec concluded from the--promptpath that hooks were an unclampable blocker, and was wrong.The allowlist contents are coupled to
SupportsMcpServersA non-matching name permits nothing, which is correct only while nothing is injected. The day the stdio call-level probe flips
SupportsMcpServerstotrue, the list must become the injected server names in the same change, or hosted Gemini ships with MCP silently broken.Gemini_McpAllowlist_IsCoupledToSupportsMcpServersasserts both directions, and it is mutation-proven: flipping the flag alone fails exactly it plus the constants test.(
--allowed-mcp-server-names ""is not usable — it crashes Gemini's config load before session start, which also makes it a trap when verifying by hand.)The daemon does not see your shell
A supervised daemon inherits nothing from an interactive shell, so
export GOOGLE_CLOUD_PROJECT=…in.zshrcis invisible to a hosted Gemini agent — and Gemini reports the absence asIneligibleTierError: … no longer supported for Gemini Code Assist for individuals, thrown by a function namedthrowIneligibleOrProjectIdError, which emits the same text for a missing project id. That message cost this investigation two retracted conclusions.Both project spellings are carried, because Gemini's own error says "The GOOGLE_CLOUD_PROJECT (or GOOGLE_CLOUD_PROJECT_ID) environment variable must be set".
Platform split, as a principle rather than per variable: persist a value into a unit only when there is no non-persistent alternative and the platform gives an owner-only guarantee this code actually enforces.
GOOGLE_CLOUD_PROJECT,GOOGLE_CLOUD_PROJECT_ID,GOOGLE_CLOUD_LOCATION,GOOGLE_GENAI_USE_VERTEXAI,GOOGLE_GENAI_USE_GCAGOOGLE_APPLICATION_CREDENTIALS,GOOGLE_GEMINI_BASE_URL,GOOGLE_VERTEX_BASE_URLGOOGLE_API_KEY,GOOGLE_CREDENTIALSThe middle column is secret-capable: a credential path discloses where the credential is, and a URL can carry userinfo or a query token. On Unix that is bounded by a guarantee this code enforces —
ServiceFileswrites0600, re-checks the mode on the open handle (umask filtersUnixCreateMode, so the request is not the result), and refuses a group/other-writable directory. Every one of those paths returns early on Windows ("ACL-governed, inherited from the user profile"), so there is no equivalent guarantee and those three are excluded there — exactly asKCAP_COPILOT_TOKEN_CMDis.The pre-existing holes closed on the way — this is most of the diff
Carrying eight new variables into a service unit meant reading what the unit writers actually do with a value. They were unsafe in ways that had nothing to do with Gemini, so this closes them rather than stepping around them. Review found six of these; the pattern is one thing, repeated.
The check is at the sink, deliberately, not at
ServiceEnvironment.Build. Build is not a boundary:DaemonCommandsaddsKCAP_DAEMON_SUPERVISEDto the dictionary afterCapturereturns, so a value already reaches the writers without passing through it. The tests call the writers directly for the same reason — routing throughBuildwould pass even if a writer were defenceless — and one case arrives by that exact bypass route.Some values guarded, their neighbours not
The unit of the bug is the artifact, not the value. Every writer interpolates several values into one line or file, and each had a subset covered:
.cmdexec lineExtraArgstokenset "K=V"Environment=ExecStartPre=that runs on every restartExecStart=ProgramArguments,StandardOut/ErrorPathEach sink now has one guard-then-escape helper that every interpolation goes through — applied even to values provably safe today, so the next value added inherits it.
Escaping one expansion without asking what else the format expands
CmdValuedoubling%was correct, and it stopped me looking. Both formats run more than one pass, and quoting is not a boundary in either:& | < > ( ) ^as metacharacters outside quotes, so quote-on-space emitted8&calc.exebare into a file the OS runs at every logon → every exec-line value is now quoted unconditionally.!NAME!delayed expansion works inside quotes and is registry-enablable machine-wide, so the execution mode is now part of the artifact (setlocal DisableDelayedExpansion+/v:off). Andcmd /c "<path>"relied on conditional quote-stripping that a path containing&defeats → the nested-quote/d /s /v:off /c ""<path>""form (/dalso skips AutoRun).%specifiers and$NAME/${NAME}inExecStart=, so a path under/home/50%offor containing${HOME}was rewritten in the executable position. Both are doubled and reversed byBinaryFromUnit.$is deliberately not doubled inEnvironment=, where systemd expands specifiers but not variables. The apostrophe is as structural to systemd's word lexer as the double quote and was missing fromNeedsQuote.Escape where verifiable; otherwise refuse
Nothing here can exercise a real systemd or cmd parser. So where the documented escape is unambiguous (
%%,$$,/snested quotes,/v:off) it is implemented and the tests assert the rendered text plus the round-trip. Where it is not — systemd's\;for a literal semicolon — the input is refused: a bare;is systemd's command separator, and a refusal cannot be subtly wrong. Same for all C0/DEL control characters at both systemd sinks, and for%in the path handed tocmd /c, where%%simply does not exist as an escape.That last point is worth naming: the same character needs different treatment at adjacent sinks.
%%is right in the wrapper's batch-file body and wrong on the Task XML command line. A per-character policy gets one of the two wrong.SystemdValue's old CR/LF→space rewrite is gone for the same reason: a service running with a value nobody chose is harder to diagnose than an install that failed and named the variable.And the XML predicate was not the XML predicate
char.IsControldiffers from XML 1.0 in both directions — it accepts U+FFFE/U+FFFF and lone surrogates (which no encoder can represent) and rejects U+007F–U+009F (which XML 1.0 permits). Replaced withXmlConvert.IsXmlCharplus surrogate-pair awareness, so an emoji in a path is accepted and a malformed surrogate is not.One fix at the source, not just the sink
--max-agentsis the only caller-suppliedExtraArgsentry, arrives unparsed off the command line, and is numeric — sokcap daemon service install --max-agents "8&calc.exe"was persisting that string into a logon-executed file. It is now validated as a positive integer where it enters, independently of the sink hardening.The diagnostic suggests, it does not diagnose
GeminiAuthHintcarries two hedges and an explicit "or it may be unrelated", because the failure it attaches to often is. Reproducing Gemini's own confidently-wrong attribution would be worse than saying nothing. Asserted clause by clause rather than by keyword scan.Verification
AcpVendorDescriptorTestsAcpHostedAgentRuntimeFactoryTestsUnitWriterExpansionTests(new)XmlRepresentableValueTests(new)UnitWriterEnvNameTests(new)WindowsTaskUnitRepresentabilityTestsServiceEnvironmentTests,ServiceFilesTests,ServiceTextTests,SystemdUnitTests,WindowsTaskUnitTests,LaunchdUnitTests27 mutants, all detected. Each was applied to source, built, run, restored from git, and the restore verified by content — a
git diffcan hide a bad restore through alignment, which happened once during this work and silently flipped a different vendor's descriptor while reporting "0 deletions".A sample of the ones that matter:
SystemdValue/QuoteArgstop escaping%or$Unpercentbecomes identity (round-trip broken)setlocal DisableDelayedExpansionmoved after the env blockchar.IsControl--max-agentsunvalidatedGOOGLE_API_KEYadded to the capture allowlistPositive controls throughout, so no suite passes on a writer that simply rejects everything: an ordinary systemd unit still renders, a legal plist with an emoji log path still renders, a wrapper path with a space and an
&is still accepted, and an ACP-injected MCP server still loads.Deliberately not here
currentModelIdisauto-gemini-2.5, a placeholder-shaped id. With model selection withheld (NoOpModelSelector, becauseConfigOptionModelSelector's write half is unverified and fails silently), a hosted Gemini session reports an unpriceable model. Known and accepted, not discovered later.tools/call, not an inference from the advertisedmcpCapabilities(Kiro honours stdio without advertising it).kcap statusproject warning — specced (§4.3), not implemented here; it needs to read the installed unit rather than the process environment, which is its own plumbing. Follow-up.🤖 Generated with Claude Code