feat(apps): durable job runtime and the shared _jobs surface

[chenmingwei23]

What is the problem?

The product has no server-side representation of "a task of mine is running".

A long task started from the dashboard -- AWS Control's backup, Dev Fleet's pull --
exists only as state inside the React component that started it. Navigate away and
that state is destroyed while the work keeps going, so the UI reports the task as
stopped. It was reported three times as three bugs; it is one app-wide
architectural gap, and no app can fix it alone because none of them owns a durable
place to record a run.

Why this issue matters to the user

The user cannot trust what the UI tells them about their own work. They either sit
on the page babysitting a backup, or they navigate away, see "stopped", and start it
again -- doing paid work twice against the same destination. There is also no way to
answer "did last night's run finish?", because nothing outlives the tab.

How our fix solves it

This is P1 of the merged spec docs/system-specs/features/app-sdk-durable-jobs-and-view-state.md.
It adds the durable run registry the gap needs, and ships with no consumers on
purpose -- migrating an app is P2, so P1 can be reviewed as a foundation rather
than as a feature.

ctx.job -- an app-scoped Job SDK (src/kiro_crew/apps/job_sdk.py), gated on a
new jobs manifest permission. A runner is REGISTERED against a kind at app init,
not passed per call, which is what lets a caller that cannot hold a Python callable
-- the browser, and the startup reconciliation pass -- address a run.

Shared _jobs/* routes (src/kiro_crew/apps/job_routes.py), mounted once for
every app with {app} as a path segment, so no app registers its own. Registered
before RouteRegistry.ensure_catch_all() because aiohttp matches in registration
order and the app catch-all would otherwise swallow every _jobs request.
Authorization does not trust the process registry: the guard re-reads the manifest
off the loop, so a grant revoked after enable refuses even while the SDK is still
published.

P1 records that a run EXISTS and how it ended -- nothing it produced. There is
no params a caller passes in and no progress or result a runner reports out. A
record holds identity, lifecycle, and one error string if it failed. This is the
scope decision that makes the rest defensible: those channels are arbitrary nested
data that must be sanitized before it can be written or served, they needed a
recursive sanitizer to do it, and P1 has no consumer that reads them. They return in
P2 designed against a real consumer, as types that are sanitized by construction
rather than by a rule each writer has to remember.

Four invariants, each enforced in ONE place rather than by convention at each call
site:

• Nothing runner-supplied reaches disk unsanitized. One line in the single
  writer, because error is the only field a runner supplies. Everything else is
  minted by the SDK from a str, an int or a bool, so a record is JSON-safe by
  construction and the terminal write cannot raise on serialization -- which is what
  used to skip the live-table and dedupe-key cleanup and leak a claim forever.
• One writer per run file. Each run is its own file, so concurrent writers never
  share a path; atomic_write gives crash-safety but no mutual exclusion, and the
  tree has no lock helper. Reads go through atomic_write.read_bytes_with_retry,
  because file-per-run settles writer-vs-writer and NOT reader-vs-writer: on Windows
  a reader racing the writer's rename fails with PermissionError.
• An async SDK method never parks the loop. Worker joins go through
  asyncio.to_thread, and every route reads the record off the loop.
• Cleanup reports the truth, and disable is terminal. remove_all_async returns
  a result that cannot hide a worker it failed to stop, and it closes the SDK under
  the same lock start claims in -- so a start racing cleanup cannot spawn a worker
  after the snapshot and leave a disabled app doing real work.

Reconciliation resolves records left non-terminal by a process that is gone.
Staleness is decided by a per-process origin token, not a pid, because a pid can be
reused by the very process doing the reconciling. It runs after the enable loop at
boot, and again after an app's startup hook registers its runners, so an app enabled
later in the gateway's life is not left reporting work that already stopped.

What tests we did

95 tests across test/test_job_sdk.py and test/test_job_routes.py, at 98%
coverage of job_sdk.py and 97% of job_routes.py. flake8, isort and the
baselined black gate are clean, and mypy is clean over 1178 files.

Every behaviour that a review round established is pinned, and the pins are
mutation-verified -- reverting the fix turns its own test red, checked one fix at a
time:

• a start after disable is refused, and no worker thread survives the attempt;
• an all-hex run id of the wrong length is rejected, so an oversized filename cannot
  reach the filesystem and surface ENAMETOOLONG as a 500 where 404 is the answer;
• a caller-supplied dedupe key does not reach the gateway log;
• the unknown-kind 404 body is scrubbed, since it reflects a path parameter;
• enabling an app reconciles its stale records, and a reconcile failure does not fail
  the enable;
• both record readers go through the Windows-safe helper, and a non-ASCII error
  round-trips as UTF-8 rather than in the host locale.

Verified by a real gateway boot in an isolated pod, 9 of 9, for the two wiring
points unit tests structurally cannot reach: GET /_jobs/active returned the job
handler rather than a 404 from the app catch-all, proving route ordering; and
foreign-origin running records written while the pod was down flipped to
interrupted on boot with different text for a known versus an unknown kind,
proving reconciliation runs after the enable loop.

Any other suggestions on the work

Two exceptions are deliberate. The dashboard owner-guard imports inside
job_routes._guarded stay function-local because module scope closes a
boot-breaking import cycle -- the same exemption cron_sdk documents for
mcp_cron. And gateway shutdown does not drain runs: these are daemon threads the
interpreter reaps at exit, and draining every app's runs would delay shutdown for
work nobody is waiting on.

Two items are accepted and tracked rather than fixed here. Revoking the jobs grant
and then disabling skips live-worker cleanup, and a worker that outlives the join
deadline does not fail trust withdrawal; both are in hooks_integration.py on the
disable path, and both want the same fix, which is for cleanup to run off the grant
rather than off the permission read.

P2 owes the payload channels on typed constructors, and a docs follow-up owes one
correction to the merged spec: it says the routes are registered through the Route
Registry so the existing permissions.api gate applies. Builtins register directly
on the aiohttp app, the Route Registry cannot register on another app's behalf, and
no new permissions.api entry is needed because _app_owns_path already covers an
app's own namespace.

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound foundation for a real, thrice-reported gap — but the new public contract ships undocumented, and its prose lags the final scope cut.

Watch

• New public surface with zero doc updates. The diff adds a manifest schema field (jobs), an app-facing SDK (ctx.job), and the _jobs/* route family, yet touches no docs: docs/app-kit/manifest-reference.md documents every other permission (including spawn) but not jobs, and the referenced spec (app-sdk-durable-jobs-and-view-state.md) is current-state analysis only — it describes none of P1's design. AGENTS.md mandates spec/doc updates in the same commit as a schema change; deferring it means P2 (and any third-party app author) works from documentation that doesn't match the shipped seam. Add the manifest-reference entry and the P1 design section here, not in a follow-up.
• Shipped prose describes a pre-cut draft. The new security_posture.py NON_EGRESS entry for job_sdk.py cites _json_safe, progress lines, params and result — machinery P1 removed; a governed redaction inventory misdescribing its mechanism misleads the next security audit. Likewise the description lists "revoked grant skips live-worker cleanup" as accepted residue while _cleanup_app_jobs's registry-keyed design explicitly fixes it. Reconcile both with what actually shipped.
• start(kind) with no params cannot drive the motivating consumers (a backup needs a destination; a fleet pull needs a target), so P2 must reshape JobFn/start after ctx.job is public to every app with the grant. Owner-gating and zero consumers make this reversible today; note the intended compat path (optional typed params) in the spec section above so a P1-adopting app doesn't harden the wrong shape.

[DESIGN-REVIEWED] d633f61

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

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

First-Principles-Verdict: CONCERNS

A derived, honestly-framed foundation — but it ships stale security-posture text describing a sanitizer that no longer exists, plus dead API surface with zero consumers.

What this change ships

Intent: give long app tasks a server-side record so navigating away no longer reports running work as stopped — an ADDITION (P1 foundation; fixes no user-visible behavior yet, by declared design).

1. Apps can declare a jobs manifest permission — justified (three reported bugs; merged spec records no existing mechanism)
2. ctx.job SDK: register kinds, start/cancel deduped runs — justified; zero consumers, declared
3. Owner-only /api/apps/{app}/_jobs/* routes (start/cancel/get/active/recent) — zero consumers, declared
4. Stale running records flip to interrupted at boot and on enable — justified (mirrors the recovery rule the spec documents in Code Review Sage)
5. Disable now stops workers and deletes run records, reported in the result — justified
6. New SEL audit lines for job access, mutations, reconcile, cleanup — justified (audit boundary)
7. Security-posture panel entry describing _json_safe/step/progress scrubbing — contradicted by the diff
8. registered_apps(), kinds(), is_cancellable() public helpers — zero consumers
9. QUEUED state constant — zero producers
10. hooks_integration.py black-reformat hunks and baseline prune — rides along, undeclared

Watch

• The security_posture.py NON_EGRESS_REDACTION_MODULES entry for apps/job_sdk.py says "_redact covers step, error and each progress line, and _json_safe recurses through nested structures" — the shipped module has no _json_safe, no step, no progress channel (grep _json_safe|progress in job_sdk.py: 0 code hits). The registered posture description asserts protection that does not exist; it will mislead the next security read.
• The permission key, route family, and on-disk state vocabulary become public App Kit surface at the next release whether or not P2 lands. Declared and spec-backed, but the routes have zero callers until an app migrates — shipping them with P2's first consumer was the smaller P1.
• The three hand-rolled siblings the spec names (AWS Control, Code Review Sage runs.json, Dev Fleet _RUNS) all remain — accepted-and-deferred to P2, declared.

Subtractions

• Shrink the apps/job_sdk.py entry in security_posture.py to the one true sentence: _persist scrubs error, the only runner-supplied field.
• Delete registered_apps(), kinds(), is_cancellable() from job_sdk.py — 0 non-test consumers each (grepped the patch; only test_job_sdk.py calls them), and cancellable is already served per-run by _public_view.
• Drop QUEUED — no code path ever sets it (grepped: definition, dataclass default, one test); default JobRun.status to STARTING.

[FIRST-PRINCIPLES-REVIEWED] d633f61

[github-actions]

GPT 5.6 Review — ✅ human override accepted

Human judgment by @chenmingwei23 overrides the GPT 5.6 finding for d633f61199c20a95330db29cbc89a7d61e17a9d4; the recorded reason is authoritative for this commit.

This comment is updated in place on each push.

The model was not re-run because an authorized human decision supersedes it.

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

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

Non-blocking: the initial job-record write omits the discard guard, leaking an orphan record when a disable wins the start race.

FINDING — src/kiro_crew/apps/job_sdk.py:585 — if not self._persist(run): omits handle, so if remove_all_async (app disable) acquires the lock after start claims into _live but before this write — setting _closed, marking the handle discarded, clearing _live/_keys, and running _store.remove_all() — the resuming _persist(run) skips the handle.discarded.is_set() check (only reached when a handle is passed) and re-writes the deleted STARTING file, after which the guarded launch sees entry is None and raises; the record leaks despite the module's "records stay deleted" contract, CleanupResult.is_clean already reported True, and re-enable surfaces a phantom INTERRUPTED run → Fix: pass the handle to the initial write — if not self._persist(run, handle): — matching every other write path in the file.

[OPUS-REVIEWED] d633f61

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

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

[chenmingwei23]

Isolated-pod end-to-end pass: 9/9 green, and it found one real defect

The Manual verification section promised this and said not to merge until it landed. Run in an isolated pod against this branch (never the live gateway); pod taken down afterwards with zero residue. A throwaway test app was installed into the pod only -- nothing in this diff -- declaring jobs: true and registering two kinds from its on_startup: one fast kind returning a dict, one cancellable kind looping on handle.cancelled while calling handle.progress(...).

#	Must-have	Result	Evidence
1	Gateway boots with the new modules; routes registered; reconciliation runs	PASS	health 200; _jobs/active served; reconciliation flipped 2 stale records on restart
2	Wiring point 1 -- _jobs/active matched before the app catch-all	PASS	GET /api/apps/<app>/_jobs/active -> {"runs": []} HTTP 200, i.e. the job handler, NOT a 404 from the app's own dispatch table
3	Real run reaches done, record on disk, params withheld	PASS	POST _jobs/fast/start -> run_id=b9c4628c..., settled done with result={'answer': 42, ...}; record present under data/jobs/<id>.json; response carries no params key
4	Cancellable run: progress recorded, cancel settles cancelled	PASS	observed pct=5 step=iteration-5; POST cancel -> cancelling=true; settled cancelled
5	dedupe_key adopts the in-flight run	PASS	two starts with the same key both returned run_id=078cc0a8...
6	Wiring point 2 -- foreign-origin running records reconcile	PASS	written while the pod was down; after boot both are interrupted, and the reason differs by runner presence: the gateway restarted while this was running vs the gateway restarted and no runner is registered for 'nonexistent-runner'
7	Disable drops the records; a live worker cannot recreate one	PASS	6 records -> 0; endpoint then answers app_disabled 403. This exercises the discarded mark in a real gateway
8	Owner gate	PASS	no-cookie request -> 403
9	An app without the permission	PASS	GET /api/apps/command-bar/_jobs/active -> {"error": "app has no job runtime", "code": "jobs_not_enabled"} 404

Both wiring points are the ones a unit test structurally cannot reach, which is why this pass existed.

The defect it exposed, fixed in cc6e23c8d

The QA pass hand-wrote its stale records with dashed run ids, and reconciliation then did nothing at all for that app -- every remaining run stayed stuck at running, which is the exact state the pass exists to clear.

Cause: JobStore._path raises ValueError on a run id it will not turn into a path, and reconcile caught only OSError, so the ValueError escaped the loop and abandoned every record after the bad one. Reconciliation is the one path in the SDK that consumes records it did not write -- a file from an older build, or hand-edited during an incident -- so a single unusable record has to cost only itself.

Fixed by catching ValueError alongside OSError per record, and the docstring now says why that path is different. Pinned by TestReconcilePoisonRecord, which places the bad record first in sort order so a regression abandons the healthy sibling behind it. Mutation-verified: with the guard narrowed back to OSError the test fails with ValueError: invalid run id: 'not-hex-at-all' escaping reconcile; with the fix it passes.

Suite is now 73 tests. Coverage stays at 99% on job_sdk.py and 98% on job_routes.py.

One pre-existing note, not a change here

A third-party app's installed.json must carry a "name" field. Without it InstalledApp.from_dict defaults the name to the empty string, the startup loop then processes the app under an empty name, and the module loader resolves its hooks against the parent apps/ directory instead of the app's own subdirectory -- so the hook silently never fires. Every real install path writes the name, so this is not reachable in normal use and is not a regression from this PR, but it is a confusing failure mode for anyone hand-assembling an app. Recorded locally rather than folded into this diff.

[chenmingwei23]

GPT dispositions -- all six fixed in 0c820d73e

Every one held against the code. None was a false positive and none needed pushing back on as disproportional.

1. hooks_integration.py:420 -- "Permission removal preserves a privileged stale SDK" -- fixed.

Permission removal preserves a privileged stale SDK

Correct, and worse than the disable path alone: the SDK was published at enable time and lived for the gateway's life, so a jobs grant revoked in the manifest afterwards kept serving through the registry entry, and my forget_sdk sat inside the if permissions.get("jobs") block so a revoke-then-disable skipped it too. Two changes: forget_sdk now runs unconditionally on disable, outside the grant check; and, cause-level, authorization no longer rests on the registry at all -- the route guard re-reads the manifest through a new _enabled_and_permitted helper (off the loop) and refuses on a revoked grant even while the SDK is still published. The registry now only answers "where are the runs", never "may you". Pinned by test_revoked_grant_is_refused_even_with_an_sdk_registered, which asserts the 404 while get_sdk still returns the SDK.

2. job_sdk.py:273 -- "Persistence failures leave records falsely running" -- fixed.

Persistence failures leave records falsely running

Held. A failed terminal write was logged and dropped, leaving the record running while the work was finished -- and reconciliation could not clean it up, because it skipped anything carrying this process's origin. Two changes: the terminal write is retried once (_write_terminal), and reconciliation's predicate is widened from "foreign origin" to "foreign origin or not in the live table", so a record this process wrote and then lost is resolved rather than spared. Residual, stated in the docstring rather than papered over: after a double disk failure the record stays running for the remainder of that process's life, because reconciliation runs at startup; a periodic sweep is deliberately out of scope for P1. Pinned by test_a_transient_terminal_write_is_retried and the two reconcile tests that now cover both halves of the predicate.

3. job_routes.py:213 -- "Start reads an unbounded record on the event loop" -- fixed.

Start reads an unbounded record on the event loop

Held, and it was an inconsistency of mine: every other handler reads through asyncio.to_thread and this one did not, so _handle_start did a blocking JSON read (up to the full progress tail) on the loop thread. Now wrapped in to_thread like its siblings.

4. job_routes.py:91 -- "Runner output bypasses credential redaction" -- fixed.

Runner output bypasses credential redaction

Held, and again an inconsistency that was itself the leak: I redacted run.error but served run.lines and run.result verbatim, so a runner that shells out could put a credential in a progress line and have it rendered. Fixed at ingest rather than at the response boundary, so the record on disk is clean too: JobHandle.progress redacts each line, and _execute redacts the string leaves of a runner's result dict (non-string values are untouched). Pinned by test_progress_lines_and_result_strings_are_scrubbed, which asserts the credential is absent from the on-disk JSON, not just from the response.

5. test/test_job_sdk.py:74 -- "Daemon workers can recreate per-test directories after teardown" -- fixed.

Daemon workers can recreate per-test directories after teardown

Held. Runs are real daemon threads, and one still alive when pytest removed tmp_path would mkdir and write into the deleted tree at its next progress or terminal write -- a real file mutation racing the fixture. The sdk fixture now marks every live handle discarded and cancelled, then bounded-joins each thread and asserts it died. Waiting is the correct answer here rather than cancelling: cancelling the wrapper would leave the thread running, which is the defect and not the fix.

6. job_sdk.py:99 -- function-local imports violate top-level-imports -- fixed.

Fix: move asyncio and security imports to module scope.

Done: asyncio and redact_credentials/redact_exfiltration_urls are module-scope in job_sdk.py. I checked before moving them -- kiro_crew.security imports only executors, sel, trust_patterns and vector_memory_constants, so there is no cycle to justify laziness there.

One deliberate exception remains, and it is not an oversight: the two dashboard owner-guard imports inside job_routes._guarded stay function-local because moving them to module scope breaks gateway boot. kiro_crew.dashboard.handlers pulls handlers.security -> apps.routes -> apps.hooks_integration, and hooks_integration is the module that imports job_routes to mount the routes. That was a real ImportError, found by importing the module for real, and it is the same exemption cron_sdk documents for its mcp_cron vetting imports. The comment at the import site names the cycle.

[chenmingwei23]

Editor's note: the machine-readable ai-review-disposition marker was removed
from this comment. It predates the one-span-per-comment disposition contract --
it named no span= finding identity and covered several findings at once, which
the readiness gate counts as a malformed record. Every finding it describes was
fixed on a head that is now superseded, and the reasoning below is unchanged.

Opus disposition -- the dedupe race, fixed in 0c820d73e

two near-simultaneous starts with the same dedupe_key (the double-click / two-tabs case the docstring says is protected) both run on executor threads via start_async -> to_thread(self.start), both call _find_live_by_key and get "" because neither has yet reached the separate claim

Held exactly as described, and it made the docstring's guarantee false: _find_live_by_key took the lock, released it, then read files, and the insertion into the live table was a second critical section much later -- with the initial record write in between. So two racing starts both saw no owner, both claimed, and both did the work. That is precisely the cost dedupe_key exists to avoid, and the case named in the spec is a double click.

Fixed by making the check and the claim one critical section with no I/O inside it:

• a new in-memory _keys index maps (kind, dedupe_key) -> run_id for runs live in this process, so the lookup needs no disk read and can sit in the same with self._lock block as the live-table insertion;
• the record, handle and unstarted thread are built before the lock, so the critical section holds only two dict operations;
• _find_live_by_key and its disk scan are deleted rather than left as a second, weaker path;
• the record write moves after the claim and before thread.start(), so the write still has no competing writer, and a write failure releases both the claim and the live entry instead of leaving the key owned by a run that never started;
• _execute releases the key when the run finishes, and remove_all_async clears the index under the same lock that clears the live table -- otherwise a key would stay owned by a run whose record no longer exists and the next start would adopt a ghost.

Pinned two ways: test_two_racing_starts_yield_one_run drives two threads through a threading.Barrier and asserts both callers got the same run id and that only one runner body ever ran; test_a_failed_initial_write_releases_the_dedupe_claim asserts the same key still works after a failed write.

This is the same defect class as the slot-claim race on #5195 -- an await/read placed between a guard and the claim it is guarding -- so the fix is the same shape: claim in the same step as the check.

[chenmingwei23]

Round 2: all five held, and four of them collapse into one restructure -- 4f4193d60

These were five NEW defects, not repeats of round 1, and every one held. But three of them (job_sdk.py:190, :195, :242) plus round 1's terminal-write finding are all the same shape: the record write path had four independent writers and each had to remember the same set of rules, and a different one was missed each time. So rather than patch four instances I restructured the write path around a single guarded writer. That is the substance of this round; the per-finding notes below say where each landed.

The restructure. JobSDK._persist(run, handle=None) is now the ONLY thing that writes a record. It holds one lock acquisition, and inside it: re-checks the discard flag, writes, and converts any failure into False instead of an exception. All four callers -- start, JobHandle.progress, the worker's terminal write, and reconcile -- go through it. JobHandle no longer holds a store; it holds the writer. Separately, _json_safe sanitizes everything a runner supplies at INGEST, so a record is JSON-safe and redacted by construction rather than at each writer.

1. job_sdk.py:190 -- "Cleanup races an already-started write" -- fixed by the restructure.

Held, and it is the honest limit of my round-1 fix: progress checked discarded and then wrote, so cleanup could delete the file in between and the write recreated it. I had narrowed the window, not closed it. Now the discard check and the write happen inside _persist under one lock acquisition -- the same lock remove_all_async sets discarded with -- so there is no check-then-act window left. Pinned by test_progress_on_a_discarded_handle_writes_nothing, which now runs against the real guarded writer rather than a stand-in.

2. job_sdk.py:242 -- "Non-JSON values corrupt live-run bookkeeping" -- fixed.

The sharpest finding of the round. json.dumps raises TypeError on a set, a Path, or any object; my handlers caught (OSError, ValueError), so a TypeError escaped the terminal write from inside the finally and skipped the _live.pop and _keys.pop below it -- leaking a dedupe claim that no later start could ever release. Two changes: _json_safe makes an unserializable record impossible at ingest (non-JSON values become their repr, recursively), and _persist returns False on any failure so no exception can skip a caller's bookkeeping again. Pinned by test_non_serializable_result_still_persists_and_frees_the_claim, which asserts the record persisted AND that a second start with the same key gets a new run.

3. job_sdk.py:195 -- "Runner output can expose credentials" -- fixed.

Held: my round-1 redaction covered error and top-level result strings but skipped step entirely and left strings nested inside dicts and lists untouched. Opus's finding on this head says the same thing more precisely. Redaction is now part of _json_safe, so it is recursive and applies to step, lines, result and params alike. Pinned by test_step_and_nested_result_strings_are_redacted, which asserts the credential is absent from the on-disk JSON.

4. hooks_integration.py:420 -- "Teardown can orphan live job workers" -- fixed.

Held, and it is embarrassing in a specific way: round 1 raised exactly this hazard about the TEST fixture, I fixed it there, and left the production path signalling-only. A disabled app's workers kept running -- doing real, side-effecting work with their records already deleted. remove_all_async now marks and clears under the lock, then bounded-joins every worker outside it (5s, since holding the lock while joining would deadlock against the worker's own final write), and reports any that outlive the deadline. Pinned by test_remove_all_waits_for_a_cooperating_worker, which asserts the worker is already finished when cleanup returns.

Accepted residue, stated rather than hidden: gateway shutdown is not drained. These are daemon threads, so the interpreter reaps them at exit; draining every app's runs there would delay shutdown for work nobody is waiting on. The docstring says so.

5. job_sdk.py:274 -- "Failed record deletion is reported as successful" -- fixed.

Held. remove_all counted only successes, so a partial delete read as a clean one and disable claimed the runs were gone while records remained -- while the cron contract it mirrors explicitly reports a failed cleanup. It now returns (removed, failed), and the disable path reports partial: removed N, M run record(s) remain with a partial SEL outcome. Pinned by the unlink-failure test, which now asserts (0, 1).

---

83 tests pass (twice, stable). Coverage 97% on job_sdk.py, 98% on job_routes.py. flake8, mypy (378 files), isort and black clean.

A note on the shape of this round, since the spans repeat: round 1 and round 2 landed different defects in the same two spans, which is the signal that patching instances was the wrong response. The restructure is the answer to that, not another four patches. If a round 3 lands more findings in the write path I will treat it as a design question rather than continuing to patch.

[chenmingwei23]

Editor's note: the machine-readable ai-review-disposition marker was removed
from this comment. It predates the one-span-per-comment disposition contract --
it named no span= finding identity and covered several findings at once, which
the readiness gate counts as a malformed record. Every finding it describes was
fixed on a head that is now superseded, and the reasoning below is unchanged.

Opus disposition: the redaction floor was too shallow -- fixed in 4f4193d60

the ingest-time _redact floor skips self._run.step = step entirely and covers run.result only at the top level (_redact(v) if isinstance(v, str) else v leaves strings inside nested dicts/lists untouched), so a runner's step label or a nested result string carrying a credential still reaches disk and the response

Held exactly, on both counts, and the second is the part I got wrong by being too literal: I wrote the top-level comprehension thinking "only string leaves can carry a credential", which is true, and then only checked the leaves one level deep. A {"outer": {"inner": ["key=..."]}} walked straight through.

Fixed by making redaction part of a single recursive _json_safe pass rather than a hand-rolled comprehension at one call site:

• every string at any depth is redacted, so step, lines, result and params are all covered by the same code;
• the same pass also coerces non-JSON values to their repr, which is what fixes GPT's separate finding that a TypeError from json.dumps used to escape the write and skip the live-table and dedupe-key cleanup;
• depth is capped so a runner cannot make the sanitizer recurse without bound;
• it is applied at INGEST -- progress, the result capture in _execute, and start's params -- so the record on disk is clean too, not just the HTTP response.

Pinned by test_step_and_nested_result_strings_are_redacted, which puts a credential in a step label AND inside a nested list, then asserts it is absent from the raw on-disk JSON as well as from the parsed record. The existing top-level test stays, so both depths are covered.

[chenmingwei23]

Root fix: the launch window is now a named state and a guarded transition.

Head 3976537ec. The maintainer ruled on the scope question from round 7: fix the
class rather than the window. Four rounds of point patches in claim -> write -> start said the sequence had no name for the state it passes through, so cleanup,
the launcher and the record were each guessing at it separately.

What was actually wrong. start claims a run into _live inside the lock,
releases, does a blocking disk write, and only then launches the thread. For the
length of that write the entry exists with no thread behind it, and three different
readers drew three different wrong conclusions from that: _join_workers assumed
every entry it snapshotted was joinable, the launcher assumed it was still allowed
to start, and the record already claimed running. Each earlier fix corrected one
reader and left the others.

What changed.

• STARTING is now a real status. start writes it, and the WORKER completes the
  transition to RUNNING as its first act, because the worker is the only party
  that knows it is running. The record can no longer assert a worker exists before
  one does, which is what list_active was serving.
• _Live.started is the single authority on whether there is a thread to join,
  flipped only inside the lock. _join_workers reads it and skips an unstarted
  entry, which cannot be stubborn -- no app code is executing. Previously it joined
  unconditionally, Thread.join() before start() raised RuntimeError,
  _cleanup_app_jobs caught only OSError, and the disable died before
  remove_all() -- so a disabled app kept its records AND any worker that had
  started.
• The launch is a GUARDED transition in a second critical section: it re-reads
  _closed and the handle's discard flag under the same lock cleanup marks with,
  and unwinds instead of starting if either is set. This is the window _closed
  could not cover on its own -- that flag refuses a start arriving after cleanup,
  and this one had already claimed before the snapshot.
• The thread-start failure path now writes its terminal record THROUGH the handle.
  It previously called _persist(run) with no handle, skipping the discard check,
  so it could recreate a record cleanup had already deleted. Nobody had reported
  that; it is a resurrection of exactly the kind round 2 fixed elsewhere, and it
  was introduced by round 3's own fix.

Also closing GPT's second finding at the same chokepoint: from_dict now coerces
each known field to its declared type. A record whose error was a number reached
_persist, where the slice after the redaction raised TypeError outside that
method's own try -- the same blast radius as a non-object body, one level in. bool
is checked before int on purpose, since bool is an int subclass and an int-first
test would accept True as a pid.

Still holding on GPT's third: a stubborn worker is reported but does not fail trust
withdrawal. The bound is deliberate -- a runner that never polls its handle must not
block an app's disable forever -- and changing it trades a longer stall for a disable
that cannot complete, which is a product call rather than a defect.

Verification. 108 tests, coverage 97% on both new modules; flake8, isort, the
black gate and mypy over 1178 files clean; the pair run three times for stability.
All four parts of the fix are mutation-verified with syntactically valid mutations,
one at a time: unconditional join, unguarded launch, RUNNING as the initial
status, and no field coercion each turn their own pin red. The status pin observes
the FIRST WRITE rather than what the worker sees, because a test that only checks
the worker would stay green if the initial status regressed.

One test changed for a reason worth naming: test_two_racing_starts_yield_one_run
read entered the instant both starts returned, which was only ever true because
the gap to the runner body was short. The worker's new first write widened it, so
the assertion now waits for the body. That was a latent timing assumption in the
test, not a behaviour change -- the dedupe property it exists to prove is asserted
unchanged.

[chenmingwei23]

Two of three fixed. The third is a third raise and needs a maintainer ruling, not another patch.

Head d633f6119. Both test-hygiene findings were correct and are mine, so they are
fixed. The third is the same item I have now held twice, and holding it a third time
would not clear a hard gate.

test_job_sdk.py -- a stubborn worker outliving its test. Correct. That test
deliberately starts a worker that ignores its cancel signal so cleanup has to wait
out the deadline, then asserts the event loop kept ticking -- and never waited for the
thread. The loop-not-stalling assertion says nothing about the thread still running,
and a daemon thread that survives the test keeps holding the SDK's lock and writing
records while later tests execute. It now retains the thread and boundedly joins it
after releasing it, asserting it is actually gone.

test_job_routes.py -- the fixture abandoned workers. Also correct. The fixture
forgot the SDK from the registry, which makes a running worker unreachable but does
not end it, so a route test that started a run and then failed an assertion left it
executing. Teardown now runs the SDK's own remove_all_async, which already discards
every handle under the writer's lock and bounded-joins each worker -- a hand-rolled
join in the fixture would be a second copy of that contract, free to drift.

Worth recording because it is the more useful half: my first version of that fixture
fix did nothing at all, and the suite stayed green. asyncio was not imported in
that module, and the except Exception I had wrapped the teardown in swallowed the
resulting NameError -- so the cleanup silently no-opped 108 tests in a row. The
guard is now except OSError, which is the only failure teardown may legitimately
shrug at, and a programming error surfaces instead of hiding. Two process notes from
it: a broad except in a teardown is indistinguishable from no teardown, and the
flake8 invocation that would have caught the undefined name had a file list that
omitted the file just edited.

Not fixed: trust revocation succeeding while a worker remains active. This is the
third round this has been raised, and my position has not changed on the substance --
the join deadline is a deliberate bound, because a runner that never polls its handle
must not be able to block an app's disable indefinitely. What has changed is that a
rebuttal cannot clear a hard gate, and this is not a false positive, so an override
would be laundering a real finding rather than dismissing a nitpick.

The prescribed fix -- make still_running fail teardown before forgetting the SDK --
is implementable, but it changes what disable MEANS: today disable always completes
and reports honestly what it could not stop; under that change a disable can fail, and
the caller has to decide what an app in that state is. That is a product decision
about the disable contract, and it is the maintainer's to make, not something to
settle inside a review loop. Both alternatives are cheap to build once the semantics
are chosen.

Verified on this head: 108 tests, three consecutive runs; flake8 and isort clean over
all five changed files this time; mypy clean over 1178 files; the black gate and both
ratchets green.

[chenmingwei23]

Disposition: the disable contract is decided semantics, not an unfixed defect

Head d633f6119. This answers GPT's sole remaining blocking finding,
hooks_integration.py:332 -- "trust withdrawal succeeds while a job worker remains
active" -- which has now been raised on three consecutive heads. The maintainer has
ruled on it, so this is the disposition rather than another patch.

The mechanism GPT describes is real, and I am not claiming otherwise.
remove_all_async marks every live handle discarded and cancelled under the writer's
lock, then bounded-joins each started worker for _CLEANUP_JOIN_SECS. A runner that
never polls handle.cancelled outlives that deadline. Its records are gone, its
registry entry is dropped, and it keeps executing until it returns on its own. So a
disable can complete while one thread of the app is still running. That is accurate.

It is the intended contract, and the alternative was considered and rejected.

The deadline is bounded on purpose. A worker thread cannot be killed -- Python offers
no safe primitive for it -- so the only two options are to wait or to stop waiting.
Waiting without a bound means any app can make its own disable un-completable simply
by shipping a runner with a long loop that never checks its cancel signal. Disable is
a user-initiated, user-visible operation; it must terminate.

The prescribed fix -- retain timed-out workers and propagate partial cleanup as a
teardown FAILURE -- does not remove the running thread. It converts "disable
completed, and here is what it could not stop" into "disable failed", and then
someone has to define what an app in that state is: still enabled, half-enabled,
retryable, or blocked from re-enable. That is a change to what disable MEANS, not a
bug fix, and it is the maintainer's call. The call is to keep the current semantics.

The residue is surfaced, not swallowed. This is what separates the current
behaviour from the failure mode the finding is written against:

• CleanupResult.still_running is a field precisely so a cleanup that left app code
  executing cannot be reported as clean; is_clean is false whenever it is non-zero.
• The disable result carries job_cleanup: partial: removed N, M worker(s) still running, so the operator sees the count in the response, not only in a log.
• A warning names each thread that outlived the deadline.
• The SDK is also closed to new work under the same lock, so nothing new can start
  after teardown begins -- the thread that survives is one that was already running,
  never a new one.

So the honest summary of the state after a disable with a stubborn worker is: no new
work can begin, the records are gone, the app is deregistered, one already-running
thread finishes on its own, and every layer reports that this happened.

What would reopen this. If a runner could be interrupted safely -- a cooperative
checkpoint the SDK could inject, or a process boundary per run rather than a thread --
then "disable leaves nothing running" becomes achievable without making disable
failable, and this would be worth revisiting. A process-per-run model is the plausible
route and is far beyond this PR's scope. Until then the choice is between a bounded
wait that reports honestly and an unbounded one that cannot be relied on to finish.

Realized as /ai-review override gpt below, because a hard gate cannot be cleared by
a comment. Not a claim that the finding is a false positive: it is a claim that the
behaviour is the designed contract and the reviewer is not the venue to reverse a
maintainer's product decision. It is the only blocking finding on this head, and Opus
4.8 reports no blocking findings on the same head, so nothing else is being suppressed
alongside it.

[chenmingwei23]

/ai-review override gpt d633f61: Maintainer ruling: this is the intended disable contract, not an unfixed defect. The join deadline is bounded on purpose so a runner that never polls its handle cannot make its own disable un-completable, and the residue is surfaced not hidden -- CleanupResult.still_running exists for it and disable reports "partial: N worker(s) still running". The prescribed fix lets disable FAIL, which redefines what disable means and was decided against. Sole blocking finding; Opus clean on this head.

[github-actions]

Human judgment recorded

@chenmingwei23 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for d633f61199c20a95330db29cbc89a7d61e17a9d4.

Maintainer ruling: this is the intended disable contract, not an unfixed defect. The join deadline is bounded on purpose so a runner that never polls its handle cannot make its own disable un-completable, and the residue is surfaced not hidden -- CleanupResult.still_running exists for it and disable reports "partial: N worker(s) still running". The prescribed fix lets disable FAIL, which redefines what disable means and was decided against. Sole blocking finding; Opus clean on this head.

This decision applies only to this commit. A new push requires a new judgment.

[buluoray]

Approving — sound foundation. Three asks, none needing another review round.

Reviewed d633f61199c20a95330db29cbc89a7d61e17a9d4 independently: read job_sdk.py and
job_routes.py in full plus all four backend diffs, ran the tests, and reproduced one of the
races. The design holds up. The scope cut — P1 records that a run exists and how it ended,
with no params, no progress, no result — is the decision that makes the rest defensible,
and it is the right response to what the five review rounds actually found: each round located a
different unsanitized field, so removing the arbitrary-payload channels removes the class instead
of patching the sixth instance. File-per-run to settle writer-vs-writer, read_bytes_with_retry
for reader-vs-writer on Windows, a per-process origin token instead of a pid for staleness, and
authorization re-reading the manifest rather than trusting the process registry — each is the
right call, and each is argued in the code rather than asserted.

Two of the three asks below are text-only, and the third is one word. I would like all three
landed before you merge, but none of them changes the shape of the change.

ASK 1 — a governed security file describes machinery this head removed

src/kiro_crew/security_posture.py:1246

The NON_EGRESS_REDACTION_MODULES justification for apps/job_sdk.py says _redact covers
"step, error and each progress line", and that "_json_safe recurses through nested
structures (dict KEYS as well as values)". None of that exists on this head: job_sdk.py has no
_json_safe (repo-wide, the only other hit is apps/builtins/md_notebook/notes.py), no step
field, and no progress channel. The registered-sink entry above it also cites "a dedupe key ...
carried out of the SDK's own exception"; no JobError message quotes the dedupe key.

This file is the inventory a security reviewer reads to decide whether a redaction call site is
classified correctly, so a stale justification is worse than a thin one — it asserts coverage the
code no longer provides. The classification itself is right; only the mechanism text needs to
match: error is the sole runner-supplied field, _redact runs on it inside _persist, and it
is truncated to 2000 chars.

ASK 2 — the new manifest field, SDK surface and route family ship undocumented

docs/app-kit/manifest-reference.md carries a permissions table and a dedicated
permissions.spawn — Background Agents section with an API pointer. jobs appears in neither,
and the example manifest block still lists only cron and spawn. The spec this PR names,
docs/system-specs/features/app-sdk-durable-jobs-and-view-state.md, is current-state analysis
only — grepping it for JobSDK, ctx.job, _jobs or P1 returns nothing, so the shipped design
is written down nowhere.

AGENTS.md § Specification management requires the spec to be updated in the same commit as a
schema change. Docs Lint passed because it checks index and link integrity rather than coverage,
which is why this needs a human. The PR body already acknowledges a docs follow-up owing "one
correction to the merged spec" — that is narrower than what is actually missing, since P2 and any
third-party app author would be working from documentation that does not mention the seam at all.

ASK 3 — the initial record write bypasses the discard guard (reproduced)

src/kiro_crew/apps/job_sdk.py:585

if not self._persist(run): omits handle, so _persist's handle.discarded.is_set() check is
skipped on that one path. Opus flagged this non-blocking; it holds. Reproduced against this head
by pausing the initial write so a disable lands in the window:

start raised: JobError
CleanupResult(removed=0, failed=0, still_running=0)  is_clean: True
orphan record files left on disk: ['1a462c7378fa41a389fcd738d25d2237.json']

So a disable racing a start leaves a record behind and reports itself clean, which contradicts
the module's own "records stay deleted" contract. The leak is a starting record — non-terminal —
so list_active reports it as running work until the next reconcile flips it to interrupted:
a phantom run for work that never began, which is the symptom class this SDK exists to remove,
arriving from the SDK itself.

Opus's fix (pass handle) is correct and one word. Worth considering the cause-level variant
instead: check self._closed inside _persist's critical section, which covers every handle-less
writer at once rather than asking each call site to remember to pass a handle — the same "enforce
in one place, not per call site" reasoning this PR already applies to _redact. _closed is set
only by remove_all_async, and the module already declares disable terminal for the instance
("a re-enable builds a new one"), so refusing post-close writes is exactly the contract. I did not
check whether an existing test writes a record after close, so that variant needs a test run.

Non-blocking observations

• Two comments now overstate. _persist holds self._lock across self._store.write(), a
  disk write, but __init__'s comment still says the lock "guards two small dicts with no awaits
  inside" and start's docstring still says it is "safe on the event loop: the only blocking work
  is one small atomic_write". Both were true before the guarded-writer restructure; now an
  on-loop start can also block acquiring the lock while another run's terminal write is on disk.
  No deadlock — remove_all_async releases the lock before joining — just stale prose.
• A disabled app can still run a full runner body. _execute writes the RUNNING transition
  through the guarded writer, ignores the result, then calls runner.fn(handle) regardless. With
  cancellable=False (the default), a runner whose disable landed just after thread.start()
  executes its whole side-effecting body. Reporting it as still_running is a consistent answer;
  a if handle.discarded.is_set(): return before the try would close the sub-window where the
  runner has not begun. Your call.
• Test count in the body. It says 95; test_job_sdk.py plus test_job_routes.py is 108
  passing locally in 10.4s. Presumably written on an earlier head.
• Caller-supplied strings are treated differently on two paths. _guarded audits
  request.path before the kind is validated, so an arbitrary caller-chosen {kind} reaches SEL
  unredacted, while start deliberately keeps dedupe_key out of the gateway log for exactly
  that reason. Auditing request.path is the repo convention, so this is consistent rather than a
  new hole — noting only the asymmetry.

What I verified

• 66 checks green, 4 skipped, 0 non-green, nothing in flight. All five bots carry a verdict on
  this head.
• Route ordering: register_job_routes(app) runs before RouteRegistry(app) /
  ensure_catch_all() in init_hooks_system, and active / recent are registered before
  {run_id}. init_hooks_system has one production call site (dashboard/server.py:3147), so
  there is no duplicate-registration hazard on the aiohttp router.
• Authorization does not rest on the registry: _enabled_and_permitted re-reads the manifest
  off the loop, and the revoked-grant 404 is pinned while get_sdk still returns the SDK.
• 108 tests pass locally on this head.
• No missed frontend wiring. AppPermissions in website/src/app-sdk/index.ts carries only
  api and events, and AppDetailPage.tsx badges storage / cron / network / memory but
  not spawn either — so jobs needing no frontend surface is consistent with how spawn is
  already handled, not an omission.

Not verified: mypy and flake8 locally (CI reports both green), and the isolated-pod 9/9 E2E
table, which I took at face value rather than re-running.