Draft
Conversation
…ancements and commit strategy Review fixes: - Section 3: "one new package" → "four new packages" (agent-config, storage, logger, tools-builtins) - Section 4: complete image-local example (add builtin-tools, plugins, compaction, all storage) - Section 4: fix storage imports from @dexto/storage, StorageFactory takes logger not context - Section 5: Plugin[] → DextoPlugin[], Logger → IDextoLogger throughout - Section 5: resolver now passes ToolCreationContext/PluginCreationContext to factories - Section 6: add missing compaction in CLI after snippet - Section 8+9: ToolCreationContext/PluginCreationContext use full DextoAgent (per task 0.5) - Section 12: customTools → tools (post-unification) - Section 13: add @dexto/storage, @dexto/logger, @dexto/tools-builtins to affected packages New sections: - Section 17: Future enhancements — tool surface refactor, YAML validation, folder configurability, include shorthand, plugin event access - Section 18: Commit & PR strategy — single mega PR with per-task commits and phase-boundary checkpoints Co-authored-by: Cursor <cursoragent@cursor.com>
Section 5, 15, and tasklist (1.28, 2.5) incorrectly listed StorageSchema and sub-schemas as moving to @dexto/agent-config. They belong in @dexto/storage alongside the implementations. agent-config imports StorageConfigSchema from @dexto/storage to compose the top-level schema. Co-authored-by: Cursor <cursoragent@cursor.com>
Same pattern as storage: logger schema lives with the implementation in @dexto/logger. agent-config imports from @dexto/logger to compose the top-level AgentConfigSchema. Fixed in sections 5, 15, and tasklist. Co-authored-by: Cursor <cursoragent@cursor.com>
… add WORKING_MEMORY.md - Reorder Phase 3 so extraction packages (storage 3.2, logger 3.3) are created before image-local rewrite (3.5) which depends on them - Add explicit notes to Phase 1A/1B/1C/1D/1F explaining that implementations stay in core as plain exports and listing the exact Phase 3 task where they physically move to their new packages - Add WORKING_MEMORY.md as a colocated agent scratchpad for tracking current task, decisions, blockers, and progress during implementation - Reference WORKING_MEMORY.md from the top of the main plan - Sync both files to dexto-cloud Co-authored-by: Cursor <cursoragent@cursor.com>
- ToolManager now accepts pre-resolved local tools and injects ToolExecutionContext at execute-time - Move config->tools resolution into DextoAgent.start via resolveLocalToolsFromConfig (temporary glue) - Update tests and taskForker wiring Exit criteria met: pnpm run build && pnpm test pass
- PluginManager now accepts pre-resolved plugins and registers extension points only - Delete plugin registry + loader paths; built-ins resolved via resolveLocalPluginsFromConfig (temporary glue) - Remove pluginRegistry from bundler/templates + update tests Exit criteria met: pnpm run build && pnpm test pass
- DextoAgent constructor now takes a single DextoAgentOptions object (validated config, no parsing) - Update CLI/server, agent-management, bundler output, and templates to validate config before construction - Preserve host overrides via options.overrides Exit criteria met: pnpm run build && pnpm test pass
- Remove configDir/configPath handling from core service wiring - SystemPromptManager no longer accepts configDir - Update integration/unit tests accordingly Exit criteria met: pnpm run build && pnpm test pass
- Add temporary glue tagging convention (remove-by phase/task) - Update working memory with latest progress + next task
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/storage/README.md (1)
79-80:⚠️ Potential issue | 🟡 MinorStale "Node implementations" reference — should be updated to match the rest of the file.
Lines 17–18 and 65 already say "Bun runtime", but this browser-safety note still says "Node implementations", creating a doc inconsistency.
📝 Proposed fix
-Do not import from `@dexto/storage` in browser bundles, since the root entry also exports Node -implementations. +Do not import from `@dexto/storage` in browser bundles, since the root entry also exports Bun +runtime implementations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/storage/README.md` around lines 79 - 80, The browser-safety note "Do not import from `@dexto/storage` in browser bundles, since the root entry also exports Node implementations." is inconsistent with other references to the Bun runtime; update that sentence to mention "Bun runtime" (or match the exact phrasing used elsewhere like "Bun runtime") so it reads e.g. "Do not import from `@dexto/storage` in browser bundles, since the root entry also exports Bun runtime implementations," ensuring the wording aligns with lines that already reference "Bun runtime."CONTRIBUTING.md (2)
148-151:⚠️ Potential issue | 🟡 MinorTemplate README example still uses
npm installThe example agent README template (line 149) was not updated to Bun, leaving inconsistency with the rest of the migrated docs.
📝 Proposed fix
-1. Install dependencies: `npm install` +1. Install dependencies: `bun install`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` around lines 148 - 151, The Setup section still shows the legacy command "npm install"; update the template README example to use Bun by replacing "npm install" with "bun install" (leave the other lines like "export YOUR_API_KEY=your-key" and "dexto --agent your-agent-name.yml" unchanged) so the example matches the migrated docs.
296-300:⚠️ Potential issue | 🟡 MinorPrerequisites section missing Bun
After this migration, Bun is both the package manager and runtime, but the prerequisites still only mention Node.js. Contributors who follow these instructions and install only Node.js won't be able to run
bun install.📝 Proposed fix
### Prerequisites - Node.js >= 20.0.0 +- Bun >= 1.2 (see https://bun.sh/docs/installation) - Git🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` around lines 296 - 300, Update the "### Prerequisites" section to include Bun as a required runtime/package manager: locate the "### Prerequisites" header and the existing bullets "- Node.js >= 20.0.0" and "- Git" and add a new bullet like "- Bun >= 1.0.0 (required for bun install and as the runtime)"; keep or adjust the Node.js line if Node is still needed for any dev workflows, and ensure the wording clearly states Bun is required to run the project and install dependencies.
🧹 Nitpick comments (13)
packages/core/src/session/session-manager.ts (2)
258-298: Redundant second DB read in the same code path.
existingMetadata(line 265) is already the fetchedSessionData. The secondgetDatabase().get<SessionData>(sessionKey)at lines 280-282 repeats the same round-trip unnecessarily. ReuseexistingMetadatadirectly.♻️ Suggested fix
const existingMetadata = await this.services.storageManager .getDatabase() .get<SessionData>(sessionKey); if (existingMetadata) { // Session exists in storage, restore it await this.updateSessionActivity(id); const runtimeConfig = this.services.stateManager.getRuntimeConfig(); const agentId = runtimeConfig.agentCard?.name ?? runtimeConfig.agentId; const sessionLogger = this.sessionLoggerFactory({ baseLogger: this.logger, agentId, sessionId: id, }); // Restore LLM override BEFORE session init so the service is created with correct config // SECURITY: Re-resolve API key from environment when restoring (never persisted) - const sessionData = await this.services.storageManager - .getDatabase() - .get<SessionData>(sessionKey); - if (sessionData?.llmOverride) { + if (existingMetadata.llmOverride) { const { resolveApiKeyForProvider } = await import('../utils/api-key-resolver.js'); - const apiKey = resolveApiKeyForProvider(sessionData.llmOverride.provider); + const apiKey = resolveApiKeyForProvider(existingMetadata.llmOverride.provider); if (!apiKey) { this.logger.warn( - `Skipped LLM override restore for session ${id}: missing API key for provider ${sessionData.llmOverride.provider}`, - { sessionId: id, provider: sessionData.llmOverride.provider } + `Skipped LLM override restore for session ${id}: missing API key for provider ${existingMetadata.llmOverride.provider}`, + { sessionId: id, provider: existingMetadata.llmOverride.provider } ); } else { const restoredConfig: ValidatedLLMConfig = { - ...sessionData.llmOverride, + ...existingMetadata.llmOverride, apiKey, }; this.services.stateManager.updateLLM(restoredConfig, id); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/session/session-manager.ts` around lines 258 - 298, The code in createSessionInternal does a redundant second DB read for the same session key; reuse the already-fetched existingMetadata instead of calling getDatabase().get<SessionData>(sessionKey) again. Replace the second reference (currently assigned to sessionData) with existingMetadata (or rename existingMetadata to sessionData for clarity) when checking llmOverride and restoring the LLM config, and remove the duplicate storageManager.getDatabase().get call to avoid the extra round-trip.
231-238: Non-null assertions in production code violate core guidelines.The pattern
.has(id)→.get(id)!repeats at lines 232, 238, 394, and 399. While safe in single-threaded JS, thepackages/coreguidelines prohibit!in production code. Prefer explicit nullish handling.♻️ Example for lines 231-238 (same pattern applies at 393-399)
-if (this.pendingCreations.has(id)) { - return await this.pendingCreations.get(id)!; -} -if (this.sessions.has(id)) { - await this.updateSessionActivity(id); - return this.sessions.get(id)!; -} +const pending = this.pendingCreations.get(id); +if (pending !== undefined) { + return await pending; +} +const existing = this.sessions.get(id); +if (existing !== undefined) { + await this.updateSessionActivity(id); + return existing; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/session/session-manager.ts` around lines 231 - 238, The code uses non-null assertions (the `!` operator) after checking `.has(id)` on maps (`pendingCreations`, `sessions`), which violates guidelines; replace these with explicit nullish checks and safe returns: fetch the value into a local variable (e.g., const pending = this.pendingCreations.get(id)) and if (pending != null) return await pending; do the same for sessions (const sess = this.sessions.get(id); if (sess) { await this.updateSessionActivity(id); return sess; }) to remove `!` while preserving behavior in methods like updateSessionActivity and any other occurrences at the same pattern (lines referencing pendingCreations, sessions, get, has).packages/cli/src/cli/ink-cli/components/overlays/custom-model-wizard/LocalModelWizard.tsx (1)
442-450:--save-text-lockfileis redundant on Bun ≥1.2.9; can be dropped for cleanliness.Since the minimum Bun version is
1.2.9(perpackage.json), the--save-text-lockfileflag is redundant—Bun v1.2.0+ made text lockfiles the default. The flag is harmless, but can be removed.The concern about "silent failures" is mitigated by solid error handling: the process checks exit code (line 458) and captures stderr (lines 453–455) to display to the user if installation fails, so a broken install would not silently exit 0. The
--trustflag correctly enables postinstall scripts fornode-llama-cpp's native builds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/cli/ink-cli/components/overlays/custom-model-wizard/LocalModelWizard.tsx` around lines 442 - 450, Remove the redundant "--save-text-lockfile" argument from the Bun install spawn call in LocalModelWizard.tsx: locate the spawn invocation that runs 'bun' with args ['add', '--trust', 'node-llama-cpp', '--save-text-lockfile'] and drop the "--save-text-lockfile" entry so the args become ['add', '--trust', 'node-llama-cpp']; keep the existing stdio, cwd, shell options and the exit-code/stderr handling around child to preserve error reporting.README.md (1)
106-106: Update AGENTS.md to reflect the Bun-over-pnpm convention change.This PR replaces pnpm with Bun as the package manager across CI workflows (
.github/workflows/*.yml) and scripts — file paths that are explicitly in scope of the existing repo guideline: "Use pnpm as the package manager (do not use npm/yarn)". Once this migration is accepted, AGENTS.md should be updated to replace that rule with the new Bun-first convention, so future contributors and automated checks have a consistent authoritative source.Based on learnings: "Keep AGENTS.md updated when... repo-wide conventions change".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 106, AGENTS.md still mandates "Use pnpm as the package manager"; update that rule to a Bun-first convention to match the repo change: replace the pnpm-specific line with a clear statement like "Use Bun as the package manager (do not use npm/yarn/pnpm)", update any examples or command snippets to use bun install / bun run, and add a note referencing CI workflows and scripts that were migrated to Bun so contributors and linters use the new authoritative source; ensure the file mentions fallback guidance (how to run pnpm only if absolutely necessary) and update any checklist or automated-check instructions that reference pnpm.packages/cli/src/cli/utils/local-model-setup.ts (1)
109-114:--save-text-lockfileis redundant with Bun 1.2.9Since the repo pins Bun 1.2.9 and the text-based
bun.lockis the default format in Bun 1.2+, this flag has no effect. It won't cause failures but can be removed.♻️ Proposed cleanup
- const child = spawn('bun', ['add', '--trust', 'node-llama-cpp', '--save-text-lockfile'], { + const child = spawn('bun', ['add', '--trust', 'node-llama-cpp'], {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/cli/utils/local-model-setup.ts` around lines 109 - 114, The spawn call that installs node-llama-cpp includes the redundant '--save-text-lockfile' flag; update the spawn invocation in local-model-setup.ts (the code that creates the 'child' variable via spawn('bun', [...] , { cwd: depsDir, ... })) to remove '--save-text-lockfile' from the arguments array so it simply calls bun add --trust node-llama-cpp, leaving stdio, cwd and shell options unchanged..github/workflows/build_and_test.yml (2)
19-23: Missingrestore-keyson the Bun install cache stepWithout a
restore-keysfallback, any lockfile change causes a complete cache miss with no partial restoration. Add a fallback key so existing package caches can still accelerate installs after lockfile updates.♻️ Proposed fix
- name: Cache Bun install uses: actions/cache@v4 with: path: ~/.bun/install/cache key: ${{ runner.os }}-bun-${{ hashFiles('bun.lock', 'bun.lockb') }} + restore-keys: | + ${{ runner.os }}-bun-🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build_and_test.yml around lines 19 - 23, The "Cache Bun install" GitHub Actions step currently only defines a strict key using hashFiles('bun.lock','bun.lockb') so any lockfile change causes a full cache miss; add a restore-keys fallback in that step (the step named "Cache Bun install" that uses actions/cache@v4) such as a less-specific prefix like `${{ runner.os }}-bun-` so the action can partially restore previous caches when the exact key is absent, keeping the existing path (~/.bun/install/cache) and primary key unchanged while adding the restore-keys entry to improve cache hits.
14-17: Intentional guideline deviation: Bun replaces pnpm in CIThe repo-wide AGENTS.md guideline specifies pnpm for workflow files; this PR is the explicit migration. No action needed — just flagging the intentional deviation so reviewers are aware. Based on learnings: "Applies to
.github/workflows/*.{yml,yaml}: Use pnpm as the package manager (do not use npm/yarn)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build_and_test.yml around lines 14 - 17, The workflow step "Set up Bun" (uses: oven-sh/setup-bun@v2 with bun-version: '1.2.9') deviates from the repo guideline to use pnpm; if this PR intentionally migrates CI to Bun, leave the change but add an explicit inline comment or update the PR description and AGENTS.md to record the intentional migration so reviewers know it's deliberate, otherwise revert the change by replacing the "Set up Bun" step with the standard pnpm setup step (i.e., restore the pnpm setup action and related install/test commands) to comply with the repo-wide guideline.scripts/link-global-cli.ts (2)
69-76: Same empty catch block issue here.♻️ Proposed fix
try { const resolved = execSync('command -v dexto', { encoding: 'utf-8' }).trim(); if (resolved && resolved !== linkPath) { console.log(`⚠️ Note: \`dexto\` currently resolves to: ${resolved}`); console.log(' Ensure Bun global bin comes first in your PATH.'); console.log(''); } - } catch {} + } catch { + // `command -v` may fail if dexto is not in PATH yet; non-fatal + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/link-global-cli.ts` around lines 69 - 76, The empty catch block in the try around execSync('command -v dexto') should be changed to capture the thrown error and handle or log it instead of silently swallowing it; update the catch to `catch (err)` and either log a debug/warn via console.warn/processLogger including the error and context (e.g., that resolving `dexto` failed) or explicitly document why it is safe to ignore, referring to the existing variables `execSync`, `resolved`, and `linkPath` in the try block so the error handling sits next to the resolution logic in scripts/link-global-cli.ts.
42-49: Add comments to empty catch blocks to satisfy ESLint and aid readability.The static analysis tool flags these as
no-emptyviolations. The silenced errors are intentional, but a brief comment explaining why makes the intent explicit and silences the linter.♻️ Proposed fix
try { chmodSync(cliEntrypoint, 0o755); - } catch {} + } catch { + // chmod may fail on Windows; non-fatal + } // Remove any Bun-installed global package first to avoid ambiguity. try { execSync('bun remove -g dexto', { stdio: 'ignore' }); - } catch {} + } catch { + // Package may not be installed globally; ignore + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/link-global-cli.ts` around lines 42 - 49, The two empty catch blocks after the chmodSync(cliEntrypoint, 0o755) call and the execSync('bun remove -g dexto', ...) call should contain brief explanatory comments so ESLint's no-empty rule is satisfied and the intent is clear; update the catch blocks to include short comments like "// ignore errors: best-effort permission change for cliEntrypoint" and "// ignore errors: best-effort attempt to remove global Bun package" (referencing the chmodSync and execSync calls to locate the spots).scripts/publish-packages.ts (1)
285-289: Consider batching tag pushes to reduce remote round-trips.Each tag is pushed individually, which means N remote connections for N published packages. A single
git push origin --tagsor batching the tag names would be faster, especially in CI.♻️ Suggested optimization
if (publishedTags.length > 0) { console.log(`🏷️ Created ${publishedTags.length} tag(s). Pushing tags...`); - for (const tagName of publishedTags) { - await $`git push origin ${tagName}`; - } + await $`git push origin ${{ raw: publishedTags.map($.escape).join(' ') }}`; } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/publish-packages.ts` around lines 285 - 289, The current code iterates publishedTags and pushes each tag individually (publishedTags loop), causing N remote round-trips; change it to a single git push by either using `git push origin --tags` or by joining the publishedTags into one git push invocation (e.g., git push origin <tag1> <tag2> ...) so you push all tags in one remote call; update the block that currently iterates publishedTags to perform one $`git push ...` call instead of awaiting a push per tag.packages/cli/src/cli/utils/scaffolding-utils.ts (1)
295-316: Hardcoded Bun install commands look correct for the migration.The
--save-text-lockfileflag produces a human-readable lockfile, which is good for version control. Note that if an end-user runs this scaffolding without Bun installed,executeWithTimeout('bun', ...)will fail with a potentially confusing error. Consider whether a pre-check or a clearer error message for missing Bun would improve UX.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/cli/utils/scaffolding-utils.ts` around lines 295 - 316, The installDependencies function currently calls executeWithTimeout('bun', ...) which will throw a confusing error if Bun is not installed; update installDependencies to first verify Bun is available (e.g., attempt a lightweight check like running 'bun --version' or checking PATH) before running add commands, and if the check fails throw or log a clear, actionable error message instructing the user to install Bun or choose an alternative package manager; alternatively wrap the executeWithTimeout calls in try/catch and translate ENOENT or related errors into the same clear message. Ensure the change references installDependencies and executeWithTimeout so reviewers can find the check and the improved error handling.packages/storage/src/database/factories/sqlite.ts (1)
21-23: No error surfacing ifbun:sqliteis unavailable (e.g. Node runtime).The removed try/catch was the right call for the Bun migration, and the old install-hint comment is now resolved. The one remaining edge: if
sqlite-store.jsis somehow imported outside a Bun process, the dynamic import throws a rawCannot find package 'bun:sqlite'without any actionable message. Sinceengines.bun >= 1.2.9guards at install time this is low risk, but a minimal guard would improve the failure UX.💡 Optional: surface a clear error on non-Bun runtimes
create: async (config, logger) => { + if (typeof Bun === 'undefined') { + throw new Error( + 'SQLite storage requires Bun runtime (bun:sqlite). Run with `bun run` or install Bun: https://bun.sh' + ); + } const module = await import('../sqlite-store.js'); logger.info(`Creating SQLite database store: ${config.path}`); return new module.SQLiteStore(config, logger); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/storage/src/database/factories/sqlite.ts` around lines 21 - 23, Wrap the dynamic import of '../sqlite-store.js' in a try/catch and, on failure, log or throw a clear, actionable error that explains bun:sqlite is unavailable on non‑Bun runtimes (include the original error message for debugging); specifically catch errors around the import(...) call that returns the module used to construct new module.SQLiteStore(config, logger), use logger.error to emit a descriptive message (e.g., "SQLite store requires bun:sqlite and must run on Bun; original error: ...") and then rethrow or throw a new Error to ensure the failure surface is meaningful to callers.package.json (1)
108-108: Removetsxfrom rootdevDependencies.Root scripts have been migrated to Bun; this package is no longer needed at the workspace level. Each example (discord-bot, telegram-bot) maintains its own
tsxdependency in their respectivepackage.jsonfiles, so removal here is safe and reduces dead weight.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 108, Remove the "tsx" entry from the root package.json devDependencies: locate the devDependencies block where "tsx": "^4.19.2" appears and delete that key/value pair; verify no root scripts reference "tsx" (scripts that were migrated to Bun) and ensure example workspaces (discord-bot, telegram-bot) still keep their own "tsx" in their package.json files so local example builds remain unaffected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/PULL_REQUEST_TEMPLATE.md:
- Line 4: The PR template line "- [ ] Changeset added via `bun x changeset`
(select packages + bump)" was changed prematurely; revert or hold this checklist
item in the WIP branch and restore the previous instruction (or temporarily add
a comment) so contributors still using the canonical toolchain won't be misled.
Specifically, update PULL_REQUEST_TEMPLATE.md to either keep the original
changeset instruction until the Bun migration is finalized, or add a clear note
next to the "`bun x changeset`" checklist item stating it should only be used
once the Bun migration is merged to main and toolchains are updated.
In `@feature-plans/bun-migration/PLAN.md`:
- Line 41: The document contains a committed local filesystem path string
'~/Projects/dexto-bun-migration' which exposes a contributor's local
environment; replace that literal with a neutral placeholder or relative path
(e.g., '<project-root>' or './dexto-bun-migration') or remove the line entirely,
update any accompanying timestamp text if needed, and ensure the string
'~/Projects/dexto-bun-migration' is no longer present in PLAN.md so the repo
contains no personal/local paths.
In `@package.json`:
- Line 23: The package.json scripts "build:check" and "build:strict" are
identical (both set to "bun run typecheck && bun run build"); inspect the git
history or project docs to determine what additional checks "build:strict"
previously ran (e.g., tsconfig strict flags, an ESLint run, a stricter typecheck
command like "bun run typecheck --strict" or a dedicated "build:strict"
pipeline) and then either restore that stricter command for the "build:strict"
script (update the "build:strict" value accordingly) or remove the redundant
"build:strict" script if it was intentionally aliased; make the change by
editing the package.json scripts entry for "build:strict" (and update any CI
references) so the intent is unambiguous.
In `@packages/cli/src/cli/utils/execute.ts`:
- Line 21: The spawn call in execute.ts uses shell on Windows, and unvalidated
user input (baseImage from create-image.ts) is passed into executeWithTimeout,
enabling shell injection; fix by validating the package spec before invoking
executeWithTimeout (or refusing unsafe input): implement npm package name/spec
validation in the code that collects baseImage (create-image.ts around the
baseImage handling) using a strict npm package-name/spec regex that allows only
alphanumerics, hyphens, underscores, dots, tildes and valid scoped forms (e.g.
`@scope/name` and optional version/tag like `@1.2.3` or `@latest`), reject or
re-prompt on invalid input, and only pass the validated value to
executeWithTimeout/spawn in execute.ts (alternatively remove shell:true and
ensure proper argv splitting for Windows shims if that approach is preferred).
Ensure the validation logic is referenced and applied to the same
symbol/parameter named baseImage before calling executeWithTimeout.
In `@packages/cli/src/cli/utils/project-files.ts`:
- Around line 12-17: The catch for fs.access in the project-files helper
currently treats every error as "not found"; update the catch to inspect the
thrown error's code (e.g., err.code) and only convert to FileNotFoundError and
log `${fileName} not found...` when the code is 'ENOENT'; for other errors (like
'EACCES') rethrow the original error (or log and throw the original Error) so
that permission and other failures are not misreported. Locate the try/catch
around fs.access (references: fs.access, filePath, fileName, FileNotFoundError,
logger.debug) and implement the conditional handling based on error.code
accordingly.
In `@scripts/unlink-global-cli.ts`:
- Around line 30-33: The empty catch block after the execSync call triggers
ESLint no-empty; modify the catch in the try { execSync('bun remove -g dexto', {
stdio: 'ignore' }); } catch {} around the execSync invocation to include an
inline ESLint suppression comment (for example add a comment like //
eslint-disable-next-line no-empty or a comment inside the catch explaining
intentional swallow) so the linter knows the empty catch is intentional while
leaving behavior unchanged.
- Around line 40-44: safeRemoveIfExists currently returns early when the target
doesn't exist but the script still prints success messages; change the flow to
only log the "✅ Unlinked dexto CLI (Bun global)" and "Removed: ..." messages
when an actual removal occurred by (A) updating safeRemoveIfExists to return a
boolean (true if removed, false if not) and using its return value to
conditionally print, or (B) checking existence first (e.g.,
fs.existsSync(linkPath)) and only calling safeRemoveIfExists + printing when the
file/link exists; reference the symbol linkPath and the safeRemoveIfExists call
to locate the change.
---
Outside diff comments:
In `@CONTRIBUTING.md`:
- Around line 148-151: The Setup section still shows the legacy command "npm
install"; update the template README example to use Bun by replacing "npm
install" with "bun install" (leave the other lines like "export
YOUR_API_KEY=your-key" and "dexto --agent your-agent-name.yml" unchanged) so the
example matches the migrated docs.
- Around line 296-300: Update the "### Prerequisites" section to include Bun as
a required runtime/package manager: locate the "### Prerequisites" header and
the existing bullets "- Node.js >= 20.0.0" and "- Git" and add a new bullet like
"- Bun >= 1.0.0 (required for bun install and as the runtime)"; keep or adjust
the Node.js line if Node is still needed for any dev workflows, and ensure the
wording clearly states Bun is required to run the project and install
dependencies.
In `@packages/storage/README.md`:
- Around line 79-80: The browser-safety note "Do not import from
`@dexto/storage` in browser bundles, since the root entry also exports Node
implementations." is inconsistent with other references to the Bun runtime;
update that sentence to mention "Bun runtime" (or match the exact phrasing used
elsewhere like "Bun runtime") so it reads e.g. "Do not import from
`@dexto/storage` in browser bundles, since the root entry also exports Bun
runtime implementations," ensuring the wording aligns with lines that already
reference "Bun runtime."
---
Duplicate comments:
In `@packages/storage/src/database/sqlite-store.ts`:
- Around line 104-128: The code reads sqliteOptions['verbose'] into verbose and
logs it but never applies it; update the sqlite-store.ts around the verbose
extraction (sqliteOptions['verbose']), the BunSqliteDatabase construction, and
the subsequent PRAGMA calls to include a short comment explaining that
bun:sqlite has no verbose equivalent so the option is intentionally ignored (or
suggest renaming to _verbose to avoid linter noise), and note that timeout is
handled via db.exec('PRAGMA busy_timeout = ...') while verbose is a no-op for
future maintainers; keep the debug log as-is but document why verbose isn't
applied.
---
Nitpick comments:
In @.github/workflows/build_and_test.yml:
- Around line 19-23: The "Cache Bun install" GitHub Actions step currently only
defines a strict key using hashFiles('bun.lock','bun.lockb') so any lockfile
change causes a full cache miss; add a restore-keys fallback in that step (the
step named "Cache Bun install" that uses actions/cache@v4) such as a
less-specific prefix like `${{ runner.os }}-bun-` so the action can partially
restore previous caches when the exact key is absent, keeping the existing path
(~/.bun/install/cache) and primary key unchanged while adding the restore-keys
entry to improve cache hits.
- Around line 14-17: The workflow step "Set up Bun" (uses: oven-sh/setup-bun@v2
with bun-version: '1.2.9') deviates from the repo guideline to use pnpm; if this
PR intentionally migrates CI to Bun, leave the change but add an explicit inline
comment or update the PR description and AGENTS.md to record the intentional
migration so reviewers know it's deliberate, otherwise revert the change by
replacing the "Set up Bun" step with the standard pnpm setup step (i.e., restore
the pnpm setup action and related install/test commands) to comply with the
repo-wide guideline.
In `@package.json`:
- Line 108: Remove the "tsx" entry from the root package.json devDependencies:
locate the devDependencies block where "tsx": "^4.19.2" appears and delete that
key/value pair; verify no root scripts reference "tsx" (scripts that were
migrated to Bun) and ensure example workspaces (discord-bot, telegram-bot) still
keep their own "tsx" in their package.json files so local example builds remain
unaffected.
In
`@packages/cli/src/cli/ink-cli/components/overlays/custom-model-wizard/LocalModelWizard.tsx`:
- Around line 442-450: Remove the redundant "--save-text-lockfile" argument from
the Bun install spawn call in LocalModelWizard.tsx: locate the spawn invocation
that runs 'bun' with args ['add', '--trust', 'node-llama-cpp',
'--save-text-lockfile'] and drop the "--save-text-lockfile" entry so the args
become ['add', '--trust', 'node-llama-cpp']; keep the existing stdio, cwd, shell
options and the exit-code/stderr handling around child to preserve error
reporting.
In `@packages/cli/src/cli/utils/local-model-setup.ts`:
- Around line 109-114: The spawn call that installs node-llama-cpp includes the
redundant '--save-text-lockfile' flag; update the spawn invocation in
local-model-setup.ts (the code that creates the 'child' variable via
spawn('bun', [...] , { cwd: depsDir, ... })) to remove '--save-text-lockfile'
from the arguments array so it simply calls bun add --trust node-llama-cpp,
leaving stdio, cwd and shell options unchanged.
In `@packages/cli/src/cli/utils/scaffolding-utils.ts`:
- Around line 295-316: The installDependencies function currently calls
executeWithTimeout('bun', ...) which will throw a confusing error if Bun is not
installed; update installDependencies to first verify Bun is available (e.g.,
attempt a lightweight check like running 'bun --version' or checking PATH)
before running add commands, and if the check fails throw or log a clear,
actionable error message instructing the user to install Bun or choose an
alternative package manager; alternatively wrap the executeWithTimeout calls in
try/catch and translate ENOENT or related errors into the same clear message.
Ensure the change references installDependencies and executeWithTimeout so
reviewers can find the check and the improved error handling.
In `@packages/core/src/session/session-manager.ts`:
- Around line 258-298: The code in createSessionInternal does a redundant second
DB read for the same session key; reuse the already-fetched existingMetadata
instead of calling getDatabase().get<SessionData>(sessionKey) again. Replace the
second reference (currently assigned to sessionData) with existingMetadata (or
rename existingMetadata to sessionData for clarity) when checking llmOverride
and restoring the LLM config, and remove the duplicate
storageManager.getDatabase().get call to avoid the extra round-trip.
- Around line 231-238: The code uses non-null assertions (the `!` operator)
after checking `.has(id)` on maps (`pendingCreations`, `sessions`), which
violates guidelines; replace these with explicit nullish checks and safe
returns: fetch the value into a local variable (e.g., const pending =
this.pendingCreations.get(id)) and if (pending != null) return await pending; do
the same for sessions (const sess = this.sessions.get(id); if (sess) { await
this.updateSessionActivity(id); return sess; }) to remove `!` while preserving
behavior in methods like updateSessionActivity and any other occurrences at the
same pattern (lines referencing pendingCreations, sessions, get, has).
In `@packages/storage/src/database/factories/sqlite.ts`:
- Around line 21-23: Wrap the dynamic import of '../sqlite-store.js' in a
try/catch and, on failure, log or throw a clear, actionable error that explains
bun:sqlite is unavailable on non‑Bun runtimes (include the original error
message for debugging); specifically catch errors around the import(...) call
that returns the module used to construct new module.SQLiteStore(config,
logger), use logger.error to emit a descriptive message (e.g., "SQLite store
requires bun:sqlite and must run on Bun; original error: ...") and then rethrow
or throw a new Error to ensure the failure surface is meaningful to callers.
In `@README.md`:
- Line 106: AGENTS.md still mandates "Use pnpm as the package manager"; update
that rule to a Bun-first convention to match the repo change: replace the
pnpm-specific line with a clear statement like "Use Bun as the package manager
(do not use npm/yarn/pnpm)", update any examples or command snippets to use bun
install / bun run, and add a note referencing CI workflows and scripts that were
migrated to Bun so contributors and linters use the new authoritative source;
ensure the file mentions fallback guidance (how to run pnpm only if absolutely
necessary) and update any checklist or automated-check instructions that
reference pnpm.
In `@scripts/link-global-cli.ts`:
- Around line 69-76: The empty catch block in the try around execSync('command
-v dexto') should be changed to capture the thrown error and handle or log it
instead of silently swallowing it; update the catch to `catch (err)` and either
log a debug/warn via console.warn/processLogger including the error and context
(e.g., that resolving `dexto` failed) or explicitly document why it is safe to
ignore, referring to the existing variables `execSync`, `resolved`, and
`linkPath` in the try block so the error handling sits next to the resolution
logic in scripts/link-global-cli.ts.
- Around line 42-49: The two empty catch blocks after the
chmodSync(cliEntrypoint, 0o755) call and the execSync('bun remove -g dexto',
...) call should contain brief explanatory comments so ESLint's no-empty rule is
satisfied and the intent is clear; update the catch blocks to include short
comments like "// ignore errors: best-effort permission change for
cliEntrypoint" and "// ignore errors: best-effort attempt to remove global Bun
package" (referencing the chmodSync and execSync calls to locate the spots).
In `@scripts/publish-packages.ts`:
- Around line 285-289: The current code iterates publishedTags and pushes each
tag individually (publishedTags loop), causing N remote round-trips; change it
to a single git push by either using `git push origin --tags` or by joining the
publishedTags into one git push invocation (e.g., git push origin <tag1> <tag2>
...) so you push all tags in one remote call; update the block that currently
iterates publishedTags to perform one $`git push ...` call instead of awaiting a
push per tag.
| "build:analytics": "turbo run build --filter='@dexto/analytics...'", | ||
| "build:client-sdk": "turbo run build --filter='@dexto/client-sdk...'", | ||
| "build:check": "pnpm run typecheck && pnpm run build", | ||
| "build:check": "bun run typecheck && bun run build", |
There was a problem hiding this comment.
build:check and build:strict are identical — likely a migration oversight.
Both resolve to bun run typecheck && bun run build. If build:strict previously applied stricter compiler flags or an additional lint/type-check pass, those constraints are silently lost.
💡 Proposed fix (if build:strict was stricter)
- "build:strict": "bun run typecheck && bun run build",
+ "build:strict": "bun run typecheck && bun run build && bun run lint",Verify what build:strict previously did and restore the stricter behavior, or remove the duplicate alias if it was already a no-op.
Also applies to: 28-28
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 23, The package.json scripts "build:check" and
"build:strict" are identical (both set to "bun run typecheck && bun run build");
inspect the git history or project docs to determine what additional checks
"build:strict" previously ran (e.g., tsconfig strict flags, an ESLint run, a
stricter typecheck command like "bun run typecheck --strict" or a dedicated
"build:strict" pipeline) and then either restore that stricter command for the
"build:strict" script (update the "build:strict" value accordingly) or remove
the redundant "build:strict" script if it was intentionally aliased; make the
change by editing the package.json scripts entry for "build:strict" (and update
any CI references) so the intent is unambiguous.
| return new Promise<void>((resolve, reject) => { | ||
| const { cwd, timeoutMs: timeout = DEFAULT_TIMEOUT_MS } = options; | ||
| const child = spawn(command, args, { cwd }); | ||
| const child = spawn(command, args, { cwd, shell: process.platform === 'win32' }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the execute.ts file
fd -t f "execute.ts" packages/cli/src/cli/utils/ && wc -l packages/cli/src/cli/utils/execute.tsRepository: truffle-ai/dexto
Length of output: 138
🏁 Script executed:
# Read the execute.ts file to understand the function
cat -n packages/cli/src/cli/utils/execute.tsRepository: truffle-ai/dexto
Length of output: 2737
🏁 Script executed:
# Find all call sites of executeWithTimeout across the codebase
rg -n --type ts "executeWithTimeout\s*\(" --context 8Repository: truffle-ai/dexto
Length of output: 8484
🏁 Script executed:
# Find where installDependencies is called to see source of deps
rg -n --type ts "installDependencies\s*\(" --context 5Repository: truffle-ai/dexto
Length of output: 6402
🏁 Script executed:
# Get more context on where deps.dependencies comes from in scaffolding-utils
sed -n '290,320p' packages/cli/src/cli/utils/scaffolding-utils.tsRepository: truffle-ai/dexto
Length of output: 895
🏁 Script executed:
# Check image-store context for packDir origin (around line 309-320)
sed -n '295,330p' packages/cli/src/cli/utils/image-store.tsRepository: truffle-ai/dexto
Length of output: 1592
🏁 Script executed:
# Find all usages of installSpecifier in image-store to understand its source
rg -n "installSpecifier" packages/cli/src/cli/utils/image-store.ts --context 3Repository: truffle-ai/dexto
Length of output: 1347
🏁 Script executed:
# Find the image installation/loading function that uses specifier
rg -n "async.*installImage\|async.*function.*specifier" packages/cli/src/cli/utils/image-store.ts --context 10 | head -100Repository: truffle-ai/dexto
Length of output: 42
🏁 Script executed:
# Look for the function signature around line 250-280 to see where specifier comes from
sed -n '250,290p' packages/cli/src/cli/utils/image-store.tsRepository: truffle-ai/dexto
Length of output: 1707
🏁 Script executed:
# Check create-app.ts to see how dependencies are built
sed -n '170,200p' packages/cli/src/cli/commands/create-app.tsRepository: truffle-ai/dexto
Length of output: 1065
🏁 Script executed:
# Check create-image.ts to see how dependencies are built
sed -n '200,240p' packages/cli/src/cli/commands/create-image.tsRepository: truffle-ai/dexto
Length of output: 1611
🏁 Script executed:
# Find where specifier parameter comes from in image-store
rg -n "specifier\s*:" packages/cli/src/cli/utils/image-store.ts --context 1 | head -40Repository: truffle-ai/dexto
Length of output: 175
🏁 Script executed:
# Find where baseImage parameter comes from in create-image.ts
rg -n "baseImage" packages/cli/src/cli/commands/create-image.ts --context 3 | head -50Repository: truffle-ai/dexto
Length of output: 1565
🏁 Script executed:
# Check the function signature of createImageCommand to see how baseImage is obtained
sed -n '1,100p' packages/cli/src/cli/commands/create-image.ts | grep -A 50 "export.*createImageCommand\|async function"Repository: truffle-ai/dexto
Length of output: 1895
🏁 Script executed:
# Look at the CLI prompt for baseImage
rg -n "baseImage\|base.*[Ii]mage" packages/cli/src/cli/commands/create-image.ts --context 2 | head -80Repository: truffle-ai/dexto
Length of output: 42
🏁 Script executed:
# Find the pinDextoPackageIfUnversioned function to see if it sanitizes the baseImage
rg -n "pinDextoPackageIfUnversioned" packages/cli/src/cli/commands/create-image.ts -A 10Repository: truffle-ai/dexto
Length of output: 884
🏁 Script executed:
# Search for the function definition
rg -n "function pinDextoPackageIfUnversioned\|pinDextoPackageIfUnversioned.*=" packages/cli/src/cli/utils/ -A 5Repository: truffle-ai/dexto
Length of output: 42
🏁 Script executed:
# Check if there's any validation/sanitization of user input before it reaches dependencies
rg -n "customBase\|baseImage" packages/cli/src/cli/commands/create-image.ts -B 2 -A 2Repository: truffle-ai/dexto
Length of output: 42
🏁 Script executed:
# Search for pinDextoPackageIfUnversioned in scaffolding-utils
rg -n "pinDextoPackageIfUnversioned" packages/cli/src/cli/utils/scaffolding-utils.ts -A 10Repository: truffle-ai/dexto
Length of output: 588
🏁 Script executed:
# Also check what textOrExit does - does it validate input?
rg -n "textOrExit\|function textOrExit" packages/cli/src/ -A 8 --type tsRepository: truffle-ai/dexto
Length of output: 42
🏁 Script executed:
# Let me check if customBase from textOrExit has any sanitization
sed -n '80,100p' packages/cli/src/cli/commands/create-image.tsRepository: truffle-ai/dexto
Length of output: 761
🏁 Script executed:
# Check npm package naming rules to see if package names can contain shell metacharacters
web_search "npm package name valid characters allowed special characters"Repository: truffle-ai/dexto
Length of output: 110
🏁 Script executed:
# Also check if there are any other sanitization/validation steps before baseImage reaches installDependencies
rg -n "installDependencies\|baseImageDependency" packages/cli/src/cli/commands/create-image.ts -B 5 -A 5Repository: truffle-ai/dexto
Length of output: 42
🏁 Script executed:
# Check what isLocalDependencySpecifier does
rg -n "isLocalDependencySpecifier" packages/cli/src/cli/utils/scaffolding-utils.ts -A 5Repository: truffle-ai/dexto
Length of output: 783
🏁 Script executed:
# Check npm package name specification
web_search "npm package name specification allowed characters RFC"Repository: truffle-ai/dexto
Length of output: 110
🏁 Script executed:
# Let me also verify if there are any other inputs that flow into executeWithTimeout args
rg -n "executeWithTimeout" packages/cli/src/cli/utils/image-store.ts -B 3 -A 3 | grep -A 5 "installSpecifier"Repository: truffle-ai/dexto
Length of output: 535
🏁 Script executed:
# Check the resolveFileLikeImageSpecifierToPath function to understand installSpecifier
rg -n "resolveFileLikeImageSpecifierToPath\|findSingleTgzFile" packages/cli/src/cli/utils/ -A 8Repository: truffle-ai/dexto
Length of output: 42
🏁 Script executed:
# Search for resolveFileLikeImageSpecifierToPath function definition
rg -n "resolveFileLikeImageSpecifierToPath" packages/cli/src/cli/utils/ --type ts -A 8Repository: truffle-ai/dexto
Length of output: 2830
🏁 Script executed:
# Search for findSingleTgzFile function definition
rg -n "findSingleTgzFile" packages/cli/src/cli/utils/ --type ts -A 8Repository: truffle-ai/dexto
Length of output: 1556
🏁 Script executed:
# Check npm package naming in docs or look at the npm spec
# Package names can only contain lowercase letters, numbers, hyphens, underscores, and `@symbol` (for scoped packages)
# Let's verify by checking actual validation in the codebase if it exists
rg -n "validateProjectName\|validate.*package\|isValidPackage" packages/cli/src/cli/utils/ --type ts -A 3Repository: truffle-ai/dexto
Length of output: 42
🏁 Script executed:
# Let me check the full context of installImageToStore to see the complete flow of specifier
sed -n '253,330p' packages/cli/src/cli/utils/image-store.tsRepository: truffle-ai/dexto
Length of output: 3282
🏁 Script executed:
# Check how baseImage flows through - does it get validated anywhere else?
rg -n "baseImage\|customBase" packages/cli/src/cli/commands/create-image.ts --context 1Repository: truffle-ai/dexto
Length of output: 42
🏁 Script executed:
# Check if there's validation of package names before they reach installDependencies
rg -n "validateProjectName\|validProjectName\|isValidPackageName" packages/cli/src/cli/utils/scaffolding-utils.ts -A 5Repository: truffle-ai/dexto
Length of output: 42
🏁 Script executed:
# Check the full context around baseImage usage to see if there's any sanitization
sed -n '83,102p' packages/cli/src/cli/commands/create-image.tsRepository: truffle-ai/dexto
Length of output: 726
🏁 Script executed:
# Look for any package name validation regex patterns in the codebase
rg -n "validate.*name\|name.*regex\|/.*[a-z].*/" packages/cli/src/cli/utils/scaffolding-utils.ts -B 2 -A 5Repository: truffle-ai/dexto
Length of output: 42
🏁 Script executed:
# Check if npm package names allow special shell characters
# Package names must match: ^(?:@[a-z0-9-*~][a-z0-9-*._~]*/)?[a-z0-9-*~][a-z0-9-*._~]*$
# Let's verify this by looking at any npm validation in the codebase
rg -n "validateProjectName\|isValidProject" packages/cli/src/cli/utils/scaffolding-utils.ts -A 8Repository: truffle-ai/dexto
Length of output: 42
🏁 Script executed:
# Let me check if there's any npm-style package name validation anywhere
rg -n "@.*[a-z0-9]|package.*name|npm.*spec" packages/cli/src/cli/utils/ --type ts -B 2 -A 3 | head -80Repository: truffle-ai/dexto
Length of output: 5740
🏁 Script executed:
# Check what the exact validation is in the project name validation
sed -n '54,75p' packages/cli/src/cli/utils/scaffolding-utils.tsRepository: truffle-ai/dexto
Length of output: 861
🏁 Script executed:
# Let me look at the comprehensive flow: where does baseImage come from and is there ANY npm package name validation?
rg -n "Enter the package name" packages/cli/src/cli/commands/ -A 15 -B 2Repository: truffle-ai/dexto
Length of output: 1560
🏁 Script executed:
# Confirm that a malicious package name would be accepted and reach executeWithTimeout unchanged
# Let's trace: textOrExit -> baseImage -> pinDextoPackageIfUnversioned -> installDependencies -> executeWithTimeout
# Check pinDextoPackageIfUnversioned again - does it sanitize?
sed -n '47,52p' packages/cli/src/cli/utils/scaffolding-utils.tsRepository: truffle-ai/dexto
Length of output: 392
Shell injection vulnerability on Windows via unvalidated package names.
When shell: true is active on Windows, spawn routes execution through cmd.exe /d /s /c, concatenating command and all args into a single shell string. If any element of args contains shell metacharacters like &, |, >, <, or ^, cmd.exe will interpret them as operators — enabling command injection.
The baseImage parameter (custom package name at CLI prompt in create-image.ts:84) flows directly into the args array with only non-empty validation (no npm package name spec validation). An attacker can inject shell metacharacters: e.g., entering my-package&echo pwned at the prompt bypasses validation and results in arbitrary command execution on Windows.
The fix resolves the .cmd/.bat shim resolution issue, but it introduces a shell injection vector unless package names are validated against the npm package naming specification (only alphanumeric, hyphens, underscores, dots, tildes, and @ for scoped packages).
Recommend adding npm package name validation before passing user-supplied package specifiers to executeWithTimeout.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/cli/utils/execute.ts` at line 21, The spawn call in
execute.ts uses shell on Windows, and unvalidated user input (baseImage from
create-image.ts) is passed into executeWithTimeout, enabling shell injection;
fix by validating the package spec before invoking executeWithTimeout (or
refusing unsafe input): implement npm package name/spec validation in the code
that collects baseImage (create-image.ts around the baseImage handling) using a
strict npm package-name/spec regex that allows only alphanumerics, hyphens,
underscores, dots, tildes and valid scoped forms (e.g. `@scope/name` and optional
version/tag like `@1.2.3` or `@latest`), reject or re-prompt on invalid input, and
only pass the validated value to executeWithTimeout/spawn in execute.ts
(alternatively remove shell:true and ensure proper argv splitting for Windows
shims if that approach is preferred). Ensure the validation logic is referenced
and applied to the same symbol/parameter named baseImage before calling
executeWithTimeout.
Comment on lines
+12
to
+17
| try { | ||
| await fs.access(filePath); | ||
| } catch { | ||
| logger.debug(`${fileName} not found in the current directory.`); | ||
| throw new FileNotFoundError(`${fileName} not found in the current directory.`); | ||
| } |
There was a problem hiding this comment.
Catch block masks non-"not found" errors (e.g., permission denied).
fs.access can fail for reasons other than the file being absent (e.g., EACCES). The current catch treats all failures identically, logging "not found" and throwing FileNotFoundError even when the file exists but is inaccessible. For a CLI guard this is likely acceptable in practice, but distinguishing error codes would prevent misleading diagnostics.
Proposed fix to distinguish ENOENT from other errors
try {
await fs.access(filePath);
- } catch {
- logger.debug(`${fileName} not found in the current directory.`);
- throw new FileNotFoundError(`${fileName} not found in the current directory.`);
+ } catch (error: unknown) {
+ const code = error instanceof Error && 'code' in error ? (error as NodeJS.ErrnoException).code : undefined;
+ if (code === 'ENOENT') {
+ logger.debug(`${fileName} not found in the current directory.`);
+ throw new FileNotFoundError(`${fileName} not found in the current directory.`);
+ }
+ throw error;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/cli/utils/project-files.ts` around lines 12 - 17, The catch
for fs.access in the project-files helper currently treats every error as "not
found"; update the catch to inspect the thrown error's code (e.g., err.code) and
only convert to FileNotFoundError and log `${fileName} not found...` when the
code is 'ENOENT'; for other errors (like 'EACCES') rethrow the original error
(or log and throw the original Error) so that permission and other failures are
not misreported. Locate the try/catch around fs.access (references: fs.access,
filePath, fileName, FileNotFoundError, logger.debug) and implement the
conditional handling based on error.code accordingly.
Comment on lines
+30
to
+33
| try { | ||
| execSync('bun remove -g dexto', { stdio: 'ignore' }); | ||
| } catch {} | ||
|
|
There was a problem hiding this comment.
Empty catch block triggers ESLint no-empty error
This is flagged by static analysis. Add a comment to suppress it intentionally.
🛠️ Proposed fix
try {
execSync('bun remove -g dexto', { stdio: 'ignore' });
- } catch {}
+ } catch {
+ // Intentionally ignored: package may not be globally installed
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try { | |
| execSync('bun remove -g dexto', { stdio: 'ignore' }); | |
| } catch {} | |
| try { | |
| execSync('bun remove -g dexto', { stdio: 'ignore' }); | |
| } catch { | |
| // Intentionally ignored: package may not be globally installed | |
| } | |
🧰 Tools
🪛 ESLint
[error] 32-32: Empty block statement.
(no-empty)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/unlink-global-cli.ts` around lines 30 - 33, The empty catch block
after the execSync call triggers ESLint no-empty; modify the catch in the try {
execSync('bun remove -g dexto', { stdio: 'ignore' }); } catch {} around the
execSync invocation to include an inline ESLint suppression comment (for example
add a comment like // eslint-disable-next-line no-empty or a comment inside the
catch explaining intentional swallow) so the linter knows the empty catch is
intentional while leaving behavior unchanged.
scripts/unlink-global-cli.ts
Outdated
Comment on lines
+40
to
+44
| const linkPath = path.join(bunGlobalBinDir, 'dexto'); | ||
| safeRemoveIfExists(linkPath); | ||
|
|
||
| console.log('✅ Unlinked dexto CLI (Bun global)'); | ||
| console.log(` Removed: ${linkPath}`); |
There was a problem hiding this comment.
Success message logged even when nothing was removed
safeRemoveIfExists returns early (without printing or signalling) if the file doesn't exist, yet lines 43–44 unconditionally print "✅ Unlinked" and "Removed: ...". This is misleading when neither bun remove -g dexto nor the file removal did anything.
🛠️ Proposed fix
- const linkPath = path.join(bunGlobalBinDir, 'dexto');
- safeRemoveIfExists(linkPath);
-
- console.log('✅ Unlinked dexto CLI (Bun global)');
- console.log(` Removed: ${linkPath}`);
+ const linkPath = path.join(bunGlobalBinDir, 'dexto');
+ const linkExists = existsSync(linkPath);
+ safeRemoveIfExists(linkPath);
+
+ if (linkExists) {
+ console.log('✅ Unlinked dexto CLI (Bun global)');
+ console.log(` Removed: ${linkPath}`);
+ } else {
+ console.log('ℹ️ No dexto link found at:', linkPath);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/unlink-global-cli.ts` around lines 40 - 44, safeRemoveIfExists
currently returns early when the target doesn't exist but the script still
prints success messages; change the flow to only log the "✅ Unlinked dexto CLI
(Bun global)" and "Removed: ..." messages when an actual removal occurred by (A)
updating safeRemoveIfExists to return a boolean (true if removed, false if not)
and using its return value to conditionally print, or (B) checking existence
first (e.g., fs.existsSync(linkPath)) and only calling safeRemoveIfExists +
printing when the file/link exists; reference the symbol linkPath and the
safeRemoveIfExists call to locate the change.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@feature-plans/bun-migration/PLAN.md`:
- Line 41: Replace the hardcoded local path string
"~/Projects/dexto-bun-migration" in PLAN.md with a neutral placeholder (e.g.,
"<project-root>" or "<path/to/project>"); locate the exact occurrence of the
path literal in the file and update it so the document contains no
machine-specific filesystem references, then save the file.
- Around line 264-265: Replace the hard-coded local path string
'~/Projects/external/opencode' in PLAN.md (the mention in the sentence
describing discovered `.opencode/` directories and MCP server configuration)
with a neutral reference such as a public repo URL or a placeholder like
'<path-to-opencode>' so no contributor local filesystem paths are committed;
update all occurrences (e.g., the instance noted near the earlier reference) to
use the same neutral form for consistency.
In `@feature-plans/bun-migration/TASKLIST.md`:
- Around line 53-66: The document has duplicate section headings labeled "## PR
2 (deferred)" for the blocks titled "Native TS `.dexto` layering + extension
system" and "Deprecate/redesign the image store"; update the second occurrence
by renaming its heading to "## PR 3 (deferred)" or collapse both into a single
"## PR 2 (deferred)" and make the titles ("Native TS `.dexto` layering +
extension system" and "Deprecate/redesign the image store") subheadings under it
so the PR numbering is unique and the structure is unambiguous.
- Around line 36-52: Update the Phase 2 checklist entry text to expand the
registry access note and clarify the three unchecked items: specify what
"outside `dexto-source`" means (e.g., forks, CI runners, or local clones missing
a .npmrc), explain why scaffolds like `dexto create-app`, `dexto create-image`,
and `dexto init` may require access to the `@dexto/*` registry (private scoped
packages referenced by templates), and give explicit workarounds for validation
(add instructions to temporarily set up an .npmrc with registry auth, use a
personal access token, or run within the monorepo). Also mark the remaining
checklist entries (`agents/agent-template.yml` npx→bunx substitution and whether
to include `--bun`, validating MCP servers/filesystem/playwright; confirming CLI
flows; validating `bun pm pack` tarball and linked installs) with short
acceptance criteria and steps to reproduce so they can be checked off.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
packages/storage/src/database/sqlite-store.ts (1)
86-91: Redundant index on PRIMARY KEY column.
idx_kv_store_keyindexeskv_store(key), butkeyis already thePRIMARY KEYon that table — SQLite automatically creates an index for it. This wastes a small amount of disk/write overhead for no benefit.Proposed fix
db.exec(` - CREATE INDEX IF NOT EXISTS idx_kv_store_key ON kv_store(key); CREATE INDEX IF NOT EXISTS idx_list_store_key ON list_store(key); CREATE INDEX IF NOT EXISTS idx_list_store_sequence ON list_store(key, sequence); `);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/storage/src/database/sqlite-store.ts` around lines 86 - 91, Remove the redundant index creation for idx_kv_store_key since kv_store(key) is already the table PRIMARY KEY and SQLite auto-indexes it; in the db.exec block that creates indexes (the SQL string executed by db.exec) delete the `CREATE INDEX IF NOT EXISTS idx_kv_store_key ON kv_store(key);` statement and leave the other list_store indexes intact to avoid unnecessary disk/write overhead..github/workflows/build_and_test.yml (1)
19-23: Consider addingrestore-keysfor partial cache hits.Without a
restore-keysfallback, any lockfile change causes a full cache miss. Adding a prefix-based fallback improves cache reuse.Proposed fix
- name: Cache Bun install uses: actions/cache@v4 with: path: ~/.bun/install/cache key: ${{ runner.os }}-bun-${{ hashFiles('bun.lock', 'bun.lockb') }} + restore-keys: | + ${{ runner.os }}-bun-🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build_and_test.yml around lines 19 - 23, The cache step "Cache Bun install" using actions/cache@v4 should include a restore-keys fallback to allow partial cache hits when bun.lock/bun.lockb change; update the step that currently sets path: ~/.bun/install/cache and key: ${{ runner.os }}-bun-${{ hashFiles('bun.lock', 'bun.lockb') }} to add a restore-keys entry (e.g. a prefix like ${{ runner.os }}-bun-) so the cache action can match on the prefix and reuse earlier caches when the full key misses.package.json (1)
108-108: Remove the unusedtsxdevDependency.After migrating to Bun, all root-level TypeScript script execution uses Bun natively (e.g.,
bun run clean,bun scripts/dev-server.ts). Thetsxdependency is no longer referenced by any scripts, build configs, or tools in the monorepo and can be safely removed from devDependencies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 108, Remove the unused devDependency "tsx" from the root package.json devDependencies block (delete the "tsx": "^4.19.2" entry) and then update the lockfile by running the project's package manager (e.g., bun install / npm install / yarn install) so the lockfile and node_modules reflect the removal; ensure no scripts or config reference the tsx symbol before committing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-docs.yml:
- Around line 29-32: Update the GitHub Actions step that sets up Bun by changing
the action reference from oven-sh/setup-bun@v1 to oven-sh/setup-bun@v2 (the step
currently named "Set up Bun" where uses: oven-sh/setup-bun@v1 and bun-version:
'1.3.5'), ensuring the workflow aligns with build_and_test.yml; no other changes
required besides updating the action version string.
In @.github/workflows/sync-cli-readme.yml:
- Around line 25-28: The workflow step named "Set up Bun" currently uses
oven-sh/setup-bun@v1; update that action reference to oven-sh/setup-bun@v2 so it
matches build_and_test.yml and standardizes Bun setup across workflows—modify
the step with the "Set up Bun" name and replace bun-version if needed but
primarily change the action tag from `@v1` to `@v2`.
In `@DEVELOPMENT.md`:
- Line 104: Update the stale npm wording in DEVELOPMENT.md: replace the phrase
"simulating `npm install -g dexto`" with the Bun equivalent (e.g., "simulating
`bun install -g dexto`" or "simulating `bun add -g dexto`") so the tarball
installation instructions remain consistent with the Bun migration.
- Around line 324-328: The documentation currently uses unsupported flags with
the command `bun pm ls -g --depth=0`; update it to remove the invalid flags and
use a supported form such as `bun pm ls` (or `bun pm ls --all` for a full tree)
to list dependencies, while keeping the existing `bun pm bin -g` and `echo
$PATH` lines unchanged; ensure the text clarifies that `-g` is valid for `bun pm
bin -g` but not for `bun pm ls`.
In `@feature-plans/bun-migration/WORKING_MEMORY.md`:
- Around line 119-125: Update WORKING_MEMORY.md to resolve the unresolved
validation note: either (A) re-run and document an end-to-end validation of
node-llama-cpp loading a GGUF model and running a prompt under Bun 1.3.5
(include exact steps, environment, bun version, and a one-line pass/fail result)
and keep the item in Open Questions marked as validated for PR 1, or (B)
explicitly defer local model validation to PR 2 by removing or moving the
node-llama-cpp / GGUF bullet from the PR 1 Open Questions list and adding a
clear statement that PR 1 does not include local model support; update the
node-llama-cpp, GGUF, Bun 1.3.5, PR 1 / PR 2 references accordingly so the
status is unambiguous.
- Around line 83-90: Remove the redundant command `bun install
--save-text-lockfile` from the "Green commands" list in WORKING_MEMORY.md
because Bun >= v1.2 uses a text-based bun.lock by default; simply delete that
line (the entry `- \`bun install --save-text-lockfile\``) or replace it with a
short note explaining it is a no-op on Bun >=1.2 to avoid propagating the flag
into CI/developer scripts.
In `@packages/storage/src/database/sqlite-store.ts`:
- Around line 196-198: The INSERT into kv_store uses Date.now() (milliseconds)
for updated_at while the schema default strftime('%s','now') uses epoch seconds;
change the value passed in the db.prepare(...).run(...) call (in the set/insert
logic referencing db.prepare and kv_store.updated_at) to use epoch seconds
(e.g., Math.floor(Date.now()/1000)) so stored timestamps match the schema
convention and time-based queries remain correct.
---
Duplicate comments:
In `@package.json`:
- Line 23: The package.json scripts "build:check" and "build:strict" are
identical; update "build:strict" to run a stricter typecheck before building
(e.g., replace its command with a strict typecheck script) so they differ: keep
"build:check" as "bun run typecheck && bun run build" and change "build:strict"
to call a stricter entry like "bun run typecheck:strict && bun run build" (and
add/modify the "typecheck:strict" script to run tsc/tsserver with --strict or
the appropriate strict flags).
In `@packages/storage/src/database/sqlite-store.ts`:
- Around line 126-136: The debug output extracts and logs
sqliteOptions['verbose'] into the debug object in sqlite-store.ts but the
bun:sqlite Database constructor doesn't accept a verbose option, so remove the
local verbose extraction and drop it from the this.logger.debug payload (or, if
you intend to keep it for informational purposes, add a short comment above the
extraction explaining it's intentionally not passed to Database). Update the
code around sqliteOptions, timeout, and the Database constructor usage (look for
the timeout variable, sqliteOptions, and the this.logger.debug call) so the
debug fields accurately reflect only options actually applied to the Database.
---
Nitpick comments:
In @.github/workflows/build_and_test.yml:
- Around line 19-23: The cache step "Cache Bun install" using actions/cache@v4
should include a restore-keys fallback to allow partial cache hits when
bun.lock/bun.lockb change; update the step that currently sets path:
~/.bun/install/cache and key: ${{ runner.os }}-bun-${{ hashFiles('bun.lock',
'bun.lockb') }} to add a restore-keys entry (e.g. a prefix like ${{ runner.os
}}-bun-) so the cache action can match on the prefix and reuse earlier caches
when the full key misses.
In `@package.json`:
- Line 108: Remove the unused devDependency "tsx" from the root package.json
devDependencies block (delete the "tsx": "^4.19.2" entry) and then update the
lockfile by running the project's package manager (e.g., bun install / npm
install / yarn install) so the lockfile and node_modules reflect the removal;
ensure no scripts or config reference the tsx symbol before committing.
In `@packages/storage/src/database/sqlite-store.ts`:
- Around line 86-91: Remove the redundant index creation for idx_kv_store_key
since kv_store(key) is already the table PRIMARY KEY and SQLite auto-indexes it;
in the db.exec block that creates indexes (the SQL string executed by db.exec)
delete the `CREATE INDEX IF NOT EXISTS idx_kv_store_key ON kv_store(key);`
statement and leave the other list_store indexes intact to avoid unnecessary
disk/write overhead.
Comment on lines
+29
to
+32
| - name: Set up Bun | ||
| uses: oven-sh/setup-bun@v1 | ||
| with: | ||
| node-version: '20' | ||
| cache: npm | ||
| cache-dependency-path: ./docs/package-lock.json | ||
| bun-version: '1.3.5' |
There was a problem hiding this comment.
Same setup-bun@v1 vs @v2 inconsistency as noted in sync-cli-readme.yml.
Align with build_and_test.yml by using oven-sh/setup-bun@v2.
Proposed fix
- name: Set up Bun
- uses: oven-sh/setup-bun@v1
+ uses: oven-sh/setup-bun@v2
with:
bun-version: '1.3.5'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Set up Bun | |
| uses: oven-sh/setup-bun@v1 | |
| with: | |
| node-version: '20' | |
| cache: npm | |
| cache-dependency-path: ./docs/package-lock.json | |
| bun-version: '1.3.5' | |
| - name: Set up Bun | |
| uses: oven-sh/setup-bun@v2 | |
| with: | |
| bun-version: '1.3.5' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build-docs.yml around lines 29 - 32, Update the GitHub
Actions step that sets up Bun by changing the action reference from
oven-sh/setup-bun@v1 to oven-sh/setup-bun@v2 (the step currently named "Set up
Bun" where uses: oven-sh/setup-bun@v1 and bun-version: '1.3.5'), ensuring the
workflow aligns with build_and_test.yml; no other changes required besides
updating the action version string.
Comment on lines
+25
to
+28
| - name: Set up Bun | ||
| uses: oven-sh/setup-bun@v1 | ||
| with: | ||
| node-version: 20 | ||
| cache: 'pnpm' | ||
| bun-version: '1.3.5' |
There was a problem hiding this comment.
Inconsistent setup-bun action version across workflows.
This workflow uses oven-sh/setup-bun@v1, while build_and_test.yml uses oven-sh/setup-bun@v2. Standardize on @v2 across all workflows to avoid subtle differences in setup behavior.
Proposed fix
- name: Set up Bun
- uses: oven-sh/setup-bun@v1
+ uses: oven-sh/setup-bun@v2
with:
bun-version: '1.3.5'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Set up Bun | |
| uses: oven-sh/setup-bun@v1 | |
| with: | |
| node-version: 20 | |
| cache: 'pnpm' | |
| bun-version: '1.3.5' | |
| - name: Set up Bun | |
| uses: oven-sh/setup-bun@v2 | |
| with: | |
| bun-version: '1.3.5' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/sync-cli-readme.yml around lines 25 - 28, The workflow
step named "Set up Bun" currently uses oven-sh/setup-bun@v1; update that action
reference to oven-sh/setup-bun@v2 so it matches build_and_test.yml and
standardizes Bun setup across workflows—modify the step with the "Set up Bun"
name and replace bun-version if needed but primarily change the action tag from
`@v1` to `@v2`.
DEVELOPMENT.md
Outdated
| bun run install-cli-fast | ||
| ``` | ||
|
|
||
| This creates tarballs and installs them globally, simulating `npm install -g dexto`. |
There was a problem hiding this comment.
Stale npm reference in Bun-migrated documentation.
This line still says "simulating npm install -g dexto" — should reference bun install -g dexto (or bun add -g dexto) to stay consistent with the rest of the migration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@DEVELOPMENT.md` at line 104, Update the stale npm wording in DEVELOPMENT.md:
replace the phrase "simulating `npm install -g dexto`" with the Bun equivalent
(e.g., "simulating `bun install -g dexto`" or "simulating `bun add -g dexto`")
so the tarball installation instructions remain consistent with the Bun
migration.
Comment on lines
+324
to
328
| bun pm ls -g --depth=0 | ||
|
|
||
| # Verify PATH includes pnpm/npm global bin | ||
| # Verify PATH includes Bun global bin | ||
| bun pm bin -g | ||
| echo $PATH |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Bun package manager pm command subcommands documentation
💡 Result:
Bun’s bun pm (package-manager utilities) subcommands are documented here: (bun.sh)
Subcommands (from the official docs): (bun.sh)
bun pm pack— create an npm-style publish tarball (.tgz)bun pm bin— print local (or-gglobal)node_modules/.binpathbun pm ls— list installed deps (use--allfor full tree)bun pm whoami— print npm username (requires logged-in creds)bun pm hash/hash-string/hash-print— lockfile hash utilitiesbun pm cache/cache rm— show or clear Bun’s global module cachebun pm migrate— migrate another package manager lockfile (no install)bun pm untrusted— list deps whose lifecycle scripts were blockedbun pm trust <name...>(--all) — run scripts + add totrustedDependenciesbun pm default-trusted— print Bun’s default trusted-deps allowlistbun pm version— show/bump package version (npm-version-like)bun pm pkg get|set|delete|fix ...— read/modifypackage.jsonfields
Citations:
Remove unsupported flags from bun pm ls command.
bun pm ls does not support the -g (global) or --depth flags. Use bun pm ls alone to list installed dependencies, or add --all for a full dependency tree. For printing the global bin path (as done in the next line), the -g flag is correct with bun pm bin -g, but it's not applicable to bun pm ls.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@DEVELOPMENT.md` around lines 324 - 328, The documentation currently uses
unsupported flags with the command `bun pm ls -g --depth=0`; update it to remove
the invalid flags and use a supported form such as `bun pm ls` (or `bun pm ls
--all` for a full tree) to list dependencies, while keeping the existing `bun pm
bin -g` and `echo $PATH` lines unchanged; ensure the text clarifies that `-g` is
valid for `bun pm bin -g` but not for `bun pm ls`.
Comment on lines
+83
to
+90
| - Green commands: | ||
| - `bun install --frozen-lockfile` | ||
| - `bun install --save-text-lockfile` | ||
| - `bun run build` | ||
| - `bun run typecheck` | ||
| - `bun run test` | ||
| - `bun run test:unit` | ||
| - `bun run lint` |
There was a problem hiding this comment.
bun install --save-text-lockfile is redundant on Bun 1.3.5
bun install --save-text-lockfile was required to generate the text-based lockfile only until Bun v1.2. Bun v1.2 changed the default lockfile format to the text-based bun.lock, so listing this flag as an explicit validation command on Bun 1.3.5 is misleading — it still works but is a no-op. Consider removing it from the "green commands" list to avoid cargo-culting it into CI scripts or developer onboarding steps.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@feature-plans/bun-migration/WORKING_MEMORY.md` around lines 83 - 90, Remove
the redundant command `bun install --save-text-lockfile` from the "Green
commands" list in WORKING_MEMORY.md because Bun >= v1.2 uses a text-based
bun.lock by default; simply delete that line (the entry `- \`bun install
--save-text-lockfile\``) or replace it with a short note explaining it is a
no-op on Bun >=1.2 to avoid propagating the flag into CI/developer scripts.
Comment on lines
+119
to
+125
| ## Open Questions / Blockers | ||
|
|
||
| 1. **Local models:** `node-llama-cpp` installs and imports successfully under Bun `1.2.9` (validated in a temp project on macOS). Should re-validate under Bun `1.3.5`. Next validation is end-to-end local model execution (GGUF load + prompt). | ||
| 2. **Repo dev scripts:** decide whether to keep `scripts/install-global-cli.ts` as-is (dev-only) or migrate it to `bunx`/Bun-first equivalents as part of PR 1. | ||
| 3. **Image store future (PR 2):** keep and port to Bun, or replace with `~/.dexto` as a package root (preferred). | ||
| 4. **Scaffolding + registry:** running `dexto create-app` outside `dexto-source` currently fails if `@dexto/*` packages aren’t available in the configured registry (observed 404 for `@dexto/storage` from the public npm registry). This is likely pre-existing; PR 1 should avoid changing this behavior, but we should document expectations. | ||
| 5. **MCP “no npm” default:** should we update default agent templates to use Bun-first commands for MCP servers (`bunx …` or `bunx --bun …`)? |
There was a problem hiding this comment.
Open Question 1 — node-llama-cpp / GGUF validation under Bun 1.3.5 is still unresolved
Line 121 explicitly notes that node-llama-cpp was only validated under Bun 1.2.9 and that re-validation under 1.3.5 is still needed. Line 149 records "bash scripts/quality-checks.sh all is green" for the 1.3.5 bump checkpoint, but a unit/build green signal does not cover end-to-end GGUF load + prompt execution. If local model support is in scope for PR 1 (no functionality changes), this should be confirmed before merge — or explicitly deferred to PR 2 and removed from the Open Questions list so its status is unambiguous.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@feature-plans/bun-migration/WORKING_MEMORY.md` around lines 119 - 125, Update
WORKING_MEMORY.md to resolve the unresolved validation note: either (A) re-run
and document an end-to-end validation of node-llama-cpp loading a GGUF model and
running a prompt under Bun 1.3.5 (include exact steps, environment, bun version,
and a one-line pass/fail result) and keep the item in Open Questions marked as
validated for PR 1, or (B) explicitly defer local model validation to PR 2 by
removing or moving the node-llama-cpp / GGUF bullet from the PR 1 Open Questions
list and adding a clear statement that PR 1 does not include local model
support; update the node-llama-cpp, GGUF, Bun 1.3.5, PR 1 / PR 2 references
accordingly so the status is unambiguous.
Comment on lines
+196
to
+198
| db.prepare( | ||
| 'INSERT OR REPLACE INTO kv_store (key, value, updated_at) VALUES (?, ?, ?)' | ||
| ).run(key, serialized, Date.now()); |
There was a problem hiding this comment.
Date.now() stores milliseconds, but schema defaults use epoch seconds.
The updated_at column default is strftime('%s', 'now') (Unix epoch in seconds), but this explicit INSERT passes Date.now() which returns milliseconds. Rows inserted via set() will have updated_at values ~1000× larger than rows using the schema default, making any time-based queries or comparisons on this column silently incorrect.
Use Math.floor(Date.now() / 1000) to match the schema convention, or change the schema default to milliseconds — but be consistent.
Proposed fix (align to seconds)
db.prepare(
'INSERT OR REPLACE INTO kv_store (key, value, updated_at) VALUES (?, ?, ?)'
- ).run(key, serialized, Date.now());
+ ).run(key, serialized, Math.floor(Date.now() / 1000));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| db.prepare( | |
| 'INSERT OR REPLACE INTO kv_store (key, value, updated_at) VALUES (?, ?, ?)' | |
| ).run(key, serialized, Date.now()); | |
| db.prepare( | |
| 'INSERT OR REPLACE INTO kv_store (key, value, updated_at) VALUES (?, ?, ?)' | |
| ).run(key, serialized, Math.floor(Date.now() / 1000)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/storage/src/database/sqlite-store.ts` around lines 196 - 198, The
INSERT into kv_store uses Date.now() (milliseconds) for updated_at while the
schema default strftime('%s','now') uses epoch seconds; change the value passed
in the db.prepare(...).run(...) call (in the set/insert logic referencing
db.prepare and kv_store.updated_at) to use epoch seconds (e.g.,
Math.floor(Date.now()/1000)) so stored timestamps match the schema convention
and time-based queries remain correct.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release Note
pnpm changeset(select packages + bump)Summary by CodeRabbit
New Features
Chores