Skip to content

feat(agent): add task completion hook with bounded exits and explicit terminal states (T36) - #172

Merged
yuluo-yx merged 2 commits into
agentic-ai-java:mainfrom
XianReallyHot-ZZH:feat/issue-139-task-completion
Oct 19, 2026
Merged

yuluo-yx merged 2 commits into
agentic-ai-java:mainfrom
XianReallyHot-ZZH:feat/issue-139-task-completion

Conversation

@XianReallyHot-ZZH

Copy link
Copy Markdown
Contributor

确认 (Acknowledged) contribution to ARGI: add the opt-in TaskCompletionHook completion predicate with bounded nudge/deadline exits and explicit NEED_INPUT/STOP terminal states as the second T36 slice for #139.

… terminal states (T36)

Introduce the opt-in TaskCompletionHook: a run whose completion predicate
(default: unfinished todos) reports unfinished work no longer ends as a
plain success just because the model produced no tool call. The hook
appends a bounded nudge re-ask (maxAttempts, optional deadline mirroring
the T04 retry vocabulary) and terminates with an explicit terminal
ActionDecision under argi.actionDecision.final: NEED_INPUT by default on
exhaustion, STOP for identical repeated decisions and for an optional
unknown-tool bound. New reason codes: UNFINISHED_REQUIRED_TASKS,
NO_PROGRESS, REPEATED_DECISION, UNKNOWN_TOOL. All counters stay
run-scoped in the run config context, a stale terminal decision is
cleared on a fresh run, and default routing is unchanged when the hook
is absent. Model-agnostic tests cover nudge, exhaustion, deadline, both
exit behaviors, stream/non-stream consistency and routing regressions.
Slice 2 for agentic-ai-java#139.

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approved.

Clean, well-engineered opt-in hook for the T36 task-completion work. The routing logic is sound and the tests pin every branch:

  • beforeModel establishes the message baseline once per run and clears any stale terminal decision (verified by the same-thread recovery test); the unknown-tool bound is enforced here by counting unresolved tool messages added since the baseline.
  • afterModel implements the bounded nudge loop: it applies the nudge budget (maxAttempts) and optional deadline first, classifies NO_PROGRESS vs UNFINISHED_REQUIRED_TASKS by whether the answers were identical, then the independent REPEATED_DECISION bound (maxConsecutiveRepeats) on identical consecutive fingerprints, and finally resets the nudge counter on any productive tool call. I traced all four termination paths against the offline/deadline/stream tests and they line up.
  • All counters live in config.context() and never enter the checkpointed state (asserted by runScopedCountersNeverEnterTheCheckpointedState); the only declared state key is the terminal decision with REPLACE strategy.
  • ActionReasonCodes gains the four new bounded-exit labels with a stability test asserting 7 unique values; the new NO_PROGRESS / REPEATED_DECISION / UNKNOWN_TOOL / UNFINISHED_REQUIRED_TASKS reasons are covered.

Non-blocking nits (no action needed to merge):

  1. Test package split is inconsistent: TaskCompletionHookTest and TaskCompletionHookDeadlineTest live in hook.taskcompletion (singular) while TaskCompletionHookOfflineTest and TaskCompletionHookStreamConsistencyOfflineTest live in hooks.taskcompletion (plural). Harmless since they only touch public API, but worth aligning.
  2. decisionFingerprint uses message.text() for final answers; two null-text assistant turns would compare as identical and trip the repeat bound. Unlikely in practice (assistant turns carry text), but a defensive null check would harden it.

All CI jobs (build, test, check-style, format, license, linter, secret-check) pass. Good to merge.

Document why TaskCompletionHookDeadlineTest deliberately lives next to
the main class (package-private nanoTime seam, mirroring the
EffectAwareToolRetryInterceptorTest layout) and defensively normalize a
null text in decisionFingerprint so a null can never collide with an
answer that literally reads "null". Two new tests pin the behavior:
identical textless assistant turns still count as repeats, while a
literal "null" answer does not collide with a textless turn.
Follow-up to the review on agentic-ai-java#172 for agentic-ai-java#139.
@XianReallyHot-ZZH

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review! Addressed both nits in a72feac:

  1. Test package layout — TaskCompletionHookTest already lives in hooks.taskcompletion with the other public-API tests; the only split is TaskCompletionHookDeadlineTest, which deliberately stays next to the main class because it exercises the package-private nanoTime seam — the same layout as EffectAwareToolRetryInterceptorTest. Its javadoc now documents this so the split reads as intentional.

  2. decisionFingerprint null hardening — now normalizes with Objects.requireNonNullElse. For the record, ChatMessage.text() joins content parts and cannot return null today (Collectors.joining contract), so the collision was theoretical; the normalization closes it regardless. Two new tests pin the semantics either way: identical textless assistant turns still count as repeats (textlessAssistantAnswersStillCountAsRepeatsWhenIdentical), while a literal "null" answer does not collide with a textless turn (textlessAnswerDoesNotCollideWithALiteralNullAnswer).

All 29 task-completion tests green; full module suite unchanged (only the Windows-local symlink cases that also fail on clean main).

@yuluo-yx
yuluo-yx merged commit 2b411a4 into agentic-ai-java:main Oct 19, 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.

3 participants