Skip to content

✨ Make a fresh <Session> a pending placement (#648) - #656

Merged
taras merged 4 commits into
mainfrom
agent/issue-648-pending-session
Aug 30, 2026
Merged

✨ Make a fresh <Session> a pending placement (#648)#656
taras merged 4 commits into
mainfrom
agent/issue-648-pending-session

Conversation

@taras

@taras taras commented Aug 30, 2026

Copy link
Copy Markdown
Owner

Closes #648.

Why

A <Session> used to establish. It took ownership, published acp-first, built
a runtime, created a backend session and asserted an identity for it — all
before anything had asked for a conversation.

Two things follow from that. A <Session.Launch> nested inside a <Session>
could not work: the enclosing element had already settled how the session was
constructed, so the launch inside it was refused for naming a session that had
just been created out from under it. And a run could retain a conversation
nothing ever had, because ensureSession() returning is not a backend accepting
a turn — a first turn that failed left a workflow mapped to a session no
provider ever made.

What changes

Before: <Session> established. <Session.Launch> inside one refused. A
workflow row committed as soon as the provider answered.

After: <Session> places. It validates the agent and where the session will
live, pins the exact Session value, and does nothing else — no route, no
runtime, no ensure, no record, no turn. The first consuming operation chooses
the immutable construction route: a first subscribed <Prompt> publishes
acp-first and constructs through ACP, and a first <Session.Launch>
constructs client-native. An established placement keeps its eager validation
exactly as before.

An ACP-first session becomes a conversation when the backend accepts its first
turn, and only the adapter can say when that happened. Both embedded adapters
now report it on one exact versioned key,
executablemd.session-materialization/v1, as {"state":"accepted"} in the
update metadata of a standard ACP session_info_update. Nothing else promotes a
session: not a returning ensure, a first text delta, a synthesized started, a
terminal result, a checkpoint token, an error code or a diagnostic.

How it works

<Session> → placement (pinned, inert)
  ├─ first subscribed <Prompt>   → acp-first → backend acceptance → established
  └─ first <Session.Launch>      → client-native                  → established

The durable order for the ACP-first branch:

route → provisional non-asserting record → backend acceptance
      → asserting record → host mapping → the turn's events

The vendored ACPX runtime carries a third behavioral patch for it. An ensure
asking for materialization: "first-turn-acceptance" persists a provisional
record — the key is occupied, the serialized agentSessionId is absent — and
holds the identity session/new returned privately, so the record is occupancy
rather than an assertion. The acceptance marker is consumed rather than
published: it reaches no event stream, no conversation and no checkpoint token.
On acceptance the runtime promotes a copy of the record, saves it through the
store's own atomic operation, adopts it in memory only after that save returns,
and then resolves AcpRuntimeTurn.materialized. Promotion is serialized against
live-checkpoint writes, so a promotion that failed is not later overwritten by
finalization as though it had succeeded.

Review guide

Start with: specs/acp-client-spec.md §Sessions and §Materialization.

Then review:

  1. architecture.md — the three lifecycle terms, the workflow session ordering
    invariant, and the two construct-inventory rows.
  2. packages/acp/src/provider.tsplacementState, placePending,
    requeried, materialize, and the reordered promptStream.
  3. packages/acp/vendor/acpx/PROVENANCE.md and the four patched generated
    files — the persistence ordering is the sensitive part.
  4. packages/cli/src/workflow-agent.ts, packages/acp/src/session-key.ts,
    packages/test-agent/src/worker/acp-server.ts.

Look carefully at:

  • Promotion ordering in runtime.js (promoteSessionMaterialization) and its
    interaction with finalizeRuntimeTurn, which awaits a promotion still in
    flight before saving.
  • materialize() in provider.ts: on refusal it releases the handle and
    publishes nothing; on a host retention failure it abandons the handle and
    detaches the placement so the next attachment reconciles the one assertion
    that exists.

What must stay true

  • A placement constructs nothing — enforced by resolveSession returning
    before owning() for a pending placement; checked by SM1, SM2, AF25,
    WAP8.
  • Only the adapter's exact marker materializes a session — enforced by
    sessionMaterializationAccepted() in the vendored runtime, which reads one
    versioned key and nothing else; checked by TM4 against a real ACP child that
    emits ordinary updates, agent text and a completed terminal result and never
    the marker.
  • The provider's assertion is durable before the host maps it — enforced by
    saving the promoted record through the store before materialized resolves,
    and by calling sessions.established() only after that; checked by TM2,
    SM4, SM9.
  • A Session value is provenance, not a key — enforced by an object-identity
    test in prepare(); checked by SM2, SM8, AF24.
  • Routes never convert — unchanged; checked by CN6, CN7, CN8, RR1,
    RR2, NL15.

How to verify it

  • packages/acp/tests/acpx-materialization.test.ts (Tier TM) drives the real
    vendored runtime against a real ACP child process. TM4 proves no fallback
    promotes a record; TM2 proves the asserting save precedes the answer; TM6
    proves a pending record is never resumed. It fails if any inference is added.
  • packages/acp/tests/adapter-protocol.test.ts EA6/EA7 spawn the real
    embedded Codex and Claude adapters and prove each emits one session-scoped
    marker per accepted turn, before the prompt response, with interleaved
    sessions not crossing. It fails if either adapter reports session-globally.
  • packages/acp/tests/provider.test.ts Tier SM covers the lifecycle at the
    provider seam, including concurrent first prompts (SM7) and the pre-commit
    window (SM9).
  • packages/test-agent/src/NativeSessionLaunch.test.md authors the composition
    in Markdown: deno task xmd test packages/test-agent/src/NativeSessionLaunch.test.md.

Scope

Included

  • Pending placement, first-consumer route precedence, deferred materialization.
  • The vendored ACPX patch and both rebuilt adapter snapshots.
  • The five contract documents.

Intentionally unchanged

  • Established-session validation, ownership, exact identity, build binding,
    cancellation, teardown, stale-value refusal and the pre-commit recovery window
    all keep their existing meaning.
  • WAL replay and partial-replay behavior.
  • Codex -32600 recovery, Codex App Server persistence, cross-process ownership
    outside native handoff — all out of scope, as Materialize fresh ACP sessions at the first consuming operation #648 states.

New abstractions

  • AcpRuntimeMaterialization and AcpRuntimeTurn.materialized exist because a
    consumer deferring durable state needs one thing to wait on; consumed by
    provider.ts and both test fakes.
  • AcpxSessionPlacement.state exists because the host, not the provider, knows
    whether a workflow session was ever established; produced by
    workflow-agent.ts and directory placement, consumed by resolveSession and
    promptStream.
  • Each new abstraction has multiple concrete uses.
  • No speculative functionality is included.

New dependencies

None. Dependencies and the lockfile are unchanged.

Generated or mechanical changes

  • packages/acp/vendor/adapters/generated/snapshots.ts comes from
    deno run --allow-all scripts/build-adapter-snapshots.ts.
  • Both .tgz files come from each upstream project's own
    npm run build && npm pack at the commits recorded in MANIFEST.json.
  • packages/acp/vendor/acpx/MANIFEST.json digests are recomputed from the
    patched files; PROVENANCE.md describes the patch by hand.

Upstream adapter commits

Both are new commits on the existing upstream pull-request branches, and each
project's full suite is green on the pushed head.

Repository Branch (PR) Head
taras/codex-acp feat/prompt-response-turn-id (agentclientprotocol/codex-acp#438) 377d2b0b
taras/claude-agent-acp feat/prompt-response-assistant-uuid (agentclientprotocol/claude-agent-acp#1047) beba04dc

Risks and limitations

Scope confirmation

  • Every changed file supports the purpose described above.
  • Unrelated cleanup and formatting changes are excluded.
  • Generated or mechanical changes are clearly identified.
  • The description matches the final diff and test results.

A `<Session>` used to establish. It took ownership, published `acp-first`,
built a runtime, created a backend session and asserted an identity for it —
all before anything had asked for a conversation. That is why a
`<Session.Launch>` nested inside a `<Session>` could not work: the enclosing
element had already settled how the session was constructed, so the launch
inside it was refused for naming a session that had just been created out from
under it. It is also why a run could retain a conversation nothing ever had:
`ensureSession()` returning is not a backend accepting a turn.

A fresh `<Session>` now places. It validates the agent and where the session
will live, pins the exact `Session` value, and does nothing else — no route, no
runtime, no ensure, no record, no turn. The first consuming operation chooses
the immutable construction route: a first subscribed `<Prompt>` publishes
`acp-first` and constructs through ACP, and a first `<Session.Launch>`
constructs client-native. An established placement keeps its eager validation
exactly as before.

An ACP-first session becomes a conversation when the backend accepts its first
turn, and only the adapter can say when that happened. Both embedded adapters
now report it on one exact versioned key,
`executablemd.session-materialization/v1`, as `{"state":"accepted"}` in the
update metadata of a standard ACP `session_info_update` — Codex when
`turn/start` returns a turn, Claude when the SDK reports that exact queued
command dispatched. Nothing else promotes a session: not a returning ensure,
a first text delta, a synthesized `started`, a terminal result, a checkpoint
token, an error code or a diagnostic.

The vendored ACPX runtime gains a third behavioral patch to carry it. An ensure
asking for deferred materialization persists a provisional record — the key is
occupied, the serialized `agentSessionId` is absent — and holds the identity
`session/new` returned privately, so the record is occupancy rather than an
assertion. The acceptance marker is consumed rather than published: it reaches
no event stream, no conversation and no checkpoint. On acceptance the runtime
promotes a copy of the record, saves it through the store's own atomic
operation, adopts it in memory only after that save returns, and then resolves
`AcpRuntimeTurn.materialized`. Promotion is serialized against live-checkpoint
writes, so a promotion that failed is not later overwritten by finalization as
though it had succeeded.

The durable order is therefore route, provisional non-asserting record, backend
acceptance, asserting record, host mapping, and only then the events the turn
produced. A first turn nobody accepted leaves the route standing, the record
non-asserting, no workflow row, no identity on the value the document holds, and
that same pending value ready to retry — which creates fresh backend state
rather than resuming the arrangement no backend took. Acceptance, not successful
terminal text, is the boundary: a turn that failed after being accepted leaves
the session established.

The pre-commit window is unchanged and now sits one step later: the provider's
assertion is saved before the host is asked to retain anything, so an
interruption between them leaves exactly one canonical assertion for the next
attachment to reconcile and commit.

Provider provenance is now the exact object rather than the key inside it. A
structural copy, a value from another provider copy or a torn-down scope, and a
value used with a different resolved agent are each refused before any provider
work — and the value a `<Session>` pinned is the one every nested `<Prompt>`
receives.

A first prompt enters its session's FIFO before any route, ensure or runtime for
that placement, and re-reads the placement once the queue grants, so two
concurrent first prompts perform one backend creation and the waiter continues
what its predecessor established.

Both adapter snapshots are rebuilt from new commits on their existing upstream
pull request branches, and the test agent reports acceptance too — an agent that
never sent the marker would leave every session it serves unestablished.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 6 redundant comments. Inline suggestions to remove them below.

harness.handleIds.push(handleId);
// Deferred materialization: the record occupies the key and asserts
// nothing, and the identity the adapter answered with is held here
// until the backend accepts a turn.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// until the backend accepts a turn.

const materialized = promiseWithResolvers<AcpRuntimeMaterialization>();
// Observed here as the provider's own deferreds are: a test that only
// reads events must not turn an unaccepted turn into an unhandled
// rejection.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// rejection.

};
if (!turn.sessionMaterializationPending) {
// Already asserting. This says the session no longer awaits first-turn
// materialization, not that the backend accepted this turn.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// materialization, not that the backend accepted this turn.

applyConversation(asserting, turn.conversation);
try {
// Serialized against live-checkpoint writes, so no interval flush can
// put the pending record back over this materialization.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// put the pending record back over this materialization.

this.activeControllers.delete(turn.record.acpxRecordId);
this.closingActiveRecords.delete(turn.record.acpxRecordId);
}
// Whatever else this turn settled as, no marker means no materialization.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// Whatever else this turn settled as, no marker means no materialization.

rejectMaterialized = reject;
});
// Observed here as the provider's own deferreds are, so a turn nobody
// waited on cannot become an unhandled rejection.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// waited on cannot become an unhandled rejection.

@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown

PR #656: ✨ Make a fresh a pending placement (#648)

34 files, +7130 / -5048

Scope

🔴 PR has 12178 lines changed. Split into focused PRs.

🟡 12178 lines changed. PRs under 400 receive more thorough review.

🟡 34 files changed. Are all changes related?

🟡 Changes span 10 directories.

Structural

Oxlint structural signals:

  • no-unused-vars ×3: packages/acp/src/provider.ts
  • no-unnecessary-type-assertion ×3: packages/acp/tests/helpers.ts, packages/cli/tests/support/fake-acp.ts, packages/acp/src/session-key.ts
  • no-empty-function ×2: packages/acp/src/provider.ts

Slop

  • packages/acp/tests/helpers.ts:304 (removed)
  • packages/acp/tests/helpers.ts:360 (removed)
  • packages/acp/tests/helpers.ts:360 (removed)
  • packages/acp/tests/helpers.ts:360 (removed)
  • packages/acp/src/provider.ts:1209// read are none of them that value.
  • packages/acp/src/provider.ts:1976// either.
  • packages/acp/src/provider.ts:1980// accepted turn.
  • packages/acp/src/provider.ts:2001// rather than staying live with one nobody may use.
  • packages/acp/src/provider.ts:2031// what lets the queue below be entered before any provider effect.
  • packages/acp/src/provider.ts:2050// session resolving.
  • packages/acp/src/provider.ts:2066// reattaches under its own acquisition.
  • packages/acp/src/provider.ts:2500// for one nothing has constructed.
  • packages/acp/src/provider.ts:2758// refuses it.
  • packages/acp/tests/fixtures/fake-codex-app-server.cjs:150// of its own, which is what makes it a separate acceptance path.
  • packages/acp/tests/helpers.ts:325// until the backend accepts a turn.
  • packages/acp/tests/helpers.ts:377// root task would be a pending operation the runner refuses to end on.
  • packages/acp/tests/helpers.ts:381// rejection.
  • packages/acp/tests/helpers.ts:425// A turn that ends still awaiting acceptance never got it.
  • packages/acp/tests/helpers.ts:490// not to.
  • packages/acp/vendor/acpx/generated/runtime.js:852// would resume an arrangement no backend ever accepted.
  • packages/acp/vendor/acpx/generated/runtime.js:944// the compatibility runTurn() surface never reads materialization.
  • packages/acp/vendor/acpx/generated/runtime.js:1071// materialization, not that the backend accepted this turn.
  • packages/acp/vendor/acpx/generated/runtime.js:1204// put the pending record back over this materialization.
  • packages/acp/vendor/acpx/generated/runtime.js:1210// still awaits materialization and finalization writes it that way.
  • packages/acp/vendor/acpx/generated/runtime.js:1323// Whatever else this turn settled as, no marker means no materialization.
  • packages/acp/vendor/acpx/generated/runtime.js:1695// would be an unhandled rejection this materialization owes nobody.
  • packages/cli/src/workflow-agent.ts:338// which is what makes the placement pending rather than established.
  • packages/cli/tests/support/fake-acp.ts:295// root task would be a pending operation the runner refuses to end on.
  • packages/cli/tests/support/fake-acp.ts:303// waited on cannot become an unhandled rejection.
  • packages/cli/tests/support/fake-acp.ts:329// before it says anything, so a manual turn reports it too.
  • packages/test-agent/src/worker/acp-server.ts:153// conversation really exists waits for exactly this and nothing else.

Static Analysis

Oxlint: 37 diagnostics across 5 files (10 rules)
Density: 0.005 violations/added-line

consistent-function-scoping (10): packages/acp/src/provider.ts
no-useless-spread (7): packages/acp/src/provider.ts, packages/cli/tests/support/fake-acp.ts, packages/acp/tests/helpers.ts
no-shadow (3): packages/acp/src/provider.ts
no-unused-vars (3): packages/acp/src/provider.ts
unbound-method (3): packages/acp/tests/helpers.ts, packages/cli/tests/support/fake-acp.ts
no-unnecessary-type-assertion (3): packages/acp/tests/helpers.ts, packages/cli/tests/support/fake-acp.ts, packages/acp/src/session-key.ts
no-underscore-dangle (2): packages/test-agent/src/worker/acp-server.ts
no-empty-function (2): packages/acp/src/provider.ts
no-unsafe-type-assertion (2): packages/acp/tests/helpers.ts, packages/acp/src/provider.ts
no-floating-promises (2): packages/acp/src/provider.ts

Correctness

No extraneous code patterns detected.

Two settled contracts the previous commit broke.

**Concurrent prompts on one advertised session.** Ownership was acquired before
the session's queue, and the coordinator refuses contention rather than queueing
it. That is right for a native UI holding the session for hours and wrong for
the next turn of a conversation this provider is already having: a second
subscription got `AgentSessionBusy` instead of running as the second turn. The
queue comes first now, and each prompt takes and gives back ownership in its
turn.

What still refuses is what genuinely competes. Another provider state, another
process and another kind of operation share no queue with this one, so they all
still meet the coordinator. A launch no longer takes the queue at all: a launch
is only ever performed for an advertised agent, so its acquisition is already
that session's exclusion, and holding the queue as well is what would have put a
prompt behind a native UI — the thing NL12 exists to prevent. It refuses
instead, and the coordinator is what refuses it.

`NO12` is the regression: an advertised adapter and a real coordinator, the
first turn held open while the second subscribes, proving two sequential grants
both granted, one materializing ensure, two turns in submission order, and one
retained identity in the provider's own store. It fails on the previous
ordering.

**Codex commands that start a turn.** The marker was published where a prompt
reaches the App Server through `turn/start` and nowhere else, so `/review`,
`/goal <objective>` and `/goal resume` accepted turns and reported nothing. All
three funnel through the adapter's command turn-started callback, which now
reports it and awaits publication; a command that starts no turn never reaches
it and still reports none. The upstream commit is
`fadc0a690e96c276629be8a34be980d35e821637` on the same pull-request branch, with
its own regressions, and the tarball, generated snapshot, manifest and
provenance are refreshed from it.

`EA8` proves it against the real embedded adapter: the fake App Server answers
`review/start` on a review thread of its own, and the adapter reports exactly one
acceptance for the session the command was sent on. It fails on the previous
tarball.

Neither fix adds an inference anywhere. Acceptance is still one exact marker
from the adapter, and the provider still waits for it.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 11 redundant comments. Inline suggestions to remove them below.

const materialized = promiseWithResolvers<AcpRuntimeMaterialization>();
// Observed here as the provider's own deferreds are: a test that only
// reads events must not turn an unaccepted turn into an unhandled
// rejection.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// rejection.

rejectMaterialized = reject;
});
// Observed here as the provider's own deferreds are, so a turn nobody
// waited on cannot become an unhandled rejection.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// waited on cannot become an unhandled rejection.

};
if (!turn.sessionMaterializationPending) {
// Already asserting. This says the session no longer awaits first-turn
// materialization, not that the backend accepted this turn.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// materialization, not that the backend accepted this turn.

this.activeControllers.delete(turn.record.acpxRecordId);
this.closingActiveRecords.delete(turn.record.acpxRecordId);
}
// Whatever else this turn settled as, no marker means no materialization.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// Whatever else this turn settled as, no marker means no materialization.

// made it. Keeping it would let a retry continue the zero-turn session
// this attempt left behind instead of creating one — and the record it
// stands on is still marked pending, so ACPX would not have resumed it
// either.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// either.

// anyone may prompt through. The provider's assertion stands, and the
// next attachment reconciles and commits that same identity — which it
// can only do by reattaching, so the placement gives up its handle here
// rather than staying live with one nobody may use.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// rather than staying live with one nobody may use.

const materialized = promiseWithResolvers<AcpRuntimeMaterialization>();
// Observed here as the provider's own deferreds are: a test that only
// reads events must not turn an unaccepted turn into an unhandled
// rejection.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// rejection.

rejectMaterialized = reject;
});
// Observed here as the provider's own deferreds are, so a turn nobody
// waited on cannot become an unhandled rejection.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// waited on cannot become an unhandled rejection.

};
if (!turn.sessionMaterializationPending) {
// Already asserting. This says the session no longer awaits first-turn
// materialization, not that the backend accepted this turn.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// materialization, not that the backend accepted this turn.

this.activeControllers.delete(turn.record.acpxRecordId);
this.closingActiveRecords.delete(turn.record.acpxRecordId);
}
// Whatever else this turn settled as, no marker means no materialization.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// Whatever else this turn settled as, no marker means no materialization.

Two of NO12's claims were timed rather than observed. It polled the fake until a
turn appeared, and then waited a fixed five milliseconds before asserting that
the second subscription had not run — which is a claim about what has *not*
happened, and a duration can only ever make that likely.

Three signals replace them. The fake runtime says when a turn has actually
started, which is what "the first turn is in flight" means. The second task says
when it is about to subscribe, immediately after constructing the stream, which
chose nothing and started nothing. And the provider's own agent resolution says
when that subscription is running — the point after which the only thing ahead
of it is the session's queue.

Only then does it assert the held state, and it asserts one more thing than
before: one ensure. A second subscription past the queue would have ensured, so
counting them is what makes "waiting" different from "not started yet".

`startedTurns(count)` on the fake runtime is the first of those signals, built
the way the workflow fake's already is. Nothing else changes; the released
sequence is asserted exactly as it was.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 14 redundant comments. Inline suggestions to remove them below.

// Identity, never the key: this provider issued exactly one value for this
// placement and kept it. A structural copy, a value from another provider
// copy or a torn-down scope, and a look-alike built around a key somebody
// read are none of them that value.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// read are none of them that value.

// made it. Keeping it would let a retry continue the zero-turn session
// this attempt left behind instead of creating one — and the record it
// stands on is still marked pending, so ACPX would not have resumed it
// either.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// either.

//
// The global route slot is deliberately not held across the wait:
// waiting for one session's turn is not a reason to stop every other
// session resolving.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// session resolving.

// Inside ownership, before the runtime exists and before a turn: a
// first Prompt constructs this session through ACP, so that is what
// its construction route says — and a session a native process
// constructed is attached to under the identity it already has.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// constructed is attached to under the identity it already has.

if (clientNative.failure === undefined) {
// A client-native route now exists for this placement, so a <Session>
// nested after this launch attaches to it eagerly instead of taking it
// for one nothing has constructed.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// for one nothing has constructed.

const awaiting = withheld.has(recordId);
// A native promise rather than an Effection future: this is the acpx
// boundary, and a turn nobody accepts leaves it unsettled — which as a
// root task would be a pending operation the runner refuses to end on.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// root task would be a pending operation the runner refuses to end on.

const materialized = promiseWithResolvers<AcpRuntimeMaterialization>();
// Observed here as the provider's own deferreds are: a test that only
// reads events must not turn an unaccepted turn into an unhandled
// rejection.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// rejection.

};
if (!turn.sessionMaterializationPending) {
// Already asserting. This says the session no longer awaits first-turn
// materialization, not that the backend accepted this turn.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// materialization, not that the backend accepted this turn.

this.activeControllers.delete(turn.record.acpxRecordId);
this.closingActiveRecords.delete(turn.record.acpxRecordId);
}
// Whatever else this turn settled as, no marker means no materialization.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// Whatever else this turn settled as, no marker means no materialization.

rejectMaterialized = reject;
});
// Observed here as the provider's own deferreds are, so a turn nobody
// waited on cannot become an unhandled rejection.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// waited on cannot become an unhandled rejection.

The "this subscription is running" barrier was the provider's agent resolution,
which is several suspensions short of the queue: between it and `turns.slot`
lies the whole outer placement. So the held-state assertions were made while the
second subscription might still have been resolving where its session lives, and
what they observed was as much "not there yet" as "waiting".

The signal is the completion of that placement now, which is the step
immediately before the provider enters the queue. It comes from
`withSessionRoute` — the provider's own dependency, supplied the way a host
supplies it — rather than from anything added to production for a test to hold
on to.

`withSessionRoute` wraps two things per subscription, and only placements
*complete* while the first turn is held: the first subscription's second call
encloses its whole turn, which is still open. So the second completion is the
second subscription's placement, and the test asserts that identification rather
than leaving it to be inferred.

The wrapper suspends once before it signals, so nothing here rests on placement
being synchronous — a scheduler that hands control back at a yield has already
done so by the time the waiting task is released, and the signalling task runs
on into the queue before that task resumes.

Restoring ownership-before-queue fails this in both shapes, with the forced
suspension and without it, and fails it on the claim that matters: the second
subscription comes back `AgentSessionBusy` instead of `undefined`.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 27 redundant comments. Inline suggestions to remove them below.

// Identity, never the key: this provider issued exactly one value for this
// placement and kept it. A structural copy, a value from another provider
// copy or a torn-down scope, and a look-alike built around a key somebody
// read are none of them that value.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// read are none of them that value.

// made it. Keeping it would let a retry continue the zero-turn session
// this attempt left behind instead of creating one — and the record it
// stands on is still marked pending, so ACPX would not have resumed it
// either.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// either.

yield* releaseHandle(entry.session.sessionKey);
// The turn's own outcome is the useful thing to say when it has one: the
// barrier only knows that this session is still awaiting its first
// accepted turn.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// accepted turn.

// anyone may prompt through. The provider's assertion stands, and the
// next attachment reconciles and commits that same identity — which it
// can only do by reattaching, so the placement gives up its handle here
// rather than staying live with one nobody may use.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// rather than staying live with one nobody may use.


const prepared = yield* withSessionRoute(context, () =>
// Where this prompt lands. Resolving it constructs nothing, which is
// what lets the queue below be entered before any provider effect.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// what lets the queue below be entered before any provider effect.

resolveAgentSession(read.value, policy, yield* assertions(placementKey), identity);
// rather than from the fact that a key is occupied. A pending ACPX record
// occupies its key and asserts nothing, so this answers `create` for it —
// which is what makes the placement pending rather than established.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// which is what makes the placement pending rather than established.

const recordKey = input.handle.acpxRecordId ?? input.handle.sessionKey;
// A native promise rather than an Effection future: this is the acpx
// boundary, and a turn nobody accepts leaves it unsettled — which as a
// root task would be a pending operation the runner refuses to end on.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// root task would be a pending operation the runner refuses to end on.

rejectMaterialized = reject;
});
// Observed here as the provider's own deferreds are, so a turn nobody
// waited on cannot become an unhandled rejection.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// waited on cannot become an unhandled rejection.

rejectMaterialized(new Error("this session still awaits first-turn materialization"));
} else {
// Acceptance is not the turn's output: a backend takes the turn
// before it says anything, so a manual turn reports it too.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// before it says anything, so a manual turn reports it too.

// This agent takes the turn the moment it is asked, so acceptance is
// reported here — before the turn runs and before anything it
// produces. A client that defers a session's durable state until a
// conversation really exists waits for exactly this and nothing else.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// conversation really exists waits for exactly this and nothing else.

@taras
taras enabled auto-merge (squash) August 30, 2026 02:46
@taras
taras disabled auto-merge August 30, 2026 02:47
@taras
taras merged commit 23b2587 into main Aug 30, 2026
30 checks passed
@taras
taras deleted the agent/issue-648-pending-session branch August 30, 2026 02:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Materialize fresh ACP sessions at the first consuming operation

1 participant