fix(runner): wrap training entry import() with error context - #223
fix(runner): wrap training entry import() with error context#223KinjalGoswami68 wants to merge 1 commit into
Conversation
Fixes arkorlab#221. If the training entry file exists but fails during import() (a broken dependency, a syntax error in an imported module, etc.), the original error bubbled up with no indication that it happened while loading the training entry, before training ever started. Wrap only the import() call (not extractTrainer, which already has its own clear error message) in try/catch, and re-throw with context using the standard Error cause option so the original error is preserved, not hidden: Failed to load training entry: <path> Cause: <original error> Added a test that writes a real entry file importing a genuinely nonexistent module, letting Node's real ESM loader throw, and asserts both the new wrapper message and that the original error is still reachable via err.cause.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
🧰 Additional context used📓 Path-based instructions (9)packages/arkor/src/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
packages/*/src/**/*.test.ts📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{js,ts,jsx,tsx,json,css,html}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{js,ts,jsx,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{js,jsx,ts,tsx}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
packages/**/*.{ts,tsx}📄 CodeRabbit inference engine (CONTRIBUTING.ja.md)
Files:
**/*📄 CodeRabbit inference engine (CONTRIBUTING.ja.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (CONTRIBUTING.ja.md)
Files:
packages/arkor/src/core/runner.ts📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (6)📓 Common learnings📚 Learning: 2026-07-24T09:55:04.181ZApplied to files:
📚 Learning: 2026-08-07T08:54:17.077ZApplied to files:
📚 Learning: 2026-07-24T09:55:04.181ZApplied to files:
📚 Learning: 2026-08-07T08:54:17.077ZApplied to files:
📚 Learning: 2026-07-24T09:55:04.181ZApplied to files:
🔇 Additional comments (2)
Walkthrough
ChangesTraining entry error handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by Qodofix(runner): add contextual error (with cause) for training entry import failures
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Greptile SummaryThe PR wraps dynamic training-entry import failures with path context while preserving the loader error as Error.cause.
Confidence Score: 4/5The import wrapper should be fixed before merging because it removes the actionable loader diagnostic from the CLI and Studio output users actually see. The original import error remains programmatically reachable, but current entry points render only the wrapper stack and never traverse its cause; the accompanying test does not exercise that user-facing boundary. Files Needing Attention: packages/arkor/src/core/runner.ts and packages/arkor/src/core/runner.test.ts
|
| Filename | Overview |
|---|---|
| packages/arkor/src/core/runner.ts | Adds contextual import-error wrapping, but the original diagnostic is hidden by current user-facing error rendering. |
| packages/arkor/src/core/runner.test.ts | Covers the wrapper and cause properties but not the actual CLI or Studio stderr output. |
Prompt To Fix All With AI
### Issue 1
packages/arkor/src/core/runner.ts:62
**Cause omitted from rendered error**
When a training entry fails during import, the new wrapper stores the loader diagnostic only in `cause`, but the CLI prints only `err.stack` or `err.message` and Studio relays that output. Users therefore see the entry path and wrapper stack without the missing-module or syntax diagnostic needed to fix the entry.
### Issue 2
packages/arkor/src/core/runner.ts:62
**Behavior change lacks documentation**
This changes the CLI and Studio diagnostic produced for training-entry import failures without adding the repository-required English and Japanese documentation, leaving users in both documentation surfaces uninformed about the new error shape and preserved cause.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(runner): wrap training entry import(..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| mod = (await import(pathToFileURL(abs).href)) as Record<string, unknown>; | ||
| } catch (err) { | ||
| throw new Error(`Failed to load training entry: ${abs}`, { cause: err }); | ||
| } |
There was a problem hiding this comment.
When a training entry fails during import, the new wrapper stores the loader diagnostic only in cause, but the CLI prints only err.stack or err.message and Studio relays that output. Users therefore see the entry path and wrapper stack without the missing-module or syntax diagnostic needed to fix the entry.
Knowledge Base Used: Arkor core runtime (packages/arkor/src/core)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/arkor/src/core/runner.ts
Line: 62
Comment:
**Cause omitted from rendered error**
When a training entry fails during import, the new wrapper stores the loader diagnostic only in `cause`, but the CLI prints only `err.stack` or `err.message` and Studio relays that output. Users therefore see the entry path and wrapper stack without the missing-module or syntax diagnostic needed to fix the entry.
**Knowledge Base Used:** [Arkor core runtime (`packages/arkor/src/core`)](https://app.greptile.com/arkor/-/custom-context/knowledge-base/arkorlab/arkor/-/docs/arkor-core-runtime.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| mod = (await import(pathToFileURL(abs).href)) as Record<string, unknown>; | ||
| } catch (err) { | ||
| throw new Error(`Failed to load training entry: ${abs}`, { cause: err }); | ||
| } |
There was a problem hiding this comment.
This changes the CLI and Studio diagnostic produced for training-entry import failures without adding the repository-required English and Japanese documentation, leaving users in both documentation surfaces uninformed about the new error shape and preserved cause.
Context Used: CLAUDE.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/arkor/src/core/runner.ts
Line: 62
Comment:
**Behavior change lacks documentation**
This changes the CLI and Studio diagnostic produced for training-entry import failures without adding the repository-required English and Japanese documentation, leaving users in both documentation surfaces uninformed about the new error shape and preserved cause.
**Context Used:** CLAUDE.md ([source](https://github.com/arkorlab/arkor/blob/main/CLAUDE.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5faa0f669c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| try { | ||
| mod = (await import(pathToFileURL(abs).href)) as Record<string, unknown>; | ||
| } catch (err) { | ||
| throw new Error(`Failed to load training entry: ${abs}`, { cause: err }); |
There was a problem hiding this comment.
When the entry exists but one of its own imports fails, this wrapper is what reaches the CLI, but packages/arkor/src/bin.ts prints only err.stack ?? err.message; Node's stack string does not include Error.cause. In that scenario arkor start now shows only Failed to load training entry: ... and hides the actionable original message such as Cannot find module or a syntax error, which is a regression from the bare import error. Please either include the original error text in this message or update the top-level formatter to print causes.
Useful? React with 👍 / 👎.
Code Review by Qodo
1. CLI hides import cause
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| } catch (err) { | ||
| throw new Error(`Failed to load training entry: ${abs}`, { cause: err }); | ||
| } |
There was a problem hiding this comment.
1. Cli hides import cause 🐞 Bug ◔ Observability
runTrainer() now throws a new wrapper Error on import() failure with the original error stored only in cause, but packages/arkor/src/bin.ts prints only err.stack ?? err.message (a string) and never formats err.cause. As a result, users will typically see only `Failed to load training entry: ...` and lose the underlying import failure details (e.g. missing module), which is a regression from the previous behavior where the original import error text was printed.
Agent Prompt
### Issue description
`runTrainer()` now wraps import-time failures with a new `Error(..., { cause })`. However, the CLI’s top-level error handler prints only `err.stack ?? err.message` (a string) and does not include `err.cause`, so the original import failure message/stack is not shown in normal CLI output.
### Issue Context
- The PR introduces a wrapper error around `import(pathToFileURL(abs).href)`.
- The CLI intentionally avoids `console.error(err)` because the bundled stack is noisy/minified, but printing only the wrapper’s stack/message loses the actionable underlying cause.
### Fix Focus Areas
- packages/arkor/src/core/runner.ts[57-62]
- packages/arkor/src/bin.ts[14-43]
### What to change
- Update `packages/arkor/src/bin.ts` catch-all printing branch to include `Error.cause` when present.
- Keep the wrapper context (`err.message`).
- If `err.cause` is an `Error`, print something like `Cause: ${cause.stack ?? cause.message}` (or recursively print nested causes).
- If `err.cause` is non-Error, print `Cause: ${String(err.cause)}`.
- (Optional alternative) If you prefer not to change CLI printing behavior, include a `Cause: ...` line in the wrapper error’s `.message` in `runTrainer()` while still setting `{ cause }`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Code Review BotNo comment/code divergences or documentation drift detected. Reviewed 2 file(s); skipped 0. |
1 participant
Fixes #221.
If the training entry file exists but fails during import() (a broken dependency, a syntax error in an imported module, etc.), the original error bubbled up with no indication that it happened while loading the training entry, before training ever started.
Wrap only the import() call (not extractTrainer, which already has its own clear error message) in try/catch, and re-throw with context using the standard Error cause option so the original error is preserved, not hidden:
Failed to load training entry:
Cause:
Added a test that writes a real entry file importing a genuinely nonexistent module, letting Node's real ESM loader throw, and asserts both the new wrapper message and that the original error is still reachable via err.cause.
Testing
pnpm format:check,pnpm --filter arkor typecheck,pnpm --filter arkor lint, andpnpm --filter arkor exec vitest run src/core/runner.test.tsall pass (12/12 tests).Summary by cubic
Clarifies errors when loading the training entry by wrapping the
import()with context and preserving the original error viaErrorcause. Fixes #221.import()in try/catch and rethrow asFailed to load training entry: <path>withcause.err.cause.Written for commit 5faa0f6. Summary will update on new commits.
Summary by CodeRabbit