refactor: correct false comments and flatten conditionals in service

[bolichen97]

Problem / Motivation

src/kiro_crew/service/ carries comment debt that AGENTS.md § Code style
explicitly forbids, and two of the comments are outright false — which is
worse than stale, because a reader auditing the sudo surface from them reaches
the wrong conclusion:

• service/linux.py's module docstring and install() docstring both claimed
  the privileged writer is tee. tee appears nowhere in the module; the
  unit is written with sudo install. A reader tracing "what runs as root here"
  was being pointed at a program that does not exist on the path.
• service/linux.py's uninstall() said it used a "non-sudo test -e". The
  code calls Path.exists(); the only privileged test -e in the module is
  _seed_env_file's, for a different reason.
• Four task-log citations (#3463 ×2, #5285 ×2) and four
  "previously / used to / the previous implementation" narrations, both of which
  AGENTS.md names as prohibited comment content.

Alongside that, the module had a handful of small readability items: a
double-negated ternary idiom written four different ways, one print() built
from a conditional expression wrapping a nested ternary inside an f-string, a
set comprehension materialised for a single membership test, and a root check
open-coded in two functions that must never disagree.

Why it matters

A false comment on a privilege boundary is a security-review hazard: it is
exactly the text a reviewer or an auditor reads instead of re-deriving the
sudo surface, so an incomplete list becomes an incorrect audit. The tee claim
had already survived long enough that the spec (docs/system-specs/modules/cli.md)
had copied the same narrow claim.

The task-log citations are the reason AGENTS.md bans them: #3463 tells a
reader nothing at the point of use, while the rationale it stands in for is
already written out two lines away.

What changed (motivation → approach → change)

One module, one commit, behaviour-preserving throughout. Nothing in
service/apparmor.py is touched (see Scope below).

Comment accuracy — 3 false claims corrected:

• linux.py module docstring: replaced the systemctl/tee claim with the
  actual escalated set — systemctl, install, mkdir, rm, rmdir, test
  directly, plus apparmor_parser, aa-exec, and the setpriv + trusted
  system python3 that service/apparmor.py runs through the privileged
  helpers this module lends it. The load-bearing part is stated as the
  mechanism rather than as a list to be trusted, and it is pinned to the thing
  that actually secures the escalation: WHICH interpreter runs — root-owned,
  resolved from a fixed list of trusted system directories, never
  sys.executable, and running a constant stdlib snippet that does not import
  kiro_crew, so no MCP / LLM / agent code is reached deliberately — and the
  docstring now stops the claim exactly there, because the interpreter is not
  isolated. Invoked without -I/-S, CPython prepends the caller's working
  directory to sys.path and imports site, so code from that working
  directory, PYTHONPATH, a user-site .pth line, sitecustomize or
  usercustomize can run as root before or during the payload's own first import
  of ctypes. That is measured rather than assumed (see Local review), and the
  invocation itself lives in service/apparmor.py, which this commit leaves
  alone (see Scope) — the docstring records the boundary rather than moving it.
  setpriv is stated separately and without any de-privileging implication: it
  reuids to the account the installer was invoked as, which under
  sudo kirocrew service install is root, so it does not by itself make the
  probe unprivileged. Cites
  docs/system-specs/modules/security.md for the reasoning behind the AppArmor
  step's four tools.
• linux.py install(): drops its own (stale) enumeration and defers to that
  paragraph.
• linux.py uninstall(): names the check that actually runs, and states the
  operative permission — a stock /etc/systemd/system is traversable, so a
  plain stat answers without escalating — and contrasts it with
  _seed_env_file's privileged probe, which exists because Path.exists()
  raises PermissionError on a locked-down directory rather than answering.
• docs/system-specs/modules/cli.md carried the same narrow "Only install and
  systemctl invocations are elevated" claim. Corrected in this commit: fixing
  one half of a duplicated false claim and leaving the other is how it comes
  back. It now defers to the linux.py module docstring that sits beside the
  call sites rather than keeping a second copy of the set to drift — the same
  mechanism-plus-pointer form install() was given — and its own claim narrows
  to the elevated executables being stock system programs, which is what is
  actually true of them.

Comment hygiene — the 4 #NNNN citations and the 4 historical narrations
(macos.py _write_plist_atomic / write_live_program / restart,
live_target.py write_target / restore) are restated in present tense. Each
rewritten comment keeps its constraint and drops only the ticket number or the
"what it used to be": the atomic_write guarantees, the restrict_to_owner
ordering versus the planted-link check, the mode-before-content property, and
the reason unload + load cannot restart the agent from inside it are all
still there.

Structural — 5 sites, all behaviour-identical:

• controller.py remove_launcher_profile: print(X if cond else Y) with a
  nested ternary inside X → a plain if/else, matching its sibling
  install_launcher_profile.
• controller.py ×4: the warning-prefix ternaries all read A if not ok else B;
  inverted to test ok positively. The two prefix spellings differ in trailing
  width ('⚠️ ' two spaces vs '⚠️ ' one), and each stayed on its own side of
  the flipped condition — verified byte-for-byte.
• controller.py installed_service_has_managed_marker: a set comprehension
  built solely for one in test → any(line.strip() == expected for line in …).
• linux.py _require_privilege: the open-coded os.geteuid root check now
  asks _privilege_prefix(), whose non-empty return is "this call will shell
  out through sudo". The two functions can no longer drift into disagreeing
  about whether escalation is needed. Equivalence holds in all four input states
  (geteuid absent → ["sudo"] → truthy, matching the old not is_root;
  geteuid() == 0 → [] → falsy; geteuid() != 0 → truthy; non-Linux hits the
  unchanged early return above). os.geteuid is still reached through this
  module's global os, which is what the existing tests monkeypatch, and
  shutil.which is still short-circuited away when already root — so a
  root-with-no-sudo container still does not raise.

Scope

service/apparmor.py is deliberately untouched, though it holds 12 of the
module's #NNNN citations. It is the module's one security-control generator,
and there the ticket numbers carry a design-failure narrative (two earlier
attachment designs and why each was wrong) rather than being bare task-log
noise — rewriting that prose is a judgment call that deserves its own review,
not a line item in a readability sweep. The criterion is a property of the file,
so it is applied to the file as a whole rather than hunk by hunk.

The mechanical detector (shadow-import / chained-ternary) reports 0
actionable sites in this module on current origin/main, so everything here is
judgment-class work under the campaign's rules.

Tests

No test changes. This is a behaviour-preserving sweep, so the existing tests are
the assertion that nothing moved — in particular test/test_service.py's four
_privilege_prefix / _require_privilege cases, which patch svc_linux.os.geteuid
and therefore still drive the real logic through the new indirection.

• Targeted: test_service.py, test_live_target.py, test_spawn_audit.py,
  test_host_service_guard.py, test_doctor_sandbox_verdict.py,
  test_pod_launchd.py — 474 passed, and 365 passed again on the final
  tree after the review fixes.
• Full backend suite on Python 3.12: 84157 passed, 87 failed. Every failure is
  pre-existing on this host and none is in service/. Attribution was done by
  failure set, not count: the same 13 files fail identically on a pristine
  detached origin/main worktree (87 failed there). The single set difference
  (auto_improvement/tests/test_github_profile.py::TestSuiteMeasurement::test_canary_produces_a_correctly_signed_win)
  passes standalone on both trees and did not fail in the branch's own full
  run, so it is order-dependent on this host and not attributable to the diff.
  The host lacks a usable user-namespace sandbox, which is what reds the
  subprocess-spawning families listed.

Manual verification

N/A — unit coverage sufficient. There is no reachable behaviour change to
exercise: after stripping docstrings, live_target.py and macos.py are
AST-identical to origin/main, and the five remaining rewrites are
expression-level and output-identical.

Local review

Four parallel reviewers ran before this PR opened, each on a distinct lens
(AUTOSDE blocking rules; behaviour equivalence; comment accuracy; security
keystone / boot path / import semantics).

Acted on — 3 findings, all in text this PR itself introduced:

1. The first draft of the linux.py sudo-scope paragraph asserted an
   exhaustive list that omitted aa-exec, apparmor_parser, setpriv and
   python3. Raised independently by three of the four lenses, verified against
   apparmor.py's verify_enforcement argv and security.md, and fixed — the
   paragraph now names them and states the de-privileging mechanism, which is
   the part the incomplete list was hiding. Replacing one wrong claim with a
   differently-wrong one would have been the worst outcome here.
2. The uninstall() comment said "world-readable" where the operative
   permission is the traverse bit, and did not acknowledge that Path.exists()
   propagates PermissionError. Corrected.
3. docs/system-specs/modules/cli.md mirrored the same stale claim. Corrected
   in this commit rather than deferred.

A verifier then re-checked those three fixes and found two new inaccuracies
that the fixes themselves had introduced, both confirmed against the code and
both fixed in the same commit:

4. The fix for (1) claimed security.md was "the authoritative list". It is not:
   it names the four AppArmor tools and sudo install / sudo systemctl, and
   never mkdir, rmdir or test. Both new pointers (linux.py and cli.md)
   now cite it for the reasoning, which is what it actually carries. Pointing a
   reader at a subset while calling it authoritative is the same defect class as
   the tee claim this PR started from.
5. The fix for (2) dropped a version scope. _seed_env_file's own docstring
   scopes the PermissionError claim to Python 3.12, and on 3.14 Path.exists()
   swallows the error and answers False instead — so the unscoped restatement
   was false on a supported interpreter (requires-python = ">=3.12"). Reworded
   to state the consequence ("cannot answer trustworthily") and defer the
   mechanism to that docstring.

Dropped — none. Both behaviour lenses returned no findings with per-rewrite
equivalence proofs (independently confirming the byte-width of the two ⚠️
prefixes and the four-state truth table for _require_privilege), and the
AUTOSDE lens returned no findings.

One finding I initially deferred and then fixed. The verifier noted that
setpriv --reuid=<uid> does not actually drop privilege under
sudo kirocrew service install, where os.getuid() is already 0, so
"setpriv drops back to the invoking uid/gid" reads as a de-privileging
guarantee it does not give. I first deferred it as pre-existing (the same claim
sits unchanged in apparmor.py, install_apparmor_profile, and security.md).
GPT 5.6 then raised it independently on the PR against text this PR wrote —
which settles it the other way: rebutting an accuracy finding on a technicality,
in a PR whose entire premise is making these comments true, would have been
inconsistent. The docstring now separates the guarantee (stated on what actually
secures it: a root-owned, trusted-directory interpreter that is never
sys.executable, running a constant snippet that does not import kiro_crew)
from setpriv (stated without the implication). The underlying behaviour —
that apparmor.py's "the probe must run unprivileged" constraint is unmet under
a sudo-invoked install — is a real pre-existing defect flagged for a
maintainer; fixing it is a behaviour change and does not belong in a
behaviour-preserving sweep.

Round 2 — 3 more findings in this PR's own text, all fixed. GPT 5.6 passed
the previous head with no blocking findings but left two advisory ones, and both
were legitimate on exactly the axis this PR exists to fix, so both were fixed
rather than rebutted:

6. linux.py asserted "no user-writable code ... ever runs as root". False:
   the escalated interpreter runs without -I/-S, so CPython's own startup
   imports site. Measured on /usr/bin/python3: with a sitecustomize.py
   and a usercustomize.py planted on a relocated per-user site path, both
   executed before the -c payload, and adding -S -I suppressed both.
7. macos.py _write_plist_atomic enumerated the interruption outcomes as
   "either the old plist or no plist at all", omitting the complete new plist
   that an interruption after os.replace leaves. All three are now named.

A re-review of fix (6) then found it still overstated — the same
one-wrong-claim-for-another trap as (1) and (4):

8. Scoping the guarantee to the payload is not enough, because the payload's own
   first line is import ctypes. With sys.path[0] == '' under -c, a planted
   ctypes.py in the caller's working directory is what that import
   resolves to — measured, and -I restores the stdlib module — and a user-site
   .pth line is a further omitted hook. The docstring now names all four
   vectors (working directory, PYTHONPATH, .pth, sitecustomize /
   usercustomize) and claims only that no MCP / LLM / agent code is reached
   deliberately. cli.md's "never kirocrew code" narrowed with it.

Both behaviour lenses again returned no findings on the final tree, one of them
re-deriving the _require_privilege / _privilege_prefix equivalence
independently, and both judged that disclosing the residual exposure is the
right call for a comment-accuracy PR rather than an obligation to change the
invocation.

Round 3 — the paragraph was restructured, not patched again. GPT passed the
next head with no blocking findings and one advisory: the phrase "safe to escalate" contradicts the root execution the same paragraph goes on to
acknowledge. Correct — and it was the fourth round on that one paragraph
(span=95458d4eeff1, whose identity is path + lane, so all four share it).
All four had one shape rather than four causes: an affirmative safety claim
broader than the code supports. Round 1 read setpriv as a de-privileging
guarantee; round 2 asserted no user-writable code ever runs as root; round 3
scoped that to the payload, missing that the payload's own first line is
import ctypes; round 4 was the word safe. So the invariant changed rather
than the sentence: the paragraph now makes no safety claim at all, stating
only what each mechanism buys — trusted resolution rules out escalating the
user-writable venv interpreter and says nothing about what it then loads,
setpriv buys nothing under a sudo-invoked install, the payload is what is
bounded — with the four open import hooks named as facts rather than as caveats
on a promise.

Flagging for a maintainer, second item. The working-directory vector is a
concrete unprivileged trigger, unlike the site-path ones: _sudo_capture pins
no cwd, so cd /tmp && sudo kirocrew service install resolves the probe's
import ctypes against a world-writable directory. Passing -I to the trusted
python3 in apparmor.verify_enforcement closes all four vectors in one token
and the payload is pure stdlib, but that edits a file this PR declares out of
scope and changes a security control's invocation, so it wants its own commit,
its own test, and its own review — not a line in a behaviour-preserving sweep.

CI

Backend Tests (Windows) was red on an earlier head for a reason that was
main's, not this diff's — all 5 failures were in test/test_autonudge_stop_auth.py,
reproducing byte-identically in a pristine detached origin/main worktree. All four
Windows shards are green on the current head after the rebase, so nothing is
outstanding there and nothing was weakened, skipped, or re-run to get it.

Dependency Audit / Audit Production Dependencies is the one red left, and it
is not this diff's. Every attempt fails identically on npm audit timed out after 120s for website/package-lock.json — a tool timeout that the gate turns
into a failure by failing closed, never a reported advisory. Three independent
lines of evidence put it on main:

• This diff cannot reach it. It touches five files — one spec markdown and
  four modules under src/kiro_crew/service/ — and no package.json,
  package-lock.json, or Python dependency manifest. The audited tree is
  main's own; this PR's merge ref is a strict subset of it (it predates
  html-to-image, which main added after this branch's base).
• The same job is red repo-wide right now — PRs fix(dashboard): name the action for the two unrecoverable mobile-link 403s #8306, feat(chat): add inline paste token composer #8310, fix(chat): fire redaction notice on the 7 exception-path assistant persists #8318, feat: skip the session reset when kiro-cli hot-reloads MCP config #8319
  and test(autonudge): pin the gate premise the paused-loop tests rely on #8321 all carry the identical failure, and the successes on fix(electron): grant the dashboard the notifications permission so its OS toasts work #8312, feat(agents): template selector becomes the definition panel's header with private fork-on-edit #8307
  and fix(snapshot): report a refused cron merge to the import caller #8315 all land inside one good 01:00–01:26 window. Identical input,
  different outcome, is timing.
• The lockfile itself is over budget. Running the job's exact command
  (npm audit --omit=dev --package-lock-only --ignore-scripts --audit-level=high) against this merge ref's lockfile and against current
  main's produces the same result — both exceed even a 220s budget. So the
  120s ceiling is marginal for this lockfile against the live registry, which is
  what makes the gate flaky for everyone rather than wrong about this PR.

The job was re-run 10 times rather than touched. The two things that would turn
it green from here — raising the timeout, or adding a
.vulnerability-exceptions.json entry — both weaken a security gate, so neither
belongs in a behaviour-preserving comment sweep. Flagging for a maintainer: this
ceiling needs raising (or the audit needs a retry) in its own change.

Screenshots / video

Why no screenshot: backend-only — the diff touches four Python files under
src/kiro_crew/service/ plus one spec markdown file, renders no UI, and changes
no user-visible string; the four print() rewrites are byte-identical in output.

Related Issues

no linked issue: this is a scheduled module-simplification sweep, not the fix for
a tracked report, so nothing here should close on merge.

It files one, though: #8414 tracks the cause-level fix for the escalation
boundary this PR only documents (deferred-finding, assignee @bolichen97,
Due: 2026-09-18). It must not be closed by this PR — the remedy it names
edits service/apparmor.py, which this commit declares out of scope.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title
• Existing tests pass and new tests added for new functionality (N/A — no new functionality)
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable) — docs/system-specs/modules/cli.md
• No secrets, credentials, or internal references in the diff

[bolichen97]

Backend Tests (Windows) (1) red — rebutted: pre-existing on main, not this diff.

The shard's 5 failures are all in test/test_autonudge_stop_auth.py:

test_applier_owed_blocked_turn_is_not_reported_as_a_merge
test_applier_owed_terminal_turn_is_not_reported_as_a_spent_cap
test_applier_settled_terminal_loop_is_not_reported_as_a_manual_pause
test_applier_a_settled_outcome_outranks_a_stale_owed_turn
test_applier_a_spent_cap_with_no_terminal_news_still_revives

with AssertionError: assert 'merged' in 'monitor_update cannot apply legacy fields to a structured monitor: max_cycles'.

Evidence it is not attributable to this PR:

1. This diff touches none of it. The changed files are service/{controller,linux,live_target,macos}.py and docs/system-specs/modules/cli.md — nothing under dashboard/, nothing monitor- or autonudge-related.
2. The same 5 node ids fail on a pristine origin/main. I ran them in a detached origin/main worktree at this PR's base (b2320e02c) with the same Python 3.12 venv: identical 5 failures, byte-identical node id set. They also still fail on a newer main (b9a65bedf), so no later commit has fixed them.
3. Deterministic, not a flake — 5/5 every run, so re-running the job would not clear it.

Root cause, for whoever picks it up: #8201 ("report a terminal subject in monitor_update, not the spent bound") is the last commit to touch both sides — the refusal string in src/kiro_crew/dashboard/session_directive_apply.py:552 and this test file — landing alongside #5184's structured-monitor path. The applier now refuses max_cycles/message as legacy fields on a structured monitor, so these tests never reach the branch whose message they assert on. Each PR was green against its own merge ref; the incompatibility only exists in the merged result, which is why nothing went red before the merge.

Per this repo's conventions I have neither weakened the assertions nor added a re-run, and fixing it here is out of scope (one module per PR, and it is not this module). Flagging for a maintainer: this reds shard 1 for every open PR until it is fixed on main.

Disposition: rebutted (CI failure not caused by this diff) — head aa017b236.

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

Design-level review of 7173cf6d6800468e2a0f09dca5d4115c238ec703 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The verification checks out: tee is indeed absent from linux.py (the unit is written with sudo install), the _privilege_prefix/_require_privilege unification is truth-table equivalent, and the corrected docstrings match the code. But I found one real inconsistency the PR leaves behind: apparmor.py:425-426 and security.md still carry the "setpriv drops back to the invoking uid/gid" and "user-writable site-packages never runs with privilege" claims that this PR's new linux.py docstring correctly contradicts.

Design-Verdict: CONCERNS

Corrections are accurate and verified, but the repo now states two contradictory accounts of the same escalation boundary, with the false one in the owning module.

Watch

• The new linux.py docstring correctly says setpriv "reuids to 0, which is a no-op" under sudo kirocrew service install and that site hooks run as root — while untouched apparmor.py ("setpriv drops back to the invoking uid/gid inside the profile before the probe") and security.md ("user-writable site-packages never runs with privilege") still assert the opposite. The PR's own rationale for fixing cli.md — "fixing one half of a duplicated false claim and leaving the other is how it comes back" — applies verbatim here, and security: AppArmor probe escalates python3 without -I, so a planted CWD module runs as root #8414 tracks the -I behavior fix, not these prose claims. An auditor starting from apparmor.py or security.md still reaches the wrong conclusion, the exact harm this PR names.
• The 35-line sudo-scope analysis lives in linux.py's docstring but describes an invocation owned by apparmor.py; when security: AppArmor probe escalates python3 without -I, so a planted CWD module runs as root #8414 lands -I, this paragraph is a second copy that must be remembered and updated.

Suggestions

• Apply the same accuracy correction to security.md's setpriv/payload sentence (and apparmor.py's two lines) in this sweep or a tracked docs follow-up — it is the same defect class as the tee and cli.md fixes already in scope.

[DESIGN-REVIEWED] 7173cf6

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 7173cf6d6800468e2a0f09dca5d4115c238ec703 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 7173cf6

Verdict parsed from the review's SHA-scoped output markers for commit 7173cf6d6800468e2a0f09dca5d4115c238ec703.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 7173cf6d6800468e2a0f09dca5d4115c238ec703: <one-sentence reason>

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 7173cf6d6800468e2a0f09dca5d4115c238ec703 and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/service/linux.py:39 -- sudo kirocrew sandbox install-profile also passes UID 0 through setpriv, contradicting "only on that path" -> Fix: scope this to either installer invoked as root.
[GPT-REVIEWED] 7173cf6

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 7173cf6d6800468e2a0f09dca5d4115c238ec703: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 7173cf6d6800468e2a0f09dca5d4115c238ec703 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All claims verified. tee appears nowhere in service/linux.py (the old docstring was indeed false); the actual escalated set matches the new docstring (_sudo_run calls at linux.py:365,420,468,470,682,690,691 cover systemctl, install, test, mkdir, rm, rmdir; apparmor.py adds apparmor_parser, aa-exec, setpriv, trusted python3). The _privilege_prefix delegation is equivalent in all four input states. Sibling sweeps: zero if not X.ok else ternaries remain in src/, zero historical narrations remain in service/, and the 12 remaining #NNNN citations are all in apparmor.py, exactly as the description declares.

First-Principles-Verdict: PASS

Corrects three provably false comments on a sudo boundary and removes the drift mechanism that produced one of them; every item is verified behavior-identical.

What this change ships

Intent: make the service module's comments tell an auditor the truth about what runs as root, per the AGENTS.md comment rules — a FIX.

1. cli.md's "only install and systemctl are elevated" replaced by a pointer to the code-adjacent docstring — justified (claim was false; dedups the drifting copy)
2. linux.py module docstring names the real escalated set and what setpriv/trusted resolution do and don't guarantee — justified (verified against the six _sudo_run call sites)
3. install() docstring defers to the module docstring instead of keeping its own stale list — justified
4. uninstall() comment names the actual Path.exists() probe, not a nonexistent test -e — justified (was false)
5. Four #NNNN task-log citations removed — justified (documented AGENTS.md prohibition)
6. Five "previously/used to" narrations restated in present tense — justified (same mandate; 0 remain in scope)
7. _require_privilege now derives "needs sudo" from _privilege_prefix() — justified, cause-level (removes the drift, equivalence verified)
8. Four warning-prefix ternaries inverted to test ok positively — declared, byte-identical output
9. Nested-ternary print flattened to if/else — declared, behavior-identical
10. Marker-check set comprehension → any() — declared, behavior-identical

Watch

Two declared deferrals a human should see, both counted: apparmor.py keeps 12 #NNNN citations (grepped #\d{4}) under a stated file-level criterion, and the interpreter-preload gap the new docstring records has its cause-level fix living in the untouched apparmor.py invocation — the description states the level this sits at, so nothing is hidden.

[FIRST-PRINCIPLES-REVIEWED] 7173cf6

[bolichen97]

• src/kiro_crew/service/linux.py:19 (span=95458d4eeff1) — "setpriv drops back" falsely describes the probe as deprivileged — fixed in 874671fa3.

FINDING -- src/kiro_crew/service/linux.py:19 -- sudo kirocrew service install passes UID/GID 0, so "setpriv drops back" falsely describes the probe as deprivileged -> Fix: state that it uses the caller-supplied UID/GID and may remain root. (origin: validation)

Legitimate and in scope. The sentence was literally true — setpriv --reuid=$(os.getuid()) does reuid to the invoking uid — but under sudo kirocrew service install that uid is 0, and "drops back" reads as a de-privileging guarantee it does not provide. That is precisely the failure mode this PR exists to remove, so rebutting it on a technicality in an accuracy PR would have been inconsistent. My own pre-PR verifier raised the same point and I had deferred it as pre-existing; two independent reviewers landing on text this PR authored settles it the other way.

The docstring now separates the two claims instead of conflating them:

The guarantee, stated on what actually secures it — the escalated interpreter is root-owned, resolved from a fixed list of trusted system directories, never sys.executable (the venv python is user-writable), and its payload is a constant stdlib snippet that does not import kiro_crew. This is what makes "no user-writable code and no MCP / LLM / agent import ever runs as root" true, and it holds under every invocation.
setpriv, stated without the implication — it reuids to the account the installer was invoked as, which under sudo is root, so it does not by itself make the probe unprivileged, and the guarantee above does not rest on it.

Scope note: the underlying behaviour — apparmor.py's constraint that "the probe itself must run UNPRIVILEGED, or it proves nothing" is not met under a sudo-invoked install — is a real pre-existing defect, and the same claim appears unchanged in apparmor.py, linux.py's install_apparmor_profile, and docs/system-specs/modules/security.md. Fixing the probe is a behaviour change and belongs in its own PR, not in a behaviour-preserving sweep; this commit makes the defect visible rather than asserting it away. Flagging it for a maintainer.

[bolichen97]

• src/kiro_crew/service/linux.py:20 (span=95458d4eeff1) — "safe to escalate" contradicts the root execution the same paragraph acknowledges — fixed in 9589701b9.

FINDING -- src/kiro_crew/service/linux.py:20 -- "safe to escalate"
contradicts the acknowledged root execution through import paths -> Fix: state
that trusted resolution only avoids the user-writable venv interpreter.
(origin: validation)

Legitimate, and the sharpest reading of the four rounds this paragraph has now taken. The sentence opened by asserting a safety property and the next three sentences withdrew it — so the paragraph argued against itself, which is worse than either half alone.

Not point-patched, because this span has now absorbed four rounds and the wording fix would have been the fourth patch. Every finding on it has had the same shape rather than four different causes: an affirmative safety claim broader than the code supports. Round 1 read setpriv as a de-privileging guarantee. Round 2 asserted no user-writable code ever runs as root. Round 3 scoped that to the payload, which misses that the payload's own first line is import ctypes. Round 4 is the word safe itself. None of them attacked a fact; all four attacked a claim.

So the invariant, not the sentence, is what changed: the paragraph now makes no safety claim at all, which is what makes the whole class unreachable rather than moving it one clause further along. It states what each mechanism buys and stops there — trusted resolution rules out escalating the user-writable venv interpreter and says nothing about what that interpreter then loads; setpriv buys nothing here, since it reuids to the account the installer was invoked as, which under sudo kirocrew service install is root; the payload is what is actually bounded, being a constant stdlib snippet importing no kiro_crew. The four import hooks that stay open (working directory, PYTHONPATH, user-site .pth, sitecustomize / usercustomize) are named as facts rather than as caveats on a promise. No "safe", no "guarantee", no "so no X ever happens" remains in it.

The cause-level fix — passing -I to the trusted python3, which closes all four at once — is tracked in #8414 (deferred-finding, assignee @bolichen97, Due: 2026-09-18) and deliberately not in this PR: it edits service/apparmor.py, which this commit's body declares out of scope, and it changes a security control's invocation, so it wants its own test and review round. That deferral is a maintainer decision on this PR, not my own call.

Diff is comment text only: the module's AST is byte-identical to 366e8937f once docstrings are blanked, and the full local gate (flake8, mypy --platform linux, the black baseline gate, docs-lint) is green on 9589701b9.

[bolichen97]

• src/kiro_crew/service/linux.py:34,492 (span=95458d4eeff1) — "buys nothing at all" and "No ... code runs under sudo" are each false on one of the two live install paths — fixed in 8dcfb7973.

FINDING -- src/kiro_crew/service/linux.py:34,492 -- ordinary installs make
setpriv drop sudo's root UID, while Python startup hooks can run code as
root, contradicting "buys nothing at all" and "No ... code runs under sudo" -> Fix: scope the first claim to sudo-invoked installs and qualify the
second with "deliberately".

Legitimate on both counts, and verified against the code rather than accepted on the reviewer's word: linux.py:625 passes os.getuid() into apparmor.install, so there are two live install paths and the previous revision described only one.

install path	os.getuid()	what setpriv --reuid= does
kirocrew service install as an ordinary user — the default, where this module escalates individual commands through sudo	the user's uid	reuids from sudo's root back to that user; the probe and anything it loads stay unprivileged
sudo kirocrew service install	0	reuids to 0, a no-op — only here does loaded code run as root

This is the fifth round on this span, and the previous four all had one shape: a claim broader than the code supports. Round 4's restructure removed the affirmative safety claims but then over-corrected into the opposite overstatement — the reviewer's own warning that widening a fix is itself a change needing a check for the OPPOSITE failure mode, walked into directly.

A third site is fixed here that the finding does not name, because the invariant demanded it: the hooks sentence said the planted code "runs as root" with no path qualifier, which is false on the default path for exactly the reason the setpriv sentence was. Fixing only the two quoted sites would have left the same defect sitting in the same span for a sixth round.

The invariant now holding, which is what makes the class unreachable rather than moving it one clause along: every claim about a mechanism in this section names the install path it holds on. The section is reorganized around that axis and separates two questions that were tangled — WHAT gets loaded (trusted resolution says nothing about it, on either path) from WHOSE privileges it gets (setpriv's call, root only on the sudo-invoked path). install()'s docstring takes the deliberately qualifier and now also points at what the escalated interpreter can load on its own.

The cause-level fix — passing -I, which closes the working-directory, PYTHONPATH, .pth and sitecustomize / usercustomize hooks in one token — remains tracked in #8414 and out of scope here by maintainer decision, since it edits service/apparmor.py and changes a security control's invocation.

Diff is comment text only: linux.py's AST is byte-identical to 9589701b9 once docstrings are blanked, and the full local gate (flake8, mypy --platform linux, the black baseline gate, docs-lint) is green on 8dcfb7973. The superseded head had itself reached readiness: passed with 55 green checks and zero red.

[bolichen97]

• src/kiro_crew/service/macos.py:175 (span=87929ff36195) — "would publish the plist world-readable" asserts unconditionally what the operator's umask decides — fixed in 7173cf6d6.

FINDING -- src/kiro_crew/service/macos.py:175 -- under umask 077,
"would publish the plist world-readable" contradicts the resulting 0600
mode -> Fix: change "would" to "could".

Legitimate, and measured rather than taken on the reviewer's word — atomic_write with no mode resolves to the umask default, so the outcome is the operator's, not this call's:

umask	resulting file mode	world-readable?
022 (common default)	0644	yes
077	0600	no

Fixed by stating the dependency rather than swapping the one word the finding suggested. could would have been true but uninformative: the reason mode=0o600 has to be passed explicitly is precisely that the permissions otherwise track a setting this code does not control, and that is what a reader auditing the plist's exposure needs to know. The sentence now says so.

A second site in the same docstring is fixed here that the finding does not name, applying the lesson from this PR's other span rather than waiting for a round to find it: the crash-outcome sentence opened with "leaves exactly one complete document at the final path" and then enumerated a branch — no prior plist, interrupted before the rename — where the path holds nothing. Same shape as the finding above, and as the five rounds the linux.py escalation paragraph took: a claim its own enumeration contradicts. It now leads with the negative guarantee (never a partial XML document launchctl load would reject) and lets the three outcomes follow, so there is no absolute left to falsify.

Diff is comment text only: macos.py's AST is byte-identical to 8dcfb7973 once docstrings are blanked, and the full local gate (flake8, mypy --platform linux, the black baseline gate, docs-lint) is green on 7173cf6d6. The superseded head had itself reached readiness: passed with 55 green checks and zero red.

[cixuuz]

and only on that path is under-inclusive — sudo kirocrew sandbox install-profile is a second live root path.

Both AppArmor entry points hand os.getuid() to the same setpriv probe, and neither is modified by this PR:

• install_apparmor_profile (L585) → apparmor.install(..., os.getuid(), os.getgid(), ...) (L631) — reached from service install
• install_launcher_profile (L643) → apparmor.install_launcher(..., os.getuid(), os.getgid(), ...) (L658) — reached from kirocrew sandbox install-profile via cli_server.py → service/controller.py::install_launcher_profile

Both converge on the one invocation in apparmor.py:

sudo aa-exec -p <profile> -- setpriv --reuid={uid} --regid={gid} --clear-groups -- python3 -c <snippet>

So the reuid is 0 whenever the installer process is root, and sandbox install-profile is a sudo command by its own --help: "Attach the userns AppArmor profile to this app (sudo on Linux)".

What makes this worth fixing rather than a nit: the paragraph opens by saying the setpriv guarantee "depends on which install path is running" and that "both install paths are live", then names only one as reaching uid 0 — so the omission lands exactly on the distinction the paragraph exists to draw. install_launcher_profile's own docstring further down this file already records that it is invoked explicitly rather than from service install, so the module docstring and that function currently disagree about how many escalating paths there are.

Suggested wording:

started under sudo — either sudo kirocrew service install or sudo kirocrew sandbox install-profile — it reuids to 0, which is a no-op, and on those paths the loaded code runs as root.

Flagging it because this PR's stated purpose is correcting false comments in a module people read to audit what runs as root, which makes a newly-introduced narrow claim here costlier than elsewhere.

[chenmingwei23]

Approving: PR Readiness green (the repo's only required check), no failing lanes, MERGEABLE.