Orchestrator refinement#20
Merged
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR refactors the orchestrator architecture by introducing session-based engine execution, replacing shell-based hooks with in-memory function-based hooks, removing the ChangesSession-Based Engine Execution
In-Memory Hook Architecture
Orchestrator Class and API Restructure
Core Package Removal
Package Structure and Publishing
Logging and Diagnostics
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
orchestrator/packages/task-source-memory/package.json (1)
3-7:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a publish whitelist for this package.
Line 3 updates version, but this manifest still has no
filesrestriction. That can unintentionally publish non-build artifacts.📦 Proposed fix
{ "name": "`@bifrost-ai/task-source-memory`", "version": "0.0.0", "description": "In-memory task source implementation for Orchestrator Framework", "type": "module", + "files": [ + "dist" + ], "main": "./dist/index.js", "types": "./dist/index.d.ts",🤖 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 `@orchestrator/packages/task-source-memory/package.json` around lines 3 - 7, The package manifest lacks a publish whitelist which can publish source/artifacts unintentionally; update the package.json for the task-source-memory package to add a "files" array that explicitly whitelists the build output (e.g., "dist/") and other necessary artifacts (like "package.json", "README.md", "LICENSE") and remove or avoid publishing dev-only files; modify the package.json (referencing the existing "main" and "types" fields) to include this "files" field so only intended files are published.orchestrator/packages/orchestrator/src/core/orchestrator.ts (1)
107-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck
engineResult.successbefore completing the task.The engine result status is never checked. When
execute()returnssuccess: false, the code still callstaskSource.completeTask()and returns{outcome: "completed"}. Additionally, theskipFulfillflag is ignored. The task should fail or be handled differently if the engine execution was unsuccessful.🤖 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 `@orchestrator/packages/orchestrator/src/core/orchestrator.ts` around lines 107 - 145, After calling engine.execute and before executing post-task hooks, check engineResult.success; if it's false then do not proceed to complete the task or run fulfillment logic: call taskSource.failTask(task.id, `Engine execution failed: ${engineResult.error ?? "unknown"}`) and return { outcome: "failed", error: engineResult.error } unless skipFulfill is true, in which case skip both fail/complete calls and return { outcome: "skipped", error: engineResult.error } so the skipFulfill flag is respected; update references in this flow to engineResult, engine.execute, skipFulfill, and taskSource.failTask / taskSource.completeTask accordingly.
🤖 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 `@orchestrator/packages/engine/src/test-engine.ts`:
- Around line 58-62: The current logic reuses a previous this.#currentSessionId
when sessionId is omitted; change it so any call without an explicit sessionId
always mints a new session id (e.g., this.#currentSessionId =
`test-session-${Date.now()}`) instead of conditional on the existing value;
update the three occurrences that reference this.#currentSessionId/sessionId
(the block shown and the other occurrences noted at lines 64 and 73) so omission
means "fresh session" rather than "reuse previous."
In `@orchestrator/packages/orchestrator/src/agent-helper.ts`:
- Around line 37-45: The loadAgent function merges in the wrong direction:
deepMerge(agent ?? {}, definition) makes the parsed definition override
caller-provided fields; change the merge to have definition as the base and
agent applied second (e.g., call deepMerge(definition, agent ?? {})) so caller
overrides like name, promptBody, and hooks take precedence; update code around
loadAgent and ensure parseAgentDefinition and AgentDefinition types are
unchanged.
In `@orchestrator/packages/orchestrator/src/core/orchestrator.ts`:
- Around line 100-104: The loop currently pre-decrements maxFollowUps (let
maxFollowUps = 10; while ((maxFollowUps -= 1) > 0) { ... }) so when all Stop
hooks keep returning "follow-up" the code falls through and calls
completeTask(), incorrectly reporting success; change the loop to use a proper
attempt counter (e.g., while (maxFollowUps-- > 0) or loop with attemptsUsed++
and compare) and after the loop detect exhaustion and return a halted/failure
outcome instead of calling completeTask(); update the same pattern wherever
maxFollowUps is pre-decremented (the duplicate blocks handling Stop hook
"follow-up") to reference maxFollowUps and completeTask() and ensure exhausted
branches call the halted/fail path.
In `@orchestrator/packages/orchestrator/src/orchestrator-class.ts`:
- Around line 39-53: The loop in run() over this.taskSource.watchTasks()
currently lets any rejection from orchestrate(...) escape and terminate the
for-await loop; wrap the per-task processing in a try/catch so each task is
handled independently: inside the try call orchestrate({ task, agent,
taskSource: this.taskSource, engine: this.engine, projectDir: process.cwd() })
and log the success path, and in the catch call
this.taskSource.failTask(task.id, <error message or stack>) (or similar) to mark
the task failed, then continue the loop; ensure you still handle the
unknown-agent branch as before (this.agents.get(task.agentId) and failTask for
unknown agent).
In `@orchestrator/packages/task-source-bifrost/src/bifrost-task-source.ts`:
- Around line 64-70: The poll loop in bifrost-task-source.ts currently emits
console.log every iteration using pollCount and pollInterval which creates noisy
logs; change the logging so it only logs on meaningful state changes or at a
sampled interval (e.g., every Nth poll or when debug mode flag is enabled) and
suppress per-iteration messages otherwise. Locate the while (true) poll loop
that increments pollCount and remove or guard the two per-iteration console.log
calls (the poll header and rune-count log) to only run when (a) pollCount %
SAMPLE_RATE === 0, (b) a detected state change occurs, or (c) a DEBUG/verbose
flag is true; keep pollCount and pollInterval logic but add a configurable
SAMPLE_RATE or debug check to control emission. Ensure you update any
tests/logging docs and use consistent logging API (replace console.log with the
existing logger if one exists) so that only sampled or state-change logs are
emitted.
- Around line 41-45: The unconditional console.log of bifrostConfig.url and
bifrostConfig.realm can leak sensitive endpoint metadata; update the logging in
bifrost-task-source.ts (the lines referencing bifrostConfig.url,
bifrostConfig.realm and this.#config.*) to either (a) only emit full URL/realm
when a debug flag is enabled (e.g., this.#config.debug or a
logger.isDebugEnabled check) or (b) redact/mask sensitive parts (host, path,
token query) before logging and always keep pollInterval/maxPollInterval as
non-sensitive. Implement the chosen approach where the current console.log calls
occur so sensitive values are not printed in normal runs.
In `@orchestrator/publish.js`:
- Around line 46-47: Replace all uses of shell-interpolated execSync with
child_process.execFileSync and pass command and arguments as an array to avoid
shell parsing; specifically, swap execSync calls (referenced as execSync in
publish.js) for execFileSync from the child_process module, invoke npm with
arguments like ["version","patch","--no-git-tag-version"] instead of a single
concatenated string, and for the case that uses currentVersion (the execSync
that previously interpolates currentVersion) pass currentVersion as an argument
element rather than injecting it into a shell string; also remove any
unnecessary shell: "/usr/bin/bash" options and ensure stdio is forwarded (e.g.,
stdio: "inherit") so the behavior remains the same.
---
Outside diff comments:
In `@orchestrator/packages/orchestrator/src/core/orchestrator.ts`:
- Around line 107-145: After calling engine.execute and before executing
post-task hooks, check engineResult.success; if it's false then do not proceed
to complete the task or run fulfillment logic: call taskSource.failTask(task.id,
`Engine execution failed: ${engineResult.error ?? "unknown"}`) and return {
outcome: "failed", error: engineResult.error } unless skipFulfill is true, in
which case skip both fail/complete calls and return { outcome: "skipped", error:
engineResult.error } so the skipFulfill flag is respected; update references in
this flow to engineResult, engine.execute, skipFulfill, and taskSource.failTask
/ taskSource.completeTask accordingly.
In `@orchestrator/packages/task-source-memory/package.json`:
- Around line 3-7: The package manifest lacks a publish whitelist which can
publish source/artifacts unintentionally; update the package.json for the
task-source-memory package to add a "files" array that explicitly whitelists the
build output (e.g., "dist/") and other necessary artifacts (like "package.json",
"README.md", "LICENSE") and remove or avoid publishing dev-only files; modify
the package.json (referencing the existing "main" and "types" fields) to include
this "files" field so only intended files are published.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 89adf1be-720c-47c9-8f5e-f69c1d382beb
⛔ Files ignored due to path filters (1)
orchestrator/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (49)
.vscode/settings.jsonorchestrator/package.jsonorchestrator/packages/core/package.jsonorchestrator/packages/core/src/hook-executor.spec.tsorchestrator/packages/core/src/hook-executor.tsorchestrator/packages/core/src/repo-installer.spec.tsorchestrator/packages/core/src/repo-installer.tsorchestrator/packages/core/src/types.tsorchestrator/packages/core/tsconfig.jsonorchestrator/packages/core/vite.config.tsorchestrator/packages/engine-claude-code/package.jsonorchestrator/packages/engine-claude-code/src/claude-code-engine.spec.tsorchestrator/packages/engine-claude-code/src/claude-code-engine.tsorchestrator/packages/engine/package.jsonorchestrator/packages/engine/src/interface.spec.tsorchestrator/packages/engine/src/interface.tsorchestrator/packages/engine/src/test-engine.spec.tsorchestrator/packages/engine/src/test-engine.tsorchestrator/packages/engine/src/types.tsorchestrator/packages/orchestrator/package.jsonorchestrator/packages/orchestrator/src/agent-helper.tsorchestrator/packages/orchestrator/src/config.spec.tsorchestrator/packages/orchestrator/src/config.tsorchestrator/packages/orchestrator/src/core/agent-parser.spec.tsorchestrator/packages/orchestrator/src/core/agent-parser.tsorchestrator/packages/orchestrator/src/core/handlebars-renderer.spec.tsorchestrator/packages/orchestrator/src/core/handlebars-renderer.tsorchestrator/packages/orchestrator/src/core/hook-executor.spec.tsorchestrator/packages/orchestrator/src/core/hook-executor.tsorchestrator/packages/orchestrator/src/core/index.tsorchestrator/packages/orchestrator/src/core/orchestrator.spec.tsorchestrator/packages/orchestrator/src/core/orchestrator.tsorchestrator/packages/orchestrator/src/core/types.tsorchestrator/packages/orchestrator/src/core/validator.spec.tsorchestrator/packages/orchestrator/src/core/validator.tsorchestrator/packages/orchestrator/src/git-root.spec.tsorchestrator/packages/orchestrator/src/git-root.tsorchestrator/packages/orchestrator/src/index.spec.tsorchestrator/packages/orchestrator/src/index.tsorchestrator/packages/orchestrator/src/orchestrator-class.tsorchestrator/packages/orchestrator/tsconfig.jsonorchestrator/packages/task-source-bifrost/package.jsonorchestrator/packages/task-source-bifrost/src/bifrost-task-source.tsorchestrator/packages/task-source-memory/package.jsonorchestrator/packages/task-source-memory/src/memory-task-source.tsorchestrator/packages/task-source/package.jsonorchestrator/publish.jsorchestrator/tsconfig.jsonoxlint.config.ts
💤 Files with no reviewable changes (15)
- orchestrator/packages/core/package.json
- orchestrator/packages/core/src/repo-installer.ts
- orchestrator/packages/orchestrator/src/config.ts
- orchestrator/packages/orchestrator/src/git-root.ts
- orchestrator/packages/task-source-memory/src/memory-task-source.ts
- orchestrator/packages/core/src/types.ts
- orchestrator/packages/core/vite.config.ts
- orchestrator/packages/orchestrator/src/config.spec.ts
- orchestrator/packages/core/src/repo-installer.spec.ts
- orchestrator/packages/core/src/hook-executor.ts
- orchestrator/packages/core/src/hook-executor.spec.ts
- orchestrator/packages/orchestrator/src/git-root.spec.ts
- orchestrator/packages/orchestrator/src/index.spec.ts
- orchestrator/packages/orchestrator/src/core/index.ts
- orchestrator/packages/core/tsconfig.json
9 tasks
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.
No description provided.