Orchestrator#19
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Bash CLAUDE hook dispatcher and helpers; new CLAUDE skills and hook scripts; introduces a TypeScript Orchestrator monorepo (core, engine, cli, task-sources) with types, implementations, tests, and build configs; adds Bifrost HTTP TaskSource and in-memory TaskSource; UI/type churn and many PRDs/docs. ChangesCLAUDE Hooks & Dispatcher
Orchestrator core (single DAG)
Bifrost Task Source (independent DAG)
In-memory Task Source (independent DAG)
CLI, Test Engine, and Utilities
Bifrost UI & Tooling (formatting/typing churn + config)
Packaging, Build, and Workspace wiring
PRDs, docs, and examples
Removals / deprecations
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant Config as ConfigLoader
participant Git as Git Root
participant TaskSource as BifrostTaskSource
participant Orch as Orchestrator
participant Agent as AGENT.md Parser
participant Validator as Validator
participant Hook as Hook Executor
participant Engine as Engine
CLI->>Config: loadConfig(projectDir)
Config->>Git: resolveGitRoot()
Config-->>CLI: OrchestratorConfig
TaskSource->>Orch: yield Task
Orch->>Agent: parseAgentDefinition(AGENT.md)
Agent-->>Orch: AgentDefinition
Orch->>Validator: validateTaskState(task.taskState, agent.template.parameters)
Validator-->>Orch: ValidationResult
alt validation fails
Orch->>TaskSource: failTask(task.id, error)
else validation succeeds
Orch->>Hook: executeHooks(Start)
Hook-->>Orch: HookResult[]
Orch->>Engine: execute(engineContext)
Engine-->>Orch: EngineResult
Orch->>Hook: executeHooks(Stop)
Hook-->>Orch: HookResult[]
Orch->>TaskSource: completeTask(task.id)
end
sequenceDiagram
participant TS as BifrostTaskSource
participant CL as ConfigLoader
participant CR as CredentialLoader
participant Home as ~/.config/bifrost
participant API as Bifrost API
TS->>CL: load(projectRoot)
CL-->>TS: { url, realm }
TS->>CR: loadToken(url)
CR->>Home: read credentials.yaml
Home-->>CR: credentials[url] -> token
CR-->>TS: token
TS->>API: GET /api/ready (auth header)
API-->>TS: ReadyRune[]
TS->>API: POST /api/claim-rune { id }
alt 204
API-->>TS: 204
TS->>API: GET /api/rune/<id>
API-->>TS: RuneDetail
TS-->>Orch: mapped Task
else 409
API-->>TS: 409 (skip)
end
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
orchestrator/last-statusline (1)
1-2:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAccidentally committed Claude Code session artifact — delete this file
This file contains an AI tool's internal rate-limit response (
TIME_LIMIT,TOKENS_LIMIT, usage percentages, reset timestamps, per-model consumption forsearch-prime/web-reader/zread). It is not part of any data model, configuration, or test fixture defined in the orchestrator domain. Keeping it in the repo:
- Exposes internal API usage details to anyone who views the repository history.
- Will cause spurious diffs on every session where the tool's state changes.
- Misleads future contributors about what files are part of the project.
Delete the file and add it to
.gitignore:🛠️ Remediation
-{"code":200,"msg":"Operation successful",...}Add to
.gitignore(ororchestrator/.gitignore):last-statusline🤖 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/last-statusline` around lines 1 - 2, Remove the accidentally committed AI session artifact file named "last-statusline" from the repository and add an entry for last-statusline to the appropriate .gitignore (e.g., orchestrator/.gitignore or the repo root .gitignore) so it is not tracked in future commits; ensure you remove the file from history or create a follow-up cleanup commit if necessary and verify git status shows it as ignored after deletion.
🟠 Major comments (18)
.claude/skills/typescript/SKILL.md-22-22 (1)
22-22:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace
intwithnumberin the GOOD example at line 22
intis not a valid TypeScript primitive type. The correct type isnumber. Since this appears in a "GOOD" example used for skill guidance, it should demonstrate correct TypeScript syntax.Fix
export type MyTypeName = { prop: string; - value: int; + value: number;🤖 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 @.claude/skills/typescript/SKILL.md at line 22, Update the GOOD example by replacing the invalid TypeScript type token "int" with the correct primitive "number" so the example field declaration (e.g., the "value" property shown as `value: int;`) uses valid TypeScript syntax (`value: number;`) and accurately demonstrates proper typing.orchestrator/packages/core/src/memory-task-state-store.ts-17-19 (1)
17-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
loadTaskStatereturns a mutable reference to internal store state
Map.get()returns the stored value directly. Any caller that mutates the returned object will silently corrupt the store without callingsaveTaskState, breaking the isolation contract that hooks depend on.const state = await store.loadTaskState('task-1') state['snapshotTests'] = {} // mutates the Map entry — no saveTaskState needed!This also interacts with the shallow spread in
saveTaskState(line 25): nested objects are still shared between the caller's original and the stored copy, so mutations propagate in both directions.The cleanest fix is to use
structuredCloneconsistently — both on write (to protect from caller mutation after save) and on read (to protect from caller mutation after load):🛠️ Proposed fix
async loadTaskState(taskId: string): Promise<Record<string, unknown> | null> { - return this.#store.get(taskId) ?? null + const stored = this.#store.get(taskId) + return stored != null ? structuredClone(stored) : null } async saveTaskState( taskId: string, taskState: Record<string, unknown> ): Promise<boolean> { - this.#store.set(taskId, { ...taskState }) + this.#store.set(taskId, structuredClone(taskState)) return true } async initializeTaskState( taskId: string, initialState: Record<string, unknown> ): Promise<boolean> { if (!this.#store.has(taskId)) { - this.#store.set(taskId, { ...initialState }) + this.#store.set(taskId, structuredClone(initialState)) return true } return false }
structuredClonehandles all serializable data, which covers every value typetaskStateis specified to hold. If non-serializable values (functions, class instances) ever need to be stored, you'll need a custom deep-clone; but the PRD'sRecord<string, unknown>intent is plainly data-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 `@orchestrator/packages/core/src/memory-task-state-store.ts` around lines 17 - 19, loadTaskState currently returns the direct Map entry which exposes internal state to external mutation; update loadTaskState and saveTaskState to use structuredClone when reading from and writing to the internal `#store` Map so callers get/put deep-cloned data (protecting nested objects), e.g. clone the value returned by this.#store.get(taskId) in loadTaskState and clone taskState before storing it in saveTaskState to ensure isolation.orchestrator/packages/core/src/memory-task-source.ts-44-89 (1)
44-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
watchTasks()yields at most one task per call;maxIterationscap is a test concern in production code.Two related issues:
The generator
returns after yielding the first task (line 76). A caller expecting a continuous stream will silently stop receiving tasks after the first one. If single-task-per-call is intentional, it should be clearly documented on the method.
maxIterations = 10with the comment "Prevent infinite loops in tests" is a test guard leaking into production code. If the caller never callscompleteTask/failTask, the generator silently terminates after 10 × 50 ms = 500 ms, which is surprising in production. Consider removing the cap and letting callers control the lifecycle, or at least using a clearly-named option rather than a magic constant.🤖 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/core/src/memory-task-source.ts` around lines 44 - 89, The watchTasks generator currently returns after yielding the first task and enforces a hard-coded maxIterations = 10; update watchTasks so it does not return after the first yield (allow continuous streaming by removing the premature return and continuing the while loop) and remove or replace the magic test-only maxIterations cap with a configurable option (e.g., a parameter or class-level setting) so production callers control lifecycle; reference the watchTasks method and the `#pending/`#claimed sets for locating the yield/return, and mention completeTask/failTask as the expected ways callers will change task state to stop the generator if needed.orchestrator/packages/task-source/src/api-task-source.ts-3-8 (1)
3-8:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
timeoutconfig is documented but never applied to anyfetchcall.
APITaskSourceConfig.timeouthas a default of 30000 ms and is set in the constructor, but none of thefetchinvocations in#fetchTasks,getTaskDetail,completeTask, orfailTaskapply it. Requests can hang indefinitely, contradicting the documented intent.Use
AbortSignal.any([this.#abortController!.signal, AbortSignal.timeout(this.#config.timeout!)])(or a combined signal) on each fetch call.🤖 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/src/api-task-source.ts` around lines 3 - 8, APITaskSourceConfig's timeout isn't wired into network calls: update every fetch in the class (including `#fetchTasks`, getTaskDetail, completeTask, failTask) to pass a combined AbortSignal that respects both the instance `#abortController` and the configured timeout (e.g., use AbortSignal.any([this.#abortController!.signal, AbortSignal.timeout(this.#config.timeout!)])); ensure the constructor still sets the default 30000ms on `#config.timeout` and use that value when creating the combined signal for each fetch's signal option so requests are aborted after the configured timeout or when the instance is aborted.orchestrator/packages/task-source/src/api-task-source.ts-36-37 (1)
36-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-entering
watchTasks()silently orphans the previousAbortController.If
watchTasks()is called while already running, the existing#abortControlleris overwritten without aborting it. The previous generator loses its stop handle and can never be cancelled viaclose().Guard against re-entry:
🐛 Proposed fix
async *watchTasks(): AsyncGenerator<Task> { + if (this.#abortController && !this.#abortController.signal.aborted) { + throw new Error('watchTasks() is already running; call close() first') + } this.#abortController = new AbortController()🤖 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/src/api-task-source.ts` around lines 36 - 37, watchTasks() currently overwrites this.#abortController and orphaned generators cannot be cancelled; before assigning a new AbortController in watchTasks(), check if this.#abortController exists and either call this.#abortController.abort() (and optionally this.#abortController.signal.close/cleanup if used) to cancel the previous generator or throw an error to prevent re-entry; update the logic around the AsyncGenerator creation (watchTasks) so any prior controller is properly aborted/cleaned up before replacing it, ensuring close() on the previous generator will work as expected.orchestrator/packages/core/src/repo-installer.ts-43-49 (1)
43-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
stat()catch swallows all errors, masking real failures.Currently, any error from
stat()— includingEACCES,ENOTDIR, or I/O errors — is silently swallowed and treated as "file doesn't exist." This means a permission problem or broken directory will silently skip script installation with no user-visible indication.Narrow the catch to
ENOENTonly:🐛 Proposed fix
- try { - await stat(targetPath) - console.log(`Already present: ${targetPath}`) - continue - } catch { - // File doesn't exist, proceed with installation - } + try { + await stat(targetPath) + console.log(`Already present: ${targetPath}`) + continue + } catch (err: unknown) { + if ((err as NodeJS.ErrnoException).code !== 'ENOENT') { + throw err + } + // File doesn't exist, proceed with installation + }🤖 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/core/src/repo-installer.ts` around lines 43 - 49, The try/catch around stat(targetPath) currently swallows all errors; change it so only ENOENT (file-not-found) is treated as "not present" and other errors are rethrown or surfaced — inspect the error.code after stat(targetPath) and if it's not 'ENOENT' throw the error (or call the existing error path) so permission/I/O errors aren't ignored; update the block around stat(targetPath) in repo-installer.ts to only suppress ENOENT and propagate everything else.orchestrator/packages/core/src/handlebars-renderer.ts-22-28 (1)
22-28:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Handlebars.compilecan throw on malformed template syntax — unhandled.
Handlebars.compilethrows aHandlebars.Exception(e.g., for unclosed{{tags or parse errors inpromptBody). SincepromptBodycomes from user-authored AGENT.md files, malformed templates will produce an unhandled exception that propagates up to the orchestrator without a descriptive error.🛡️ Proposed fix
- const template = Handlebars.compile(promptBody, { - strict: false, - knownHelpers: { if: true } - }) - return template(taskState) + let template: HandlebarsTemplateDelegate + try { + template = Handlebars.compile(promptBody, { + strict: false, + knownHelpers: { if: true }, + }) + } catch (err) { + throw new Error( + `Failed to compile prompt template: ${err instanceof Error ? err.message : String(err)}` + ) + } + return template(taskState)🤖 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/core/src/handlebars-renderer.ts` around lines 22 - 28, Wrap the Handlebars.compile + template(taskState) call in a try/catch to handle Handlebars.Exception (and generic errors) raised for malformed templates: try compiling promptBody with Handlebars.compile(...) and rendering with template(taskState), and in the catch produce a clear, descriptive error that includes context (e.g., that the AGENT.md prompt/template is malformed, the original Handlebars error.message, and identifying info such as the prompt id or name if available) before rethrowing or returning a structured error; update the code around Handlebars.compile, promptBody, and the template(taskState) return to implement this error handling..claude/hooks/dispatch-3-3 (1)
3-3:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
set -ewill exit silently if the hook script is absent or non-executable.With
set -e, if the target path on line 21 does not exist or is not executable, the dispatcher exits with a non-zero code but emits no user-visible error. Claude's hook infrastructure will receive a failure with no diagnostic information.Add an explicit existence check before executing:
🛠️ Proposed fix
+target="$CLAUDE_PROJECT_DIR/.claude/skills/$1/hooks/$hook_event.d/$2" +if [[ ! -x "$target" ]]; then + echo "dispatch: hook not found or not executable: $target" >&2 + exit 1 +fi +exec "$target" -$CLAUDE_PROJECT_DIR/.claude/skills/$1/hooks/$(get_field hook_event_name).d/$2🤖 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 @.claude/hooks/dispatch at line 3, The dispatcher currently uses `set -e` which can cause silent failures when the hook executable is missing or non-executable; add an explicit check of the target hook path before executing it: test that the target file exists and is executable (e.g., [ -e "$HOOK_PATH" ] && [ -x "$HOOK_PATH" ]), and if not print a clear error to stderr (using echo >&2 or similar) and exit with a non-zero code; perform this check in the dispatcher script immediately before the exec/command invocation that runs the hook so the user gets a diagnostic instead of a silent failure while leaving `set -e` in place if desired.orchestrator/packages/core/vite.config.ts-20-26 (1)
20-26:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the dead
output.globalsconfiguration—it has no effect for ES-only builds.The
output.globalsconfig is only consumed for UMD/IIFE formats. Sinceformats: ['es']specifies an ES-only build, this object is silently ignored and creates misleading configuration that future maintainers may mistakenly rely on.Note:
gray-matteris properly declared as a dependency inpackage.jsonand actively used in the source code, so no dependency issue exists.🛠️ Proposed fix
rollupOptions: { external: ['@orchestrator/engine', '@orchestrator/task-source', 'gray-matter'], - output: { - globals: { - '@orchestrator/engine': 'Engine', - '@orchestrator/task-source': 'TaskSource', - 'gray-matter': 'grayMatter', - }, - }, },🤖 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/core/vite.config.ts` around lines 20 - 26, The output.globals block is dead for ES-only builds (when formats: ['es'] is used); remove the output.globals object entirely from the Vite/Rollup config to avoid misleading config. Locate the Vite config where output.globals is defined (look for the output.globals property and the surrounding formats: ['es'] setting) and delete that property so the build config only contains relevant options.orchestrator/packages/cli/src/config.spec.ts-103-120 (1)
103-120:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFallback test doesn't exercise the actual fallback path.
The test passes the full file path
/home/user/.orchestrator.yamlasprojectDirand the mock always resolves immediately — it never simulates the "project config not found" scenario that would trigger a home-directory fallback. A proper fallback test would:
- Make the first
readFilecall reject withENOENT.- Have the second call (for the home-dir path) resolve with the YAML content.
As written, if
loadConfigbuilds its path aspath.join(projectDir, '.orchestrator.yaml'), the effective path becomes/home/user/.orchestrator.yaml/.orchestrator.yaml, which means the test may be accidentally green rather than validating the stated behavior.🔧 Proposed correct fallback test structure
- it('should load from home directory when not in project', async () => { - // Test loading config from home directory as fallback - const yamlContent = ` + it('should load from home directory when project config is not found', async () => { + const yamlContent = ` orchestrate: task_source: type: memory engine: type: test task_state_store: type: memory ` - - vi.mocked(readFile).mockResolvedValue(yamlContent) - - const config = await loadConfig('/home/user/.orchestrator.yaml') - + // First call (project dir) throws ENOENT, second call (home dir) resolves + vi.mocked(readFile) + .mockRejectedValueOnce(Object.assign(new Error('ENOENT'), { code: 'ENOENT' })) + .mockResolvedValueOnce(yamlContent) + + const config = await loadConfig('/some/project') + expect(config.orchestrate.task_source.type).toBe('memory') })🤖 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/cli/src/config.spec.ts` around lines 103 - 120, The test for loadConfig doesn't simulate the missing project config; change it to pass a project directory (e.g., '/home/user') not the full file path, then mock readFile so the first call rejects with an ENOENT error and the second call resolves with yamlContent; specifically use vi.mocked(readFile).mockRejectedValueOnce(new Error('ENOENT')) (or set code='ENOENT') followed by .mockResolvedValueOnce(yamlContent) so loadConfig's project-path read fails and the home-directory fallback path is exercised.orchestrator/packages/core/src/repo-installer.spec.ts-5-5 (1)
5-5:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset mocked call history between tests
With module-level mocks, call counts can leak across tests. In this file, tests that assert
writeFile/console.logcalls can pass because of earlier test invocations. Add abeforeEachcleanup to enforce isolation.Suggested change
vi.mock('node:fs/promises') describe('Repo Script Installer - US-5', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + describe('First run installs repo scripts into working repository', () => {Also applies to: 48-71, 73-97
🤖 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/core/src/repo-installer.spec.ts` at line 5, The module-level mock vi.mock('node:fs/promises') causes mocked call history to leak between tests; add a beforeEach in repo-installer.spec.ts that clears mock call history (e.g. beforeEach(() => { vi.clearAllMocks(); })) so assertions against writeFile and console.log are isolated between tests; if you need to also restore mocked implementations, use vi.resetAllMocks() or vi.restoreAllMocks() instead of clearAllMocks().orchestrator/packages/core/src/orchestrator.ts-150-172 (1)
150-172:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
engine.sendFollowUpis called and thenengine.executeis called again in the same follow-up cycle; whensendFollowUpdoesn't exist, the follow-up message is silently lost.Case 1 —
sendFollowUpexists: after callingsendFollowUp(followUpMessage)at line 153, the loop continues to the next iteration, which callsengine.execute({...engineContext})at line 103 again. The engine is therefore invoked twice per follow-up cycle (once viasendFollowUpand once viaexecute), likely re-running the original prompt and wasting tokens.Case 2 —
sendFollowUpabsent: the else branch at lines 169–172 does nothing. The next iteration callsengine.execute({...engineContext})with the identical original context — the stop-hook's feedback (followUpMessage) is never delivered to the engine, so the agent has no way to correct itself.Both cases suggest the follow-up dispatch path is incomplete. The intended design should be clarified — either
sendFollowUpreplaces theexecutecall for follow-up iterations, orfollowUpMessagemust be merged intoengineContextbefore the nextexecutecall.🤖 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/core/src/orchestrator.ts` around lines 150 - 172, The follow-up path is flawed: when engine.sendFollowUp exists it is called but the loop still falls through to engine.execute (causing duplicate invocations), and when sendFollowUp is absent the followUpMessage is dropped; fix by making follow-up delivery explicit — if engine.sendFollowUp is defined, call it and then skip the next engine.execute (e.g., set a flag or continue to the loop start without re-calling execute) so the follow-up is delivered only via sendFollowUp, and if engine.sendFollowUp is not defined merge/apply followUpMessage into engineContext (or otherwise inject it into the inputs used by engine.execute) before the next call so the follow-up isn't lost; use the existing symbols engine.sendFollowUp, followUpMessage, engineContext and preserve/merge telemetry updates (totalTelemetry, numTurns) as currently done.orchestrator/packages/core/src/orchestrator.ts-131-143 (1)
131-143:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFatal stop-hook result is silently dropped when preceded by a
needsFollowUpresult.The for-loop at line 132 iterates stop-hook results in declaration order. When the first result has
needsFollowUp = true, the loopbreaks immediately (line 136) before checking any subsequent results. A later hook withfatal = trueis never inspected, so the task is not failed and the error is not surfaced.🐛 Proposed fix — scan all results before deciding
for (const hook of stopHookResults) { + // Fatal takes precedence over follow-up + if (hook.fatal) { + await taskSource.failTask(task.id, `Stop hook ${hook.hookName} failed: ${hook.stderr}`) + return { outcome: 'failed', error: hook.stderr } + } if (hook.needsFollowUp) { - needsFollowUp = true - followUpMessage = hook.stdout - break - } - - // FR-14 step 13: If any Stop hook exits with code 2: mark UoW as failed - if (hook.fatal) { - await taskSource.failTask(task.id, `Stop hook ${hook.hookName} failed: ${hook.stderr}`) - return { outcome: 'failed', error: hook.stderr } + if (!needsFollowUp) { + needsFollowUp = true + followUpMessage = hook.stdout + } } }🤖 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/core/src/orchestrator.ts` around lines 131 - 143, The loop over stopHookResults currently breaks as soon as it finds a hook with needsFollowUp, which can skip later hooks that may have hook.fatal=true; update the logic in the loop that processes stopHookResults so you always inspect every hook: if you encounter the first needsFollowUp set needsFollowUp=true and set followUpMessage (but do NOT break), and if you encounter any hook.fatal call taskSource.failTask(task.id, `Stop hook ${hook.hookName} failed: ${hook.stderr}`) and immediately return the failed outcome; ensure the loop uses the existing variables stopHookResults, needsFollowUp, followUpMessage and invokes taskSource.failTask and returns { outcome: 'failed', error: hook.stderr } when fatal is found.orchestrator/packages/core/src/orchestrator.ts-96-177 (1)
96-177:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExhausting the 10-iteration follow-up cap still marks the task as
completed.When
maxFollowUpsreaches 0, the while loop exits withoutbreaking on a clean success path. Execution falls through to line 177 (taskSource.completeTask) and returns{ outcome: 'completed' }. If the agent was in a persistent follow-up loop that never resolved, the task should be marked ashaltedorfailed, not completed.🐛 Proposed fix
+ let followUpExhausted = false while (maxFollowUps-- > 0) { // ... existing loop body ... if (!needsFollowUp) { break } + if (maxFollowUps === 0) { + followUpExhausted = true + } // ... } + if (followUpExhausted) { + await taskSource.failTask(task.id, 'Max follow-up iterations reached without resolution') + return { outcome: 'halted', error: 'Max follow-up iterations reached without resolution' } + } + await taskSource.completeTask(task.id)Note: the simpler alternative is to track whether the loop broke cleanly via a flag set inside the
if (!needsFollowUp) { break }branch.🤖 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/core/src/orchestrator.ts` around lines 96 - 177, The loop can exit due to exhausting maxFollowUps and still mark the task completed; add a break-detection flag (e.g., brokeCleanly = false) set to true in the if (!needsFollowUp) { break } branch inside the loop (or inspect maxFollowUps after the loop) and after the loop decide outcome: if brokeCleanly then proceed to await taskSource.completeTask(task.id) and return { outcome: 'completed' }, otherwise call taskSource.failTask or taskSource.haltTask with a clear message about max follow-ups reached and return { outcome: 'halted' } (update any telemetry/lastMessage handling accordingly). Ensure you modify the loop logic around maxFollowUps, the needsFollowUp/followUp handling and the post-loop taskSource.completeTask/return path so exhausted follow-ups do not mark the task as completed.prd-lvl3-agent-definition-v4.md-576-585 (1)
576-585:⚠️ Potential issue | 🟠 MajorPRD's
HookSpecdefinition is incorrect and misleading.The PRD documents
HookSpecwith fieldsspecPathandisRepoScript, but neither belongs there:
specPathdoesn't exist anywhere in the codebase (phantom field).isRepoScriptbelongs toAgentWithRepoScripts(used byrepo-installer.ts), notHookSpec.- The actual
HookSpecintypes.tscorrectly includestimeout?, which is missing from the PRD.Update the PRD to document
HookSpecas:{ name: string; scriptPath: string; timeout?: number }to match the implementation.🤖 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 `@prd-lvl3-agent-definition-v4.md` around lines 576 - 585, The PRD's HookSpec block is incorrect — replace the documented shape to match the implementation by removing the phantom specPath and the misplaced isRepoScript, and instead include the optional timeout property; update the HookSpec definition in the PRD to read { name: string; scriptPath: string; timeout?: number } to match the HookSpec type in types.ts and avoid mixing in properties that belong to AgentWithRepoScripts (used by repo-installer.ts).orchestrator/packages/engine/src/test-engine.ts-43-51 (1)
43-51:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
mockStatscurrently replaces defaults instead of overriding them.Line 43 uses
??, so a partialmockStatscan drop required telemetry fields entirely. Merge defaults with overrides to keep a completestatspayload.Suggested fix
- const stats: EngineResult['stats'] = this.#config.mockStats ?? { + const defaultStats: EngineResult['stats'] = { durationMs: this.#config.simulateDelay ?? 10, inputTokens: 100, outputTokens: 50, cacheReadTokens: 10, cacheCreationTokens: 5, totalCostUsd: 0.005, numTurns: 1 } + const stats: EngineResult['stats'] = { + ...defaultStats, + ...(this.#config.mockStats ?? {}) + }🤖 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/engine/src/test-engine.ts` around lines 43 - 51, The stats construction currently uses the nullish coalescing operator with this.#config.mockStats so a partial mockStats can omit required fields; change it to merge defaults with overrides instead (e.g., build a defaultStats object then spread ...defaultStats and ...this.#config.mockStats) so EngineResult['stats'] contains all fields; ensure durationMs still uses this.#config.simulateDelay as the default when mockStats.durationMs is undefined and reference the same identifiers (stats, this.#config.mockStats, this.#config.simulateDelay) when applying the merge.orchestrator/packages/core/src/hook-executor.ts-64-66 (1)
64-66:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle unexpected exit codes as fatal instead of proceeding.
At Lines 64-66,
shouldProceedis true for any code except2. That means unknown failure codes (e.g.,127) are treated as recoverable. Restrict proceed to known success/recoverable codes only (0/1).Suggested fix
- const fatal = exitCode === 2 - const shouldProceed = exitCode !== 2 + const isKnown = exitCode === 0 || exitCode === 1 || exitCode === 2 + const fatal = exitCode === 2 || !isKnown + const shouldProceed = exitCode === 0 || exitCode === 1 const needsFollowUp = lifecycle === 'Stop' && exitCode === 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 `@orchestrator/packages/core/src/hook-executor.ts` around lines 64 - 66, The current logic treats any exitCode !== 2 as recoverable; change it to only allow proceeding for known good/recoverable codes (0 and 1): set shouldProceed to (exitCode === 0 || exitCode === 1), make fatal true for any other unexpected code (e.g., fatal = exitCode !== 0 && exitCode !== 1), and keep needsFollowUp as lifecycle === 'Stop' && exitCode === 1 so follow-up behavior for exitCode 1 remains unchanged; update the logic around the variables fatal, shouldProceed, needsFollowUp and any downstream checks that use them to rely on the new definitions.orchestrator/packages/core/src/hook-executor.ts-84-98 (1)
84-98:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
timedOutshould only be true for actual timeout errors.Line 97 sets
timedOut: truefor every exception path, which mislabels non-timeout failures and can skew retry/diagnostics logic.Suggested fix
} catch (error) { const durationMs = Date.now() - startTime + const message = error instanceof Error ? error.message : String(error) + const timedOut = + error instanceof Error && + (error.name === 'TimeoutError' || + /timeout|timed out|ETIMEDOUT/i.test(message)) // Hook execution exception - treat as fatal (exit code 2) results.push({ hookName: hook.name, exitCode: 2, stdout: '', - stderr: error instanceof Error ? error.message : String(error), + stderr: message, durationMs, shouldProceed: false, fatal: true, needsFollowUp: false, - timedOut: true + timedOut })🤖 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/core/src/hook-executor.ts` around lines 84 - 98, The catch block in hook-executor.ts always sets timedOut: true in the results.push payload, incorrectly flagging non-timeout errors; change it to detect timeout errors (e.g., inspect error.name === 'TimeoutError' or error.code === 'ETIMEDOUT' or error.name === 'AbortError', or whatever timeout sentinel your executor throws) into a boolean like isTimeout, and use that for timedOut instead of hardcoding true in the results object pushed by results.push (the object created in the catch branch for hook.name).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e7bdf1ef-d341-48a7-8c80-8c67ac25e9e8
⛔ Files ignored due to path filters (2)
orchestrator/package-lock.jsonis excluded by!**/package-lock.jsonorchestrator/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (118)
.claude/hooks/dispatch.claude/settings.json.claude/skills/node/SKILL.md.claude/skills/node/examples/tsconfig-references.md.claude/skills/node/examples/vite-build.md.claude/skills/node/examples/vitest-setup.md.claude/skills/node/examples/workspace-setup.md.claude/skills/node/hooks/PreToolUse.d/ban-npx.claude/skills/node/hooks/settings.json.claude/skills/typescript/SKILL.md.vscode/settings.jsonorchestrator-prd-v1.mdorchestrator-prd-v2.mdorchestrator-prd-v3.mdorchestrator/.bifrost.yaml.exampleorchestrator/.claudeorchestrator/.gitignoreorchestrator/README.mdorchestrator/last-statuslineorchestrator/package.jsonorchestrator/packages/cli/package.jsonorchestrator/packages/cli/src/config.spec.tsorchestrator/packages/cli/src/config.tsorchestrator/packages/cli/src/git-root.spec.tsorchestrator/packages/cli/src/git-root.tsorchestrator/packages/cli/src/index.spec.tsorchestrator/packages/cli/src/index.tsorchestrator/packages/cli/tsconfig.jsonorchestrator/packages/cli/vite.config.tsorchestrator/packages/core/package.jsonorchestrator/packages/core/src/agent-parser.spec.tsorchestrator/packages/core/src/agent-parser.tsorchestrator/packages/core/src/file-task-state-store.spec.tsorchestrator/packages/core/src/file-task-state-store.tsorchestrator/packages/core/src/handlebars-renderer.spec.tsorchestrator/packages/core/src/handlebars-renderer.tsorchestrator/packages/core/src/hook-executor.spec.tsorchestrator/packages/core/src/hook-executor.tsorchestrator/packages/core/src/index.tsorchestrator/packages/core/src/memory-task-source.spec.tsorchestrator/packages/core/src/memory-task-source.tsorchestrator/packages/core/src/memory-task-state-store.spec.tsorchestrator/packages/core/src/memory-task-state-store.tsorchestrator/packages/core/src/orchestrator.spec.tsorchestrator/packages/core/src/orchestrator.tsorchestrator/packages/core/src/repo-installer.spec.tsorchestrator/packages/core/src/repo-installer.tsorchestrator/packages/core/src/task-state-store.spec.tsorchestrator/packages/core/src/task-state-store.tsorchestrator/packages/core/src/types.tsorchestrator/packages/core/src/validator.spec.tsorchestrator/packages/core/src/validator.tsorchestrator/packages/core/tsconfig.jsonorchestrator/packages/core/vite.config.tsorchestrator/packages/engine-claude-code/pyproject.tomlorchestrator/packages/engine-claude-code/src/engine_claude_code/__init__.pyorchestrator/packages/engine-claude-code/src/engine_claude_code/agent_catalog/__init__.pyorchestrator/packages/engine-claude-code/src/engine_claude_code/agent_catalog/loader.pyorchestrator/packages/engine-claude-code/src/engine_claude_code/agent_catalog/parser.pyorchestrator/packages/engine-claude-code/src/engine_claude_code/agent_catalog/types.pyorchestrator/packages/engine-claude-code/src/engine_claude_code/config.pyorchestrator/packages/engine-claude-code/src/engine_claude_code/sdk_runner.pyorchestrator/packages/engine-claude-code/tests/test_claude_config_dir.pyorchestrator/packages/engine/package.jsonorchestrator/packages/engine/src/index.tsorchestrator/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.spec.tsorchestrator/packages/engine/src/types.tsorchestrator/packages/engine/tsconfig.jsonorchestrator/packages/engine/vite.config.tsorchestrator/packages/interface-engine/pyproject.tomlorchestrator/packages/interface-engine/src/interface_engine/__init__.pyorchestrator/packages/interface-engine/src/interface_engine/base.pyorchestrator/packages/interface-engine/src/interface_engine/config.pyorchestrator/packages/interface-engine/src/interface_engine/types.pyorchestrator/packages/interface-tasks/pyproject.tomlorchestrator/packages/interface-tasks/src/interface_tasks/__init__.pyorchestrator/packages/interface-tasks/src/interface_tasks/base.pyorchestrator/packages/interface-tasks/src/interface_tasks/config.pyorchestrator/packages/interface-tasks/src/interface_tasks/types.pyorchestrator/packages/orchestrator/pyproject.tomlorchestrator/packages/orchestrator/src/orchestrator/__init__.pyorchestrator/packages/orchestrator/src/orchestrator/cli/__init__.pyorchestrator/packages/orchestrator/src/orchestrator/cli/agent_entry.pyorchestrator/packages/orchestrator/src/orchestrator/cli/config.pyorchestrator/packages/orchestrator/src/orchestrator/core/__init__.pyorchestrator/packages/orchestrator/src/orchestrator/core/config.pyorchestrator/packages/orchestrator/src/orchestrator/core/domain.pyorchestrator/packages/orchestrator/src/orchestrator/core/factory.pyorchestrator/packages/orchestrator/src/orchestrator/core/hook_runner.pyorchestrator/packages/orchestrator/src/orchestrator/core/orchestrator.pyorchestrator/packages/orchestrator/src/orchestrator/core/reporting.pyorchestrator/packages/task-source/package.jsonorchestrator/packages/task-source/src/api-task-source.spec.tsorchestrator/packages/task-source/src/api-task-source.tsorchestrator/packages/task-source/src/index.tsorchestrator/packages/task-source/src/interface.spec.tsorchestrator/packages/task-source/src/interface.tsorchestrator/packages/task-source/src/types.spec.tsorchestrator/packages/task-source/src/types.tsorchestrator/packages/task-source/tsconfig.jsonorchestrator/packages/task-source/vite.config.tsorchestrator/packages/tasks-bifrost/pyproject.tomlorchestrator/packages/tasks-bifrost/src/tasks_bifrost/__init__.pyorchestrator/packages/tasks-bifrost/src/tasks_bifrost/api_client.pyorchestrator/packages/tasks-bifrost/src/tasks_bifrost/config.pyorchestrator/packages/tasks-bifrost/src/tasks_bifrost/models.pyorchestrator/packages/tasks-bifrost/src/tasks_bifrost/task_source.pyorchestrator/pyproject.tomlorchestrator/scripts/agent.pyorchestrator/scripts/dispatcher.pyorchestrator/tsconfig.base.jsonorchestrator/tsconfig.jsonorchestrator/vitest.config.tsprd-lvl3-agent-definition-v4.md
💤 Files with no reviewable changes (42)
- orchestrator/packages/engine-claude-code/src/engine_claude_code/init.py
- orchestrator/README.md
- orchestrator/packages/orchestrator/src/orchestrator/init.py
- orchestrator/packages/interface-engine/src/interface_engine/init.py
- orchestrator/packages/orchestrator/src/orchestrator/core/hook_runner.py
- orchestrator/scripts/dispatcher.py
- orchestrator/pyproject.toml
- orchestrator/packages/tasks-bifrost/src/tasks_bifrost/api_client.py
- orchestrator/packages/engine-claude-code/src/engine_claude_code/sdk_runner.py
- orchestrator/packages/interface-tasks/pyproject.toml
- orchestrator/packages/tasks-bifrost/src/tasks_bifrost/config.py
- orchestrator/packages/tasks-bifrost/pyproject.toml
- orchestrator/packages/engine-claude-code/tests/test_claude_config_dir.py
- orchestrator/packages/engine-claude-code/src/engine_claude_code/config.py
- orchestrator/packages/orchestrator/src/orchestrator/core/factory.py
- orchestrator/packages/orchestrator/src/orchestrator/cli/config.py
- orchestrator/packages/engine-claude-code/pyproject.toml
- orchestrator/packages/orchestrator/src/orchestrator/cli/init.py
- orchestrator/.bifrost.yaml.example
- orchestrator/packages/interface-engine/src/interface_engine/types.py
- orchestrator/packages/engine-claude-code/src/engine_claude_code/agent_catalog/types.py
- orchestrator/packages/interface-tasks/src/interface_tasks/config.py
- orchestrator/packages/tasks-bifrost/src/tasks_bifrost/models.py
- orchestrator/packages/orchestrator/src/orchestrator/cli/agent_entry.py
- orchestrator/packages/interface-engine/src/interface_engine/base.py
- orchestrator/packages/orchestrator/pyproject.toml
- orchestrator/packages/interface-engine/pyproject.toml
- orchestrator/packages/tasks-bifrost/src/tasks_bifrost/init.py
- orchestrator/packages/orchestrator/src/orchestrator/core/config.py
- orchestrator/packages/interface-engine/src/interface_engine/config.py
- orchestrator/packages/engine-claude-code/src/engine_claude_code/agent_catalog/loader.py
- orchestrator/packages/engine-claude-code/src/engine_claude_code/agent_catalog/init.py
- orchestrator/packages/tasks-bifrost/src/tasks_bifrost/task_source.py
- orchestrator/scripts/agent.py
- orchestrator/packages/orchestrator/src/orchestrator/core/reporting.py
- orchestrator/packages/interface-tasks/src/interface_tasks/types.py
- orchestrator/packages/orchestrator/src/orchestrator/core/init.py
- orchestrator/packages/interface-tasks/src/interface_tasks/base.py
- orchestrator/packages/orchestrator/src/orchestrator/core/domain.py
- orchestrator/packages/engine-claude-code/src/engine_claude_code/agent_catalog/parser.py
- orchestrator/packages/interface-tasks/src/interface_tasks/init.py
- orchestrator/packages/orchestrator/src/orchestrator/core/orchestrator.py
| export -f deny | ||
| export INPUT | ||
|
|
||
| $CLAUDE_PROJECT_DIR/.claude/skills/$1/hooks/$(get_field hook_event_name).d/$2 No newline at end of file |
There was a problem hiding this comment.
Path traversal / command injection: $1, $2, and hook_event_name are unsanitized in the exec path.
The final line constructs and directly executes a path from three unsanitized inputs:
$1— CLI argument (skill name)$(get_field hook_event_name)— extracted from the JSON stdin payload$2— CLI argument (hook handler name)
If any of these contain .., /, or shell metacharacters, an attacker controlling the Claude hook invocation payload could execute an arbitrary file outside .claude/skills/. At minimum, validate that none of the path components contain directory separators:
🛡️ Proposed fix
+# Validate path components contain no directory separators
+for component in "$1" "$2"; do
+ if [[ "$component" == */* || "$component" == *..* ]]; then
+ echo "dispatch: invalid path component: $component" >&2
+ exit 1
+ fi
+done
+
+hook_event=$(get_field hook_event_name)
+if [[ "$hook_event" == */* || "$hook_event" == *..* || -z "$hook_event" ]]; then
+ echo "dispatch: invalid or empty hook_event_name" >&2
+ exit 1
+fi
+
-$CLAUDE_PROJECT_DIR/.claude/skills/$1/hooks/$(get_field hook_event_name).d/$2
+exec "$CLAUDE_PROJECT_DIR/.claude/skills/$1/hooks/$hook_event.d/$2"📝 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.
| $CLAUDE_PROJECT_DIR/.claude/skills/$1/hooks/$(get_field hook_event_name).d/$2 | |
| # Validate path components contain no directory separators | |
| for component in "$1" "$2"; do | |
| if [[ "$component" == */* || "$component" == *..* ]]; then | |
| echo "dispatch: invalid path component: $component" >&2 | |
| exit 1 | |
| fi | |
| done | |
| hook_event=$(get_field hook_event_name) | |
| if [[ "$hook_event" == */* || "$hook_event" == *..* || -z "$hook_event" ]]; then | |
| echo "dispatch: invalid or empty hook_event_name" >&2 | |
| exit 1 | |
| fi | |
| exec "$CLAUDE_PROJECT_DIR/.claude/skills/$1/hooks/$hook_event.d/$2" |
🤖 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 @.claude/hooks/dispatch at line 21, The dispatch hook builds and executes a
path using untrusted inputs ($1, $2 and the output of get_field hook_event_name)
which allows path traversal/command injection; fix by validating/sanitizing
those components before use: ensure get_field hook_event_name, $1 and $2 contain
only a safe whitelist of characters (e.g. alphanumerics, hyphen, underscore,
dot) and reject any value containing ../, /, null bytes, or shell
metacharacters, then construct the path from the validated tokens (use
basename-like normalization) and verify it is under the expected directory and
is a regular file before executing; reference the dispatch hook variables ($1,
$2) and get_field hook_event_name when implementing these checks.
| const parsed = yamlParse(configContent) as Record<string, unknown> | ||
|
|
||
| if (!parsed.orchestrate) { | ||
| return DEFAULT_CONFIG | ||
| } |
There was a problem hiding this comment.
Crash on empty or comment-only .orchestrator.yaml file.
YAML.parse('') currently returns null, so an empty config file (or one containing only YAML comments) causes parsed to be null. Line 75 then attempts null.orchestrate, throwing TypeError: Cannot read properties of null (reading 'orchestrate') instead of returning DEFAULT_CONFIG.
🐛 Proposed fix
- const parsed = yamlParse(configContent) as Record<string, unknown>
+ const parsed = yamlParse(configContent) as Record<string, unknown> | null
- if (!parsed.orchestrate) {
+ if (!parsed || !parsed.orchestrate) {
return DEFAULT_CONFIG
}🤖 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/cli/src/config.ts` around lines 73 - 77, The code
assumes yamlParse(configContent) returns an object but it can return null for
empty or comment-only files, causing a TypeError when checking
parsed.orchestrate; update the logic around the parsed variable (the const
parsed in config.ts) to handle null/undefined (e.g., treat parsed as {} when
null) before accessing parsed.orchestrate and ensure the function returns
DEFAULT_CONFIG when parsed is falsy or lacks the orchestrate key.
| const root = await resolveGitRoot('/home/devzeebo/git/bifrost/orchestrator/packages/cli') | ||
|
|
||
| // Then projectDir is set to the git root | ||
| expect(root).toContain('/bifrost') | ||
| }) | ||
|
|
||
| it('should find git root from subdirectory', async () => { | ||
| // Given a git repository rooted at /home/user/myrepo | ||
| // And a developer runs the orchestrator from /home/user/myrepo/src/lib | ||
| const root = await resolveGitRoot('/home/devzeebo/git/bifrost/orchestrator/packages/cli/src') | ||
|
|
||
| // When the orchestrator program starts | ||
| // Then projectDir is /home/user/myrepo | ||
| expect(root).toBeTruthy() | ||
| }) |
There was a problem hiding this comment.
Hardcoded developer paths make tests non-portable and will always fail in CI.
Lines 9 and 18 pass absolute paths rooted at /home/devzeebo/git/bifrost/… that only exist on one developer's machine. These tests will always fail in any CI environment or on another developer's machine.
Replace them with paths derived at runtime (e.g., import.meta.dirname or process.cwd()) to resolve a directory that is actually inside the repository under test:
🛠️ Proposed fix
- it('should set projectDir to git root when run from inside repository', async () => {
- const root = await resolveGitRoot('/home/devzeebo/git/bifrost/orchestrator/packages/cli')
+ it('should set projectDir to git root when run from inside repository', async () => {
+ // Use the actual package directory resolved at runtime
+ const root = await resolveGitRoot(import.meta.dirname)
expect(root).toContain('/bifrost')
})
- it('should find git root from subdirectory', async () => {
- const root = await resolveGitRoot('/home/devzeebo/git/bifrost/orchestrator/packages/cli/src')
+ it('should find git root from subdirectory', async () => {
+ const root = await resolveGitRoot(new URL('../../..', import.meta.url).pathname)Additionally, the assertion on line 22 (toBeTruthy()) doesn't verify the returned root is the correct one — prefer a more meaningful assertion:
- expect(root).toBeTruthy()
+ expect(root).toMatch(/bifrost/)📝 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.
| const root = await resolveGitRoot('/home/devzeebo/git/bifrost/orchestrator/packages/cli') | |
| // Then projectDir is set to the git root | |
| expect(root).toContain('/bifrost') | |
| }) | |
| it('should find git root from subdirectory', async () => { | |
| // Given a git repository rooted at /home/user/myrepo | |
| // And a developer runs the orchestrator from /home/user/myrepo/src/lib | |
| const root = await resolveGitRoot('/home/devzeebo/git/bifrost/orchestrator/packages/cli/src') | |
| // When the orchestrator program starts | |
| // Then projectDir is /home/user/myrepo | |
| expect(root).toBeTruthy() | |
| }) | |
| const root = await resolveGitRoot(import.meta.dirname) | |
| // Then projectDir is set to the git root | |
| expect(root).toContain('/bifrost') | |
| }) | |
| it('should find git root from subdirectory', async () => { | |
| // Given a git repository rooted at /home/user/myrepo | |
| // And a developer runs the orchestrator from /home/user/myrepo/src/lib | |
| const root = await resolveGitRoot(new URL('../../..', import.meta.url).pathname) | |
| // When the orchestrator program starts | |
| // Then projectDir is /home/user/myrepo | |
| expect(root).toMatch(/bifrost/) | |
| }) |
🤖 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/cli/src/git-root.spec.ts` around lines 9 - 23, The two
tests use hardcoded absolute paths that won't exist in CI; update the 'should
find git root' and 'should find git root from subdirectory' specs to compute
test paths at runtime (e.g., derive a path inside the repo using process.cwd()
or import.meta.url/dirname and path.join) and pass those to resolveGitRoot
instead of '/home/devzeebo/...'. Also tighten the assertions: replace the loose
expect(root).toBeTruthy() with a concrete check that the returned root equals or
contains the repository root you derived (use the same runtime-derived repo root
value for comparison) so the tests are both portable and meaningful.
| const params = taskState | ||
|
|
||
| // FR-14 step 8: Render Handlebars prompt with taskState values | ||
| const renderedPrompt = renderPrompt(agent.promptBody, taskState) | ||
|
|
||
| // FR-14 step 9: Build EngineContext and execute engine | ||
| const engineContext: EngineContext = { | ||
| taskId: task.id, | ||
| workingDir: projectDir, | ||
| agentName: agent.name, | ||
| verbose: false | ||
| } | ||
|
|
||
| // Main execution loop (handles follow-ups) | ||
| let maxFollowUps = 10 // Prevent infinite loops | ||
| let lastMessage = '' | ||
|
|
||
| while (maxFollowUps-- > 0) { | ||
| numTurns++ | ||
|
|
||
| // Execute engine | ||
| const engineResult: EngineResult = await engine.execute({ | ||
| ...engineContext, | ||
| // Pass rendered prompt and any follow-up context | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find EngineContext type definition
rg -n --type=ts 'EngineContext' -A 10Repository: devzeebo/bifrost
Length of output: 15032
🏁 Script executed:
# Read the full orchestrator.ts to see the complete function context
head -n 200 orchestrator/packages/core/src/orchestrator.ts | tail -n 120Repository: devzeebo/bifrost
Length of output: 4075
🏁 Script executed:
# Search for engine implementations and how they use context
rg -n "execute.*context" orchestrator/packages/engine/src/ -A 5 -B 2Repository: devzeebo/bifrost
Length of output: 6021
🏁 Script executed:
# Search for any LLM or real engine implementations
find orchestrator/packages -name "*.ts" -type f ! -path "*/node_modules/*" ! -path "*/dist/*" | xargs grep -l "implements Engine" 2>/dev/nullRepository: devzeebo/bifrost
Length of output: 159
🏁 Script executed:
# Search for where engine is created/passed in orchestrator.ts
rg -n "engine" orchestrator/packages/core/src/orchestrator.ts -B 3 -A 1 | head -80Repository: devzeebo/bifrost
Length of output: 2338
🏁 Script executed:
# Check how renderPrompt is defined and used elsewhere
rg -n "renderPrompt" orchestrator/packages/core/src/ -B 2 -A 3Repository: devzeebo/bifrost
Length of output: 6907
🏁 Script executed:
# Search for any integration tests or usage of the orchestrate function
rg -n "orchestrate" orchestrator/packages/core/src --type=ts -B 2 -A 5 | grep -A 10 "test\|spec" | head -60Repository: devzeebo/bifrost
Length of output: 2749
🏁 Script executed:
# Read the orchestrator spec to see how engine is mocked and tested
head -n 250 orchestrator/packages/core/src/orchestrator.spec.tsRepository: devzeebo/bifrost
Length of output: 6495
renderedPrompt is computed but never passed to the engine — incomplete implementation.
renderPrompt is called at line 85 and stored in renderedPrompt, but the variable is never referenced again. engine.execute is called at line 103 with only engineContext ({ taskId, workingDir, agentName, verbose }) — the rendered prompt does not reach the engine. The inline comment at line 105 ("Pass rendered prompt and any follow-up context") confirms this was intended but left incomplete.
params at line 82 is also unused (const params = taskState is never read).
The proposed fix cannot work as written: EngineContext type is defined in @orchestrator/engine/src/types.ts with exactly four fields and has no prompt field. Adding prompt: renderedPrompt would fail type-checking.
The prompt must reach the engine through one of:
- Update
EngineContexttype to include apromptfield (requires coordination with@orchestrator/enginepackage) - Pass the prompt through a different mechanism (e.g., constructor, context middleware, or as a separate parameter to
execute) - Store it in engineContext before building it, or pass it via a spread override
Remove the unused params variable and resolve how renderedPrompt should be delivered to the engine before this function is production-ready.
🤖 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/core/src/orchestrator.ts` around lines 82 - 106,
renderedPrompt (from renderPrompt) is computed but never passed into
engine.execute and the local const params = taskState is unused; either (A)
expand the EngineContext type in `@orchestrator/engine` to include a prompt field
and then spread renderedPrompt into the engine.execute call (i.e., build
engineContext: EngineContext & { prompt: string } and call engine.execute({
...engineContext, prompt: renderedPrompt, ...followUpContext })), or (B) if you
prefer not to change EngineContext, update the engine.execute API in the engine
package to accept the prompt as a separate argument (e.g.,
engine.execute(engineContext, renderedPrompt, followUpContext)) and update all
callers; in either case remove the unused params = taskState line and ensure
follow-up context is merged when calling engine.execute.
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
bifrost/ui/src/pages/accounts/@id/+Page.tsx (1)
949-963:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBroken
aria-describedbyreference in Create PAT dialogLine 950 points to
create-pat-description, but no element with that id is rendered after the JSX change at Lines 961-962. Screen readers lose dialog description context.Suggested fix
<div> <BaseDialog.Title id="create-pat-title" className="text-xl font-bold uppercase tracking-tight" style={{ color: "var(--color-blue)" }} > Create Personal Access Token </BaseDialog.Title> - Create a new token and copy it immediately. This token will not be retrievable - again. + <BaseDialog.Description + id="create-pat-description" + className="text-sm mt-1" + style={{ color: "var(--color-text-muted)" }} + > + Create a new token and copy it immediately. This token will not be retrievable again. + </BaseDialog.Description> </div>🤖 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 `@bifrost/ui/src/pages/accounts/`@id/+Page.tsx around lines 949 - 963, The dialog sets aria-describedby="create-pat-description" but there is no element with that id after the JSX change, so add a visible or visually-hidden element with id "create-pat-description" containing the dialog description text (e.g., "Create a new token and copy it immediately. This token will not be retrievable again.") adjacent to BaseDialog.Title (id "create-pat-title"), or alternatively update/remove the aria-describedby attribute to point to an existing element; ensure the id matches exactly so screen readers receive the dialog description.bifrost/ui/src/pages/onboarding/+Page.tsx (1)
474-481:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
StepContentsilently ignores thecolorprop.
StepContentPropsdeclarescolor: stringbut the component only destructureschildren. Every call site (e.g., lines 93, 111, 129, 147) passes a color string that is dropped on the floor. Either remove the prop from the type and all call sites, or apply it (e.g., as a border/accent style on the wrapperdiv).🐛 Proposed fix — remove the unused prop
type StepContentProps = { children: React.ReactNode; - color: string; }; -function StepContent({ children }: StepContentProps) { +function StepContent({ children }: StepContentProps) { return <div className="step-content">{children}</div>; }Then remove the
colorattribute from each<StepContent color="...">call site (lines 93, 111, 129, 147).🤖 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 `@bifrost/ui/src/pages/onboarding/`+Page.tsx around lines 474 - 481, StepContentProps declares a color prop that StepContent ignores; fix by either removing the prop from the type and deleting the color attribute from all StepContent call sites (e.g., the JSX usages of StepContent) OR make StepContent actually use the prop by updating the StepContent function to accept and destructure color from StepContentProps (keep the children) and apply it to the wrapper div (e.g., as an inline style, CSS variable, or an accent/border class driven by the color value) so callers like the existing <StepContent color="..."> usages get the intended styling; update the type and component consistently.bifrost/ui/src/pages/realms/+Page.tsx (2)
241-262:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
aria-labelto the "+" create button — screen readers will announce the symbol literally.The button's only visible content is
+. Without anaria-label, assistive technologies have nothing descriptive to read.♿ Proposed fix
<Button onClick={() => setIsCreateDialogOpen(true)} className="px-3 py-2 text-xs font-bold uppercase tracking-wider transition-all duration-150" + aria-label="Create Realm" style={{🤖 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 `@bifrost/ui/src/pages/realms/`+Page.tsx around lines 241 - 262, The "+" button uses the Button component and only displays a plus glyph, so add an aria-label to provide an accessible name (e.g., aria-label="Create realm" or "Create") on the same Button element that calls setIsCreateDialogOpen in its onClick; keep the visible "+" but ensure the aria-label conveys the action for screen readers and matches the button's purpose.
304-316:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Enterkey doesn't submit the Create Realm dialog.There's no
<form>wrapper and the confirm button istype="button", so pressing Enter in the input does nothing. Add anonKeyDownhandler.🛠️ Proposed fix
<Input id="new-realm-name" value={newRealmName} onChange={(e) => setNewRealmName(e.target.value)} + onKeyDown={(e) => { if (e.key === "Enter") handleCreateRealm(); }} placeholder="Engineering"🤖 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 `@bifrost/ui/src/pages/realms/`+Page.tsx around lines 304 - 316, The input for newRealmName needs an onKeyDown handler so pressing Enter triggers the same action as the confirm button; add onKeyDown={(e) => { if (e.key === 'Enter') { e.preventDefault(); /* call the same create handler used by the confirm button (e.g. createRealm / handleCreateRealm / onCreateRealm) */ } }} to the Input (the one using newRealmName and setNewRealmName) so Enter invokes the existing create/confirm logic (or alternatively trigger the confirm button click via a ref or change the surrounding UI to a form with the confirm button as type="submit").
♻️ Duplicate comments (1)
orchestrator/packages/cli/src/config.ts (1)
71-75:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUnresolved: null crash on empty or comment-only
.orchestrator.yaml.
yamlParse('')returnsnull; line 73 then throwsTypeError: Cannot read properties of null (reading 'orchestrate').🤖 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/cli/src/config.ts` around lines 71 - 75, The code assumes yamlParse(configContent) returns an object but yamlParse('') can return null; update the guard in config.ts to handle null/non-object results before accessing parsed.orchestrate — for example, change the check to verify parsed is truthy and an object (e.g. if (!parsed || typeof parsed !== 'object' || !parsed.orchestrate) return DEFAULT_CONFIG) so DEFAULT_CONFIG is returned for empty or comment-only files; ensure references to parsed.orchestrate are only used after this guard.
🟡 Minor comments (9)
.vscode/settings.json-13-13 (1)
13-13:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
*/**/.claudeglob won't exclude a root-level.claudedirectory.The pattern
*/**/.clauderequires at least one leading path segment, so a.claudefolder sitting directly at the workspace root won't be hidden. The standard VS Code glob for "any.claudefolder anywhere in the workspace" is**/.claude.🛠️ Proposed fix
- "*/**/.claude": true + "**/.claude": 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 @.vscode/settings.json at line 13, The glob pattern "*/**/.claude" in the settings does not match a root-level .claude directory; update the glob used (replace the "*/**/.claude" entry) to the standard recursive workspace pattern "**/.claude" so that any .claude folder — including one at the repository root — is excluded; ensure the updated pattern remains in the same settings key where "*/**/.claude" currently appears.bifrost/ui/src/pages/accounts/+Page.tsx-132-137 (1)
132-137:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInactive filter currently includes suspended accounts
Line 136 treats
"inactive"asstatus !== "active", so suspended accounts are mixed into the Inactive view. If the label is intended literally, filter by exact status.Suggested fix
const filteredAccounts = statusFilter === "all" ? accounts - : accounts.filter((account) => - statusFilter === "active" ? account.status === "active" : account.status !== "active", - ); + : accounts.filter((account) => account.status === statusFilter);🤖 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 `@bifrost/ui/src/pages/accounts/`+Page.tsx around lines 132 - 137, The current filteredAccounts logic treats "inactive" as status !== "active" and thus includes suspended accounts; change the conditional to compare account.status exactly to the requested filter value: keep the "all" branch returning accounts, then for non-"all" use a filter that, when statusFilter === "active", returns account.status === "active", otherwise returns account.status === statusFilter (so "inactive" matches only "inactive" and "suspended" stays out); update the code around filteredAccounts/statusFilter/accounts/account.status accordingly.orchestrator/packages/task-source-memory/vite.config.ts-7-7 (1)
7-7:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix typo in the package name passed to
base().
"tast-source-memory"appears to be a typo and should match the package naming convention used elsewhere.🔧 Proposed fix
export default base({ - name: "tast-source-memory", + name: "task-source-memory", pkg, tsconfig, });🤖 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/vite.config.ts` at line 7, There's a typo in the package name passed to base(): change the string "tast-source-memory" to the correct package name "task-source-memory" wherever it's used (e.g., the name property or argument to base()) so the call to base("tast-source-memory") / name: "tast-source-memory" becomes base("task-source-memory") / name: "task-source-memory".bifrost/ui/src/pages/realms/+Page.tsx-122-133 (1)
122-133:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win"Inactive" filter tab silently shows an empty list when "Show Suspended Realms" is off (the default).
The
showSuspendedgate at line 124 removes all"inactive"entries before thestatusFilterat line 132 ever runs. WithshowSuspended=false(default), selecting the "Inactive" tab always yields zero rows with no contextual explanation.Consider one of:
- Auto-set
showSuspended(true)whenstatusFilteris set to"inactive".- Show a targeted empty-state message when
statusFilter === "inactive" && !showSuspended.- Hide/disable the "Inactive" tab while
showSuspendedisfalse.🤖 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 `@bifrost/ui/src/pages/realms/`+Page.tsx around lines 122 - 133, The filter currently removes all "inactive" realms before the statusFilter runs (filteredRealms uses showSuspended), causing the "Inactive" tab to appear empty when showSuspended is false; update the logic so that when statusFilter === "inactive" you do not pre-filter out inactive realms (i.e., change filteredRealms to allow inactive items through if statusFilter === "inactive" or compute the status-based filter first), and add a targeted empty-state in the +Page.tsx render that shows a message like "No inactive realms are visible because 'Show Suspended Realms' is off" when statusFilter === "inactive" && !showSuspended so users understand why the list is empty; touch the filteredRealms computation and the UI branch that renders the list (the component surrounding filteredRealms) to implement this.prds/archive/bifrost-task-source-v3.md-215-222 (1)
215-222:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
asynckeyword on interface method declarations is invalid TypeScriptTypeScript interfaces declare signatures, not implementations —
asyncis not a valid modifier on interface method declarations. This example would be a compile error if pasted into TypeScript code. The actualinterface.tsis correct (noasynckeyword). Update the PRD example to match the valid interface shape and the actual code:📝 Corrected interface example
interface TaskSource { - async watchTasks(): AsyncIterator<Task>; - async completeTask(taskId: string): Promise<void>; - async failTask(taskId: string, error: string): Promise<void>; - async setState(taskId: string, taskState: Record<string, unknown>): Promise<void>; + watchTasks(): AsyncGenerator<Task>; + completeTask(taskId: string): Promise<void>; + failTask(taskId: string, error: string): Promise<void>; + setState(taskId: string, taskState: Record<string, unknown>): Promise<void>; }🤖 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 `@prds/archive/bifrost-task-source-v3.md` around lines 215 - 222, The TaskSource example in the PRD wrongly uses the async modifier on interface method declarations (TaskSource.watchTasks, completeTask, failTask, setState), which is invalid TypeScript; remove the "async" keyword from all method signatures in the TaskSource interface example so it matches the real interface.ts shape (keep return types: AsyncIterator<Task> for watchTasks and Promise<void> for the others, and maintain the taskId and taskState parameter names).orchestrator/packages/task-source/src/types.ts-25-41 (1)
25-41:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
createdAt: Datewill be a string at runtime when deserialized from JSON
JSON.parse()does not automatically convert ISO date strings toDateobjects. SinceNoteEntryandRetroEntryare populated from Bifrost HTTP responses (where the field iscreated_at: string), any implementation that doesn't explicitly callnew Date(created_at)will carry astringat runtime while the type saysDate. This can cause silent failures anywhere these fields are consumed (e.g.,.toISOString()calls will throw).Given that the sibling
ReadyRunetype already usesstringfor itscreated_at/updated_atfields, usingstringhere would be consistent and remove the conversion requirement.💡 Option A — use string (consistent with HTTP layer)
export type NoteEntry = { id: string; content: string; - createdAt: Date; + createdAt: string; }; export type RetroEntry = { id: string; content: string; - createdAt: Date; + createdAt: string; };💡 Option B — keep Date but verify mapping code converts explicitly
#!/bin/bash # Confirm all sites that construct NoteEntry / RetroEntry do `new Date(...)` conversion rg -n 'NoteEntry\|RetroEntry\|createdAt\|created_at' \ orchestrator/packages/task-source-bifrost/src/ -A3🤖 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/src/types.ts` around lines 25 - 41, The types NoteEntry and RetroEntry declare createdAt: Date but runtime JSON from Bifrost uses created_at: string; change these fields to createdAt: string to match ReadyRune and the HTTP layer (or alternatively ensure every constructor/mapping that creates NoteEntry/RetroEntry does an explicit new Date(created_at)); update any consumers that call Date methods to convert where necessary and adjust import/usage of NoteEntry/RetroEntry accordingly so the TypeScript types reflect the actual serialized shape..claude/skills/typescript/hooks/PostToolUse.d/lint-oxlint-5-5 (1)
5-5:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUnquoted subshell expansion will break on paths with spaces
$(get_field "tool_input.file_path")is unquoted, so any path containing whitespace is word-split into multiple arguments, causing the lint invocation to silently operate on wrong paths or fail.🛡️ Proposed fix
-npm run lint $(get_field "tool_input.file_path") || exit 2 +file_path=$(get_field "tool_input.file_path") +npm run lint "$file_path" || exit 2🤖 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 @.claude/skills/typescript/hooks/PostToolUse.d/lint-oxlint at line 5, The shell command uses an unquoted subshell expansion in the lint invocation which breaks on file paths with spaces; change the invocation to quote the subshell expansion so the entire path is passed as a single argument (i.e., replace npm run lint $(get_field "tool_input.file_path") || exit 2 with a version that quotes the subshell expansion like " $(get_field "tool_input.file_path") " ), ensuring the quoted form is used wherever the subshell output is passed to npm run lint to prevent word-splitting and globbing.orchestrator/packages/core/src/orchestrator.ts-8-9 (1)
8-9:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
"halted"outcome is declared but never returned; loop exhaustion silently completes the task.
OrchestrationResult.outcomeincludes"halted"as a union member, but no code path returns it. WhenmaxFollowUpsis decremented to zero the while loop exits and the task is markedcompleteTask—even if the final Stop hook still signalledneedsFollowUp. The task appears fulfilled despite never converging, and the"halted"branch becomes dead type.Consider returning
{ outcome: "halted" }when the follow-up budget is exhausted, and callingtaskSource.failTaskaccordingly.Also applies to: 88-88, 159-159
🤖 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/core/src/orchestrator.ts` around lines 8 - 9, OrchestrationResult currently lists "halted" but no code returns it; modify the orchestration loop that decrements maxFollowUps (the while loop that checks follow-up budget and processes the Stop hook) so that when the follow-up budget reaches zero and the last hook still signals needsFollowUp you return an OrchestrationResult with outcome: "halted" and invoke taskSource.failTask (instead of calling completeTask). Update any places that currently call completeTask on loop exit to handle and propagate the halted result so the "halted" branch is no longer dead (refer to OrchestrationResult, maxFollowUps, the while loop that processes Stop hooks, completeTask, and taskSource.failTask).prds/archive/bifrost-task-source-v2.md-56-56 (1)
56-56:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language identifiers to fenced code blocks (MD040).
These fences are missing a language specifier and trigger markdownlint warnings.
Also applies to: 77-77, 91-91, 98-98, 116-116, 132-132, 148-148, 164-164, 182-182, 199-199
🤖 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 `@prds/archive/bifrost-task-source-v2.md` at line 56, The markdown has multiple fenced code blocks using plain ``` which trigger MD040; update each triple-backtick fence to include the appropriate language identifier (e.g., ```json, ```yaml, ```bash, ```ts, etc.) that matches the block content so linters know the language—replace each plain ``` with the correct ```<lang> for every occurrence noted in the review to silence MD040 and ensure proper syntax highlighting.
🧹 Nitpick comments (2)
bifrost/ui/src/components/Wizard/Wizard.tsx (1)
96-220: ⚡ Quick winReplace
dangerouslySetInnerHTMLwith a JSX template-literal<style>tag.The CSS content is fully static, so there's no real XSS risk here. However,
bifrost/ui/src/pages/onboarding/+Page.tsx(line 340) already uses the simpler<style>{...}</style>pattern for the same purpose. UsingdangerouslySetInnerHTMLhere is inconsistent, bypasses React's virtual DOM diffing for the node, and triggers the static-analysis warning unnecessarily.♻️ Proposed refactor
- <style - dangerouslySetInnerHTML={{ - __html: ` - .wizard { + <style>{` + .wizard { ... - `, - }} - /> + `}</style>🤖 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 `@bifrost/ui/src/components/Wizard/Wizard.tsx` around lines 96 - 220, The inline CSS in the Wizard component is injected via dangerouslySetInnerHTML on the <style> element (see the style block in Wizard.tsx), causing inconsistency and static-analysis warnings; replace that usage by rendering the same CSS string as a JSX child of <style> (i.e., <style>{`...`}</style>) inside the Wizard component so React handles it normally and matches the pattern used in onboarding +Page.tsx; ensure the exact CSS content and class names (wizard, wizard-indicators, wizard-step-indicator, step-number, step-connector, wizard-content, wizard-navigation, wizard-button, wizard-button-back/next/done) are preserved.orchestrator/packages/core/src/agent-parser.ts (1)
130-158: ⚡ Quick winDuplicate hook-parsing logic for
StartandStop— extract a helper.The
Start(lines 130–143) andStop(lines 145–158) blocks are structurally identical. A single helper eliminates the duplication and reduces the surface area for divergence if the hook shape ever changes.♻️ Proposed refactor
+const parseHooks = ( + hooksData: Record<string, unknown> | undefined, + lifecycle: "Start" | "Stop", +): Array<{ name: string; scriptPath: string; timeout?: number }> => { + const raw = hooksData?.[lifecycle]; + if (!Array.isArray(raw)) return []; + return raw.flatMap((hook) => { + if (typeof hook !== "object" || hook === null) return []; + const h = hook as Record<string, unknown>; + if (typeof h.name !== "string" || typeof h.scriptPath !== "string") return []; + return [{ name: h.name, scriptPath: h.scriptPath, timeout: typeof h.timeout === "number" ? h.timeout : undefined }]; + }); +}; + // Parse hooks const hooksData = data.hooks as Record<string, unknown> | undefined; -const hooks: { ... } = { Start: [], Stop: [] }; - -if (hooksData?.Start && Array.isArray(hooksData.Start)) { - // ... 13 lines ... -} -if (hooksData?.Stop && Array.isArray(hooksData.Stop)) { - // ... 13 lines ... -} +const hooks = { + Start: parseHooks(hooksData, "Start"), + Stop: parseHooks(hooksData, "Stop"), +};🤖 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/core/src/agent-parser.ts` around lines 130 - 158, The Start/Stop parsing blocks duplicate logic; extract a helper function (e.g., parseHookArray) that accepts the source array (hooksData?.Start or hooksData?.Stop) and returns/ pushes normalized Hook objects into the target array; update the existing code to call parseHookArray(hooksData?.Start, hooks.Start) and parseHookArray(hooksData?.Stop, hooks.Stop). The helper should: validate the input is an array, iterate items, check each item is a non-null object, cast to Record<string, unknown>, verify name and scriptPath are strings, and set timeout only when typeof timeout === "number" before pushing {name, scriptPath, timeout} into the provided target array; keep function name parseHookArray (or similar) and maintain current field names to preserve behavior.
🤖 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/cli/src/config.ts`:
- Around line 83-87: The config currently rejects "bifrost" because
validTaskSourceTypes is missing that value; update the array referenced by
validTaskSourceTypes to include "bifrost" so the check if
(!validTaskSourceTypes.includes(taskSourceType)) accepts taskSourceType ===
"bifrost" and no longer throws; ensure this covers any callers that rely on
taskSourceType and that plugin instantiation for Bifrost proceeds as expected.
In `@orchestrator/packages/core/src/agent-parser.ts`:
- Around line 8-29: extractHandlebarsTokens is picking up Handlebars built-ins
and data variables (e.g. {{else}}, {{this}}, {{`@index`}}, helper calls) as
undeclared tokens; after you extract token/basePath in both the simpleTokenRegex
and blockTokenRegex loops, skip adding it to tokens when the basePath starts
with "@" or equals "else" or "this" (and other built-ins like
"true","false","null"), or when the token looks like a helper invocation
(contains quotes, parentheses, or is a known helper name such as "log" or
"lookup"); update the filtering logic in extractHandlebarsTokens (referencing
simpleTokenRegex, blockTokenRegex, the match variable and basePath extraction)
to return only real parameter names and ignore those special cases.
In `@orchestrator/packages/core/src/orchestrator.ts`:
- Around line 91-155: Hoist engineResult (and followUpResult) out of the while
loop so their final values are visible after the loop, then enforce EngineResult
semantics: if the last engineResult (or followUpResult when applicable) has
success === false call taskSource.failTask(task.id, /* use engineResult.error ||
lastMessage || followUpResult.error */) and return a failed outcome instead of
completing; if the last engineResult.skipFulfill === true do NOT call
taskSource.completeTask (leave claimed for follow-up); otherwise call
taskSource.completeTask as before. Ensure you reference the existing symbols
engineResult, followUpResult, skipFulfill, success, taskSource.completeTask, and
taskSource.failTask when making the changes.
In `@orchestrator/packages/task-source-bifrost/src/bifrost-task-source.spec.ts`:
- Line 274: The test uses an invalid matcher chain for the Promise returned by
setState; replace the `.resolves.not.toThrow()` assertion on the Promise from
source.setState("rune-1", { step: 2 }) with a proper Vitest matcher for
Promise<void>, e.g., `.resolves.toBeUndefined()`, so update the assertion
referencing setState to expect the resolved value to be undefined.
- Around line 130-137: The test currently starts a fire-and-forget consumer over
source.watchTasks() that keeps polling after assertions; change it to capture
the async iterator (e.g., const iterator =
source.watchTasks()[Symbol.asyncIterator]() or const iterator =
source.watchTasks()) and use that iterator in the for-await loop so you can stop
it, then call and await iterator.return?.() (or iterator.return() if present) in
test teardown/afterEach to terminate the background consumer and prevent
cross-test leakage; update the code that pushes to tasks to use this iterator
reference (iterator) and ensure the cleanup awaits the return call.
In `@orchestrator/packages/task-source-bifrost/src/bifrost-task-source.ts`:
- Around line 60-62: In watchTasks() the function returns immediately after
yielding this.#mapToTask(detail, agentId), terminating the async iterator;
remove the stray return (or replace it with continue if within a loop) so the
iterator keeps running and can emit subsequent tasks, ensuring any necessary
loop/cleanup logic still executes and that errors from this.#mapToTask(detail,
agentId) are handled without exiting the whole generator.
In `@orchestrator/packages/task-source-bifrost/src/client/bifrost-http-client.ts`:
- Around line 4-7: The class currently exposes the bearer token as a public
readonly field `token`; change it to a private class field `#token` and remove
the public `token` declaration, assign the incoming token parameter to
`this.#token` in the constructor, and update all internal uses (e.g., the
`request` method's `Authorization` header) to read `this.#token`; if external
code needs access, provide an explicit method (e.g., `getAuthHeader()` or
similar) rather than exposing the token property.
In `@orchestrator/packages/task-source-bifrost/src/config/config-loader.ts`:
- Around line 19-27: The isValidConfig type guard currently only checks that
config.url and config.realm are strings but allows empty strings; update
isValidConfig to also ensure both (config as { url: string }).url and (config as
{ realm: string }).realm are non-empty (e.g. .trim().length > 0) so empty or
whitespace-only values are rejected at validation time; keep the function
signature and other type checks intact and return false when either value is
empty.
In `@orchestrator/packages/task-source-bifrost/src/config/credential-loader.ts`:
- Around line 29-37: The type guard isValidCredentials currently allows array
shapes and inner nulls; tighten it by verifying the top-level credentials value
is a plain object (typeof === "object", !== null, and !Array.isArray), and also
verify (credentials as { credentials: unknown }).credentials is a non-null plain
object (typeof === "object", !== null, and !Array.isArray`) before returning
true; update the isValidCredentials implementation to perform these
Array.isArray and null checks so property access elsewhere (e.g., in functions
that read inner fields) cannot crash.
In `@orchestrator/packages/task-source-bifrost/src/integration.spec.ts`:
- Around line 7-38: createTestSource currently mutates process state (cwd and
BIFROST_TEST_HOME) but if any step throws during setup the teardown is never
run; wrap the setup sequence inside a try/catch (or try/finally) so that on any
error you restore process.cwd and process.env.BIFROST_TEST_HOME and remove the
tempDir before rethrowing the error, and ensure the returned cleanup function
still exists for successful case; update createTestSource (and references to new
BifrostTaskSource initialization) so that all state changes are undone on both
success and failure.
- Line 121: The test assertion is using .resolves.not.toThrow() on a
Promise<void; replace it with asserting the resolved value directly by calling
await expect(source.setState("rune-1", { step: 2 })).resolves.toBeUndefined();
locate the test that calls source.setState and update the expectation to
.resolves.toBeUndefined() so Vitest correctly verifies the Promise<void>
resolution.
In `@orchestrator/packages/task-source-bifrost/src/types.ts`:
- Around line 27-33: The RuneDetail type is missing the notes,
acceptanceCriteria, and retro collections which `#mapToTask` currently initializes
in Task.metadata; update the RuneDetail definition (the exported RuneDetail
type) to include optional fields notes, acceptanceCriteria, and retro (as
arrays) with the correct element shape based on the Bifrost /api/rune response
(e.g., string[] or an object type matching Bifrost's items), and then ensure
`#mapToTask` reads those arrays instead of overriding them with empty arrays so
existing data from the API is preserved.
In `@orchestrator/packages/task-source-memory/src/memory-task-source.ts`:
- Around line 26-52: The generator watchTasks() prematurely returns after
yielding a task, which stops the AsyncGenerator after one item and makes the
this.#claimed.size branch dead; remove the early return following the yield in
watchTasks() so execution falls back into the while loop and the generator
continues yielding subsequent tasks from this.#pending (keep the existing
this.#claimed.add(...) and task.status = "IN_PROGRESS" behavior), or if the
design truly intends one-task-per-generator, instead remove the
this.#claimed.size check from the while condition to avoid confusion — reference
watchTasks(), this.#pending, this.#claimed, and this.#tasks when applying the
change.
In `@orchestrator/vitest.setup.ts`:
- Around line 1-4: Replace the blanket no-op overrides of
console.log/warn/error/info in vitest.setup.ts with per-test spies: remove the
global assignments to console.log, console.warn, console.error, console.info and
instead use vi.spyOn(console, 'error'|'warn'|'log'|'info') inside a
beforeEach/afterEach pair (or rely on restoreMocks: true in vitest.config.ts or
call vi.restoreAllMocks() in afterEach) so each test can assert calls and
originals are automatically restored; ensure tests that need to suppress output
create their own spies/mocks rather than permanently mutating the global
console.
---
Outside diff comments:
In `@bifrost/ui/src/pages/accounts/`@id/+Page.tsx:
- Around line 949-963: The dialog sets aria-describedby="create-pat-description"
but there is no element with that id after the JSX change, so add a visible or
visually-hidden element with id "create-pat-description" containing the dialog
description text (e.g., "Create a new token and copy it immediately. This token
will not be retrievable again.") adjacent to BaseDialog.Title (id
"create-pat-title"), or alternatively update/remove the aria-describedby
attribute to point to an existing element; ensure the id matches exactly so
screen readers receive the dialog description.
In `@bifrost/ui/src/pages/onboarding/`+Page.tsx:
- Around line 474-481: StepContentProps declares a color prop that StepContent
ignores; fix by either removing the prop from the type and deleting the color
attribute from all StepContent call sites (e.g., the JSX usages of StepContent)
OR make StepContent actually use the prop by updating the StepContent function
to accept and destructure color from StepContentProps (keep the children) and
apply it to the wrapper div (e.g., as an inline style, CSS variable, or an
accent/border class driven by the color value) so callers like the existing
<StepContent color="..."> usages get the intended styling; update the type and
component consistently.
In `@bifrost/ui/src/pages/realms/`+Page.tsx:
- Around line 241-262: The "+" button uses the Button component and only
displays a plus glyph, so add an aria-label to provide an accessible name (e.g.,
aria-label="Create realm" or "Create") on the same Button element that calls
setIsCreateDialogOpen in its onClick; keep the visible "+" but ensure the
aria-label conveys the action for screen readers and matches the button's
purpose.
- Around line 304-316: The input for newRealmName needs an onKeyDown handler so
pressing Enter triggers the same action as the confirm button; add
onKeyDown={(e) => { if (e.key === 'Enter') { e.preventDefault(); /* call the
same create handler used by the confirm button (e.g. createRealm /
handleCreateRealm / onCreateRealm) */ } }} to the Input (the one using
newRealmName and setNewRealmName) so Enter invokes the existing create/confirm
logic (or alternatively trigger the confirm button click via a ref or change the
surrounding UI to a form with the confirm button as type="submit").
---
Minor comments:
In @.claude/skills/typescript/hooks/PostToolUse.d/lint-oxlint:
- Line 5: The shell command uses an unquoted subshell expansion in the lint
invocation which breaks on file paths with spaces; change the invocation to
quote the subshell expansion so the entire path is passed as a single argument
(i.e., replace npm run lint $(get_field "tool_input.file_path") || exit 2 with a
version that quotes the subshell expansion like " $(get_field
"tool_input.file_path") " ), ensuring the quoted form is used wherever the
subshell output is passed to npm run lint to prevent word-splitting and
globbing.
In @.vscode/settings.json:
- Line 13: The glob pattern "*/**/.claude" in the settings does not match a
root-level .claude directory; update the glob used (replace the "*/**/.claude"
entry) to the standard recursive workspace pattern "**/.claude" so that any
.claude folder — including one at the repository root — is excluded; ensure the
updated pattern remains in the same settings key where "*/**/.claude" currently
appears.
In `@bifrost/ui/src/pages/accounts/`+Page.tsx:
- Around line 132-137: The current filteredAccounts logic treats "inactive" as
status !== "active" and thus includes suspended accounts; change the conditional
to compare account.status exactly to the requested filter value: keep the "all"
branch returning accounts, then for non-"all" use a filter that, when
statusFilter === "active", returns account.status === "active", otherwise
returns account.status === statusFilter (so "inactive" matches only "inactive"
and "suspended" stays out); update the code around
filteredAccounts/statusFilter/accounts/account.status accordingly.
In `@bifrost/ui/src/pages/realms/`+Page.tsx:
- Around line 122-133: The filter currently removes all "inactive" realms before
the statusFilter runs (filteredRealms uses showSuspended), causing the
"Inactive" tab to appear empty when showSuspended is false; update the logic so
that when statusFilter === "inactive" you do not pre-filter out inactive realms
(i.e., change filteredRealms to allow inactive items through if statusFilter ===
"inactive" or compute the status-based filter first), and add a targeted
empty-state in the +Page.tsx render that shows a message like "No inactive
realms are visible because 'Show Suspended Realms' is off" when statusFilter ===
"inactive" && !showSuspended so users understand why the list is empty; touch
the filteredRealms computation and the UI branch that renders the list (the
component surrounding filteredRealms) to implement this.
In `@orchestrator/packages/core/src/orchestrator.ts`:
- Around line 8-9: OrchestrationResult currently lists "halted" but no code
returns it; modify the orchestration loop that decrements maxFollowUps (the
while loop that checks follow-up budget and processes the Stop hook) so that
when the follow-up budget reaches zero and the last hook still signals
needsFollowUp you return an OrchestrationResult with outcome: "halted" and
invoke taskSource.failTask (instead of calling completeTask). Update any places
that currently call completeTask on loop exit to handle and propagate the halted
result so the "halted" branch is no longer dead (refer to OrchestrationResult,
maxFollowUps, the while loop that processes Stop hooks, completeTask, and
taskSource.failTask).
In `@orchestrator/packages/task-source-memory/vite.config.ts`:
- Line 7: There's a typo in the package name passed to base(): change the string
"tast-source-memory" to the correct package name "task-source-memory" wherever
it's used (e.g., the name property or argument to base()) so the call to
base("tast-source-memory") / name: "tast-source-memory" becomes
base("task-source-memory") / name: "task-source-memory".
In `@orchestrator/packages/task-source/src/types.ts`:
- Around line 25-41: The types NoteEntry and RetroEntry declare createdAt: Date
but runtime JSON from Bifrost uses created_at: string; change these fields to
createdAt: string to match ReadyRune and the HTTP layer (or alternatively ensure
every constructor/mapping that creates NoteEntry/RetroEntry does an explicit new
Date(created_at)); update any consumers that call Date methods to convert where
necessary and adjust import/usage of NoteEntry/RetroEntry accordingly so the
TypeScript types reflect the actual serialized shape.
In `@prds/archive/bifrost-task-source-v2.md`:
- Line 56: The markdown has multiple fenced code blocks using plain ``` which
trigger MD040; update each triple-backtick fence to include the appropriate
language identifier (e.g., ```json, ```yaml, ```bash, ```ts, etc.) that matches
the block content so linters know the language—replace each plain ``` with the
correct ```<lang> for every occurrence noted in the review to silence MD040 and
ensure proper syntax highlighting.
In `@prds/archive/bifrost-task-source-v3.md`:
- Around line 215-222: The TaskSource example in the PRD wrongly uses the async
modifier on interface method declarations (TaskSource.watchTasks, completeTask,
failTask, setState), which is invalid TypeScript; remove the "async" keyword
from all method signatures in the TaskSource interface example so it matches the
real interface.ts shape (keep return types: AsyncIterator<Task> for watchTasks
and Promise<void> for the others, and maintain the taskId and taskState
parameter names).
---
Duplicate comments:
In `@orchestrator/packages/cli/src/config.ts`:
- Around line 71-75: The code assumes yamlParse(configContent) returns an object
but yamlParse('') can return null; update the guard in config.ts to handle
null/non-object results before accessing parsed.orchestrate — for example,
change the check to verify parsed is truthy and an object (e.g. if (!parsed ||
typeof parsed !== 'object' || !parsed.orchestrate) return DEFAULT_CONFIG) so
DEFAULT_CONFIG is returned for empty or comment-only files; ensure references to
parsed.orchestrate are only used after this guard.
---
Nitpick comments:
In `@bifrost/ui/src/components/Wizard/Wizard.tsx`:
- Around line 96-220: The inline CSS in the Wizard component is injected via
dangerouslySetInnerHTML on the <style> element (see the style block in
Wizard.tsx), causing inconsistency and static-analysis warnings; replace that
usage by rendering the same CSS string as a JSX child of <style> (i.e.,
<style>{`...`}</style>) inside the Wizard component so React handles it normally
and matches the pattern used in onboarding +Page.tsx; ensure the exact CSS
content and class names (wizard, wizard-indicators, wizard-step-indicator,
step-number, step-connector, wizard-content, wizard-navigation, wizard-button,
wizard-button-back/next/done) are preserved.
In `@orchestrator/packages/core/src/agent-parser.ts`:
- Around line 130-158: The Start/Stop parsing blocks duplicate logic; extract a
helper function (e.g., parseHookArray) that accepts the source array
(hooksData?.Start or hooksData?.Stop) and returns/ pushes normalized Hook
objects into the target array; update the existing code to call
parseHookArray(hooksData?.Start, hooks.Start) and
parseHookArray(hooksData?.Stop, hooks.Stop). The helper should: validate the
input is an array, iterate items, check each item is a non-null object, cast to
Record<string, unknown>, verify name and scriptPath are strings, and set timeout
only when typeof timeout === "number" before pushing {name, scriptPath, timeout}
into the provided target array; keep function name parseHookArray (or similar)
and maintain current field names to preserve behavior.
🪄 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: ff5f587b-c51e-44a1-a311-86a422fe1c74
⛔ Files ignored due to path filters (2)
orchestrator/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (130)
.claude/settings.json.claude/skills/typescript/hooks/PostToolUse.d/lint-oxlint.gitignore.vscode/settings.jsonbifrost/ui/.prettierignorebifrost/ui/index.htmlbifrost/ui/oxlint.config.tsbifrost/ui/package.jsonbifrost/ui/playwright.config.tsbifrost/ui/src/components/Dialog/Dialog.tsxbifrost/ui/src/components/RealmSelector/RealmSelector.spec.tsxbifrost/ui/src/components/RealmSelector/RealmSelector.tsxbifrost/ui/src/components/Toast/Toast.spec.tsxbifrost/ui/src/components/Toast/Toast.tsxbifrost/ui/src/components/TopNav/TopNav.spec.tsxbifrost/ui/src/components/TopNav/TopNav.tsxbifrost/ui/src/components/Wizard/Wizard.spec.tsxbifrost/ui/src/components/Wizard/Wizard.tsxbifrost/ui/src/index.cssbifrost/ui/src/lib/api.spec.tsbifrost/ui/src/lib/api.tsbifrost/ui/src/lib/auth.tsxbifrost/ui/src/lib/realm.tsxbifrost/ui/src/lib/router.tsbifrost/ui/src/lib/theme.tsxbifrost/ui/src/lib/toast.tsxbifrost/ui/src/main.tsxbifrost/ui/src/pages/+Layout.tsxbifrost/ui/src/pages/+config.tsbifrost/ui/src/pages/_error/+Page.tsxbifrost/ui/src/pages/account/+Page.tsxbifrost/ui/src/pages/accounts/+Page.tsxbifrost/ui/src/pages/accounts/@id/+Page.tsxbifrost/ui/src/pages/accounts/@id/+config.tsbifrost/ui/src/pages/accounts/new/+Page.tsxbifrost/ui/src/pages/dashboard/+Page.tsxbifrost/ui/src/pages/login/+Page.tsxbifrost/ui/src/pages/onboarding/+Page.tsxbifrost/ui/src/pages/realms/+Page.tsxbifrost/ui/src/pages/realms/@id/+Page.tsxbifrost/ui/src/pages/realms/@id/+config.tsbifrost/ui/src/pages/realms/new/+Page.tsxbifrost/ui/src/pages/runes/+Page.tsxbifrost/ui/src/pages/runes/@id/+Page.tsxbifrost/ui/src/pages/runes/@id/+config.tsbifrost/ui/src/pages/runes/@id/edit/+Page.tsxbifrost/ui/src/pages/runes/new/+Page.tsxbifrost/ui/src/types/account.tsbifrost/ui/src/types/realm.tsbifrost/ui/src/types/rune.tsbifrost/ui/vite.config.tsbifrost/ui/vitest.config.tsbifrost/ui/vitest.setup.tsorchestrator/package.jsonorchestrator/packages/cli/src/config.spec.tsorchestrator/packages/cli/src/config.tsorchestrator/packages/cli/src/git-root.spec.tsorchestrator/packages/cli/src/git-root.tsorchestrator/packages/cli/src/index.spec.tsorchestrator/packages/cli/src/index.tsorchestrator/packages/cli/tsconfig.jsonorchestrator/packages/cli/vite.config.tsorchestrator/packages/core/src/agent-parser.spec.tsorchestrator/packages/core/src/agent-parser.tsorchestrator/packages/core/src/handlebars-renderer.spec.tsorchestrator/packages/core/src/handlebars-renderer.tsorchestrator/packages/core/src/hook-executor.spec.tsorchestrator/packages/core/src/hook-executor.tsorchestrator/packages/core/src/index.tsorchestrator/packages/core/src/orchestrator.spec.tsorchestrator/packages/core/src/orchestrator.tsorchestrator/packages/core/src/repo-installer.spec.tsorchestrator/packages/core/src/repo-installer.tsorchestrator/packages/core/src/types.tsorchestrator/packages/core/src/validator.spec.tsorchestrator/packages/core/src/validator.tsorchestrator/packages/core/tsconfig.jsonorchestrator/packages/core/vite.config.tsorchestrator/packages/engine/src/index.tsorchestrator/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.spec.tsorchestrator/packages/engine/src/types.tsorchestrator/packages/engine/vite.config.tsorchestrator/packages/task-source-bifrost/package.jsonorchestrator/packages/task-source-bifrost/src/bifrost-task-source.spec.tsorchestrator/packages/task-source-bifrost/src/bifrost-task-source.tsorchestrator/packages/task-source-bifrost/src/client/bifrost-http-client.spec.tsorchestrator/packages/task-source-bifrost/src/client/bifrost-http-client.tsorchestrator/packages/task-source-bifrost/src/config/config-loader.spec.tsorchestrator/packages/task-source-bifrost/src/config/config-loader.tsorchestrator/packages/task-source-bifrost/src/config/credential-loader.spec.tsorchestrator/packages/task-source-bifrost/src/config/credential-loader.tsorchestrator/packages/task-source-bifrost/src/index.tsorchestrator/packages/task-source-bifrost/src/integration.spec.tsorchestrator/packages/task-source-bifrost/src/types.tsorchestrator/packages/task-source-bifrost/tsconfig.jsonorchestrator/packages/task-source-bifrost/vite.config.tsorchestrator/packages/task-source-memory/package.jsonorchestrator/packages/task-source-memory/src/index.tsorchestrator/packages/task-source-memory/src/memory-task-source.spec.tsorchestrator/packages/task-source-memory/src/memory-task-source.tsorchestrator/packages/task-source-memory/tsconfig.jsonorchestrator/packages/task-source-memory/vite.config.tsorchestrator/packages/task-source/src/index.tsorchestrator/packages/task-source/src/interface.spec.tsorchestrator/packages/task-source/src/interface.tsorchestrator/packages/task-source/src/types.spec.tsorchestrator/packages/task-source/src/types.tsorchestrator/packages/task-source/vite.config.tsorchestrator/tsconfig.base.jsonorchestrator/tsconfig.jsonorchestrator/tsconfig.test.jsonorchestrator/vite.base.tsorchestrator/vitest.config.tsorchestrator/vitest.setup.tsoxlint.config.tspackage.jsonprds/.claudeignoreprds/archive/bifrost-task-source-v1.mdprds/archive/bifrost-task-source-v2.mdprds/archive/bifrost-task-source-v3.mdprds/archive/orchestrator-prd-v1.mdprds/archive/orchestrator-prd-v2.mdprds/archive/prd-lvl3-agent-definition-v4.mdprds/bifrost-task-source.mdprds/orchestrator.mdprettier.config.mjs
💤 Files with no reviewable changes (3)
- bifrost/ui/src/types/account.ts
- bifrost/ui/oxlint.config.ts
- bifrost/ui/src/types/rune.ts
✅ Files skipped from review due to trivial changes (59)
- prds/.claudeignore
- bifrost/ui/src/pages/+Layout.tsx
- bifrost/ui/.prettierignore
- bifrost/ui/src/types/realm.ts
- bifrost/ui/src/pages/runes/@id/+config.ts
- bifrost/ui/src/pages/realms/@id/+config.ts
- bifrost/ui/src/pages/accounts/@id/+config.ts
- bifrost/ui/vitest.setup.ts
- orchestrator/packages/task-source-memory/tsconfig.json
- bifrost/ui/src/lib/auth.tsx
- orchestrator/packages/task-source-bifrost/tsconfig.json
- bifrost/ui/package.json
- bifrost/ui/src/pages/+config.ts
- bifrost/ui/src/lib/router.ts
- orchestrator/tsconfig.test.json
- orchestrator/packages/engine/src/interface.ts
- bifrost/ui/src/lib/theme.tsx
- bifrost/ui/src/components/RealmSelector/RealmSelector.spec.tsx
- .gitignore
- bifrost/ui/vitest.config.ts
- orchestrator/packages/task-source-bifrost/package.json
- bifrost/ui/src/components/RealmSelector/RealmSelector.tsx
- package.json
- bifrost/ui/src/components/Toast/Toast.tsx
- orchestrator/tsconfig.json
- bifrost/ui/src/components/Dialog/Dialog.tsx
- orchestrator/packages/task-source-bifrost/vite.config.ts
- bifrost/ui/src/pages/realms/new/+Page.tsx
- orchestrator/packages/cli/tsconfig.json
- bifrost/ui/src/components/Toast/Toast.spec.tsx
- orchestrator/packages/task-source-memory/src/memory-task-source.spec.ts
- orchestrator/packages/task-source-memory/src/index.ts
- bifrost/ui/index.html
- bifrost/ui/src/components/TopNav/TopNav.spec.tsx
- bifrost/ui/src/pages/dashboard/+Page.tsx
- bifrost/ui/src/lib/toast.tsx
- orchestrator/packages/task-source-bifrost/src/index.ts
- bifrost/ui/src/main.tsx
- orchestrator/packages/core/src/agent-parser.spec.ts
- bifrost/ui/playwright.config.ts
- orchestrator/packages/cli/src/git-root.spec.ts
- orchestrator/package.json
- bifrost/ui/src/components/Wizard/Wizard.spec.tsx
- bifrost/ui/src/pages/runes/new/+Page.tsx
- bifrost/ui/src/pages/runes/+Page.tsx
- bifrost/ui/src/pages/accounts/new/+Page.tsx
- orchestrator/packages/cli/src/config.spec.ts
- orchestrator/packages/core/src/repo-installer.spec.ts
- bifrost/ui/vite.config.ts
- bifrost/ui/src/index.css
- bifrost/ui/src/lib/api.ts
- bifrost/ui/src/pages/runes/@id/edit/+Page.tsx
- orchestrator/packages/engine/src/test-engine.spec.ts
- bifrost/ui/src/components/TopNav/TopNav.tsx
- bifrost/ui/src/lib/realm.tsx
- bifrost/ui/src/pages/runes/@id/+Page.tsx
- bifrost/ui/src/pages/realms/@id/+Page.tsx
- bifrost/ui/src/lib/api.spec.ts
- bifrost/ui/src/pages/account/+Page.tsx
🚧 Files skipped from review as they are similar to previous changes (18)
- orchestrator/vitest.config.ts
- orchestrator/packages/engine/src/index.ts
- orchestrator/packages/core/src/types.ts
- orchestrator/packages/core/tsconfig.json
- orchestrator/packages/cli/src/git-root.ts
- orchestrator/packages/engine/src/types.spec.ts
- orchestrator/packages/core/src/repo-installer.ts
- orchestrator/packages/engine/src/interface.spec.ts
- orchestrator/packages/engine/src/test-engine.ts
- orchestrator/packages/core/src/validator.spec.ts
- orchestrator/packages/cli/src/index.spec.ts
- orchestrator/packages/core/src/hook-executor.ts
- orchestrator/packages/core/src/handlebars-renderer.spec.ts
- orchestrator/packages/core/src/hook-executor.spec.ts
- orchestrator/packages/core/src/handlebars-renderer.ts
- orchestrator/packages/core/src/validator.ts
- orchestrator/tsconfig.base.json
- orchestrator/packages/cli/src/index.ts
| // FR-13: If unknown task source type, raise error | ||
| const validTaskSourceTypes = ["api", "memory", "file", "queue"]; | ||
| if (!validTaskSourceTypes.includes(taskSourceType)) { | ||
| throw new Error(`Unknown task source type: ${taskSourceType}`); | ||
| } |
There was a problem hiding this comment.
"bifrost" is missing from validTaskSourceTypes — breaks the entire Bifrost integration introduced in this PR.
Any .orchestrator.yaml with task_source.type: "bifrost" will throw "Unknown task source type: bifrost" at config load time, before the plugin is ever instantiated. The PRD (US-1 acceptance criteria, line 57) explicitly uses type: "bifrost".
🐛 Proposed fix
- const validTaskSourceTypes = ["api", "memory", "file", "queue"];
+ const validTaskSourceTypes = ["api", "memory", "file", "queue", "bifrost"];📝 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.
| // FR-13: If unknown task source type, raise error | |
| const validTaskSourceTypes = ["api", "memory", "file", "queue"]; | |
| if (!validTaskSourceTypes.includes(taskSourceType)) { | |
| throw new Error(`Unknown task source type: ${taskSourceType}`); | |
| } | |
| // FR-13: If unknown task source type, raise error | |
| const validTaskSourceTypes = ["api", "memory", "file", "queue", "bifrost"]; | |
| if (!validTaskSourceTypes.includes(taskSourceType)) { | |
| throw new Error(`Unknown task source type: ${taskSourceType}`); | |
| } |
🤖 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/cli/src/config.ts` around lines 83 - 87, The config
currently rejects "bifrost" because validTaskSourceTypes is missing that value;
update the array referenced by validTaskSourceTypes to include "bifrost" so the
check if (!validTaskSourceTypes.includes(taskSourceType)) accepts taskSourceType
=== "bifrost" and no longer throws; ensure this covers any callers that rely on
taskSourceType and that plugin instantiation for Bifrost proceeds as expected.
| const extractHandlebarsTokens = (content: string): Set<string> => { | ||
| const tokens = new Set<string>(); | ||
|
|
||
| // Match simple tokens: {{variableName}} | ||
| const simpleTokenRegex = /\{\{([^#/][^}]*)\}\}/g; | ||
| let match; | ||
| while ((match = simpleTokenRegex.exec(content)) !== null) { | ||
| const token = match[1].trim(); | ||
| // Extract the base path (first part before any dots or spaces) | ||
| const basePath = token.split(".")[0].split(" ")[0]; | ||
| tokens.add(basePath); | ||
| } | ||
|
|
||
| // Match block helpers: {{#if variableName}}...{{/if}} | ||
| const blockTokenRegex = /\{\{#(?:if|unless|each)\s+([^}]+)\}\}/g; | ||
| while ((match = blockTokenRegex.exec(content)) !== null) { | ||
| const token = match[1].trim(); | ||
| const basePath = token.split(".")[0].split(" ")[0]; | ||
| tokens.add(basePath); | ||
| } | ||
|
|
||
| return tokens; |
There was a problem hiding this comment.
{{else}}, {{this}}, and @-prefixed data variables are incorrectly treated as undeclared tokens.
The simpleTokenRegex captures any {{...}} token whose content does not start with # or /. This means:
{{else}}→ basePathelse{{this}}→ basePaththis{{@index}}/{{@key}}→ basePath@index/@key{{log "msg"}}→ basePathlog
...are all extracted and validated as if they were template parameters. Since none are declared in template.parameters, parsing fails with "Undeclared Handlebars token: else" (or similar).
The PRD explicitly calls out {{#if paramName}}...{{/if}} as the idiomatic guard for optional parameters, and {{else}} is the natural extension of that pattern. Any AGENT.md using {{#if x}}...{{else}}...{{/if}} will fail to parse.
🐛 Proposed fix
+const HANDLEBARS_BUILTINS = new Set(["else", "log", "lookup", "this"]);
+
const extractHandlebarsTokens = (content: string): Set<string> => {
const tokens = new Set<string>();
// Match simple tokens: {{variableName}}
const simpleTokenRegex = /\{\{([^#/][^}]*)\}\}/g;
let match;
while ((match = simpleTokenRegex.exec(content)) !== null) {
const token = match[1].trim();
// Extract the base path (first part before any dots or spaces)
const basePath = token.split(".")[0].split(" ")[0];
- tokens.add(basePath);
+ // Skip Handlebars built-in keywords and data variables like `@index`, `@key`
+ if (!basePath.startsWith("@") && !HANDLEBARS_BUILTINS.has(basePath)) {
+ tokens.add(basePath);
+ }
}🧰 Tools
🪛 OpenGrep (1.20.0)
[ERROR] 14-14: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 23-23: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 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/core/src/agent-parser.ts` around lines 8 - 29,
extractHandlebarsTokens is picking up Handlebars built-ins and data variables
(e.g. {{else}}, {{this}}, {{`@index`}}, helper calls) as undeclared tokens; after
you extract token/basePath in both the simpleTokenRegex and blockTokenRegex
loops, skip adding it to tokens when the basePath starts with "@" or equals
"else" or "this" (and other built-ins like "true","false","null"), or when the
token looks like a helper invocation (contains quotes, parentheses, or is a
known helper name such as "log" or "lookup"); update the filtering logic in
extractHandlebarsTokens (referencing simpleTokenRegex, blockTokenRegex, the
match variable and basePath extraction) to return only real parameter names and
ignore those special cases.
| while (maxFollowUps-- > 0) { | ||
| numTurns++; | ||
|
|
||
| const engineResult: EngineResult = await engine.execute(engineContext); | ||
|
|
||
| if (engineResult.stats) { | ||
| if (!totalTelemetry) { | ||
| totalTelemetry = { ...engineResult.stats }; | ||
| } else { | ||
| totalTelemetry.durationMs += engineResult.stats.durationMs; | ||
| totalTelemetry.inputTokens += engineResult.stats.inputTokens; | ||
| totalTelemetry.outputTokens += engineResult.stats.outputTokens; | ||
| totalTelemetry.cacheReadTokens += engineResult.stats.cacheReadTokens; | ||
| totalTelemetry.cacheCreationTokens += engineResult.stats.cacheCreationTokens; | ||
| totalTelemetry.totalCostUsd += engineResult.stats.totalCostUsd; | ||
| totalTelemetry.numTurns += engineResult.stats.numTurns; | ||
| } | ||
| } | ||
|
|
||
| lastMessage = engineResult.lastMessage || lastMessage; | ||
|
|
||
| // Step 4: Execute post-task hooks | ||
| const stopHookResults = await executeHooks(agent.hooks.Stop, "Stop", hookContext, execFn); | ||
|
|
||
| let needsFollowUp = false; | ||
| let followUpMessage = ""; | ||
|
|
||
| for (const hook of stopHookResults) { | ||
| if (hook.needsFollowUp) { | ||
| needsFollowUp = true; | ||
| followUpMessage = hook.stdout; | ||
| break; | ||
| } | ||
|
|
||
| if (hook.fatal) { | ||
| await taskSource.failTask(task.id, `Stop hook ${hook.hookName} failed: ${hook.stderr}`); | ||
| return { outcome: "failed", error: hook.stderr }; | ||
| } | ||
| } | ||
|
|
||
| if (!needsFollowUp) { | ||
| break; | ||
| } | ||
|
|
||
| if (engine.sendFollowUp) { | ||
| const followUpResult = await engine.sendFollowUp(followUpMessage); | ||
| if (followUpResult.stats) { | ||
| if (!totalTelemetry) { | ||
| totalTelemetry = { ...followUpResult.stats }; | ||
| } else { | ||
| totalTelemetry.durationMs += followUpResult.stats.durationMs; | ||
| totalTelemetry.inputTokens += followUpResult.stats.inputTokens; | ||
| totalTelemetry.outputTokens += followUpResult.stats.outputTokens; | ||
| totalTelemetry.cacheReadTokens += followUpResult.stats.cacheReadTokens; | ||
| totalTelemetry.cacheCreationTokens += followUpResult.stats.cacheCreationTokens; | ||
| totalTelemetry.totalCostUsd += followUpResult.stats.totalCostUsd; | ||
| totalTelemetry.numTurns += followUpResult.stats.numTurns; | ||
| } | ||
| } | ||
| numTurns++; | ||
| } | ||
| } | ||
|
|
||
| // Step 5: Report success | ||
| await taskSource.completeTask(task.id); |
There was a problem hiding this comment.
engineResult.skipFulfill and engineResult.success are never read — completeTask fires unconditionally.
EngineResult exposes both skipFulfill and success, but orchestrate discards them and always calls taskSource.completeTask after the loop. Two concrete breakages:
skipFulfill: true— the PRD explicitly specifies that whenskipFulfillis set the orchestrator must NOT callcompleteTask. The rune stays inclaimedstate for a follow-up workflow, but the current code marks it fulfilled regardless.success: false— a failed engine execution is silently reported as a successful completion rather than triggeringfailTask.
engineResult is also block-scoped inside the while, so it must be hoisted to be readable after the loop.
🐛 Proposed fix
let maxFollowUps = 10;
let lastMessage = "";
+ let lastEngineResult: EngineResult | null = null;
while (maxFollowUps-- > 0) {
numTurns++;
const engineResult: EngineResult = await engine.execute(engineContext);
+ lastEngineResult = engineResult;
// ... stats aggregation and hook handling unchanged ...
}
- // Step 5: Report success
- await taskSource.completeTask(task.id);
+ // Step 5: Handle engine outcome
+ if (lastEngineResult && !lastEngineResult.success) {
+ const errMsg = lastEngineResult.lastMessage || "Engine reported failure";
+ await taskSource.failTask(task.id, errMsg);
+ return { outcome: "failed", error: errMsg };
+ }
+ if (!lastEngineResult?.skipFulfill) {
+ await taskSource.completeTask(task.id);
+ }🤖 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/core/src/orchestrator.ts` around lines 91 - 155, Hoist
engineResult (and followUpResult) out of the while loop so their final values
are visible after the loop, then enforce EngineResult semantics: if the last
engineResult (or followUpResult when applicable) has success === false call
taskSource.failTask(task.id, /* use engineResult.error || lastMessage ||
followUpResult.error */) and return a failed outcome instead of completing; if
the last engineResult.skipFulfill === true do NOT call taskSource.completeTask
(leave claimed for follow-up); otherwise call taskSource.completeTask as before.
Ensure you reference the existing symbols engineResult, followUpResult,
skipFulfill, success, taskSource.completeTask, and taskSource.failTask when
making the changes.
| void (async () => { | ||
| for await (const task of source.watchTasks()) { | ||
| tasks.push(task); | ||
| } | ||
| })(); | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 300)); | ||
|
|
There was a problem hiding this comment.
Stop the background watchTasks() consumer before test teardown.
The fire-and-forget async loop keeps running and polling after assertions, which can create flaky cross-test side effects.
Suggested fix
- void (async () => {
- for await (const task of source.watchTasks()) {
- tasks.push(task);
- }
- })();
+ const iterator = source.watchTasks();
+ const consume = (async () => {
+ for await (const task of iterator) {
+ tasks.push(task);
+ }
+ })();
...
+ await iterator.return?.();
+ await consume;🤖 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-bifrost/src/bifrost-task-source.spec.ts`
around lines 130 - 137, The test currently starts a fire-and-forget consumer
over source.watchTasks() that keeps polling after assertions; change it to
capture the async iterator (e.g., const iterator =
source.watchTasks()[Symbol.asyncIterator]() or const iterator =
source.watchTasks()) and use that iterator in the for-await loop so you can stop
it, then call and await iterator.return?.() (or iterator.return() if present) in
test teardown/afterEach to terminate the background consumer and prevent
cross-test leakage; update the code that pushes to tasks to use this iterator
reference (iterator) and ensure the cleanup awaits the return call.
| async function createTestSource() { | ||
| const tempDir = join("/tmp", `bifrost-test-${randomBytes(8).toString("hex")}`); | ||
| await mkdir(tempDir, { recursive: true }); | ||
|
|
||
| const bifrostConfig = "url: https://bifrost.example.com\nrealm: test-realm\n"; | ||
| await writeFile(join(tempDir, ".bifrost.yaml"), bifrostConfig, "utf-8"); | ||
|
|
||
| const homeDir = join(tempDir, "home"); | ||
| await mkdir(join(homeDir, ".config", "bifrost"), { recursive: true }); | ||
| const credentials = "credentials:\n https://bifrost.example.com:\n token: test-token\n"; | ||
| await writeFile(join(homeDir, ".config", "bifrost", "credentials.yaml"), credentials, "utf-8"); | ||
|
|
||
| const originalCwd = process.cwd(); | ||
| const originalHome = process.env.BIFROST_TEST_HOME; | ||
| process.chdir(tempDir); | ||
| process.env.BIFROST_TEST_HOME = homeDir; | ||
|
|
||
| const { BifrostTaskSource } = await import("./bifrost-task-source.js"); | ||
|
|
||
| return { | ||
| source: new BifrostTaskSource(), | ||
| cleanup: async () => { | ||
| process.chdir(originalCwd); | ||
| if (originalHome === undefined) { | ||
| delete process.env.BIFROST_TEST_HOME; | ||
| } else { | ||
| process.env.BIFROST_TEST_HOME = originalHome; | ||
| } | ||
| await rm(tempDir, { recursive: true, force: true }); | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Ensure test environment cleanup is guaranteed on failures.
createTestSource() mutates global process state, but cleanup is left to each test. If a test fails before await cleanup(), cwd/env/tmp state can leak and cascade into unrelated tests.
Suggested refactor
+async function withTestSource<T>(run: (source: InstanceType<typeof (await import("./bifrost-task-source.js")).BifrostTaskSource>) => Promise<T>): Promise<T> {
+ const { source, cleanup } = await createTestSource();
+ try {
+ return await run(source);
+ } finally {
+ await cleanup();
+ }
+}🤖 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-bifrost/src/integration.spec.ts` around
lines 7 - 38, createTestSource currently mutates process state (cwd and
BIFROST_TEST_HOME) but if any step throws during setup the teardown is never
run; wrap the setup sequence inside a try/catch (or try/finally) so that on any
error you restore process.cwd and process.env.BIFROST_TEST_HOME and remove the
tempDir before rethrowing the error, and ensure the returned cleanup function
still exists for successful case; update createTestSource (and references to new
BifrostTaskSource initialization) so that all state changes are undone on both
success and failure.
| statusText: "Internal Server Error", | ||
| }); | ||
|
|
||
| await expect(source.setState("rune-1", { step: 2 })).resolves.not.toThrow(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Vitest v4.1.5 support await expect(promise).resolves.not.toThrow()? What is the recommended assertion pattern for a Promise<void> that should resolve successfully?
💡 Result:
No. Vitest v4.1.5 does not support using toThrow with await expect(promise).resolves.not.toThrow() as written. Reason 1 (API limitation): Vitest’s resolves is for unwrapping a promise and then applying matchers to the resolved value; if the promise rejects, the assertion fails. The expect API docs state that toThrow/aliases are not supported in the promise/expect.poll flow because the “condition is always resolved before the matcher gets the value” (so toThrow can’t meaningfully run in that context) [1]. Reason 2 (you can’t pass a function to .resolves): Vitest also treats .resolves as requiring an actual Promise; using a function with .resolves can error (example error: “You must provide a Promise to expect when using.resolves, not 'function'”) [2][3]. Recommended assertion pattern for a Promise<void> that should resolve successfully: 1) Use .resolves with a matcher that works on the resolved value. For Promise<void>, resolved value is undefined, so assert toBeUndefined() (or toBeTruthy/toEqual(undefined) accordingly): await expect(promise).resolves.toBeUndefined Vitest’s docs show the general resolves pattern (“unwrap the promise and then apply the matcher to its resolved value… If the promise rejects, the assertion will fail”) and include the need to await the assertion [4]. 2) Alternatively, if you just care that it doesn’t reject, you can await the promise directly and let the test fail on rejection (Vitest will mark the test failed if the returned promise rejects) [5]. For completeness: await expect(promise).rejects.toThrow() is the correct pattern when you expect a rejection with an error [4].
Citations:
- 1: https://vitest.dev/api/expect.html
- 2: expect().resolves does not accept a function vitest-dev/vitest#10281
- 3: "You must provide a Promise to expect()" inconsistent between
resolvesandrejectsvitest-dev/vitest#6643 - 4: https://main.vitest.dev/guide/learn/async
- 5: https://main.vitest.dev/api
🏁 Script executed:
# Check if the file exists
ls -la "orchestrator/packages/task-source-bifrost/src/integration.spec.ts" 2>/dev/null && echo "File found" || echo "File not found"
# If file exists, read around line 121
if [ -f "orchestrator/packages/task-source-bifrost/src/integration.spec.ts" ]; then
echo "=== Lines around line 121 ==="
sed -n '118,125p' "orchestrator/packages/task-source-bifrost/src/integration.spec.ts"
fiRepository: devzeebo/bifrost
Length of output: 456
🏁 Script executed:
# Find package.json to check Vitest version
find "orchestrator/packages/task-source-bifrost" -name "package.json" | head -1 | xargs cat 2>/dev/null | grep -A 5 -B 5 "vitest" || echo "No vitest found in package.json"
# Also check root package.json
if [ -f "package.json" ]; then
echo "=== Root package.json ==="
cat package.json | grep -A 5 -B 5 "vitest" || echo "No vitest in root package.json"
fiRepository: devzeebo/bifrost
Length of output: 573
Use .resolves.toBeUndefined() for Promise<void> assertions.
Vitest v4.1.5 does not support toThrow() within the .resolves chain. For a Promise<void> that should resolve successfully, assert the resolved value directly.
Suggested fix
- await expect(source.setState("rune-1", { step: 2 })).resolves.not.toThrow();
+ await expect(source.setState("rune-1", { step: 2 })).resolves.toBeUndefined();📝 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.
| await expect(source.setState("rune-1", { step: 2 })).resolves.not.toThrow(); | |
| await expect(source.setState("rune-1", { step: 2 })).resolves.toBeUndefined(); |
🤖 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-bifrost/src/integration.spec.ts` at line
121, The test assertion is using .resolves.not.toThrow() on a Promise<void;
replace it with asserting the resolved value directly by calling await
expect(source.setState("rune-1", { step: 2 })).resolves.toBeUndefined(); locate
the test that calls source.setState and update the expectation to
.resolves.toBeUndefined() so Vitest correctly verifies the Promise<void>
resolution.
| export type RuneDetail = ReadyRune & { | ||
| description: string; | ||
| branch?: string; | ||
| saga_id?: string; | ||
| assignee_id?: string; | ||
| dependencies: Array<{ target_id: string; relationship: string }>; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if notes/AC/retro are referenced anywhere in bifrost task-source
rg -n 'notes\|acceptanceCriteria\|retro\|NoteEntry\|ACEntry\|RetroEntry' \
orchestrator/packages/task-source-bifrost/src/ -A2Repository: devzeebo/bifrost
Length of output: 42
🏁 Script executed:
# First, let's see the full types.ts file to understand ReadyRune and RuneDetail context
head -50 orchestrator/packages/task-source-bifrost/src/types.tsRepository: devzeebo/bifrost
Length of output: 710
🏁 Script executed:
# Search for ReadyRune definition
rg -n 'type ReadyRune\|interface ReadyRune' orchestrator/packages/task-source-bifrost/src/ -A10Repository: devzeebo/bifrost
Length of output: 42
🏁 Script executed:
# Find the API endpoint handler that returns RuneDetail (likely in a controller)
fd -e ts -e js orchestrator/packages/task-source-bifrost/src | head -20Repository: devzeebo/bifrost
Length of output: 546
🏁 Script executed:
# Look for route definitions or controller files in bifrost
find orchestrator/packages/task-source-bifrost -name '*controller*' -o -name '*route*' -o -name '*handler*' 2>/dev/nullRepository: devzeebo/bifrost
Length of output: 42
🏁 Script executed:
find orchestrator/packages/task-source-bifrost -type f -name "*.ts" -o -name "*.js" | grep -E "(controller|route|service|api)" | head -20Repository: devzeebo/bifrost
Length of output: 42
🏁 Script executed:
# List all TypeScript files in the package
find orchestrator/packages/task-source-bifrost -type f -name "*.ts" | head -30Repository: devzeebo/bifrost
Length of output: 891
🏁 Script executed:
# Search for where RuneDetail is actually used/returned
rg -n 'RuneDetail' orchestrator/packages/task-source-bifrost/Repository: devzeebo/bifrost
Length of output: 541
🏁 Script executed:
# Search for GET /api/rune or similar endpoint definitions
rg -n 'rune.*detail|GET.*rune|/api/rune' orchestrator/packages/task-source-bifrost/ -iRepository: devzeebo/bifrost
Length of output: 2398
🏁 Script executed:
cat -n orchestrator/packages/task-source-bifrost/src/bifrost-task-source.ts | head -100Repository: devzeebo/bifrost
Length of output: 3886
🏁 Script executed:
# Read around line 57 where getRune is called
sed -n '50,80p' orchestrator/packages/task-source-bifrost/src/bifrost-task-source.tsRepository: devzeebo/bifrost
Length of output: 987
🏁 Script executed:
# Read the spec test to see what's being tested for rune detail mapping
sed -n '283,350p' orchestrator/packages/task-source-bifrost/src/bifrost-task-source.spec.tsRepository: devzeebo/bifrost
Length of output: 2351
🏁 Script executed:
# Check if there's any mention of Task or metadata in the task-source
rg -n 'metadata\|notes\|acceptanceCriteria' orchestrator/packages/task-source-bifrost/src/ -B2 -A2Repository: devzeebo/bifrost
Length of output: 42
🏁 Script executed:
# Read the `#mapToTask` method
sed -n '98,150p' orchestrator/packages/task-source-bifrost/src/bifrost-task-source.tsRepository: devzeebo/bifrost
Length of output: 1174
🏁 Script executed:
# Also check if there's a Task type definition in task-source package
find orchestrator/packages/task-source/src -name "*.ts" | head -10Repository: devzeebo/bifrost
Length of output: 313
🏁 Script executed:
# Check the task-source package for Task and metadata definition
rg -n 'type Task\|interface Task' orchestrator/packages/task-source/src/ -A 20 | head -60Repository: devzeebo/bifrost
Length of output: 42
🏁 Script executed:
# Read the full method parameter type and see if notes/acceptanceCriteria/retro are expected
sed -n '98,145p' orchestrator/packages/task-source-bifrost/src/bifrost-task-source.tsRepository: devzeebo/bifrost
Length of output: 1174
🏁 Script executed:
# Check the Task metadata type definition
cat orchestrator/packages/task-source/src/types.tsRepository: devzeebo/bifrost
Length of output: 871
RuneDetail is missing notes, acceptanceCriteria, and retro fields
The RuneDetail type does not include these fields, yet the #mapToTask method initializes them as empty arrays in the returned Task.metadata. This means any data these fields might contain from the Bifrost API response would be lost. Verify whether the Bifrost /api/rune endpoint returns these collections and add them to RuneDetail if so.
🤖 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-bifrost/src/types.ts` around lines 27 - 33,
The RuneDetail type is missing the notes, acceptanceCriteria, and retro
collections which `#mapToTask` currently initializes in Task.metadata; update the
RuneDetail definition (the exported RuneDetail type) to include optional fields
notes, acceptanceCriteria, and retro (as arrays) with the correct element shape
based on the Bifrost /api/rune response (e.g., string[] or an object type
matching Bifrost's items), and then ensure `#mapToTask` reads those arrays instead
of overriding them with empty arrays so existing data from the API is preserved.
| console.log = () => {}; | ||
| console.warn = () => {}; | ||
| console.error = () => {}; | ||
| console.info = () => {}; |
There was a problem hiding this comment.
Blanket console suppression will hide real test failures and cannot be selectively restored.
Permanently overwriting all four console methods with no-ops has two concrete risks:
console.erroris swallowed globally. React's internal errors, unhandled promise rejections surfaced through libraries, and your ownconsole.errordiagnostics will all disappear silently. A failing test that emits an error will look green with no signal.- The originals are never saved/restored. Because
setupFilesruns once before the entire suite (not per-test), there is no way for an individual test to re-enable output, assert that an error was logged, or recover the original implementation.
The idiomatic Vitest approach is to use vi.spyOn in a beforeEach/afterEach pair, which gives you per-test isolation, automatic restore, and the ability to assert calls:
♻️ Proposed replacement using `vi.spyOn`
-console.log = () => {};
-console.warn = () => {};
-console.error = () => {};
-console.info = () => {};
+import { beforeEach, vi } from 'vitest';
+
+beforeEach(() => {
+ vi.spyOn(console, 'log').mockImplementation(() => {});
+ vi.spyOn(console, 'info').mockImplementation(() => {});
+ vi.spyOn(console, 'warn').mockImplementation(() => {});
+ vi.spyOn(console, 'error').mockImplementation(() => {});
+});Vitest automatically restores all spies after each test when restoreMocks: true is set in vitest.config.ts, or you can call vi.restoreAllMocks() in an afterEach. This also lets individual tests assert expect(console.error).toHaveBeenCalledWith(...) when needed.
📝 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.
| console.log = () => {}; | |
| console.warn = () => {}; | |
| console.error = () => {}; | |
| console.info = () => {}; | |
| import { beforeEach, vi } from 'vitest'; | |
| beforeEach(() => { | |
| vi.spyOn(console, 'log').mockImplementation(() => {}); | |
| vi.spyOn(console, 'info').mockImplementation(() => {}); | |
| vi.spyOn(console, 'warn').mockImplementation(() => {}); | |
| vi.spyOn(console, 'error').mockImplementation(() => {}); | |
| }); |
🤖 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/vitest.setup.ts` around lines 1 - 4, Replace the blanket no-op
overrides of console.log/warn/error/info in vitest.setup.ts with per-test spies:
remove the global assignments to console.log, console.warn, console.error,
console.info and instead use vi.spyOn(console, 'error'|'warn'|'log'|'info')
inside a beforeEach/afterEach pair (or rely on restoreMocks: true in
vitest.config.ts or call vi.restoreAllMocks() in afterEach) so each test can
assert calls and originals are automatically restored; ensure tests that need to
suppress output create their own spies/mocks rather than permanently mutating
the global console.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.claude/skills/typescript/hooks/PostToolUse.d/lint-capture:
- Around line 5-9: The sentinel path and file name handling is unsafe and allows
blank entries; store the sentinel path expansion once (e.g. SENTINEL="$(sentinal
lint)") and use the quoted variable everywhere instead of unquoted $(sentinal
lint); check FILE_NAME (from FILE_NAME=$(get_field "tool_input.file_path")) is
non-empty before appending (only echo when FILE_NAME is not empty), and quote
all expansions (e.g. echo "$FILE_NAME" >> "$SENTINEL" and sort -u "$SENTINEL" -o
"$SENTINEL") to avoid word-splitting and globbing.
In @.claude/skills/typescript/hooks/Stop.d/lint-oxlint:
- Around line 4-7: The current check using wc -l and unquoted expansions fails
when the sentinel file (invoked via sentinal lint) is missing and breaks on
filenames with whitespace; change the logic to first test for the sentinel
file's existence and readability, then read filenames safely (e.g. mapfile -t
FILES < "$(sentinal lint)" or while IFS= read -r line; do FILES+=( "$line" );
done < "$(sentinal lint)"), check the array length ([ "${`#FILES`[@]}" -gt 0 ])
instead of wc -l, and run lint with null-delimited input to handle spaces (e.g.
printf '%s\0' "${FILES[@]}" | xargs -0 npm run lint") capturing output into
OUTPUT and preserving the existing error handling that calls block with the
OUTPUT; ensure all variable expansions and command substitutions are quoted.
🪄 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: fd32da0d-2497-419d-9c2b-8e031073c1e8
📒 Files selected for processing (6)
.claude/hooks/dispatch.claude/settings.json.claude/skills/typescript/hooks/PostToolUse.d/lint-capture.claude/skills/typescript/hooks/Stop.d/lint-oxlintorchestrator/packages/task-source-bifrost/package.jsonorchestrator/packages/task-source-bifrost/src/bifrost-task-source.ts
✅ Files skipped from review due to trivial changes (1)
- orchestrator/packages/task-source-bifrost/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- orchestrator/packages/task-source-bifrost/src/bifrost-task-source.ts
| FILE_NAME=$(get_field "tool_input.file_path") | ||
|
|
||
| echo "$FILE_NAME" >> $(sentinal lint) | ||
|
|
||
| sort -u $(sentinal lint) -o $(sentinal lint) No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "lint-capture" -type f 2>/dev/null | head -20Repository: devzeebo/bifrost
Length of output: 121
🏁 Script executed:
cat -n "./.claude/skills/typescript/hooks/PostToolUse.d/lint-capture"Repository: devzeebo/bifrost
Length of output: 292
Quote the sentinel path and skip empty file-path entries.
The code risks word-splitting or globbing on the unquoted $(sentinal lint) expansion on lines 7 and 9. Additionally, blank lines are appended to the sentinel file when tool_input.file_path is empty. Store the sentinel path in a quoted variable, check that FILE_NAME is non-empty before appending, and quote all variable expansions:
Suggested patch
-FILE_NAME=$(get_field "tool_input.file_path")
+FILE_NAME="$(get_field "tool_input.file_path")"
+SENTINEL_FILE="$(sentinal lint)"
-echo "$FILE_NAME" >> $(sentinal lint)
+if [[ -n "$FILE_NAME" ]]; then
+ printf '%s\n' "$FILE_NAME" >> "$SENTINEL_FILE"
+ sort -u "$SENTINEL_FILE" -o "$SENTINEL_FILE"
+fi
-sort -u $(sentinal lint) -o $(sentinal lint)📝 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.
| FILE_NAME=$(get_field "tool_input.file_path") | |
| echo "$FILE_NAME" >> $(sentinal lint) | |
| sort -u $(sentinal lint) -o $(sentinal lint) | |
| FILE_NAME="$(get_field "tool_input.file_path")" | |
| SENTINEL_FILE="$(sentinal lint)" | |
| if [[ -n "$FILE_NAME" ]]; then | |
| printf '%s\n' "$FILE_NAME" >> "$SENTINEL_FILE" | |
| sort -u "$SENTINEL_FILE" -o "$SENTINEL_FILE" | |
| fi |
🤖 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 @.claude/skills/typescript/hooks/PostToolUse.d/lint-capture around lines 5 -
9, The sentinel path and file name handling is unsafe and allows blank entries;
store the sentinel path expansion once (e.g. SENTINEL="$(sentinal lint)") and
use the quoted variable everywhere instead of unquoted $(sentinal lint); check
FILE_NAME (from FILE_NAME=$(get_field "tool_input.file_path")) is non-empty
before appending (only echo when FILE_NAME is not empty), and quote all
expansions (e.g. echo "$FILE_NAME" >> "$SENTINEL" and sort -u "$SENTINEL" -o
"$SENTINEL") to avoid word-splitting and globbing.
| if [[ "$(wc -l <$(sentinal lint))" -gt 0 ]]; then | ||
| OUTPUT=$(cat $(sentinal lint) | xargs npm run lint) || { | ||
| block "You MUST fix the following lint errors, even if you think they are unrelated to your changes:\n$OUTPUT" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the file mentioned in the review
fd -a "lint-oxlint" --type fRepository: devzeebo/bifrost
Length of output: 42
🏁 Script executed:
# Also check the directory structure around it
ls -la .claude/skills/typescript/hooks/ 2>/dev/null || echo "Directory not found, searching for similar paths"
fd -a "Stop.d" --type dRepository: devzeebo/bifrost
Length of output: 282
🏁 Script executed:
# Search for files with "oxlint" in the name
find . -name "*oxlint*" -type f 2>/dev/null | head -20Repository: devzeebo/bifrost
Length of output: 132
🏁 Script executed:
cat -n "./.claude/skills/typescript/hooks/Stop.d/lint-oxlint"Repository: devzeebo/bifrost
Length of output: 364
🏁 Script executed:
# Search for the sentinal function definition
rg "sentinal" --type bash --type sh -B 2 -A 5Repository: devzeebo/bifrost
Length of output: 89
🏁 Script executed:
# Also check if there's a .bashrc or similar that might define it
find . -name ".bashrc" -o -name ".bash_profile" -o -name "*.bashrc" 2>/dev/null | head -10Repository: devzeebo/bifrost
Length of output: 42
🏁 Script executed:
# Search for sentinal function without type filter
rg "sentinal" -B 2 -A 5Repository: devzeebo/bifrost
Length of output: 42
🏁 Script executed:
# Check the Stop.d directory for other scripts
ls -la "./.claude/skills/typescript/hooks/Stop.d/"Repository: devzeebo/bifrost
Length of output: 229
🏁 Script executed:
# Look for function definitions in the directory
cat -n "./.claude/skills/typescript/hooks/Stop.d/"* 2>/dev/null | head -50Repository: devzeebo/bifrost
Length of output: 364
🏁 Script executed:
# Check the PostToolUse.d directory for comparison
ls -la "./.claude/skills/typescript/hooks/PostToolUse.d/"
cat -n "./.claude/skills/typescript/hooks/PostToolUse.d/"*Repository: devzeebo/bifrost
Length of output: 462
🏁 Script executed:
# Search for where sentinal might be defined or used
rg "sentinel" --ignore-caseRepository: devzeebo/bifrost
Length of output: 42
🏁 Script executed:
# Check for .claude configuration files
find .claude -type f -name "*.json" -o -name "*.yaml" -o -name "*.yml" -o -name "*.sh" | head -20Repository: devzeebo/bifrost
Length of output: 82
Make lint execution robust for missing sentinel and filename parsing.
Current flow errors when the sentinel file does not exist (unquoted wc -l redirects fail with "No such file or directory") and mishandles filenames containing whitespace (unquoted variables and xargs without proper delimiters).
Suggested patch
-if [[ "$(wc -l <$(sentinal lint))" -gt 0 ]]; then
- OUTPUT=$(cat $(sentinal lint) | xargs npm run lint) || {
- block "You MUST fix the following lint errors, even if you think they are unrelated to your changes:\n$OUTPUT"
- }
+SENTINEL_FILE="$(sentinal lint)"
+if [[ -s "$SENTINEL_FILE" ]]; then
+ mapfile -t LINT_FILES < "$SENTINEL_FILE"
+ OUTPUT="$(npm run lint -- "${LINT_FILES[@]}" 2>&1)" || {
+ block "You MUST fix the following lint errors, even if you think they are unrelated to your changes:\n$OUTPUT"
+ }
fi📝 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.
| if [[ "$(wc -l <$(sentinal lint))" -gt 0 ]]; then | |
| OUTPUT=$(cat $(sentinal lint) | xargs npm run lint) || { | |
| block "You MUST fix the following lint errors, even if you think they are unrelated to your changes:\n$OUTPUT" | |
| } | |
| SENTINEL_FILE="$(sentinal lint)" | |
| if [[ -s "$SENTINEL_FILE" ]]; then | |
| mapfile -t LINT_FILES < "$SENTINEL_FILE" | |
| OUTPUT="$(npm run lint -- "${LINT_FILES[@]}" 2>&1)" || { | |
| block "You MUST fix the following lint errors, even if you think they are unrelated to your changes:\n$OUTPUT" | |
| } | |
| fi |
🤖 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 @.claude/skills/typescript/hooks/Stop.d/lint-oxlint around lines 4 - 7, The
current check using wc -l and unquoted expansions fails when the sentinel file
(invoked via sentinal lint) is missing and breaks on filenames with whitespace;
change the logic to first test for the sentinel file's existence and
readability, then read filenames safely (e.g. mapfile -t FILES < "$(sentinal
lint)" or while IFS= read -r line; do FILES+=( "$line" ); done < "$(sentinal
lint)"), check the array length ([ "${`#FILES`[@]}" -gt 0 ]) instead of wc -l, and
run lint with null-delimited input to handle spaces (e.g. printf '%s\0'
"${FILES[@]}" | xargs -0 npm run lint") capturing output into OUTPUT and
preserving the existing error handling that calls block with the OUTPUT; ensure
all variable expansions and command substitutions are quoted.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bifrost/ui/src/components/Wizard/Wizard.tsx (1)
16-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against empty
stepsto prevent a render-time crash.
steps[currentStep]at Line 75 throws whensteps.length === 0. Please add a guard path before dereferencing step content.💡 Proposed fix
export const Wizard: React.FC<WizardProps> = ({ steps, onComplete, colors = DEFAULT_COLORS }) => { const [currentStep, setCurrentStep] = useState(0); + + if (steps.length === 0) { + return null; + } const isLastStep = currentStep === steps.length - 1; const isFirstStep = currentStep === 0;Also applies to: 75-75
🤖 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 `@bifrost/ui/src/components/Wizard/Wizard.tsx` around lines 16 - 20, The Wizard component currently dereferences steps[currentStep] and will crash when steps is empty; add a guard at the top of the Wizard function that checks if (!steps || steps.length === 0) and return a safe fallback (e.g., null or a placeholder UI) so later uses of currentStep, isLastStep, isFirstStep and any reference to steps[currentStep] are never executed when steps is empty; ensure the guard appears before computing isLastStep/isFirstStep or rendering step content so the component never dereferences steps[0].
♻️ Duplicate comments (1)
orchestrator/packages/core/src/agent-parser.ts (1)
12-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandlebars built-ins/data variables are still treated as template parameters.
Lines 12–27 still add tokens like
else,this,@index, and helper names, which can reject valid templates as undeclared parameters.Proposed patch
+const HANDLEBARS_BUILTINS = new Set(["else", "this", "true", "false", "null", "log", "lookup"]); + const extractHandlebarsTokens = (content: string): Set<string> => { const tokens = new Set<string>(); @@ while ((match = simpleTokenRegex.exec(content)) !== null) { const token = match[1].trim(); - // Extract the base path (first part before any dots or spaces) - const [basePath] = token.split(".")[0].split(" "); - tokens.add(basePath); + const [pathToken] = token.split(/\s+/); + if ( + pathToken && + !pathToken.startsWith("@") && + !HANDLEBARS_BUILTINS.has(pathToken) + ) { + tokens.add(pathToken); + } } @@ while ((match = blockTokenRegex.exec(content)) !== null) { const token = match[1].trim(); - const [basePath] = token.split(".")[0].split(" "); - tokens.add(basePath); + const [pathToken] = token.split(/\s+/); + if ( + pathToken && + !pathToken.startsWith("@") && + !HANDLEBARS_BUILTINS.has(pathToken) + ) { + tokens.add(pathToken); + } }🤖 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/core/src/agent-parser.ts` around lines 12 - 27, The parser is currently adding Handlebars built-ins and data vars (e.g., else, this, `@index`) as template parameters; update the logic around simpleTokenRegex and blockTokenRegex where you compute basePath and call tokens.add(basePath) to filter these out: create a small set/array of built-in names (e.g., "else","this","if","unless","each", plus other helpers) and skip any basePath that is empty, startsWith("@"), is purely numeric, or is found in that built-ins set before calling tokens.add; apply the same filtering for both simpleTokenRegex and blockTokenRegex code paths so Handlebars built-ins are not treated as parameters.
🧹 Nitpick comments (1)
orchestrator/packages/core/vite.config.ts (1)
2-3: ⚡ Quick winRemove
@ts-ignoreon the vite.base importThe
vite.base.tsmodule atorchestrator/vite.base.tsexports a properly typed default function with explicit parameter and return types (ViteBaseOptions → UserConfig). The import path../../vite.basecorrectly resolves fromorchestrator/packages/core/. This suppression appears unnecessary and can hide real issues; prefer fixing the underlying cause if TypeScript errors occur.Note: This pattern is repeated across multiple
vite.config.tsfiles. If removal requires configuration changes, apply consistently.Proposed cleanup
import pkg from "./package.json"; -// `@ts-ignore` import base from "../../vite.base"; import tsconfig from "./tsconfig.json";🤖 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/core/vite.config.ts` around lines 2 - 3, Remove the unnecessary "// `@ts-ignore`" above the import of vite.base: delete the suppression and make the import statement `import base from "../../vite.base";` rely on TypeScript checking; if TypeScript reports errors, fix the underlying typing by ensuring the module exported in vite.base (the default export function and its ViteBaseOptions → UserConfig signature) is correctly declared and exported, and make any equivalent fixes in the other vite.config.ts files so imports match the named/default export and project tsconfig/moduleResolution settings (adjust export/import style or tsconfig as needed) rather than silencing the error.
🤖 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 `@bifrost/ui/src/components/Wizard/Wizard.tsx`:
- Line 36: getStepColor currently does modulo with colors.length which yields
NaN when colors is an empty array; update getStepColor to first check that
colors && colors.length > 0 and return a sensible fallback (e.g., a default
color constant or colorsDefault[0]) when empty, otherwise use colors[stepIndex %
colors.length]; reference the getStepColor function and the colors variable to
locate and fix the guard around the modulo operation.
In `@bifrost/ui/src/pages/realms/`@id/+Page.tsx:
- Around line 263-271: The add/remove member handlers set setIsLoading(true) but
never reset it; update both handlers (the blocks that call setIsLoading(true),
await api.getRealm(realm.id), setRealm(...), setRealmMemberIds(...)) to reset
loading in their finally clauses by calling setIsLoading(false) (alongside the
existing setIsAssigning(false)), so that after the try/catch the UI is no longer
stuck in loading; modify the finally blocks for those flows to include
setIsLoading(false).
In `@orchestrator/packages/core/src/agent-parser.ts`:
- Around line 17-18: The token extraction currently truncates nested paths by
using token.split(".")[0].split(" "), which only keeps the first dot-segment;
change the token parsing in the token processing block that builds the tokens
Set (where tokens and basePath are defined) to first trim the token, split on
whitespace to get the whole token string, then split on '.' to preserve all
nested segments and add each incremental parent path (e.g., a.b.c -> a, a.b,
a.b.c) to tokens; also fix the parent traversal logic that uses index -= 2 to
instead decrement by 1 so intermediate parent levels are not skipped (update the
loop that manipulates index during upward traversal accordingly).
- Around line 80-83: The code only checks that data.tools is an array but later
force-casts it to string[] (data.tools -> line ~164), which can allow non-string
elements to slip through; update the validation to verify every element is a
string (e.g., Array.isArray(data.tools) && data.tools.every(t => typeof t ===
'string')) and if not, log an error and return null (or normalize by
filtering/mapping to strings) before the cast; change the site that currently
force-casts to use the validated/normalized array instead of directly casting.
In `@orchestrator/packages/engine/src/test-engine.ts`:
- Around line 30-56: The duration timing starts after the simulated delay so
stats.durationMs is wrong and sendFollowUp() uses the configured delay rather
than measured elapsed time; move the startTime measurement in execute() to the
very beginning of the method (before any simulated delay) so the simulatedDelay
is included in elapsed time, and update sendFollowUp() to compute durationMs
from the actual elapsed time (Date.now() - startTime or by passing the measured
startTime into sendFollowUp) instead of using this.#config.simulateDelay; adjust
where stats.durationMs is assigned in execute() and sendFollowUp() to use that
measured elapsed value.
- Around line 4-10: TestEngineConfig's optional property mockStats is typed as
Partial<EngineResult["stats"]> but is being assigned null; change the default to
undefined (or remove the explicit null assignment) or update the type to allow
null (e.g., Partial<EngineResult["stats"]> | null) so it conforms to
strictNullChecks; update the code paths that read mockStats (in TestEngineConfig
usage) to handle undefined if you choose undefined as the default.
In `@orchestrator/packages/task-source-bifrost/src/config/config-loader.ts`:
- Line 11: The instance method is calling a static method incorrectly: change
the call from this.isValidConfig(config) to reference the static on the class
(ConfigLoader.isValidConfig(config)) or alternatively make isValidConfig an
instance method by removing the static modifier; update the code that currently
invokes isValidConfig in the ConfigLoader instance method to use the chosen
approach so the runtime TypeError is resolved.
- Line 2: The import and static-method usage are wrong: replace importing
readFile from "node:path" with importing readFile from "node:fs/promises" (keep
join from "node:path" if needed) so await readFile(...) works, and call the
static validator using the class name (e.g., ConfigLoader.isValidConfig(...))
instead of this.isValidConfig; alternatively, remove the static modifier on
isValidConfig if you intend to call it on the instance—update references
accordingly to avoid the TypeError.
---
Outside diff comments:
In `@bifrost/ui/src/components/Wizard/Wizard.tsx`:
- Around line 16-20: The Wizard component currently dereferences
steps[currentStep] and will crash when steps is empty; add a guard at the top of
the Wizard function that checks if (!steps || steps.length === 0) and return a
safe fallback (e.g., null or a placeholder UI) so later uses of currentStep,
isLastStep, isFirstStep and any reference to steps[currentStep] are never
executed when steps is empty; ensure the guard appears before computing
isLastStep/isFirstStep or rendering step content so the component never
dereferences steps[0].
---
Duplicate comments:
In `@orchestrator/packages/core/src/agent-parser.ts`:
- Around line 12-27: The parser is currently adding Handlebars built-ins and
data vars (e.g., else, this, `@index`) as template parameters; update the logic
around simpleTokenRegex and blockTokenRegex where you compute basePath and call
tokens.add(basePath) to filter these out: create a small set/array of built-in
names (e.g., "else","this","if","unless","each", plus other helpers) and skip
any basePath that is empty, startsWith("@"), is purely numeric, or is found in
that built-ins set before calling tokens.add; apply the same filtering for both
simpleTokenRegex and blockTokenRegex code paths so Handlebars built-ins are not
treated as parameters.
---
Nitpick comments:
In `@orchestrator/packages/core/vite.config.ts`:
- Around line 2-3: Remove the unnecessary "// `@ts-ignore`" above the import of
vite.base: delete the suppression and make the import statement `import base
from "../../vite.base";` rely on TypeScript checking; if TypeScript reports
errors, fix the underlying typing by ensuring the module exported in vite.base
(the default export function and its ViteBaseOptions → UserConfig signature) is
correctly declared and exported, and make any equivalent fixes in the other
vite.config.ts files so imports match the named/default export and project
tsconfig/moduleResolution settings (adjust export/import style or tsconfig as
needed) rather than silencing the error.
🪄 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: d03d55fc-e0da-4afb-af15-3fdae123b426
📒 Files selected for processing (78)
.claude/hooks/dispatch.claude/skills/node/wanted-skills/package.json.md.claude/skills/typescript/hooks/PostToolUse.d/lint-capture.claude/skills/typescript/hooks/Stop.d/lint-oxlintbifrost/ui/src/components/Dialog/Dialog.spec.tsxbifrost/ui/src/components/Dialog/Dialog.tsxbifrost/ui/src/components/RealmSelector/RealmSelector.spec.tsxbifrost/ui/src/components/Toast/Toast.spec.tsxbifrost/ui/src/components/Toast/Toast.tsxbifrost/ui/src/components/TopNav/TopNav.spec.tsxbifrost/ui/src/components/Wizard/Wizard.spec.tsxbifrost/ui/src/components/Wizard/Wizard.tsxbifrost/ui/src/lib/api.spec.tsbifrost/ui/src/lib/api.tsbifrost/ui/src/lib/auth.tsxbifrost/ui/src/lib/realm.tsxbifrost/ui/src/lib/theme.tsxbifrost/ui/src/lib/toast.tsxbifrost/ui/src/pages/account/+Page.tsxbifrost/ui/src/pages/accounts/+Page.tsxbifrost/ui/src/pages/dashboard/+Page.tsxbifrost/ui/src/pages/onboarding/+Page.tsxbifrost/ui/src/pages/realms/+Page.tsxbifrost/ui/src/pages/realms/@id/+Page.tsxbifrost/ui/src/pages/runes/+Page.tsxbifrost/ui/src/pages/runes/@id/+Page.tsxbifrost/ui/src/types/account.tsbifrost/ui/src/types/realm.tsbifrost/ui/src/types/rune.tsbifrost/ui/src/types/session.tsorchestrator/packages/cli/src/config.spec.tsorchestrator/packages/cli/src/config.tsorchestrator/packages/cli/src/git-root.spec.tsorchestrator/packages/cli/src/index.spec.tsorchestrator/packages/cli/src/index.tsorchestrator/packages/cli/vite.config.tsorchestrator/packages/core/src/agent-parser.spec.tsorchestrator/packages/core/src/agent-parser.tsorchestrator/packages/core/src/handlebars-renderer.spec.tsorchestrator/packages/core/src/hook-executor.spec.tsorchestrator/packages/core/src/hook-executor.tsorchestrator/packages/core/src/index.tsorchestrator/packages/core/src/orchestrator.spec.tsorchestrator/packages/core/src/orchestrator.tsorchestrator/packages/core/src/repo-installer.spec.tsorchestrator/packages/core/src/repo-installer.tsorchestrator/packages/core/src/validator.spec.tsorchestrator/packages/core/src/validator.tsorchestrator/packages/core/vite.config.tsorchestrator/packages/engine/src/index.tsorchestrator/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.spec.tsorchestrator/packages/task-source-bifrost/src/bifrost-task-source.spec.tsorchestrator/packages/task-source-bifrost/src/bifrost-task-source.tsorchestrator/packages/task-source-bifrost/src/client/bifrost-http-client.spec.tsorchestrator/packages/task-source-bifrost/src/client/bifrost-http-client.tsorchestrator/packages/task-source-bifrost/src/config/config-loader.spec.tsorchestrator/packages/task-source-bifrost/src/config/config-loader.tsorchestrator/packages/task-source-bifrost/src/config/credential-loader.spec.tsorchestrator/packages/task-source-bifrost/src/config/credential-loader.tsorchestrator/packages/task-source-bifrost/src/index.tsorchestrator/packages/task-source-bifrost/src/integration.spec.tsorchestrator/packages/task-source-bifrost/src/types.tsorchestrator/packages/task-source-memory/src/index.tsorchestrator/packages/task-source-memory/src/memory-task-source.spec.tsorchestrator/packages/task-source-memory/src/memory-task-source.tsorchestrator/packages/task-source-memory/vite.config.tsorchestrator/packages/task-source/src/index.tsorchestrator/packages/task-source/src/interface.spec.tsorchestrator/packages/task-source/src/interface.tsorchestrator/packages/task-source/src/types.spec.tsorchestrator/packages/task-source/src/types.tsorchestrator/vite.base.tsorchestrator/vitest.setup.tsoxlint.config.ts
✅ Files skipped from review due to trivial changes (28)
- bifrost/ui/src/components/Dialog/Dialog.spec.tsx
- orchestrator/packages/task-source-memory/src/index.ts
- bifrost/ui/src/types/session.ts
- .claude/skills/node/wanted-skills/package.json.md
- orchestrator/packages/task-source-bifrost/src/index.ts
- orchestrator/packages/task-source/src/types.ts
- bifrost/ui/src/components/Toast/Toast.spec.tsx
- orchestrator/packages/cli/src/git-root.spec.ts
- bifrost/ui/src/lib/toast.tsx
- orchestrator/packages/core/src/handlebars-renderer.spec.ts
- bifrost/ui/src/lib/auth.tsx
- bifrost/ui/src/pages/runes/+Page.tsx
- orchestrator/packages/core/src/agent-parser.spec.ts
- bifrost/ui/src/lib/theme.tsx
- bifrost/ui/src/pages/dashboard/+Page.tsx
- bifrost/ui/src/components/Toast/Toast.tsx
- orchestrator/packages/cli/src/index.spec.ts
- bifrost/ui/src/components/Dialog/Dialog.tsx
- orchestrator/packages/task-source-bifrost/src/client/bifrost-http-client.spec.ts
- bifrost/ui/src/components/RealmSelector/RealmSelector.spec.tsx
- orchestrator/packages/cli/src/config.spec.ts
- bifrost/ui/src/components/TopNav/TopNav.spec.tsx
- bifrost/ui/src/lib/api.spec.ts
- orchestrator/packages/core/src/repo-installer.spec.ts
- bifrost/ui/src/pages/accounts/+Page.tsx
- bifrost/ui/src/lib/api.ts
- bifrost/ui/src/pages/runes/@id/+Page.tsx
- bifrost/ui/src/pages/account/+Page.tsx
🚧 Files skipped from review as they are similar to previous changes (42)
- orchestrator/packages/task-source/src/index.ts
- orchestrator/packages/engine/src/index.ts
- .claude/skills/typescript/hooks/PostToolUse.d/lint-capture
- orchestrator/packages/engine/src/interface.ts
- orchestrator/packages/cli/vite.config.ts
- orchestrator/packages/task-source-bifrost/src/config/config-loader.spec.ts
- orchestrator/vitest.setup.ts
- orchestrator/packages/task-source-bifrost/src/types.ts
- orchestrator/packages/core/src/validator.spec.ts
- orchestrator/packages/cli/src/index.ts
- orchestrator/packages/task-source-bifrost/src/config/credential-loader.ts
- orchestrator/vite.base.ts
- orchestrator/packages/task-source/src/types.spec.ts
- oxlint.config.ts
- bifrost/ui/src/types/rune.ts
- orchestrator/packages/core/src/index.ts
- orchestrator/packages/task-source/src/interface.ts
- orchestrator/packages/task-source-bifrost/src/client/bifrost-http-client.ts
- orchestrator/packages/core/src/repo-installer.ts
- orchestrator/packages/task-source/src/interface.spec.ts
- bifrost/ui/src/lib/realm.tsx
- orchestrator/packages/core/src/orchestrator.spec.ts
- .claude/skills/typescript/hooks/Stop.d/lint-oxlint
- bifrost/ui/src/components/Wizard/Wizard.spec.tsx
- orchestrator/packages/task-source-memory/src/memory-task-source.ts
- orchestrator/packages/core/src/hook-executor.ts
- orchestrator/packages/task-source-memory/vite.config.ts
- orchestrator/packages/task-source-bifrost/src/bifrost-task-source.spec.ts
- orchestrator/packages/engine/src/interface.spec.ts
- orchestrator/packages/engine/src/types.spec.ts
- orchestrator/packages/task-source-bifrost/src/bifrost-task-source.ts
- orchestrator/packages/engine/src/test-engine.spec.ts
- .claude/hooks/dispatch
- orchestrator/packages/task-source-bifrost/src/integration.spec.ts
- orchestrator/packages/cli/src/config.ts
- bifrost/ui/src/pages/realms/+Page.tsx
- orchestrator/packages/core/src/validator.ts
- orchestrator/packages/core/src/orchestrator.ts
- orchestrator/packages/task-source-memory/src/memory-task-source.spec.ts
- bifrost/ui/src/pages/onboarding/+Page.tsx
- orchestrator/packages/task-source-bifrost/src/config/credential-loader.spec.ts
- orchestrator/packages/core/src/hook-executor.spec.ts
| const getStepColor = (stepIndex: number) => { | ||
| return colors[stepIndex % colors.length] || colors[0]; | ||
| }; | ||
| const getStepColor = (stepIndex: number) => colors[stepIndex % colors.length] || colors[0]; |
There was a problem hiding this comment.
Handle an empty colors array before modulo math.
If colors is passed as [], stepIndex % colors.length becomes NaN, and step colors resolve to undefined.
💡 Proposed fix
- const getStepColor = (stepIndex: number) => colors[stepIndex % colors.length] || colors[0];
+ const palette = colors.length > 0 ? colors : DEFAULT_COLORS;
+ const getStepColor = (stepIndex: number) => palette[stepIndex % palette.length];🤖 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 `@bifrost/ui/src/components/Wizard/Wizard.tsx` at line 36, getStepColor
currently does modulo with colors.length which yields NaN when colors is an
empty array; update getStepColor to first check that colors && colors.length > 0
and return a sensible fallback (e.g., a default color constant or
colorsDefault[0]) when empty, otherwise use colors[stepIndex % colors.length];
reference the getStepColor function and the colors variable to locate and fix
the guard around the modulo operation.
| setIsLoading(true); | ||
| const realmData = await api.getRealm(realm.id); | ||
| setRealm(normalizeRealmDetail(realmData) ?? toFallbackRealm(realm.id)); | ||
| setRealmMemberIds(extractRealmMemberIds(realmData)); | ||
| } catch { | ||
| showToast('Error', 'Failed to add account to realm', 'error'); | ||
| showToast("Error", "Failed to add account to realm", "error"); | ||
| } finally { | ||
| setIsAssigning(false); | ||
| } |
There was a problem hiding this comment.
Reset isLoading after add/remove member flows.
isLoading is set to true on Line 263 and Line 290, but never set back to false in those handlers. This can leave the page stuck on the loading screen after a successful mutation/refresh.
Proposed fix
} catch {
showToast("Error", "Failed to add account to realm", "error");
} finally {
setIsAssigning(false);
+ setIsLoading(false);
}
};
@@
} catch {
showToast("Error", "Failed to remove member", "error");
} finally {
setIsRemovingMember(false);
+ setIsLoading(false);
}
};Also applies to: 290-302
🤖 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 `@bifrost/ui/src/pages/realms/`@id/+Page.tsx around lines 263 - 271, The
add/remove member handlers set setIsLoading(true) but never reset it; update
both handlers (the blocks that call setIsLoading(true), await
api.getRealm(realm.id), setRealm(...), setRealmMemberIds(...)) to reset loading
in their finally clauses by calling setIsLoading(false) (alongside the existing
setIsAssigning(false)), so that after the try/catch the UI is no longer stuck in
loading; modify the finally blocks for those flows to include
setIsLoading(false).
| const [basePath] = token.split(".")[0].split(" "); | ||
| tokens.add(basePath); |
There was a problem hiding this comment.
Nested token validation is truncated and parent traversal skips valid levels.
Line 17/25 strips context.prDescription down to context, so undeclared nested fields can pass. Also, Line 106 uses index -= 2, which skips intermediate parent paths.
Proposed patch
- // Extract the base path (first part before any dots or spaces)
- const [basePath] = token.split(".")[0].split(" ");
- tokens.add(basePath);
+ // Keep full path token (e.g. context.prDescription)
+ const [pathToken] = token.split(/\s+/);
+ tokens.add(pathToken);
@@
- const [basePath] = token.split(".")[0].split(" ");
- tokens.add(basePath);
+ const [pathToken] = token.split(/\s+/);
+ tokens.add(pathToken);
@@
- for (let index = parts.length; index > 0; index -= 2) {
+ for (let index = parts.length; index > 0; index -= 1) {
const parentPath = parts.slice(0, index).join(".");Also applies to: 25-26, 106-107
🤖 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/core/src/agent-parser.ts` around lines 17 - 18, The
token extraction currently truncates nested paths by using
token.split(".")[0].split(" "), which only keeps the first dot-segment; change
the token parsing in the token processing block that builds the tokens Set
(where tokens and basePath are defined) to first trim the token, split on
whitespace to get the whole token string, then split on '.' to preserve all
nested segments and add each incremental parent path (e.g., a.b.c -> a, a.b,
a.b.c) to tokens; also fix the parent traversal logic that uses index -= 2 to
instead decrement by 1 so intermediate parent levels are not skipped (update the
loop that manipulates index during upward traversal accordingly).
| if (!Array.isArray(data.tools)) { | ||
| console.error("Missing or invalid required field: tools"); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Validate tools element types before casting to string[].
Line 80 only checks array shape; Line 164 force-casts values to string[]. Non-string entries can pass parsing and break downstream consumers.
Proposed patch
- if (!Array.isArray(data.tools)) {
+ if (!Array.isArray(data.tools) || !data.tools.every((tool) => typeof tool === "string")) {
console.error("Missing or invalid required field: tools");
return null;
}
+ const tools = data.tools as string[];
@@
- tools: data.tools as string[],
+ tools,Also applies to: 164-164
🤖 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/core/src/agent-parser.ts` around lines 80 - 83, The
code only checks that data.tools is an array but later force-casts it to
string[] (data.tools -> line ~164), which can allow non-string elements to slip
through; update the validation to verify every element is a string (e.g.,
Array.isArray(data.tools) && data.tools.every(t => typeof t === 'string')) and
if not, log an error and return null (or normalize by filtering/mapping to
strings) before the cast; change the site that currently force-casts to use the
validated/normalized array instead of directly casting.
| export type TestEngineConfig = { | ||
| success?: boolean; | ||
| lastMessage?: string; | ||
| simulateError?: boolean; | ||
| simulateDelay?: number; // milliseconds | ||
| mockStats?: Partial<EngineResult["stats"]>; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify TypeScript nullability settings and locate relevant config usage.
fd -i 'tsconfig*.json' -t f -x sh -c 'echo "==> $1"; cat "$1" | rg -n "\"strict\"|\"strictNullChecks\"|\"noUncheckedIndexedAccess\"" -n' sh {}
rg -n --type=ts 'mockStats\s*:\s*null|mockStats\?\s*:\s*Partial<EngineResult\["stats"\]>'Repository: devzeebo/bifrost
Length of output: 636
🏁 Script executed:
cat orchestrator/packages/engine/tsconfig.jsonRepository: devzeebo/bifrost
Length of output: 277
🏁 Script executed:
sed -n '20,30p' orchestrator/packages/engine/src/test-engine.tsRepository: devzeebo/bifrost
Length of output: 324
🏁 Script executed:
cat orchestrator/tsconfig.base.jsonRepository: devzeebo/bifrost
Length of output: 470
🏁 Script executed:
rg -n "type EngineResult|interface EngineResult" orchestrator/packages/engine/src/ -A 10Repository: devzeebo/bifrost
Length of output: 807
🏁 Script executed:
find orchestrator/packages/engine -name "*.json" -type f | head -20Repository: devzeebo/bifrost
Length of output: 145
🏁 Script executed:
rg -n "mockStats" orchestrator/packages/engine/src/test-engine.ts -B 5 -A 5Repository: devzeebo/bifrost
Length of output: 1021
🏁 Script executed:
cat orchestrator/packages/engine/package.jsonRepository: devzeebo/bifrost
Length of output: 427
mockStats default value violates its declared type.
Line 9 declares mockStats?: Partial<EngineResult["stats"]>, but line 25 sets it to null. Under strictNullChecks (enabled in tsconfig.base.json), optional properties cannot be assigned null—only undefined or the specified type. This is a type error that will fail the build.
Proposed fix
simulateError: false,
simulateDelay: 0,
- mockStats: null,
+ mockStats: {},
...config,🤖 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/engine/src/test-engine.ts` around lines 4 - 10,
TestEngineConfig's optional property mockStats is typed as
Partial<EngineResult["stats"]> but is being assigned null; change the default to
undefined (or remove the explicit null assignment) or update the type to allow
null (e.g., Partial<EngineResult["stats"]> | null) so it conforms to
strictNullChecks; update the code paths that read mockStats (in TestEngineConfig
usage) to handle undefined if you choose undefined as the default.
| public async execute(context: EngineContext): Promise<EngineResult> { | ||
| // Apply simulated delay if configured | ||
| if (this.#config.simulateDelay && this.#config.simulateDelay > 0) { | ||
| await new Promise((resolve) => setTimeout(resolve, this.#config.simulateDelay)); | ||
| } | ||
|
|
||
| // Simulate error if configured | ||
| if (this.#config.simulateError) { | ||
| throw new Error("Simulated engine error"); | ||
| } | ||
|
|
||
| const startTime = Date.now(); | ||
|
|
||
| const defaultStats: EngineResult["stats"] = { | ||
| durationMs: 0, | ||
| inputTokens: 100, | ||
| outputTokens: 50, | ||
| cacheReadTokens: 10, | ||
| cacheCreationTokens: 5, | ||
| totalCostUsd: 0.005, | ||
| numTurns: 1, | ||
| }; | ||
|
|
||
| const stats: EngineResult["stats"] = { ...defaultStats, ...this.#config.mockStats }; | ||
|
|
||
| stats.durationMs = Date.now() - startTime; | ||
|
|
There was a problem hiding this comment.
durationMs is measured inconsistently and misses simulated delay in execute().
execute() starts timing at Line 41 after the delay, so Line 55 reports near-zero runtime. sendFollowUp() (Line 72) uses configured delay instead of actual elapsed time. This skews orchestrator stats accumulation.
Proposed fix
public async execute(context: EngineContext): Promise<EngineResult> {
+ const startTime = Date.now();
// Apply simulated delay if configured
if (this.#config.simulateDelay && this.#config.simulateDelay > 0) {
await new Promise((resolve) => setTimeout(resolve, this.#config.simulateDelay));
}
@@
- const startTime = Date.now();
-
@@
const stats: EngineResult["stats"] = { ...defaultStats, ...this.#config.mockStats };
-
- stats.durationMs = Date.now() - startTime;
+ stats.durationMs = Date.now() - startTime;
@@
public async sendFollowUp(message: string): Promise<EngineResult> {
+ const startTime = Date.now();
// Apply simulated delay if configured
if (this.#config.simulateDelay && this.#config.simulateDelay > 0) {
await new Promise((resolve) => setTimeout(resolve, this.#config.simulateDelay));
}
@@
const stats: EngineResult["stats"] = {
- durationMs: this.#config.simulateDelay ?? 10,
+ durationMs: Date.now() - startTime,
inputTokens: 50,
outputTokens: 25,Also applies to: 65-79
🤖 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/engine/src/test-engine.ts` around lines 30 - 56, The
duration timing starts after the simulated delay so stats.durationMs is wrong
and sendFollowUp() uses the configured delay rather than measured elapsed time;
move the startTime measurement in execute() to the very beginning of the method
(before any simulated delay) so the simulatedDelay is included in elapsed time,
and update sendFollowUp() to compute durationMs from the actual elapsed time
(Date.now() - startTime or by passing the measured startTime into sendFollowUp)
instead of using this.#config.simulateDelay; adjust where stats.durationMs is
assigned in execute() and sendFollowUp() to use that measured elapsed value.
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
♻️ Duplicate comments (4)
orchestrator/packages/task-source-bifrost/src/config/credential-loader.ts (1)
29-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
isValidCredentialsstill allowscredentials: null, which can crash at Line 20.The current guard accepts
{ credentials: null }becausetypeof null === "object", socredentials.credentials[normalizedUrl]can throw instead of returning the intended validation error.Proposed fix
private static isValidCredentials(credentials: unknown): credentials is BifrostCredentials { + if ( + typeof credentials !== "object" || + credentials === null || + Array.isArray(credentials) || + !("credentials" in credentials) + ) { + return false; + } + + const map = (credentials as { credentials: unknown }).credentials; return ( - typeof credentials === "object" && - credentials !== null && - "credentials" in credentials && - typeof (credentials as { credentials: unknown }).credentials === "object" + typeof map === "object" && + map !== null && + !Array.isArray(map) ); }🤖 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-bifrost/src/config/credential-loader.ts` around lines 29 - 35, The type guard isValidCredentials currently treats credentials.credentials === null as valid (because typeof null === "object"); update isValidCredentials to also check that (credentials as { credentials: unknown }).credentials is not null and is a plain object (e.g., use !== null and typeof ... === "object") and optionally ensure it has the expected shape (like being a record or having the normalizedUrl key) so callers (e.g., code indexing credentials.credentials[normalizedUrl]) cannot crash when accessing properties.orchestrator/packages/task-source-bifrost/src/config/config-loader.ts (1)
6-12:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject empty/whitespace
urlandrealmvalues at validation time.
isValidConfigcurrently acceptsurl: ""/realm: " ", so invalid config passes load-time checks and fails later in less clear ways.Suggested fix
export const isValidConfig = (config: unknown): config is { url: string; realm: string } => typeof config === "object" && config !== null && "url" in config && "realm" in config && typeof (config as { url: unknown }).url === "string" && - typeof (config as { realm: unknown }).realm === "string"; + (config as { url: string }).url.trim().length > 0 && + typeof (config as { realm: unknown }).realm === "string" && + (config as { realm: string }).realm.trim().length > 0;🤖 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-bifrost/src/config/config-loader.ts` around lines 6 - 12, isValidConfig currently only checks types and allows empty or whitespace-only strings for url and realm; update isValidConfig so that after confirming config is an object and url/realm are strings, also verify (config as { url: string }).url.trim().length > 0 and (config as { realm: string }).realm.trim().length > 0 to reject empty or whitespace-only values. Locate and update the isValidConfig function to add these trimmed non-empty checks for url and realm so invalid configs fail validation early..claude/hooks/dispatch (1)
58-58:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPath traversal / command injection: unsanitized path components in exec.
The final line constructs and executes a path from three unsanitized inputs:
$1— skill name (CLI argument)$2— handler name (CLI argument)${3:-$(get_field hook_event_name)}— event name override or from JSON payloadIf any contain
../,/, or shell metacharacters, an attacker could execute arbitrary scripts outside.claude/skills/. Validate that all components are safe before constructing the path:🛡️ Proposed fix
+# Validate path components contain only safe characters +for component in "$1" "$2"; do + if [[ "$component" =~ [^a-zA-Z0-9_-] || "$component" == *..* ]]; then + echo "dispatch: invalid path component: $component" >&2 + exit 1 + fi +done + +hook_event="${3:-$(get_field hook_event_name)}" +if [[ "$hook_event" =~ [^a-zA-Z0-9_-] || "$hook_event" == *..* || -z "$hook_event" ]]; then + echo "dispatch: invalid or empty hook_event_name" >&2 + exit 1 +fi + -$CLAUDE_PROJECT_DIR/.claude/skills/$1/hooks/${3:-$(get_field hook_event_name)}.d/$2 +exec "$CLAUDE_PROJECT_DIR/.claude/skills/$1/hooks/$hook_event.d/$2"🤖 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 @.claude/hooks/dispatch at line 58, The dispatch line builds and execs a path from untrusted inputs ($1, $2, ${3:-$(get_field hook_event_name)}) which allows path traversal/command injection; fix by validating and canonicalizing each component before using them: restrict $1, $2 and the event name to a safe allowlist/regex (e.g., alphanumerics, hyphen/underscore only), strip any `..` or `/` and shell metacharacters, or use basename on each component, then join them and resolve with realpath and assert the resolved path is inside the intended base directory ($CLAUDE_PROJECT_DIR/.claude/skills) before executing; also invoke the handler without a shell (use exec with argv array / execve) instead of passing a single shell-interpolated string.bifrost/ui/src/pages/realms/@id/+Page.tsx (1)
267-275:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset
isLoadingafter the member add/remove refreshes.Both handlers set
isLoading(true)before refetching, but neitherfinallyblock restores it. After a successful mutation, the page can stay on the full-screen loading state indefinitely.Suggested fix
} catch { showToast("Error", "Failed to add account to realm", "error"); } finally { setIsAssigning(false); + setIsLoading(false); } }; @@ } catch { showToast("Error", "Failed to remove member", "error"); } finally { setIsRemovingMember(false); + setIsLoading(false); } };Also applies to: 294-306
🤖 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 `@bifrost/ui/src/pages/realms/`@id/+Page.tsx around lines 267 - 275, The handlers that refetch realm data call setIsLoading(true) but never reset it, leaving the UI stuck; update the try/catch/finally blocks in the add/remove member handlers (the code that calls setIsLoading(true), api.getRealm, normalizeRealmDetail, extractRealmMemberIds and uses setIsAssigning) to ensure the finally clause calls setIsLoading(false) (in addition to setIsAssigning(false)), and apply the same change to the other handler that mirrors this logic so both member-add and member-remove flows always clear the loading flag.
🟡 Minor comments (13)
bifrost/ui/src/pages/runes/new/Relationships.tsx-61-88 (1)
61-88:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winExpose toggle state for direction buttons with
aria-pressed.These buttons act as a 2-state toggle but don’t expose pressed state to assistive tech. Add
aria-pressedto both so screen-reader users can identify the active direction.Suggested fix
<button type="button" onClick={() => onRelationshipDirectionChange("depends_on")} + aria-pressed={relationshipDirection === "depends_on"} className="px-3 py-2 text-xs font-bold uppercase tracking-wider"<button type="button" onClick={() => onRelationshipDirectionChange("depended_on_by")} + aria-pressed={relationshipDirection === "depended_on_by"} className="px-3 py-2 text-xs font-bold uppercase tracking-wider"🤖 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 `@bifrost/ui/src/pages/runes/new/Relationships.tsx` around lines 61 - 88, The direction toggle buttons in Relationships.tsx (the button elements that call onRelationshipDirectionChange and read relationshipDirection) need an accessible pressed state; add aria-pressed to each button and set it to the boolean expression comparing relationshipDirection to the button's direction (e.g., aria-pressed={relationshipDirection === "depends_on"} for the "Depends On" button and similarly for "depended_on_by") so assistive tech can identify the active state.orchestrator/packages/task-source-bifrost/src/config/config-loader.spec.ts-12-13 (1)
12-13:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid hardcoded
"/tmp"in tests; useos.tmpdir()for portability.Line 12 assumes a Unix filesystem path, breaking test execution on non-Unix environments (Windows, etc.). The same issue exists in
bifrost-task-source.spec.ts,credential-loader.spec.ts, andintegration.spec.ts.Suggested fix
import { mkdir, rm, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { randomBytes } from "node:crypto"; +import { tmpdir } from "node:os"; import { loadConfig } from "./config-loader"; @@ - tempDir = join("/tmp", `bifrost-test-${randomBytes(8).toString("hex")}`); + tempDir = join(tmpdir(), `bifrost-test-${randomBytes(8).toString("hex")}`);🤖 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-bifrost/src/config/config-loader.spec.ts` around lines 12 - 13, The test hardcodes "/tmp" when constructing tempDir; change it to use the platform temp directory by importing and calling os.tmpdir() (replace the string literal in the join call used to create tempDir where join("/tmp", `bifrost-test-${randomBytes(8).toString("hex")}`) is used), and keep the rest (randomBytes and mkdir(..., { recursive: true })) the same so the test becomes portable across OSes.bifrost/ui/src/pages/account/PATList.tsx-83-83 (1)
83-83:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBug: Incorrect shadow reset on mouse leave.
The
onMouseLeavehandler setsboxShadowto"var(--shadow-soft-hover)", but it should reset to the original value"var(--shadow-soft)"(as defined on line 74).🐛 Proposed fix
onMouseLeave={(_event) => { - _event.currentTarget.style.boxShadow = "var(--shadow-soft-hover)"; + _event.currentTarget.style.boxShadow = "var(--shadow-soft)"; _event.currentTarget.style.transform = "translate(0, 0)"; }}🤖 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 `@bifrost/ui/src/pages/account/PATList.tsx` at line 83, The onMouseLeave handler in PATList.tsx mistakenly sets _event.currentTarget.style.boxShadow to "var(--shadow-soft-hover)"; change it to reset to the original value "var(--shadow-soft)". Locate the onMouseLeave callback (where _event.currentTarget.style.boxShadow is assigned) and replace the hover shadow string with "var(--shadow-soft)" so it restores the pre-hover shadow used earlier in the component..claude/skills/typescript/hooks/common.d/quality-20-28 (1)
20-28:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winQuote the workspace parameter.
Line 21 has the same unquoted
$1issue as thelint()function.🛡️ Proposed fix
test() { - OUTPUT=$(npm run test -s -w $1) || { + OUTPUT=$(npm run test -s -w "$1" 2>&1) || { if [[ "$HOOK" == "SessionStart" ]]; then additionalContext "$OUTPUT" else block "$OUTPUT" fi } }Note: Added
2>&1to capture stderr as well, which is useful for test failures.🤖 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 @.claude/skills/typescript/hooks/common.d/quality around lines 20 - 28, The test() hook currently passes the unquoted positional workspace parameter $1 to npm (-w $1) which breaks when the workspace contains spaces; update the call inside test() to quote the workspace parameter (use -w "$1") and also capture stderr by appending 2>&1 so OUTPUT contains both stdout and stderr; apply the same quoting pattern used in lint() to ensure consistent behavior for $1, and leave the existing error handling that uses OUTPUT and HOOK unchanged..claude/skills/typescript/hooks/Stop.d/lint-oxlint-14-14 (1)
14-14:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winQuote the
$SENTINELvariable expansion.Shell variable expansions should always be quoted to prevent word splitting and glob expansion.
🛡️ Proposed fix
-rm $SENTINEL +rm "$SENTINEL"🤖 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 @.claude/skills/typescript/hooks/Stop.d/lint-oxlint at line 14, The rm invocation uses an unquoted variable expansion (rm $SENTINEL) which can cause word splitting or globbing; update the call to quote the expansion and protect against flags by using rm -- "$SENTINEL" (or validate non-empty before calling) so the sentinel filename is removed safely..claude/skills/typescript/hooks/common.d/quality-7-18 (1)
7-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winQuote function parameters and add JSON parsing error handling.
Two issues in the
lint()function:
- Line 8:
$1is unquoted — shell best practice requires quoting all variable expansions- Lines 9-10: If
npm run lintfails for reasons other than lint errors (e.g., missing dependency, invalid config),OUTPUTwon't be valid JSON andjqwill fail with a cryptic error🛡️ Proposed fix
lint() { - OUTPUT=$(npm run lint -s -w $1 -- --format=json) || { - FIRST_FILE=$(echo "$OUTPUT" | jq -r '.diagnostics[0].filename') - FILE_ERRORS=$(echo "$OUTPUT" | jq -rc "[.diagnostics[] | select(.filename == \"$FIRST_FILE\")]") + OUTPUT=$(npm run lint -s -w "$1" -- --format=json 2>&1) || { + # Validate OUTPUT is valid JSON before parsing + if ! echo "$OUTPUT" | jq -e . >/dev/null 2>&1; then + # npm run lint failed for non-lint reasons + if [[ "$HOOK" == "SessionStart" ]]; then + additionalContext "Linting failed: $OUTPUT" + else + block "Linting failed: $OUTPUT" + fi + return + fi + + FIRST_FILE=$(echo "$OUTPUT" | jq -r '.diagnostics[0].filename // empty') + if [[ -z "$FIRST_FILE" ]]; then + # No diagnostics in JSON output + if [[ "$HOOK" == "SessionStart" ]]; then + additionalContext "Linting failed with no diagnostics: $OUTPUT" + else + block "Linting failed with no diagnostics: $OUTPUT" + fi + return + fi + + FILE_ERRORS=$(echo "$OUTPUT" | jq -rc "[.diagnostics[] | select(.filename == \"$FIRST_FILE\")]") if [[ "$HOOK" == "SessionStart" ]]; then additionalContext "$FILE_ERRORS" else block "$FILE_ERRORS" fi } }🤖 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 @.claude/skills/typescript/hooks/common.d/quality around lines 7 - 18, In the lint() function, quote the positional parameter when passing to npm (use "$1" instead of $1) to avoid word-splitting and globbing, and add robust JSON parsing/error handling around OUTPUT before calling jq: detect if OUTPUT is valid JSON (or if npm failed for non-lint reasons) and handle that case (log or block with a clear error) instead of directly piping potentially invalid OUTPUT into jq; update references to OUTPUT, FIRST_FILE and FILE_ERRORS logic so you only compute FIRST_FILE/FILE_ERRORS after validating OUTPUT and preserve existing behavior for HOOK/SessionStart branching.bifrost/ui/package.json-11-11 (1)
11-11:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
prettiertodevDependenciesor reconfigure workspace dependency sharing.The
formatscript fails with "prettier: not found" when executed from the bifrost/ui directory, even though prettier exists in the workspace root. The npm run format command requires prettier to be available in bifrost/ui's dependency context.Either add
prettieras a devDependency in bifrost/ui/package.json, or configure the workspace to properly expose root dependencies to this package.🤖 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 `@bifrost/ui/package.json` at line 11, The "format" npm script in bifrost/ui's package.json invokes "prettier" but prettier isn't available in this package's dependency context; add "prettier" to bifrost/ui's devDependencies in package.json (or update workspace package hoisting/config so root prettier is exposed to this package) so the "format" script can resolve and run, ensuring the package.json's "format" script remains "prettier --write ." and that package manager lock/install is updated afterwards..claude/skills/typescript/hooks/Stop.d/lint-oxlint-8-12 (1)
8-12:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFilename whitespace handling:
xargswill split paths incorrectly.Line 9 pipes file paths to
xargswithout null-delimited input. If any TypeScript file path contains spaces,xargswill treat it as multiple files and lint will fail.🛡️ Proposed fix using null delimiters
if [[ -n "$TS_FILES" ]]; then - OUTPUT=$(echo "$TS_FILES" | xargs npm run lint) || { + OUTPUT=$(echo "$TS_FILES" | tr '\n' '\0' | xargs -0 npm run lint 2>&1) || { block "You MUST fix the following lint errors, even if you think they are unrelated to your changes:\n$OUTPUT" } fiAlternatively, use a while-read loop:
if [[ -n "$TS_FILES" ]]; then - OUTPUT=$(echo "$TS_FILES" | xargs npm run lint) || { + LINT_ARGS=() + while IFS= read -r file; do + LINT_ARGS+=("$file") + done <<< "$TS_FILES" + + OUTPUT=$(npm run lint -- "${LINT_ARGS[@]}" 2>&1) || { block "You MUST fix the following lint errors, even if you think they are unrelated to your changes:\n$OUTPUT" } fi🤖 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 @.claude/skills/typescript/hooks/Stop.d/lint-oxlint around lines 8 - 12, The current lint invocation pipes $TS_FILES into xargs which will split on whitespace and break paths with spaces; update the lint invocation in the block that sets OUTPUT and calls xargs so it consumes null-delimited filenames (e.g., produce null-delimited input and call xargs with -0) or switch to a safe while-read loop that reads filenames with read -r -d '' and invokes npm run lint for each path; target the variables and commands TS_FILES, OUTPUT and the xargs/npm run lint use in that conditional to ensure filenames with spaces are handled correctly.bifrost/ui/src/pages/realms/new/+Page.tsx-124-142 (1)
124-142:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
isSubmittingindicator below the wizard is mostly unreachable.
Wizard.onCompleteis fired synchronously by clicking Done;handleSubmitflipsisSubmitting=true, awaits the network call, then either navigates away or resets it infinally. Untilawait api.createRealmresolves the wizard remains visible too, but there's no disable on the Done button — the user can click Done multiple times and fire concurrentcreateRealmcalls. Either pass a disabled/loading hook intoWizard, or guardhandleSubmitwith an early return whenisSubmittingis already 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 `@bifrost/ui/src/pages/realms/new/`+Page.tsx around lines 124 - 142, The submit flow allows multiple concurrent submissions because Wizard.onComplete calls handleSubmit synchronously and the Done button isn't disabled while isSubmitting is true; update handleSubmit to immediately return if isSubmitting is already true (guard at start of handleSubmit referencing isSubmitting), and/or pass a loading/disabled prop into the Wizard component (e.g., disabled={isSubmitting} or loading={isSubmitting}) so the Done button is disabled/indicates progress while awaiting api.createRealm; ensure any finally block still resets isSubmitting only when appropriate and that navigation after a successful api.createRealm remains unchanged.bifrost/ui/src/lib/api.spec.ts-10-20 (1)
10-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDuplicate
beforeEachblock.Lines 10–14 and 16–20 are byte-identical. Vitest will run both before every test, doubling the work and (more importantly) causing confusion when someone later changes only one of them. Drop the second copy.
🧹 Proposed fix
beforeEach(() => { apiClient = new ApiClient(); mockFetch = vi.fn(); globalThis.fetch = mockFetch as unknown; }); - - beforeEach(() => { - apiClient = new ApiClient(); - mockFetch = vi.fn(); - globalThis.fetch = mockFetch as unknown; - });🤖 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 `@bifrost/ui/src/lib/api.spec.ts` around lines 10 - 20, There are two identical beforeEach blocks that initialize apiClient, mockFetch, and globalThis.fetch (both using new ApiClient(), vi.fn(), and assigning mockFetch to globalThis.fetch); remove the duplicate second beforeEach so the setup runs only once and keep the single block that initializes ApiClient, mockFetch, and globalThis.fetch.bifrost/ui/src/pages/runes/new/PrioritySelector.tsx-16-21 (1)
16-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEdge case:
Number("")is0, which passes theNumber.isNaNguard.If
onValueChangeever fires with an empty string (e.g., deselection in a future Base UI release or withtoggleMultipleenabled),nextPrioritybecomes0andonPriorityChange(0)is called — outside the valid 1–4 range. Tighten the guard to also enforce membership in the allowed set.🛡️ Proposed fix
onValueChange={([nextPriorityString]) => { const nextPriority = Number(nextPriorityString); - if (!Number.isNaN(nextPriority)) { + if (Number.isInteger(nextPriority) && nextPriority >= 1 && nextPriority <= 4) { onPriorityChange(nextPriority); } }}🤖 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 `@bifrost/ui/src/pages/runes/new/PrioritySelector.tsx` around lines 16 - 21, The current onValueChange handler in PrioritySelector.tsx converts the incoming string to a number and only checks Number.isNaN, which lets Number("") => 0 slip through; update the guard in the onValueChange callback (the code that computes nextPriority and calls onPriorityChange) to also verify that nextPriority is one of the allowed values (1,2,3,4) before calling onPriorityChange(nextPriority), so ignore empty-string/invalid values and only invoke onPriorityChange for membership in the set {1,2,3,4}.bifrost/ui/src/lib/api.ts-496-500 (1)
496-500:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
accountIdis not URL-encoded ingetPATs.Every other read endpoint (
getRune,getRealm,getAdminAccount) goes throughencodeURIComponent, butgetPATsinterpolatesaccountIddirectly. If anaccount_idever contains&,#,+, or whitespace it will produce a malformed query string and either silently mismatch or fail server-side validation. UseencodeURIComponenthere for consistency and safety.🛡️ Proposed fix
public async getPATs(accountId: string): Promise<PatEntry[]> { - return this.request<PatEntry[]>(`/pats?account_id=${accountId}`, { + return this.request<PatEntry[]>(`/pats?account_id=${encodeURIComponent(accountId)}`, { method: "GET", }); }🤖 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 `@bifrost/ui/src/lib/api.ts` around lines 496 - 500, The getPATs method interpolates accountId directly into the query string which can break for characters like &, #, + or spaces; update the getPATs function to URL-encode accountId using encodeURIComponent when building `/pats?account_id=...` so it matches the pattern used by getRune/getRealm/getAdminAccount and prevents malformed queries or server validation errors.bifrost/ui/src/pages/login/+Page.tsx-43-46 (1)
43-46:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTrim the PAT before sending it.
Validation uses
pat.trim(), but the payload still sends rawpat. A copied token with trailing whitespace will enable the button and then fail login unexpectedly.Suggested fix
- await login({ pat, remember_me: rememberMe }); + await login({ pat: pat.trim(), remember_me: rememberMe });🤖 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 `@bifrost/ui/src/pages/login/`+Page.tsx around lines 43 - 46, Validation uses pat.trim() but the login payload sends the raw pat, causing failures with trailing whitespace; update the login call in the block where setIsLoading(true) is set so it sends a trimmed token (use pat.trim()) instead of pat (i.e., call login({ pat: pat.trim(), remember_me: rememberMe })) and ensure any other places that construct the payload (or call login) also use the trimmed value to keep behavior consistent with the validation.
🧹 Nitpick comments (14)
bifrost/ui/src/pages/account/PATList.tsx (1)
76-85: ⚡ Quick winConsider using CSS hover states instead of inline event handlers.
Directly manipulating DOM styles via event handlers bypasses React's declarative model and makes the component harder to maintain. Consider using Tailwind's hover utilities or a CSS class toggle instead.
♻️ Proposed refactor using Tailwind hover utilities
Remove the
onMouseEnterandonMouseLeavehandlers and update the button styling:<Button onClick={() => onSetPatToRevoke(pat.id)} disabled={revokingPATId === pat.id} - className="px-3 py-1 text-xs font-bold uppercase tracking-wider transition-all duration-150 disabled:opacity-50" + className="px-3 py-1 text-xs font-bold uppercase tracking-wider transition-all duration-150 disabled:opacity-50 hover:enabled:translate-x-[1px] hover:enabled:translate-y-[1px]" style={{ backgroundColor: "var(--color-red)", border: "2px solid var(--color-border)", color: "white", boxShadow: "var(--shadow-soft)", + '--tw-shadow-hover': 'var(--shadow-soft-hover)', }} - onMouseEnter={(_event) => { - if (revokingPATId !== pat.id) { - _event.currentTarget.style.boxShadow = "var(--shadow-soft-hover)"; - _event.currentTarget.style.transform = "translate(1px, 1px)"; - } - }} - onMouseLeave={(_event) => { - _event.currentTarget.style.boxShadow = "var(--shadow-soft-hover)"; - _event.currentTarget.style.transform = "translate(0, 0)"; - }} >Then add a custom CSS class that applies the hover shadow via CSS variables in your global styles.
🤖 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 `@bifrost/ui/src/pages/account/PATList.tsx` around lines 76 - 85, The inline onMouseEnter/onMouseLeave handlers in PATList.tsx that check revokingPATId !== pat.id and directly set _event.currentTarget.style.boxShadow and .transform should be removed; instead, add a conditional CSS class to the button (e.g., apply a base class plus a hover class when revokingPATId !== pat.id) and use Tailwind hover utilities or a custom CSS class that reads the same CSS variables (var(--shadow-soft-hover) and transform translate) so the shadow/translate behavior is handled declaratively via CSS rather than DOM style mutation.bifrost/ui/src/pages/accounts/@id/+Page.tsx (1)
134-134: ⚡ Quick winRemove or use the
realmFilterparameter.The
_realmFilterparameter is destructured but never used in the component body. The underscore prefix acknowledges it's unused, but it's still included in the props destructuring. Consider either using it (e.g., for controlled input state) or removing it from the props interface and destructuring.♻️ Option 1: Remove from destructuring if truly unused
const _AssignRoleDialog = ({ open, onClose, onAssign, accountId, username, - _realmFilter, setRealmFilter, selectedRealmId,🤖 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 `@bifrost/ui/src/pages/accounts/`@id/+Page.tsx at line 134, The destructured parameter _realmFilter in the +Page component is unused; either remove it from the props destructuring and the props/interface/type definition (so delete _realmFilter from where props are defined and from the destructuring in +Page.tsx) or wire it into the component (e.g., use it to initialize controlled state or pass it to the filter input) — locate the destructuring where _realmFilter is listed and update the props type and all call sites accordingly to keep types consistent with the chosen approach..claude/hooks/dispatch (1)
42-48: 💤 Low valueConsider validating
session_idfor path safety.While unlikely, if
session_idfrom the hook payload contains/or.., it could create sentinel files outside/tmp/.claude/. Adding basic validation would harden this function:🛡️ Optional validation
sentinel() { SESSION_ID=$(get_field "session_id") + + # Validate session_id contains no path separators + if [[ "$SESSION_ID" == */* || "$SESSION_ID" == *..]]; then + echo "Invalid session_id" >&2 + return 1 + fi mkdir -p /tmp/.claude - echo "/tmp/.claude/$SESSION_ID.$1.sentinel" }🤖 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 @.claude/hooks/dispatch around lines 42 - 48, The sentinel() helper currently uses SESSION_ID=$(get_field "session_id") without validating it, which can allow path traversal; update sentinel() to validate/sanitize the value returned by get_field: ensure SESSION_ID contains only a safe charset (e.g. alphanumerics, dot, underscore, hyphen) or explicitly reject values containing "/" or ".." and handle errors (exit non‑zero or fallback to a safe generated id); keep the existing mkdir -p /tmp/.claude and then construct the path using the validated SESSION_ID when echoing "/tmp/.claude/$SESSION_ID.$1.sentinel".bifrost/ui/src/pages/runes/new/+Page.tsx (3)
154-168: ⚡ Quick win
isSubmittingnot reset on the navigate-success path is fine, but consider the unmount race.On success the component navigates away, so leaving
isSubmitting=trueis harmless (component unmounts). On error you correctly reset it. Just confirmnavigatesynchronously unmounts the page; if it queues a navigation, an extra click on Create Rune between the resolve and the unmount could fire a secondapi.createRune. Aif (isSubmitting) return;guard at the top ofhandleSubmitwould close that gap cheaply.🤖 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 `@bifrost/ui/src/pages/runes/new/`+Page.tsx around lines 154 - 168, Add a guard at the start of the handleSubmit function to return early if isSubmitting is true to prevent double submits/race between resolve and navigate; ensure you set setIsSubmitting(true) immediately before kicking off the async api.createRune call (so the guard works) and keep the existing error path that sets setIsSubmitting(false) — this prevents a second api.createRune while navigation is queued without changing other behavior.
130-139: 💤 Low valueVerify the
depends_on→blocked_by/ else →blocksmapping is correct.For each selected relationship, the code maps
relationship.direction === "depends_on"to the wire value"blocked_by", and anything else to"blocks". That impliesRelationshipDirectionis a binary union and the only "other" value is"blocks"(or similar). If a third direction is ever added, it will silently be mis-encoded as"blocks". Aswitch(or exhaustiveness check) makes the intent explicit and breaks the build when the union grows.🤖 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 `@bifrost/ui/src/pages/runes/new/`+Page.tsx around lines 130 - 139, The mapping of relationship.direction to the wire value in the selectedRelationships → api.addDependency call is currently a two-way ternary (relationship.direction === "depends_on" ? "blocked_by" : "blocks") which will silently mis-encode if RelationshipDirection gains a third variant; replace the ternary with an explicit switch (or pattern match) on relationship.direction inside the selectedRelationships.map callback and handle each known case (e.g., "depends_on" -> "blocked_by", "blocks" -> "blocks") and add a default that throws an error or uses a TypeScript exhaustiveness check so the compiler fails if RelationshipDirection is extended; update the mapping used for the relationship field passed to api.addDependency (where rune.id and selectedRealm are used) to use the new switch-based result.
188-191: ⚡ Quick win
navigate("/login")during render is a side-effect-in-render anti-pattern.Calling an imperative router from the render path triggers a navigation as a side effect of rendering, which can cause double-renders, hydration warnings under SSR/Vike, and is non-idiomatic in React 19 (which is stricter about render purity). Either route the redirect through
useEffect, or — to mirror the new auth pattern inrealms/new/+Page.tsx— render an inline "Authentication Required" panel instead of redirecting.♻️ Proposed fix using effect-based redirect
+ useEffect(() => { + if (!authLoading && !isAuthenticated) { + navigate("/login"); + } + }, [authLoading, isAuthenticated]); + if (!isAuthenticated) { - navigate("/login"); return null; }🤖 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 `@bifrost/ui/src/pages/runes/new/`+Page.tsx around lines 188 - 191, The render currently calls navigate("/login") when isAuthenticated is false which is a side-effect-in-render anti-pattern; change this by removing the direct navigate call from the render path and either (A) perform the redirect inside a useEffect that watches isAuthenticated and calls navigate("/login") (while rendering a placeholder or null), or (B) follow the pattern used in realms/new by returning an inline "Authentication Required" panel when isAuthenticated is false instead of navigating; update the conditional around isAuthenticated where navigate("/login") is used to reference the component’s useEffect (or the auth-panel) so no imperative navigation happens during render.bifrost/ui/src/pages/realms/new/+Page.tsx (1)
27-29: 💤 Low valueCurried setter is rebuilt each render and is invoked oddly.
handleInputChangereturns a new function on every render and at the call site you immediately invoke it:handleInputChange("name")(event.target.value). Just inline a direct setter, which is clearer and avoids per-render allocations:♻️ Proposed simplification
- const handleInputChange = (field: keyof FormData) => (value: string) => { - setFormData((prev) => ({ ...prev, [field]: value })); - }; + const handleInputChange = (field: keyof FormData, value: string) => { + setFormData((prev) => ({ ...prev, [field]: value })); + };Then call sites become
handleInputChange("name", event.target.value).🤖 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 `@bifrost/ui/src/pages/realms/new/`+Page.tsx around lines 27 - 29, The current curried setter handleInputChange creates a new function each render and is invoked as handleInputChange("name")(event.target.value); change handleInputChange to a direct setter function like handleInputChange(field: keyof FormData, value: string) that calls setFormData(prev => ({ ...prev, [field]: value })), and update all callers to call handleInputChange("name", event.target.value) (replace any handleInputChange("x")(...) usages); this removes per-render allocations and simplifies intent while keeping FormData and setFormData usage intact.bifrost/ui/src/lib/api.ts (2)
38-68: ⚖️ Poor tradeoff
request<TResult>returningnull as TResultfor 204 silently lies to callers.When
TResultis a non-nullable shape (e.g.RuneDetail), the cast letsnullflow into the return as that type, surfacing as a runtime null-deref far from the call site. Most public methods that hit 204 endpoints already declarePromise<void>, butgetSession()is the only one that explicitly accommodatesnullin its result type. Consider making the 204 path explicit by typingrequestas returningTResult | null(and forcing void callers to ignore it), or by exposing a separaterequestVoidhelper for endpoints that return no body. Not strictly broken today, but it's a sharp edge.🤖 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 `@bifrost/ui/src/lib/api.ts` around lines 38 - 68, The request<TResult> helper currently returns "null as TResult" on 204 responses which can hide a real null at runtime; change the signature to return Promise<TResult | null> (or add a separate requestVoid helper with Promise<void>) and update callers accordingly (e.g. getSession() already expects nullable) so consumers must handle the nullable result; adjust the 204 branch in request<TResult> to return null without a type cast and update any call sites that assume non-null TResult to handle or unwrap the null, keeping ApiError usage unchanged.
409-429: 💤 Low valueRedundant
realmIdparameter onassignRole/revokeRole.Both methods accept a
realmIdargument but already receiverequest.realm_id. The fallbackrealmId ?? request.realm_idmeans the explicit parameter wins only when it disagrees with the body — which is almost certainly a bug (you'd sendX-Bifrost-Realm: Awhile the body saysrealm_id: B). Recommend removing the parameter and always usingrequest.realm_id.🤖 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 `@bifrost/ui/src/lib/api.ts` around lines 409 - 429, The assignRole and revokeRole methods have a redundant realmId parameter that can mismatch request.realm_id; remove the optional realmId parameter from both function signatures (assignRole and revokeRole) and change the header computation to always use ApiClient.withRealmHeader(request.realm_id); update any call sites to stop passing realmId and ensure types reflect the new signatures so the body and X-Bifrost-Realm header always come from request.realm_id.bifrost/ui/src/pages/runes/new/StatusSelector.tsx (1)
23-36: 💤 Low valueHoist the static option list out of render.
["draft", "open"]is recreated on every render. Move it to a module-level constant typed asreadonly ("draft" | "open")[]to avoid the allocation and tighten typings (also removes the need for the runtime narrowing inonValueChange).🤖 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 `@bifrost/ui/src/pages/runes/new/StatusSelector.tsx` around lines 23 - 36, Hoist the inline array ["draft", "open"] out of render by creating a module-level constant (e.g., STATUS_OPTIONS) typed as readonly ("draft" | "open")[] and use that in the JSX map instead of recreating the array each render; update references in StatusSelector (the map over statusOption and the Toggle key/value) to use STATUS_OPTIONS and adjust onValueChange to rely on the tighter type so you can remove any ad-hoc runtime narrowing there.bifrost/ui/src/pages/runes/new/RuneForm.tsx (1)
32-32: 💤 Low value
autoFocusshifts focus on every mount and breaks expectations for keyboard/screen-reader users navigating in.Auto-focusing the title input is fine for a dedicated "new" page, but if
RuneFormever gets reused (e.g., inside a modal that mounts/unmounts) it will steal focus on each remount. Consider lifting the autofocus decision to the parent or making it a prop.🤖 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 `@bifrost/ui/src/pages/runes/new/RuneForm.tsx` at line 32, The title input currently includes a hard-coded autoFocus which will steal focus on every mount; update the RuneForm component to accept an optional prop (e.g., autoFocus?: boolean) and remove the hard-coded autoFocus from the title input JSX, then forward that prop into the input element (using the prop name) so parents can decide whether to autofocus (and the "new" page parent can pass true while modals or reused contexts pass false); also provide a sensible default (false) in RuneForm's props if desired.bifrost/ui/src/pages/runes/new/PrioritySelector.tsx (1)
24-29: 💤 Low valueHoist
PRIORITY_OPTIONSto module scope.The options array (and its closure) is rebuilt on every render. Hoisting it to a module-level
constis a small allocation/readability win, and applyingas constlets TypeScript infer narrower literal types.🤖 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 `@bifrost/ui/src/pages/runes/new/PrioritySelector.tsx` around lines 24 - 29, The inline priority options array inside the JSX causes a new allocation on every render; hoist it to module scope as a named constant (PRIORITY_OPTIONS) and declare it with as const to get literal types, then replace the inline array in PrioritySelector's render with PRIORITY_OPTIONS.map(...); this removes the closure allocation, improves readability, and gives TypeScript narrower types for the option values/labels.bifrost/ui/src/components/Wizard/Wizard.tsx (2)
107-231: 💤 Low valueStatic analysis flag on
dangerouslySetInnerHTMLis a false positive here.The CSS string is a hard-coded literal with no user-controlled interpolation, so XSS via this
<style>injection is not reachable. That said, since this is React 19 you could simply render the CSS as a normal child of<style>(<style>{css}</style>) which avoids the static-analysis noise without behavior change. Long term, migrating these styles to a CSS module / Tailwind utilities would let the repo lint rule stay strict.🤖 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 `@bifrost/ui/src/components/Wizard/Wizard.tsx` around lines 107 - 231, The style block in the Wizard component uses dangerouslySetInnerHTML which triggers static-analysis XSS warnings despite the CSS being a hard-coded string; replace the <style dangerouslySetInnerHTML={{ __html: `...` }} /> usage in the Wizard (file contains the Wizard component rendering the .wizard CSS) with a normal <style>{cssString}</style> child (or inline the template string as the style element's child) to remove the false-positive without changing behavior; later consider moving these rules into a CSS module or Tailwind classes for permanent lint friendliness.
60-84: 💤 Low valueAvoid using array index as React
key.
key={index}is an anti-pattern when the list could ever be reordered or items inserted; React will reuse component state across logically different steps. SinceWizardStepdoesn't have an inherent id, preferstep.title(assuming titles are unique) or have callers supply a stable id.🤖 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 `@bifrost/ui/src/components/Wizard/Wizard.tsx` around lines 60 - 84, The mapping in Wizard.tsx that renders the step indicators uses key={index} which can cause React to reuse state incorrectly; change the key on the rendered element in the Wizard component (the div with className "wizard-step-indicator" inside the steps.map) to a stable unique value such as step.title (or add and use a stable id property on WizardStep provided by callers) and ensure titles/ids are unique across steps before removing the index-based key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d3ca937c-cbd7-42f8-97d5-c4a2bd2b3dc3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (69)
.claude/agents/typescript-quality/AGENT.md.claude/hooks/dispatch.claude/settings.json.claude/skills/react/SKILL.md.claude/skills/typescript/hooks/PostToolUse.d/format.claude/skills/typescript/hooks/PostToolUse.d/lint-capture.claude/skills/typescript/hooks/Stop.d/lint-oxlint.claude/skills/typescript/hooks/common.d/quality.gitignorebifrost/ui/package.jsonbifrost/ui/src/components/Dialog/Dialog.tsxbifrost/ui/src/components/RealmSelector/RealmSelector.tsxbifrost/ui/src/components/Toast/Toast.spec.tsxbifrost/ui/src/components/Toast/Toast.tsxbifrost/ui/src/components/TopNav/TopNav.spec.tsxbifrost/ui/src/components/TopNav/TopNav.tsxbifrost/ui/src/components/Wizard/Wizard.tsxbifrost/ui/src/lib/api.spec.tsbifrost/ui/src/lib/api.tsbifrost/ui/src/lib/auth.tsxbifrost/ui/src/lib/realm.tsxbifrost/ui/src/lib/router.tsbifrost/ui/src/lib/theme.tsxbifrost/ui/src/lib/toast.tsxbifrost/ui/src/main.tsxbifrost/ui/src/pages/+Layout.tsxbifrost/ui/src/pages/+Wrapper.tsxbifrost/ui/src/pages/_error/+Page.tsxbifrost/ui/src/pages/account/+Page.tsxbifrost/ui/src/pages/account/PATList.tsxbifrost/ui/src/pages/accounts/+Page.tsxbifrost/ui/src/pages/accounts/@id/+Page.tsxbifrost/ui/src/pages/accounts/new/+Page.tsxbifrost/ui/src/pages/dashboard/+Page.tsxbifrost/ui/src/pages/index/+Page.tsxbifrost/ui/src/pages/login/+Page.tsxbifrost/ui/src/pages/onboarding/+Page.tsxbifrost/ui/src/pages/realms/+Page.tsxbifrost/ui/src/pages/realms/@id/+Page.tsxbifrost/ui/src/pages/realms/new/+Page.tsxbifrost/ui/src/pages/runes/+Page.tsxbifrost/ui/src/pages/runes/@id/+Page.tsxbifrost/ui/src/pages/runes/@id/edit/+Page.tsxbifrost/ui/src/pages/runes/new/+Page.tsxbifrost/ui/src/pages/runes/new/PrioritySelector.tsxbifrost/ui/src/pages/runes/new/Relationships.tsxbifrost/ui/src/pages/runes/new/RuneForm.tsxbifrost/ui/src/pages/runes/new/StatusSelector.tsxbifrost/ui/src/types/account.tsbifrost/ui/src/types/realm.tsbifrost/ui/src/types/rune.tsbifrost/ui/src/types/session.tsbifrost/ui/vitest.setup.tsorchestrator/package.jsonorchestrator/packages/cli/src/config.tsorchestrator/packages/core/src/hook-executor.spec.tsorchestrator/packages/core/src/validator.tsorchestrator/packages/engine/src/interface.spec.tsorchestrator/packages/task-source-bifrost/src/bifrost-task-source.spec.tsorchestrator/packages/task-source-bifrost/src/bifrost-task-source.tsorchestrator/packages/task-source-bifrost/src/config/config-loader.spec.tsorchestrator/packages/task-source-bifrost/src/config/config-loader.tsorchestrator/packages/task-source-bifrost/src/config/credential-loader.spec.tsorchestrator/packages/task-source-bifrost/src/config/credential-loader.tsorchestrator/packages/task-source/src/interface.spec.tsorchestrator/packages/task-source/src/types.tsorchestrator/vite.base.tsoxlint.config.tspackage.json
✅ Files skipped from review due to trivial changes (9)
- .claude/skills/typescript/hooks/PostToolUse.d/format
- bifrost/ui/src/pages/index/+Page.tsx
- .gitignore
- .claude/agents/typescript-quality/AGENT.md
- bifrost/ui/src/components/Toast/Toast.spec.tsx
- bifrost/ui/src/pages/_error/+Page.tsx
- orchestrator/packages/task-source-bifrost/src/config/credential-loader.spec.ts
- bifrost/ui/src/pages/+Layout.tsx
- bifrost/ui/src/pages/accounts/+Page.tsx
🚧 Files skipped from review as they are similar to previous changes (30)
- .claude/skills/typescript/hooks/PostToolUse.d/lint-capture
- bifrost/ui/src/lib/theme.tsx
- bifrost/ui/vitest.setup.ts
- orchestrator/packages/task-source/src/types.ts
- orchestrator/package.json
- bifrost/ui/src/lib/router.ts
- bifrost/ui/src/types/account.ts
- .claude/settings.json
- orchestrator/vite.base.ts
- oxlint.config.ts
- orchestrator/packages/task-source-bifrost/src/bifrost-task-source.spec.ts
- orchestrator/packages/core/src/hook-executor.spec.ts
- bifrost/ui/src/types/realm.ts
- bifrost/ui/src/components/RealmSelector/RealmSelector.tsx
- bifrost/ui/src/types/rune.ts
- bifrost/ui/src/components/Toast/Toast.tsx
- orchestrator/packages/engine/src/interface.spec.ts
- bifrost/ui/src/types/session.ts
- bifrost/ui/src/components/Dialog/Dialog.tsx
- orchestrator/packages/cli/src/config.ts
- orchestrator/packages/task-source/src/interface.spec.ts
- bifrost/ui/src/components/TopNav/TopNav.spec.tsx
- bifrost/ui/src/lib/auth.tsx
- orchestrator/packages/task-source-bifrost/src/bifrost-task-source.ts
- bifrost/ui/src/components/TopNav/TopNav.tsx
- bifrost/ui/src/pages/dashboard/+Page.tsx
- orchestrator/packages/core/src/validator.ts
- bifrost/ui/src/pages/accounts/new/+Page.tsx
- bifrost/ui/src/pages/onboarding/+Page.tsx
- bifrost/ui/src/pages/runes/@id/+Page.tsx
| deny() { | ||
| JSON=$(escape_json "$1") | ||
| echo '{"hookSpecificOutput":{"hookEventName":"'$(get_field hook_event_name)'","permissionDecision":"deny","permissionDecisionReason":"'$JSON'"}}' | ||
|
|
||
| exit 2 | ||
| } | ||
|
|
||
| block() { | ||
| JSON=$(escape_json "$1") | ||
| echo -e '{"hookSpecificOutput":{"hookEventName":"'$(get_field hook_event_name)'","decision":"block","reason":"'$JSON'"}}' >&2 | ||
|
|
||
| exit 2 | ||
| } | ||
|
|
||
| additionalContext() { | ||
| JSON=$(escape_json "$1") | ||
| echo -e '{"hookSpecificOutput":{"hookEventName":"'$(get_field hook_event_name)'","additionalContext":"'$JSON'"}}' | ||
|
|
||
| exit 0 | ||
| } |
There was a problem hiding this comment.
Shell/JSON injection: all three output functions are vulnerable.
The deny(), block(), and additionalContext() functions use escape_json() but then concatenate the result into a shell string literal. This approach is still vulnerable:
- If
$(get_field hook_event_name)contains"or other JSON metacharacters, it will produce malformed JSON - The shell string interpolation defeats the escaping done by
escape_json() - An attacker controlling hook payloads could inject arbitrary JSON keys
Use jq -n with --arg flags to construct JSON safely:
🛡️ Proposed fix for all three functions
deny() {
- JSON=$(escape_json "$1")
- echo '{"hookSpecificOutput":{"hookEventName":"'$(get_field hook_event_name)'","permissionDecision":"deny","permissionDecisionReason":"'$JSON'"}}'
-
+ jq -n \
+ --arg event "$(get_field hook_event_name)" \
+ --arg reason "$1" \
+ '{"hookSpecificOutput":{"hookEventName":$event,"permissionDecision":"deny","permissionDecisionReason":$reason}}'
exit 2
}
block() {
- JSON=$(escape_json "$1")
- echo -e '{"hookSpecificOutput":{"hookEventName":"'$(get_field hook_event_name)'","decision":"block","reason":"'$JSON'"}}' >&2
-
+ jq -n \
+ --arg event "$(get_field hook_event_name)" \
+ --arg reason "$1" \
+ '{"hookSpecificOutput":{"hookEventName":$event,"decision":"block","reason":$reason}}' >&2
exit 2
}
additionalContext() {
- JSON=$(escape_json "$1")
- echo -e '{"hookSpecificOutput":{"hookEventName":"'$(get_field hook_event_name)'","additionalContext":"'$JSON'"}}'
-
+ jq -n \
+ --arg event "$(get_field hook_event_name)" \
+ --arg context "$1" \
+ '{"hookSpecificOutput":{"hookEventName":$event,"additionalContext":$context}}'
exit 0
}If this fix is applied, the escape_json() function becomes unused and should be removed (or kept only if other code needs it).
🤖 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 @.claude/hooks/dispatch around lines 21 - 40, The deny(), block(), and
additionalContext() functions build JSON by concatenating shell-escaped strings
which can still be broken by characters in $(get_field hook_event_name) or
payloads; replace each manual JSON assembly with jq -n and --arg to safely
construct the JSON object (e.g. call get_field and capture its output into a
variable, then use jq -n --arg hook_event_name "$HOOK" --arg reason "$1"
'{hookSpecificOutput: {hookEventName:$hook_event_name,
permissionDecision:"deny", permissionDecisionReason:$reason}}' or similar for
each function, writing block’s output to stderr as currently done), keep the
same exit codes, and remove escape_json() if no other code uses it; reference
functions: deny, block, additionalContext, escape_json, get_field.
| The ONLY acceptable use for a useRef is to capture a native component using | ||
| `ref={myRef}`. In order to track state between calls, a useState, useMemo, and | ||
| useEffect ensures proper rerenders happen. useRef is too brittle for state | ||
| management. |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In React 19 official docs, what are valid use cases for useRef beyond DOM/native element refs?
💡 Result:
According to the official React 19 documentation, valid use cases for useRef beyond DOM/native element refs include storing mutable values that persist across re-renders without triggering re-renders, such as interval IDs, timeout IDs, or other implementation details not used for rendering.[1][2] The docs explicitly state: "refs are perfect for storing information that doesn't affect the visual output of your component. For example, if you need to store an interval ID and retrieve it later, put it in a ref."[1] They emphasize: "Changing a ref does not trigger a re-render, so refs are not appropriate for storing information you want to display on the screen. Use state for that instead."[1][2] Older docs (still relevant as core behavior unchanged in React 19) note useRef is "handy for keeping any mutable value around similar to how you’d use instance fields in classes."[3] No other specific non-DOM examples are listed in the official docs, but the general purpose is clear: mutable, non-rendering-persisting values like timers or handles to external resources.
Citations:
- 1: https://react.dev/reference/react/useRef
- 2: https://beta.reactjs.org/reference/react/useRef
- 3: https://reactjs.org/docs/hooks-reference.html
useRef rule is overly restrictive and contradicts React's documented API.
Lines 12–15 claim the only valid use of useRef is capturing native component refs. React's official documentation explicitly permits useRef for storing mutable values that persist across renders without triggering re-renders—including interval IDs, timeout IDs, and handles to external resources. The current guidance forces developers into incorrect patterns and will cause unnecessary state/effect churn when mutable references are the appropriate choice.
Update the guidance to reflect the full documented use cases: DOM refs and stable mutable values for non-visual data.
🧰 Tools
🪛 LanguageTool
[style] ~13-~13: Consider a more concise word here.
Context: ...a native component using ref={myRef}. In order to track state between calls, a useState, ...
(IN_ORDER_TO_PREMIUM)
🤖 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 @.claude/skills/react/SKILL.md around lines 12 - 15, The guidance is too
strict: update the SKILL.md text that currently states "The ONLY acceptable use
for a useRef..." to say that useRef is appropriate both for capturing
DOM/component refs (ref={myRef}) and for storing stable mutable values that
persist across renders without causing re-renders (e.g., interval IDs, timeout
IDs, external resource handles); mention that useState/useMemo/useEffect are
still preferred when changes should trigger rerenders, but useRef is valid for
non-visual mutable data and provide those example use-cases (interval IDs,
timeout IDs, mutable caches) to align with React docs.
| ANY new array or object definition that is used in JSX MUST be wrapped in a | ||
| useMemo with ALL dependencies in the dependency array. |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What does the React team recommend about when to use useMemo and useCallback, and do they recommend wrapping all objects/functions by default?
💡 Result:
The React team recommends using useMemo and useCallback only in specific optimization scenarios, not by default for all objects and functions[1][2]. For useCallback: It caches functions passed as props to memoized child components (wrapped in React.memo) to prevent unnecessary re-renders when dependencies haven't changed. There is no benefit otherwise, though no significant harm; however, it reduces code readability and a single non-memoized value can break memoization[1]. For useMemo: Use for expensive calculations where dependencies rarely change, or when passing memoized values as props to memoized children to skip re-renders. Do not rely on it semantically—code must work without it. Avoid for other cases to maintain readability[2]. They explicitly advise against wrapping everything by default: "There is no benefit to wrapping a function in useCallback in other cases... The downside is that code becomes less readable"[1]. Similarly for useMemo[2]. Profile first, then optimize; some teams over-memoize, but official docs prioritize targeted use[1][2]. With React Compiler (React 19+), manual memoization is often unnecessary as it auto-optimizes[3][4].
Citations:
- 1: https://react.dev/reference/react/useCallback
- 2: https://react.dev/reference/react/useMemo
- 3: https://react.dev/reference/eslint-plugin-react-hooks/lints/preserve-manual-memoization
- 4: https://dev.to/norah_aiden_427d44c007ec5/react-compiler-vs-usememo-real-benchmarks-jg5
Revise blanket memoization requirements to align with React best practices.
Lines 42-43 and 51-52 mandate memoization for all objects/arrays/functions used in JSX, which contradicts official React guidance. The React team explicitly recommends using useMemo and useCallback only when there is demonstrated performance benefit—such as memoizing expensive calculations or preventing unnecessary re-renders in memoized child components. Wrapping everything by default increases maintenance burden and reduces code readability without benefit. Remove or narrow these requirements to performance-critical scenarios 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 @.claude/skills/react/SKILL.md around lines 42 - 43, Update the policy text
that currently mandates wrapping "ANY new array or object definition ... used in
JSX" in useMemo so it reflects React best practices: remove the blanket
requirement and instead instruct authors to use useMemo and useCallback only
when there is a demonstrated performance benefit (e.g., expensive computations,
stabilizing props for memoized children, or avoiding repeated heavy
allocations), and to always include correct dependency arrays when used;
reference the terms useMemo, useCallback and JSX in the revised wording so
reviewers know which constructs this guidance applies to.
| const handleCreateRune = () => { | ||
| navigate("/runes/create"); | ||
| }; |
There was a problem hiding this comment.
Point Create Rune at the route added in this PR.
This handler navigates to /runes/create, but the new page in this branch lives under bifrost/ui/src/pages/runes/new/+Page.tsx. As written, the CTA will go to a missing route.
Suggested fix
const handleCreateRune = () => {
- navigate("/runes/create");
+ navigate("/runes/new");
};📝 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.
| const handleCreateRune = () => { | |
| navigate("/runes/create"); | |
| }; | |
| const handleCreateRune = () => { | |
| navigate("/runes/new"); | |
| }; |
🤖 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 `@bifrost/ui/src/pages/runes/`+Page.tsx around lines 179 - 181, The
handleCreateRune handler currently calls navigate("/runes/create") which points
to a non-existent route; update it to navigate("/runes/new") so the CTA routes
to the new page implemented in new/+Page.tsx (adjust the string in
handleCreateRune and any other places referencing "/runes/create" to
"/runes/new" to keep routes consistent).
| onValueChange={(value) => { | ||
| if (typeof value === "string") { | ||
| onRelationshipTargetIdChange(value); | ||
| } | ||
| }} |
There was a problem hiding this comment.
Handle combobox clear events to prevent stale relationship target IDs.
When the combobox emits a non-string value (e.g., clear/reset), this handler ignores it, so relationshipTargetId can stay stale and “Add Relationship” can submit an unintended prior target.
Suggested fix
onValueChange={(value) => {
- if (typeof value === "string") {
- onRelationshipTargetIdChange(value);
- }
+ onRelationshipTargetIdChange(typeof value === "string" ? value : "");
}}📝 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.
| onValueChange={(value) => { | |
| if (typeof value === "string") { | |
| onRelationshipTargetIdChange(value); | |
| } | |
| }} | |
| onValueChange={(value) => { | |
| onRelationshipTargetIdChange(typeof value === "string" ? value : ""); | |
| }} |
🤖 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 `@bifrost/ui/src/pages/runes/new/Relationships.tsx` around lines 94 - 98, The
combobox onValueChange handler currently ignores non-string values which leaves
relationshipTargetId stale; update the onValueChange callback used in
Relationships.tsx to call onRelationshipTargetIdChange with a cleared value
(e.g., empty string or null) when value is not a string so clearing the combobox
resets relationshipTargetId and prevents "Add Relationship" from submitting a
prior target; locate the onValueChange lambda and ensure it handles both string
and non-string cases by invoking onRelationshipTargetIdChange appropriately.
| "use client"; | ||
|
|
||
| import { Input } from "@base-ui/react/input"; | ||
| import type { FormData } from "../+Page"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate FormData declarations and re-exports under bifrost/ui/src/pages/runes
fd -e tsx -e ts . bifrost/ui/src/pages/runes | xargs rg -nP '\b(type|interface)\s+FormData\b|export\s+\{[^}]*\bFormData\b'Repository: devzeebo/bifrost
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check if the files mentioned in the review exist
echo "=== Checking file existence ==="
ls -la bifrost/ui/src/pages/runes/new/RuneForm.tsx 2>&1
ls -la bifrost/ui/src/pages/runes/+Page.tsx 2>&1
ls -la bifrost/ui/src/pages/runes/new/+Page.tsx 2>&1
echo ""
echo "=== Directory structure ==="
find bifrost/ui/src/pages/runes -type f -name "*.tsx" -o -name "*.ts" | head -20Repository: devzeebo/bifrost
Length of output: 793
🏁 Script executed:
#!/bin/bash
# Read the actual content of RuneForm.tsx to see the import statement
echo "=== RuneForm.tsx line 1-10 ==="
head -10 bifrost/ui/src/pages/runes/new/RuneForm.tsx 2>&1 || echo "File not found"
echo ""
echo "=== Check runes/+Page.tsx existence and content ==="
if [ -f bifrost/ui/src/pages/runes/+Page.tsx ]; then
echo "File exists. First 30 lines:"
head -30 bifrost/ui/src/pages/runes/+Page.tsx
else
echo "File does not exist at bifrost/ui/src/pages/runes/+Page.tsx"
fi
echo ""
echo "=== Check runes/new/+Page.tsx existence and content ==="
if [ -f bifrost/ui/src/pages/runes/new/+Page.tsx ]; then
echo "File exists. First 30 lines:"
head -30 bifrost/ui/src/pages/runes/new/+Page.tsx
else
echo "File does not exist at bifrost/ui/src/pages/runes/new/+Page.tsx"
fiRepository: devzeebo/bifrost
Length of output: 2629
🏁 Script executed:
#!/bin/bash
# Search for FormData definition more broadly
echo "=== Search for FormData in entire runes directory ==="
rg "FormData" bifrost/ui/src/pages/runes/ -A 2 -B 2 2>&1 || echo "No matches found"Repository: devzeebo/bifrost
Length of output: 3439
🏁 Script executed:
#!/bin/bash
# Search for FormData type definition (not just imports)
echo "=== Search for FormData type definition ==="
rg "type FormData|interface FormData" bifrost/ui/src/pages/runes/ -B 1 -A 5
echo ""
echo "=== Check what runes/+Page.tsx exports ==="
grep -n "export" bifrost/ui/src/pages/runes/+Page.tsx | head -20
echo ""
echo "=== Check what runes/new/+Page.tsx exports ==="
grep -n "export" bifrost/ui/src/pages/runes/new/+Page.tsx | head -20Repository: devzeebo/bifrost
Length of output: 238
🏁 Script executed:
#!/bin/bash
# Read the full type definition section of runes/new/+Page.tsx
echo "=== runes/new/+Page.tsx lines around FormData definition ==="
sed -n '1,100p' bifrost/ui/src/pages/runes/new/+Page.tsx | tail -80Repository: devzeebo/bifrost
Length of output: 2847
🏁 Script executed:
#!/bin/bash
# Read the entire runes/new/+Page.tsx to find FormData definition and export
wc -l bifrost/ui/src/pages/runes/new/+Page.tsx
echo ""
echo "=== Full file or search for type/export pattern ==="
cat -n bifrost/ui/src/pages/runes/new/+Page.tsx | grep -E "(type FormData|export.*FormData|export \{|^\s*[0-9]+\s+type|^\s*[0-9]+\s+interface)"Repository: devzeebo/bifrost
Length of output: 378
🏁 Script executed:
#!/bin/bash
# Get lines 1-20 to see imports and early declarations
echo "=== runes/new/+Page.tsx lines 1-20 ==="
sed -n '1,20p' bifrost/ui/src/pages/runes/new/+Page.tsx
echo ""
echo "=== Search entire file for 'type FormData' ==="
grep -n "type FormData" bifrost/ui/src/pages/runes/new/+Page.tsx
echo ""
echo "=== Check last 30 lines for exports ==="
tail -30 bifrost/ui/src/pages/runes/new/+Page.tsxRepository: devzeebo/bifrost
Length of output: 1970
🏁 Script executed:
#!/bin/bash
# Check runes/+Page.tsx for FormData definition
echo "=== runes/+Page.tsx line count ==="
wc -l bifrost/ui/src/pages/runes/+Page.tsx
echo ""
echo "=== Search for 'type FormData' in runes/+Page.tsx ==="
grep -n "type FormData\|interface FormData" bifrost/ui/src/pages/runes/+Page.tsx
echo ""
echo "=== Search for 'FormData' in runes/+Page.tsx ==="
rg "FormData" bifrost/ui/src/pages/runes/+Page.tsx -nRepository: devzeebo/bifrost
Length of output: 241
🏁 Script executed:
#!/bin/bash
# Let's check if there's a types.ts file in the runes directories
echo "=== Look for types files in runes ==="
find bifrost/ui/src/pages/runes -name "types.ts" -o -name "types.tsx"
echo ""
echo "=== Look at the actual import statement in runes/new/+Page.tsx line 16 more carefully ==="
sed -n '16p' bifrost/ui/src/pages/runes/new/+Page.tsxRepository: devzeebo/bifrost
Length of output: 275
🏁 Script executed:
#!/bin/bash
# Check the entire runes/new/+Page.tsx to find FormData definition
echo "=== Full content of runes/new/+Page.tsx ==="
cat -n bifrost/ui/src/pages/runes/new/+Page.tsx | grep -E "(type FormData|interface FormData|export.*FormData)" -B 2 -A 5
echo ""
echo "=== Check if FormData is defined anywhere after line 16 ==="
awk 'NR>=16 && NR<=60' bifrost/ui/src/pages/runes/new/+Page.tsx | cat -nRepository: devzeebo/bifrost
Length of output: 2258
🏁 Script executed:
#!/bin/bash
# Let's look for type definitions right around where FormData would be used
echo "=== Lines 16-50 of runes/new/+Page.tsx ==="
sed -n '16,50p' bifrost/ui/src/pages/runes/new/+Page.tsx | cat -nRepository: devzeebo/bifrost
Length of output: 1737
🏁 Script executed:
#!/bin/bash
# Search the entire runes/new/+Page.tsx for any type definition
echo "=== Full runes/new/+Page.tsx to find type definitions ==="
cat bifrost/ui/src/pages/runes/new/+Page.tsx | grep -n "^type\|^interface\|^export type\|^export interface" | head -20
echo ""
echo "=== Search for type keyword anywhere ==="
cat bifrost/ui/src/pages/runes/new/+Page.tsx | grep -n "^[[:space:]]*type\|^[[:space:]]*export[[:space:]]*type" | head -20
echo ""
echo "=== Check all lines that start with 'type' ==="
grep -n "^type " bifrost/ui/src/pages/runes/new/+Page.tsxRepository: devzeebo/bifrost
Length of output: 392
🏁 Script executed:
#!/bin/bash
# The import at line 16 says "./+Page" - that's self-referential. Let's verify what TypeScript sees
# Read the entire file to see if FormData might be defined inline somewhere
echo "=== Lines 1-100 of runes/new/+Page.tsx ==="
sed -n '1,100p' bifrost/ui/src/pages/runes/new/+Page.tsx | cat -nRepository: devzeebo/bifrost
Length of output: 4369
🏁 Script executed:
#!/bin/bash
# Read the full file to find where types are exported
echo "=== Lines 280-306 of runes/new/+Page.tsx (end of file) ==="
sed -n '280,306p' bifrost/ui/src/pages/runes/new/+Page.tsx | cat -n
echo ""
echo "=== Search for 'export' statements ==="
grep -n "^export" bifrost/ui/src/pages/runes/new/+Page.tsx
echo ""
echo "=== Check if there's a second +Page file ==="
find bifrost/ui/src/pages/runes/new -name "+Page*"Repository: devzeebo/bifrost
Length of output: 1253
🏁 Script executed:
#!/bin/bash
# Let's search the entire file for where FormData/RelationshipDirection/SelectedRelationship are exported
echo "=== Entire runes/new/+Page.tsx to search for exports ==="
cat bifrost/ui/src/pages/runes/new/+Page.tsx | tail -100 | head -50
echo ""
echo "=== More of the file ==="
sed -n '250,306p' bifrost/ui/src/pages/runes/new/+Page.tsxRepository: devzeebo/bifrost
Length of output: 3720
🏁 Script executed:
#!/bin/bash
# This is the smoking gun - let's verify this is a circular/broken import
# The file imports from "./+Page" (itself) at line 16, but has no type definitions
echo "=== Verify: runes/new/+Page.tsx imports from itself ==="
sed -n '16p' bifrost/ui/src/pages/runes/new/+Page.tsx
echo ""
echo "=== Count total lines in runes/new/+Page.tsx ==="
wc -l bifrost/ui/src/pages/runes/new/+Page.tsx
echo ""
echo "=== Search for 'FormData' in all files under runes ==="
rg "export.*FormData|type FormData.*=" bifrost/ui/src/pages/runes/ -n
echo ""
echo "=== Double-check: does runes/+Page.tsx (parent list page) export FormData? ==="
grep "FormData\|export" bifrost/ui/src/pages/runes/+Page.tsx | head -20Repository: devzeebo/bifrost
Length of output: 450
🏁 Script executed:
#!/bin/bash
# Let's see if maybe the types are supposed to be in a separate file
echo "=== Search for type definitions anywhere related to runes form ==="
rg "type FormData\|interface FormData" bifrost/ui/src/ -n --max-count=10
echo ""
echo "=== Check other component files in runes/new ==="
ls -la bifrost/ui/src/pages/runes/new/Repository: devzeebo/bifrost
Length of output: 622
🏁 Script executed:
#!/bin/bash
# Let's check if maybe the types ARE supposed to be in +Page but are just not exported
echo "=== Get entire runes/new/+Page.tsx to look for type definitions ==="
cat bifrost/ui/src/pages/runes/new/+Page.tsx | grep -n "^type\|^interface\|^export" | tail -20
echo ""
echo "=== Let's check if these types are defined using ast-grep ==="
ast-grep --pattern 'type FormData = $_'
echo ""
echo "=== Check if maybe they're defined with 'export type' ==="
rg "export type FormData" bifrost/ui/src/Repository: devzeebo/bifrost
Length of output: 855
🏁 Script executed:
#!/bin/bash
# Verify the actual state by checking what happens with the self-import
# The file runes/new/+Page.tsx line 16 imports from "./+Page" (itself)
# but the file doesn't export those types
# This is the critical issue
echo "=== CRITICAL: runes/new/+Page.tsx structure ==="
echo "1. Line 16 imports FROM itself: './+Page'"
sed -n '16p' bifrost/ui/src/pages/runes/new/+Page.tsx
echo ""
echo "2. But the file ONLY exports:"
grep "^export" bifrost/ui/src/pages/runes/new/+Page.tsx
echo ""
echo "3. So it tries to import types that don't exist in itself"
echo ""
echo "4. RuneForm.tsx imports from '../+Page' (parent list page):"
sed -n '4p' bifrost/ui/src/pages/runes/new/RuneForm.tsx
echo ""
echo "5. But parent list page also ONLY exports:"
grep "^export" bifrost/ui/src/pages/runes/+Page.tsxRepository: devzeebo/bifrost
Length of output: 507
🏁 Script executed:
#!/bin/bash
# Final verification: Check if types ARE defined anywhere in runes/new/+Page.tsx
# using more comprehensive patterns
echo "=== Search for any type/interface definitions in runes/new/+Page.tsx ==="
cat bifrost/ui/src/pages/runes/new/+Page.tsx | awk '/^(type|interface|export type|export interface)/ {found=1} found {print NR": "$0; if (/;$/ || /\}$/) found=0}'
echo ""
echo "=== Compare with realms/new/+Page.tsx which has FormData ==="
head -30 bifrost/ui/src/pages/realms/new/+Page.tsx | grep -n "type FormData" -A 5
echo ""
echo "=== Compare with accounts/new/+Page.tsx which has FormData ==="
head -30 bifrost/ui/src/pages/accounts/new/+Page.tsx | grep -n "type FormData" -A 5Repository: devzeebo/bifrost
Length of output: 499
Both files have critical import issues with missing type definitions.
runes/new/+Page.tsx (line 16) attempts to import FormData, RelationshipDirection, and SelectedRelationship from "./+Page" (itself), but these types are never defined in that file. Comparing to similar pages (realms/new/+Page.tsx, accounts/new/+Page.tsx), the types should be defined locally in the page component.
Additionally, RuneForm.tsx imports FormData from "../+Page" (the parent runes list page), which also does not export this type.
Fix: Define the missing types in runes/new/+Page.tsx alongside the Page component and export them, then update RuneForm.tsx to import from "./+Page" instead of "../+Page".
🤖 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 `@bifrost/ui/src/pages/runes/new/RuneForm.tsx` at line 4, Define and export the
missing types (FormData, RelationshipDirection, SelectedRelationship) inside the
runes/new/+Page.tsx next to the Page component (mirroring how
realms/new/+Page.tsx and accounts/new/+Page.tsx do it) so the page file is the
authoritative source of these types; then update the import in RuneForm.tsx to
import FormData from "./+Page" (relative to the new rune page) instead of
"../+Page" and ensure the exported type names match the ones used in
RuneForm.tsx and the Page component.
1 participant
No description provided.