Skip to content

docs(adr): 0007 — deferred delivery posts at most once per part, retry-safe (proposed) - #41

Merged
marklysze merged 6 commits into
mainfrom
docs/adr-0007-deferred-delivery
Nov 7, 2026
Merged

marklysze merged 6 commits into
mainfrom
docs/adr-0007-deferred-delivery

Conversation

@marklysze

Copy link
Copy Markdown
Contributor

Proposed ADR. Agents that finish work after their Turn (image generation, builds, scheduled tasks) get a way to deliver text and files to the room exactly once:

  1. A standing listener for spawned agents that advertise ag2/deliver.
  2. Worker-owned room bindings, each with its own export directory.
  3. _ag2/deliver, a JSON-RPC request whose response is the acknowledgment. Its results are:
    • Delivered;
    • Retry;
    • Final, which is one of unknown, conflict, rejected or expired.
  4. Standard ACP resource_link files in a Turn are sent.

Depends on the broker change, an optional Idempotency-Key on POST /v1/rooms/{room}/media and a new POST /v1/rooms/{room}/messages. Until that ships, _ag2/deliver is not advertised.

Reviewed so far:

  • backend-openclaw cross-checked it against the proposed key semantics: the 409 state field, unknown being final, and the TTL.
  • The OpenClaw agent confirmed it fits their Gateway-owned durable queue and the standing stdio listener. Their requests are folded in: expires_at on every result; renewal keeps the binding and its folder; no cleanup during an in-flight delivery; a binding is a destination, not a credential.

Decisions recorded:

  • Keys live 24h; agent-connect stops retrying at 23h.
  • Bindings expire 30 days after the room's last Turn, renewed on each Turn.
  • Export folders live at <workspace>/export/<binding>/.
  • The delivery record is <workspace>/deliveries.json, entries kept 30 days.
  • The work ships as agent-connect 0.3.0, with relay client 0.2.0.

Docs only; no code changes.

@sonichi sonichi 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.

Reviewed at 78f6c3d. Comment only; it is a proposed ADR, so I read it for consistency and failure behavior, not code. I found no flaw in the delivery protocol itself.

Sound. Each part is posted under Idempotency-Key = sha256(binding, id, part), so a lost response retried under the same id replays instead of posting twice; unknown is final on purpose (a missing file, never a duplicate); the 23-hour stop sits inside the broker's 24-hour key lifetime, and the ledger keeps first-seen times across restarts so expired still refuses; the honest limit (a resend after the 30-day ledger entry is gone can post twice) is stated. Binding to a room by the Worker, not the agent, plus per-binding export folders and "a binding is a destination, not a credential" are the right boundaries.

Things to settle before it goes from proposed to accepted

  1. Bindings and #40. The ADR keys a binding per (room, Access Tier). #40 admits listed collaborators as guest tier but gives each their own Session per (room, sender). Two collaborators in one room would then share a guest-tier binding and its export_dir, though their Sessions are separate. Say whether the binding follows session_key instead (per sender) or deliberately the tier, and what that does to the "one room cannot send another's file" guarantee between senders in the same room.
  2. Revocation vs a 30-day binding. A binding lives 30 days after the room's last Turn. If the owner removes a collaborator from AGENT_CONNECT_ACP_COLLABORATORS (or the room stops granting Team), an agent that already has that binding can still deliver into the room for the rest of the window. A rejected covers a room the Worker may not post to, not a sender who is no longer admitted. Either re-check admission for the requesting sender at delivery, or drop bindings created for a sender when they leave the list.
  3. No content filter on deliveries. #40's docs say the room's sensitive-data filter is not implemented; deliveries add an asynchronous path (text and files, up to 30 days later) that the filter would also miss. Worth one sentence under Consequences.
  4. resource_link files in a Turn. Treating a file:// link as an outgoing file under the existing allowlist matches [file: …], so it is no wider than today; for an admitted collaborator it means a prompt can ask the agent to return any file inside the allowlist roots, which is the same as the marker. Worth saying so, so the allowlist roots are chosen with collaborators in mind.

@marklysze

Copy link
Copy Markdown
Contributor Author

Thanks @sonichi, all four addressed in 8757433:

  1. Bindings follow the Session key, not the tier. That means one binding per (room, owner) or (room, collaborator). Two collaborators in one room never share a binding or an export_dir, so the cross-room file guarantee also holds between senders in the same room. Test added.
  2. Revocation. A collaborator's binding records the sender. At delivery the Worker re-checks the allowlist and answers rejected if the sender is gone. A Turn in which the broker no longer attests them as Team collaborator drops their bindings. Test added.
  3. Consequences. Deliveries, including a collaborator's up to 30 days later, are not scanned by the sensitive data filter.
  4. Consequences. resource_link is no wider than [file: …]. For collaborators, choose the egress roots accordingly.

@marklysze
marklysze requested a review from sonichi November 7, 2026 04:04

@akag2 akag2 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.

Re-reviewed at 8757433. The additions you listed are there: per-sender bindings, the allowlist re-check with rejected, dropping bindings on demotion, and the Consequences bullets. Some defects remain.

1. Cross-binding egress (lines 40-41 vs 48). The Worker still adds <workspace>/export/ to the shared egress allowlist, and Turn egress ([file:], resource_link) checks only the allowlist (outgoing.py:86-110). So a collaborator's Turn can attach export/<owner-binding>/report.pdf and post the owner's deferred export in the room. Line 48 ("one room cannot send another room's file") doesn't hold, and with per-sender bindings the same leak now crosses senders too. Please keep export/ out of the Turn allowlist, or apply the binding check to Turn egress.

2. Revocation is not immediate (lines 49-55). Broker-side revocation is noticed only when a later Turn is no longer attested. A sender whose Team grant is removed, or who leaves the room, may never send another Turn, so their bindings keep delivering for up to 30 days. The env list is read once, so "at once" really means after a Worker restart. Line 49 (a binding never expires while a delivery is in flight) also conflicts with the immediate drop, so please state which wins.

3. Crash and compatibility windows (lines 92-96, 128).

  • Ledger ordering: the ADR needs to say the first-seen ledger entry is written and fsynced before the first relay call. Otherwise a crash after posting restarts the 23h clock, and a resend after 24h posts twice.
  • Broker support: "not advertised until this lands" checks only the relay-client version. A broker that ignores Idempotency-Key makes every retry a silent duplicate. Please require a capability probe, or an echoed key status, and refuse delivery without it.

Minor:

  • Title: it says "exactly once", but the body describes at most one post per part while the key lives, with retries.
  • Key encoding: sha256(binding, id, part) (line 69) needs a fixed encoding (length-prefixed or canonical JSON). Otherwise id="x1",part=0 and id="x",part=10 collide.
  • Ordering: within and across deliveries it is unspecified.
  • Setup: export/ must exist before the egress client is built (outgoing.py:96-97), or every delivery is refused. Also say whether WorkingDirectoryPolicy extends to export_dir.
  • Ledger size: deliveries.json is rewritten whole and keeps 30 days of entries. Please add a cap or compaction rule.

@sonichi sonichi 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.

Re-reviewed at 8757433 (docs only, one commit over 78f6c3d). Approving once CI is green. All four of my points are answered in the text:

  • Bindings follow the Session key, so two collaborators in one room never share a binding or an export_dir; the test list adds that case.
  • Revocation takes effect at once. A collaborator's binding records the sender; delivery re-checks the allowlist and answers rejected for a removed sender, and a Turn where the broker no longer attests Team collaborator drops their bindings; a test covers delivery after removal.
  • Consequences now says deliveries are not scanned by the room's sensitive data filter (including one up to 30 days after a collaborator's Turn), and that resource_link is no wider than a [file: …] marker, so the allowlist roots should be chosen with collaborators in mind.

Still a proposed ADR with its broker dependency unmet, so nothing ships from it; approving the design as written.

@yixuan-ag2 yixuan-ag2 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.

Reviewed at 8757433. Requesting changes: several guarantees in the ADR don't hold as written. I agree with all of @akag2's points at this head and won't repeat them. Below is evidence for some of them and five new items.

What I checked of the pushed changes. Per-sender bindings, the list re-check with rejected, the drop on a non-attested Turn, and the two Consequences bullets are all in the text. The added tests are two new cases in the ADR's planned test list (lines 169-170); the PR is still one Markdown file, and the repo has no ag2/deliver, binding or ledger code yet. So "a test covers delivery after removal" in the approval above describes a planned test, not one that runs today.

Blocking

  1. Shared export/ root (lines 39-41 vs 37 and 47-48). Agree with @akag2 #1. I confirmed it against the relay client: EgressAllowlist([repo]) refuses export/<other-binding>/x, EgressAllowlist([repo, workspace/export]) allows it, and markers.parse picks it up from a Turn's [file: …]. RoomOps.upload and complete share that one allowlist (relay-client client.py:425-432). Two additions:

    • The per-binding check in line 47 is a Worker-side pre-approval of a path string, which outgoing.py:158-161 says a consumer must not do. Put the narrowing in the relay client, judged on the descriptor. Two options: a separate allowlist per binding, or an upload that walks from export_dir as its root.
    • The agent writes every export_dir as the same user, so it can copy files between them. State the guarantee as "the Worker refuses cross-binding paths", not "one room cannot send another room's file".
  2. Today's Workers answer _ag2/deliver with success and post nothing. Only the agent declares ag2/deliver (line 23-24). Every 0.2.x Worker already says ag2/receive: true. _ClientCallbacks subclasses acp.Client without defining ext_method, so it inherits the Protocol stub that returns None. With agent-client-protocol 0.12.1's build_client_router, _ag2/deliver to _ClientCallbacks comes back as a JSON-RPC success with result: null; a client without ext_method gets -32601. An agent that follows line 134 ("treat only a success response as delivered") drops the delivery and nothing reaches the room. It is the same class of hole ADR 0005 fixed with ag2/receive. Please:

    • have the client declare _meta["ag2/deliver"] = true on initialize, and the agent send _ag2/deliver only to a client that did;
    • require a result whose delivered list covers every part.

    Line 128's "not advertised" then becomes "the Worker does not declare it". As written it names the agent, which the Worker can't control.

  3. The ledger is fail-open (line 95). Agree with @akag2 on write and fsync before the first relay call, and on a size cap. Separately, "written like the Session map" means:

    • an unreadable file is an empty store;
    • write errors are swallowed;
    • there is no fsync, and the temp name is fixed (sessions.py:23-26, 208-215, 229-240).

    A truncated store loads empty and the Worker keeps going. Since the ledger is the only thing that enforces expired after the key's 24 hours, a corrupt, deleted or unwritable ledger turns every old resend into a new post. Please:

    • fail closed while it cannot be read or the first-seen write fails (Retry, never "new");
    • require a single writer: two Worker processes on one workspace each rewrite the whole file;
    • say what is lost if it is deleted.
  4. A broker without the key gives silent duplicates (lines 115-132). Agree with @akag2 on requiring an echo. As far as I can see the broker doesn't implement the header yet: the media route reads only the JSON body, and there is no POST on /v1/rooms/{room}/messages. So a header sent today is ignored, and a retry after a lost response posts the file twice with two 200s. Line 127 covers "no header", not "a header the broker doesn't know". The relay-client 0.2.0 / agent-connect 0.3.0 versions say nothing about the broker the token points at. Please require the broker to echo that it honoured the key, and refuse the delivery path without it.

  5. The relay client can't produce the outcomes in lines 77-93, and a 409 switches off room ops everywhere.

    • RoomOps._call maps every failure to None (roomops.py:365-389) and trips a 300 s cooldown for all room ops (:74, :391-393).
    • UploadResult carries no event_id.

    With a fake transport: 409 running, 409 unknown and 413 all return None, and the next message to an unrelated room is not sent. A 200 returns the id. So the Worker can't tell Retry from unknown, conflict or rejected, and an ordinary running during a retry stops placeholders and edits in every room for five minutes. Dependencies needs:

    • the status, 409 state and event_id surfaced;
    • 409 and 413 kept out of the cooldown;
    • a cooldown refusal (nothing sent) mapped to Retry.

Should-fix

  • Revocation (lines 51-54). @sonichi's #2 is only partly met; agree with @akag2 #2. #40 reads the list from the environment, so "at once" means after a restart. Outside a Turn the Worker can't learn that the room stopped granting Team or that the sender left. Give collaborator bindings a short life (for example the Session idle limit), or state the window.
  • Contradiction. Lines 89-90 make "a room the Worker may not post to" a Final rejected (drop and log). Lines 161-163 say the agent retries and it is not dropped.
  • Unknown binding. Line 44 names no outcome, and unknown already means the 409 case. Concurrent dialers are allowed (ADR 0004), so a Worker that didn't issue a binding, or that lost its store, will be handed one. A Final there drops a deliverable delivery. Define a distinct "not this client" outcome, let the binding identify its Worker, and say how the binding store is written.
  • Connection closes mid-delivery. Deliveries can arrive on a Turn's dial, which closes, or whose process is killed, at Turn end. Say that an accepted delivery runs to completion regardless, or accept _ag2/deliver only on the standing listener.
  • Partial delivery. A Final after part 0 leaves a caption promising a file that never comes. The relay client's rule is that a failed file is said in the room. Post a one-line notice, or send file parts first.
  • Dependencies.
    • Lines 35-36 and 51-52 depend on #40 (ADR 0003's amendment, AGENT_CONNECT_ACP_COLLABORATORS). List it.
    • ADR 0005's "Posting is unconditional on Tier: the agent is only ever reached at owner tier" stops being true. Amend it or reference it.
  • export_dir. Agree with @akag2 that the root must exist before the client is built, and that WorkingDirectoryPolicy must cover the folder. Also: create the folders 0700, delete without following symlinks the agent placed there, and say whether a delivered attachment is removed or kept until the binding expires (up to 30 days after the last Turn).

Nits

  • Key encoding: agree with @akag2. Also state that the broker scopes keys per agent identity.
  • Define the part index for the text, and whether text and files are separate events.
  • A resource_link from a dialled remote agent names a remote path (ADR 0004). Limit it to spawned or loopback agents.
  • Line 37 isn't wrapped like the rest of the file.

Credit: @akag2 for the cross-binding egress, revocation, ledger ordering, broker probe, encoding and setup points; @sonichi for the original four.

@marklysze marklysze changed the title docs(adr): 0007 — deferred delivery reaches the room exactly once (proposed) docs(adr): 0007 — deferred delivery posts at most once per part, retry-safe (proposed) Nov 7, 2026
@marklysze

Copy link
Copy Markdown
Contributor Author

Thanks @akag2 and @yixuan-ag2. Addressed in 116fb3c; @sonichi's earlier four still hold. Point by point:

Blocking

  1. Shared export/ (akag2 feat: one-line installer (curl | sh) for BYO-agent connect #1, yixuan feat: one-line installer (curl | sh) for BYO-agent connect #1). export/ is no longer a Turn egress root. Delivery uploads are judged by the relay client against an allowlist rooted at that binding's export_dir, on the descriptor, with no Worker string pre-approval. The guarantee now reads "the Worker refuses cross-binding paths", and the ADR says the agent can copy between folders as the same user. "Add export/ to Turn egress" is listed as a rejected option.
  2. Success-null from today's Workers (yixuan fix(install): fetch the full 8-module relay closure #2). The Worker declares _meta["ag2/deliver"] on initialize. The agent sends only to a client that declared it, and the ADR cites _ClientCallbacks lacking ext_method as the reason. Delivered requires delivered to cover every part.
  3. Ledger fails closed (akag2 feat(install): relay client from PyPI (ag2-sparrow) + launcher that actually starts it under services #3, yixuan feat(install): relay client from PyPI (ag2-sparrow) + launcher that actually starts it under services #3). The first-seen entry is fsynced before the first relay call. Unreadable or unwritable means Retry, never empty. There is a single writer via a lock file. Size cap is 10,000 with in-progress entries never dropped. What deletion loses is stated. It no longer says "written like the Session map".
  4. Broker that ignores the key (akag2 feat(install): relay client from PyPI (ag2-sparrow) + launcher that actually starts it under services #3, yixuan fix(worker): parse headers after task: — access_tier was silently dropped #4). Keyed responses must echo Idempotency-Status: created|replayed. Without the echo the Worker does not declare ag2/deliver, and a missing echo is answered Retry. Keys are scoped per agent identity.
  5. Relay client (yixuan feat(install): --sutando-workspace — connect an already-running Sutando (relay-only) #5). Dependencies now require:
    • status, 409 state, echo and event_id returned to the caller, with event_id on UploadResult;
    • 409 and 413 kept out of the cooldown;
    • a cooldown refusal told apart from a failure, and answered Retry.

Should-fix

  • Revocation (akag2 fix(install): fetch the full 8-module relay closure #2, yixuan). A collaborator binding lives 24h after that sender's last attested Turn, and that window is stated honestly. The env list takes effect on restart. Revocation and expiry stop new deliveries; one already accepted completes.
  • Contradiction. Consequences now matches Final rejected.
  • Unknown binding. A new Final not_mine means "go back to the issuing client", not a drop. Bindings are <worker id>.<random>. The binding store is written under the ledger's rules.
  • Connection closes. _ag2/deliver is accepted only on the standing listener, and an accepted delivery runs to completion.
  • Partial delivery. Files go first and text last. A later Final posts a one-line notice. Part indexing is defined, and each part is its own event.
  • Dependencies. feat(acp): admit listed Team collaborators, with per-sender sessions and requester metadata #40 is listed. ADR 0005's owner-only assumption is flagged for amendment.
  • export_dir. The root is created before the egress client. Folders are 0700. The Permission Policy covers the session's own folder. Deletion doesn't follow symlinks. Files are kept until the binding expires.

Nits

  • The title is now "at most once per part, retry-safe". The guarantee is spelled out, including that it isn't exactly-once.
  • Key encoding: canonical JSON of [binding, id, part].
  • Ordering: parts are sequential, with none across deliveries.
  • resource_link is limited to spawned or loopback agents.
  • Line wrapping is fixed.
  • Collaborator bindings aren't issued while the room's sensitive data filter is on, until feat(acp): admit listed Team collaborators, with per-sender sessions and requester metadata #40's result scan covers deliveries.

The test list is marked as planned, since this PR is docs only.

@sonichi sonichi 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.

Re-reviewed at 116fb3c (docs only; the ADR was substantially revised). Approving once CI is green; my earlier approval was at 8757433.

I read the whole revised text. The weak points of the first version are closed in the right places:

  • Both sides declare ag2/deliver, and the ADR says why with the concrete hole: today's Workers have no ext_method, so an unknown request comes back as a JSON-RPC success with result: null, which an agent would read as delivered.
  • The ledger fails closed: first-seen is fsynced (file and directory) before the first relay call, an unreadable ledger answers Retry and is never treated as empty, one process owns it by lock, and the honest limit if the file is deleted is stated.
  • export/ is not a Turn egress root, so a [file:] or resource_link cannot name another binding's files; delivery attachments are judged by the relay client against one binding's folder on the opened descriptor, not by a path-string pre-check. The key is a canonical JSON hash, so ("x1", 0) and ("x", 10) cannot collide, and keys are scoped per agent identity at the broker.
  • Collaborator bindings are per sender, last 24 hours from that sender's last attested Turn, drop when the broker stops attesting them, and are re-checked against the list at delivery; the 24-hour bound is explained as the honest limit of what the Worker can know.
  • The guarantee is restated honestly as "at most once per part while the broker remembers the key", with unknown and expired as ways a delivery ends unposted.

Two things to reconcile, not blockers: Consequences says collaborator bindings are not issued while the room's filter is on until #40's result scan covers deliveries, but #40 as it stands gives collaborator Sessions no ag2/destination at all, so no binding is issued whether or not the filter is on; say which is meant. And when #40 lands, ADR 0003's amendment and this ADR should cross-link the guard so a later delivery path knows it must go through it.

@akag2 akag2 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.

Re-reviewed at 116fb3c. My earlier points:

  • Closed:
    • Cross-binding egress: lines 84-88, 237.
    • Ledger order: fsync before the first relay call, fail-closed (150-154).
    • Broker capability: the echo (197-204).
    • Title: fixed.
    • Key encoding: canonical JSON (110).
    • export/ at build time: 78-79.
  • Bounded honestly: revocation latency (62-69).

Remaining:

1. major: re-binding brings duplicates back (lines 74-76, 132-134, 157). On an expired or lost binding the Worker answers not_mine, and the agent waits for a new Turn's binding. The binding is part of the key, so a new binding means a new key, and the ledger entry is dropped when the binding expires. Failure scenario:

  1. A collaborator delivery posts part 0, and the response is lost.
  2. The agent gets Retry, which releases the in-flight hold.
  3. The binding expires at 24h.
  4. The agent resends the same id under the next Turn's binding, and part 0 posts again.

Please key by an agent-scoped id rather than the binding, or make not_mine final for any id that already has a first-seen entry.

Minor:
2. Startup echo check (line 202). Proving the broker honours the key needs a keyed post somewhere. Please name a side-effect-free probe endpoint.
3. Overlapping roots (line 84). export/ is only excluded as a root of its own. If AGENT_CONNECT_REPO or AGENT_CONNECT_EGRESS_ROOTS contains the workspace (e.g. a repo at ~), other bindings' exports are under a Turn root again. Please refuse or strip overlapping roots, and give the test at 267 that rule.
4. Lock and notice (155-156, 105-107).

  • Say the lock is flock. A plain lock file left by kill -9 would block the listener permanently.
  • Key the partial-failure room notice too, so a crash plus retry can't post it twice.

@yixuan-ag2 yixuan-ag2 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.

Re-reviewed at 116fb3c. Thanks for the thorough rewrite. All five of my earlier blockers are fixed in the text:

  • export/ is out of Turn egress, judged by the relay client on the descriptor (84-90, 237-238);
  • both sides declare ag2/deliver, and Delivered must cover every part (28-34, 119-121);
  • the ledger fails closed, is written ahead with fsync and has one writer (146-163);
  • the broker echo is required (197-205);
  • the relay-client outcomes and cooldown are fixed (206-219).

My should-fixes are in too:

  • the revocation window (60-71);
  • consistent rejected (140-142, 252-254);
  • listener-only delivery (36-44);
  • files first and the notice (103-107);
  • #40 and ADR 0005 listed (182-186);
  • folder hygiene (78-82).

I'm still requesting changes, for one blocker.

Blocking

Re-binding brings duplicates back (74-76, 132-134, 157). Agree with @akag2 #1, including the fix: key by an agent-scoped id, or make not_mine final for an id that has a first-seen entry. Three additions:

  • It also undoes revocation. A demoted collaborator's dropped binding (65-66) answers not_mine, not rejected (141). "Wait for a new Turn's binding" then lets that delivery go out under the next binding in the room, which can be the owner's. A dropped or revoked own binding should be Final.
  • Check the ledger before the binding, and keep entries their full 30 days regardless of binding expiry. Otherwise line 43-44 ("learns the result on its retry, from the ledger") fails whenever the binding expired while the delivery finished (70-71). Limit not_mine to a <worker id> that isn't this Worker's, and say that id is stable and stored in the workspace.
  • This is partly my fault. My last review suggested a "not this client" answer for a lost store, and it should have been Final.

Should-fix

  • Bound the per-binding root (84-88, 217). The relay client's allowlist is fixed at construction and sealed, so it can't be widened at runtime. Bindings appear at runtime, so a plain "upload with this root" parameter would be exactly that widening. Suggest: build the client with <workspace>/export/ as a separate delivery root (line 78 already creates it before the client), and require a per-delivery root to resolve strictly inside it. It only narrows. This sits next to @akag2 #3 on overlapping Turn roots, which I agree with: with the repo at ~, EgressAllowlist([~]) allows ~/.agent-connect/workspace/export/<other>/x.
  • A keyed 200 without the echo (197-205). Agree with @akag2 #2 on a side-effect-free probe. Separately, a keyed 200 without the echo means the part did post. Answering Retry (203) invites a resend that posts twice once the path is re-enabled. Record it as posted, answer Delivered for it, and stop declaring for new deliveries.
  • Where the declaration goes (29 vs 36). Line 29 puts it "beside ag2/receive", which Turn connections also send (ADR 0005). Say ag2/deliver is declared only on the listener's initialize, and that _ag2/deliver on a Turn connection gets an explicit error, never the inherited success-null.
  • #40's ag2/destination. As @sonichi noted, #40 at 84fa123 gives non-owner Sessions no ag2/destination at all, while this ADR carries collaborator bindings inside it (49-56, 244-246). Please either:
    • put collaborator bindings under their own key, only when sensitive_data_filter is off; or
    • issue none until deliveries pass through #40's guard.

Nits

  • Line 110: pin the JSON escaping (RFC 8785, or ASCII-escaped). Escaped and raw UTF-8 give different keys for a non-ASCII id.
  • Line 105-107: for unknown the part may have posted, so "may not have been sent". Keying the notice is @akag2 #4.
  • Line 127: say which expires_at a Retry carries when the ledger can't be read.

@marklysze

Copy link
Copy Markdown
Contributor Author

Thanks @akag2 and @yixuan-ag2. Addressed in 92f738f:

Re-binding duplicates (akag2 #1, yixuan blocker)

  • The key no longer includes the binding. It is sha256(JCS([worker id, id, part])), with JCS being RFC 8785. The worker id is random, stored in <workspace>/worker-id and stable across restarts. Resending an id can't create a new key.
  • The ledger is read before the binding. A known id is answered from the ledger (its stored result, or expired), or continued if still in progress. A resend under a different binding is conflict. The ledger also records each id's binding and room. Entries live their full 30 days, whatever happens to their binding.
  • not_mine is narrowed to another Worker's binding with no ledger entry. This Worker's own binding that is expired, revoked or missing is a Final rejected, so a demoted collaborator's delivery can't go out later under the owner's binding.
  • Tests added for the failure scenario: lost response, binding expires, same id resent, no second post. Also for a known id under another binding, a result after the binding expired, and a demoted sender.

Startup probe (akag2 #2): a side-effect-free GET /v1/capabilities reporting the idempotency header, TTL and routes, added to the broker dependency. It runs at start and on each listener restart.

Keyed 200 without the echo (yixuan): recorded as posted and answered Delivered, and ag2/deliver stops being declared for new deliveries until a probe succeeds.

Overlapping roots (akag2 #3, yixuan): an allowlist can't exclude a subtree. So if any Turn egress root contains <workspace>/export/ (for example a repo at ~), delivery isn't enabled at all, and the Worker says why at start. Test added.

Per-binding root (yixuan): the relay client is built with <workspace>/export/ as a sealed delivery root. A per-delivery export_dir must resolve strictly inside it, so it only narrows. Test added.

Lock and notice (akag2 #4): the lock is flock, which the kernel releases on kill -9. The partial-failure notice is keyed as part = "notice". For unknown it says "may not have been sent".

Where it's declared (yixuan): ag2/deliver goes only on the listener's initialize. _ag2/deliver on a Turn connection gets an explicit method-not-found error.

#40's ag2/destination (yixuan): bindings move to their own _meta["ag2/delivery"]. A collaborator Session gets it only while the room's sensitive data filter is off for that agent, until deliveries pass #40's result guard.

Nits

  • A Retry when the ledger is unreadable carries expires_at: null, and the agent keeps its own deadline.
  • JSON escaping is pinned to RFC 8785.

@marklysze
marklysze requested review from akag2 and yixuan-ag2 November 7, 2026 04:34
@marklysze

Copy link
Copy Markdown
Contributor Author

Aligned with #40 in d1a36ea. A room moderator can change an agent's sensitive-data-filter flag, so a collaborator Session gets ag2/delivery only while the owner has set AGENT_CONNECT_UNCHECKED_COLLABORATORS on their machine, not when the room flag is off. Also, the broker side is now ag2space-backend#3552 (draft), with GET /v1/capabilities → room_post_idempotency {header, ttl_seconds: 86400, routes} and Idempotency-Status: created|replayed, matching this ADR.

@akag2 akag2 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.

Re-reviewed at d1a36ea. Verified fixed:

  • Key: sha256(JCS([worker id, id, part])), with no binding in it (125-129).
  • Ledger before binding: a known id under another binding gets conflict (164-169).
  • not_mine and rejected: not_mine now covers only a foreign binding with no record (150-152). This Worker's own expired bindings get rejected instead.
  • Retention: 30 days, independent of the binding.
  • Earlier minors: the GET /v1/capabilities probe (226-235), overlapping roots disabling delivery (92-96), flock (180-182), and the keyed notice (122-123).

1. major: a lost worker-id with the ledger still present (59-61, 174-189). The id is random and kept in <workspace>/worker-id, but the ADR doesn't say it is fsynced. It also doesn't say what happens if the file is missing or unreadable at start while deliveries.json exists. "Create once if absent" would silently mint a new id. Every key would change, an in-progress entry would be "continued" (166) under the new key, and parts that already landed would post again. The Worker's own live bindings would also carry the old id and get not_mine. Fix:

  • store the worker id in each ledger entry and key on the stored value;
  • refuse to enable delivery when the ledger exists but worker-id is missing or disagrees;
  • fsync worker-id like the ledger.

Minor: line 196 says "_ag2/notify is unchanged", which leaves #40's notify bypass as the unguarded path next to the guarded _ag2/deliver. Please state that, or depend on #40's fix.

@marklysze

Copy link
Copy Markdown
Contributor Author

Thanks @akag2. Addressed in bf1f6d9:

  1. Lost worker-id (major):
    • Each ledger entry stores its worker id, and an entry is always continued under the key it started with, never the current file's id.
    • worker-id is written like the ledger (temp file, fsync, rename, fsync of the directory).
    • If deliveries.json exists but worker-id is missing, unreadable or disagrees with the entries, delivery isn't enabled and the Worker says why. A new id is never minted over an existing ledger.
    • Planned test added.
  2. _ag2/notify (minor): section 4 now says feat(acp): admit listed Team collaborators, with per-sender sessions and requester metadata #40 guards it. While any collaborator is listed, notify text goes through feat(acp): admit listed Team collaborators, with per-sender sessions and requester metadata #40's result guard and may only target a room where the owner has a Session; otherwise it is withheld. Both unasked paths are guarded. Notify stays best-effort, with no retry safety.

I also rewrapped two lines that had run long.

@marklysze
marklysze requested review from akag2 and removed request for yixuan-ag2 November 7, 2026 05:07

@yixuan-ag2 yixuan-ag2 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.

Re-reviewed at bf1f6d9, which also covers d1a36ea. Approving.

The re-binding duplicate is fixed:

  • the key drops the binding (129-137);
  • the ledger is read before the binding (170-175);
  • entries live their full 30 days (189);
  • not_mine covers only a foreign binding with no record, and this Worker's own expired, revoked or missing binding is a Final rejected (81-84, 156-166).

I walked both paths:

  • Lost response, then the binding expires, then a resend: answered from the ledger, or continued under the stored key, so landed parts replay.
  • A demoted collaborator: a new id is rejected; a known one is answered in its original room. Neither is re-routed.

@akag2's lost-worker-id major is fixed at bf1f6d9 (60-66, 133-134). I'd found the same independently: the key for one id and part changes with the worker id, so re-minting would re-post landed parts.

My earlier should-fixes and nits are all in:

  • sealed delivery root (95-109);
  • probe and un-echoed 200 (235-245);
  • listener-only declaration (28-38);
  • separate ag2/delivery with the owner's opt-out (51-56);
  • RFC 8785 keys;
  • keyed notice;
  • expires_at: null.

Should-fix

  • Section 4 says _ag2/deliver is guarded (202-206), but Consequences says deliveries are not checked by #40's guard (284-286). What actually holds is that collaborator Sessions get no binding without the opt-out. A delivery under the owner's binding is unchecked, and the agent picks the binding (111-112). So a listed collaborator's Turn can steer the agent into delivering under the owner's binding in a shared room. That is the reason #40 now checks every _ag2/notify while collaborators are listed: a queued message's Turn is unknown. Deliveries carry files, too. Please either:
    • apply #40's notify rule to every delivery while collaborators are listed and the opt-out is unset;
    • not enable delivery in that state;
    • or drop "are guarded" and state the gap.
  • Re-check the opt-out at delivery (51-56, 78-80). The collaborator list is re-checked; AGENT_CONNECT_UNCHECKED_COLLABORATORS is not. Collaborator bindings live up to 24 h, so an owner who unsets it and restarts still gets unchecked deliveries under old bindings. Reject a collaborator binding at delivery unless the opt-out is still set.
  • Keep a delivery on its binding, and tell agents (153, 162-163, 172-173, 260-262). Ledger-first means the original binding keeps working for a known id after it expires, so nothing is stranded if the agent resends exactly what it sent. But the contract only says "same id". A newer Turn hands the Session a new binding, and an agent that moves queued deliveries to it gets a Final conflict. After a Worker crash mid-delivery, nothing else resumes the remaining parts. Suggestions:
    • say the agent resends with the original binding;
    • put the recorded binding in conflict's data;
    • name the binding case in the conflict bullet.
      Also, 199-200 ("the same key makes a second attempt harmless anyway") no longer holds across Workers, since the key includes the worker id. not_mine is what protects a misdirected resend, and an id moved to another Worker's binding posts again.
  • Reconcile the probe section with the broker PR you linked (222-237). I read it. The capability response it implements differs from the ADR's in three places: the top-level key, the TTL field name and the route strings. So a Worker built to the ADR as written would never enable delivery. The key scope and which responses carry the echo also differ from 226-231. The exact names are in that PR; I've kept them out of this public thread.
  • Say what ends a running key whose holder died (147, 229-231). If the broker dies after claiming a key, or fails to record the outcome after a successful post, the key stays running. If it stays that way until the key expires (please check this against the broker PR), the Worker answers Retry for 23 hours, ends with expired, and posts a notice saying the part "could not be sent", even when it did post. Either the broker leases running and turns a stale record into unknown, or the ADR reports that case as unknown ("may not have been sent").

Nits

  • Spawned-agent bootstrap (28-32, 39-43). Since the Worker declares only on the listener, say how it learns that a spawned agent supports delivery (the agent declares in every initialize response?), and whether the first Turn's Session gets ag2/delivery.
  • After an un-echoed 200 (241-245): say what the remaining parts of that delivery get, and how the declaration is withdrawn on a live listener.
  • A 502, 504 or timeout from a keyed post should be Retry, never rejected. The broker's record answers the next attempt with the real state.
  • Retention (189-192, 81-93):
    • don't cap-evict a finished entry younger than 24 h;
    • let an in-progress entry older than 24 h become expired;
    • keep a binding's folder while an in-progress entry names it;
    • extend the never-re-mint check to bindings.json;
    • give the worker id a size.
  • name and mimeType (119) have no route: the relay client's upload takes neither.

@akag2 akag2 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.

Approving at bf1f6d9. The worker-id loss case is closed:

  • the id is stored in each ledger entry, and a resend reuses the stored id (lines 132-134, 178);
  • worker-id uses temp file, fsync, rename and a directory fsync (60-61);
  • a missing or mismatched id with a ledger present disables delivery instead of minting a new one (62-65).

The _ag2/notify wording now matches #40's post_unasked (202-206).

Nit: line 63 says a mismatch "does not enable delivery", but the planned test at line 330 says "an in-progress entry resumes under its stored worker id". Please say whether in-progress entries keep running while new deliveries are refused.

@marklysze
marklysze merged commit d754ee5 into main Nov 7, 2026
10 checks passed
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.

4 participants