[v2] Work item source with dependencies + Bifrost adapter - #49
Merged
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR extends the ChangesWork item dependency support and Bifrost adapter
Sequence Diagram(s)sequenceDiagram
participant Source as BifrostWorkItemSource
participant Config as loadConfig/CredentialLoader
participant Client as BifrostHttpClient
participant Bifrost as Bifrost API
Source->>Config: loadConfig() + loadToken(url)
Config-->>Source: url, realm, token
Source->>Client: new BifrostHttpClient(url, realm, token)
loop watchWorkItems polling
Source->>Client: getReadyRunes()
Client->>Bifrost: GET /api/ready
Bifrost-->>Client: ready runes
Client-->>Source: runes list
Source->>Client: claimRune(id)
Client->>Bifrost: POST /api/claim-rune
Bifrost-->>Client: 200 or 409
Source->>Client: getRune(id)
Client->>Bifrost: GET /api/runes/:id
Bifrost-->>Client: rune detail
Source->>Source: mapToWorkItem(rune)
end
Source->>Client: createDraftWorkItem(input)
Client->>Bifrost: POST /api/create-rune
Bifrost-->>Client: created rune
Client-->>Source: rune id
Source->>Client: setDependency(id, dependsOnId)
Client->>Bifrost: POST /api/add-dependency
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Owner
Author
|
@CodeRabbit review |
Contributor
✅ Action performedReview finished.
|
![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 (5)
orchestrator-v2/packages/work-item-source-bifrost/src/bifrost-work-item-source.spec.ts (1)
408-423: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win"Adaptive polling" test doesn't exercise the adapter.
This test only re-implements backoff arithmetic inline; it never calls into
BifrostWorkItemSource, so it can't catch a regression in the actual backoff logic. Consider driving this via fake timers and asserting on the interval betweenfetchcalls to/api/readyduringwatchWorkItems(), matching how the "gating" test observes the fetch mock.🤖 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-v2/packages/work-item-source-bifrost/src/bifrost-work-item-source.spec.ts` around lines 408 - 423, The adaptive polling test currently duplicates the backoff math instead of exercising BifrostWorkItemSource, so update the "adaptive polling" spec to drive watchWorkItems() and observe real polling behavior. Use fake timers and the fetch mock, similar to the existing gating test, then assert on the interval between repeated /api/ready requests to verify exponential backoff and the maxPollInterval cap through the actual adapter logic.orchestrator-v2/packages/work-item-source-bifrost/src/bifrost-work-item-source.ts (1)
157-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate dependency-mapping logic.
The
{target_id, relationship} → {workItemId, type}mapping here is duplicated inmapToWorkItem(Lines 224-227). Extract a shared static helper to avoid drift if the shape changes.♻️ Suggested refactor
+ private static mapDependencies(dependencies: RuneDetail["dependencies"]): WorkItemDependency[] { + return dependencies.map((dep) => ({ + workItemId: dep.target_id, + type: dep.relationship, + })); + } + public async getDependencies(workItemId: string): Promise<WorkItemDependency[]> { const client = await this.#getClient(); const detail = await client.getRune(workItemId); - return detail.dependencies.map((dep) => ({ - workItemId: dep.target_id, - type: dep.relationship, - })); + return BifrostWorkItemSource.mapDependencies(detail.dependencies); }🤖 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-v2/packages/work-item-source-bifrost/src/bifrost-work-item-source.ts` around lines 157 - 164, The dependency mapping logic in getDependencies is duplicated with mapToWorkItem, so extract a shared static helper in BifrostWorkItemSource for converting each dependency’s target_id and relationship into { workItemId, type }. Update both getDependencies and mapToWorkItem to use that helper so the mapping stays consistent if the shape changes.orchestrator-v2/packages/work-item-source-bifrost/src/client/bifrost-http-client.spec.ts (1)
1-256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMinor coverage gaps.
Suite is thorough but doesn't cover
unclaimRune, thegetRunesuccess path, or timeout/abort behavior ofrequest().🤖 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-v2/packages/work-item-source-bifrost/src/client/bifrost-http-client.spec.ts` around lines 1 - 256, Add tests to cover the missing BifrostHttpClient paths: implement a success case for getRune, a request to unclaimRune, and timeout/abort behavior in request(). Use the existing BifrostHttpClient test suite and mockFetch setup in bifrost-http-client.spec.ts, asserting the expected endpoint, method/body, and that request() aborts or rejects appropriately when the timeout is exceeded.orchestrator-v2/packages/work-item-source-bifrost/src/client/bifrost-http-client.ts (2)
96-121: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTimeout aborts surface as generic AbortError.
When the 30s timeout fires,
fetchrejects with anAbortErrorthat isn't distinguished from other network failures, making it harder to diagnose Bifrost latency/timeouts vs. real API errors from logs.💡 Suggested fix
try { const response = await fetch(url, { ...options, headers: Object.assign( { "Content-Type": "application/json", Authorization: `Bearer ${this.token}`, "X-Bifrost-Realm": this.realm, }, options.headers, ), signal: controller.signal, }); + } catch (err) { + if ((err as Error).name === "AbortError") { + throw new Error(`Bifrost request timed out after ${this.timeout}ms: ${url}`); + } + throw err; + } finally {🤖 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-v2/packages/work-item-source-bifrost/src/client/bifrost-http-client.ts` around lines 96 - 121, The request logic in BifrostHttpClient.request treats all fetch aborts the same, so timeout-triggered AbortError is indistinguishable from other failures. Update the timeout path in request to mark when the AbortController fired due to the setTimeout and, in the catch handling around fetch, detect that case and log/throw a timeout-specific error message instead of a generic AbortError; keep the normal network/error handling unchanged for non-timeout failures.
122-136: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGeneric 409/404 mapping applies to every endpoint, not just claim/get.
request()hardcodes "Rune already claimed" for any 409 and "Rune not found" for any 404, but this helper backscreateRune,addDependency,updateRuneState,forgeRune,fulfillRune,failRune, andunclaimRunetoo. A 409 fromaddDependency(e.g. duplicate/cyclic edge) orcreateRunewould be mislabeled as a claim conflict, and a 404 fromupdateRuneState/addDependencyon an unknown target could be similarly misleading.Consider making the error mapping endpoint-aware (e.g. pass a status-message map into
request(), or map errors in each wrapper method).🤖 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-v2/packages/work-item-source-bifrost/src/client/bifrost-http-client.ts` around lines 122 - 136, The generic 409/404 handling in request() is too specific for all callers, since it always throws “Rune already claimed” or “Rune not found” even for endpoints like createRune, addDependency, updateRuneState, forgeRune, fulfillRune, failRune, and unclaimRune. Update request() in bifrost-http-client.ts to accept endpoint-specific status-to-message mapping or move the 409/404 translation into each wrapper method so the thrown error matches the operation that failed. Use the existing request() helper and the endpoint methods to keep the mapping accurate per call site.
🤖 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-v2/packages/work-item-source-bifrost/src/bifrost-work-item-source.spec.ts`:
- Around line 150-162: The detached async iteration over source.watchWorkItems()
in the bifrost-work-item-source.spec tests is never cancelled, while
watchWorkItems() itself is a perpetual poll loop, so it can keep running after
cleanup and interfere with later tests. Update the affected test blocks to
explicitly stop the iterator before cleanup by calling return() on the async
iterator or by wiring an AbortSignal into watchWorkItems(), and make sure this
is done in both test cases that use the void async loop.
In
`@orchestrator-v2/packages/work-item-source-bifrost/src/bifrost-work-item-source.ts`:
- Around line 127-134: The setState method in BifrostWorkItemSource is
swallowing persistence failures by catching and only logging, unlike the sibling
mutation methods. Remove the internal try/catch in setState so errors from
this.#getClient() or client.updateRuneState(...) propagate to the caller,
keeping behavior consistent with completeWorkItem, failWorkItem, pauseWorkItem,
startWorkItem, and setDependency.
- Around line 96-107: The backoff logic in BifrostWorkItemSource is inverted:
successful polling in the work-item loop should not always increase the delay,
while failures in the catch branch should. Update the polling flow around
BifrostWorkItemSource.mapToWorkItem and BifrostWorkItemSource.sleep so
pollInterval only grows when an error occurs and resets (or stays at
defaultPollInterval) when work is found, ensuring busy queues stay responsive
and repeated failures back off progressively.
- Around line 86-99: The post-claim fetch path in
BifrostWorkItemSource.claimAndYield leaves a rune claimed if client.getRune()
fails after client.claimRune() succeeds. Update this flow to compensate on fetch
failure by calling a release/unclaim method on the claimed rune before the error
is handled, and keep the existing 409 skip behavior intact. Use the existing
claimRune, getRune, and BifrostWorkItemSource mapping flow as the touchpoints so
a transient get failure does not strand the work item.
---
Nitpick comments:
In
`@orchestrator-v2/packages/work-item-source-bifrost/src/bifrost-work-item-source.spec.ts`:
- Around line 408-423: The adaptive polling test currently duplicates the
backoff math instead of exercising BifrostWorkItemSource, so update the
"adaptive polling" spec to drive watchWorkItems() and observe real polling
behavior. Use fake timers and the fetch mock, similar to the existing gating
test, then assert on the interval between repeated /api/ready requests to verify
exponential backoff and the maxPollInterval cap through the actual adapter
logic.
In
`@orchestrator-v2/packages/work-item-source-bifrost/src/bifrost-work-item-source.ts`:
- Around line 157-164: The dependency mapping logic in getDependencies is
duplicated with mapToWorkItem, so extract a shared static helper in
BifrostWorkItemSource for converting each dependency’s target_id and
relationship into { workItemId, type }. Update both getDependencies and
mapToWorkItem to use that helper so the mapping stays consistent if the shape
changes.
In
`@orchestrator-v2/packages/work-item-source-bifrost/src/client/bifrost-http-client.spec.ts`:
- Around line 1-256: Add tests to cover the missing BifrostHttpClient paths:
implement a success case for getRune, a request to unclaimRune, and
timeout/abort behavior in request(). Use the existing BifrostHttpClient test
suite and mockFetch setup in bifrost-http-client.spec.ts, asserting the expected
endpoint, method/body, and that request() aborts or rejects appropriately when
the timeout is exceeded.
In
`@orchestrator-v2/packages/work-item-source-bifrost/src/client/bifrost-http-client.ts`:
- Around line 96-121: The request logic in BifrostHttpClient.request treats all
fetch aborts the same, so timeout-triggered AbortError is indistinguishable from
other failures. Update the timeout path in request to mark when the
AbortController fired due to the setTimeout and, in the catch handling around
fetch, detect that case and log/throw a timeout-specific error message instead
of a generic AbortError; keep the normal network/error handling unchanged for
non-timeout failures.
- Around line 122-136: The generic 409/404 handling in request() is too specific
for all callers, since it always throws “Rune already claimed” or “Rune not
found” even for endpoints like createRune, addDependency, updateRuneState,
forgeRune, fulfillRune, failRune, and unclaimRune. Update request() in
bifrost-http-client.ts to accept endpoint-specific status-to-message mapping or
move the 409/404 translation into each wrapper method so the thrown error
matches the operation that failed. Use the existing request() helper and the
endpoint methods to keep the mapping accurate per call site.
🪄 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: 63397ff3-408b-41e0-b80d-946b57d9cad5
⛔ Files ignored due to path filters (1)
orchestrator-v2/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
orchestrator-v2/examples/lvl3/agents/cowsay/AGENT.mdorchestrator-v2/examples/lvl3/doSomething.tsorchestrator-v2/examples/lvl3/orchestrator.tsorchestrator-v2/examples/lvl3/package.jsonorchestrator-v2/examples/lvl3/runner.tsorchestrator-v2/packages/interfaces-work/src/index.tsorchestrator-v2/packages/interfaces-work/src/types.tsorchestrator-v2/packages/orchestrator/src/test-helpers.tsorchestrator-v2/packages/work-item-source-bifrost/package.jsonorchestrator-v2/packages/work-item-source-bifrost/src/bifrost-work-item-source.spec.tsorchestrator-v2/packages/work-item-source-bifrost/src/bifrost-work-item-source.tsorchestrator-v2/packages/work-item-source-bifrost/src/client/bifrost-http-client.spec.tsorchestrator-v2/packages/work-item-source-bifrost/src/client/bifrost-http-client.tsorchestrator-v2/packages/work-item-source-bifrost/src/config/config-loader.tsorchestrator-v2/packages/work-item-source-bifrost/src/config/credential-loader.tsorchestrator-v2/packages/work-item-source-bifrost/src/index.tsorchestrator-v2/packages/work-item-source-bifrost/src/types.tsorchestrator-v2/packages/work-item-source-bifrost/vite.config.ts
Comment on lines
+150
to
+162
| void (async (): Promise<void> => { | ||
| for await (const workItem of source.watchWorkItems()) { | ||
| workItems.push(workItem); | ||
| } | ||
| })(); | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 300)); | ||
|
|
||
| expect(workItems).toHaveLength(0); | ||
| expect(callCount).toBeGreaterThanOrEqual(1); | ||
|
|
||
| await cleanup(); | ||
| }, 10000); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the spec and the watchWorkItems implementation
git ls-files | rg 'orchestrator-v2/packages/work-item-source-bifrost/src/.*(\.spec\.ts|\.ts)$'
# Find references to watchWorkItems
rg -n "watchWorkItems|cleanup\(" orchestrator-v2/packages/work-item-source-bifrost/src -S
# Inspect the relevant test sections
sed -n '120,190p' orchestrator-v2/packages/work-item-source-bifrost/src/bifrost-work-item-source.spec.ts
printf '\n====\n'
sed -n '360,430p' orchestrator-v2/packages/work-item-source-bifrost/src/bifrost-work-item-source.spec.ts
# Inspect the implementation of watchWorkItems
rg -n "async \*|watchWorkItems\s*\(" orchestrator-v2/packages/work-item-source-bifrost/src -SRepository: devzeebo/bifrost
Length of output: 7542
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the implementation of watchWorkItems and surrounding helpers
sed -n '1,220p' orchestrator-v2/packages/work-item-source-bifrost/src/bifrost-work-item-source.ts
# Read the relevant test areas with line numbers for precise context
cat -n orchestrator-v2/packages/work-item-source-bifrost/src/bifrost-work-item-source.spec.ts | sed -n '95,170p'
printf '\n====\n'
cat -n orchestrator-v2/packages/work-item-source-bifrost/src/bifrost-work-item-source.spec.ts | sed -n '345,415p'Repository: devzeebo/bifrost
Length of output: 12689
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,240p' orchestrator-v2/packages/work-item-source-bifrost/src/client/bifrost-http-client.ts
printf '\n====\n'
rg -n "createTestSource|cleanup\s*=" orchestrator-v2/packages/work-item-source-bifrost/src/bifrost-work-item-source.spec.ts -n -A 20 -B 20Repository: devzeebo/bifrost
Length of output: 16480
Cancel the detached watchWorkItems() loops
The void (async () => { for await (...) { ... } })() loops in bifrost-work-item-source.spec.ts:150-162 and :390-404 are never stopped, while watchWorkItems() in bifrost-work-item-source.ts runs an infinite while (true) poll loop with no cancellation path. That can keep calling the next test’s global.fetch mock after cleanup(), causing flaky cross-test interference. Break the iterator explicitly (return()/AbortSignal) before cleanup, or stop after the expected assertion.
🤖 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-v2/packages/work-item-source-bifrost/src/bifrost-work-item-source.spec.ts`
around lines 150 - 162, The detached async iteration over
source.watchWorkItems() in the bifrost-work-item-source.spec tests is never
cancelled, while watchWorkItems() itself is a perpetual poll loop, so it can
keep running after cleanup and interfere with later tests. Update the affected
test blocks to explicitly stop the iterator before cleanup by calling return()
on the async iterator or by wiring an AbortSignal into watchWorkItems(), and
make sure this is done in both test cases that use the void async loop.
Comment on lines
+86
to
+99
| try { | ||
| await client.claimRune(rune.id); | ||
| } catch (error) { | ||
| if ((error as { status?: number }).status === 409) { | ||
| continue; | ||
| } | ||
| throw error; | ||
| } | ||
|
|
||
| const detail = await client.getRune(rune.id); | ||
| pollInterval = defaultPollInterval; | ||
|
|
||
| yield BifrostWorkItemSource.mapToWorkItem(detail, agentName); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and inspect the relevant section with line numbers.
ast-grep outline orchestrator-v2/packages/work-item-source-bifrost/src/bifrost-work-item-source.ts --view expanded || true
echo "-----"
sed -n '1,280p' orchestrator-v2/packages/work-item-source-bifrost/src/bifrost-work-item-source.ts | cat -n
echo "===== SEARCH: client methods ====="
rg -n "claimRune|unclaimRune|getRune|setState|pollInterval|defaultPollInterval" orchestrator-v2/packages -g '*.ts'Repository: devzeebo/bifrost
Length of output: 18535
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the Bifrost client type and any rune detail shapes relevant to acceptance criteria.
rg -n "interface .*Rune|type .*Rune|acceptance_criteria|acceptanceCriteria|satisfied" orchestrator-v2/packages -g '*.ts'Repository: devzeebo/bifrost
Length of output: 1765
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the implementation of setState and any surrounding error handling patterns.
rg -n "async setState|setState\\(" orchestrator-v2/packages -g '*.ts'Repository: devzeebo/bifrost
Length of output: 1147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the target file and related client/type definitions.
echo "=== bifrost-work-item-source.ts ==="
sed -n '1,280p' orchestrator-v2/packages/work-item-source-bifrost/src/bifrost-work-item-source.ts | cat -n
echo
echo "=== search relevant symbols ==="
rg -n "claimRune|unclaimRune|getRune|setState|pollInterval|defaultPollInterval|acceptance_criteria|acceptanceCriteria|satisfied" orchestrator-v2/packages -g '*.ts'Repository: devzeebo/bifrost
Length of output: 17773
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' orchestrator-v2/packages/work-item-source-bifrost/src/client/bifrost-http-client.ts | cat -n
echo "-----"
sed -n '1,120p' orchestrator-v2/packages/work-item-source-bifrost/src/types.ts | cat -nRepository: devzeebo/bifrost
Length of output: 6977
Claimed runes need a compensating release on post-claim fetch failure orchestrator-v2/packages/work-item-source-bifrost/src/bifrost-work-item-source.ts:86-99 — claimRune() succeeds before getRune(), and any fetch error falls into the outer catch, which only logs and sleeps; that leaves the rune claimed and unavailable to other workers. Add a retry or unclaimRune() in this path so a transient failure doesn’t strand the work item.
🤖 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-v2/packages/work-item-source-bifrost/src/bifrost-work-item-source.ts`
around lines 86 - 99, The post-claim fetch path in
BifrostWorkItemSource.claimAndYield leaves a rune claimed if client.getRune()
fails after client.claimRune() succeeds. Update this flow to compensate on fetch
failure by calling a release/unclaim method on the claimed rune before the error
is handled, and keep the existing 409 skip behavior intact. Use the existing
claimRune, getRune, and BifrostWorkItemSource mapping flow as the touchpoints so
a transient get failure does not strand the work item.
Comment on lines
+96
to
+107
| pollInterval = defaultPollInterval; | ||
|
|
||
| yield BifrostWorkItemSource.mapToWorkItem(detail, agentName); | ||
| } | ||
|
|
||
| pollInterval = Math.min(pollInterval * 2, maxPollInterval); | ||
| const jitter = pollInterval * 0.2 * (Math.random() * 2 - 1); | ||
| await BifrostWorkItemSource.sleep(pollInterval + jitter); | ||
| } catch (error) { | ||
| console.error(error); | ||
| await BifrostWorkItemSource.sleep(pollInterval); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Adaptive backoff logic doesn't behave as intended.
pollInterval is reset to defaultPollInterval per processed rune (Line 96), but Line 101 unconditionally doubles it right after the loop regardless of whether work was found, so a busy queue still gets progressively slower polling. Meanwhile the error branch (104-107) never grows pollInterval, so repeated failures retry at a flat interval instead of backing off — the opposite of where backoff is actually needed.
🔧 Suggested fix
+ let foundWork = false;
for (const rune of readyRunes) {
...
- pollInterval = defaultPollInterval;
+ foundWork = true;
yield BifrostWorkItemSource.mapToWorkItem(detail, agentName);
}
- pollInterval = Math.min(pollInterval * 2, maxPollInterval);
+ pollInterval = foundWork
+ ? defaultPollInterval
+ : Math.min(pollInterval * 2, maxPollInterval);
const jitter = pollInterval * 0.2 * (Math.random() * 2 - 1);
await BifrostWorkItemSource.sleep(pollInterval + jitter);
} catch (error) {
console.error(error);
+ pollInterval = Math.min(pollInterval * 2, maxPollInterval);
await BifrostWorkItemSource.sleep(pollInterval);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pollInterval = defaultPollInterval; | |
| yield BifrostWorkItemSource.mapToWorkItem(detail, agentName); | |
| } | |
| pollInterval = Math.min(pollInterval * 2, maxPollInterval); | |
| const jitter = pollInterval * 0.2 * (Math.random() * 2 - 1); | |
| await BifrostWorkItemSource.sleep(pollInterval + jitter); | |
| } catch (error) { | |
| console.error(error); | |
| await BifrostWorkItemSource.sleep(pollInterval); | |
| } | |
| let foundWork = false; | |
| for (const rune of readyRunes) { | |
| ... | |
| foundWork = true; | |
| yield BifrostWorkItemSource.mapToWorkItem(detail, agentName); | |
| } | |
| pollInterval = foundWork | |
| ? defaultPollInterval | |
| : Math.min(pollInterval * 2, maxPollInterval); | |
| const jitter = pollInterval * 0.2 * (Math.random() * 2 - 1); | |
| await BifrostWorkItemSource.sleep(pollInterval + jitter); | |
| } catch (error) { | |
| console.error(error); | |
| pollInterval = Math.min(pollInterval * 2, maxPollInterval); | |
| await BifrostWorkItemSource.sleep(pollInterval); | |
| } |
🤖 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-v2/packages/work-item-source-bifrost/src/bifrost-work-item-source.ts`
around lines 96 - 107, The backoff logic in BifrostWorkItemSource is inverted:
successful polling in the work-item loop should not always increase the delay,
while failures in the catch branch should. Update the polling flow around
BifrostWorkItemSource.mapToWorkItem and BifrostWorkItemSource.sleep so
pollInterval only grows when an error occurs and resets (or stays at
defaultPollInterval) when work is found, ensuring busy queues stay responsive
and repeated failures back off progressively.
Comment on lines
+127
to
+134
| public async setState(workItemId: string, state: Record<string, unknown>): Promise<void> { | ||
| try { | ||
| const client = await this.#getClient(); | ||
| await client.updateRuneState(workItemId, state); | ||
| } catch (err) { | ||
| console.error(`Failed to update state for work item ${workItemId}:`, err); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
setState silently swallows errors, unlike sibling mutation methods.
completeWorkItem, failWorkItem, pauseWorkItem, startWorkItem, and setDependency all propagate errors to the caller, but setState catches and only logs. Callers have no way to know a state persist failed, which is risky if downstream orchestration logic relies on state being durably saved.
🤖 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-v2/packages/work-item-source-bifrost/src/bifrost-work-item-source.ts`
around lines 127 - 134, The setState method in BifrostWorkItemSource is
swallowing persistence failures by catching and only logging, unlike the sibling
mutation methods. Remove the internal try/catch in setState so errors from
this.#getClient() or client.updateRuneState(...) propagate to the caller,
keeping behavior consistent with completeWorkItem, failWorkItem, pauseWorkItem,
startWorkItem, and setDependency.
This was referenced Jul 8, 2026
Merged
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
Closes #34
Closes #40
WorkItemSourceinterface with draft/live lifecycle (createDraftWorkItem,startWorkItem) and dependency graph methods (setDependency,getDependencies), keeping dependency resolution inside the source rather than the orchestrator.@bifrost-ai/work-item-source-bifrost, a self-contained v2 port of the v1 Bifrost task source that maps runes to work items via the Bifrost HTTP API — including adaptive polling, agent tag filtering, draft creation, forge-to-live promotion, and dependency edges.Test plan
vp testinorchestrator-v2/packages/work-item-source-bifrost— dependency mapping, draft/live API calls, agent tag filtering, polling, complete/fail/pause/setStatevp testinorchestrator-v2/packages/work-item-source-bifrost— HTTP client unit testsvp check && vp testfromorchestrator-v2/root to confirm no regressions across packages