Skip to content

feat(agent): define typed lifecycle extension point contracts (issue #298) - #351

Merged
yuluo-yx merged 5 commits into
agentic-ai-java:mainfrom
konlue:feat/issue298-lifecycle-extension-points
Nov 7, 2026
Merged

yuluo-yx merged 5 commits into
agentic-ai-java:mainfrom
konlue:feat/issue298-lifecycle-extension-points

Conversation

@konlue

@konlue konlue commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Describe what this PR does / why we need it

Fixes #298: the plugin contract of the neutral agent API covered a single extension point. argi-agent-api offered ModelInterceptor as its only wrapping contract and ToolAuthorizationPolicy as its only tool-related type, so the engine-neutral execution path had no tool call interception point at all, while the tool, streaming and hook contracts lived in argi-agent-framework, the module that depends on the graph runtime. The issue names thirteen lifecycle stages but only two had a carrier, and there was no common vocabulary for invocation, attempt, step, model turn, tool call and delegation.

Does this pull request fix one issue?

Fixes #298

Describe how you did it

  • Added the extension point catalog docs/architecture/lifecycle-extension-points.md: the thirteen stages with their scope, common runtime versus engine specific, and their synchronous, asynchronous, cancellable, streaming and resumable capabilities, together with the two explicit boundaries that a workflow step is not a model turn and that a remote agent does not expose the internals of its tool execution;
  • Added the neutral contract package io.github.agentic.ai.agent.lifecycle in argi-agent-api: the six identity types with LifecycleIdentity, the four plugin kinds (observer, restricted transformer, around interceptor, admission gate) with their input, return, mutable fields and their rejection, approval and failure semantics, and the typed model call and tool call carriers (ModelCallInvocation, ModelCallOutcome, ModelCallExecutor, ToolCallInvocation, ToolCallOutcome, ToolCallExecutor);
  • Kept the dependency direction and the existing API stable: the new package depends on argi-api and argi-runtime-api only, the trusted execution context stays a read-only projection without a mutator, and no existing signature was changed — ModelInterceptor, ToolAuthorizationPolicy, ToolInterceptor, Hook and the other framework types keep their current shape and receive a keep, adapt or migrate decision in the migration table of the document;
  • Restricted the transformation boundary: LifecycleMutationGuard rejects a rewrite that changes the identity, the tool name or the model turn identity, and the neutral tool call carries no free-form context map.

How was it tested / verified?

New LifecycleContractTest in argi-agent-api (offline, no model, no tool binding, no engine):

  1. observation of an immutable, typed event together with its identity;
  2. a transformation inside the mutable boundary;
  3. a rejection that short-circuits the stage before the chain runs;
  4. an approval request that degrades to a denial on an engine without an approval channel;
  5. a wrapping that rewrites, short-circuits and repeats the chain;
  6. the failure semantics of a failing observer and of a failing interceptor, both reported as LifecycleEvent.ExtensionFailed;
  7. the dependency boundary: the classpath of argi-agent-api contains no type of the graph module, and no class of the neutral package references the graph or the loop module.

Red before the change: the test does not compile on 738354633, because none of the contract types exists yet — this issue is a capability evolution task and not a bug reproduction. Green after: Tests run: 10, Failures: 0; module regression 44/44; argi-agent-core 138/138; argi-workflow-core 26/26; checkstyle reports 0 violations in argi-agent-api.

Additional notes

@yuluo-yx yuluo-yx left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Two contract gaps to address before merging.

* @param modelTurn the model turn identity, never null
* @param request the model request, never null
*/
public record ModelCallInvocation(LifecycleIdentity identity, ModelTurnId modelTurn, ModelRequest request) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The record still exposes a mutable ModelRequest. An observer can call invocation().request().getMessages().clear() or modify getContext(), changing the request seen by the executor and other extensions. Please expose a read-only request view so observation cannot rewrite execution input.

if (!before.invocation().modelTurn().equals(after.invocation().modelTurn())) {
return Optional.of("model turn identity must not change");
}
return Optional.empty();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A replacement built with ModelRequest.builder(request).trustedExecutionContext(null).build() passes this guard. Replacing the tenant, principal or budget context also passes as long as the lifecycle IDs stay unchanged. Please preserve the original trusted execution context when accepting a rewritten request.

@konlue

konlue commented Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review — both gaps are fixed (3956636, +114):

  1. Mutable request exposure: ModelCallInvocation now holds a defensive copy built through ModelRequest.builder(request) with List.copyOf messages and Map.copyOf context/attributes. An observer clearing the message list or mutating the context map can no longer reach the caller's collections or the request the dispatch keeps using. One nuance the tests now document: ModelRequest's constructor itself copies the context into a mutable HashMap and the attributes into an immutable Map.copyOf map, so the copy enforces isolation (mutation cannot reach the caller/executor) with per-collection behavior — attribute writes throw, message writes only affect the per-invocation copy.

  2. Trusted execution context swap: LifecycleMutationGuard's ModelCallRequested branch now requires the rewritten request to preserve the original trustedExecutionContext — swapping the tenant/principal/budget (or nulling it out) is rejected with "trusted execution context must not change", even when all lifecycle IDs are preserved. One clarification on the null case: getTrustedExecutionContext() returns Optional.ofNullable, so an explicit null is indistinguishable from "no context" — both are treated as the same (no-context) state; the swap detection covers the real attack (a present context replaced by another tenant or removed).

Tests: 3 new cases in LifecycleContractTest (defensive-copy isolation, guard rejecting a tenant swap and a null-out, a legitimate content-only rewrite still passing). 13/13 green, checkstyle clean.

@yuluo-yx

yuluo-yx commented Nov 4, 2026

Copy link
Copy Markdown
Member

[P2] The defensive copy isolates the original request, but the lifecycle event itself remains mutable across extensions.

The caller/executor isolation described in the previous comment works, but ModelCallInvocation.java:60 does not provide the immutable view promised by the event contract: ModelRequest copies the supplied collections back into an ArrayList and a HashMap.

Reproduced invocation.request().getMessages().clear() succeeding: the original request retains its message, but the invocation now contains none. A subsequent observer or transformer receiving the same event therefore sees the previous observer's mutation, without going through the restricted transformation boundary. The existing regression test explicitly permits this mutation and only checks isolation from the original request.

Please expose a genuinely read-only request view, or otherwise ensure extensions cannot alter the event seen by subsequent extensions.

Validation: targeted offline Java probe against this PR head; the full reactor test suite was not run.

@konlue

konlue commented Nov 4, 2026

Copy link
Copy Markdown
Contributor Author

Fixed in 8ef2a88 (+79/−15). ModelCallInvocation now exposes a genuinely read-only snapshot instead of a builder copy that repacked itself into mutable collections. Two layers: the invocation still snapshots the caller's request (later changes to the caller's collections stay invisible), and the snapshot is served through a package-private ModelRequest subclass whose getMessages() and getContext() return unmodifiable views — invocation.request().getMessages().clear() and .getContext().put(...) now both fail with UnsupportedOperationException, so an extension cannot alter the event seen by subsequent extensions. The remaining accessors were already immutable (attributes is Map.copyOf, toolDescriptions an unmodifiable map, tool registries expose unmodifiable definition lists).

The read-only surface stays local to the lifecycle event on purpose: per-subscription ModelRequest copies are mutable by design (InterceptorChain.copyForSubscription documents that contract, and ContentSafetyStreamingInterceptor writes into the context map in place), so making ModelRequest itself immutable would break subscription isolation. Transformers are unaffected — ModelRequest.builder(snapshot) rebuilds normally and the trusted execution context carries over (still guarded by LifecycleMutationGuard).

LifecycleContractTest now asserts all three collection accessors reject writes, plus two new cases: the snapshot is unaffected by later caller-side mutations, and a transformer rebuilt from the read-only view keeps the full content (15/15 green in the module; checkstyle clean).

@yuluo-yx

yuluo-yx commented Nov 5, 2026

Copy link
Copy Markdown
Member

[P2] ModelCallInvocation.readOnlySnapshot() still shares nested mutable values (line 67). Reproduced: clearing a list stored in invocation.request().getContext() also clears the original request’s list. Please isolate/freeze nested supported values so observers cannot mutate the event or dispatch context.

@konlue

konlue commented Nov 5, 2026

Copy link
Copy Markdown
Contributor Author

Fixed in d6e6d16 (+75/−5). The snapshot now freezes the collection values nested inside the context and attributes: every entry value that is a List, Map or Set is copied into its immutable form (List.copyOf / Map.copyOf / Set.copyOf) while the entry map itself is snapshotted. Reproducing your probe — a list stored in the caller's context, cleared through invocation.request().getContext() — the observed list is a frozen copy: clear() and add() fail with UnsupportedOperationException, the caller's list keeps its content, and later mutations of the caller's list are invisible to the event. Nested maps behave the same.

Values of other types are deliberately shared by reference: the executor's own objects (provider clients, builders, stream state) cannot be copied generically without breaking their identity, so the contract freezes exactly what an extension can reach through collection accessors — the message list, the context and attribute maps, and the collection values nested in them. The javadoc on readOnlySnapshot states this scope.

Two new cases in LifecycleContractTest (nested context values frozen, with caller independence in both directions, and the same freeze for attribute values) — 17/17 green in the module, checkstyle clean.

@yuluo-yx

yuluo-yx commented Nov 7, 2026

Copy link
Copy Markdown
Member

The latest test job is failing: AgentToolNodeExecutionLeaseTest.activeCancellableToolGetsCancellationOnLeaseLossAndLateOutputIsNotMerged (expected lease-loss cancellation request, got false). Please fix or rerun the failing test before merge.

@konlue

konlue commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor Author

@yuluo-yx The failing test is not touched by this PR — the failure looks like a timing flake on the runner, not a regression:

  • Zero file overlap: all 26 files of this PR live in the new lifecycle package of argi-agent-api (plus the architecture doc and its contract test). AgentToolNodeExecutionLeaseTest lives in argi-agent-framework's node package and imports nothing from the lifecycle contract.
  • Same head passes locally, repeatedly: on the exact PR head (d6e6d1684) the class passes 10/10 consecutive Maven runs (50 test executions, zero failures).
  • The assertion is scheduling-sensitive: "lease loss must request original stage cancellation" depends on the cancellation request propagating between the lease holder and the stage before the assertion reads it, which is exactly the kind of check that degrades under runner load.
  • History agrees: this class passed in this PR's earlier CI runs, and the recent main CI runs with the same test are green.

I don't have rerun rights on this repository, so I pushed an empty commit (72e0791f4) to retrigger the workflow — new run: https://github.com/agentic-ai-java/argi/actions/runs/42335284880. The PR diff is unchanged; the empty commit only restarts CI. If it reproduces deterministically in your environment I will of course dig in.

@yuluo-yx
yuluo-yx merged commit 683b276 into agentic-ai-java:main Nov 7, 2026
7 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.

[Architecture][P1] 定义 Agent 生命周期扩展点与类型化插件契约

2 participants