From 6a7f8f91ad4c657483786a10a604b32b8503c1d4 Mon Sep 17 00:00:00 2001 From: "glm-5.2" Date: Sun, 21 Jun 2026 10:34:12 +0000 Subject: [PATCH] =?UTF-8?q?docs(architecture):=20resolve=20S1=20=E2=80=94?= =?UTF-8?q?=20abort=20policy=20on=20OperationContext,=20not=20wire?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADR-016 Decision 6 specifies that the abort policy (abort-dependents vs continue-running) is set on OperationContext and propagated through OperationEnv::invoke() — the composing handler decides the child's policy, not the wire caller. The call.requested payload does not carry an abort policy field. This resolves the TBD that was masquerading as a two-way door: two of the three options ADR-016 floated (wire payload, per-operation declaration) were inconsistent with the ADR's own assumptions. Also marks review #001 as resolved — all 5 critical, 4 warning, and 4 suggestion findings are now addressed. --- .../architecture/crates/call/call-protocol.md | 4 + .../016-abort-cascade-for-nested-calls.md | 61 ++++++++++++-- ...mplementation-architecture-sanity-check.md | 83 ++++++++++--------- 3 files changed, 104 insertions(+), 44 deletions(-) diff --git a/docs/architecture/crates/call/call-protocol.md b/docs/architecture/crates/call/call-protocol.md index 729cc08..211ec35 100644 --- a/docs/architecture/crates/call/call-protocol.md +++ b/docs/architecture/crates/call/call-protocol.md @@ -121,6 +121,8 @@ The `payload` of a `call.requested` event has this shape: - `input` — the operation input, matching the operation's `input_schema` (JSON Schema). Always a `serde_json::Value`. - `auth_token` — optional. If present, the `CallAdapter` resolves it via `IdentityProvider::resolve_from_token()` and the resulting `Identity` takes precedence over the connection-level identity for this request. See [Identity Resolution](#authcontext-and-identity-resolution) below. +The `call.requested` payload does **not** carry an abort policy field. The abort policy (`abort-dependents` vs `continue-running`, ADR-016) is set on `OperationContext` and propagated through `OperationEnv::invoke()` — the composing handler decides the child's policy, not the wire caller. See [Abort Cascade and Nested Calls](#abort-cascade-and-nested-calls) below. + **Leading-slash convention**: `operationId` on the wire always has a leading slash (`/fs/readFile`). `OperationSpec.name` in the registry and in `services/list` responses never has a leading slash (`fs/readFile`). `OperationSpec.path()` produces the wire form (`/fs/readFile`). This is a single rule applied consistently — do not mix the two forms. ### `call.error` Payload @@ -340,6 +342,8 @@ When `call.aborted` arrives for a parent request, the protocol cascades the abor An opt-in **`continue-running`** policy is available for cases where long-running work should survive a parent's abort (e.g., a subscription that should keep streaming). Under `continue-running`, descendants that have already started continue to completion; descendants that haven't started yet are aborted; no new descendants start. +The abort policy is set on `OperationContext` and propagated through `OperationEnv::invoke()` — the composing handler decides the child's policy, not the wire caller. The `call.requested` payload does not carry an abort policy field (the wire caller doesn't know the composition tree). The root context gets the default (`abort-dependents`); a handler can opt a child into `continue-running` at `invoke()` time. See ADR-016 Decision 6. + Handlers clean up resources when their call is cancelled (in Rust, the future is dropped and `Drop` guards release resources — HTTP streams, file handles, locks). This is a handler-level concern; the protocol's job is to cascade the abort. See ADR-016. ## Constraints diff --git a/docs/architecture/decisions/016-abort-cascade-for-nested-calls.md b/docs/architecture/decisions/016-abort-cascade-for-nested-calls.md index 37d2ac4..8c2d88a 100644 --- a/docs/architecture/decisions/016-abort-cascade-for-nested-calls.md +++ b/docs/architecture/decisions/016-abort-cascade-for-nested-calls.md @@ -84,9 +84,10 @@ Use cases for `continue-running`: a long-running subscription that should keep streaming after its parent's sibling failed; a background task that was spawned by a handler and should survive the handler's abort. -The caller or handler specifies the policy at call time. The specific mechanism -(a field in the `call.requested` payload, a field on `OperationContext`, or a -per-operation declaration) is a two-way door for implementation. +The caller or handler specifies the policy at call time. The policy is set +on the `OperationContext` and propagated to children via `OperationEnv::invoke()` +— see Decision 6 below. The default is `abort-dependents`; `continue-running` +is an opt-in for long-running work that should survive a parent's abort. ### 4. Cleanup hooks @@ -116,6 +117,52 @@ as a separate Rust crate for consumers that need richer call-tree visualization or reactive status tracking. It is not required for the protocol-level cascade — a parent-indexed map suffices. +### 6. The abort policy is set on `OperationContext`, not on the wire payload + +The abort policy (`abort-dependents` vs `continue-running`) is set on +`OperationContext` and propagated to children via `OperationEnv::invoke()`. +It is NOT a field in the `call.requested` wire payload, and it is NOT a +per-operation declaration on `OperationSpec`. + +**Why not the wire payload**: the wire caller doesn't know the composition +tree. The caller of `/agent/chat` cannot meaningfully decide whether +`/fs/readFile` (composed internally by the agent handler) should survive an +abort — the handler that composes the child knows that, not the wire caller. +Putting the policy on the wire payload would give the wire caller control +over internal composition behavior it can't see. + +**Why not per-operation declaration**: ADR-016 Assumption 5 says the policy +is per-call, not per-operation. The same operation may need +`abort-dependents` in one composition context and `continue-running` in +another. A static property on `OperationSpec` can't express that. + +**How it works on `OperationContext`**: the root context +(`build_root_context` in the CallAdapter) gets the default policy +(`abort-dependents`). When a handler composes a child via +`env.invoke()`, it can specify the policy for that child: + +```rust +// Default: abort-dependents (child aborts if parent aborts) +context.env.invoke("fs", "readFile", input, &context).await + +// Opt-in: continue-running (child survives parent's abort) +context.env.invoke_with_policy( + "fs", "readFile", input, &context, AbortPolicy::ContinueRunning +).await +``` + +The child's `OperationContext` carries the policy. If the child itself +composes grandchildren, the policy propagates unless the child explicitly +overrides it. This is consistent with the composition authority and scoped +env propagation in ADR-022 — the parent handler decides the child's +runtime context, including abort policy. + +The `OperationEnv` trait gains an optional policy parameter. The specific +API shape (a separate `invoke_with_policy` method, a policy field on an +`InvokeOptions` struct, or a builder pattern) is a two-way door for +implementation — but the policy enters through `OperationEnv::invoke()`, +not through the wire and not through `OperationSpec`. + ## Consequences **Positive:** @@ -175,10 +222,10 @@ or reactive status tracking. It is not required for the protocol-level cascade release, lock release). 5. **`continue-running` is per-call, not per-operation.** The policy is - specified at call time, not declared at registration. If the policy should - be a static property of the operation (declared in `OperationSpec`), the - model changes. The assumption is that the caller or handler decides at call - time based on the specific context. + specified at call time via `OperationEnv::invoke()`, not declared at + registration on `OperationSpec` and not set by the wire caller. The + composing handler decides the child's policy based on the specific + context. See Decision 6. ## References diff --git a/docs/reviews/001-pre-implementation-architecture-sanity-check.md b/docs/reviews/001-pre-implementation-architecture-sanity-check.md index 629c14f..eb839f2 100644 --- a/docs/reviews/001-pre-implementation-architecture-sanity-check.md +++ b/docs/reviews/001-pre-implementation-architecture-sanity-check.md @@ -1,5 +1,5 @@ --- -status: draft +status: resolved last_updated: 2026-06-20 reviewed_documents: - overview.md @@ -443,37 +443,33 @@ subsumed by the version-based one. ## Suggestions -### S1. Abort Policy Field Absent from `call.requested` Payload +### S1. Abort Policy Field Absent from `call.requested` Payload (RESOLVED) **Documents**: call-protocol.md (lines 112–118, call.requested payload), ADR-016 (lines 86–89, 177–181) -**Suggestion**: ADR-016 says the `abort-dependents` vs `continue-running` policy -is per-call and "the specific mechanism (a field in the `call.requested` -payload, a field on `OperationContext`, or a per-operation declaration) is a -two-way door for implementation." call-protocol.md's `call.requested` payload -(L112–118) shows `operationId`, `input`, `auth_token` — no policy field. Since -ADR-016 explicitly leaves the mechanism open, this isn't an inconsistency, but -a one-line note in call-protocol.md ("the abort policy is conveyed via [TBD — -two-way door, ADR-016]") would prevent an implementer from assuming the payload -is final and forgetting the policy exists. +**Resolution**: ADR-016 Decision 6 (added during this review's resolution +pass) specifies that the abort policy is set on `OperationContext` and +propagated through `OperationEnv::invoke()` — not on the wire payload, not +on `OperationSpec`. The composing handler decides the child's policy. The +`call.requested` payload explicitly does not carry an abort policy field. +call-protocol.md updated to state this. ADR-016 Assumption 5 updated to be +definitive. No longer a TBD. --- -### S2. `build_root_context` "CallAdapter's Own Capabilities (If Any)" Hedge +### S2. `build_root_context` "CallAdapter's Own Capabilities (If Any)" Hedge (RESOLVED) **Documents**: call-protocol.md (line 274) -**Suggestion**: The "(if any)" hedge reflects the C3 ambiguity. Once C3/C4 is -resolved, this comment should be made definitive — either "the handler's -capabilities, looked up from the registry by operation name" (if option (a)) -or removed (if capabilities are closure-captured only). Leaving it hedged -propagates the ambiguity into the one place an implementer looks for "how does -the root context get built." +**Resolution**: Resolved by ADR-022. The hedge is gone — +`build_root_context` now reads `registration.capabilities.clone()` from the +`HandlerRegistration` bundle looked up by operation name. The comment is +definitive. --- -### S3. Example Style: `Arc::new(handler)` Inline vs. Pre-wrapped +### S3. Example Style: `Arc::new(handler)` Inline vs. Pre-wrapped (RESOLVED) **Documents**: operation-registry.md (lines 149, 264) @@ -482,40 +478,53 @@ L264 uses `.with(spec, agent_handler)` where `agent_handler` was already `Arc::new`'d at L253. Both are valid; pick one style in the examples to avoid a moment of "wait, does `with` take an `Arc` or a `Handler`?" confusion. Trivial. +**Resolution**: Resolved by ADR-022. The registration API changed to +`with(HandlerRegistration)` / `with_local(...)` / `with_leaf(...)`, so the +inline `Arc::new` vs pre-wrapped ambiguity no longer exists — the builder +methods take the handler as part of the registration bundle. + --- ### S4. `last_updated` Dates Scattered; README Index Older Than Sub-docs **Documents**: frontmatter across all docs -**Suggestion**: The README index date (2026-06-19) is older than several +**Suggestion**: The README index date (2026-06-19) was older than several sub-documents (core/README 06-21, call/operation-registry 06-22, etc.). If frontmatter dates are meant to mean something, bump README/overview when sub-docs change. Not worth a cycle on its own. +**Resolution**: README and overview dates bumped during the ADR-022/023 +resolution pass. Ongoing date maintenance is cosmetic and can be addressed +opportunistically. + --- ## Summary Statistics | Severity | Count | Status | |----------|-------|--------| -| Critical | 5 | Needs resolution before implementation | -| Warning | 4 | Should resolve — mostly quick spec clarifications | -| Suggestion | 4 | Consider for clarity | +| Critical | 5 | All resolved (C1–C4 via ADR-022, C5 via ADR-023) | +| Warning | 4 | All resolved (W1–W4 fixed in spec edits) | +| Suggestion | 4 | All resolved (S1 via ADR-016 Decision 6, S2 via ADR-022, S3 via ADR-022, S4 during ADR-022/023 pass) | ## Recommended Resolution Order -1. **Critical findings C1–C4** — These are one tangle (the registration - bundle). Resolve together via ADR-022 (Handler Registration and Dispatch - Context) before any call-crate implementation begins. C1, C2, C3 are kept as - separate findings for traceability (each cites specific lines) but C4 - synthesizes them: the registration bundle carries handler identity, scoped - env, and capabilities, and the dispatch path reads from it. This is the - highest-value work before implementation. -2. **Critical finding C5** — Error schemas. Resolve via ADR-023 (Operation - Error Schemas) before implementing `from_openapi`/`to_openapi`, otherwise - the adapter contract in ADR-017 can't be faithfully implemented. -3. **Warning findings W1–W4** — Small doc edits. W1 (stale ADR-020 path) and W4 - (missing service.md method) should go in the same pass as the ADR-022 spec - updates since they touch the same files. W2 and W3 are one-line fixes. -4. **Suggestions S1–S4** — Address opportunistically during implementation. \ No newline at end of file +All findings resolved. Resolution summary: + +1. **C1–C4** (registration bundle tangle) → ADR-022 (Handler Registration, + Provenance, and Composition Authority). Registration bundle carries + provenance, composition authority, scoped env, capabilities. Dispatch + path reads from bundle. +2. **C5** (error schemas) → ADR-023 (Operation Error Schemas). + `error_schemas` on `OperationSpec`, `details` on `call.error`, adapter + fidelity for `from_openapi`/`to_openapi`. +3. **W1–W4** (spec drift/missing methods) → Fixed in spec edits during the + ADR-022/023 resolution pass. +4. **S1–S4** → S1 resolved by ADR-016 Decision 6 (abort policy on + `OperationContext`, not wire payload). S2, S3 resolved by ADR-022 (API + surface change eliminated the hedges/ambiguities). S4 resolved during + the ADR-022/023 pass (dates bumped). + +Review #001 is complete. All 5 critical, 4 warning, and 4 suggestion +findings resolved. \ No newline at end of file