refactor prompt seeding to a unified steer/queue api#336
Open
Nek-12 wants to merge 34 commits into
Open
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
server/runtime/engine_part11_test.go (1)
355-359: 💤 Low valueConsider safer type assertion to avoid panic.
The unchecked type assertion on line 355 will panic if
eng.messageFlowis not of type*defaultMessageLifecycle. While acceptable in test code (the panic would fail the test), a checked assertion would provide clearer failure messages and make the test more resilient to implementation changes.♻️ Proposed refactor for safer assertion
- messageFlow := eng.messageFlow.(*defaultMessageLifecycle) - var messages []QueuedUserMessage - if messageFlow != nil && messageFlow.queue != nil { - messages = messageFlow.queue.Snapshot() - } + var messages []QueuedUserMessage + if messageFlow, ok := eng.messageFlow.(*defaultMessageLifecycle); ok && messageFlow != nil && messageFlow.queue != nil { + messages = messageFlow.queue.Snapshot() + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/runtime/engine_part11_test.go` around lines 355 - 359, Replace the unchecked type assertion on eng.messageFlow with a checked (comma-ok) assertion: attempt to cast eng.messageFlow to *defaultMessageLifecycle and, if the assertion fails, call the test failure helper (e.g., t.Fatalf or t.Fatalf-like) with a clear message mentioning eng.messageFlow and defaultMessageLifecycle; then proceed to read messageFlow.queue and call Snapshot() only when messageFlow and messageFlow.queue are non-nil, returning or failing early otherwise so the test reports a clear, non-panic error.server/runtime/exclusive_step.go (1)
106-106: ⚡ Quick winVerify ignoring steerEvent error is safe in deferred cleanup.
The error from
steerEventis explicitly ignored (_). In this deferred cleanup path, if steering theEventInFlightClearFailedevent fails, the failure is already being joined into the returned error viaerrors.Join(err, wrapped)at line 107. However, the event might not be observable to clients.Please confirm this is acceptable behavior: if the event steering fails during cleanup, is it sufficient to return the error without the event being emitted, or should this be logged/handled differently?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/runtime/exclusive_step.go` at line 106, The deferred call to s.engine.steerEvent(EventInFlightClearFailed) currently discards its error which can hide failure to emit the event; change the cleanup to capture the steerEvent error and either (a) log it via the component logger (e.g., s.logger.Errorf or s.engine.logger) so the missing event is observable, or (b) include it in the returned error by joining it with errors.Join(err, wrapped, steerErr); update the call site that currently invokes steerEvent(...) (look for steerEvent and EventInFlightClearFailed) to handle steerErr accordingly so failures to emit the event are not silently ignored.server/runtime/engine_message_ops.go (2)
80-88: 💤 Low valueRemove unused function
appendUserMessageWithoutConversationUpdate.Static analysis indicates this function is unused after the steering migration. Consider removing it to avoid dead code accumulation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/runtime/engine_message_ops.go` around lines 80 - 88, The function appendUserMessageWithoutConversationUpdate is dead code after the steering migration; remove this function definition from Engine to avoid unused code accumulation and, if present, also remove any now-unused helper steerUserMessageWithoutDerivedEventIntent to keep the codebase clean; ensure any callers are updated (none should exist per static analysis) and run tests/build to confirm no references remain.Source: Linters/SAST tools
259-301: 💤 Low valueRemove unused function
appendMessageWithoutConversationUpdate.Static analysis indicates this function is unused after the steering migration. Consider removing it to avoid dead code accumulation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/runtime/engine_message_ops.go` around lines 259 - 301, Remove the dead helper appendMessageWithoutConversationUpdate from Engine, since it is no longer referenced after the steering migration. Delete the unused method and verify there are no remaining call sites or interface/usage expectations tied to steerMessageWithoutDerivedEventIntent in this path; if that intent helper becomes unused as a result, clean it up as well.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/features/workflow-editor/WorkflowEditorRoute.tsx`:
- Around line 126-141: The canvas currently drops execution-only errors because
cleanLayoutValidation and graphValidation prefer cachedDraftValidation and never
merge execution results; add a helper mergeWorkflowValidations(draft, execution)
that returns execution when draft is null, draft when execution is null, or a
merged Validation (valid = draft.valid && execution.valid, errors =
[...draft.errors, ...execution.errors]) and replace usages: compute
cachedDraftValidation and cachedExecutionValidation as before, then set
cleanLayoutValidation = mergeWorkflowValidations(cachedDraftValidation,
cachedExecutionValidation) and set graphValidation = dirty.graphDirty ?
emptyWorkflowValidation : mergeWorkflowValidations(draftValidation,
executionValidation); also use the helper when computing layoutValidation where
you currently pick between draft/execution so execution-only errors are
preserved on the layout.
In `@prompts/reviewer_system_prompt.md`:
- Line 9: Fix the spelling mistake in the reviewer system prompt sentence where
"confrm" is used; locate the sentence "As a supervisor, your responsibilities
are to catch errors in model outputs, prevent hallucinations, ensure output
quality and worker diligence, confrm and enforce instruction following, send
reminders about unfinished work or incomplete plan items, prevent regressions,
review code, and maintain high-quality project architecture." and replace
"confrm" with "confirm" so it reads "...ensure output quality and worker
diligence, confirm and enforce instruction following...".
In `@server/runtime/tool_executor.go`:
- Line 48: The call to e.steerEvent(stepID, started) currently discards its
error; update the code in tool_executor.go to check the returned error from
e.steerEvent and handle it consistently with other event calls (either log the
error via the existing logger or return/propagate the error so transcript
consistency is preserved), ensuring that failures to persist the "started" event
are not silently ignored while subsequent e.steer(...) tool completion calls
still run; reference the e.steerEvent invocation and the surrounding
e.steer(...) completion handling to implement the error check and choose logging
or propagation to maintain event pairing.
In `@server/tools/transcript_contracts.go`:
- Around line 777-781: formatOutputDefault currently swallows json.Marshal
errors by returning "", which lets formatGenericToolResult misreport success as
"done"; change formatOutputDefault to capture and log the marshal error and
return a safe fallback string (e.g., a trimmed raw JSON string or a sentinel
like "<marshal error: ...>") instead of "", and/or return an explicit error
indicator that formatGenericToolResult can detect; then update
formatGenericToolResult to treat that sentinel / error return as a tool failure
(so it yields "tool failed" and includes the marshal error details) rather than
silently reporting "done". Ensure you update the implementations of
formatOutputDefault and formatGenericToolResult to reference their existing
function names so the fallback is logged/propagated instead of discarded.
---
Nitpick comments:
In `@server/runtime/engine_message_ops.go`:
- Around line 80-88: The function appendUserMessageWithoutConversationUpdate is
dead code after the steering migration; remove this function definition from
Engine to avoid unused code accumulation and, if present, also remove any
now-unused helper steerUserMessageWithoutDerivedEventIntent to keep the codebase
clean; ensure any callers are updated (none should exist per static analysis)
and run tests/build to confirm no references remain.
- Around line 259-301: Remove the dead helper
appendMessageWithoutConversationUpdate from Engine, since it is no longer
referenced after the steering migration. Delete the unused method and verify
there are no remaining call sites or interface/usage expectations tied to
steerMessageWithoutDerivedEventIntent in this path; if that intent helper
becomes unused as a result, clean it up as well.
In `@server/runtime/engine_part11_test.go`:
- Around line 355-359: Replace the unchecked type assertion on eng.messageFlow
with a checked (comma-ok) assertion: attempt to cast eng.messageFlow to
*defaultMessageLifecycle and, if the assertion fails, call the test failure
helper (e.g., t.Fatalf or t.Fatalf-like) with a clear message mentioning
eng.messageFlow and defaultMessageLifecycle; then proceed to read
messageFlow.queue and call Snapshot() only when messageFlow and
messageFlow.queue are non-nil, returning or failing early otherwise so the test
reports a clear, non-panic error.
In `@server/runtime/exclusive_step.go`:
- Line 106: The deferred call to s.engine.steerEvent(EventInFlightClearFailed)
currently discards its error which can hide failure to emit the event; change
the cleanup to capture the steerEvent error and either (a) log it via the
component logger (e.g., s.logger.Errorf or s.engine.logger) so the missing event
is observable, or (b) include it in the returned error by joining it with
errors.Join(err, wrapped, steerErr); update the call site that currently invokes
steerEvent(...) (look for steerEvent and EventInFlightClearFailed) to handle
steerErr accordingly so failures to emit the event are not silently ignored.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 608ec292-dae3-429f-992d-97dfc9431201
📒 Files selected for processing (95)
AGENTS.mdapps/desktop/src/features/workflow-editor/WorkflowEditorRoute.test.tsxapps/desktop/src/features/workflow-editor/WorkflowEditorRoute.tsxapps/desktop/src/features/workflow-editor/WorkflowInspectorSidebar.tsxapps/desktop/src/features/workflow-editor/workflowEditorDraft.test.tsapps/desktop/src/features/workflow-editor/workflowEditorDraft.tsapps/desktop/src/features/workflow-editor/workflowGraphLayout.tsdocs/dev/specs/core-runtime-tools.mddocs/dev/specs/tui-transcript.mdprompts/embed.goprompts/embed_test.goprompts/reviewer_system_prompt.mdprompts/tool_preambles_prompt.mdprompts/workflow/final_answer_nudge.mdserver/runprompt/headless.goserver/runprompt/headless_test.goserver/runprompt/service.goserver/runtime/background.goserver/runtime/background_test.goserver/runtime/chat_store_part2_test.goserver/runtime/compaction.goserver/runtime/compaction_carryover.goserver/runtime/compaction_executor.goserver/runtime/compaction_overflow_repair.goserver/runtime/compaction_persistence.goserver/runtime/compaction_planner.goserver/runtime/compaction_reminder.goserver/runtime/compaction_utils.goserver/runtime/engine.goserver/runtime/engine_message_ops.goserver/runtime/engine_part10_test.goserver/runtime/engine_part11_test.goserver/runtime/engine_part15_test.goserver/runtime/engine_queue_submission.goserver/runtime/engine_queue_submission_test.goserver/runtime/engine_reviewer_helpers.goserver/runtime/engine_state.goserver/runtime/exclusive_step.goserver/runtime/exclusive_step_test.goserver/runtime/goal.goserver/runtime/message_lifecycle.goserver/runtime/meta_context.goserver/runtime/meta_context_architecture_test.goserver/runtime/meta_context_runtime.goserver/runtime/orchestration_collaborators.goserver/runtime/output_steering.goserver/runtime/persisted_transcript_scan.goserver/runtime/queued_user_messages.goserver/runtime/request_cache_lineage.goserver/runtime/request_cache_lineage_test.goserver/runtime/reviewer_pipeline.goserver/runtime/state_stores_test.goserver/runtime/step_executor.goserver/runtime/subagents_test.goserver/runtime/tool_executor.goserver/runtime/tool_presentation.goserver/runtime/transcript_persistence.goserver/runtime/transcript_scan.goserver/runtime/workflow_completion.goserver/runtime/workflow_completion_test.goserver/runtime/worktree_reminder.goserver/runtimewire/runtimewire_test.goserver/runtimewire/wiring.goserver/session/fork.goserver/session/store.goserver/sessionview/dormant_cache.goserver/sessionview/snapshot.goserver/startup/startup.goserver/startup/startup_test.goserver/tools/edit/matcher.goserver/tools/patch/errors.goserver/tools/patch/tool.goserver/tools/patch/tool_commit.goserver/tools/patch/tool_state.goserver/tools/shell/manager.goserver/tools/shell/manager_notice.goserver/tools/shell/manager_output.goserver/tools/shell/manager_types.goserver/tools/shell/tool.goserver/tools/transcript_contracts.goserver/transport/gateway.goserver/transport/gateway_stream_handlers.goserver/transport/gateway_test.goserver/workflowstore/graph_save.goserver/workflowstore/protocol_violations.goserver/workflowstore/store.goserver/workflowstore/transitions.goserver/workflowview/service.goshared/client/remote_rpc.goshared/client/remote_stream.goshared/config/config_overlay.goshared/config/config_registry.goshared/rpcwire/websocket.goshared/transcript/patchformat/render.goshared/transcriptdiag/diag.go
💤 Files with no reviewable changes (5)
- server/runprompt/service.go
- shared/config/config_overlay.go
- server/tools/shell/tool.go
- shared/client/remote_rpc.go
- server/session/fork.go
Nek-12
commented
Jun 9, 2026
Replace one-line wrappers (appendMessage, appendUserMessage, appendAssistantMessage, persistToolCompletion, appendPersistedLocalEntry*, clearStreamingAssistantState, steerReasoningEntries, etc.) with direct steer/intent calls so all runtime output mutations go through a single unified steering API. Also fixes typo in reviewer system prompt, removes useless string-asserting test for WorkflowFinalAnswerNudgePrompt, adds AGENTS.md rule against such tests, and propagates steerEvent error from tool start in tool_executor. Addresses review threads on PR #336 (engine_message_ops, tool_executor, embed_test, reviewer_system_prompt).
Remove emit/emitConversationUpdated/emitCommittedTranscriptAdvanced/ emitCommittedMessageTranscriptAdvanced and their steer-side equivalents, plus appendGoalDeveloperMessage and steerPersistedGoalDeveloperMessage. All call sites now construct steerEventIntent / steerMessageIntent inline. Also: - Drop the duplicate "User ran shell command directly" developer message in SubmitUserShellCommand; the user_initiated tool-call already carries the meta-context, so detail mode no longer renders the command twice. - Replace ad-hoc "1 suggestion"/"%d suggestions" with a pluralizeEnglish helper for reviewer status text. - Split goal loop persistence failure into a separate transcript error so it does not overflow the status line.
…text - Switch the precise input-token cache key from the raw JSON request payload to its sha256 digest so large payloads no longer balloon the in-memory map. - Stop falling back to time.Now() inside the meta context builder when the caller passes a zero timestamp; require all callers to pass a real time and fix the lone caller (skillsContextMessageWithDisabled). - Coalesce skill-discovery warnings into a single transcript message rather than emitting one per warning.
…on fallback - Merge cached draft and execution validation results when computing the workflow editor's layout/graph validation so execution-only errors stay visible on the canvas instead of collapsing into the empty draft result. - formatOutputDefault now falls back to the trimmed raw JSON string when json.Marshal fails so successful tool calls no longer get reported as the default "done" status when serialization breaks down.
…eral Restore a named token-estimator helper instead of repeating the (len + 3) / 4 idiom in compaction sizing, and store the collapsed-shell fallback as a package-level JSON RawMessage so the json.Marshal failure branch in collapsedCompactionOverflowShellOutput stops re-deriving it.
- Remove Store.MarkAgentsInjected, Store.AppendEvent (the WithCommitStatus- dropping variant), Store.appendObservedEventsLocked, Store.appendEventsAtomicLocked, and unlockAndObservePersistenceWithCommitStatus. Callers now use SetAgentsInjected(true), the 3-return AppendEvent (now always exposing commit status), or the inlined unlock-observe pattern.
…ms wrapper InjectAgentsIfNeeded just delegated to steerBaseMetaContextIfNeeded; remove the messageLifecycle interface method and the engine alias. compactionSeedItems was an unused alias for llm.CloneResponseItems.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e29923dd24
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…a guard Base meta context (AGENTS.md/skills/subagents/environment) is now injected once via an in-process guard instead of a persisted session.Meta field and a SQLite column. The guard is reset by history replacement and re-set after compaction reinjection, and derived from transcript presence on resume, so the self-heal-after-bare-replacement behavior is preserved without the missing-message walk. Drops sessions.agents_injected via migration 00032 (regenerated sqlc).
…pt scan Headless state now lives in session.Meta.HeadlessActive (persisted in session.json and mirrored into the metadata read model) rather than being re-derived by scanning the conversation for enter/exit markers. The transition reconciles the launch mode (cfg.HeadlessMode) against the persisted flag: entering headless appends the enter prompt once and sets the flag, returning to interactive appends the exit prompt once and clears it, and matching states are a no-op so repeated --continue launches never duplicate the prompt. Compaction reinjects the enter prompt after the carryover/handoff messages when the flag is set, and forks inherit it. Removes the chat-store headless scan and the headlessModeActive/shouldInjectHeadlessModePromptForState helpers.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b660bc656
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… of a stub compact_and_continue_session previously fabricated a developer "compacted continuation" message into a fresh cross-role child session, never actually compacting the source. It now reuses the real /compact flow: a linear continuation continues in the source session in place and runs engine.CompactContext before the node turn, while a fan-out branch first clones the source session (new session.CloneSession) so parallel branches never compact or mutate the shared source concurrently. The mode now carries the same subagent-role requirement as continue_session. RunStartContext.IsFanoutBranch surfaces the scheduler's existing parallel-branch marker so the starter can choose fork vs in-place.
Base meta context is now reinjected as part of the single history_replaced commit instead of a separate steer afterward, so a compacted active list is born carrying it atomically (summary precedes meta in both provider and transcript order). The base-meta guard is now a deterministic length check — seeded from restored-history length at startup and replacement length after compaction — with no transcript scan and no persisted flag: every session's active list is born with base meta, so re-injection cannot occur. Removes the missing-message presence walk and its helpers, and drops the self-heal repair tests that simulated a now-impossible bare-replacement state.
…eleted nodes - Memoize the merged clean validation so the render-time layout-snapshot update no longer sees a fresh object every render (was a potential "too many re-renders" loop in the clean editor state). [P1] - Vary the draft-validation query key by draft metadata so display-name/ description edits revalidate instead of reusing the staleTime:Infinity result. [P2] - Drop draft entities that no longer resolve to a model when projecting a dirty draft onto the previous layout, so deleted nodes/groups/edges stop rendering until the next ELK layout. [P2]
…le editing Adds a lightweight workflow.graph.deriveWiring RPC that returns only the derived wiring for a draft graph, skipping the expensive validation modes. The editor runs it as a dedicated query enabled during the dirty period (keyed on graphVersion, which only bumps on real graph changes), so edge/node inspector wiring suggestions stay current while editing instead of falling back to stale pre-edit wiring until save/reload. Keeps wiring server-authoritative rather than recomputing it client-side.
The persisted Meta.HeadlessActive flag is now seeded from the restored transcript on session open, which both keeps it authoritative and migrates sessions created before the flag existed (whose metadata has no flag but whose transcript may end in a headless marker) — otherwise reopening a headless session interactively skipped the exit prompt and continuing it headlessly duplicated the enter prompt. ForkAtUserMessage now derives the child's flag from the replayed events (the state at the fork boundary) instead of copying the parent's latest flag. The transcript scan is used only at these restore/fork boundaries; per-request transitions still read the persisted flag.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2151c6cfdf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…on field edits - StartWorkflowRun no longer cleans up the plan store on setup failure when the plan reuses an existing session (resume, continue, or in-place compact-and-continue) — previously a setup failure could delete the prior node's persisted session. Only fresh/cloned run sessions stay disposable. - Skip the pre-turn compaction when resuming a started compact-and-continue run (input.Run.SessionID already set), so an interrupted run does not recompact its already-compacted history on retry. - Key the derive-wiring query on the draft graph content instead of graphVersion, since wiring-relevant edits (node input fields, edge parameters) leave graphVersion unchanged and would otherwise serve stale wiring.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb9d5bb2e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Route policy now short-circuits auth gate when RequiresStartupAuth is false (non-first-party openai_base_url), so session.plan and session.runtime.activate are no longer blocked for local-provider users with no auth configured - serviceBaseURL no longer overrides an explicit base URL with the hardcoded codex endpoint when OAuth is active; explicit URL wins - Fix double "server auth is not configured" error caused by errors.Join duplicating the sentinel message back to itself
Gating compact_and_continue on a populated run SessionID was wrong: AttachRunSession persists the reused source session before CompactContext commits, so a run interrupted during compaction skipped it on resume and continued from un-compacted history. Record the committing run in the history_replaced payload, rebuild it on restore, and gate on that instead so compaction happens exactly once per handoff.
The dirty-graph projection reused the stale ELK layout object and only refreshed node/edge data, so a re-targeted edge stayed attached to its old endpoint and a regrouped node rendered in the wrong container until the next layout (or indefinitely if layout failed). Reconnect edge endpoints from the draft model and drop a node whose group membership changed until ELK relays.
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/workflowstore/runs.go (1)
293-314:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate
IsFanoutBranchon the runnable return path.
StartWorkflowRun()bails out whenWorktreeID/WorktreeRootare empty, so every real runtime start uses this return block. BecauseIsFanoutBranchis only populated in the earlier no-worktree branch, fan-outcompact_and_continue_sessionruns arrive downstream asfalsehere and get treated as in-place reuse instead of isolated clones.Suggested fix
return RunStartContext{ Run: runRecordFromTaskRun(run), Task: taskRecordFromTask(task), Workflow: workflowRecord, Node: nodeRecordFromSnapshot(snapshot.Node, snapshot.WorkflowID), ContextMode: transitionContext.ContextMode, SourceRunID: transitionContext.SourceRunID, SourceSessionID: transitionContext.SourceSessionID, SourceNode: transitionContext.SourceNode, + IsFanoutBranch: isFanoutBranch, TransitionIDs: transitionIDsFromSnapshot(snapshot), TransitionOptions: transitionOptionsFromSnapshot(snapshot), PromptTemplate: strings.TrimSpace(runMetadata.PromptTemplate), Parameters: parameters, ParameterValues: parameterValues,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/workflowstore/runs.go` around lines 293 - 314, The RunStartContext return in StartWorkflowRun is missing propagation of IsFanoutBranch so fan-out runs are misclassified; update the returned RunStartContext (in the return block that currently sets Run, Task, Workflow, Node, etc.) to include IsFanoutBranch sourced from the transitionContext (e.g., transitionContext.IsFanoutBranch) so the same flag set in the earlier no-worktree branch is preserved for actual runtime starts; ensure you use transitionContext.IsFanoutBranch (with a sensible default if nil) when constructing the RunStartContext.
🧹 Nitpick comments (1)
server/workflowsvc/service_test.go (1)
967-969: ⚡ Quick winAssert derived wiring identity, not only count.
This check can pass even with wrong edge derivation as long as cardinality matches. Validate per-edge identity/mapping to avoid false positives.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/workflowsvc/service_test.go` around lines 967 - 969, The test currently only compares the lengths of derived.DerivedWiring.Edges and graph.Edges; update it to assert identity/mapping for each edge by iterating both slices and comparing unique identifiers or full edge structs (e.g., compare edge.ID or all relevant fields) rather than just counts, and fail the test with a helpful message when any derived edge doesn't match the expected graph edge; you can use reflect.DeepEqual or a comparison utility (like cmp.Diff) to produce readable diffs for mismatches and refer to derived.DerivedWiring.Edges and graph.Edges to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/runtime/committed_transcript_events_test.go`:
- Around line 36-37: The test currently ignores the error returned by
eng.steer("stream-step", steerClearStreamingStateIntent()), which can mask
steering failures; update the test to capture and assert the error (e.g., if err
:= eng.steer(...); err != nil { t.Fatalf(...) }) so steering failures fail fast
with a clear message instead of producing misleading downstream assertion
failures in assertEventFlags; references: eng.steer and
steerClearStreamingStateIntent and assertEventFlags.
In `@server/workflowrunner/starter.go`:
- Around line 328-335: When a disposable fan-out clone is created by
cloneSourceSessionForFanout(), ensure it and its transient stores are always
cleaned up if any subsequent step fails: after calling
cloneSourceSessionForFanout() (in the block using continuationSessionID and the
similar block at lines 355-371) immediately register a deferred/cleanup path or
try/finally so that if planner.PlanSession(...) or plan.Store.EnsureDurable()
(or any later error) occurs you delete the cloned session and close the
transient sourceStore and cloned store handles; also close those stores on the
success path once they are no longer needed. Use the same session ID and store
references returned by cloneSourceSessionForFanout(), and ensure cleanup runs on
both error and normal exit so no unattached session or open store is left
behind.
---
Outside diff comments:
In `@server/workflowstore/runs.go`:
- Around line 293-314: The RunStartContext return in StartWorkflowRun is missing
propagation of IsFanoutBranch so fan-out runs are misclassified; update the
returned RunStartContext (in the return block that currently sets Run, Task,
Workflow, Node, etc.) to include IsFanoutBranch sourced from the
transitionContext (e.g., transitionContext.IsFanoutBranch) so the same flag set
in the earlier no-worktree branch is preserved for actual runtime starts; ensure
you use transitionContext.IsFanoutBranch (with a sensible default if nil) when
constructing the RunStartContext.
---
Nitpick comments:
In `@server/workflowsvc/service_test.go`:
- Around line 967-969: The test currently only compares the lengths of
derived.DerivedWiring.Edges and graph.Edges; update it to assert
identity/mapping for each edge by iterating both slices and comparing unique
identifiers or full edge structs (e.g., compare edge.ID or all relevant fields)
rather than just counts, and fail the test with a helpful message when any
derived edge doesn't match the expected graph edge; you can use
reflect.DeepEqual or a comparison utility (like cmp.Diff) to produce readable
diffs for mismatches and refer to derived.DerivedWiring.Edges and graph.Edges to
locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4fd2c0dc-38ae-42de-8d68-4af2a08e7d47
📒 Files selected for processing (118)
.claude/skillsAGENTS.mdCLAUDE.mdapps/desktop/src/api/client.tsapps/desktop/src/api/schemas/workflow.tsapps/desktop/src/app/queryKeys.tsapps/desktop/src/features/workflow-editor/WorkflowEditorRoute.tsxapps/desktop/src/features/workflow-editor/workflowGraphLayout.test.tsapps/desktop/src/features/workflow-editor/workflowGraphLayout.tscli/app/session_lifecycle_part2_test.gocli/app/session_lifecycle_test.gocli/app/ui_mode_flow_test.gocli/app/ui_part4_test.gocli/app/ui_part5_test.gocli/app/ui_part7_test.gocli/app/ui_runtime_adapter_part3_test.gocli/app/ui_runtime_client_test.gocli/app/ui_runtime_status_test.goprompts/reviewer_system_prompt.mdprompts/system_prompt/harness_workflow_autonomy.mdserver/core/bundles.goserver/core/composition_test.goserver/core/core.goserver/embedded/embedded_test.goserver/launch/planner_test.goserver/lifecycle/lifecycle_test.goserver/llm/openai_http_test.goserver/llm/openai_http_transport_helpers.goserver/metadata/migrations/00032_drop_session_agents_injected.up.sqlserver/metadata/queries.sqlserver/metadata/sqlitegen/models.goserver/metadata/sqlitegen/queries.sql.goserver/metadata/store.goserver/metadata/store_part2_test.goserver/metadata/store_test.goserver/runtime/chat_store.goserver/runtime/committed_transcript_events_test.goserver/runtime/compaction.goserver/runtime/compaction_carryover.goserver/runtime/compaction_executor.goserver/runtime/compaction_overflow_repair.goserver/runtime/compaction_overflow_repair_test.goserver/runtime/compaction_persistence.goserver/runtime/compaction_runtime_state.goserver/runtime/compaction_trim_test.goserver/runtime/compaction_utils.goserver/runtime/engine.goserver/runtime/engine_message_ops.goserver/runtime/engine_part10_test.goserver/runtime/engine_part11_test.goserver/runtime/engine_part12_test.goserver/runtime/engine_part13_test.goserver/runtime/engine_part14_test.goserver/runtime/engine_part15_test.goserver/runtime/engine_part4_test.goserver/runtime/engine_part6_test.goserver/runtime/engine_part8_test.goserver/runtime/engine_part9_test.goserver/runtime/engine_reviewer_helpers.goserver/runtime/engine_state.goserver/runtime/engine_test.goserver/runtime/exclusive_step_test.goserver/runtime/goal.goserver/runtime/history_replacement.goserver/runtime/message_lifecycle.goserver/runtime/meta_context.goserver/runtime/meta_context_runtime.goserver/runtime/orchestration_collaborators.goserver/runtime/output_steering.goserver/runtime/prompt_cache_continuity_test.goserver/runtime/request_cache_lineage_part2_test.goserver/runtime/request_cache_lineage_test.goserver/runtime/reviewer_request_test.goserver/runtime/skills.goserver/runtime/state_stores_test.goserver/runtime/step_executor.goserver/runtime/subagents_test.goserver/runtime/token_usage_test.goserver/runtime/tool_executor.goserver/runtime/transcript_runtime_state.goserver/runtime/workflow_completion_test.goserver/runtime/worktree_reminder_test.goserver/runtimecontrol/service_test.goserver/runtimeview/projection_test.goserver/runtimeview/transcript_suffix_test.goserver/session/fileless_metadata_test.goserver/session/fork.goserver/session/runs.goserver/session/snapshot_test.goserver/session/store.goserver/session/store_part2_test.goserver/session/store_test.goserver/session/types.goserver/sessionlifecycle/service_test.goserver/sessionruntime/service.goserver/sessionview/dormant_cache_test.goserver/sessionview/service_test.goserver/sessionview/snapshot_parity_test.goserver/sessionview/transcript_suffix_test.goserver/tools/transcript_contracts.goserver/transport/gateway.goserver/transport/gateway_unary_handlers.goserver/transport/route_policy.goserver/workflowrunner/starter.goserver/workflowrunner/starter_test.goserver/workflowruntime/completion.goserver/workflowstore/runs.goserver/workflowstore/store.goserver/workflowsvc/service.goserver/workflowsvc/service_test.goserver/workflowview/service_test.goshared/client/remote.goshared/client/remote_rpc.goshared/client/workflow.goshared/protocol/handshake.goshared/rpccontract/routes.goshared/serverapi/workflow.goshared/servicecontract/services.go
💤 Files with no reviewable changes (8)
- server/runtime/orchestration_collaborators.go
- server/runtime/transcript_runtime_state.go
- server/runtime/state_stores_test.go
- server/runtime/chat_store.go
- server/metadata/sqlitegen/models.go
- server/runtime/output_steering.go
- server/metadata/queries.sql
- server/launch/planner_test.go
✅ Files skipped from review due to trivial changes (11)
- .claude/skills
- CLAUDE.md
- server/metadata/migrations/00032_drop_session_agents_injected.up.sql
- server/workflowruntime/completion.go
- server/sessionview/snapshot_parity_test.go
- AGENTS.md
- cli/app/ui_mode_flow_test.go
- cli/app/session_lifecycle_test.go
- prompts/system_prompt/harness_workflow_autonomy.md
- server/workflowview/service_test.go
- server/metadata/sqlitegen/queries.sql.go
🚧 Files skipped from review as they are similar to previous changes (14)
- server/runtime/workflow_completion_test.go
- server/runtime/meta_context_runtime.go
- server/runtime/compaction_carryover.go
- server/transport/gateway.go
- server/runtime/compaction_overflow_repair.go
- server/runtime/engine_reviewer_helpers.go
- server/runtime/request_cache_lineage_test.go
- server/runtime/goal.go
- prompts/reviewer_system_prompt.md
- server/runtime/compaction_utils.go
- server/runtime/engine.go
- server/tools/transcript_contracts.go
- server/runtime/tool_executor.go
- server/runtime/step_executor.go
A fan-out compact_and_continue branch persists a fresh clone before the rest of planSession runs, but StartWorkflowRun's cleanup hook only covers failures after planSession returns. A failure in PlanSession/EnsureDurable left the clone directory orphaned. Track the disposable clone and remove it on any planning failure.
Ignoring the steer error let a steering failure surface as a misleading downstream event-assertion mismatch instead of the real cause.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e94431520b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+251
to
+252
| data: { | ||
| ...edge.data, |
There was a problem hiding this comment.
Fresh evidence since the previous thread: WorkflowGraphEdge renders workflowEdgePath, which uses data.routePoints whenever at least two points exist before falling back to React Flow's source/target Bezier, so spreading the old edge.data here preserves the old routed endpoints. When a dirty draft reconnects or re-targets an edge and ELK is still pending or fails, the visible path and hit target remain attached to the old endpoints even though source/target were updated; clear routePoints or recompute them when projecting endpoint-changing edges.
Useful? React with 👍 / 👎.
Comment on lines
+155
to
+156
| if reusesExistingSession(input) { | ||
| return nil |
There was a problem hiding this comment.
When this guard skips cleanup for reused sessions, failures after SetWorktreeReminderState succeeds but before the run is attached (for example UpdateSessionExecutionTargetByID or AttachRunSession) now leave the reused source session alive with its worktree-reminder generation reset for a run that never started. Reopening/continuing that prior node session can then inject a duplicate/stale worktree-mode reminder; for reused sessions, defer the mutation until setup is committed or restore the previous reminder state on these later setup errors.
Useful? React with 👍 / 👎.
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 by CodeRabbit
Release Notes
New Features
Improvements
Documentation