[codex] Keep harness runtime importable at the edge#124
Conversation
The local dev entrypoint failed before rendering the TUI because Node watch attempted to observe too much of the workspace and hit macOS file-watch limits. A second source-condition run exposed a Node 24 ESM ambiguity in the harness dotenv loader, so the loader now uses static Node imports instead of require inside an ESM graph. Constraint: minimal-agent dev must keep using source-condition TypeScript entrypoints for workspace packages Constraint: Node 24 rejects require inside this top-level-await ESM execution graph Rejected: Watch sibling workspace source directories from minimal-agent dev | Node watch still hits EMFILE in this environment Confidence: high Scope-risk: narrow Directive: Do not broaden minimal-agent dev watch paths without validating macOS file-watch behavior Tested: pnpm -F "@plugsuits/minimal-agent" dev Tested: pnpm -F "@ai-sdk-tool/harness" typecheck Tested: pnpm -F "@ai-sdk-tool/harness" test Tested: pnpm -F "@plugsuits/minimal-agent" typecheck Tested: pnpm -F "@plugsuits/minimal-agent" test Tested: pnpm run check Not-tested: Watching harness/tui/headless sibling source trees from the minimal-agent dev script
|
Important Review skippedAuto reviews are disabled on this repository. To trigger a review, include ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses dotenv loading issues on Node 24 by refactoring imports in the harness package and updating the development script in the minimal-agent package. Review feedback highlights that converting dynamic requires to top-level static imports for Node-specific modules breaks compatibility with edge runtimes. Additionally, the use of hardcoded file paths in the watch script is noted as a maintenance risk that could lead to missed updates when new files are added.
| import { existsSync } from "node:fs"; | ||
| import { dirname, resolve } from "node:path"; |
There was a problem hiding this comment.
Moving node:fs and node:path to top-level static imports breaks the "Safe to omit in edge runtimes" guarantee mentioned on line 7. Edge environments (e.g., Cloudflare Workers, Vercel Edge) typically do not support node:fs and will throw a module resolution error at import time, even if loadDotEnvFilesIfAvailable is never called. If cross-platform compatibility is still a goal, consider using dynamic import() or update the documentation to reflect that this module is now Node-only.
| "benchmark": "node --conditions=@ai-sdk-tool/source --import tsx benchmark.ts", | ||
| "build": "tsc && node ../../scripts/fix-esm-imports.mjs dist", | ||
| "dev": "node --watch --conditions=@ai-sdk-tool/source --import tsx index.ts", | ||
| "dev": "node --watch --watch-path=index.ts --watch-path=env.ts --watch-path=preferences.ts --conditions=@ai-sdk-tool/source --import tsx index.ts", |
There was a problem hiding this comment.
Hardcoding specific filenames in --watch-path is fragile and creates a maintenance burden. Any new files added to the package (e.g., a new tools.ts or utils.ts) will not be watched by default, which can lead to confusion when changes don't trigger a reload. While this approach avoids EMFILE errors, consider if watching the current directory with an ignore pattern for the session directory would be more sustainable.
The first fix made minimal-agent runnable locally but moved Node filesystem imports into the harness runtime graph. That regressed the existing edge-runtime intent, so this separates Node-only dotenv loading behind an explicit env-node subpath and keeps runtime core imports free of Node built-ins. Constraint: Cloudflare Workers can run Node APIs only under nodejs_compat, and node:fs availability depends on newer compatibility dates or explicit flags Constraint: minimal-agent and tgbot still need package/workspace .env loading in Node entrypoints Rejected: Keep dotenv loading inside createAgentRuntime | edge runtimes should receive bindings/env from the host instead of probing local .env files Rejected: Statically import SkillsEngine and MCP plumbing from runtime core | those paths pull node:fs/node:path into consumers that do not use skills or MCP Confidence: high Scope-risk: moderate Directive: Do not add Node built-in imports to the @ai-sdk-tool/harness/runtime static graph; extend runtime-edge-imports.test.ts when the boundary changes Tested: pnpm -F "@ai-sdk-tool/harness" typecheck Tested: pnpm -F "@ai-sdk-tool/harness" test Tested: pnpm -F "@ai-sdk-tool/harness" build Tested: pnpm -F "@plugsuits/minimal-agent" typecheck Tested: pnpm -F "@plugsuits/minimal-agent" test Tested: pnpm -F "@plugsuits/minimal-agent" dev Tested: pnpm -F "@plugsuits/tgbot" typecheck Tested: pnpm -F "@plugsuits/tgbot" test Tested: pnpm run check Tested: runtime import/openSession smoke via @plugsuits/minimal-agent workspace context Not-tested: Wrangler deploy/build against a real Cloudflare Worker project
minpeter
changed the title
Oracle review found that the harness runtime source graph was edge-clean but the external AI SDK path can reach @vercel/oidc's Node build unless the browser condition is active. Add a process-less import/openSession smoke under browser conditions so the PR covers that dependency-resolution requirement too. Constraint: Do not add Wrangler as a dependency just to validate this package-level boundary Rejected: Rely only on source static graph scanning | it misses external package conditional exports Confidence: medium Scope-risk: narrow Directive: If Cloudflare support becomes a first-class promise, add a real Wrangler/Workers bundle test in addition to this browser-condition smoke Tested: pnpm -F "@ai-sdk-tool/harness" test -- src/runtime/runtime-edge-imports.test.ts Tested: pnpm -F "@ai-sdk-tool/harness" typecheck Tested: pnpm -F "@ai-sdk-tool/harness" test Tested: pnpm run check Not-tested: Wrangler deploy/build against a real Cloudflare Worker project
FileSnapshotStore now accepts a top-level state directory, resolves it at construction time, owns the <root>/sessions layout, and best-effort appends that root to the containing worktree's .gitignore. Consumers no longer precompute sessions directories or maintain ignore entries independently. This keeps session storage stable even if process.cwd() changes after store construction, and it aligns the shipped release notes with the public rootDir/sessionsDir getter contract. Constraint: Preserve #124's edge-safe runtime import changes while rebasing onto current main Rejected: Leave relative rootDir values as-is | contradicted the resolved-path contract and made later cwd changes affect save/load paths Confidence: high Scope-risk: moderate Directive: Do not reintroduce caller-managed /sessions paths without updating downstream env names and migration notes Tested: pnpm run typecheck; pnpm run test; pnpm run build; pnpm run check Not-tested: Live model-backed minimal-agent session
FileSnapshotStore now accepts a top-level state directory, resolves it at construction time, owns the <root>/sessions layout, and best-effort appends that root to the containing worktree's .gitignore. Consumers no longer precompute sessions directories or maintain ignore entries independently. This keeps session storage stable even if process.cwd() changes after store construction, and it aligns the shipped release notes with the public rootDir/sessionsDir getter contract. Constraint: Preserve #124's edge-safe runtime import changes while rebasing onto current main Rejected: Leave relative rootDir values as-is | contradicted the resolved-path contract and made later cwd changes affect save/load paths Confidence: high Scope-risk: moderate Directive: Do not reintroduce caller-managed /sessions paths without updating downstream env names and migration notes Tested: pnpm run typecheck; pnpm run test; pnpm run build; pnpm run check Not-tested: Live model-backed minimal-agent session
1 participant
Summary
@ai-sdk-tool/harness/runtimefree of unconditional Node-only imports so edge consumers do not pullnode:fs,node:path, ornode:cryptothrough the runtime subpath.@ai-sdk-tool/harness/env-nodesubpath.EMFILEfailure.Root Cause
The first local fix made
minimal-agentrunnable but moved Node filesystem imports intoharness/env.ts. That broke the existing edge-runtime intent becausecreateAgentRuntime()imports runtime internals that should be usable from Cloudflare Workers without probing local.envfiles or statically importing filesystem-backed skills/MCP code.An Oracle review also found that the harness source graph alone is not enough: the external AI SDK path can resolve to a Node-oriented
@vercel/oidcbuild unless browser conditions are active. The new smoke test covers that package-resolution requirement without adding Wrangler as a project dependency.Worker-Safe Minimum
The supported minimal edge path is intentionally narrow: import
@ai-sdk-tool/harness/runtime, pass Worker bindings into the provider directly, callcreateAgentRuntime({ cwd: "/" }), open a session, and run a turn without file persistence, skills, MCP, TUI/headless adapters, or@ai-sdk-tool/harness/env-node.FileSnapshotStore, file-backed preferences, local skills discovery, stdio MCP, and Node dotenv loading remain Node-oriented surfaces. A real Cloudflare Worker app still needs its bundler/deploy path to resolve browser conditions correctly; if Cloudflare support becomes a first-class guarantee, add a Wrangler/Workers bundle smoke on top of this package-level test.Validation
pnpm -F "@ai-sdk-tool/harness" test -- src/runtime/runtime-edge-imports.test.tspnpm -F "@ai-sdk-tool/harness" typecheckpnpm -F "@ai-sdk-tool/harness" testpnpm -F "@ai-sdk-tool/harness" buildpnpm -F "@plugsuits/minimal-agent" typecheckpnpm -F "@plugsuits/minimal-agent" testpnpm -F "@plugsuits/minimal-agent" devpnpm -F "@plugsuits/tgbot" typecheckpnpm -F "@plugsuits/tgbot" testpnpm run checkNotes
The new
runtime-edge-imports.test.tsguards the runtime static import graph against Node built-ins and verifies the external dependency path under browser conditions withprocessremoved.