Skip to content

feat(hitl): Human-in-the-Loop pause/resume framework - #585

Merged
ginccc merged 142 commits into
mainfrom
feat/hitl-framework
Jul 13, 2026
Merged

feat(hitl): Human-in-the-Loop pause/resume framework#585
ginccc merged 142 commits into
mainfrom
feat/hitl-framework

Conversation

@ginccc

@ginccc ginccc commented Jul 2, 2026

Copy link
Copy Markdown
Member

Summary

This PR delivers a ** Human-in-the-Loop (HITL) framework**: config-driven pause → approve/reject → resume for EDDI conversations, so an agent designer can require a human to review sensitive actions before they take effect. It is built as two complementary approval gates layered onto the existing lifecycle pipeline, sharing one durable pause/resume core, and comes with the persistence, multi-surface approver access, audit trail, and observability needed to run it in production.

Two gates, one machinery:

  1. Rule-level pause — a behavior rule emits the reserved PAUSE_CONVERSATION action; the conversation moves to AWAITING_HUMAN and the whole turn is held until a human resolves it via POST /agents/{conversationId}/resume.
  2. Tool-level approval gatinghitlConfig.toolApprovals gates individual LLM tool calls. When the model invokes a tool matching a requireApproval glob (across all 8 tool sources), the conversation pauses before the tool runs (hitlPauseType: TOOL_CALL); the reviewer can approve / reject / amend arguments per call, and a write-ahead journal guarantees at-most-once execution across pod crashes and re-approvals.

Everything is agent-designer-configurable — no HITL behavior is hardcoded in Java. The engine only reads and executes the JSON config (agent-level hitlConfig plus a per-task toolApprovals override).

Beyond the feature, the branch was hardened through two rounds of adversarial multi-agent review (a whole-branch pass and a review of the fixes themselves); every confirmed finding is fixed. It ships 77 new test files and lifts branch coverage back over the project's OpenSSF-Gold JaCoCo gate.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📝 Documentation update (adds docs/hitl.md + changelog)
  • ♻️ Refactoring (no functional changes)
  • 🔧 Chore (dependency updates, CI changes, etc.)

Backward-compatible: agents without hitlConfig are byte-identical to the pre-HITL path; legacy persisted snapshots and old JSON/ZIP configs load and resume unchanged (verified by the review's backward-compat dimension, which came back clean).

Related Issue

Closes #585

Changes Made

Approval engine

  • ToolApprovalGate / ToolApprovalPatterns — split an LLM tool-call batch into gated vs allowed by glob patterns over source:name (exempt beats require; fail-safe on unknown source).
  • AgentOrchestrator — the tool-calling loop pauses on a gated call, freezes a PendingToolCallBatch (redacted + size-capped args, capped chat transcript, effective config), and on resume re-enters the same step, applies per-call verdicts, replays the transcript, and continues the model loop.
  • LlmTask — same-index re-entry (executeResume), task-override-wins effective-config resolution, eddi.hitl.tool.transcript-max-bytes wiring; CascadingModelExecutor threads identical gating.
  • ConversationService / Conversation — the pause/resume state machine: durable AWAITING_HUMAN, CAS-guarded commits (no resurrection of terminated conversations), timeout scheduling, cancellation, and EXECUTION_INTERRUPTED recovery.

Durability & recovery

  • IHitlToolJournalStore write-ahead journal (claim → execute → record) for at-most-once tool execution — Mongo and PostgreSQL implementations, selected via DataStoreProducers.
  • HitlCrashRecoveryObserver — reconciles/re-arms stuck pauses on startup; retention + GDPR-erasure cascade cover the journal.
  • Timeout policies (AUTO_APPROVE / AUTO_REJECT / WAIT_INDEFINITELY) via ScheduleFireExecutor, with no-progress and per-turn pause-cap guards; audit-ledger entries for every decision (EU AI Act).

Config & approver surfaces

  • hitlConfig on AgentConfiguration + per-task toolApprovals; HitlConfigValidation, reserved-action + inert-config save-time lints.
  • MCP approver surface: McpHitlTools (9 list/status/resume/cancel tools for regular + group), HitlAccessGuard (owner / eddi-admin / eddi-approver scoping, server-side attribution), eddi.mcp.hitl.mutations.enabled kill-switch.
  • Slack approval cards + resume-outcome delivery; delegated + group-conversation parity.
  • Published reference docs/hitl.md; running changelog updated.

Quality & security hardening (this branch's review work)

  • Two adversarial multi-agent reviews; all confirmed findings fixed — incl. the init-turn gate bypass (fail-open), an EXECUTION_INTERRUPTED input-lock regression, task-scoped timeout resolution, cancel/undo races, a raw-conversation-read leak of tool args + transcript, and the missing Postgres journal store.
  • Security: names-only projection on generic read surfaces, CodeQL log-injection sanitization, MCP error-code correctness.
  • ~560 new focused unit tests targeting previously-uncovered HITL branches to clear the JaCoCo branch gate.

How to Test

  1. Deploy an agent with either a PAUSE_CONVERSATION behavior rule or hitlConfig.toolApprovals.requireApproval matching a side-effecting tool (e.g. "transfer_*", "mcp:*").
  2. Start a conversation and trigger the gated tool / rule → the turn returns conversationState: AWAITING_HUMAN with pauseDetails (redacted args, gate reason, callIds).
  3. Discover it via GET /agents/{env}/pending-approvals or the MCP list_pending_approvals tool; inspect via GET /agents/{id}/approval-status.
  4. Resolve it via POST /agents/{conversationId}/resume (with per-call toolDecisions for a TOOL_CALL pause), or MCP resume_conversation, or the Slack approval card → the approved tool executes exactly once and the turn completes with the final answer.
  5. Verify at-most-once + recovery: kill the node mid-tool and re-approve → the result is replayed (or an honest "outcome unknown"), never re-executed.
  6. Backend: ./mvnw test — HITL unit suites green; Testcontainers ITs (HitlToolPauseResumeIT, Mongo/Postgres journal round-trips) run in CI.

Checklist

  • My code follows the project's code style
  • I have added tests that prove my fix/feature works (77 new test files; HITL unit + integration suites)
  • Existing tests pass locally (./mvnw clean verify -DskipITs) — unit green; @QuarkusTest ITs are CI-verified (loopback-restricted on the dev machine)
  • I have updated documentation if needed (docs/hitl.md, docs/changelog.md)
  • My commit messages follow conventional commits
  • I have not committed any secrets, API keys, or tokens
  • This PR has a clear, focused scope (one concern per PR) — this is a large Phase-9b feature branch; recommend Squash and merge to land it as a single, coherent commit on main.

ginccc added 30 commits July 1, 2026 00:21
Wave 0: storeIfFieldEquals CAS, ControlSignal/DiscussionControlToken,
        ConversationPauseException, cancel infrastructure
Wave 1: ConversationState.AWAITING_HUMAN, HITL bookmark fields,
        HitlDecision/HitlTimeoutPolicy, LifecycleManager extensions,
        Conversation.resume()
Wave 2: REST endpoints (resume/cancel/approval-status/pending-approvals),
        IConversationMemoryStore.compareAndSetState, timeout guard
Wave 3: SharedTaskList HITL methods, GroupConversation HITL fields,
        GroupConversationService cancel/resume, group REST endpoints
Wave 4: AgentConfiguration.HitlConfig, AgentGroupConfiguration.HitlConfig,
        HitlTimeoutHandler, ScheduleFireExecutor integration
- HitlTimeoutHandler: wrap HitlDecision in GroupApprovalRequest for resumeGroup (type mismatch)
- HitlTimeoutHandler: add null guard on metadata 'policy' key
- ConversationService.cancelConversation: only update cache when CAS succeeds (cache corruption fix)
- ConversationService.resumeConversation: add error handling to resume callable (state recovery)
- GroupConversationService.resumeDiscussion: save pausedAtPhaseIndex before clearing, skip completed phases on resume (was replaying from phase 0)
Coverage across all HITL components:
- Model classes: ControlSignal, DiscussionControlToken, ConversationPauseException,
  HitlDecision, HitlTimeoutPolicy, PendingApprovalSummary, GroupApprovalRequest
- SharedTaskList HITL methods: submitForApproval, approveTask, rejectTask,
  resetToAssigned, hasAwaitingApproval, full lifecycle paths
- ConversationMemory HITL bookmark fields and round-trip conversion
- LifecycleManager: cancel check, PAUSE_CONVERSATION detection, executeLifecycleFromIndex
- Conversation: AWAITING_HUMAN blocking, pause/resume flow, re-pause, rejection
- HitlTimeoutHandler: all policies (AUTO_APPROVE/REJECT, ABORT, WAIT_INDEFINITELY)
- ConversationService: cancelConversation CAS, listPendingApprovals
- RestAgentEngine: cancel and listPendingApprovals endpoints
- GroupConversation, EventSink, ConversationState, Snapshot HITL fields
- AgentConfiguration and AgentGroupConfiguration HitlConfig defaults
…nership, CAS, timeout scheduling

B1: Set in-memory state to AWAITING_HUMAN (not IN_PROGRESS) so
    Conversation.resume() guard passes.
B2: Wire phase.requiresApproval() + task submitForApproval() +
    hasAwaitingApproval() scan. Add commitPause() helper. Guard
    COMPLETED and finally blocks against AWAITING_APPROVAL.
B3+M2: Add ownership validation to 4 group REST endpoints.
    Set decidedBy server-side from SecurityIdentity.
B4: Use updateIfState(gc, AWAITING_APPROVAL) for atomic CAS.
M1: Inject IScheduleStore, create one-shot timeout schedule on pause.
M3: Seed turnCounter from pausedTurnCount, don't reset on resume.
Minor: Bookmark mismatch guard, SSE leak fix, subList→startPhaseIndex.
ConversationServiceResumeTest (11): B1 resume state, rejected resume,
    double-resume CAS, M1 timeout schedule creation, bookmark mismatch.
GroupConversationServiceHitlTest (9): Phase-level pause, task-level
    submitForApproval, resume phase index, double-resume CAS, M3 turn
    budget seeding, finally guard, rejection, wrong-state resume.
RestGroupConversationHitlTest (12): Ownership denied/allowed/admin,
    decidedBy server-side enforcement, 409 conflict on CAS failure.
Update 6 existing tests for new ConversationService constructor params.
BLOCKER: resumeDiscussion reads hitlPauseType before clearing — TASK
resumes at same phase (idempotent), PHASE at +1.

MAJOR-1: PHASE/TASK HITL gates are now mutually exclusive.
MAJOR-2: commitPause creates group timeout schedule via IScheduleStore.
MAJOR-3: deleteSchedulesByName added to IScheduleStore (Mongo+Postgres);
called on resume and cancel for both regular and group surfaces.
MAJOR-4: REJECTED branch uses updateIfState CAS.
MAJOR-5: activeTokens populated in executeDiscussion, cleaned in finally.
MAJOR-6: listPendingApprovals filtered by caller ownership in REST layer.
MINOR-1: metrics/events only fire on fresh discussion (startPhaseIndex==0).
MINOR-2: requireOwnerOrAdminStrict for fail-closed on null-owner state ops.
…TASK path

NEW-1 BLOCKER: submitForApproval gate now requires taskLevelHitl AND
phase.requiresApproval(). Without both, completeTask() is called.
Fixes TASK_FORCE preset stranding tasks in AWAITING_APPROVAL.

NEW-2 BLOCKER: activeTokens.remove() is now unconditional in the
finally block. Paused GCs have no running thread — lingering tokens
caused cancelDiscussion to take the no-op signal branch.

NEW-3 MAJOR: token.shouldStop() safe-points in phase loop and wave
loop. setActiveFuture() on wave allOf for IMMEDIATE cancel interrupt.

AUTO_APPROVE MAJOR: When TASK granularity + APPROVED + null
taskApprovals, resumeDiscussion auto-approves all AWAITING_APPROVAL
tasks. Fixes infinite reschedule from timeout handler.

Also fixed enum comparison bug (String vs HitlPauseType enum).
4 regression tests added (NEW-1, NEW-2, AUTO_APPROVE, TASK resume).
R1 MAJOR: isCancelled() guard before HITL gate prevents cancel
signal from being silently converted to a pause when the wave loop
breaks and control reaches the HITL gate before the next phase-loop
iteration's shouldStop() check.

R2 MAJOR: Explicit CancellationException catch in wave allOf.get()
handler. Forward-cancels all source agent futures (allOf.cancel
doesn't propagate to children). Generic catch blocks now check
token.isCancelled() and route to CANCELLED instead of FAILED.

In-flight cancel regression test: latch-based concurrent test
launches discuss() on separate thread, blocks say() with latch,
fires cancelDiscussion(GRACEFUL), asserts CANCELLED state.
Item 1: Add CANCEL_IMMEDIATE in-flight cancel test using latch-based
concurrency. Asserts CANCELLED state (not FAILED), verifying the R2
CancellationException catch and cancel-aware generic catch blocks.

Item 2: Harden CancelOfPaused — reflection-verify that activeTokens
is actually empty for the paused GC (NEW-2 guarantee), proving the
DB-write branch is taken vs the signal branch.

Item 2: Harden TaskResumeCompletesDependent — assert exact state
(IN_PROGRESS not just != AWAITING_APPROVAL), pausedAtPhaseIndex=-1,
pausedAt=null, and verify updateIfState CAS guard was called.
Add PHASE resume test verifying advance to pausedAt+1.

Test count: 14 → 16 HITL tests, 252 broad GCS tests, all green.
CancelOfPaused: drives through a real pause via discuss() with
requiresApproval=true, then verifies activeTokens is empty
(NEW-2 finally guarantee), then cancels. Would fail if NEW-2
reverted: stale token would route to signal branch (silent no-op).

IMMEDIATE cancel: uses PLAN+EXECUTE phases with pre-configured
task so say() blocks inside executeTaskExecutionPhase's
CompletableFuture.runAsync — the exact allOf.get() surface where
CancellationException fires. Would fail if R2 catch removed:
CancellationException routes to FAILED instead of CANCELLED.

TaskResumeCompletesDependent: passes a capturing listener to
resumeDiscussion, mocks the async groupStore config reload, and
asserts onPhaseStart receives phaseIndex==1 for TASK (same phase)
and phaseIndex==2 for PHASE (pausedAt+1). Would fail if the
startFromPhase calculation were broken.
Phase 1a: Delta-based checkIfPauseConversationAction — only throws if the
just-executed task added PAUSE_CONVERSATION (not stale from prior turn).
Belt-and-braces: strip action in Conversation.resume() before re-entering
the pipeline.

Phase 1b: Decision visibility — verdict stored as conversation output
(hitlDecision) and conversation-scoped property (hitlVerdict). REJECTED
emits public output for UI rendering.

Phase 2a: Request body validation — null/missing verdict returns 400 on
both REST surfaces (regular + group + streaming).

Phase 3a: Group cancel state guard — terminal states cannot be overwritten.
Phase 3b: Timeout rescheduling — re-pause during resume arms new timeout.
Phase 4a: Undo/redo gate — blocked during AWAITING_HUMAN state.
Phase 5g: Double-approve mapped to 409 Conflict (was 500).

Tests: 32 HITL tests pass (16 lifecycle + 16 group), 0 failures.
Phase 5b: Non-EXECUTE phases with TASK granularity now fall back to
PHASE-style pause. Only EXECUTE phases (which have a SharedTaskList)
use TASK-level per-task approval.

Phase 5e: Resume ordering — timeout schedule is now deleted only
AFTER the CAS succeeds. If the CAS fails, the schedule is preserved
so the timeout can still fire (prevents stuck conversations).

Phase 5f: Config drift guard — on resume, the bookmarked phase name
is validated against the loaded config. If the config was edited
while paused (phase renamed/reordered), resume fails cleanly with
a FAILED state + ERROR transcript entry.

Phase 4c: Audit collector added to the resume path — same pattern
as the say path. Without this, HITL resume operations were invisible
to the audit ledger.

Tests: 32 HITL tests pass (16 lifecycle + 16 group), 0 failures.
Phase 5a: Task rejection policy — new onTaskRejection field in
HitlConfig (FAIL/RETRY). RETRY resets rejected tasks to ASSIGNED
for re-execution via new SharedTaskList.resetFromAnyToAssigned().

Phase 5c: Timed-out task fixup — after wave timeout, stranded
IN_PROGRESS tasks are reset to ASSIGNED.

Phase 5d: Nested group HITL guard — sub-group returning
AWAITING_APPROVAL yields SKIPPED entry instead of extracting
partial answer.

Phase 6c: Pause reason — hitlPauseReason field set at commitPause
with human-readable explanation.

Phase 6d: Bookmark timeout fields — hitlTimeoutPolicy and
hitlApprovalTimeout copied from config at pause time for REST
visibility. All three fields cleared on resume.

Tests: 32 HITL tests pass (16 lifecycle + 16 group), 0 failures.
Add GET /groups/{groupId}/conversations/pending-approvals to list all
group conversations currently in AWAITING_APPROVAL state. Uses the
existing findByState(AWAITING_APPROVAL) store query. This enables
admin dashboards to show all pending HITL items across all groups.

Chain: IGroupConversationStore.findByState → GroupConversationService
.listGroupPendingApprovals → IRestGroupConversation → RestGroupConversation

Tests: 32 HITL tests pass, 0 failures.
Add HitlCrashRecoveryObserver — on startup, scans for conversations
stuck in AWAITING_HUMAN (regular) or AWAITING_APPROVAL (group) longer
than a configurable threshold (default: 24 hours). Stale conversations
are transitioned to ERROR/FAILED to prevent them from being stuck
forever after a server crash.

Configuration:
  eddi.hitl.crash-recovery.enabled=true (default)
  eddi.hitl.crash-recovery.stale-threshold=PT24H (default)

Uses existing store queries:
  IConversationMemoryStore.findConversationIdsByState()
  IGroupConversationStore.findByState()

Tests: 32 HITL tests pass, 0 failures.
Replace string fields with typed enums across both HitlConfig classes:
- HitlTimeoutPolicy: WAIT_INDEFINITELY, AUTO_APPROVE, AUTO_REJECT, ABORT
- HitlGranularity: PHASE, TASK
- HitlRejectionPolicy: FAIL, RETRY

Enums live on AgentGroupConfiguration and are reused by
AgentConfiguration.HitlConfig to avoid duplication. Jackson
serializes enum names as strings, so JSON backward compat is
preserved. Invalid values now fail at deserialization time
instead of silently passing through.

All callers updated from equalsIgnoreCase() string comparisons
to direct enum identity checks. All tests updated.

Tests: BUILD SUCCESS (all HITL + config + resume tests pass).
Phase 4d — Micrometer counters for HITL observability:
- eddi_hitl_pause_count (tag: surface=regular|group)
- eddi_hitl_resume_count (tag: surface=regular|group)
- eddi_hitl_timeout_count (tag: surface=regular|group|unknown)

Counters increment at the exact CAS transition points
(commitPause, resumeConversation, HitlTimeoutHandler).

Phase 6a — Deduplicate LifecycleManager task loop:
Both executeLifecycle() and executeLifecycleFromIndex() now
delegate to a shared private executeTaskRange() method,
eliminating ~120 lines of near-identical code. Public API
and behavior are unchanged.

Tests: 72 HITL tests pass, 0 failures.
… + 3d)

Phase 4b — Strict ownership hardening for HITL endpoints:
- New requireOwnerAdminOrApprover() in OwnershipValidator: accepts
  owner, eddi-admin, OR eddi-approver role. Fail-closed for unowned
  resources (delegates to requireOwnerOrAdminStrict).
- HITL endpoints (resume, cancel, approve, status) on both regular
  and group surfaces now use the strict+approver validator via
  validateConversationOwnership(id, true) overload.
- Non-HITL endpoints unchanged (lenient requireOwnerOrAdmin).
- 6 new unit tests covering all access paths.

Phase 3d — Discriminating status codes:
- Group cancelDiscussion: moved cancelDiscussion() inside try-catch
  so GroupDiscussionException → 409, ResourceNotFoundException → 404
  instead of leaking as 500.

Tests: 77 tests pass (including 6 new approver role tests).
… 7b)

Phase 3d fix: Remove impossible GroupDiscussionException catch from
cancelDiscussion (checked exception not declared on the interface).
Keep ResourceNotFoundException → 404 and catch-all → 500 mappings.

Phase 7b: Add HitlCrashRecoveryObserverTest (6 tests):
- Disabled mode: no interactions with stores
- Stale regular conversation → ERROR transition
- Fresh regular conversation → left alone
- Null snapshot → handled gracefully
- Stale group conversation → FAILED transition
- Fresh group conversation → left alone

Tests: 89 HITL-specific tests pass (6 new).
…ete persistence, cancel windows, rollback widening, init-pause bookkeeping
…K-gate bypass, pause restore, terminal cleanup

- Register control tokens BEFORE executor submit (async start + resume);
  executeDiscussion uses computeIfAbsent so signalled tokens are never wiped
- convertPauseToCancelIfSignalled: cancel landing during commitPause converts
  the pause to CANCELLED instead of silently surviving a successful cancel
- TASK gate also pauses when an aborted wave left executable tasks behind
  (previously fell through to VERIFY/synthesis over unexecuted work)
- Config-drift and pre-executeDiscussion resume failures restore the pause
  (restoreGroupPause) + fire group_error; submit failures roll back;
  REJECTED fires group_complete so SSE streams terminate
- cleanupAfterTerminalState releases ephemeral agents + lastVerifiedIndex on
  paused-then-terminal paths (cancel-of-paused, REJECTED resume)
- cancelDiscussion returns boolean -> REST 409 when already terminal;
  paused-cancels audited (hitl.approval, verdict CANCELLED)
- taskApprovals values validated up front; {} treated as approve-all;
  approveGroupPhase returns a fresh copy (no live-object serialization)
- scheduleGroupHitlTimeout reads the pause bookmark, not the group config
- Pending listing: bounded PendingApprovalSummary + groupId + ?limit
  (query-level filter via findByState(state, groupId, limit))

[skip docker]
- detail=full on both approval-status endpoints is gated for approver-only
  callers (not owner, not admin): full content readable ONLY while the
  conversation is awaiting approval -> 403 otherwise. The approver role is
  for deciding pending approvals, not a universal read-everything grant.
- New OwnershipValidator.isOwner (pure identity comparison, no roles)
- Group approval-status finally honors detail: summary projection by default
  (state, pausedAt, phase, pauseType, reason, timeoutPolicy, awaiting task
  ids; stale fields suppressed outside AWAITING_APPROVAL) instead of always
  dumping the full conversation incl. transcript
- OpenAPI annotations document the 403 + summary/full semantics
- Tests: approver paused/non-paused full reads, owner/admin bypass, summary
  projection shape + stale-field suppression on both surfaces

[skip docker]
- Recovery sweep runs on a background virtual thread (off the boot path);
  runRecovery package-private for deterministic tests
- Paused-regular sweep reads bounded projected summaries (10k cap, logged
  when hit) instead of full documents; WAIT_INDEFINITELY skipped with zero
  further reads. PendingApprovalSummary gains approvalTimeout (projected on
  Mongo + Postgres, set on group summaries too)
- rearmSchedule re-checks pause state AFTER create and withdraws the
  schedule if a resume/cancel landed in the window
- New name index on schedules (Mongo idx_schedules_name + Postgres) — HITL
  timeout delete/re-arm by name no longer collection-scans
- Mongo findPendingApprovalSummaries bounds the ids query with .limit() at
  the DB

[skip docker]
ginccc added 2 commits July 6, 2026 17:52
Follow-up to the MCP whitelist commit, from an adversarial code review
(whitelist-correctness and test-robustness dimensions came back clean).

- McpToolFilterTest: add test_noLangchain4jBuiltinToolIsWhitelisted — the
  inverse guard that auto-discovers every dev.langchain4j.agent.tool.Tool under
  modules.llm.tools and fails if any effective name is whitelisted (would leak an
  internal agent tool to external MCP clients). Turns the one-time manual
  no-collision check into a build-time invariant; proven to fail on an injected
  collision. Both discovery helpers now load classes without static init
  (Class.forName(name, false, ...)) to avoid bean/config init side effects.
- docs/mcp-server.md: the role table named non-existent roles mcp-user/mcp-admin
  and cited @RolesAllowed; the code enforces eddi-viewer/eddi-editor/eddi-admin
  (requireRole) and eddi-approver/owner (HitlAccessGuard). Rewrote with the real
  role strings and mechanism — load-bearing now that role-guarded memory/GDPR
  tools are reachable over MCP.

A third low finding (memory/GDPR mutations lack an independent MCP kill-switch
like eddi.mcp.hitl.mutations.enabled) is left as a maintainer decision — it is
consistent with the pre-existing posture of other whitelisted destructive tools.
McpCallsTask was committed without its bootstrap registration, so on a fresh
checkout it was never added to the @LifecycleExtensions lifecycle-task provider
map and the mcpcalls feature silently failed to wire into the pipeline.

Add McpCallsModule (@startup(1000) + @PostConstruct) mirroring the sibling
module bootstraps (ApiCallsModule, LlmModule, OutputGenerationModule, etc.),
which registers McpCallsTask.ID. Surfaced while auditing the working tree during
the MCP-whitelist review.

Copilot AI 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.

Pull request overview

Copilot reviewed 112 out of 271 changed files in this pull request and generated 2 comments.

Comment thread src/main/java/ai/labs/eddi/integrations/channels/ChannelTargetRouter.java Outdated
Comment thread src/main/java/ai/labs/eddi/engine/api/IRestAgentEngine.java Outdated
ginccc added 3 commits July 6, 2026 20:34
…licate allocations

ChannelIntegrationConfiguration.getPlatformConfig() returns a defensive copy
and never returns null (field init'd non-null, setter null-guarded), so the
`getPlatformConfig() != null` sub-checks were dead code and each doubled call
allocated a throwaway map. Cleaned up all six sites in ChannelTargetRouter:
one flagged by a Copilot PR review, five identical siblings it missed.

Genuine guards (integration != null, config != null, getChannelType() != null)
are preserved; behavior is unchanged. Verified with ./mvnw compile (checkstyle
+ formatter + javac clean).
…ot review)

Join @QueryParam/@DefaultValue onto one line for this single-param method;
fits the 150-col limit (checkstyle LineLength / formatter lineSplit) and
matches the file's convention. Pure whitespace, no behavior change.

@niedch niedch left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks, good to me! Just some more comments about the Typing for the configuration Pojos

Comment thread src/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.java Outdated
ginccc added 4 commits July 13, 2026 16:19
…eoutPolicy enum

Address PR review (@niedch): prefer the enum type for the HITL timeout
policy on the group transcript. The HitlTimeoutPolicy enum already exists
and is the declared type in all three HITL config POJOs; GroupConversation
was the lone raw-String outlier. Retype the field + getter/setter and all
group set/read-sites (commitPause, restoreGroupPause, resumeDiscussion,
scheduleGroupHitlTimeout, listGroupPendingApprovals, crash recovery, REST +
MCP summaries). Wire-safe: Jackson serializes the enum by name(), so JSON in
Mongo/Postgres/REST/Manager-UI is byte-identical (same as GroupConversationState).

The shared parsePolicy(String) is left intact for its regular-surface callers;
the group crash-recovery site inlines the null -> WAIT_INDEFINITELY default.

Deliberately NOT applying the reviewer's Duration suggestion for
hitlApprovalTimeout: String is the uniform convention across every carrier,
and under quarkus.jackson.write-dates-as-timestamps=true (no duration
override) a Duration would serialize as a number, breaking the raw-over-REST
OpenAPI/Manager-UI contract. Left as a possible separate cross-cutting effort.

Tests updated (reflective restoreGroupPause signature/args, enum assertions);
149 affected unit tests pass.
…Policy enum

Extend the group-surface enum change to the regular (agent) conversation
surface so both are consistent. IConversationMemory / ConversationMemory /
ConversationMemorySnapshot now carry HitlTimeoutPolicy instead of String;
the ConversationService bookmark set-sites drop .name() (their sources are
already the enum), scheduleHitlTimeout compares == WAIT_INDEFINITELY and
emits .name() only into the String schedule-metadata map, and the read/
display sites (RestAgentEngine, ConversationMemoryStore pending-approval
projection, SlackEventHandler) call .name(). Crash recovery inlines the
null -> WAIT_INDEFINITELY default at the regular site; parsePolicy(String)
stays for the PendingApprovalSummary projection.

Wire-safe: the snapshot persists as a JSON blob (enum -> name()), so stored
AWAITING_HUMAN bookmarks and the Postgres data->>'hitlTimeoutPolicy'
projection are unchanged, and the REST/Manager-UI contract is byte-identical.
hitlApprovalTimeout stays String (same rationale as the group change).

258 regular + group HITL unit tests pass; full tree compiles (Testcontainer
store tests run in CI).
…P read site)

Adversarial code review of the enum refactor caught the one missed site: the
regular-surface getApprovalStatus put snapshot.getHitlTimeoutPolicy() (now an
enum) into a Map<String,String> without .name(). Unlike the Map<String,Object>
group twin at :359, this fails to compile (bad type in conditional expression).
The group twin and RestAgentEngine:407 were fixed; this regular twin was missed.

The earlier `mvnw test` gave a FALSE pass — Maven incremental compilation reused
a stale McpHitlTools.class (source untouched, dependency retyped). `mvnw clean
compile` fails. Fix appends the guarded .name() to match its three siblings; wire
output is byte-identical. Verified with `mvnw clean test`: full tree compiles from
scratch, 258 affected tests pass.
…n guard

Formalizes the triple-check of the String->enum refactor. Pure-unit (no
Testcontainers) test replicating both production mappers — JSON (Postgres
JSONB + REST) and BSON (MongoDB) — across both surfaces (ConversationMemory
Snapshot, GroupConversation). Asserts enum<->name() round-trip for all four
values, BSON encodes a string not an ordinal, null is omitted (NON_NULL) and
round-trips to null, and legacy pre-refactor bare-string documents still
deserialize into the enum. 15/15 pass, covering the residual runtime/
persistence risk locally (CI Testcontainer store tests confirm the rest).

A second deep adversarial review (4 orthogonal angles + verify + critic)
returned zero findings / CORRECT_AND_COMPLETE, confirming McpHitlTools:185
was the sole defect.
…after enum audit

Documentation audit of the HitlTimeoutPolicy enum refactor. User-facing docs
(README, AGENTS.md, docs/hitl.md) correctly need no change — they reference
timeoutPolicy only at the config/REST layer, which the internal String->enum
retype leaves byte-identical. Two accuracy fixes:

- HitlCrashRecoveryObserver: the group re-arm comment said the inline default
  avoids "the String overload the regular surface shares" — stale once the
  regular surface also became an enum. Now states both bookmarks are enum and
  parsePolicy(String) survives only for the PendingApprovalSummary projection.
- changelog: the regular-surface entry listed the McpHitlTools regular read
  site as updated, but it was the missed site fixed in the follow-up commit.

Copilot AI 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.

Pull request overview

Copilot reviewed 111 out of 272 changed files in this pull request and generated 3 comments.

Comment thread src/main/resources/application.properties Outdated
Comment thread src/main/java/ai/labs/eddi/engine/security/OwnershipValidator.java
…very

Copilot PR review: the eddi.schedule.poll-batch-size comment claimed
"cluster-wide CAS still guarantees exactly-once", contradicting IScheduleStore's
documented at-least-once contract (an expired lease can be stolen; targets must
be idempotent). Corrected to: per-lease CAS gives a single claimant, delivery is
at-least-once, and the HITL timeout handler is idempotent (CAS on conversation
state). Comment-only change.
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