Repository navigation
feat(agent): define typed lifecycle extension point contracts (issue #298) - #351
Conversation
yuluo-yx
left a comment
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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.
…execution context (issue agentic-ai-java#298 review)
|
Thanks for the review — both gaps are fixed (3956636, +114):
Tests: 3 new cases in |
|
[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 Reproduced 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. |
…ll invocations (issue agentic-ai-java#298 review)
|
Fixed in 8ef2a88 (+79/−15). The read-only surface stays local to the lifecycle event on purpose: per-subscription
|
|
[P2] |
… attributes (issue agentic-ai-java#298 review)
|
Fixed in d6e6d16 (+75/−5). The snapshot now freezes the collection values nested inside the context and attributes: every entry value that is a 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 Two new cases in |
|
The latest test job is failing: |
|
@yuluo-yx The failing test is not touched by this PR — the failure looks like a timing flake on the runner, not a regression:
I don't have rerun rights on this repository, so I pushed an empty commit ( |
…deExecutionLeaseTest, see review comment)
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-apiofferedModelInterceptoras its only wrapping contract andToolAuthorizationPolicyas 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 inargi-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
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;io.github.agentic.ai.agent.lifecycleinargi-agent-api: the six identity types withLifecycleIdentity, 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);argi-apiandargi-runtime-apionly, the trusted execution context stays a read-only projection without a mutator, and no existing signature was changed —ModelInterceptor,ToolAuthorizationPolicy,ToolInterceptor,Hookand the other framework types keep their current shape and receive a keep, adapt or migrate decision in the migration table of the document;LifecycleMutationGuardrejects 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
LifecycleContractTestinargi-agent-api(offline, no model, no tool binding, no engine):LifecycleEvent.ExtensionFailed;argi-agent-apicontains 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 regression44/44;argi-agent-core138/138;argi-workflow-core26/26; checkstyle reports 0 violations inargi-agent-api.Additional notes