fix: honest done-vs-error reporting on committed tools + routine retry reconciliation (#506) - #527
Merged
Merged
Conversation
createRoutine committed the DB row before the turn's confirmation was delivered and only rolled back on scheduler-registration failure. A retry after the confirmation leg dropped (channel timeout, dropped response) hit the (tenant, user, name) unique constraint and surfaced as a bare RoutineNameConflictError — a false negative with no diagnostic value that pushed the caller toward creating an actual duplicate under a different name. createRoutine now catches that conflict, looks up the row that already won the race (RoutineStore.getByName, new), and returns it when cron/prompt/channel/timeoutMs match what was just requested — the earlier call already succeeded, so the retry now reports success too. A conflict with genuinely different fields still raises RoutineNameConflictError as before. Threading a correlation/trace id through routine-turn error responses end-to-end, and the Teams-side wording, remain out of scope (see docs/CHANGELOG.md entry and PR description). Closes #506
Issue #506's reconciliation in RoutineRunner.createRoutine matched a retried create against ANY existing row with the same (tenant, user, name) whose cron/prompt/channel/timeoutMs were identical, regardless of that row's status. A paused same-name row would silently be returned as if the create had succeeded, with no active schedule ever registered. Reconciliation now additionally requires existing.status === 'active' before treating the conflict as the caller's own in-flight retry. A paused/inactive same-name collision still raises RoutineNameConflictError, as a genuine, separate name collision should. Adds unit test coverage for the paused-existing-row case and documents (in-code and in the changelog) the decision not to additionally gate on existing.createdAt recency.
isSameRoutineRequest() compared only cron/prompt/channel/timeoutMs when deciding whether a name-conflicting create is the same request as an existing active row. It omitted outputTemplate — an independently- settable object field on both Routine and CreateRoutineInput (Phase C structured-output templates). Concrete failing case: a routine is created active with one outputTemplate, then createRoutine is called again for the same tenant/user/name/cron/prompt/channel/timeoutMs but a DIFFERENT outputTemplate. The unique-name constraint fires, getByName() finds the active row, and isSameRoutineRequest() returned true because outputTemplate wasn't checked — the call silently returned the OLD row, discarding the caller's new template while reporting success. That is exactly the class of silent-wrong-result bug issue #506 was filed to eliminate, on a field the fix's own comparison forgot. isSameRoutineRequest now compares outputTemplate via node:util's isDeepStrictEqual (it is an object, so reference/=== equality is not sufficient). Adds a unit test asserting a create that differs only in outputTemplate still raises RoutineNameConflictError, and a companion test asserting a structurally-identical-but-distinct-object outputTemplate still reconciles as a true retry. Closes #506
A user already at maxActivePerUser who retries an identical createRoutine call (the exact issue #506 scenario — their turn's confirmation was dropped) was rejected by the quota check before the retry ever reached the RoutineNameConflictError reconciliation added earlier on this branch, resurfacing the false-negative as RoutineQuotaExceededError instead. createRoutine now looks up the existing row by (tenant, userId, name) before the quota check and before calling store.create(). If it is active and matches the request, it is returned immediately — no new row, no quota check. A genuinely new routine name still goes through the quota gate as before. The store.create() catch-block reconciliation is unchanged; it remains the race-safety net for a concurrent create between this lookup and the insert. Adds two tests: a same-request retry at capacity reconciles without throwing RoutineQuotaExceededError, and a genuinely new routine name at capacity still throws it.
RoutineRunner.createRoutine now calls store.getByName unconditionally (both proactively before the quota check and in the conflict- reconciliation catch block), but hrRoutineTemplate.integration.test.ts defines its own hand-rolled InMemoryRoutineStore that lacked the method. Its runner.createRoutine calls would throw 'this.store.getByName is not a function' at runtime. Add getByName mirroring the semantics of the real RoutineStore.getByName: match on tenant + userId + name, return null if none exists.
isSameRoutineRequest excluded conversationRef from its equality check, reasoning that it was a delivery-mechanism detail the caller doesn't control byte-for-byte. That's wrong for the cold-start outreach path: ManageRoutineTool.handleCreate resolves conversationRef from targetEmail via buildEmailColdStartTarget before calling createRoutine, so conversationRef IS caller-specified there. A create for a new targetEmail that otherwise matches an existing active routine (same tenant/user/name/cron/prompt/channel/timeoutMs/ outputTemplate) would silently reconcile to the existing row and report success, while the new recipient never gets set up and the routine keeps messaging the original one. Add conversationRef to the isSameRoutineRequest comparison via isDeepStrictEqual (same pattern as outputTemplate — buildEmailColdStartTarget resolves deterministically per email, so deep equality correctly distinguishes a true retry from a different-recipient request) and update the doc comment to drop the now-incorrect exclusion rationale.
chatStreamInner wraps its whole per-turn iteration loop in one try/catch, so an exception in a LATER iteration's model call (e.g. generating the confirmation) discarded an EARLIER iteration's already-committed tool result behind a bare `error` event -- the issue's actual one-click repro, not just the retry case fixed earlier on this branch. Track (generically, by tool name only) whether at least one tool_result succeeded this turn. If the catch block is reached with a committed result recorded, yield `done` with an honest answer naming the tool(s) that completed, instead of the bare error. A turn where nothing committed yet still yields `error`, unchanged. Adds middleware/test/orchestrator/committedToolReporting.test.ts and a CHANGELOG entry.
The emergency-done path added last round for a committed tool call followed by a later model-call failure skipped sessionLogger.log(), unlike every other done-emission site in chatStreamInner. Without a transcript entry, a follow-up turn had no record the tool already ran and could re-invoke it for any tool without routine-create's DB-level reconciliation — the duplicate-side-effect class of bug issue #506 exists to prevent. Log with the same argument shape as the other sites before yielding; best-effort, never swallows the done event. Test file now builds the orchestrator with a recording sessionLogger (the prior 2 tests built none) and asserts the log call on the emergency path plus its absence on a genuine failure.
…ion (#506) isSameRoutineRequest compared the stored (store.create()-normalized, `?? {}`) conversationRef against the raw retry input with no equivalent default, unlike timeoutMs/outputTemplate. A genuine retry with an omitted conversationRef (the ordinary non-cold-start create path) therefore fell through to RoutineNameConflictError instead of reconciling. Apply the same `?? {}` default the store uses; mirror the fix in both hand-rolled InMemoryRoutineStore test fakes so the fake's create() matches the real store's normalization; add a regression test.
…in multi-tool test The maintainer was presented with generic-across-all-tools vs. narrowed-to-routine-create-only vs. drop-the-fix-entirely for the emergency done-instead-of-error path in chatStreamInner and explicitly chose to keep the current generic, tool-agnostic committedToolNames tracking, accepting the residual risk that a benign read-only success can mask a later, silently-skipped mutation. - Add a doc comment above committedToolNames and above the catch block's done-vs-error branch in orchestrator.ts explaining the tradeoff, the residual risk, and that this is a deliberate, maintainer-reviewed decision (issue #506), not an oversight. - Add a committedToolReporting.test.ts case that pins this accepted behavior: a read-only tool succeeds in one iteration, a later iteration's model call throws before it could request a second, different (mutating) tool call, and the turn still reports done. - Update docs/CHANGELOG.md's existing #506 entry to record the maintainer's explicit sign-off on this tradeoff. No production logic changed.
…onale Prior wording asserted an unverifiable 'the maintainer was presented with X/Y/Z and explicitly chose' narrative in source comments and the CHANGELOG. Rephrased as engineering rationale (what was considered, why the generic behavior was kept) without the review-round framing.
…emplate-conflict-check # Conflicts: # docs/CHANGELOG.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #506 — a generic "Etwas ist schief gegangen" (something went wrong) error was shown to the user even when the requested action (e.g. creating a routine) had actually succeeded server-side. This was a false-negative with zero diagnostic value, and in the retry case, one that could nudge the user into creating a genuine duplicate.
Two independent root causes are fixed:
1. Streamed-turn false negative on a committed tool call (
middleware/packages/harness-orchestrator/src/orchestrator.ts)This is the fix for the issue's actual one-click repro (no retry involved).
chatStreamInnerwraps its whole per-turn iteration loop in a singletry/catch. An exception thrown by a later iteration's model call (e.g. the call that generates the natural-language confirmation text) — after an earlier iteration's tool call had already committed a real side effect and yielded a successfultool_result— still produced a bare{ type: 'error' }event, discarding the fact the action already happened.The loop now tracks, generically and tool-agnostically (by tool name only — no per-tool special-casing), which tool(s) committed a successful result this turn. When the catch block is reached with at least one such result recorded, it yields the channel-SDK's existing
doneevent instead — the same event type every ordinary successful turn already uses, so no changes are needed in any channel adapter (including the external, private Teams adapter) — with an honest answer naming the tool(s) that completed and stating the turn itself didn't finish generating a follow-up response. A turn where nothing committed yet (a genuine failure) is unaffected and still yieldserror.The emergency
donepath also callssessionLogger.log(...), matching every otherdone-emission site in this function, so a follow-up turn has a transcript record of the commit and won't blindly re-invoke the same tool.Documented, deliberate tradeoff: this tracking treats any successful tool call as grounds for reporting
done, without distinguishing read-only tools from mutating ones. A read-only tool succeeding early in a turn, followed by a later, more consequential tool call silently never running (transient failure), would still reportdone. This was weighed against narrowing the fix to routine-create only or dropping it entirely, and resolved in favor of the generic behavior — the alternative leaves the reported bug unfixed for every tool except one. See the code comments oncommittedToolNamesand the catch block inorchestrator.ts, and the pinned regression testcommittedToolReporting.test.ts("reports done even when a later intended action never ran (accepted tradeoff, see code comment)").2. Routine-create retry reconciliation (
middleware/src/plugins/routines/routineRunner.ts,routineStore.ts)Defense in depth for the retry path:
RoutineRunner.createRoutinepreviously let a retriedcreate(e.g. after a turn's confirmation was dropped before fix #1 above existed, or simply a caller/model retry) fall through toRoutineNameConflictError, nudging the caller toward retrying under a different name and genuinely duplicating the routine.It now reconciles: on a name conflict (or proactively, before the per-user quota check — a retry from a user already at quota was previously rejected with the wrong exception type before ever reaching reconciliation), it looks up the existing row via
RoutineStore.getByName(new) and, if that row isactiveand matches the request field-by-field —cron/prompt/channel/timeoutMs/outputTemplate/conversationRef, each compared with the same defaulting the store itself applies (?? 600_000,?? null,?? {}) — returns that row instead of raising. A conflict against genuinely different fields, or against a paused/inactive same-name row, still raisesRoutineNameConflictError— those are real, separate collisions, not the caller's own in-flight retry.Out of scope (documented in
docs/CHANGELOG.md)ChatTurnInput/ChatTurnResultcontract (@omadia/channel-sdk) plus support in every channel adapter. Fix chore(deps,ci): Bump docker/build-push-action from 6 to 7 #1 above resolves the practical symptom anyway: a successfully-committed action is no longer reported as an opaque failure at all.omadia-byte5-plugins), outside this repo.Verification
Closes #506
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.