Skip to content

fix(usage): log the transcripts the UNC gate refuses instead of a silent zero - #7285

Merged
iamwhatever merged 1 commit into
mainfrom
fix/unc-home-usage-validate-6733
Sep 3, 2026
Merged

fix(usage): log the transcripts the UNC gate refuses instead of a silent zero#7285
iamwhatever merged 1 commit into
mainfrom
fix/unc-home-usage-validate-6733

Conversation

@chenmingwei23

@chenmingwei23 chenmingwei23 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

On a Windows roaming profile the home directory is itself a UNC share, so
hooks.unc_probe_allowed gates every path under it. It admits the crew data home,
the temp directory and the kiro agents dir, but not <kiro home>/sessions/cli.
So dashboard.handlers.usage._parse_sessions has every kiro-cli transcript refused
by validate_file_path, continues on the None, and the usage page renders a
confident zero with nothing anywhere -- log, payload or UI -- to say why.

That is two harms: a wrong number, and silence about it. This PR fixes only the
silence.
The scope was reduced during review; see "What is deliberately not here".

Why it matters

A zero on the usage page is indistinguishable from an idle account. An operator on a
roaming profile has no way to tell that the page is reporting a refusal rather than
a fact, and no thread to pull -- nothing is logged, so there is nothing to search
for. After this change the same zero is accompanied by a server-side warning naming
the directory and the number of transcripts excluded, which is enough to recognise
the platform problem and find #8079.

What changed

One counter and one log record in _parse_sessions:

  • Refusals from validate_file_path are counted instead of silently skipped.
  • After the loop, if any were refused, one logger.warning reports the count and
    the directory.

Three deliberate details:

  • Aggregated, not per file. The failure mode refuses all transcripts at once,
    so a per-file record would emit one line per session file for a single cause.
  • Not a response field. No renderer reads it, so adding it to the payload would
    ship an unused API surface; the test pins that it stays absent.
  • Server-side only. %s of a Path is a filesystem path, and the returned
    payload deliberately never carries one (same reason as the existing iterdir
    error handler just above).

Tests

test/test_usage.py, run with pytest test/test_usage.py -n 2 -- 91 passed.

  • test_refused_transcripts_are_reported_not_swallowed -- refusals are counted and
    one aggregated warning is emitted; also pins that the count is not a payload
    field.
  • test_no_refusal_log_when_every_transcript_validates -- the negative case, so the
    log cannot become unconditional noise.

Mutation-verified rather than assumed: replacing if refused_transcripts: with
if False: turns the first test red (assert 0 == 1) and leaves the second green,
which is the discrimination you want from the pair. Also clean: flake8, isort,
mypy, and scripts/check_black_formatting.py.

What is deliberately not here

The UNC-gate admission for the transcript directory is not in this PR. The page
still reports zero on a roaming-profile home. Tracked in #8079, with the reason it
cannot be a small change recorded there:

unc_probe_allowed is lexical on the RAW path -- it never touches the filesystem,
because touching a UNC path is the outbound SMB probe the gate exists to prevent.
So admitting the root admits any link planted under it, and the reader then resolves
that link. The window is check -> realpath, not check -> open:
validate_file_path calls os.path.realpath immediately after the gate admits, and
that call is the probe. A no-follow open is too late, and
getattr(os, "O_NOFOLLOW", 0) is 0 on Windows -- the only platform where UNC
applies. Closing it needs an atomic no-follow resolution in pinned_fs
(CreateFileW + FILE_FLAG_OPEN_REPARSE_POINT), added once so every
validate_file_path consumer benefits.

An earlier revision of this PR admitted the root and guarded the reader with an
lstat-based reparse check. Review found that shape reachable from two directions
and both findings prescribed the same remedy -- remove the sessions directory from
the global UNC roots until resolution refuses links atomically. That is what this
revision does, so neither finding's span exists in the diff any more: the roots
tuple is untouched and the pre-validator check is gone. hooks.py,
docs/system-specs/modules/security.md and test/test_acp_prompt_blocks.py are
back to their base contents and no longer appear in the diff at all.

Pattern harvest

Rule candidate: review-prompt
Pattern: a gate that signals refusal by returning a sentinel, consumed by a bare
continue inside an aggregation loop -- so a total refusal renders as a
legitimate empty result.

This defect was not a typo; it is that shape. validate_file_path returns None
to mean refused, the loop did if resolved_str is None: continue, and the function
returns a count. One refused file is noise worth skipping; every file refused is a
platform misconfiguration, and both compile to the same rendered 0. The tell is
structural and reviewable: a sentinel-returning validator whose None branch has no
counter, no log and no distinct return, inside a loop whose product is an aggregate.

Not proposed as semgrep, deliberately: the rule needs to know which callee is a
gate rather than an ordinary lookup that may legitimately miss, and that is
project knowledge rather than syntax. A review prompt can ask the question ("this
loop skips on a sentinel -- what does the caller see when EVERY item skips?") where
a syntactic matcher would mostly fire on benign dict.get misses.

Refs #6733
Refs #7505
Refs #8079

@chenmingwei23
chenmingwei23 requested a review from a team as a code owner August 31, 2026 16:15
@chenmingwei23
chenmingwei23 requested a review from buluoray August 31, 2026 16:15
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 31, 2026
@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

A deliberately scoped observability fix — the silent zero gets a named cause, and the harder UNC-admission root fix is correctly deferred with its hazard documented.

[DESIGN-REVIEWED] 2a31082

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 2a31082a06e2a1cc49ce8b49faf1acc0b985e0fa — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 2a31082

Verdict parsed from the review's SHA-scoped output markers for commit 2a31082a06e2a1cc49ce8b49faf1acc0b985e0fa.

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

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 2a31082a06e2a1cc49ce8b49faf1acc0b985e0fa — 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 checks are done. The diff does exactly one thing, the description declares its symptom-level placement with a derived security reason for the deferral, validate_file_path is silent by design at the chokepoint (no existing mechanism duplicated), and no new API surface ships. One minor description-vs-diff mismatch found on a test claim. Final review:

First-Principles-Verdict: PASS

One aggregated warning where a diagnosable silence was; the deferred cause (#8079) is named with a derived security reason, and nothing rides along.

What this change ships

Intent: let an operator on a UNC roaming-profile home tell a refusal-zero on the usage page from a genuinely idle account — a FIX (of the silence half of #6733, with the wrong number explicitly deferred).

  1. A zero caused by refused transcripts now leaves one server-side warning naming the count and directory — justified
  2. Nothing else: no payload field, no config key, no per-file noise; two tests pin the warning and the quiet healthy path — justified

The harm is a reported defect, so the requirement is derived. Zero option: the silence continues and the operator has no thread to pull. No duplicate: validate_file_path (hooks.py:2030) returns bare None by design, and no caller-side refusal log existed at this site. The two silent-continue siblings I counted (skills.py:1761, telemetry.py:257 — pattern validate_file_path(...) is None + silent skip) don't share the failure mode: skills live under the admitted crew data home and telemetry logs at its directory-level check (telemetry.py:251), so this is not a point patch. Symptom-level placement relative to the wrong count is declared in both the description and the code comment, with the cause (check → realpath on a lexical UNC gate) genuinely out of one-PR scope.

Watch

  • The description claims "the test pins that it stays absent" (a payload field), but no assertion in the diff checks the payload for absence — the test asserts only the log and total_sessions. Shrink the claim or the pin is imaginary.

[FIRST-PRINCIPLES-REVIEWED] 2a31082

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 2a31082a06e2a1cc49ce8b49faf1acc0b985e0fa and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 2a31082

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

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 31, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/unc-home-usage-validate-6733 branch from 4c156e0 to bd515d4 Compare August 31, 2026 16:55
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 31, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/unc-home-usage-validate-6733 branch from bd515d4 to b502e57 Compare August 31, 2026 17:12
@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Aug 31, 2026

@buluoray buluoray left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed at b502e5763. 1 blocking / 0 yellow / 1 blue — comment only. The blocker is one stale sentence in the PR body, not a code or security problem; correct it and this is approvable as-is.

The security substance is sound

I reviewed this as the highest-risk change in the batch, since it widens a trusted root. All four axes came back clean:

  • No keystone exposure. The new root <kiro home>/sessions/cli overlaps nothing in security._SENSITIVE_HOME_DIRS or _CREW_SECRET_LEAVES, and unc_probe_allowed is not the read/write authorization gate — is_sensitive_path still runs post-resolution, so security_policy.json, profiles/, admission_policy.json and computer_use.json stay unreadable and unwritable. settings/mcp.json stays refused, with a pinned test. Admission is the transcript dir exactly, not the kiro home.
  • The root match is not bypassable by a crafted path. It is normcase(normpath(...)), which handles case folding, mixed separators and \\?\ prefixes, and it refuses by default — so a crafted variant either lexically sits under the root or is rejected. Critically it is separator-boundary-anchored (cand == rootn or cand.startswith(rootn.rstrip("\\/") + os.sep)) rather than a bare prefix compare, so a sibling like <root>-evil does not match. I mutated it to a bare startswith and both neighbour_directory_* tests reddened, so that anchoring is load-bearing rather than incidental.
  • Junctions are handled. The new leaf-level is_reparse_point check uses islink plus st_reparse_tag, which is the part path.is_symlink() alone would miss on Windows.
  • No deny coverage changed. security.py is untouched, so ~/.kiro and the legacy ~/.kirocrew coverage is exactly as before.

All five mutations I ran reddened their target test and none proved inert: separator-anchor → bare startswith, dropping the sessions accessor, defeating the memo (3 == 1), removing the reparse guard, disabling the counter.

BLOCKING — PR body §3 describes a payload field the code deliberately does not add

The body currently says:

... and a refused_transcripts count in the payload, matching the machine-readable code: sessions_dir_unreadable this function already returns for the unreadable-dir case. The path stays server-side in the log; the payload carries only the count.

That is not what head does. refused_transcripts appears in usage.py only as a local (1772), an increment (1810), and two log arguments (1853, 1862) — it never reaches the return dict at 1895. The count is log-only, and both the code comment and test/test_usage.py:~112 state explicitly that it is not an API field, attributing that to the First Principles review on this PR.

So the body is stale from a pre-review revision. The shipped behavior is the more conservative of the two, which is why this costs nothing to fix: it is a one-sentence edit, not a code change. I am flagging it as blocking only because anyone reading the body — a reviewer, an operator, or someone writing a usage-page renderer — is being told the API exposes a field that does not exist.

Blue

hooks.py:1984 (validate_file_path) — the UNC gate is lexical on the raw path and realpath runs after admission, so a symlink or junction at a directory component of any trusted root (not just the transcript leaf) is still resolved through. This is pre-existing, applies equally to the data-home, temp and agents roots, is disclosed in docs/system-specs/modules/security.md, and tracked separately. The new leaf-level check is a net improvement, so this is context rather than a request.


Fix the §3 sentence and I will approve.

@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 1, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/unc-home-usage-validate-6733 branch from b502e57 to b7d221d Compare September 1, 2026 03:56
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: passed Eligible automated validation passed for the current revision merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: checking Automated validation is still running labels Sep 1, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Disposition for head b7d221d3d: the GPT BLOCKING is REBUTTED on merits, and its substance is filed as #7505

This head is a rebase onto current main (the 13-PR merge batch put the branch in
conflict; the only conflict was an import list in usage.py, resolved as the
union of main's emit_turn_usage and this branch's is_reparse_point). The
rebase re-rolled the review lanes, and GPT raised one BLOCKING finding on the new
head:

BLOCKING -- src/kiro_crew/dashboard/handlers/usage.py:1826 -- Check-then-resolve
leaves a UNC symlink-swap window
Fix: Remove the sessions-root admission until entries are opened through a
race-free, no-follow mechanism.

The window is real. The attribution is wrong, and the prescribed fix would
reduce protection.


1. The class is unconditional at base on three other roots, and this PR's root
is the only one that checks at all.

unc_probe_allowed admits four roots. Measured at this PR's merge-base
271428883 with git grep is_reparse_point -- src/, the call sites are
apps/routes.py (2), snapshot.py (4) and eval/bench/safepath.py. None is
on the data-home, temp-dir or agents-dir path, and none is inside
validate_file_path:

trusted root reparse check before validate_file_path at base
crew data home none
temp directory none
kiro agents dir none
kiro-cli transcript dir admitted by this PR, and this PR adds the check

So the primitive GPT describes - plant a link under a trusted root, have
realpath follow it, gateway opens SMB to the attacker's host - is available at
base on three roots with no race to win. The attacker capability required to
exploit the window here (a same-user writer with Windows symlink privilege) is
strictly sufficient to exploit those three unconditionally. Admitting the fourth
root therefore grants no capability that base did not already grant more cheaply.

2. The prescribed fix subtracts protection. "Remove the sessions-root
admission" would revert the #6733 repair - on a roaming (UNC) home the usage page
counts zero sessions and reports a confident zero - while leaving the three
unchecked roots exactly as they are. The net effect is fewer working features and
the same exposure.

3. The correct fix is one change in pinned_fs, not a subtraction in one
consumer.
Closing the race needs a no-follow open with the read taken from
the handle (CreateFileW + FILE_FLAG_OPEN_REPARSE_POINT on Windows; Python's
os.open exposes O_NOFOLLOW on POSIX only). Doing that once for every consumer
is the right shape; doing it inside this one consumer would leave the other three
roots unfixed and add a second spelling of the same mechanism.


Filed as #7505, which covers both halves - the unconditional gap on three
roots (the larger exposure, reachable without timing) and this TOCTOU refinement -
and records the ordering argument. The spec section this PR writes already said
the property "holds for the data home, the temp dir and the agents dir too ...
tracked separately rather than folded in here"; #7505 is that tracker, which
previously did not exist. That was a fair gap in the PR and it is now closed.

To be explicit about what this disposition is and is not: it is not a
deferral. I am not asking for a security defect to be accepted and fixed later
inside this diff's scope. I am arguing the finding is not attributable to this
diff - the diff's own root is the best-protected of the four, and the finding's
consequence chain runs identically at base through roots this diff does not
touch. If a maintainer reads that differently, the alternative I would take is
adding the no-follow open to pinned_fs in a separate PR and rebasing this one
on top of it, rather than dropping the transcript root.


Also on this head, unrelated to the above: Backend Tests shard 3 and
Frontend Lint & Type Check are red for main-owned reasons, filed as #7504
(log-site census has slack for dashboard/handlers/files.py, reproduced on a
frontend-only PR) and #7499 (main sits at 660 eslint warnings against a 659
ratchet, measured 660 with and without a branch diff applied). Neither is
actionable here. Coverage Gate and PR Readiness are downstream aggregates of
those two plus the GPT verdict.

First Principles CONCERNS on this head is still open and will get its own
disposition once the main-owned reds clear and the lanes settle on a stable head -
answering it against a head that is about to be rebased again would just be churn.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

First Principles CONCERNS on b7d221d3d: one item FIXED, one already filed

Both watch items are correct. Taking them in the order of how much they cost a reader.


1. "Declared-but-absent": the description promised a refused_transcripts payload
field the diff does not ship. FIXED in the PR body just now.

Verified before changing anything - the reviewer is right and the diff is right:

usage.py:1800   refused_transcripts = 0
usage.py:1838       refused_transcripts += 1
usage.py:1881   if refused_transcripts:
usage.py:1888       "usage: %d transcript(s) refused by path validation in %s; "

It is a local counter feeding one logger.warning. It reaches no response
payload, and test_refused_transcripts_are_reported_not_swallowed's own docstring
already states the intent ("the count is deliberately not an API field while no
renderer reads it").

So the code was right and the prose was stale - which is the worse of the two
failure modes here, because the description is what a maintainer reads before
clicking merge, and it promised an API field. I have rewritten that paragraph to
say what ships and why the payload field is deliberately absent, and to record
that the earlier wording was stale. This is the reviewer's own conclusion ("the
shipped (smaller) form is the right one - the description is stale against its own
diff"), so no code changed.

Noting the cost knowingly: a body edit re-triggers the codex lane on this same
head. I judged that acceptable here rather than leaving the description wrong,
because this head already carries a GPT verdict I have rebutted in-thread, so a
re-roll is neutral at worst and gives that lane the adjudication to read.


2. "The link check patches one of four roots." ACCEPTED, filed as #7505.

Confirmed independently rather than accepted on assertion. At this PR's
merge-base 271428883, git grep is_reparse_point -- src/ returns
apps/routes.py (2), snapshot.py (4), eval/bench/safepath.py - and nothing on
the data-home, temp-dir or agents-dir paths, and nothing inside
validate_file_path. So of the four roots unc_probe_allowed admits, the one
this PR adds is the only one that checks for a reparse point at all.

That is also why I declined GPT's separate BLOCKING on the same span (dispositioned
above): its prescribed fix was to drop the transcript root, which would remove the
one root that checks while leaving three that do not.

#7505 carries both halves - the unconditional gap on three roots, which is the
larger exposure and needs no race to win, and the check-then-resolve refinement on
this one - plus the argument for fixing them together in pinned_fs rather than
per consumer.

One correction to the reviewer's suggested shape, offered as disagreement rather
than deferral: it proposes "one scoped check inside validate_file_path's already
I/O-bearing Windows branch (lstat after admission, before realpath)". An lstat
there closes the unconditional gap for all four roots, which is worth doing - but
it does not close the race, because it is still check-then-resolve. Making it
race-free needs a no-follow open with the read taken from the handle
(CreateFileW + FILE_FLAG_OPEN_REPARSE_POINT; Python's os.open exposes
O_NOFOLLOW on POSIX only). #7505 records both steps and their ordering so the
cheap one is not mistaken for the complete one.

The spec hunk in this PR said the property on the other three roots was "tracked
separately" while no tracker existed. That was a fair gap and #7505 closes it.

@chenmingwei23
chenmingwei23 force-pushed the fix/unc-home-usage-validate-6733 branch from b7d221d to c00d456 Compare September 2, 2026 05:06
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 2, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Handoff on the GPT block for head c00d4568b -- correcting an earlier comment in this thread

This is a handoff note, not a disposition. The decision below belongs to a
maintainer; I am not recording a fixed/rebutted/deferred verdict.

Correcting the earlier comment in this thread. The comment above titled
"the GPT BLOCKING is REBUTTED on merits" was written against head b7d221d3d.
I no longer think the rebuttal holds, for three reasons found after rebasing
onto current main (63a043a7e, head now c00d4568b).

1. It is not lane sampling; it is a stable finding. The earlier read was that
GPT produced two different findings on one tree, so the lane was sampling. But the
same check-then-resolve finding has now been raised on b7d221d3d AND on
c00d4568b, while the "trust leaks into every UNC consumer" variant appeared once.
The converged finding is the TOCTOU one.

2. It is not an infrastructure failure. Worth stating because the check-run
output is empty, which usually looks like one. The GPT 5.6 Review job of run
33593397410: pass 1 emitted No findings., pass 2 emitted the finding followed
by [BLOCK-MERGE] c00d4568b... and [GPT-REVIEWED] c00d4568b..., both naming the
current head. No Bedrock 401, no expired token, no permission fail-close anywhere in
the log. The verdict="review incomplete" string in that log is echoed script
source, an untaken branch of the gate step.

3. The reachability argument was wrong. GPT's chain reads "agent swaps a regular
transcript for a UNC symlink after this check". The actor is an agent, and an
agent-writable path is exactly what is_sensitive_path / pinned_fs exist to
constrain, so this is reachable by a party the product actively defends against.
The earlier framing (a writer with this access could do worse anyway) does not apply
to that actor.

What is still true from the earlier comment: base parity. Verified at
63a043a7e -- git grep is_reparse_point -- src/ returns apps/routes.py,
snapshot.py and eval/bench/safepath.py, none on the data-home / temp-dir /
agents-dir path, and validate_file_path contains no reparse or lstat check. So the
class genuinely is unconditional at base on three sibling roots. That justifies
tracking it as #7505; it does not by itself justify admitting a fourth root while
the hole is open.

One correction to the shape of any fix. The window is check -> realpath, not
check -> open: validate_file_path calls os.path.realpath, and that is the
outbound touch. So adding O_NOFOLLOW + fstat to the read
(resolved.open("rb")) would harden a different stage and would not close this
window. It should not be offered as a fix for this finding.

Two paths, neither requiring an override

(a) Prerequisite change in pinned_fs, then rebase this PR onto it. An atomic
no-follow open (CreateFileW + FILE_FLAG_OPEN_REPARSE_POINT on Windows,
O_NOFOLLOW on POSIX -- note getattr(os, "O_NOFOLLOW", 0) is 0 on Windows, which
is the only platform where UNC applies). This satisfies the escape clause in GPT's
own prescribed fix ("until transcript resolution refuses links atomically"), fixes
all four roots, and keeps the #6733 repair.

(b) Split this PR. Drop the kiro_sessions_dir admission and ship only the
non-silent-refusal logging. That still fixes half of #6733 -- the usage page stops
reporting a confident zero with nothing logged -- and the admission lands after (a).

Everything else on this head is green

68 of 69 checks pass. PR Readiness is red only because it aggregates the blocking
verdict. The three Backend Tests shard-3 reds were a main-owned census drift fixed
by #7761 and cleared on the rebase; the Backend Tests (Windows) (3) red was a flake
in test/test_session_control.py (too many sessions created recently, a file this
diff does not touch) which passed on attempt 2 of the same code. Design, Opus and
First Principles are clean on this head, and there are no unresolved review threads.

@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 2, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/unc-home-usage-validate-6733 branch from c00d456 to 869b771 Compare September 2, 2026 20:45
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 2, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Update for head 869b7713e -- correcting the tally in my earlier handoff note

Still a handoff note, not a disposition. The decision remains a maintainer's.

Head moved. The PR went mergeable_state=dirty when main advanced 65 commits, so
c00d4568b was rebased onto main 556f08f93 -> 869b7713e. One conflict, in
docs/system-specs/modules/security.md: main's telemetry change added a new
"Privacy-safe OAuth rejection diagnostics" paragraph beside the three-roots UNC
paragraph this commit rewrites to four roots. Resolution kept main's new paragraph and
this commit's rewrite and dropped the superseded original -- 1 insertion / 1 deletion
against main for that file, and the commit's diffstat is unchanged at 5 files
+416/-57. hooks.py auto-merged and main's telemetry symbols are present at identical
counts.

Correction to the earlier note. It said the check-then-resolve finding was "the
converged finding" and that the global-allowlist variant "appeared once". That is no
longer true. Both have now recurred on separate heads:

finding raised on
A -- hooks.py, global UNC allowlist exposes an SMB credential probe b7d221d3d, 869b7713e
B -- usage.py, link check can be raced before UNC resolution b7d221d3d, c00d4568b

Two distinct findings, two heads each. Neither is lane noise. The live block on this
head is A:

BLOCKING -- src/kiro_crew/hooks.py:1995 -- Global UNC allowlist exposes an SMB credential probe
> `_config_paths.kiro_sessions_dir,`
Agent-planted image symlink under the transcript root + channel message containing its
path -> build_prompt_blocks -> Path.is_file() follows the UNC target -> outbound SMB
authentication.
Fix: Remove the sessions directory from the global UNC roots.

What matters for the decision: both findings prescribe the same remedy. A says
"remove the sessions directory from the global UNC roots"; B said the same "until
transcript resolution refuses links atomically". Four review rolls, two independent
framings, one remedy. So the two paths already on the table are unchanged, and the
choice between them is now better specified:

  • (a) Land an atomic no-follow open in pinned_fs as a prerequisite, then rebase
    this PR onto it -- keeps the transcript-root admission and closes the class for all
    four roots. Satisfies A and B's shared "until it refuses links atomically" clause.
  • (b) Split this PR: drop the kiro_sessions_dir admission, ship only the
    non-silent-refusal logging. That is literally what both findings ask for, and it
    still fixes half of Windows UNC-home: usage page silently counts zero sessions (validate_file_path refuses ~/.kiro/sessions/cli transcripts) #6733 -- the usage page stops reporting a confident zero with
    nothing logged. The admission then lands after (a).

Base parity re-verified at the current base 556f08f93 (not restated from the earlier
note): this diff does not touch src/kiro_crew/acp/prompt_blocks.py,
is_reparse_point guards none of the data-home / temp-dir / agents-dir roots, and
validate_file_path contains no reparse check. The class is still unconditional at
base on three sibling roots, which is what #7505 tracks -- it argues for fixing the
class, not for admitting a fourth root while it is open.

Everything else on this head is clean: mergeable=true, one commit, Design and
Opus clean, zero unresolved review threads, and local gates green (black, flake8,
isort, mypy, plus the two touched test files at 188 passed / 1 skipped). The only reds
are the GPT lane and the PR Readiness status that aggregates it.

@github-actions github-actions Bot removed the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 3, 2026
…ent zero

On a Windows roaming-profile home the home directory is itself a UNC share, so
`unc_probe_allowed` gates every path under it. It admits the crew data home, the
temp directory and the kiro agents dir but not `<kiro home>/sessions/cli`, so
`usage._parse_sessions` has every kiro-cli transcript refused by
`validate_file_path` and `continue`s on `None`. The usage page reported a
confident zero with nothing anywhere to say why.

Count those refusals and emit one aggregated warning naming the directory. It is
aggregated after the loop rather than logged per file because that failure mode
refuses all of them at once. The count is deliberately not a response field while
no renderer reads it, and the record is server-side only because `%s` of a `Path`
is a filesystem path the returned payload never carries.

This does NOT make the count correct on a UNC home. The page still reports zero
there; it just stops reporting it silently, which was the second half of the
reported defect. Admitting the transcript directory to the UNC gate is what would
restore the count, and that is deferred to #8079 with the prerequisite recorded:
the gate is lexical on the RAW path, so admitting the root admits any link planted
under it, and the window is `check -> realpath` rather than `check -> open` --
`validate_file_path` resolves immediately after admitting and that resolution is
itself the outbound probe. A no-follow open therefore cannot close it, and
`getattr(os, "O_NOFOLLOW", 0)` is 0 on Windows, the only platform where UNC
applies. It needs an atomic no-follow resolution in `pinned_fs` first.

Refs #6733
Refs #8079
@chenmingwei23 chenmingwei23 changed the title fix(security): admit the kiro transcript dir as a UNC trusted root fix(usage): log the transcripts the UNC gate refuses instead of a silent zero Sep 3, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/unc-home-usage-validate-6733 branch from 869b771 to 2a31082 Compare September 3, 2026 04:44
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 3, 2026
@iamwhatever
iamwhatever enabled auto-merge (squash) September 3, 2026 06:04

@iamwhatever iamwhatever left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Tier 1 auto-approve: fix (2 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: counts transcripts refused by path validation in _parse_sessions and emits one aggregated server-side warning, so a UNC/roaming-profile home no longer renders an indistinguishable confident zero; no payload field added, no change to the validator itself.

@iamwhatever
iamwhatever merged commit d32be78 into main Sep 3, 2026
73 of 74 checks passed
@iamwhatever
iamwhatever deleted the fix/unc-home-usage-validate-6733 branch September 3, 2026 06:04
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants