fix(apps): three places the Job SDK asserted a fact it never verified

[chenmingwei23]

Problem / Motivation

Four places in the newly-landed Job SDK (#6682) wrote or served a claim the SDK
had never verified, and review found a fifth inside this PR's own new code. Each
is silent -- the record reads as an ordinary success or an ordinary failure, so
nothing tells the reader the claim is wrong.

1. register() accepted a coroutine function. _execute calls fn(handle) on a
   worker thread and discards the return value by design, so an async def
   runner returns a coroutine that is dropped un-awaited: no line of the runner's
   body executes, and the terminal write records done.
2. reconcile() flattened three distinct events into one record. It assigned
   status over the top of the only evidence of how far a run had got, then
   chose error from whether a runner is registered -- so a run that died
   between claiming and launching was written "the gateway restarted while this
   was running", which is false by the module's own definition of starting.
3. JobStore.read crashed on a record whose bytes it could not decode, so a
   damaged record took down the lookup instead of reading as absent.
4. _execute set status = RUNNING, called _persist, and discarded the return.
   A lost write left the record at starting while the runner went on to commit
   real work; if the process then died before the terminal write, reconcile read
   that record and stated the run had never begun -- while side effects existed.
   Found by review on this PR, in the code this PR added interrupted_from to.
5. reconcile chose the interruption cause from whether a runner is registered,
   so a record minted by THIS process with no live worker was written "the gateway
   restarted while this was running" -- while the gateway was still up. Found by
   review, in the cause vocabulary this PR itself introduces.

Why it matters

A durable run record is only worth having if what it says is true. All three
failures are silent and all three mislead in the direction of false confidence:

• A run that reports success having executed nothing is the worst outcome
  available here. The only trace is an un-awaited-coroutine warning in the
  gateway log, and an app author gets a green done for work that never ran.
• A consumer cannot tell a run that may have committed side effects from one
  that provably committed none. Code Review Sage writes two different recovery
  messages for exactly that distinction: a review interrupted from starting
  posted nothing, while one interrupted from running may have already delivered
  comments to a pull request.
• A 500 where a 404 is the honest answer is unactionable for a client, and the
  two readers of the same file disagreed about identical damage: iter_runs
  twelve lines below already treated an unreadable record as merely skippable.

What changed (motivation -> approach -> change)

One PR rather than three because there is one story: the record or the API
asserted a fact the SDK never verified. Each fix closes the gap at the point
where the answer is still cheap to get.

D1 -- a run that did no work is recorded failed, not done. The symptom is
a done record for work that never ran. There are two halves, and only one of
them is the safety property.

The registration guard is a fast, friendly error. register() refuses a
callable whose invocation cannot do work: a coroutine function, an async
generator function, or a plain generator function -- called, each returns a lazy
object, the body never executes, and the record says done. The check goes
through __call__ as well as the object itself, because that is what invocation
reaches: inspect reports a callable instance as an ordinary object, so a
check written against bare functions lets an async def __call__ straight
through. It runs BEFORE the runner-table write, so a refused kind leaves no
runner behind. Its value is timing and a clear message -- the app author hears
about it in their own on_startup, not from a run record later.

The execution check is the safety property. A shape check is a proxy and can
only refuse spellings somebody thought of. Measured, three forms and what each
guard sees:

form	registration guard	execution check
async def runner	refused	n/a
instance with async def __call__	refused	n/a
def run(h): return _do_it(h) over an async def	passes	failed
ordinary sync runner	passes	done

Row three is the one that matters: a plain wrapper returning a coroutine is an
ordinary refactor, not an exotic construction. It is a plain function, so every
shape predicate says it is fine; it runs far enough to construct the coroutine,
hands it back, _execute discards it by design, and the work never happens. No
guard that enumerates callable shapes can ever see it.

So after runner.fn(handle) returns, if the result still needs driving -- an
awaitable, or an async generator -- the run is recorded failed with an error
naming the cause, and the result is closed so it does not warn from the garbage
collector at a point far from the run that caused it. That makes the whole class
unreachable however the callable was written, including forms nobody has thought
of yet.

Two boundaries, both deliberate. The new branch sits inside the existing
try and only sets fields -- the terminal write below is still the one write
site, because one-writer-per-run is what keeps a lost write from leaking a dedupe
claim (pinned by a test that counts the write calls in _execute). And a
returned plain generator is not treated as failure: generator functions are
refused at registration, while a generator object handed back by a runner that
did its work is ambiguous in a way an awaitable is not.

Deliberate scope line: driving coroutines properly is a design question,
refusing them is not. Doing it right means deciding which loop owns the
coroutine, how cancellation crosses the thread boundary, and what a blocking
runner does to that loop. This change answers none of those. It answers only the
question a caller cannot currently ask -- "did my runner run".

D2 -- record both axes of an interruption and derive the message. The root
cause is that one string was carrying two independent facts. They are
independent because they describe different times: how far the run got (past)
and whether the kind can be serviced now (present). Neither implies the other,
and a consumer reads them for different decisions -- whether to warn about
partial work, and whether a retry is even possible.

• interrupted_from holds the prior status, drawn from the existing
  QUEUED/STARTING/RUNNING constants. No new vocabulary.
• interrupt_cause is a small closed set (process_gone,
  runner_unregistered), not a bool: reconcile's own docstring already admits
  the not-registered branch covers two events -- the app was disabled, or the kind
  was removed -- so a boolean would freeze that conflation on the day the field
  lands. Adding the third value costs one constant and one table entry.
• error is still populated, composed at ONE site from a table keyed on cause
  with the progress interpolated. With two axes a conditional needs four arms and
  grows multiplicatively; the table grows by one line per cause. The
  process_gone + running cell reproduces the previously shipped string byte
  for byte, because that cell was the only one that was ever true.

D4 -- read treats an unreadable record as absent. It caught
FileNotFoundError and NotADirectoryError, so a record whose bytes are not the
UTF-8 JSON this store writes crashed the lookup instead of reading as absent. The
fix adds ValueError (which covers the UnicodeDecodeError from the explicit
decode, and the malformed id _path rejects) and nothing else.

It deliberately does NOT catch PermissionError, and an earlier revision of this
PR got that wrong. That revision widened the tuple to OSError on the argument
that read's own adjacent comment is about the Windows PermissionError case, so
not catching it looked like an oversight. It was not. A sharing violation from a
concurrent os.replace is TRANSIENT -- read_bytes_with_retry exists to outlast
it -- so answering None claims "no such run" about a record that is present and
intact, and a client reading 404 as "gone" drops live work. The same holds for the
rest of OSError: EIO, EMFILE and EBUSY are facts about the host, not about
whether the run was started. Those reach the route as a 500, which honestly says
"ask again" instead of confidently saying the wrong thing.

That is the same defect this PR is named for, committed by one of its own fixes:
read asserted an absence it had not verified. It is now pinned from both sides --
a mutant that restores the OSError widening is killed by this PR's own tests and
independently by test_on_loop_read_propagates_permission_error_offloaded_read_retries,
which #7703 added to keep that error propagating.

The RUNNING transition is now a precondition, not a notification. _execute
called self._persist(run, handle) and threw the boolean away, then ran the body
as though it had succeeded. If the store cannot record that a run is running, the
SDK no longer runs it: the run is marked failed with a reason naming the write,
and runner.fn is never called.

The alternative -- execute anyway and simply never claim "never started" -- is
unachievable across a crash, because the knowledge that the write failed dies with
the process. Only refusing to start keeps every surviving record's claim true.

State the cost plainly: a transient store error now fails a run that might have
succeeded. That is a real regression in availability, chosen deliberately. For an
SDK whose product is a durable record, a run nobody can account for is worse than a
run not attempted, and the failure is at least visible and retryable by the caller
rather than silently misdescribed later. Neither branch adds a write site -- both
set fields and let the single terminal write persist them, which the source ratchet
pins at exactly one _persist and one _write_terminal inside _execute.

The interruption cause no longer lets a proxy outrank a fact. reconcile
picked between two causes on known (is a runner registered for this kind), and
both of those causes say the gateway restarted. But run.origin == _ORIGIN -- one
line above -- is positive evidence that it did not: this process minted the
record. So a record this process wrote, whose worker is gone from the live table,
was being told it had survived a restart that never happened. Not an unverified
claim but a counter-verified one, with the evidence in hand at the point of the
write.

Own origin is now tested first, because a verified fact must outrank a proxy, and
it selects a third cause: terminal_write_lost. Reaching that branch means
_write_terminal spent both attempts and logged that the run would be reconciled,
so what is genuinely unknown is whether the work finished -- and the message says
exactly that rather than guessing either way. The known split survives, scoped to
foreign origins where a restart really is the explanation.

This is reachable without any restart, which is what makes it a fix rather than
a rebuttal. reconcile_all has one call chain and aiohttp fires it once per
process, so the obvious reading is "reaching reconcile implies a restart happened".
That reading is wrong: reconcile_all runs AFTER the app loop, while
register_sdk runs BEFORE each app's on_startup hook -- so an app's startup hook
can start a job in this same process, and a job whose terminal write is lost is
then reconciled by the process that minted it. The rebuttal that the call chain
alone suggests would have been false.

The write-lost message states what is known, per axis. An earlier revision gave
that cause ONE sentence ending "whether its work finished is not known", which is true
for a running record and false for a starting one: making the running transition a
precondition (above) means a record still at starting whose terminal write was lost
proves the body never ran. The consequence clause is now DERIVED from the progress axis
-- exactly what the two-axis design is for -- so a starting record says its body
never ran and names the precondition, and only running says the outcome is unknown.
An unreadable status falls back to outcome-unknown, which is the honest default when
the axis itself cannot be read.

The cause is composed into a local and assigned once, so interrupt_cause keeps
the single-assignment-site property the ratchet checks. That ratchet had to be
rewritten: it pinned the exact TEXT of the old conditional, so a third cause failed
it for the wrong reason while the invariant it existed to protect was intact. It
now asserts the invariant -- one assignment site, and every value the local can
hold is a CAUSE_* constant.

The classifier asks PROTOCOL questions, not type questions, and that is the
whole design. An earlier form enumerated types -- coroutine, async generator,
generator, asyncio.Future -- and was extended once per review round, five rounds
running, each fix producing the next finding. The last one was
concurrent.futures.Future: not awaitable, so it fell through the early return and
a pending or failed one was recorded DONE. A type list is open-ended. Two protocol
questions close the space:

1. Does it report whether it is SETTLED? Anything with done() -- both future
   flavours, Task, and anything written to the same protocol. Unsettled means the
   work is unfinished and nothing here will finish it; settled splits three ways,
   because done() answers "is it settled", not "did it succeed".
2. Can it say what it did? A suspendable cannot -- see round 9 below. *_CLOSED
   covers both a drained suspendable and one closed before it began, so the state is
   refused as an answer and the kind alone decides.

Measured across every shape these rounds chased:

object	carrier	state	verdict
coroutine / generator, never began	suspendable	not consulted	failed: contract violation
coroutine / generator, suspended partway	suspendable	not consulted	failed: contract violation
coroutine / generator, ran to completion	suspendable	not consulted	failed: contract violation (was allowed before round 9)
async generator	suspendable	not consulted	failed: contract violation
bare awaitable	suspendable	not consulted	failed: contract violation
asyncio.Future, unsettled	future	done() False	failed
asyncio.Future, cancelled	future	settled	failed: never completed
asyncio.Future, holds an exception	future	settled	failed: ran and raised
asyncio.Future, holds a value	future	settled	allowed
concurrent.futures.Future, all four	future	as above	same four verdicts
plain value	none	n/a	allowed

One protocol question, two carriers, answered on different grounds. The question
is "is this finished?" A future answers it, by done() and then by how it settled, so
its four verdicts are kept and deliberately not flattened. A suspendable does NOT answer
it -- *_CLOSED conflates two histories -- so it is refused by kind, and its state is
never read. Keeping the future distinctions while collapsing the suspendable ones is the
rule: report what the object can tell you, and refuse what it cannot.

Why this is complete, which after nine review rounds is a fair thing to ask. Not
because cases stopped being found, but because the two carriers rest on different
arguments. Suspendables need no enumeration at all: every state reaches one verdict, so a
future Python adding a state cannot produce an unmapped case and there is no table to go
stale. The future protocol offers pending, cancelled, exception and result, and every one
is mapped. What remains true of the old argument is why an unreadable answer fails
rather than passes: for a durable record an uninterpretable state is precisely where
done must not be asserted -- and that default is pinned by a mutant, after a first
attempt at pinning it survived.

The three frame verdicts are three because they are three different facts. "Never
began" says the runner returned instead of running. "Suspended partway" says it ran
and stopped, and since the SDK never drives what a runner returns, that frame never
resumes -- so done would assert a completion nobody observed. An earlier revision of
this PR allowed suspended frames on the grounds that "the body ran", which was true
and insufficient.

(CLOSED also covers a suspendable closed before it ever began, indistinguishable
from a drained one because the frame is gone either way. Disclosed rather than
papered over: a runner that builds a generator, closes it unstarted and hands it back
is telling us nothing, and this reads it as finished.)

The test that this is a fix rather than another row: it would have caught
concurrent.futures.Future before anyone reported it. That type needed no
enumeration -- it answers question 1, so all four of its states are classified by the
same code path that handles asyncio.Future. A reviewer can check that claim against
the table above.

An async generator is answered ahead of both questions because its state does not
matter: driving one needs a running loop this worker thread does not have, so it is
undriven whatever it reports. (inspect.getasyncgenstate is also 3.12+ while this
module supports 3.10, so that state is not uniformly readable.) An awaitable
answering neither question is undriven by elimination. Anything answering neither and
not awaitable is an ordinary value, discarded by design.

One implementation detail is a trap worth naming, because the obvious form of this
check silently breaks on half the CI matrix. The uniform-looking way to ask "has
this body begun" is the frame attribute (cr_frame / gi_frame / ag_frame) plus an
f_lasti comparison. f_lasti is -1 for an unstarted frame up to 3.10 and an
instruction offset (0) from 3.11, and CI runs both 3.10 and 3.12 -- so a < 0 test
matches NOTHING on 3.12 and every async def runner would be recorded done again,
which is the defect this PR exists to remove. Measured, not assumed: on 3.12.13 an
unstarted coroutine, generator and async generator all report f_lasti == 0. The
inspect.get*state helpers are exact and have been present since 3.2 and 3.5, so
those are used instead, and a test pins the distinction so the trap cannot be
reintroduced by someone simplifying the table.

The async-generator arm is the one place this cannot be state-checked at all:
inspect.getasyncgenstate is 3.12+, so on 3.10 the state is unreadable and a
version-dependent record would be its own defect. It is therefore unconditional, on
the independent ground that driving one needs a running loop this worker thread does
not have.

The cause remains unfixed and is filed as
#7804, with its other half as
#7814. The SDK still never
observes whether work happened; it inspects what came back. The runner surface is
cancelled, discarded and run_id, with no progress channel -- grep -c 'def progress' returns 0. The protocol fix makes the classifier right about what it can
see, which is not the same as observing the fact: a runner that returns a plain value
having done nothing is indistinguishable from one that did everything.

The tests are written as a TABLE of factory to expected verdict rather than a case per
type, because appending a branch per reported shape is the failure mode this design
replaced. Two of the rows assert properties rather than instances: that every object
needing to be driven answers at least one protocol question, and that a legitimate
value is not failed closed -- a classifier carrying only negative tests eventually
rejects something valid, and that error looks like a real failure, so it is harder to
notice than the one it replaced.

The generator case is worth keeping in view, because it is why the protocol
framing matters. An earlier revision allowed any returned generator, arguing the
runner's body
had already run and the generator was incidental. That argument is true for a
generator the runner stepped and false for one it merely created -- creating a
generator runs no body, so a never-started generator is the unawaited-coroutine case
wearing different clothes, which the same function correctly fails. One row, two
opposite truths, and a test pinning only the benign one.

It was never the judgement call it was argued to be, only an unmeasured one:
inspect.getgeneratorstate distinguishes GEN_CREATED from GEN_SUSPENDED and
GEN_CLOSED, so this is the pending-versus-done rule already applied to futures,
asked of a generator. Applying it removes the last hand-argued exception from the
classifier. (GEN_CLOSED does also cover a generator closed before it ever started,
indistinguishable from a drained one and read as ok -- a runner that builds a
generator, closes it unstarted and hands it back is telling us nothing either way.)

Async generators are deliberately NOT state-checked: driving one needs a running
loop this worker thread does not have, so a returned async generator is undriven
whatever state it is in -- and inspect.getasyncgenstate is 3.12+ while this module
supports 3.10, so the state is not uniformly readable the way a sync generator's is.

The three future outcomes get three distinct reasons rather than one, because a
cancelled future and a future holding an exception are different facts and a record
that flattens them tells the next reader less than it knows. _execute's message
also stopped hardcoding "its body did not run to completion", which was true only
for the lazy rows and false for the two new ones.

One ordering detail is load-bearing and invisible once correct. cancelled()
must be asked before exception(): on a cancelled future -- both asyncio's and
concurrent.futures' -- exception() RAISES CancelledError rather than returning
it, and asyncio.CancelledError is a BaseException, so it would pass straight
through _execute's except Exception and escape the error handling of the very
run this function exists to describe. Verified rather than assumed, and pinned by a
mutant that neutralises the cancelled() guard so a cancelled future reaches
exception(). Retrieving the exception also marks it retrieved, which suppresses
the "never retrieved" warning _close_quietly already reasons about for the pending
case -- classification and quiet retirement are answering the same question here.

Worth recording that the reported form was narrower than described:
concurrent.futures.Future is not awaitable at all, so a thread-pool runner never
reached this check. Pinned as its own row so the boundary stays explicit.

_public_view serves the two new fields. D2's justification is a consumer
requirement, and the consumer is a future one -- stating that precisely, because
it matters: Code Review Sage demonstrates the requirement today, writing two
different recovery messages for exactly this distinction, but it does so in its
own _RUNS store and does not use the Job SDK at all. No app registers a JobSDK
runner yet. So the fields have no reader in-tree, and First Principles is right
to say so (see its CONCERNS below).

Given that, the reason to serve them rather than only store them is
consistency: a client is the only party that can act on either fact -- whether
the run may have committed side effects, and whether retrying is possible now --
so a record that carries them behind an API that withholds them is the same
record-honest/API-not split as D4. If the fields are worth persisting they are
worth serving; if they are not worth serving they are not worth persisting. They
stand or fall together, which is why they are not split.

Three pre-existing tests were updated, and here is why. Stating this plainly
because an author who silently edits a drift guard looks like one routing around it:

• test_a_returned_value_is_not_persisted_anywhere asserts the exact serialized
  field set.
• test_error_is_the_only_runner_supplied_field asserts error is the only
  runner-supplied field.

Both failed, both were working as designed -- they exist to stop the build until
somebody justifies a new field, and the second was written in P1 for precisely
this moment. Both new fields are listed as SDK-minted, because reconcile writes
them from closed sets this module owns and no runner or caller can reach either,
so error remains the only runner-supplied field and the one-line sanitize
backstop still has one input.

The third is different in kind and worth reading closely:

• test_a_transient_terminal_write_is_retried failed the SECOND store write and
  called it the terminal one. The second write is the starting -> running
  transition, so the test never exercised a terminal write at all -- and it passed
  only because a failed running transition was being ignored, which is the defect
  fixed below. Its premise was the bug. It now targets the write by the STATUS
  being written rather than by call index, so it tests the retry it is named for
  and cannot silently re-encode the same assumption.

"SDK-minted" is only true if nothing runner-supplied can reach those fields, and
a sentence in a test docstring cannot enforce that. So a ratchet was added:
interrupted_from and interrupt_cause each have exactly ONE assignment site,
from a constant. That turns the claim into a checkable invariant.

Tests

Every test pins fields and behaviour, never wording -- a test on prose is a
test on wording, and the wording is the part expected to change.

• TestCoroutineRunnerIsRefusedAtRegistration -- a coroutine function is
  refused; a callable object with an async def __call__ is refused, with the
  premise pinned (iscoroutinefunction(instance) is False while
  iscoroutinefunction(instance.__call__) is True, which is why the check needs
  both); generator and async-generator runners are refused; the refused kind is
  left unregistered (verified three ways: kinds(), is_cancellable, and
  start() raising UnknownJobKind); an async functools.partial and an aliased
  async def are refused; ordinary callables (plain function, lambda, partial,
  __call__ object) still register; and a sync runner driving its own loop with
  asyncio.run still works, since that is the path the refusal message names.

• TestInterruptionRecordsBothAxes -- the prior status is preserved for each of
  queued/starting/running; the cause distinguishes a lost process from a
  missing runner; all six (status, cause) combinations are separately
  observable, which is the assertion the old single-string form could not
  satisfy; a run that never started is not described as running; the message
  still names the restart for all six; both fields survive a round trip through
  disk (the point, since the writer is a process that has died); a normal run
  leaves both fields empty; and composition tolerates a status or cause this
  build does not know, so a record from a newer gateway cannot stop the pass.

• TestAMalformedRecordIsAbsentAnUnreadableOneIsNot -- the two directions are
  pinned separately. Absent: a non-UTF-8 record, a missing file, a bad path and a
  malformed id all answer None. NOT absent: a real 0o000 record (skipped as
  root, where mode bits do not bite), a patched PermissionError, and an EIO
OSError all propagate, and restoring the mode restores the record -- which is
  the proof that None would have been a lie, since nothing about it ever changed.
  A damaged record still does not stop reconciliation, which is why the scan below
  keeps its own broader guard while this lookup narrows.

• test_the_interruption_fields_reach_the_client (routes) -- the two fields are
  served over HTTP while origin/pid stay withheld.

• TestAnUndrivenResultIsFailedNotDone -- the safety property. A sync wrapper
  returning a coroutine reports failed and its body provably never ran (with the
  premise pinned: _lazy_call_shape(wrapper) is "", so no registration check
  could have caught it); a returned async generator reports failed; a returned
  Future reports failed, which is why the predicate is isawaitable and not
  iscoroutine; an ordinary runner returning data is still done; a returned
  plain generator is still done, pinning the documented boundary; the undriven
  result is closed; and a source ratchet counts the write calls in _execute so
  the new branch cannot become a second write site.

Mutation-verified. Each fix was broken with a syntactically valid mutation
applied one at a time, and re-verified after the rebase onto current main:

Mutation	Result
undriven result reports done instead of failed (the whole point)	KILLED
the undriven result is not closed (warns from the collector later)	KILLED
drop the call leg -- reproduces the hole GPT found	KILLED
drop the generator legs, keep only the coroutine one	KILLED
predicate looks right but tests the wrong protocol	KILLED
guard placed AFTER the table write (kind stays registered)	KILLED
prior status not preserved (the old implicit assumption)	KILLED
cause collapsed to one value	KILLED
message claims a never-started run was running	KILLED
a second composition site is introduced	KILLED
the swallow re-introduced -- read() eats PermissionError again	KILLED
the swallow re-introduced -- killed by my own tests too, not just main's	KILLED
ValueError dropped, so a non-UTF-8 record crashes instead of reading absent	KILLED
the running transition's return is discarded again (the reported defect)	KILLED
futures lose their cancel path, so a pending future is never retired	KILLED
own-origin check dropped -- reproduces the reported defect exactly	KILLED
the proxy outranks the fact -- known tested before own-origin	KILLED
the new cause swallows the restart case for foreign origins	KILLED
the future PROTOCOL narrowed back to awaitables (the round-6 defect)	KILLED
unsettled futures pass as done	KILLED
the cancelled leg dropped, so exception() is reached on a cancelled future	KILLED
settled-badly futures pass as done	KILLED
the unsettled reason drops the overlap disclosure	KILLED
the write-lost message claims a restart after all	KILLED
the consequence axis collapses -- starting told its outcome is unknown	KILLED
inverted -- a running record told its body never ran	KILLED
RUNNING wrongly added to the never-ran set	KILLED
the suspendable carrier dropped entirely	KILLED
the generator kind removed -- round 9's defect exactly	KILLED
the coroutine kind removed	KILLED
the async generator kind removed	KILLED
the verdict claims the body never ran -- Raymond's wording constraint	KILLED
the by-elimination awaitable leg dropped	KILLED
state smuggled back in -- CLOSED allowed again	KILLED
the awaitable leg widened, so an ordinary value is failed closed	KILLED
the reconcile call site drops the terminal guard	KILLED
the re-read inside _persist is neutered	KILLED
the terminal test inverted, so finished runs are the ones overwritten	KILLED
the re-read reads a different run, so the guard never matches	KILLED

39 mutants, each confirmed to still parse (a mutation that does not compile
proves nothing), source restored to a matching sha256 after every run. Three rows
carry the most weight: the first, because reporting done for a run that did
nothing is the defect this whole PR is about; the OSError swallow, because that
widening was in an earlier revision of this PR and is now pinned against by both
this PR's tests and main's; and the discarded _persist return, because that
mutant restores the exact defect review found here.

One mutant SURVIVED on the first run of this round -- removing the cancel path
from _close_quietly broke nothing, because the behaviour was asserted only in a
docstring that was itself wrong about it. That is the harness earning its place: a
fix nothing can detect the absence of is not pinned. It is now covered from both
sides, cancel for a pending future and close for a coroutine.

The class, enumerated

The reviewer found one more instance of the same class on each of three rounds. That
is a fact about the class, not about the rounds, so the answer is an enumeration
rather than another patch. Every assignment to a lifecycle-assertion field
(status, interrupted_from, interrupt_cause, error, finished_at, origin)
in job_sdk.py: 18 sites across 4 functions.

site	writes	what it observes
_persist	error	sanitizes a value already present; asserts nothing
start (3)	status, error, finished_at	the thread-start call raised
_execute	status = RUNNING	now a precondition, and the write is checked
_execute (2)	status, error	_persist returned False
_execute (2)	status, error	the result table's verdict for what was returned
_execute	status = DONE	the runner returned without raising
_execute (2)	status, error	the runner raised
_execute	finished_at	the clock
reconcile	interrupted_from	the record's own prior status
reconcile	interrupt_cause	own origin first, then the registration proxy
reconcile	status = INTERRUPTED	no live worker for a non-terminal record
reconcile (2)	finished_at, error	derived from the two axes at one site

Seventeen of the eighteen now record what they observed.

The eighteenth is a disclosed gap, deliberately left, and here is why.
status = CANCELLED if handle.cancelled.is_set() else DONE infers that the work
stopped early from the fact that a cancel was REQUESTED. Cancellation is
cooperative and polling is optional, so a runner that never reads the flag finishes
all of its work and is still recorded cancelled. Verified end to end against the
real SDK: a runner that ignores the flag completed, and the record read cancelled.

It is left because closing it is a semantics change to shipped behaviour, not a
tightening of this PR's own code:

• The line is P1's, not this PR's -- it is on main today, and this PR's diff
  only re-indents it while wrapping it in the new precondition branch.
• Four pre-existing tests pin the current meaning, three of them at the route
  level, so the HTTP surface's definition of cancelled is part of the contract.
  One of them states in its own comment that its runner does not poll the flag, and
  still asserts the run ends cancelled.
• So P1 defines cancelled as "a cancel was requested and the run then ended",
  which the code implements faithfully. Redefining it as "the work was truncated"
  changes an API meaning and would need its own change with its own review.

Making that judgement inside a PR whose subject is exactly this class, without
saying so, is the failure mode this section exists to prevent. Named here so the
next reader inherits the finding rather than rediscovering it.

Manual verification

N/A -- unit coverage sufficient. All three fixes are backend-only and every
observable was exercised in-process against the real modules, including the two
behaviours that needed execution rather than reading: that a coroutine runner's
body never runs, and that a record whose bytes cannot be decoded reads as absent
while an environmental fault still escapes.

Round 9: a returned suspendable is a contract violation, and this REVERSES an

earlier decision in this same change set

Earlier rounds read a returned suspendable's frame state and ALLOWED CLOSED, on the
reasoning that a closed frame ran to completion. A test pinned that as correct. Both are
now inverted, deliberately, and a reviewer who read those rounds should see the reversal
stated here rather than infer it from a flipped assertion.

The reason is this change set's own thesis turned on itself. CLOSED covers a
suspendable drained to completion AND one closed before it ever began. Allowing the
state recorded DONE for the second case -- a completion nobody observed, which is the
exact class of claim the PR exists to remove. The distinction was then measured rather
than assumed: no public attribute, dir() entry, code object, gi_frame, gi_running
or gi_yieldfrom separates a drained generator from one closed unstarted.

Rejected alternative, and this is the reason the tightening is a judgement rather than
a shrug. The distinction IS observable. gc.get_referents returns ('str', 'str') for
a drained generator and ('code', 'function', 'str', 'str') for one closed unstarted --
stable across five trials, both creation orders, an explicit gc.collect(), and with or
without a closure cell, with same-history controls matching each other. It is refused
anyway: it is an undocumented interpreter internal, generators were restructured in 3.11
with lazily-created frames, and CI runs 3.10 AND 3.12. A classifier that is right on one
interpreter and wrong on another writes false records with confidence, which is worse
than declining to classify. That is the same trap as the f_lasti test rejected in an
earlier round, and it was found by looking for it rather than by review.

So the inference is replaced by a rule: any returned suspendable is a runner-contract
violation and records FAILED, whatever its state. The reason string does not say the
body never ran, because for a drained generator it did -- that wording would swap a
false success for a false explanation, which is the same defect one layer down and the
reason an earlier round's proposed remedy was refused.

What this costs, stated rather than buried: a runner that drains a generator and returns
it did real work and is now recorded FAILED. Three things bound that cost, all measured
rather than argued. No runner is registered anywhere in src/ -- every .register( call
is atexit, selectors, or an MCP/workflow registry -- and no app declares the jobs
permission, so there is no installed base to break. The reach is narrower than it
sounds: itertools.chain, map, filter, zip, enumerate, reversed, list/set/dict
views, range and file objects are not suspendables, so returning any of them is
untouched, and generator FUNCTIONS were already refused at registration. And a false
FAILED does not re-run work: there is no retry-on-failure in this module -- the only
"attempts" are _write_terminal's two write attempts -- so the cost lands as a visible
record for an owner to judge, not as duplicated execution.

The tightening also removes code. The per-kind state tables and their three verdict
constants are gone, and the async-generator special case dissolves with them: it existed
only because inspect.getasyncgenstate is 3.12+ while this module supports 3.10, and no
state is read any more. Completeness is now a stronger claim than a state map -- the
verdict cannot vary by state, so a future Python adding a fourth state cannot produce an
unmapped case.

Round 10: reconciliation could destroy a true terminal record

This is the most serious defect in the change set, and it is different in kind from
the others. Every earlier finding was about a record asserting something nobody
verified. This one deleted a fact that was already on disk and wrote a false one over
it.

reconcile skips terminal records -- but it tests is_terminal against a SNAPSHOT that
iter_runs decoded from disk, and it writes later. A worker that finishes in the gap
has already written its terminal record and left _live, and the live check cannot
close the window because a worker leaves _live only AFTER its terminal write lands. So
the pass wrote interrupted over a true done. The cause it recorded was
terminal_write_lost: it claimed the terminal write had been lost when the write had in
fact succeeded and this pass had destroyed it.

Reproduced by driving the real interleaving, not by reasoning:

before      : running | live: True
snapshot    : running (this is what reconcile decided from)
on disk now : done | live: False
reconcile   : flipped = 1
AFTER       : interrupted | cause: terminal_write_lost

The fix enforces a rule this module already states rather than adding one.
TERMINAL_STATES are documented as never revisited, so a pass that overwrites a terminal
record is not merely producing a wrong value -- it is violating the module's own stated
invariant. The write now goes through _persist(only_if_not_terminal=True), which
re-reads inside the same lock acquisition it writes with. The placement is the fix:
self._lock is not reentrant, so _persist is the only place the re-read and the write
are one atomic step. A second check next to the decision would have narrowed the window
and left it open, and the worker's own terminal write goes through that same lock, so a
record that reads terminal there is finished. Cost is one file read per non-terminal
record on the boot pass, inside a lock already held across a file write.

The regression test asserts full-record equality with what the worker wrote -- not
that reconcile returned early. "Returned early" is a proxy for "did not corrupt", and
substituting a proxy for the fact is the defect this PR exists to remove, so the test
must not commit it either. The ordering is driven rather than mocked: every step of
reconcile runs for real and only the worker's completion is scheduled, using the
generator's own yield as the suspension point. Mutation-verified -- with the guard
removed the test fails on the record diff (interrupted_from: 'running' against ''),
which is the record content, not a return value.

Whole-module prose audit

Because seven of the defects in this change set were prose a code change invalidated,
every claim the module makes was re-checked against the code as it now stands rather
than only the sentences a reviewer happened to flag.

Inventory, extracted mechanically from the AST rather than by eye: 472 behaviour
claims -- 190 sentences across 30 docstrings, plus 282 comment lines that assert what
happens rather than restate a line. 209 carry an absolute ("only", "never", "cannot",
"every") or name another callable, which is where prose goes stale.

13 of those are decidable by grep rather than by argument, and all 13 hold now:
_persist is the only record writer (1 call site); _interrupt_error is the only
message-composition site (3 _INTERRUPT_MESSAGE references: def, lookup, fallback);
_execute has exactly one _persist and one _write_terminal; interrupted_from and
interrupt_cause each have one assignment site; cancelled() precedes exception();
getasyncgenstate is referenced only in prose, never called; no f_lasti comparison
exists; every CAUSE_* has a template; _PROGRESS_PHRASE covers all three
pre-terminal statuses; _WRITE_LOST_BODY_NEVER_RAN is exactly the pre-execution set;
the runner is called with the handle alone; and grep 'def progress' returns 0.

Two claims were still false, and both were falsified by THIS PR rather than
pre-existing. _lazy_call_shape's docstring and the register docstring both said a
lazy-returning runner's run "is recorded done" in the present tense -- true of the
defect, false since the execution-side check records failed. Both now state it as the
prior behaviour and name _undriven_result as what makes the record honest, which also
corrects the standing of the registration guard: it is the friendly early error, not
the safety property. Neither had been reported by any reviewer.

That was the audit's result at the time. It did not hold, and the way it failed is
the more useful finding.

The audit ran before round 9. Round 9's by-kind rule then created a FRESH stale claim in
_execute's docstring -- prose still saying a returned generator is classified by
whether its body has begun, which the ruling had just made false -- and a reviewer found
it, not the audit. So the audit was not a one-time repair. It is a step that any
behavioural change re-opens, and the honest statement is not "we cleaned up the drift"
but nothing checks prose, so drift returns with every change. The mechanically
checkable claims survived because a script checks them; every claim that rotted was one
only a reader could have caught.

That is also why the count matters less than the mechanism. Of the claims found false
across the whole change set, none were pre-existing defects in main -- every one was
prose that a change in THIS PR invalidated, including two created by the PR's own later
rounds. A prose claim has no test, so the only thing standing between it and a false
statement about behaviour is whoever happens to read it next.

Pattern harvest

Rule candidate: review-prompt
Pattern: a durable record asserts a lifecycle fact its writer never observed.

The sharper finding, arrived at after seven instances: every single one was prose
that a code change invalidated, and nothing checks prose. Seven claim-versus-code
defects surfaced across this change set -- a comment arguing the exception tuple
"collapses rather than widening" when it widened; a docstring saying futures expose a
synchronous close when neither flavour has one; a claim that backend.entryPoint
leaves an app with no registration path when the context builder gates only on the
jobs permission; a note calling the returned-generator case ambiguous when
getgeneratorstate decides it; a sentence saying interrupted_from measures progress
"before its process died", falsified by the same-process write loss this PR added a
cause for; plus the two lifecycle records the PR opened with. One of the seven was
created by this PR's own round-5 fix, which is the part that matters: the mechanism
is ongoing, not historical. Code has tests; prose has nothing, so a fix silently
turns its own neighbouring comment into a lie.

That reframes the whole change set. It began as "three places the SDK asserted a fact
it never verified" and the real subject is larger: an assertion is an assertion
whether it lives in a record or in a comment, and only one of the two has a gate.
A repo that wants this class closed needs the prose adjacent to a change treated as
part of the change's blast radius -- the practical form being that editing a
conditional means re-reading the comment above it, because that comment was written
to describe the branch you just replaced.

All three defects are one failure mode, which is what makes them one PR. In each
case the writer substituted a proxy for the fact it recorded, and every proxy
failed silently in the direction of false confidence:

• done came from "the runner returned without raising", not from "the runner
  ran". Calling a coroutine function returns without raising and executes nothing.
• "the gateway restarted while this was running" came from "a runner is
  registered for this kind", not from the run's own prior status -- which the same
  line had just overwritten.
• A 500 came from "an exception escaped", not from the fact actually observed,
  which was "this record cannot be read" -- that is, a 404.

Stated so a reviewer can apply it to unrelated code: when a writer records a
terminal or lifecycle outcome, ask what it observed. If the answer is "no
exception was raised" or "some adjacent state was true", it is asserting rather
than reporting. The tell is a proxy standing in for the fact, and the cost is
always a record that looks normal while being wrong.

One slice of this is mechanically checkable and worth a lint or semgrep rule:
a callback type whose return is Any cannot reject a coroutine function.
JobFn = Callable[["JobHandle"], Any] looks like it constrains the runner and
does not. Verified rather than assumed -- mypy over an async def both assigned
to JobFn and passed to register() reports no issues, because Coroutine is
assignable to Any. So a registry that will never await its callback cannot rely
on the annotation. That travels to every Callable[..., Any] registration point
whose call site discards the return.

The rule caught this PR's own first fix, which is the strongest thing about
it. D1 originally shipped as a registration-time inspection of the callable's
shape -- and a registration-time shape check is exactly an adjacent-state proxy,
the thing the rule names. It failed the way the rule predicts, twice: a reviewer
found that an async def __call__ instance bypasses it, and then measuring the
guard against a matrix of forms surfaced a second bypass nobody had named -- a
plain def run(h): return _do_it(h) over an async def, which defeats every
possible shape predicate. Applying the rule to the fix produced a different
design: observe the fact instead. "Did the runner do the work" is available at
exactly one place, what the call handed back, and checking there makes the class
unreachable rather than the enumerated spellings refused. The shape guard was
kept, demoted to what it is honestly good for -- telling the app author at
register() rather than in a run record later.

That is the generalizable shape of the remedy, not just of the defect: when a
check inspects a proxy, ask where the fact itself is observable, and move the
check there. The proxy check can stay as a fast error; it must not be the
safety property.

What does not generalize, said plainly: D4's specific shape is too local to lint,
since narrowing an exception tuple is usually deliberate. What does travel is the
sibling form: two readers of the same resource disagreeing about what damage
means. Here read and the iter_runs scan twelve lines below now disagree ON
PURPOSE, and the reason is the generalizable part -- a scan that skips one file
still returns the others, so a partial result is honest, while a targeted lookup's
None asserts absence, and absence is not something an unreadable file
establishes.

The rule has now caught this PR's own fixes three times, which is the strongest
evidence for it. Beyond D1's shape-versus-fact story above, review found two
more instances in this change set itself:

• D4's first form widened read to catch OSError, which made get answer 404
  for a transient Windows sharing violation -- read asserting an absence it had
  not observed, about a record that was present and intact. Main's Flaky: test_job_routes.py::test_get_existing_run_is_200 fails on Windows with PermissionError on the run JSON #7703 test
  independently requires that error to propagate.
• _execute set status = RUNNING, called _persist, and discarded the
  return, then ran the body on the assumption the write had succeeded. A lost
  write left the record at starting while the runner committed real work, and
  reconcile later read that record and stated the run had never begun. The
  proxy here is subtler than a lifecycle field: it is assumed success of an
  operation whose result was available and ignored.

That last one sharpens the rule into a second checkable form: a write whose
return value reports failure must be checked before anything acts on its assumed
success. Mechanically greppable at any call site that discards a boolean-returning
persist. The remedy also follows the rule's own logic -- make the transition a
precondition for running the body, so no surviving record can claim a run did not
begin while its side effects exist. The correction that was NOT taken is worth
recording too: mapping stale starting to running would have made one lie
universal to hide another, which is why the field exists in the first place.

And then the rule caught the fix to the fix, which is the part worth generalizing.
Exempting done() futures from the undriven check was itself an unverified
assertion: done() means settled, not successful, so a future holding an exception
and a cancelled future were both recorded DONE. Two consecutive review rounds each
found a defect inside the previous round's fix, in the same function, and the reason
is structural rather than careless -- _undriven_result is the single place where
"did the work happen" is decided, and every appended branch is another chance to
answer it wrongly for a shape nobody enumerated.

The remedy generalizes better than any of the individual fixes: stop discovering
the input shapes by being told about them, and enumerate the contract instead. The
function now carries a table of every result shape with its verdict, and one test per
row, so a shape nobody considered is a visibly missing row rather than a silent
done in a durable record. The tell that a function needs this treatment is a
sequence of appended if branches, each added in response to a specific reported
input -- patch-per-report is a growth pattern, and it converges on the enumerated set
only by accident.

Applied once more to the whole file, which is how the enumeration above exists.
After the third round found a third instance of the same class, the response was to
count the class rather than fix the report: 18 lifecycle-assertion write sites, each
classified by what it observes. That enumeration is what found the cancelled gap --
no reviewer reported it, and it is now disclosed rather than waiting to be found in a
fourth round. The rule generalizes past a single function: when a reviewer finds one
more instance of a class every round, enumerate the class and the rounds stop.

And the sharpest version, because the corrected enumeration still missed a state.
Moving from types to protocols was the right level, and the protocol table STILL had
only two suspendable outcomes -- unstarted and everything else -- so a suspended frame
was recorded as completed work. The abstraction was right; its content was borrowed
from how the problem had been discussed rather than from Python's own definition of
the states. The language defines three, the conversation had two, and the code
inherited the conversation. So: enumerate from the AUTHORITY, not from your own
framing of it. The check is mechanical -- name the source that defines the set
(a language reference, a protocol spec, a schema) and count its members against your
rows. If you cannot name that source, you are enumerating your own understanding, and
it will be missing whatever you had not thought to say out loud.

The step before that one still holds and is worth keeping: enumerate at the right
LEVEL, or the enumeration is just a longer list of instances. Writing down the
result shapes did
not stop the rounds, because the enumeration was of TYPES. Types are open-ended --
each round added a row and the next round found the type that row had missed. The
protocol questions close the space because the language defines them: three
suspendable kinds with a *_CREATED state, one future protocol, and __await__ must
wrap one of those. The tell that an enumeration is at the wrong level is that it
grows on contact with reality: if reviewers keep adding rows, the rows are instances
and there is a question underneath them that would cover the lot. The check is
whether a candidate abstraction would have caught the last thing reported without
being told about it -- here it would have, since concurrent.futures.Future needs no
row.

A related caution, learned the hard way in this PR's own tooling. The mutation
harness anchors on source text, so every one of these rewrites silently invalidated
anchors: four went stale in one round (matching 0 times, therefore proving nothing
while still reporting), then one more, then ten at once in the protocol rewrite; one
mutant was inert because it moved a getattr rather than the call it was supposed to
reorder; a rewritten ratchet failed for the wrong reason because it pinned an
expression's TEXT instead of the invariant; and a test asserting the word "await"
appeared in an error message broke when the message was reworded to name the protocol
instead of the type. A test that pins a literal string stops testing the moment the
string moves, and it fails in the direction that looks like a real defect, which
costs a round to diagnose.

And one more refinement of the enumeration rule, from the round that followed it.
Enumerating the result shapes did not by itself catch the generator problem, because
the enumeration inherited a row that was too coarse -- "generator object, ok" reads
like a decided case, so writing the table down did not expose that two opposite
truths were hiding inside it. An enumeration is only as good as the granularity of
its rows, and the tell for a row that needs splitting is a justification that
argues about the input's HISTORY rather than its type: "the runner already did its
work" is a claim about what happened before the value was returned, and if that claim
can be true or false for the same type, it is two rows. The check that settles it is
whether the distinction is observable -- here inspect.getgeneratorstate made it
observable, so what had been defended as a deliberate boundary was only an unmeasured
one.

A documentation instance of the same class is fixed in this push too: the module
docstring claimed an app declaring backend.entryPoint "has no registration path",
while context.py gates ctx.job on permissions.jobs alone and the gateway hooks
publish it regardless. The scope is per RUNNER, not per app -- such an app does get an
SDK, and what it cannot do is register a runner from its own process. A false claim
about the SDK's own scoping, in the module whose subject is writers asserting what
they never observed.

A second pattern, about process rather than code. Two pre-existing drift guards
(test_a_returned_value_is_not_persisted_anywhere and
test_error_is_the_only_runner_supplied_field) failed on this change and both
were right to: they exist to stop the build until somebody justifies a new field
on the record, and the second was written in P1 for exactly this moment. That is
a guard doing its job, and worth naming as a pattern to copy. The follow-on
lesson is that satisfying such a guard with prose is not enough -- "SDK-minted"
is only true if nothing runner-supplied can reach the field, and a sentence in a
test docstring cannot enforce that. The ratchet added here (each new field has
exactly ONE assignment site, from a constant) is what converts the claim into
something checkable. That guard-plus-ratchet pairing generalizes to any
invariant currently asserted only in a docstring.

A third, and the sharpest of the process patterns: a test can encode the very
defect it appears to guard against. test_a_transient_terminal_write_is_retried
failed the second store write and named it the terminal one. It was the
starting -> running transition, so the test never exercised a terminal write,
and it passed only because that transition's failure was being ignored. Green, and
evidence of nothing. The tell is a test that identifies its target POSITIONALLY
(call index, ordinal) rather than by a property of the thing itself -- when the
sequence changes, the assertion silently moves to a different subject. It now
selects the write by the status being written.

Notes

The cancelling work that landed with #6682 (cancelling_ids, and
_public_view's required cancelling argument) is main's, not this PR's; the
rebase kept it intact and this PR only adds two keys to the same dict.

Related follow-ups from the same audit, none of which this PR claims to fix:
#7588 (no retention, and status-only liveness makes eviction unsafe), #7590 (P2
payload channels, with measured requirements from two consumers), #7591 (what
migrating Code Review Sage would require), plus #7582 and #7583. #7589 asked for
exactly the cancelling mechanism that shipped in #6682.

The AWS Control backup consumer PR depends on this one but is not blocked by
it: that branch carries its own test_the_runner_is_not_a_coroutine_function,
so it does not rely on D1 landing first.

Refs #6682

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound honesty fixes; the future carrier duck-types on done(), so an ordinary return value can be probed, false-failed, and side-effectfully closed.

Watch

• _undriven_result treats any returned object with a callable done() as a future ("Carrier 2: a future... done = getattr(result, "done", None)"). A legitimate runner value that happens to carry done() — a progress tracker, a status object — is invoked, and if it returns falsy the run is recorded FAILED ("a future that is not settled") and _close_quietly then calls its close()/cancel(). Consequence: the documented "return value is DISCARDED" contract becomes "inspected, methods invoked, possibly mutated," and a successful run yields a false failure record — the same false-record class this PR exists to remove, inverted. Fail-closed and visible, so CONCERNS not BLOCK, and no in-tree runner exists yet — but the misclassification lands on the first app author who returns the wrong-shaped object.

Suggestions

• Narrow carrier 2 to isinstance(result, (asyncio.Future, concurrent.futures.Future)) (Tasks subclass the former) and keep the existing isawaitable fallback: every row in the PR's own verdict table still classifies identically, unknown third-party futures fall to the fail-closed awaitable leg, and no plain value is ever probed or closed.

[DESIGN-REVIEWED] 7a0f385

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

Both candidates fail Step-1 falsification when I try to ground them to a concrete condition that occurs in practice.

CANDIDATE 1 (re-read in _persist(only_if_not_terminal=True) aborting reconcile): I traced the path. reconcile runs off-loop (await asyncio.to_thread(reconcile_all) in hooks_integration.py:597), so read_bytes_with_retry can spend its retry budget — the common Windows sharing-violation transient is outlasted rather than raised. A statically-unreadable record never reaches the re-read: iter_runs (which catches OSError and skips) would have dropped it first, and for malformed bytes/ids read() returns None rather than raising, so the loop proceeds. The record being re-read is the exact one iter_runs decoded microseconds earlier, so the deterministic path does not raise. What remains is a persistent host fault (EIO/EMFILE) or a Windows hold exceeding the 0.45 s budget landing in the precise re-read window — a race/environmental tail, not an input that occurs in practice. (a) resolves to "might," and the candidate itself is unreproduced. Below the bar.

CANDIDATE 2 (duck-typed done() on a returned object): grounding (a) requires a runner returning a domain object exposing a callable done() meaning something other than a future. The SDK discards return values by design, and the candidate admits no such runner exists in this codebase. That is "if a caller were to," not a condition in practice. Below the bar.

No Step-2 finding grounds to 80+: the future branch asks cancelled() before exception() (so CancelledError cannot escape except Exception), _close_quietly correctly reaches cancel for futures and close for suspendables, and the undriven-result and running-transition paths write only through the single guarded terminal-write site.

No findings.

[OPUS-REVIEWED] 7a0f385

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

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

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 7a0f385f13808f5f4c9293e530358afa2ca517cc and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/apps/job_sdk.py:305 -- "inspect.isgenerator" records completed runners returning a drained generator as failed, contradicting the stated scope that plain generator returns remain allowed -> Fix: remove the plain-generator predicate.
[GPT-REVIEWED] 7a0f385

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

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 7a0f385f13808f5f4c9293e530358afa2ca517cc — 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 run. Composing the review.

First-Principles-Verdict: CONCERNS

Every fix targets a verified cause, but the two new API fields ship with zero counted consumers and a justifying consumer the diff itself disproves.

What this change ships

Intent: make the Job SDK's durable run records state only facts it verified — a FIX.

1. Registering an async/generator-shaped runner fails fast with a clear error — justified
2. A runner handing back undriven or unfinished work records failed, never done — justified
3. A run whose "running" write fails never starts its body, reads failed — justified
4. Interrupted records say how far the run actually got, not always "was running" — justified
5. Runs minted by a live gateway no longer blame a restart — justified
6. interrupted_from/interrupt_cause persisted and served on the public run API — zero consumers
7. Looking up a corrupt run record answers "not found" instead of crashing — justified
8. Reconcile can no longer overwrite a run that finished mid-pass — justified
9. New spec section on Job SDK process scope — rides along, documents pre-existing behavior

Watch

• The description's consumer for item 6 — "Code Review Sage writes two different recovery messages for exactly that distinction" — is not a consumer: Sage keeps its own in-process registry and composes those messages itself (code_review_sage/backend/routes.py:215-221), declares no jobs permission (grepped "jobs" in apps/**/app.json: 0 matches), and per this PR's own new spec section a backend-process app has no registration path. Grep for interrupted_from|interrupt_cause across src/ and website/: 0 consumers beyond the defining SDK, the serving route, tests, and docs. No shipped app registers any Job SDK runner today.
• Fix 3's root cause (a discarded _persist return) has one unfixed sibling at job_sdk.py:1066 — mitigated by the JobError raised beside it, so not silent.

Subtractions

• Defer the two persisted/served fields: _interrupt_error can take the cause and prior status as locals inside reconcile, so the honest message — the reported defect — needs neither JobRun.interrupted_from/interrupt_cause nor the two _public_view keys (0 consumers counted). Add the fields when the first permissions.jobs consumer lands.

[FIRST-PRINCIPLES-REVIEWED] 7a0f385

[iamwhatever]

Tier 1 auto-approve: fix (5 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: five silent Job SDK defects where the run record or the API asserted a fact the SDK never verified -- register() now refuses coroutine/async-generator/generator runners (whose body never executes, yet the record said done) with an execution-side check as the real safety property, reconcile() keeps interrupted_from/interrupt_cause instead of flattening three distinct events into one status, JobStore.read treats an undecodable record as absent rather than 500ing the lookup, _execute no longer discards the RUNNING _persist result, and the interruption cause is no longer inferred from runner registration; job_routes serves the two interruption fields a client needs to recover. Spec files changed as a ride-along (a minority of the diff on both file count and changed lines), not reviewed as a design decision: docs/system-specs/features/app-sdk-durable-jobs-and-view-state.md.

[bolichen97]

Description / code mismatch

One of the five change sections the Description presents as a code fix — D4 — makes no functional change to JobStore.read: the exception leg it claims to add is already in the base.

1. D4 is described as a code fix, but read's exception tuple is unchanged from main

The Description says —

D4 -- read treats an unreadable record as absent. It caught
FileNotFoundError and NotADirectoryError, so a record whose bytes are not the
UTF-8 JSON this store writes crashed the lookup instead of reading as absent. The
fix adds ValueError (which covers the UnicodeDecodeError from the explicit
decode, and the malformed id _path rejects) and nothing else.

Problem / Motivation item 3 states the same premise:

JobStore.read crashed on a record whose bytes it could not decode, so a damaged record took down the lookup instead of reading as absent.

And "Why it matters" presents it as a live, user-visible defect:

A 500 where a 404 is the honest answer is unactionable for a client, and the two readers of the same file disagreed about identical damage: iter_runs twelve lines below already treated an unreadable record as merely skippable.

The code does — read catches (FileNotFoundError, NotADirectoryError, ValueError) at src/kiro_crew/apps/job_sdk.py:717, and that tuple is unchanged from the merge base: the ValueError leg has been in read since the Job SDK landed in #6682. An undecodable record therefore already read as absent on main, and read already agreed with the iter_runs scan about what damage means — the two readers were never in disagreement about it. The diff's contribution to D4 is the explanatory comment plus the TestAMalformedRecordIsAbsentAnUnreadableOneIsNot tests. The OSError widening the Description discusses is churn internal to this branch; withdrawing it lands back on exactly the base state, so no observable behaviour changes here.

Risk — A maintainer reading this PR concludes that a 500 where a 404 was the honest answer reached users and is fixed by this change. It did not exist. That misdirects review attention: the added test class and its two mutation rows characterise pre-existing behaviour rather than pinning anything this PR introduces, so a reviewer budgeting scrutiny by the Description's claimed scope spends it on the one section with no functional delta and less on the four that do change behaviour. The Pattern-harvest generalization inherits the error — it says the two readers "now disagree ON PURPOSE", making a pre-existing agreement read as a deliberate outcome of this change.

Required change — Correct the Description to state that read's exception tuple is unchanged from main, and that this PR's D4 contribution is documentation plus regression tests that pin the existing behaviour against the OSError widening an earlier revision attempted. Specifically:

• Drop or restate Problem / Motivation item 3, which asserts read crashed on an undecodable record.
• Drop or restate the "Why it matters" 500-vs-404 bullet, which presents the 500 and the reader disagreement as live defects.
• Correct the Pattern-harvest sibling-form paragraph, which says read and the iter_runs scan "now disagree ON PURPOSE" when they already disagreed identically on main. The later Pattern-harvest bullet about the OSError widening is accurate as written — it correctly attributes the 404 defect to this PR's own earlier revision — and needs no change.

Alternatively, drop D4 from the PR's claimed scope. Note also that the title says "three places" while the body enumerates five, so whichever framing is chosen should make the title and the enumeration agree.