Skip to content

feat(harness): persist delegation ownership [Agent Map 12/15] - #831

Merged
ynadge merged 1 commit into
mainfrom
review/agent-map-12-delegation-state
Sep 6, 2026
Merged

feat(harness): persist delegation ownership [Agent Map 12/15]#831
ynadge merged 1 commit into
mainfrom
review/agent-map-12-delegation-state

Conversation

@ynadge

@ynadge ynadge commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Primary change type

  • Feature

Problem and motivation

Concurrent delegation and retries need one durable reservation and ownership record, including defined capacity, replay, release, and recovery behavior.

Summary and scope

Add the delegation request codec and atomic state machine for bindings, reserved session IDs, fenced claims, context/delivery epochs, bounded history, and dormant release.

Keep unfinished private cleanup proof after receipt expiration until the exact child is closed. Clear completed spawn claims and use locale-independent release-key ordering for stable durable replay.

How this increment fits

The complete delegation store is independently testable. Private child lifecycle arrives in part 13 and public delegation activates in part 14.

Stack and review boundary

  • Part 12 of 15 in the Agent Map review stack; review this increment against its predecessor.
  • Base: review/agent-map-11-focused-briefs.
  • Current head: 58fb5cfe70f892f1301b6e4c222311b12b333bcc; 4,354 changed lines across 7 files, counting additions and deletions including tests.
  • Repackages the corresponding final behavior from #808. Original code and review history remain preserved.
  • Complete coworker testing branch: fix/studio-onboarding-followups.
  • The stack remains unmerged. Dependent PRs target their predecessor, so their diffs do not repeat earlier increments.

Related work

Agent Map checkpoint SAP-3147; relevant work SAP-3151. This packaging follows the maintainer-approved 15-PR split.

Validation

Root checks ran against e2471dcb2d0075328a60926750459275e8bd6312. The final head changes only README terminology or commit ancestry; a complete tracked-file comparison confirms identical executable source and build inputs. The terminology gate was rerun on 58fb5cfe70f892f1301b6e4c222311b12b333bcc.

pnpm build — passed (exit 0)
pnpm typecheck — passed (exit 0)
pnpm lint — passed (exit 0)
pnpm test — passed (exit 0)

Tests and documentation

Regression coverage: Reservations, capacity, fenced claims, lifecycle transitions, replay, stable release digests, retained cleanup proof, expiration, and compaction.

See part 15 for integrated browser, native CLI, and Mac journey validation. The checks above were run independently on this PR’s own commit.

Linux tests run with ordinary user filesystem permissions; the sandbox's extra ambient capabilities are dropped. Hosted CI and automated review are separate from these recorded local results.

Compatibility and release impact

  • Compatibility: Adds public delegation contracts, lifecycle records, limits, codecs, and digest helpers. Missing cleanupComplete is conservatively treated as unfinished cleanup.
  • Changeset: Included: .changeset/durable-delegation-state.md

Security

  • No secrets, credentials, private user data, or unsanitized logs are included.
  • This PR does not publicly disclose a suspected vulnerability.

AI assistance

  • Codex assembled the implementation, addressed reproduced defects, supplied tests and documentation, inspected the diff, and ran the checks above. Reviews are handled by hosted PR automation.

Checklist

  • Read CONTRIBUTING.md; implementation follows the requested 15-PR split.
  • Description reflects this PR's actual predecessor-relative diff.
  • Relevant tests accompany the changed behavior.
  • Root build, typecheck, lint, and test evidence matches the final implementation; any documentation-only update is identified above.
  • Release/documentation treatment is explained above.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Review — PR #831 (round 1)

No confidentiality issues: the changeset, JSDoc, comments and test fixtures are provider-neutral and name no company, customer, tracker or host. Three findings below.


1. transitionSession out of spawn-claimed always fails with malformed_state (correctness)

packages/harness/src/core/subsession-coordinator-store.ts:1946

parseBinding enforces spawnClaim !== null iff sessionState === "spawn-claimed"
(subsession-coordinator-store.ts:470), and transact re-runs
parseSubsessionCoordinatorAggregate on the sealed candidate before every write
(:963) without catching. Every other exit from spawn-claimed nulls the claim —
closeBinding:1065, attachSpawnedRuntime:1936, releaseUnspawnedClaim:1897
but transitionSession never touches spawnClaim, while the transition table
(:704) advertises spawn-claimed → reserved | starting | failed | closed.

Failure: the coordinator claims a spawn, the process launch fails, it calls
transitionSession(identity, bindingId, { …, expectedRuntimeToken: null, state: "failed" }).
Guards pass, sessionState becomes "failed" with spawnClaim still set, the seal-time
revalidation throws SubsessionCoordinatorStoreError("malformed_state"), nothing persists.
The binding is stranded in spawn-claimed, lastError is never recorded, and the caller
gets a storage-corruption-class code for an ordinary spawn failure.

No test covers this: every transitionSession call in
subsession-coordinator-store.test.ts (451, 550, 795, 871, 938, 1017, 1128) either starts
from reserved or runs attachSpawnedRuntime first, so spawn-claimed is never the
source state. (Verified by reading — I could not run a scratch repro in this sandbox.)

Fix: clear spawnClaim when leaving spawn-claimed, or reject that source state in
transitionSession and prune it from transitions so the declared machine matches the
persistence invariant. Add a regression from spawn-claimed → failed.

2. patch changeset ships ~30 new public exports, described as "internal"

.changeset/durable-delegation-state.md, packages/harness/src/index.ts:237-277

The diff adds 8 constants, 21 types and 5 functions to the @sapiom/harness public entry
point. That is a backward-compatible addition, which the repo's own precedent bumps as
minor (.changeset/focused-project-briefs.md, .changeset/bright-studios-start.md), not
patch. Worse, the published CHANGELOG line reads "Add internal durable delegation
storage…" — a consumer reading that in the npm tarball gets no signal that 30 symbols
entered the API surface they can now pin against. Changeset prose cannot be edited after
publish.

Nothing in-repo consumes these exports: grep for the modules returns only the store, the
codec and their tests. Either (a) bump to minor and say what the surface is, or (b) drop
the src/index.ts re-exports until PR 14 actually activates the tool — which matches the
PR body's own claim that "the complete store state machine is internal".

Note also that PROJECT_SUBSESSION_MAX_DEPTH and PROJECT_SUBSESSION_LIVE_SESSION_LIMIT
are the two constants not re-exported, so the published set is already an arbitrary
subset of the same module.

3. release keys are sorted by locale collation, feeding a durable digest

packages/harness/src/shared/subsession-delegation-codec.ts:301

The delegate branch sorts delegation keys by code point (:221-227); the release
branch sorts the same alphabet with left.localeCompare(right). That sorted array is
canonicalised into computeCanonicalDelegationRequestDigest, and the digest is persisted
in the receipt and compared byte-for-byte on replay
(subsession-coordinator-store.ts:1155, :1263).

localeCompare is ICU- and locale-dependent. ["ab", "a-c"] orders a-c, ab by code
point but ab, a-c under en-US collation, because punctuation is treated as variable.
So a process on a small-icu Node build, or after an ICU data upgrade, recomputes a
different digest for the identical request and the replay path throws
request_key_reused instead of returning the idempotent result — exactly the retry the
receipt exists to absorb. The keys are already constrained to [A-Za-z0-9._-] by isKey,
so there is no reason to reach for collation: use the same code-point comparator as the
delegate branch, and cover it with a key pair that the two orderings disagree on.


Verdict: request changes — #1 is a live state-machine bug with no test coverage, and
#2 puts an unintended API contract and misleading copy into an unretractable npm
CHANGELOG.

@ynadge
ynadge force-pushed the review/agent-map-11-focused-briefs branch from 6766473 to 662edab Compare September 5, 2026 11:57
@ynadge
ynadge force-pushed the review/agent-map-12-delegation-state branch from 92b36da to e2471dc Compare September 5, 2026 11:57
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Review — PR #831 (round 2, follow-up)

Delta since 92b36da8: one commit, e2471dcb. No confidentiality issues in the rewritten changeset or new tests.

Resolved: round-1 #2 (changeset now minor, prose names the published surface) and #3 (release keys sort by code point, matching the delegate branch, with a ["ab","a-c"] regression).

1. Round-1 #1 fixed only on the exit edge — the entry edge still corrupts (correctness)

packages/harness/src/core/subsession-coordinator-store.ts:1969

if (request.state !== "spawn-claimed") binding.spawnClaim = null; clears the claim when leaving spawn-claimed, but transitions (:704) also advertises reserved | exited | failed → spawn-claimed, and transitionSession never sets a claim. parseBinding:470 requires spawnClaim !== null iff sessionState === "spawn-claimed", and transact re-parses the sealed candidate (:963), so:

transitionSession(identity, bindingId, { expectedLifecycleEpoch, expectedSpawnEpoch, expectedRuntimeToken: null, state: "spawn-claimed" }) from reserved passes every guard, writes sessionState: "spawn-claimed" with spawnClaim: null, then throws SubsessionCoordinatorStoreError("malformed_state") at seal. Nothing persists, the caller gets a storage-corruption code for a transition the table declares legal.

claimSpawn:1788 is the only correct entry — it also bumps spawnEpoch (:1822), which transitionSession cannot. So the table is wrong, not the writer: drop "spawn-claimed" from the reserved/exited/failed target lists (or reject it in transitionSession) and add a regression asserting lifecycle_conflict rather than malformed_state. No test exercises this direction — state: "spawn-claimed" appears nowhere in the suite.

Nit

  • Round 1's note stands: PROJECT_SUBSESSION_MAX_DEPTH and PROJECT_SUBSESSION_LIVE_SESSION_LIMIT are still the only limits not re-exported (src/index.ts:239-245) while the changeset now advertises "bounded limits" as published — export them or drop the claim.

Verdict: request changes — one half of round-1 #1 remains, same failure mode, opposite direction.

@ynadge
ynadge force-pushed the review/agent-map-11-focused-briefs branch from 662edab to 74884b1 Compare September 5, 2026 12:17
@ynadge
ynadge force-pushed the review/agent-map-12-delegation-state branch from e2471dc to 58fb5cf Compare September 5, 2026 12:17
Base automatically changed from review/agent-map-11-focused-briefs to main September 6, 2026 22:21
@ynadge
ynadge merged commit 2b5a234 into main Sep 6, 2026
2 checks passed
@ynadge
ynadge deleted the review/agent-map-12-delegation-state branch September 6, 2026 22:23
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.

1 participant