fix(interactions): reclassify checkpoint rows missing the run partition

[AlexLiu190625]

resolve_interaction_anchor decided a checkpoint row was corrupt whenever it
failed any of six self-consistency conditions. One of those six is the
run-partition match, and a row can fail only that one because its run field is
absent entirely rather than holding the wrong value. That shape is a row whose
checkpoint predates the run-partition field -- not damaged data -- and reporting
it as corruption registers an ops degradation signal for a database state that
is expected.

Reclassify a row that is missing the run partition, rather than calling it corrupt.

The corrupt branch now asks a narrower question first: did the row fail only
the partition-match condition, and is that because the run field is absent? A
shared helper, is_missing_run_partition_only (services/trace_event_staging.py),
answers it from the set of failed conditions failed_checkpoint_row_conditions
(same module) returns.

A row matching that shape is reported as absence. It reuses
COUNTER_ANCHOR_ABSENT_LEGACY_CHECKPOINT_TYPE when the row's checkpoint_type
is a legacy one -- that counter already describes exactly this kind of row,
reached by a different path through the data -- and increments the new
COUNTER_ANCHOR_ABSENT_MISSING_RUN_PARTITION otherwise.

What changes for existing data.

Until now this reclassification had no runtime effect: resolve_interaction_anchor
has no production callers. Bringing _load_pk_anchored_checkpoint
(api/trace_handlers.py) to the same classification does have one, because that
function is on the live checkpoint read path.

The rows affected are the ones the 2026-08-04 migration created when it added
tasks.last_checkpoint_trace_event_id: it backfills the pointer by matching a
task's legacy event-id column against an existing trace_events row, and that
row can predate the run-partition field, in which case it carries none.

For such a task today, the by-primary-key read raises CheckpointCorruptError.
The A2A surface turns that into a 400 ("The task's saved progress is
unreadable."); the /v1 reply surface turns it into a 409
INTERACTION_NOT_RESUMABLE. The task is permanently unresumable through either.

After this change the read defers to the legacy scan instead. That scan filters
candidates by run partition (checkpoint_run_partition_filter,
services/trace_event_staging.py), a row carrying no run-partition field is not
among them, and the read returns "no checkpoint" -- the task resumes without prior
progress rather than failing.

That is the verdict these rows had before the pointer column existed. The scan
was the whole read path then, and it excluded them for exactly the same reason it
excludes them now. The backfill is what turned "no checkpoint" into "corrupt
checkpoint" for this shape, without anyone deciding that it should. This change
restores the earlier verdict rather than inventing a new one.

The narrower reading is deliberate: the deferral applies only when the
run-partition match is the only failed condition and the field is absent rather
than wrong. A row that is also wrong in any other way, or that carries a
genuinely mismatched partition, is still reported corrupt. Both limits have their
own test.

One definition instead of three hand-copied ones.

The row-validity judgment used to exist as two hand-copied six-condition
disjunctions kept aligned by a static AST comparison, and reclassifying this row
shape would have added a third copy of five of those conditions to each side.
Both resolvers now read one function, failed_checkpoint_row_conditions
(services/trace_event_staging.py), which returns the set of failed condition
names so a caller can ask which condition failed rather than only whether any
did. That is what makes the "only the run partition, and only when absent"
question expressible without a second copy of the conditions.

It lives beside checkpoint_run_partition_filter, which an earlier change had
already moved into that same module from trace_handlers.py for the same
reason: the services layer must not import from the api layer, so a
services-layer consumer needs the predicate to live below both. That move is
not part of this PR -- the function is already in trace_event_staging.py at
this branch's merge base, and trace_handlers.py's delegating staticmethod is
unchanged here.

Removing the copies also closes a divergence the AST comparison could not see: it
pinned the six operands, both of which read not partition_matches, while the two
sides computed partition_matches differently -- one treated a null run_id as a
partition matched by a null run field, the other did not. That difference was
unreachable (the anchor resolver returns earlier when the task has no run id), but
nothing was checking it.

_resolve_read_direction_anchor (services/task_interaction_service.py) is not
converted. Its judgment carries a seventh condition the shared predicate has no
input for (the trace row's event_id against the interaction row's
resume_event_id) and compares the partition against a non-null
resume_run_partition rather than a task's possibly-null run_id, so it is not a
drop-in consumer; and the reclassified row shape is unreachable from it, since the
write side never produces an anchor for such a row. Its docstring is updated to
name the predicate and say why it does not read it.

Counters for the two remaining uncounted outcomes.

COUNTER_ANCHOR_ABSENT_NO_CHECKPOINT_POINTER covers the no-pointer return, and
COUNTER_ANCHOR_RESOLVED covers the resolved one. Together with the other four,
"how many resolutions produced an anchor" can now be read beside "how many ended
in absence or unavailability" -- which is what tells an operator whether "no row
was written after ignition" happened because the gate blocked publication or
because anchor resolution produced nothing. This is not a call total: the
true-corrupt path increments no counter, so the counters sum to fewer than the
number of calls by exactly the number of corrupt verdicts. That gap is read off
INTERACTION_ANCHOR_CORRUPT (ops_signals.py), not off the counters.

Verification -- uv run pytest, Python 3.12, local SQLite-backed default configuration:

• tests/web/services/test_task_interaction_anchor.py +
  tests/web/services/test_checkpoint_row_conditions.py +
  tests/web/test_agent_checkpoint_stream.py +
  tests/web/services/test_trace_event_staging.py: 138 passed.
• Rollout guard suites unchanged by this branch, run as a regression check --
  test_interaction_rollout_config.py, test_interaction_rollout_gate.py,
  test_interaction_rollout_guards.py, test_interaction_rollout_vocabulary.py:
  73 passed.
• tests/web/api/test_a2a_api.py + tests/web/api/v1/test_task_reply.py +
  tests/web/services/test_task_lease_recovery.py +
  tests/web/services/test_checkpoint_pointer_pairing.py +
  tests/web/services/test_task_interaction_service.py: all passing, run as a
  regression check on the consumers of CheckpointCorruptError and the shared
  predicate's third (non-converted) reader.
• Mutation checks against the two new decision points
  (failed_checkpoint_row_conditions's run-partition condition,
  is_missing_run_partition_only's "only, and only when absent" narrowing):
  each of three targeted mutations flips exactly the cells it should and leaves
  the rest passing.

Timing note on the resolution counters, for the record. The comment this diff removes said the step-2 and step-6 counters belong to whichever change wires resolve_interaction_anchor's first production caller. That change is not this one: the resolver still has zero production callers — this PR's own test asserts exactly that — so the new counter keys cannot appear in the counters snapshot yet. The counters land early so that the wiring change ships against a complete rate surface instead of growing one mid-review; what that wiring needs the rates for is covered in the module comment. If the wiring change does not land, these counters come back out together with their tests — the same treatment their earlier removal applied.

[gemini-code-assist]

Code Review

This pull request introduces a startup check (_check_task_status_pg_enum_drift) to detect drift between the PostgreSQL taskstatus enum and the application's TaskStatus enum. It also refines the interaction anchor resolution logic to reclassify rows that are only missing the run-partition field as absent rather than corrupt, and adds detailed counter instrumentation for all resolution outcomes. The feedback suggests restricting the PostgreSQL enum query with pg_type_is_visible(t.oid) to ensure correct behavior in multi-schema environments.

[AlexLiu190625]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a PostgreSQL enum drift check to prevent startup when the live database's taskstatus enum labels mismatch the Python TaskStatus members. It also refactors resolve_interaction_anchor to reclassify pre-existing rows missing a run-partition field as absent rather than corrupt, adding new counters and comprehensive tests. The reviewer feedback suggests minor code improvements in the drift check, specifically caching the database URL to avoid redundant calls and using SQLAlchemy's connection.scalars() for more idiomatic query execution.

[AlexLiu190625]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a PostgreSQL enum drift check (_check_task_status_pg_enum_drift) at startup to ensure the live taskstatus enum labels match the Python TaskStatus definition. It also refines the task interaction anchor resolution logic to reclassify rows missing only the run-partition field as absent rather than corrupt, and adds comprehensive instrumentation via new counters for all resolution outcomes. Corresponding unit tests have been added to validate the enum drift check across various database configurations and to verify the updated anchor resolution outcomes and counter metrics.

[rogercloud]

Summary

This PR changes how resolve_interaction_anchor classifies checkpoint pointer rows whose run-partition field is entirely absent (as opposed to mismatched): such rows are now treated as pre-existing/legacy "absence," not "corruption," and two previously-uncounted resolution outcomes get their own counters. Separately, it adds a startup-time check that compares the live PostgreSQL taskstatus enum against the Python TaskStatus enum and fails startup on drift. resolve_interaction_anchor itself has zero production callers today, so the reclassification is pre-wiring hardening rather than a behavior change visible to any current caller.

Design-level findings

Anchor reclassification (acceptable with reservations): The classification change is reasonable in isolation, but the module's own docstring (src/xagent/web/services/task_interaction_anchor.py:93-99, untouched by this diff) states that reconciling this classification "must change both sides in one change, not this one alone." This PR changes only resolve_interaction_anchor and leaves trace_handlers._load_pk_anchored_checkpoint (src/xagent/web/api/trace_handlers.py:568-584), which has real production callers, still treating the identical row shape as corrupt. There is no live impact today since resolve_interaction_anchor is unwired, but the two functions now disagree on identical data, contradicting the module's own documented invariant. Either update _load_pk_anchored_checkpoint to match, or amend the docstring to explain why only one side changed and record the divergence as tracked tech debt to resolve before the first production caller is wired.

Enum-drift check placement (wrong direction): _check_task_status_pg_enum_drift lives in interaction_rollout.py, a module scoped (per its own docstring) to rollout-policy decisions, and raises InteractionRolloutConfigError, whose docstring scopes it to invalid rollout configuration — neither fits a general tasks.status schema-integrity check. The repo's one comparable check, validate_builtin_public_mcp_apps (src/xagent/web/builtin_mcp_registry.py:1207), lives in its own domain module, runs from _initialize_database_schema in database.py (i.e., after migrations run), and only warns rather than hard-failing. This PR's check is the first pre-init_db(), hard-fail schema check in the codebase, and departs from that convention without discussion. Relocating it into _initialize_database_schema/database.py, after try_upgrade_db, and taking a bind: Connection the way the MCP check does, would also resolve the ordering hazard noted below.

Line-level findings

Major

• src/xagent/web/services/interaction_rollout.py:471 (blocking) — the new enum-drift check silently gives validate_interaction_rollout_at_startup() real, unguarded DB I/O, breaking its documented "raises only InteractionRolloutConfigError" contract. Details and fix suggestion below in Blocking status.
• src/xagent/web/services/task_interaction_anchor.py:290-319 — the new non-legacy "absence" branch (the exact scenario the PR cites as motivating) has zero test coverage; the only existing run_partition=None test uses a legacy checkpoint type and asserts the other counter, and the AST-pin test never calls resolve_interaction_anchor.

Moderate / Minor

• src/xagent/web/services/interaction_rollout.py:330-342 — the drift error message unconditionally tells operators to "run the pending migration," which is actively wrong when only extra (unexpected) labels are present, since PostgreSQL has no DROP VALUE for enums and no migration can fix that state.
• src/xagent/web/services/interaction_rollout.py:278-279 (docstring) / src/xagent/web/app.py:1220 — the enum-drift check runs before init_db() (migrations) and outside database_startup_lock. Currently latent (no migration in the repo adds a TaskStatus label), but a future migration doing so would crash-loop startup before that migration could run, with no code-level remedy.
• src/xagent/web/services/task_interaction_anchor.py:123-146 (not modified by this diff, so no inline anchor is possible) — the module docstring paragraph still describes the pre-PR classification behavior and frames it as an open decision for a future change, contradicting the header table above it (already updated) and the decision this PR actually makes. Please update or remove it.
• src/xagent/web/services/task_interaction_anchor.py:308 — COUNTER_ANCHOR_ABSENT_LEGACY_CHECKPOINT_TYPE is now incremented from two different call sites for two distinguishable pre-existing scenarios (deliberate, per the module docstring), but the new counter's name (..._MISSING_RUN_PARTITION) may overpromise since it only covers current-type rows.
• Docstring around src/xagent/web/services/task_interaction_anchor.py:44 — the claim that COUNTER_ANCHOR_RESOLVED gives the absence/unavailable counters a "denominator" is imprecise: the true-corrupt path still increments no counter, so summed counters can undercount total calls. The PR's own description discloses this in the next sentence, so it's not misleading in context.
• tests/web/services/test_task_interaction_anchor.py:285-287 (not modified by this diff, so no inline anchor is possible) — stale comment ("only the three absence/unavailable outcomes" register a counter) after this PR added three more counters (six total).
• tests/web/services/test_task_interaction_anchor.py:807-823 — _SIX_OUTCOME_CELLS claims to cover "every one of the six" judgment-table outcomes, but the table now has seven (true-corrupt split into three). Neither new reclassification outcome has a matrix cell.
• tests/web/services/test_interaction_rollout_pg_enum_drift.py:127 — test_pg_enum_labels_match_startup_passes derives both the "live" and "expected" labels from the same TaskStatus class, so it cannot fail on an actual label mismatch; it only proves the query executes.
• src/xagent/web/services/interaction_rollout.py:266 — the docstring says the check is "a no-op on SQLite," but the code no-ops on any non-"postgresql" backend; cosmetic only.

Additional coverage gaps noted but not tied to one line (no fix required, tracked for awareness): tests/web/services/test_interaction_rollout_pg_enum_drift.py is missing a case asserting the "unexpected labels" portion of the message is populated for the real type, a combined missing+extra case, a non-postgres/non-sqlite dialect case, and coverage for the legacy postgres:// URL scheme (a confirmed but currently unreachable false-negative, since no DATABASE_URL in this repo uses that scheme); and no test covers a legacy-type row with a mismatched (not absent) run-partition field, though the code is confirmed to classify that correctly as corrupt today.

Simplification opportunities

• L290: yagni. The five-condition disjunction in resolve_interaction_anchor is a hand-copied mirror of the six-condition disjunction in trace_handlers._load_pk_anchored_checkpoint, kept in sync only by a second AST-pin test. Replace both with a shared predicate (e.g., a function returning failed-condition names) used by both call sites — no circular-import obstruction exists between the two modules. This also lets both AST-pin tests (test_row_validity_conditions_match_trace_handlers_structurally, test_missing_partition_discriminator_mirrors_the_main_disjunction) and the ~30-line justification comment be deleted.
• L807: shrink. _SIX_OUTCOME_CELLS is fully redundant with the existing individual per-outcome tests (verified for all six current cells) and its own comment admits this. Safe to remove — but keep the individual tests, since they additionally pin log messages and result fields the matrix doesn't check.

Net effect: worth doing, roughly matrix-and-duplication-sized reduction (a few dozen lines), not a major restructuring.

Blocking status & recommended decision

Blocking: yes
Recommended event: REQUEST_CHANGES

Blocking issue:

• src/xagent/web/services/interaction_rollout.py:471 [new] — validate_interaction_rollout_at_startup() silently gained real DB I/O through _check_task_status_pg_enum_drift(). The create_engine/connection.scalars(text(...)) calls inside that helper are unwrapped; only the "labels differ" branch raises the documented InteractionRolloutConfigError. A DB outage, bad DATABASE_URL, or missing driver now lets raw SQLAlchemy exceptions (OperationalError, NoSuchModuleError, ArgumentError) escape a function whose docstring documents its contract as raising only InteractionRolloutConfigError — breaking that contract for any caller that catches it to produce a friendly startup failure. This runs inside with _policy_lock:, and none of the existing rollout config unit tests isolate DATABASE_URL, so they now silently perform real network I/O in any environment where a real Postgres DATABASE_URL happens to be set. Impact: undocumented exception types can propagate out of a function with a narrower documented contract, and previously-pure unit tests can now do real network I/O. Fix: wrap the DB-I/O portion of _check_task_status_pg_enum_drift and re-raise as InteractionRolloutConfigError (matching the existing pattern for the "labels differ" branch), update the docstring to disclose the I/O, and have affected tests pin/clear DATABASE_URL.

[AlexLiu190625]

Responding to the two review-body-level findings (the design-level note and the simplification suggestion), which don't have their own inline anchors.

The split. This PR's title used to cover two unrelated hardening changes, and your review split neatly along that same seam: the taskstatus enum-drift startup check picked up the one blocking finding, the design-level-B note, and five of the inline comments; the checkpoint-row reclassification picked up the Major finding, the design-level-A note, and the other four. The enum-drift check has been moved out entirely and will come back as its own PR, reworked along the lines you suggested (living beside TaskStatus in models/task.py, called from _initialize_database_schema with bind: Connection, running after migrations and inside the startup lock). This PR now covers only the checkpoint-row reclassification.

Design-level A. Both sides are converged now, on one shared definition rather than a third hand-copied condition set. One correction to the citation, though: task_interaction_anchor.py's "both sides in one change" note (around the module docstring's discussion of legacy checkpoint types) is about a different disagreement -- between this resolver and task_interaction_service._resolve_read_direction_anchor, over whether a legacy checkpoint_type is an acceptable anchor target. It isn't about trace_handlers.py. The underlying point stands, though: this resolver and _load_pk_anchored_checkpoint (trace_handlers.py) would otherwise disagree about the same row, so both are converged in this PR. The third resolver, _resolve_read_direction_anchor, is not converted -- its judgment carries a seventh condition (the trace row's event_id) the shared predicate has no input for, and it compares the partition against a non-null resume_run_partition rather than a task's possibly-null run_id, so it isn't a drop-in consumer; the reclassified row shape is also unreachable from it, since the write side never anchors an active row to it. Its docstring is updated to name the predicate and explain why it stays out.

The simplification suggestion (a shared predicate instead of hand-copied conditions). Adopted, not deferred. failed_checkpoint_row_conditions and is_missing_run_partition_only now live in services/trace_event_staging.py, and both by-primary-key resolvers read them -- see the PR description's "One definition instead of three hand-copied ones" section for the full writeup, including a divergence the old AST-comparison tests couldn't see (the two sides computed partition_matches differently for a null run_id, unreachable today but never actually checked).

Behavior change. Converging trace_handlers.py onto the same classification changes runtime behavior on the live checkpoint read path, not just this resolver's (still zero-caller) one. The PR description's "What changes for existing data" section covers what changes, for which rows, and why the new verdict is a restoration of pre-migration behavior rather than a new one.

The module docstring's paragraph on the two kinds of pre-existing row, and the test comment on which outcomes register a counter -- both rewritten; see the diff. The module docstring no longer describes an open decision that's already been made, and the test comment now says the true-corrupt outcome is the only one that registers no counter (rather than naming a now-stale count of which outcomes do).

[rogercloud]

PR Summary

A 2026-08-04 migration backfilled tasks.last_checkpoint_trace_event_id by matching against pre-existing trace_events rows, some of which predate the run-partition field and therefore carry it as entirely absent (not wrong) rather than corrupt. The existing six-condition row-validity check in the two by-PK checkpoint resolvers treated "field absent" the same as "field wrong" — both corrupt. This PR carves out "the ONLY failed condition is run-partition, AND it's absent (not wrong)" as its own case, reclassifies it as "no checkpoint" instead of corrupt, and unifies the two resolvers' previously hand-copied 6-condition checks into one shared predicate (failed_checkpoint_row_conditions / is_missing_run_partition_only in trace_event_staging.py).

Re-review scope note

This round reviews the current head. The PostgreSQL enum-drift startup check that dominated the prior review round has been entirely moved out of this PR into #1981 (confirmed open, file scope matches) — all 8 enum-drift findings from the prior round are dropped here as moot and tracked there instead. This round also verifies the fixes landed for CF-A, CF-B, CF-D, CF-E and the waiver for CF-C, and evaluates the anchor-reclassification portion that remains in this PR, including two newly confirmed blocking findings.

Design verdict: acceptable-with-reservations for the refactor; wrong-direction for the trace_handlers.py reclassification itself

The shared-predicate refactor is sound engineering: both call sites now import and use the identical functions, closing a real divergence the old AST-pin test could not detect (a null-run_id edge case the two hand copies computed differently, though unreachable in practice today).

The reclassification decision itself is more contested, for the reasons in the findings below. A few design-level points worth the author's consideration (not bugs, no single line owns them):

• The PR description claims tasks move from "permanently unresumable" (400/409 errors) to "resumes with no prior progress." For the /v1 reply and A2A interfaces this is not accurate: both already had an explicit fail-closed policy for unreadable/untagged legacy checkpoints (see a2a.py:520-523's own comment that untagged/unreadable legacy checkpoints are never resumed and the caller must start a new task explicitly). Before/after responses on those two interfaces are equivalent — same status code and error code, and for /v1 a byte-identical V1ApiError object. The stated user-facing benefit only actually materializes on the websocket path covered by Blocking Finding 1 below.
• A more elegant alternative exists and isn't discussed: _load_pk_anchored_checkpoint already treats a different missing field (execution_id) permissively — reading the row anyway, on the reasoning (its own docstring, ~lines 506-522) that "a pointer that names a row is a stronger identity claim than a JSON field match" — while treating the missing run-partition field strictly and discarding the row. If the pointer is already treated as a strong identity claim for one field, it isn't obviously wrong to trust it for the other; that alternative would preserve the checkpoint's progress instead of discarding it, with less permanent special-casing. This asymmetry isn't discussed in the PR description.
• The root cause — the 2026-08-04 migration's backfill SQL lacking a run_id/checkpoint_type filter — isn't fixed by this PR. A one-time corrective migration nulling out the affected pointers would produce the same read-time outcome without leaving two permanent special-case branches in the read paths.

Prior findings checklist

• CF-A (must-change-both-sides invariant): FIXED — both resolve_interaction_anchor (task_interaction_anchor.py:280-296) and _load_pk_anchored_checkpoint (trace_handlers.py:570-592) now call the same shared predicates and agree on the reclassification.
• CF-B (missing test coverage for the new absence branch): PARTIAL — resolve_interaction_anchor's branch is now tested and the tests are real (verified to fail under mutation), but _load_pk_anchored_checkpoint's parallel absence branch (trace_handlers.py:577-592) — the one with actual production callers — has zero test coverage. This folds into Blocking Finding 1 below (same root gap).
• CF-C (counter naming symmetry): WAIVED — confirmed deliberate, documented design choice; not a defect.
• CF-D ("denominator" docstring wording): FIXED — word removed, replaced with accurate non-totality wording.
• CF-E (outcome-counter test matrix): FIXED — renamed to _OUTCOME_CELLS, both new cells added with correct assertions.
• 8 enum-drift findings (PostgreSQL enum-drift startup check): DROPPED / moot for this PR — the check moved to #1981; re-review there.

Findings by severity

BLOCKING — Major — src/xagent/web/api/trace_handlers.py:577-592 (within the PR's diff hunk — also posted inline)
Silent progress loss on the one production path that actually changes behavior. Of execute_resume_background's 4 call sites, 3 are gated by a post_user_message/posted check that intercepts a None/corrupt checkpoint with an explicit error response before this code path matters. The 4th — the websocket resume_task command handler (websocket.py:9025, _handle_resume_task_unserialized) — has no such gate. Before this PR, a row missing only the run-partition field raised CheckpointCorruptError, which is excluded from the isinstance tuple at websocket.py:3843-3847, so it fell to the else branch: the task settled as TaskStatus.FAILED and a terminal task_error was broadcast to the client. After this PR, the read returns None instead; AgentRunner.run/resume (src/xagent/core/agent/runner.py, guard around lines 90-97) skips the corrupt-raise and builds a brand-new empty context with no replay messages — the task silently reruns from a blank history with no re-seeded task string, no log/trace/counter anywhere on this path (the only related signal, trace_handlers.py:585, logs at logger.info while every other "defer to legacy scan" branch in this file logs at logger.warning). Zero test coverage exists for this scenario in tests/core/agent/test_runner.py or any websocket test suite. This is a regression from a loud, client-visible terminal failure to a silent no-op that looks like success, on a real, reachable, supported user action.

• Blocking: yes
• Fix suggestion: add a pre-check/gate to the resume_task websocket handler consistent with the other 3 call sites, or make the absence branch emit logger.warning plus a counter, and add a test covering AgentRunner.run(resume=True, checkpoint=None) reached via this path.

BLOCKING — Major — src/xagent/web/services/task_lease_service.py:370-372 (not part of this PR's diff — no diff line to anchor an inline comment; body only)
A third, independent hand-copied validity judgment exists at _checkpoint_row_matches_candidate, untouched by this PR:

checkpoint_run_id = data.get(TASK_RUN_ID_TRACE_FIELD)
if checkpoint_run_id is None:
    return False

This unconditionally returns False when the run-partition field is absent, regardless of whether that's the row's only failing condition. This propagates through resolve_checkpoint_recovery (task_lease_service.py:449-456) to CheckpointRecoveryVerdict.NOT_RECOVERABLE, and recover_task_lease_candidate_no_commit (task_lease_recovery.py:59, 68-72) maps that to TaskStatus.FAILED with TASK_LEASE_EXPIRED_ERROR — terminal, not retried. This function reads the same Task.last_checkpoint_trace_event_id column as the two patched call sites. Lease recovery runs on a normal operational path (crash/restart/timeout), not a corner case. Net effect: for the exact row shape this PR set out to make "resumable, not corrupt," lease recovery — an unpatched third path — still fails the task outright, partially undermining the PR's own stated goal. Two docstrings now overstate what the code delivers: task_lease_service.py:349-357 claims the two vocabularies ("mismatch" vs. "corrupt") describe the same judgment and aren't worth reconciling — true before this PR, false now for this row shape; task_interaction_anchor.py's module docstring claims classification is "settled the same way on both read paths" but never mentions this third path, undercounting by one.

• Blocking: yes
• Fix suggestion: either extend the reclassification to _checkpoint_row_matches_candidate for consistency, or explicitly scope the docstrings to acknowledge this known third divergence as tracked follow-up work rather than asserting a unification that doesn't fully exist.

Major, non-blocking — src/xagent/web/api/trace_handlers.py:495-497 (this docstring line is outside the changed hunk — body only)
The function's docstring still states that once a target row is found, its identity is authoritative and a validation mismatch raises rather than falling back to search other rows. This is now false: the run-partition-absent-only case (lines 577-592) is exactly an identity-validation mismatch that now falls back to the legacy scan instead of raising.

• Fix suggestion: update the docstring to reflect the new exception.

Minor, non-blocking — documentation staleness cluster (body only — comments, not code lines within this diff)

• src/xagent/web/services/ops_signals.py:55-62 — comment still says INTERACTION_ANCHOR_CORRUPT is set when a row "fails the ownership/type/partition/identity checks," no longer true for the partition-absent case.
• src/xagent/web/services/task_interaction_anchor.py:~20-21 — calls the reclassification helper "a five-condition mirror of this table's six"; is_missing_run_partition_only is a set-equality plus a field-absence test, not five conditions, and will mislead a reader.

Minor, non-blocking — src/xagent/web/services/trace_event_staging.py:347 (within the diff hunk — also posted inline)
is_missing_run_partition_only's absent-vs-wrong test (row_data.get(TASK_RUN_ID_TRACE_FIELD) is None) cannot distinguish an explicitly-stored JSON null from a genuinely absent key — both read as "absent," contradicting the docstring's claim that the field must be "absent entirely" rather than holding a wrong value. Exposure is narrow (a null run_id is rejected upstream before the writer could stamp one), but the guard lives elsewhere, not in this predicate, and no test covers the explicit-null case.

Minor, non-blocking — PR description accuracy (body only, no code defect)

• The "fixed divergence" between the two old hand-copied checks (a null-run_id edge case computed differently) is actually unreachable from resolve_interaction_anchor, which returns earlier when task.run_id is None — unifying was still correct, but it doesn't fix a reachable bug as implied.
• The description says checkpoint_run_partition_filter was "moved" from trace_handlers.py into trace_event_staging.py; verified false — it already lived in trace_event_staging.py in the base commit, and trace_handlers.py has an unchanged delegating staticmethod.

Minor, non-blocking — test coverage gaps (body only)

• No integration test for _load_pk_anchored_checkpoint's absence branch specifically at the trace_handlers.py call site — only unit-level coverage exists at the other call site.
• tests/web/test_agent_checkpoint_stream.py's coverage of the new absence branch (~lines 1571-1630) asserts only result is None, with no log-message pin and no counter assertion, unlike its counterpart at the other call site which pins both. A regression that returns the fallback for the wrong reason would stay green.
• No test asserts that both by-PK call sites actually use the shared predicate (vs. a future re-inlined private copy) — the deleted AST-pin test used to guard exactly this, and the file already has the static-scan machinery to restore a lightweight version cheaply.

Minor, non-blocking — naming/interface (body only)
is_missing_run_partition_only reads as "the run partition is missing, only" but doesn't convey that it also requires the field to be absent rather than merely wrong — a name like is_only_absent_run_partition_failure would state both halves. The function also takes two arguments (failed, row_data) that must be derived from the same row with nothing enforcing that pairing — a caller could pass mismatched values.

Simplification opportunities

L230: yagni CHECKPOINT_ROW_CONDITIONS tuple has exactly one consumer, a test that only checks it stays in sync with the six constants above it. Delete the tuple and its self-registration test; assert the six constants directly where needed.

(File: src/xagent/web/services/trace_event_staging.py. Verified: zero production consumers, deleting loses no real coverage.)

net: ~10 lines possible.

Blocking status & recommended decision

Blocking: yes — Blocking Finding 1 and Blocking Finding 2 independently qualify.

Recommended event: REQUEST_CHANGES

Blocking issues:

• src/xagent/web/api/trace_handlers.py:577-592 — Major — [new] — silently discards prior progress and reruns from a blank context on the websocket resume_task path, with no log/counter/test proving the intended behavior.
• src/xagent/web/services/task_lease_service.py:370-372 — Major — [new] — an unpatched third judgment site still hard-fails the exact row shape this PR set out to make recoverable, undermining the PR's stated goal, and two docstrings now overstate the unification that exists.

[AlexLiu190625]

Responding to the remaining points from this round that don't have an inline thread.

Blocker: lease recovery's third hand-copied judgment. Confirmed -- _checkpoint_row_matches_candidate (task_lease_service.py) was untouched when the two by-primary-key read paths were unified onto failed_checkpoint_row_conditions, so the exact row shape this PR set out to stop calling corrupt still failed lease recovery outright. Both of its paths (the exact-pointer branch in resolve_checkpoint_recovery, and the legacy-scan path through _checkpoint_row_matches_candidate) now read the shared predicate.

Three real semantic differences had to be resolved rather than papered over: (1) execution identity -- the shared predicate's execution_id is passed as str(candidate.task_id), the same value the other two by-primary-key callers pass, since web's execution id is the task id; (2) a candidate with no run_id fails closed before the predicate runs, because the shared predicate treats a null run_id as a legitimate partition (matched by a row whose own run field is also absent) and this module's own rule is the opposite -- an exact pointer alone cannot prove a checkpoint belongs to the expired run without provenance; (3) the str() coercion the old hand-copied check did and the shared predicate doesn't turned out not to matter in practice (the write side only ever stores str | None), and dropping it is a tightening, not a loosening.

The result for the row shape you flagged does not change -- it still ends in FAILED. That's not "fixed equals unchanged": "no checkpoint" already mapped to FAILED here before this round, by design (see _resolve_legacy_checkpoint_recovery's own docstring: "an unset pointer or a zero-row match is an authoritative absence... NOT_RECOVERABLE, not INDETERMINATE"). What the fix adds is that a pointer row missing only the run-partition field now gets a second chance the same way a dangling pointer already does -- it defers to the legacy event_id scan instead of failing on the spot -- and if that scan finds a genuinely valid checkpoint for the same run, recovery now succeeds where it previously could not. Nothing about fail-closed is loosened: the legacy scan re-validates whatever row it finds through the same shared predicate, so a RECOVERABLE verdict still requires a real run-partition match.

Why the import is inside the function, not at the top of the file. trace_event_staging.py already imports task_lease_service for TASK_RUN_ID_TRACE_FIELD and TaskLease. Adding a module-level import in the other direction is a cycle, verified by reproducing it directly (temporarily added the import and ran the module):

ImportError: cannot import name 'TASK_RUN_ID_TRACE_FIELD' from partially
initialized module 'xagent.web.services.task_lease_service'
(most likely due to a circular import)

The function-level import avoids this; it's the same pattern already used elsewhere in this services package for the same reason.

_resolve_read_direction_anchor is still the only path not on the shared predicate. After this round, every reader that judges tasks.last_checkpoint_trace_event_id row validity -- _load_pk_anchored_checkpoint, resolve_interaction_anchor, and lease recovery's _candidate_row_failures -- reads failed_checkpoint_row_conditions. _resolve_read_direction_anchor (task_interaction_service.py) is not converted, for two reasons: it has a seventh condition the shared predicate has no input for (the trace row's event_id must equal the interaction row's resume_event_id), and it compares the partition against a non-null resume_run_partition rather than a possibly-null run_id. The missing-run-partition shape can't reach it either way: resume_run_partition is only ever written by resolve_interaction_anchor, and that resolver returns "no anchor" for a missing-partition row rather than persisting one -- so there's no path where a row lands there missing that field. Left alone this round.

N1/N2/N3 (stale comments the reclassification made inaccurate). All three updated: _load_pk_anchored_checkpoint's docstring (the "a validation mismatch raises" claim now states the one carved-out shape), INTERACTION_ANCHOR_CORRUPT's comment in ops_signals.py (now says a partition-only, absent-field failure increments a counter instead of setting the signal), and task_interaction_anchor.py's module docstring (the "five-condition mirror" description replaced with what the check actually does -- a set-equality plus a field-absence test).

N6 (PR description's "moved" claim). Confirmed false, verified independently against the merge base rather than taking your word for it:

$ git show bbb9d4c4f:src/xagent/web/services/trace_event_staging.py | grep -n "def checkpoint_run_partition_filter"
182:def checkpoint_run_partition_filter(run_id: str | None) -> Any:
$ git show bbb9d4c4f:src/xagent/web/api/trace_handlers.py | grep -n "checkpoint_run_partition_filter"
...
475:    def _checkpoint_run_partition_filter(run_id: str | None) -> Any:
476:        return checkpoint_run_partition_filter(run_id)

The function was already in trace_event_staging.py at the branch's merge base, with the delegating staticmethod already in place. PR description corrected to say an earlier change made that move, not this PR.

N7/N8/N9 (test coverage gaps). N7 (missing call-site-level test for the by-primary-key read reclassification) and N8 (the existing test's weak is None assertion) are both closed by the same change: the test now pins the warning message and the counter, not just the return value. N9 (no test guards against re-inlining a private copy after the AST pins were retired) is closed by a replacement pin that asserts both by-primary-key resolvers still import and call failed_checkpoint_row_conditions, rather than comparing hand-copied text -- deliberately not extended to lease recovery's consumer, since it reads the predicate through a function-level import an import-shaped scan can't see; that path is covered by its own behavior tests instead.

N10, the pairing half. failed and row_data must come from the same row and nothing in the signature enforced that. Not changing the signature (every caller already computes both on adjacent lines from one row, so a parameter change would move code for a constraint nothing violates today) -- the docstring now states the contract explicitly.

---

Two points from this round are not being taken, both discussed here since they don't have their own thread:

On the PR description implying a reachable bug (N5): the disclaimer you're asking for is already in the description, in the same paragraph that makes the claim. Quoting it unchanged:

Removing the copies also closes a divergence the AST comparison could not see: it pinned the six operands, both of which read not partition_matches, while the two sides computed partition_matches differently -- one treated a null run_id as a partition matched by a null run field, the other did not. That difference was unreachable (the anchor resolver returns earlier when the task has no run id), but nothing was checking it.

"That difference was unreachable (the anchor resolver returns earlier when the task has no run id)" states exactly the reachability caveat, with the same reason you give. No description change is being made for this point. If the concern is that the sentence reads as a concession rather than as the headline, say so and it'll get reworded -- the fact isn't misstated.

On the name is_missing_run_partition_only (N10, first half): not renaming. is_only_absent_run_partition_failure moves the ambiguity rather than removing it -- "only absent" reads as "merely absent", where the actual meaning is "the only failed condition is the partition, and the reason is absence". Both halves are already stated in the docstring, and each half has its own test cell (dropping the "only" makes a doubly-wrong row look pre-existing; dropping the "absent" makes a wrong-value row look pre-existing -- distinct failure modes with distinct coverage). A rename would touch the definition, both call sites, and the tests for one word choice against another equally imperfect one.

Fixed in b340a9f (blocker 1), b36f466 (blocker 2 + doc corrections), 49c9cdb (static pin + yagni cleanup + N1-N3), b8b2086 (null-vs-absent wording + pairing contract).