fix(cron): resolve the calling session from the injected caller block

[chenmingwei23]

What is the problem?

kirocrew-cron worked out which session was calling it by reading its own
process environment. On a pooled backend one process serves many sessions, and
process environment can only ever name one of them -- and gatewayd forwards no
session-identifying variable to a shared backend at all, so what this server
actually read there was EMPTY.

Every session-scoped path then took its empty branch, and those branches did not
agree with each other:

path	behaviour on an empty identity
_check_cron_job_ownership	return None -- allow
cron_list	filter skipped -- every session's jobs visible
cron_add	stores session_key="" -- ownerless row
cron_remove_all	hard refusal

Two of those are fail-open. So the per-job ownership gate was dead code for
every caller arriving through the gateway -- which is every caller it exists to
separate: cron_update, cron_remove, cron_pause, cron_resume and
cron_trigger accepted any job id from any session. cron_add was minting rows
with no recorded owner, which is also why session="origin" delivery had nothing
to route back to.

The mechanism for doing this correctly already shipped, and this server had not
adopted it.

Why this issue matters to the user

Two sessions cannot be kept apart. A user running a dashboard tab and a Slack
thread has two sessions whose scheduled jobs are supposed to be their own; today
either one can list, pause, retime or delete the other's, and nothing in the
audit trail can attribute which did.

Nothing crossed tenants BEFORE this change only because identity was uniformly
empty -- there was no other identity to cross into. That is not a property to
rely on: the moment any caller resolves a real key, the fail-open branches start
granting an unidentified one authority over that caller's rows.

How our fix solves it

Chain from the symptom to the root cause:

1. Symptom -- session-scoped cron operations behave as if unattached.
2. mcp_cron resolves identity through mcp_core's resolver, whose FIRST source
   is the injected caller block, and the stdio shim in mcp_shared binds that
   block on every tools/call unconditionally. So the consumer side was already
   correct.
3. Root cause -- mcp_gateway/backend.py strips any stub-supplied caller
   block from every forwarded request and re-injects its own only when the
   backend advertised kirocrew.caller-identity. This server never advertised,
   so the block never arrived and the resolver had nothing to read.
4. And the unadvertised state was not "gatewayd keeps it per-session": nothing
   declines to POOL an unadvertised backend. rewriter.UNPOOLABLE_SERVERS is
   empty and its own comment records that the capability is read only to decide
   injection. So cron was pooled AND identity-blind, and a comment in mcp_core
   asserting the opposite is corrected here.

The change:

• Advertise the capability (ADVERTISE_CALLER_IDENTITY = True, passed to
  run_mcp_stdio_loop) and add kirocrew-cron to
  mcp_discovery._MANAGED_SERVERS_CALLER_AWARE. The existing ratchet in
  test/test_mcp_managed_caller_identity.py makes those two impossible to drift:
  it drives each server's real serve entry and compares what the shim was handed
  against the set.

• One strict resolver for authorization. _authz_session_key() uses the
  STRICT resolver, so an ownership decision can no longer come from the lenient
  resolver's /proc ancestor walk over a per-pid file that mcp_core itself
  documents as "agent-writable and therefore forgeable". Labelling an audit row
  from it is tolerable; deciding who may delete whose scheduled job is not.

• One scope for reads and writes. _owned_by is the single function both go
  through: a caller reaches only the rows whose session_key matches its own, and
  an empty key reaches NOTHING. Two scopes were two chances to pick the wrong one,
  and cron_remove_all picked the wider one once during this review. Every write
  additionally refuses with one shared message naming the CLI route. This follows
  from one measurable fact: gatewayd forwards with caller=None when a stub's
  Register carries no session key and peer-identity resolution fails
  (gatewayd.py, the SO_PEERCRED + /proc path), so an unidentified caller can
  be sharing a pooled backend with identified ones. Empty identity does not imply a
  1:1 transport, so such a caller gets neither their rows nor authority over them.
  The CLI keeps its admin bypass -- note KIROCREW_CLI is set nowhere in src/,
  so that in-process bypass is dead code today; the CLI reaches CronService
  directly and never routes through this server.

• A row with no recorded owner is outside every session's scope, for reading
  and for writing alike. An earlier revision of this change kept such rows VISIBLE,
  arguing a row with no owner has no owner's privacy to breach; that silently
  assumed the only identified session is the operator's, and it is not -- an
  allowlisted Slack or Telegram participant gets a session of their own, while a
  cron's message is arbitrary prompt text with command/script payloads beside
  it. Showing the admin surface's rows to one of those is a disclosure to a
  different principal, so they are withheld. cron_add stops this server
  contributing new ownerless rows, but the set keeps growing regardless:
  cli_commands and the onboarding importer both create jobs with no session to
  name, which is why this is a permanent scope rule and not a time-boxed exemption
  for legacy rows.

  The consequence, stated plainly: a cron created with kirocrew cron add no
  longer appears in cron_list from chat, and a job the old pooled cron_add
  minted with an empty owner becomes invisible and immutable from chat after
  upgrade. No migration is possible -- the owner was never recorded -- and the CLI
  remains the management surface for both. This belongs in the release notes, since
  a user will otherwise read it as their crons vanishing.

• The scoping decision is audited. cron_list withholding rows is an
  authorization decision, and every other one in this module already landed on the
  SEL trail; this one did not, and it was the least visible of them (an
  unidentifiable caller has EVERYTHING withheld). It now emits denied when
  nothing survives the filter and scoped when some rows do -- and NO event when
  the caller owns everything it could see, because cron_list is called often and
  a per-call event would bury the trail it is meant to serve.

• The gate's three refusals are one string. A pre-existing enumeration oracle,
  found by one of the new tests: the gate's own comment claimed anti-enumeration
  while it answered Job not found: <id> for an unknown id and Error: job not found: <id> for another session's row -- two distinguishable strings, so a
  caller could tell an id that exists from one that does not. All three branches
  now return one identical string. Post-gate messages still name the row: by then
  the caller owns it.

• cron_add's channel default had the same defect as the session key
  (KIROCREW_CHANNEL_ID from process environment); it now reads channelId off
  the same caller block, with the env var as the non-gateway fallback.

• cron_trigger checks the id's shape before ownership, so a malformed id
  keeps its own message instead of the ownership gate's deliberately vague "job
  not found". The enforcing check inside trigger_cron_job is unchanged; this
  only makes its reason reachable.

Scope note

The issue also proposes deleting the session_bound_by_construction reason code
once nothing produces it. That is not yet possible and is deliberately left out:
kirocrew-computer and kirocrew-dashboard are the other two managed servers
and neither advertises, so the input still has producers. Both already resolve
through the strict resolver, so each is the same small adoption -- worth its own
change rather than folding three servers' pooling behaviour into one review. The
verdict is now asserted explicitly for all four servers so the remaining two are
visible rather than implied.

What tests we did

The ratchet bites in both directions (checked before writing the fix, on the
untouched tree): adding cron to the caller-aware set without flipping the flag
fails test_session_bound_is_the_inverse_of_advertising[kirocrew-cron], and
flipping the flag without updating the set fails the same test. Neither
half-change can land silently.

New unit coverage (test/test_mcp_cron_caller_identity.py, 15 tests):

• the caller block WINS over process environment -- asserted with the environment
  naming a DIFFERENT session, so a test that merely read the block back cannot
  pass by accident;
• with no block, the environment still resolves (a non-gateway launch is not
  regressed);
• the forgeable per-pid lookup is made to succeed loudly and the answer is still
  empty, proving it is not an authorization source;
• each of the five mutating tools refuses an unidentified caller AND leaves the
  row untouched; cron_add refuses and stores nothing; cron_list still reads;
  the CLI bypass still works;
• one session can neither see nor touch another's job (this assertion could not
  have held before -- both callers resolved empty);
• the channel default comes from the block;
• a job created through this server always records an owner; an ownerless row is
  neither listed nor mutable, and its refusal is byte-identical to the answer for
  an id that does not exist; _owned_by(jobs, "") is empty rather than every
  ownerless row; and the list-scoping decision lands on the SEL trail as denied /
  scoped / no-event.

New integration coverage -- the half unit tests cannot reach. They prove cron
CONSUMES a block; they cannot prove gatewayd SENDS one, and the capability is
parsed off a REAL initialize response, a step the in-process stub seam every
other gateway test uses does not go through. So
test_mcp_gateway_pool_integ.py::test_two_stubs_on_one_backend_are_told_apart
drives two REAL stub processes with different session keys onto one pooled
backend and reads back what the backend was actually handed, asserting it saw
both keys in order. The bundled fake server records the sessionKey of each
tools/call; advertising is bundled with recording deliberately, since a
recorder that stayed silent would observe nothing and read as a broken gateway.

Mutation-verified, 9 probes, all killed. Each reverts one fixed line and
confirms the named test goes red: lenient resolver restored; per-job gate back to
allowing; cron_add back to minting ownerless rows; channel default back to
env-only; legacy exemption dropped; visibility filter dropping ownerless rows;
authorization reading env directly; the backend not advertising; and gatewayd
injecting one hardcoded identity for every call.

Existing suites. 26 existing cron tests failed on the first run because they
exercised cron_add as an anonymous caller -- fixture debt, not a design signal:
they test cron's FIELD handling and always assumed a caller the gateway can name,
they simply never said so, because the unidentified state used to be allowed to
write. A shared named_cron_caller fixture states that precondition; tests that
mock CronService now stamp an owner on the fake row. Two tests pinned the old
contract by name and were rewritten to the new one.

Full backend suite: 54194 passed, 15 failed. All 15 reproduce identically on
an unmodified checkout of this base -- host-environment failures (a /local/home
vs /home layout, CPU-count-derived worker budgets, flock holders on this box,
stray bytecode). Zero attributable to this change. isort, flake8, mypy and
the black-baseline gate are clean; the baseline lost 3 graduated entries it was
already failing on at this base (the graduated check is repo-wide, not
diff-scoped), and only shrinks.

Not verified locally, and stated as such: the non-gateway launch where neither
KIROCREW_SESSION_KEY nor KIROCREW_HOST_PID exists (a GUI-launched kiro-cli)
cannot be reproduced on this Linux host, so it is covered by the unit assertion
rather than end to end.

Any other suggestions on the work

1. rewriter.UNPOOLABLE_SERVERS is a trap while empty. A server that cannot
   support the extension has no way to say so: the intended signal -- refuse to
   pool a backend that did not advertise -- is documented as unimplemented. That
   is fine while every managed server is adopting the extension, and it is a real
   hole the moment one cannot.
2. caller=None on a pooled connection deserves a name. It is currently an
   implicit state that each consumer re-interprets. Since it can coexist with
   identified stubs on one backend, a first-class "unattributable caller" concept
   would stop the next server from having to rediscover that empty is not the
   same as alone.
3. The lenient resolver is still used in this file for audit labelling and for the
   governance-surface lookup. Both improve for free now that the block arrives,
   and neither decides row ownership, so they are left alone -- but the
   governance-surface read is the next candidate if the forgeable source is to be
   removed from decisions entirely.

Why no screenshot: backend identity resolution, no UI surface.

Fixes #4622

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 57724333d64c4d9d61dd9c16dd89975658f16be6 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 5772433

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

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound fail-closed redesign at the right layer; the one-way scope rule strands every pre-existing chat-created cron with no in-band recovery.

Watch

• Every cron minted before this change carries session_key="" (the old pooled cron_add "stores session_key=\"\" -- ownerless row"), so on upgrade the ownerless-rows rule makes all previously chat-created jobs invisible ("No cron jobs.") and immutable ("job not found") from chat, with no migration possible and the only explanation living in release notes the user may never read. The withholding is correctly derived (a Slack/Telegram participant's session is not the operator), but the recovery story is "use the CLI" with no in-product breadcrumb; a follow-up CLI adoption path (assign an owner to an ownerless row) would turn a permanent stranding into a one-command fix — worth tracking before this ships in a release.
• The scope note leaves kirocrew-computer and kirocrew-dashboard pooled-and-identity-blind — the exact state this PR proves is fail-open-shaped. Deferral is reasonable and test-visible, but the two follow-ups should be filed, not implied.

[DESIGN-REVIEWED] 5772433

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 57724333d64c4d9d61dd9c16dd89975658f16be6 — 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.

I've read the contract, the intent file, the patch, and verified the load-bearing claims against the repo (UNPOOLABLE_SERVERS empty at rewriter.py:103, KIROCREW_CLI read at one site and set nowhere in src/, _MANAGED_SERVERS_CALLER_AWARE now two of four managed servers, strict resolver pre-existing at mcp_core.py:547).

First-Principles-Verdict: CONCERNS

The fix adopts the mechanism that already existed and lands at cause level — but two of four managed servers share the exact root cause and stay unfixed.

What this change ships

Intent: make cron's per-session ownership gate actually enforce, by giving the pooled cron server the calling session's identity. A FIX.

1. Cron tools now know which session calls them (caller block advertised + consumed) — justified
2. One session can no longer list, pause, retime or delete another's jobs — justified
3. An unidentifiable caller's writes refuse with one message naming the CLI — justified
4. cron_add refuses instead of storing an ownerless row — justified
5. CLI-created crons vanish from chat's cron_list — declared, derived from the messaging boundary
6. All three ownership refusals read identically (enumeration oracle closed) — justified
7. cron_add's default delivery channel comes from the caller block — justified, same root cause
8. Malformed id on cron_trigger gets its own error — rides along, declared
9. cron_list withholding rows lands on the SEL trail — justified
10. Audit rows name the calling session instead of the literal "mcp_cron" — justified

Watch

• Counted unfixed siblings: 2. kirocrew-computer and kirocrew-dashboard are unadvertised, pooled (rewriter.UNPOOLABLE_SERVERS empty), and already resolve via _resolve_session_key_strict whose first source is the caller block — the same one-line advertisement fixes each. The description defers them; the deferral is acknowledged, but the identical hole stays open on both.
• The verdict flip managed_server_is_session_bound("kirocrew-cron") → False also changes what the shareability assessment and dashboard render for cron; the truncated description covers the reason-code side but a reader should know the dashboard row changes too.

Subtractions

• Delete the KIROCREW_CLI=1 bypass (_caller_is_cli, mcp_cron.py:1482) — 0 setters in src/ (grep KIROCREW_CLI), and the description itself calls it dead code; the real CLI reaches CronService directly. The change wraps and perpetuates an env-settable authz bypass nothing uses.

[FIRST-PRINCIPLES-REVIEWED] 5772433

[chenmingwei23]

Round 2: GPT's blocking finding is ACCEPTED and fixed forward on e7b8c4b2d. No override.

Its diagnosis was right and my justification was wrong. I had written that the
ownerless set "cannot grow, because cron_add now refuses" -- so the _UNOWNED
mutation exemption would drain. I checked every creation path before answering and
that claim is false: cli_commands.py:854/862 (kirocrew cron add) and
onboarding_import.py:4809 both call add_job with no session_key, so every
CLI-created and every imported cron is an ownerless row, continuously. With the
exemption in place, any identified MCP session could pause, retime, trigger or
delete any of them. Fail-open, exactly as reported.

The finding also caught an inconsistency in my own reasoning. I used one fact --
gatewayd forwards caller=None when a stub registers without a session key and
peer resolution fails, so an unidentified caller can share a pooled backend with
identified ones -- to justify refusing WRITES, then ignored the same fact on
READS and left cron_list unfiltered for that caller. That is a disclosure path
by the same mechanism.

What changed:

1. _UNOWNED is no longer a mutation exemption. An ownerless row is refused by
   all five mutating tools with a message naming the CLI, since an unowned row is
   not an unclaimed one.
2. cron_remove_all now uses a separate _mutable_by scope (own rows only)
   instead of the visibility scope, so a bulk delete cannot sweep CLI-created
   jobs -- the same fail-open wearing a different sleeve.
3. An unidentified caller's read is narrowed to ownerless rows instead of
   unfiltered, so it can never be handed an identified session's rows.
4. Ownerless rows stay VISIBLE to an identified session. This is the one place I
   did not follow the suggested fix, and the reason is stated rather than
   assumed: a row with no owner has no owner's privacy to breach, and hiding it
   would make every cron created via kirocrew cron add silently invisible in
   chat. So the split is by permission (read yes, write no), not by existence.

Verification: 5 new mutation probes, all killed -- gate back to allowing an
ownerless row; cron_remove_all reusing the visibility scope; _mutable_by
dropping its empty-key guard; cron_list waving an unidentified caller through;
ownerless rows becoming invisible. The third probe SURVIVED on the first run
because no caller reaches _mutable_by with an empty key today, which would have
made that guard unverified code protecting a default that fails open ("" matches
every ownerless row), so it got its own direct test rather than being deleted.

The PR body's now-falsified "the set cannot grow" paragraph has been corrected in
place; it should not have shipped as a justification.

[chenmingwei23]

Round 3 on cd630b417. GPT's blocking finding ACCEPTED (again, no override), Design Review's two Watch items both actioned.

GPT was right and my round-2 reasoning was too narrow. I had kept ownerless
rows VISIBLE to a session, arguing "a row with no owner has no owner's privacy to
breach". That silently assumed the only identified session is the operator's. It
is not: an allowlisted Slack or Telegram participant gets a session of their own,
and a cron's message is arbitrary prompt text with command/script payloads
beside it. Disclosing the admin surface's rows to one of those is a disclosure to
a different principal. GPT named exactly that caller ("allowed messaging caller"),
which is the part I had not considered.

So reads and writes now share ONE scope function, _owned_by: a caller reaches
only the rows whose session_key matches its own, and an empty key reaches
nothing. _visible_to/_mutable_by collapse into it -- two scopes were two
chances to pick the wrong one, and cron_remove_all had already picked the wider
one once during this review.

Consequence, stated rather than buried: a cron created with kirocrew cron add
no longer appears in cron_list from chat. The CLI remains its management surface.
That is a real behaviour change for anyone who creates crons from the CLI and
lists them from chat, and it is the price of not disclosing them to every
identified session.

A pre-existing enumeration oracle, found by one of the new tests. The gate's
own comment claimed anti-enumeration, but it answered Job not found: <id> for an
unknown id and Error: job not found: <id> for another session's row -- two
distinguishable strings, so a caller could tell an id that exists from one that
does not. All three refusal branches now return one identical string via
_not_found(). Post-gate messages still name the row freely: by then the caller
owns it.

Design Review Watch items:

1. Spec not updated -- fixed in this commit. docs/system-specs/modules/learn-cron-dashboard.md
   said identity comes from KIROCREW_SESSION_KEY and that cron_remove_all
   scopes to the calling session with "CLI/admin with no key still removes all",
   all of which this PR falsifies. Rewritten to the caller-block contract, the one
   _owned_by scope, the unidentified-caller rule, and the ownerless-row rule.
2. _caller_is_cli() is an env-based admin bit on a pooled process -- confirmed
   and worth saying plainly: KIROCREW_CLI is read in exactly one place and set
   NOWHERE in src/, so the in-process bypass is dead code today. The CLI reaches
   CronService directly and never routes through this server, so the refusal
   messages that point at kirocrew cron ... are still correct advice. Left in
   place for this PR and documented as such, per the review's own "worth a
   follow-up" framing -- deleting it touches 11 assertions across 3 test modules
   and belongs in its own change, where the alternative (moving the admin bit onto
   the caller block) can be considered properly.
3. GUI-launch case -- unchanged and still covered by a unit assertion rather than
   end to end; it cannot be reproduced on this Linux host.

First Principles Review returned "could not complete" (model error, advisory,
no verdict) -- it re-runs on this push.

Verification: 6 new mutation probes, all killed -- scope widening back to
include ownerless rows; the empty-key guard removed; cron_list waving an
unidentified caller through; the ownerless refusal becoming distinguishable again;
the not-found branch reverting to its old wording; cron_remove_all widening.
Four more cron test modules needed the owner precondition stated (they assert
cron_list OUTPUT FORMAT, and an unowned fixture row is no longer listed).
Full backend suite: 54197 passed, 15 failed -- the same 15 host-environment
failures that reproduce on an unmodified checkout of this base.

[chenmingwei23]

Round 4 on 41750c954. GPT's blocking finding ACCEPTED (no override). Design Review's description-mismatch Watch fixed.

GPT: the cron_list scoping decision was not audited. Correct, and it is a gap
my own change widened. Every other authorization decision in this module already
landed on the SEL trail -- both refusal helpers, and cron_remove_all's scoped
outcome -- while cron_list's filter logged nothing. That was tolerable when the
filter was effectively a no-op (identity was always empty, so nothing was ever
withheld); now an unidentifiable caller has EVERYTHING withheld, which made the
least visible denial in the module also the broadest.

_audit_list_scope now records it: denied when nothing survives the filter,
scoped when some rows do, and no event at all when the caller owns everything
it could see. The last part is deliberate -- cron_list is called often, and a
per-call event would bury the trail it exists to serve, so only a decision with an
effect is logged.

Design Review: the description documented a withdrawn contract. Also correct,
and the more embarrassing of the two: the PR body still described round 2's
"read is NARROWED to ownerless rows" and "visible but never mutable", which round 3
replaced with the opposite. Anyone auditing this boundary from the description
would have checked the wrong property. Rewritten to the shipped contract -- one
_owned_by scope for reads and writes, ownerless rows withheld in both directions
-- plus the two items the review asked be made explicit rather than buried:

• Release-notes item, now in the body: a cron created with kirocrew cron add
  no longer appears in cron_list from chat, and a job the old pooled cron_add
  minted with an empty owner becomes invisible and immutable from chat after
  upgrade. No migration is possible (the owner was never recorded). The CLI is the
  management surface for both.
• _caller_is_cli() is dead code today (KIROCREW_CLI is set nowhere in
  src/; the CLI reaches CronService directly and never routes through this
  server), stated in the body rather than left for a reader to discover. Removing
  it stays a follow-up per the review's own framing.

The identity-less non-gateway launch remains unverified end-to-end and is called
out as such in the body; it cannot be reproduced on this Linux host.

Verification: 3 new mutation probes, all killed -- the audit call removed
entirely; denied collapsed into scoped (a total withhold stops being
distinguishable); an event firing even when nothing was withheld. Scoped suite:
6105 passed, 1 failed (the known host flock failure). All local gates green.

[chenmingwei23]

Round 5 on 481b54854. GPT's blocking finding ACCEPTED in substance, fixed in a different place than suggested. No override.

The gap is real; its location is not where the finding points. GPT asks for an
allowed event inside _audit_list_scope before the early return. I checked what
already reaches the trail first: mcp_shared.call_tool_with_logging records EVERY
cron tool invocation, including a successful cron_list, so an event does exist.
But mcp_cron._call_tool was handing it the hardcoded label "mcp_cron" instead
of a session, so that record could not say WHO was authorized. Combined with the
scoping helper's deliberate silence when nothing is withheld, the authorized case
was attributable to nobody -- which is the substance of the finding.

So the fix is one line at the wrapper, not a third event:
session_key=_resolve_session_key() or "mcp_cron". mcp_core has done exactly
this for a while, with a comment stating a hardcoded label "lost attribution for
every standard tool audit in shared backends" -- cron was the outlier. Now every
cron tool call is attributed to its calling session, the authorized list included,
and no call pays for a duplicate event. The lenient resolver is correct at this
site: it labels an audit row rather than deciding access, which stays
_authz_session_key's job with its refusal of forgeable sources.

Emitting the suggested extra event would have double-logged every cron_list --
two records per call, same tool, same session, one carrying no information the
other lacks -- in a trail whose value is being readable.

Verification: 2 mutation probes, both killed after one was found MIS-TARGETED.
The first probe reported SURVIVED, and the cause was the probe, not the test: the
anchor string session_key=_resolve_session_key() or "mcp_cron", occurs twice in
the file and a first-occurrence replace mutated an unrelated audit site, leaving
the line under test untouched. Re-anchored on the preceding comment line, it kills.
Worth stating because a mis-targeted probe reads exactly like a weak test, and
acting on that reading would have meant weakening a correct assertion.

Scoped suite (cron/mcp/gateway/audit): 7928 passed, 1 failed -- the known host
flock failure. All local gates green.

[chenmingwei23]

Round 6 on 57724333d. GPT 5.6 clean, Design Review PASS. First Principles CONCERNS (advisory) -- two of its three items fixed here, the third escalated.

Its sharpest point is right and I had half-fixed the thing I called the root cause. The PR's own analysis is "nothing declines to pool an unadvertised backend", and I corrected the one comment asserting otherwise in mcp_core.py while leaving two more standing -- including in mcp_caller.py, the module whose docstring calls itself the single source of truth for this extension. Grepped and confirmed:

• mcp_caller.py -- "gatewayd will refuse to pool them and fall back to per-session spawn (the equivalent of the old UNPOOLABLE_SERVERS list, but auto-detected)". Never implemented.
• mcp_shared.py -- "gatewayd pools ONLY backends that advertise the caller-identity extension (others fall back to per-session spawn)".

Both now state what the code does: the capability decides INJECTION, not pooling, so not advertising buys a shared process with no identity rather than a private one. Point-patching the comment in the file I happened to be editing, while the authoritative one kept the myth, would have left the next reader exactly the false safety property that let this bug live. Both rewrites point at rewriter.UNPOOLABLE_SERVERS as the only real mechanism.

Stale docstring, also right: test_remove_all_never_sweeps_an_ownerless_row carried TWO docstrings -- the round-2 one describing the abandoned two-scope revision sat directly below its replacement. Removed, and I swept the touched files with an AST check for the same residue elsewhere: none.

Escalated rather than actioned: deleting the KIROCREW_CLI bypass. Two lanes have now asked (Design Review's Watch, then this), and the evidence is not in dispute -- 0 setters in src/, so _caller_is_cli() guards nothing reachable, and the CLI reaches CronService directly without traversing this server. I agree it should go, and I am not doing it in this PR without the author's call: it deletes an admin bypass from a module whose authorization this PR just rewrote, and it invalidates 11 assertions across 3 test modules that exercise the bypass as if it were live. That is a second reviewable change riding on a security fix, and the safer sequencing is its own PR where "should the admin bit move onto the caller block instead of vanishing" can be answered properly. Flagged to the author.

The remaining Watch -- the invariant gap for kirocrew-computer / kirocrew-dashboard, still pooled while unadvertised -- is the scope note's deferral, and the review marks it noted rather than demanded.

Verification: comment-and-docstring changes only, no behaviour touched; test_mcp_caller.py + test_mcp_shared.py + the identity suite pass (59), scoped suite 6106 passed with the one known host flock failure. All local gates green.

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 57724333d64c4d9d61dd9c16dd89975658f16be6 — this comment is updated in place on each push.

Review details

Both candidates were rated low-confidence by the discovery pass, and neither survives falsification:

Candidate 1 requires that a pooled kirocrew-cron backend's process environment carries a non-empty KIROCREW_SESSION_KEY. The documented pool-spawn contract (mcp_core._resolve_session_key: "Warm-pool kiro-cli processes have no KIROCREW_SESSION_KEY env var (the pool spawns with an empty key)") is the opposite; the design treats the env var as a gateway-authored source precisely because gatewayd does not forward it to a shared backend. I could not open any code showing the env leaks into a pooled cron backend, so input (a) stays "if a caller were to" — ungrounded.

Candidate 2 is confirmed as ordering (script/command vetting in cron_add runs before the identity refusal at line 1808), but the "observable wrong outcome" is file-existence disclosure to a co-tenant that already shares the same uid and filesystem — not a boundary this codebase draws, and strictly more restrictive than pre-PR behaviour, which ran the same vetting and then stored the row. No wrong outcome (c) at the required bar.

Step 2 turned up nothing grounded: the cron_trigger shape-check-before-ownership adds no enumeration oracle (malformed hex ids cannot name a real job), _owned_by("") returning [] is tested against the fail-open case, and every mutating path routes through _authz_session_key/_check_cron_job_ownership.

No findings.

[OPUS-REVIEWED] 5772433

Verdict parsed from the review's SHA-scoped output markers for commit 57724333d64c4d9d61dd9c16dd89975658f16be6.

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

[chenmingwei23]

Round 7 on 57724333d. All five lanes have now reported and the PR is review-ready: GPT 5.6 clean, Opus 4.8 clean, Design Review CONCERNS (advisory), First Principles CONCERNS (advisory), 58 checks green, CodeQL green, readiness: passed, MERGEABLE.

Both advisory verdicts converge on three asks. Two are now filed rather than implied, which is what each lane specifically requested:

• kirocrew-computer and kirocrew-dashboard are pooled and identity-blind #4659 -- kirocrew-computer and kirocrew-dashboard are pooled and identity-blind, the same state this PR fixes for cron. Each is the same one-line adoption (both already resolve through _resolve_session_key_strict, whose first source is the caller block), but the issue makes the real work explicit: audit each server's empty-identity branches BEFORE flipping, because cron's disagreed with each other and two were fail-open. Once all four advertise, session_bound_by_construction loses its producer and the reason code can be deleted -- the second half of kirocrew-cron resolves its calling session from process env instead of the injected caller block #4622 this PR deliberately did not attempt.
• No in-band recovery for a cron row with no recorded owner #4660 -- no in-band recovery for an ownerless row. Design Review is right that the boundary is correctly derived while the recovery story is a dead end: the only explanation lives in release notes, and from inside the product the jobs just stop appearing. The issue proposes a CLI adoption path (kirocrew cron adopt) and records why auto-adopt-on-first-touch was rejected -- it grants the claim to whoever asks first, which is the fail-open this PR removed, moved earlier in time.

The third ask, deleting the KIROCREW_CLI bypass, stays escalated to the author (see the round 6 comment). Both lanes are right that it is dead -- one reader, zero setters in src/ -- and I am not deleting an admin bypass from a module whose authorization this PR just rewrote, in the same PR, when doing so also invalidates 11 assertions across 3 test modules that exercise it as though it were live.

One item to record rather than change, from First Principles: flipping managed_server_is_session_bound("kirocrew-cron") to False also changes what the MCP-servers surface RENDERS for cron -- it now reads as shareable. The PR body covers the reason-code side of that flip; this is the user-visible half. Noted here rather than by editing the body, because a body edit re-triggers the GPT lane on an unchanged head and GPT is currently clean.

Not verified locally, restated so it is not lost in the thread: the identity-less non-gateway launch (a kiro-cli with neither KIROCREW_SESSION_KEY nor KIROCREW_HOST_PID, e.g. GUI-launched on macOS) cannot be reproduced on this Linux host, so it is covered by a unit assertion rather than end to end. Writes there fail closed with the CLI-pointing refusal.

Five review rounds, and the reviewers were right about the defect in every one of them -- a fail-open ownership exemption, a disclosure path, a missing audit event, an unattributed successful authorization, and a false premise I had only half-corrected in the very comment I was citing as the root cause. What changed each time was my justification, not their diagnosis.

Awaiting the author's approve. Not merging.