fix(aios_connectors/resolver): treat cross-account session as DETACHED, not SESSION_MISSING, after reparent

[detail-app]

Detail bug report: View on Detail

Summary

After reparent_connection moves a connection across accounts, the carry-over CTE rewrites child rows' account_id to the destination but leaves their session_id/target_id pointing at source-account sessions (sessions are account-scoped and are not reparented). The resolver's _session_is_archived conflated a 0-row (cross-account) session lookup with "live", so it returned the stale cross-account id with drop=None; handle_inbound then hit append_event's account-scoped seq allocation (0 rows) → NotFoundError → InboundDrop.SESSION_MISSING → HTTP 404. Since the connector-http runner treats 404 as non-fatal, every previously-routed chat on the reparented connection silently dropped under a generic session_missing reason that did not point at reparent as the cause.

The reparent docstring's risk model only considered children whose account_id is left on the source (the case it fixes); it had a blind spot for the opposite — children whose account_id is moved to the destination while their session_id stays on source — which is exactly what the CTE produces.

Fix: _session_is_archived now treats a 0-row (cross-account) session the same as an archived one (returns True), so the post-reparent inbound surfaces as ResolveDrop.DETACHED → InboundDrop.DETACHED (HTTP 422) — the terminal signal the reparent docstring already says is the expected outcome when a child points at the source account, and the read-time tenancy re-check the carry-over tests credit the resolver with performing.

This is a diagnostic improvement: under the aios-connector-http runner it changes the dropped reason from session_missing to detached but not whether the message is dropped (both 404 and 422 are non-fatal there). The operator-facing gain is that the per-message connector.inbound.refused log now carries drop_reason=detached, which points at reparent rather than a generic "session not found". It does not restore delivery to pre-reparent chats — that requires a session-reparent primitive, explicitly out of scope.

Substrate state changes

None — code-only change.

Test plan

New coverage (red-then-green verified):

• tests/integration/test_resolver_reparent_cross_account.py — pins tier-1 ledger, tier-3 single_session, and handle_inbound returning DETACHED (not SESSION_MISSING) for cross-account session ids post-reparent, plus a live same-account session control that still routes (guards against over-eager DETACH).
• tests/e2e/test_reparent_cross_account_inbound.py — drives the full multipart POST to /v1/connectors/runtime/inbound over a live uvicorn socket, asserting the HTTP response is 422 with drop_reason=detached after reparent (was 404 pre-fix), and that a live same-account inbound still returns 201.
• Corrected the false "the resolver re-checks tenancy at read time" comment in test_reparent_unique_index.py to cross-reference the test that now actually verifies that re-check.

No regressions: existing resolver archived-session tests (the archived-session DETACHED path is unchanged by this fix), the reparent carry-over suite, the per_chat spawn path, the full unit suite (5898 passed), and the full integration suite (1138 passed) all green. mypy, ruff, and the pooled-await lint are clean.

Live-traffic verification run: Confirmed the end-to-end POST over a real uvicorn socket is red before the fix (HTTP 404, error_type: not_found — exactly the reported surface) and green after (422, drop_reason=detached).

Could not fully verify the "per_chat new chat delivers post-reparent" scenario: I attempted it and found a related, distinct cross-account defect — reparent_connection's carry-over does not rewrite bindings.session_template_id, and session_templates are account-scoped and not reparented, so a reparented per_chat connection whose template stayed on the source account raises NotFoundError: session template ... not found (a 500) on the next new-chat inbound via get_session_template. This is the same defect class as the session_id bug but for session_template_id, falls outside the recommended fix (which targets only _session_is_archived), and is worth filing separately. The intended guarantee — that this fix does not regress the per_chat spawn path — was verified via the existing per_chat spawn integration test (the fix doesn't touch _spawn_per_chat_session).

Risk / rollback

Low — the change only widens _session_is_archived to return True for one additional case (0-row lookup) that previously returned False; the live-session path is unchanged (the or short-circuits on row["archived_at"] is not None). Revert the single resolver change to roll back.

---

Automatic Fixes PRs can be configured here.

[eumemic]

Held — and this one is NOT held for the reason the label suggests.

Sweeping the ten PRs carrying needs:human/merge-approval, this one is different from the other nine: it has no adversarial review at all.

• issues/2378/comments — 0 comments containing a review verdict
• pulls/2378/reviews — 0 formal reviews

The other nine each carry a posted verdict (seven pass, two fail). This PR carries the label that means "a machine reviewed it and now a human decides" — but the first half of that never happened. The label was doing the work of a verdict.

I found it only because I checked each PR's review comment individually instead of trusting the shared label. A batch labelled uniformly is not uniform, and the gate label is the thing that made them look alike.

Disposition

Held, with the reason corrected: this needs an uncorrelated review before it needs a merge decision. It should not inherit the "just needs a human to say go" framing from its neighbours — there is no green to approve.

Also true of all ten, including this one: the branch is behind master, so it needs a rebase and a fresh CI green before merge is even mechanically possible. A green earned against an older master is a stale green.

No fix-round dispatch: the fixround driver is deliberately disabled under the aios#2396 spend freeze. When the freeze lifts, this PR's first step is review, not fix and not merge.

[eumemic]

Relabelled: needs:human/merge-approval → needs:review.

Re-verified today: this PR has no adversarial review of any kind.

• issues/2378/comments — 1 comment, 0 containing a review verdict
• pulls/2378/reviews — 0 formal reviews

CI is green at head 47a1039 (9 contexts, 0 failing, 0 pending), and mergeable_state is behind.

Green CI on a never-reviewed PR is not a merge-ready PR. CI proves the tests that exist pass; it says nothing about whether the change is correct, whether its tests actually discriminate, or whether it introduces the defect it set out to fix. That judgment is what the uncorrelated review exists to supply, and here it has simply never happened.

Asking a human to approve a merge on a diff no reviewer has examined would make the approval latency without detection — the exact thing the chairman retired his own merge gate to eliminate. The label promised a decision that could not responsibly be made.

Correct next step is a review, then (if it passes) merge under the standing rule. Review dispatch is paused under the aios#2396 spend freeze, so this waits — deliberately, not stranded.

[eumemic]

Uncorrelated adversarial review: PASS as scoped — with two caveats that must not be lost at merge.

This PR had no adversarial review of any kind (0 review comments, 0 formal reviews) for 8 days. Dispatched one rather than merging it unreviewed or holding it indefinitely. The reviewer had no hand in authoring it and returned its verdict privately; findings are reproduced here in full.

The defect is real — reproduced by execution, not reading

No Docker in the reviewer's sandbox, so it ran a local Postgres 15 and pointed the suite at it, bypassing the testcontainer fixture. Reverting only the resolver hunk at head 47a10392:

pre-fix : HTTP 404 {"type":"not_found", ... "drop_reason":"session_missing"}   <- the exact reported surface
post-fix: HTTP 422 drop_reason=detached

Observed over a live uvicorn socket, not asserted against an internal enum. The stale-pointer premise is confirmed structurally too: the reparent CTE rewrites account_id on four tables and rewrites no session_id / target_id / session_template_id anywhere.

Mutation evidence — three mutants, all killed, kills tier-resolved

mutant	kills	proves
_session_is_archived → return True	ONLY the live same-account test	the positive control is load-bearing
strip AND account_id = $2 → AND $2 = $2	all 3 cross-account tests, control stays green	tests bind to the tenancy predicate, not merely "some guard ran"
remove guard from tier-3 branch only	exactly the tier-3 test	per-tier coverage, not one test carrying the file

Red→green baseline: reverting the hunk fails 3 of 4 new integration tests + 1 of 2 new e2e; restoring returns 4/4 and 2/2. Regression sweep: 94 passed, 13 skipped (all Docker-gated), 0 failed.

The positive control genuinely discriminates: test_live_same_account_session_still_routes passes both pre- and post-fix, so it is a true invariant rather than an assertion co-moving with the change — and the over-refusing mutant is precisely what kills it. The guard has been observed to admit as well as refuse.

Caveat 1 — the reported harm is NOT closed

DETACHED→422 and SESSION_MISSING→404 take the identical non-fatal path: _is_fatal_inbound_status returns True only for 401/403. Both end in a log line and a default no-op on_inbound_refused. Every consumer of drop_reason was grepped; nothing branches on it. Messages still vanish silently. Classification improves; delivery does not. The PR body states this honestly and that claim verifies.

Caveat 2 — a same-class hole remains, and it poisons the ledger

Tier-2 routing_rules with target_type='session' still fails open cross-account, and the resolver persists the stale cross-account id into chat_sessions. Filed as aios#2419 with the reproduction.

On my own framing

I briefed the reviewer with a framing_check requirement, and it corrected me: my framing was incomplete. I wrote that the CTE leaves session_id/target_id stale without noticing that target_id is the tier-2 column the fix never touches. The reviewer planted a routing rule, reparented, and showed the fail-open persisting at head. That is the correction earning its keep.

It also self-corrected an intermediate belief of its own: it suspected CI's green contexts never executed the new e2e file because of the needs_docker gate, verified that rather than shipping it, and found it false — pytestmark = pytest.mark.docker and the e2e (docker) job runs -m "docker and not perf". CI did run it.

Disposition

Mergeable as a diagnostic improvement. Relabelling needs:review → reviewed. Do not close the parent bug on this PR — aios#2419 carries the remaining holes.

[eumemic-bot]

Code review

Verdict: PASS — scoped correctly, and the central invariant holds on inspection.

Head verified: git -C /mnt/review rev-parse HEAD → b5b58a7b5cb8adc0291f6c1050c2be13b0d7c524, matching the requested SHA. This head is a merge of master (479e2598) into the fix commit 47a10392.

Scope of re-verification

I confirmed the substantive diff is unchanged from the head the prior eumemic-bot review examined, so its red→green baseline and mutation evidence carry over rather than being re-run:

• src/aios_connectors/resolver.py — byte-identical between 47a10392 and b5b58a7b
• all three test files — byte-identical
• the merge brings in only master commits; of the files the new tests depend on, tests/helpers/connections.py, tests/e2e/conftest.py, src/aios/services/connections.py, and src/aios/api/routers/connectors.py are untouched. tests/integration/conftest.py gained only an optional output_style kwarg on seed_agent_env_session (callers unaffected); src/aios/services/inbound.py changed only _RESERVED_METADATA_KEYS.

CI at this head: 9/9 green, including integration and e2e (docker). The docker shard runs -m "docker and not perf" and the new e2e module sets pytestmark = pytest.mark.docker, so the new e2e file did execute.

The load-bearing property, checked directly

The change is only defensible if the resolver's refusal predicate is the exact complement of append_event's acceptance predicate — otherwise widening it trades a mislabelled drop for a newly-introduced drop of traffic that previously delivered. It is:

• append_event (src/aios/db/queries/events.py:1700) allocates a seq under WHERE id = $1 AND account_id = $2 AND archived_at IS NULL; a zero-row match raises NotFoundError → SESSION_MISSING.
• _session_is_archived (src/aios_connectors/resolver.py:87-91) selects on WHERE id = $1 AND account_id = $2 and refuses on row is None or row["archived_at"] is not None.

Refuse ⟺ ¬(row exists for this account ∧ not archived) ⟺ ¬(append would succeed). Exact complement, same account_id value threaded from handle_inbound through both. So no inbound that previously delivered can now DETACH, and the classification is moved strictly earlier in the pipeline rather than changing which messages survive. That matches the PR's own risk statement and is what makes test_live_same_account_session_still_routes a real invariant rather than a tautology.

The 0-row case genuinely arises: the reparent CTE (src/aios/db/queries/connections.py:799+) rewrites account_id on connections, bindings, chat_sessions, inbound_grants, and routing_rules, and rewrites no session_id / target_id / session_template_id anywhere.

Claims in the PR body I spot-checked

• connector.inbound.refused carrying drop_reason — confirmed at packages/aios-connector-http/aios_connector_http/runner.py:604-610, fed by _parse_drop_reason off the error envelope.
• "does not restore delivery" — confirmed. _is_fatal_inbound_status (runner.py:134) is status_code in (401, 403), so 404 and 422 take the identical non-fatal path. Nothing in the tree branches on drop_reason beyond logging and the default no-op hook. This is honestly disclosed in the PR body; flagging it so it is not lost at merge.
• DETACHED → 422 / SESSION_MISSING → 404 mapping — confirmed at src/aios/api/routers/connectors.py:119-133.

Non-blocking observations (no fix required for this PR)

1. Tier-2 remains uncovered, as scoped. _dispatch_routing_target assigns target_session_id = target_id for target_type == "session" with no session-tenancy guard; the subsequent FOR UPDATE re-check validates the connection row, not the session. It then stamps chat_sessions with the cross-account id. So the first post-reparent inbound through a routing rule still surfaces as session_missing. This pre-exists the PR, is outside its stated scope, and is already filed as aios#2419. Worth noting that the fix does improve the second such inbound, since the poisoned ledger row is then caught at tier-1.
2. Helper name now under-describes its contract. _session_is_archived returns True for "not routable", and the two call-site comments still read "whether the bound session was archived". The docstring is explicit about the widened meaning, so this is a readability nit rather than a correctness risk — but a rename (e.g. _session_is_unroutable) would remove the standing invitation to re-conflate the two cases.
3. Test correction is accurate. The amended comment in test_reparent_unique_index.py:397-404 replaces a claim the suite did not actually verify with a cross-reference to the test that now does.

What I could not evaluate

The sandbox has no Docker and no installed project environment (no asyncpg, no venv), so I did not execute the suite locally. This is not treated as green-by-assumption: the affected tests are integration- and docker-marked, both shards completed success at this exact SHA, and the substantive diff is byte-identical to the head against which the prior review performed its red→green and three-mutant kill analysis. No consequential part of the diff was left undetermined.

Blocking issues: none.