packages/core/src/orchestrator/orchestrator-agent.ts (1)

962-1000: 💤 Low value

Optional: extract the full-pattern check to remove 4× duplication.

The block

if (INVOKE_WORKFLOW_FULL_RE.test(accumulated) || REGISTER_PROJECT_FULL_RE.test(accumulated)) {
  commandFullyParsed = true;
}

appears twice in handleStreamMode (lines 982–987, 994–999) and twice in handleBatchMode (lines 1157–1162, 1166–1170). A small helper would reduce repetition and centralize the "is the command fully captured?" decision alongside the regex constants near line 100.

♻️ Suggested helper

+const isCommandFullyParsed = (s: string): boolean =>
+  INVOKE_WORKFLOW_FULL_RE.test(s) || REGISTER_PROJECT_FULL_RE.test(s);
+
+const hasCommandPrefix = (s: string): boolean =>
+  INVOKE_WORKFLOW_PREFIX_RE.test(s) || REGISTER_PROJECT_PREFIX_RE.test(s);

Then both modes collapse to:

if (!commandDetected) {
  const accumulated = allMessages.join('');
  if (hasCommandPrefix(accumulated)) {
    commandDetected = true;
    if (isCommandFullyParsed(accumulated)) commandFullyParsed = true;
  } else {
    await platform.sendMessage(conversationId, msg.content);
  }
} else if (!commandFullyParsed) {
  if (isCommandFullyParsed(allMessages.join(''))) commandFullyParsed = true;
}

🤖 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 `@packages/core/src/orchestrator/orchestrator-agent.ts` around lines 962 -
1000, Extract the duplicated full-pattern check into a small helper (e.g.
isCommandFullyParsed(accumulated)) placed near the regex constants so it returns
true when INVOKE_WORKFLOW_FULL_RE.test(accumulated) ||
REGISTER_PROJECT_FULL_RE.test(accumulated); then replace the four duplicated
if-blocks in handleStreamMode and handleBatchMode with calls to
isCommandFullyParsed(allMessages.join('')) (or the accumulated variable) and set
commandFullyParsed = true when it returns true; keep the existing flow and only
centralize the regex logic into that helper.

packages/core/src/orchestrator/orchestrator-agent.test.ts (1)

1839-1868: 💤 Low value

Optional: strengthen the suppression test by asserting parsed command body, not just sendMessage calls.

This test verifies that extra trailing is not streamed via sendMessage, but doesn't directly assert the post-command chunk was excluded from accumulation/parsing. Since mockParseCommand is stubbed to return args: ['Foo', '/path', 'extra', 'trailing'], the actual default_cwd passed to createCodebase is decoupled from the regex parse — so a regression where commandFullyParsed failed to engage and the suppressed chunk was appended would still pass this test.

Consider asserting mockCreateCodebase was called with the parsed-from-regex value to lock in the accumulation behavior:

♻️ Stronger assertion

     // Full command was accumulated and parsed
-    expect(mockCreateCodebase).toHaveBeenCalled();
+    // The mocked parseCommand returns ['Foo','/path','extra','trailing'], but createCodebase
+    // is invoked through `/register-project ${name} ${path}` synthesized from the regex parse.
+    // If post-command suppression failed, the regex would see ' extra trailing' and the
+    // synthesized command path would include it. We assert the path stops at /path.
+    expect(mockCreateCodebase).toHaveBeenCalled();
+    // Optional: spy on handleRegisterProject input to assert the synthesized message
+    // ends with '/path' (not '/path extra trailing').

🤖 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 `@packages/core/src/orchestrator/orchestrator-agent.test.ts` around lines 1839
- 1868, Add an assertion that the accumulated/parsed command fed to
createCodebase matches the expected parsed args (i.e., that mockCreateCodebase
was invoked with the parsed default_cwd and not with the appended "extra
trailing" part) so the test verifies accumulation/parsing, not just suppression
of sendMessage; specifically, after calling handleMessage check
mockCreateCodebase (or the function that ultimately consumes the parsed command)
was called and inspect its argument(s) to assert default_cwd (or args) equals
the value returned by mockParseCommand (e.g., '/path' and
['Foo','/path','extra','trailing'] or assert the post-command text is not
present), using the existing mocks mockParseCommand and mockCreateCodebase to
locate where to add the assertion.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/core/src/orchestrator/orchestrator-agent.test.ts (1)

1780-1816: ⚡ Quick win

Test gap: /invoke-workflow … --prompt split across chunks isn't covered.

The regression suite covers the bare /invoke-workflow assist --project my-project shape but never exercises a chunk boundary that lands between --project <name> and --prompt "…". Per prompt-builder.ts, the synthesized --prompt is mandatory and must be self-contained, so it's the most likely token to be dropped if accumulation stops too early. As noted in the comment on INVOKE_WORKFLOW_FULL_RE in orchestrator-agent.ts, the current full-pattern matches as soon as the project name is seen — a test like the one below would expose that gap and pin behavior once it's fixed.

♻️ Proposed additional test

+  test('stream mode — invoke-workflow with --prompt split across 3 chunks preserves synthesizedPrompt', async () => {
+    mockListCodebases.mockReturnValueOnce(Promise.resolve([makeCodebase('my-project')]));
+    mockDiscoverWorkflowsWithConfig.mockReturnValueOnce(
+      Promise.resolve({ workflows: [makeTestWorkflowWithSource({ name: 'assist' })], errors: [] })
+    );
+    mockSendQuery.mockImplementationOnce(async function* () {
+      yield { type: 'assistant', content: 'Running.\n\n/invoke-workflow assist --project my-project ' };
+      yield { type: 'assistant', content: '--prompt "Add dark mode support"' };
+      yield { type: 'result', sessionId: 'sess-1' };
+    });
+
+    const platform = makePlatform();
+    (platform.getStreamingMode as ReturnType<typeof mock>).mockReturnValue('stream');
+    await handleMessage(platform, 'conv-1', 'add dark mode');
+
+    expect(mockDispatchBackgroundWorkflow).toHaveBeenCalled();
+    const dispatchArgs = mockDispatchBackgroundWorkflow.mock.calls[0][0] as { originalMessage: string };
+    expect(dispatchArgs.originalMessage).toBe('Add dark mode support');
+  });

🤖 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 `@packages/core/src/orchestrator/orchestrator-agent.test.ts` around lines 1780
- 1816, The tests currently cover chunk splits before the project flag but not a
split that separates the required --prompt argument; add a new test in
orchestrator-agent.test.ts that mirrors the existing "split across 2 chunks"
cases but yields assistant chunks where the first chunk ends with '--project
my-project ' and the second chunk begins with '--prompt "some prompt text"' (use
mockSendQuery to produce those two assistant yields and a final result),
exercise handleMessage with both streaming and batch platform.getStreamingMode
values, and assert that the workflow is dispatched/executed according to the
intended behavior; reference the existing test structure and symbols
handleMessage, mockSendQuery, mockDispatchBackgroundWorkflow,
mockExecuteWorkflow, and the INVOKE_WORKFLOW_FULL_RE behavior in
orchestrator-agent.ts so the test targets the prompt-boundary regression
described.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/core/src/orchestrator/orchestrator-agent.test.ts (1)

1892-1931: 💤 Low value

Consider tightening this test to avoid masking real path-corruption behavior.

The mocked parseCommand returning clean ['Foo', '/path'] lets createCodebase succeed even though the accumulated command text is /register-project Foo /path extra trailing (trailing chunk concatenated into the unquoted path). The comment honestly notes this would fail existsSync in production, but the assertion expect(mockCreateCodebase).toHaveBeenCalledWith(expect.objectContaining({ default_cwd: '/path' })) reads as if the path was correctly isolated when it actually is not.

A safer formulation would be either (a) terminate chunk 2 with \n so commandFullyParsed fires and chunk 3 is genuinely suppressed (then assert default_cwd: '/path' truthfully), or (b) drop the mockParseCommand override and assert the suppression-only behavior on platform.sendMessage — leaving createCodebase call expectations out — so the test doesn't appear to validate path correctness it isn't actually validating. Optional, since the streaming-suppression invariant is what this test is really about.

🤖 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 `@packages/core/src/orchestrator/orchestrator-agent.test.ts` around lines 1892
- 1931, The test currently masks real path-corruption because mockParseCommand
returns clean args while mockSendQuery yields a third " extra trailing" chunk
that would actually corrupt the reconstructed unquoted path; either (a) make the
second assistant chunk include a trailing "\n" (e.g. yield { type: 'assistant',
content: ' /register-project Foo /path\n' }) so commandFullyParsed becomes true
and the third chunk is suppressed (keep mockParseCommand and the createCodebase
assertion), or (b) remove the mockParseCommand override and drop the
createCodebase assertion so the test only asserts streaming behavior via
platform.sendMessage and does not pretend to validate path correctness (adjust
mockSendQuery accordingly); update the mockSendQuery yields and expectations to
match the chosen approach and keep references to handleMessage, mockSendQuery,
mockParseCommand, and mockCreateCodebase when editing.

packages/core/src/orchestrator/orchestrator-agent.ts (1)

177-179: 💤 Low value

normalizeCommandText won't strip bold wrappers on indented lines.

The pattern /^\*+(\/[^\n]*?)\**$/gm anchors directly on * at line start, so **/register-project foo /path** (any leading whitespace, blockquote, list marker, etc.) won't be normalized and the prefix regex will still miss the line. If observed Pi outputs always emit bold-wrapped commands flush-left, this is fine; otherwise, allow optional leading whitespace and trim trailing whitespace too.

♻️ Optional: tolerate leading/trailing whitespace

-function normalizeCommandText(text: string): string {
-  return text.replace(/^\*+(\/[^\n]*?)\**$/gm, '$1');
-}
+function normalizeCommandText(text: string): string {
+  // Tolerate leading whitespace (blockquote/indent) and trailing whitespace
+  // around the bold/italic wrappers — some models indent their output.
+  return text.replace(/^[ \t]*\*+(\/[^\n]*?)\**[ \t]*$/gm, '$1');
+}

🤖 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 `@packages/core/src/orchestrator/orchestrator-agent.ts` around lines 177 - 179,
normalizeCommandText fails to strip bold wrappers when lines have leading
whitespace; update the function (normalizeCommandText) to allow optional leading
whitespace before the opening asterisks and to trim trailing whitespace after
the closing asterisks (e.g., accept /^\s*\*+(\/[^\n]*?)\**\s*$/gm or
alternatively apply .trim() to the captured text) so indented or spaced lines
like "   **/register-project foo /path**" are normalized correctly.

packages/core/src/orchestrator/orchestrator-agent.ts (3)

170-185: 💤 Low value

normalizeCommandText only strips asterisks — confirm Pi/Claude don't also wrap with backticks/underscores.

The docstring mentions "bold/italic decorators", but the regex only matches \*+. Markdown italic with underscores (_/register-project ..._) or inline code (\/register-project ...`) would still defeat both INVOKE_WORKFLOW_PREFIX_REandREGISTER_PROJECT_PREFIX_RE`. If the only observed bad-output mode from Pi was asterisks, this is fine as-is; otherwise consider broadening the pattern to cover the other two wrappers.

🤖 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 `@packages/core/src/orchestrator/orchestrator-agent.ts` around lines 170 - 185,
normalizeCommandText currently only removes surrounding asterisks, but other
markdown wrappers (backticks or underscores) can also prevent
INVOKE_WORKFLOW_PREFIX_RE / REGISTER_PROJECT_PREFIX_RE and the full regexes from
matching; update normalizeCommandText to also strip a single pair of surrounding
backticks and/or underscores (and combinations like `_*...*_`) so lines whose
first non-wrapper char is '/' are normalized. Keep the normalization used by
isCommandFullyParsed (which tests INVOKE_WORKFLOW_FULL_RE and
REGISTER_PROJECT_FULL_RE) consistent with the prefix regexes by expanding the
replacement pattern to remove leading/trailing `, _, and * wrappers around the
/... command text while preserving internal characters.

---

1457-1467: 💤 Low value

Normalize-before-extract is correct, but the indexOf fallback can land on prose.

Normalizing first ensures the ** wrappers don't leak into textBeforeReg — good. The fallback normalizedForExtraction.indexOf('/register-project') at line 1463 is defensive but, by definition, will trigger only when the line-anchored regex misses, in which case the only available occurrence is non-line-anchored (i.e., almost certainly a prose mention like "use /register-project ..."). In that case we'd send the prose containing the command as the user-visible preamble and then attempt registration with whatever parseOrchestratorCommands captured — which itself is line-anchored, so registration should never be set in this branch in the first place. Consider replacing the fallback with an early return / log warning so the inconsistency is visible rather than silently producing an odd preamble.

♻️ Optional: make the no-line-match case explicit

-  const normalizedForExtraction = normalizeCommandText(fullResponse);
-  // Match line-anchored to avoid landing on a prose mention of "/register-project".
-  const regLineMatch = /^\/register-project\b/m.exec(normalizedForExtraction);
-  const regIndex = regLineMatch?.index ?? normalizedForExtraction.indexOf('/register-project');
-  const textBeforeReg = normalizedForExtraction.slice(0, regIndex).trim();
+  const normalizedForExtraction = normalizeCommandText(fullResponse);
+  // Match line-anchored to avoid landing on a prose mention of "/register-project".
+  const regLineMatch = /^\/register-project\b/m.exec(normalizedForExtraction);
+  if (!regLineMatch) {
+    getLog().warn({ conversationId }, 'orchestrator.register_project_line_not_found');
+  }
+  const regIndex = regLineMatch?.index ?? normalizedForExtraction.length;
+  const textBeforeReg = normalizedForExtraction.slice(0, regIndex).trim();

As per coding guidelines: "Use Pino logger factory with event naming format {domain}.{action}_{state}".

🤖 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 `@packages/core/src/orchestrator/orchestrator-agent.ts` around lines 1457 -
1467, The code currently falls back to
normalizedForExtraction.indexOf('/register-project') which will match prose and
produce an inconsistent preamble; change the logic in the block around
normalizeCommandText, regLineMatch and regIndex so that if the line-anchored
regex (/^\/register-project\b/m.exec) returns null you do not use indexOf but
instead log a warning and return early so no prose is sent; specifically, remove
the normalizedForExtraction.indexOf fallback, add an explicit branch when
regLineMatch is falsy that calls the module Pino logger (use event name like
"orchestrator.extract_no_line_match" per the pino format) and then return
without calling platform.sendMessage(conversationId, ...); keep
parseOrchestratorCommands behavior as-is.

---

1206-1256: 💤 Low value

Batch-mode gating and unconditional total-chunk cap look correct.

Pushing into allChunks unconditionally while gating assistantMessages accumulation/truncation on !commandFullyParsed correctly preserves the parse tokens, and enforcing MAX_BATCH_TOTAL_CHUNKS outside the commandDetected branch keeps the unconditional allChunks.push bounded. One minor caveat worth noting in a comment: if the AI never completes a detected command (no newline / closing quote ever arrives), assistantMessages will skip the MAX_BATCH_ASSISTANT_CHUNKS shift and grow up to MAX_BATCH_TOTAL_CHUNKS — that's still bounded by the outer cap, but it's a behavior change from “always cap assistant chunks” worth calling out for future maintainers.

Also applies to: 1299-1304

🤖 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 `@packages/core/src/orchestrator/orchestrator-agent.ts` around lines 1206 -
1256, Add a clarifying comment near the loop handling
allChunks/assistantMessages (around allChunks, assistantMessages,
commandDetected, commandFullyParsed logic) that explains the edge case: once a
command is detected (commandDetected=true) we stop enforcing the
MAX_BATCH_ASSISTANT_CHUNKS shift, so if the AI never yields a complete command
(isCommandFullyParsed never true) assistantMessages can grow until the outer
MAX_BATCH_TOTAL_CHUNKS bound is hit; explicitly document this behavior and
rationale so future maintainers know why assistant chunk capping is conditional
and that total chunks still provide an overall cap.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/core/src/orchestrator/orchestrator-agent.ts (1)

170-179: 💤 Low value

Asymmetric markdown stripping — minor edge case.

The pattern /^\s*\*+(\/[^\n]*?)\**\s*$/gm requires at least one leading asterisk (\*+), so it normalizes symmetric (**/cmd ...**, */cmd ...*) and leading-only (**/cmd ...) cases, but a trailing-only emission (/register-project foo bar**) is left untouched and would leak ** into the path token captured by parseOrchestratorCommands's (.+)$. Symmetric bold is by far the common emission pattern, so this is unlikely to fire in practice; flagging for awareness only.

🤖 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 `@packages/core/src/orchestrator/orchestrator-agent.ts` around lines 170 - 179,
The regex in normalizeCommandText requires at least one leading asterisk so it
misses trailing-only cases like "/register-project foo**"; change the quantifier
on the leading asterisks from \*+ to \** so lines with zero or more leading
asterisks are normalized (i.e., update the pattern in normalizeCommandText from
/^\s*\*+(\/[^\n]*?)\**\s*$/gm to use /^\s*\**(\/[^\n]*?)\**\s*$/gm) to strip
trailing-only, leading-only, and symmetric asterisks consistently.