Skip to content

fix(discovery): bind project discovery to the running framework install - #3561

Merged
kojiwakayama merged 1 commit into
mainfrom
fix/dx-20260811-0742-3
Aug 11, 2026
Merged

kojiwakayama merged 1 commit into
mainfrom
fix/dx-20260811-0742-3

Conversation

@kojiwakayama

@kojiwakayama kojiwakayama commented Aug 11, 2026 •

Copy link
Copy Markdown
Contributor

Found by a DX dogfood walk of https://veryfront.com/docs/code/getting-started/create-project against published CLI 0.1.1228, installed the way the installation page tells you to (npm install -g veryfront).

Symptom

veryfront init clean-app --template ai-agent
cd clean-app
veryfront dev --port 3222      # then click the template's "Run the numbers" suggestion

The app boots and renders with a clean browser console, but every chat message fails — including the template's own built-in suggestion buttons:

event: RunError
data: {"message":"Unknown tool reference: calculator. Tool names must exactly match
tool({ id: \"...\" }). Available tools: execute_skill_script, load_skill, load_skill_reference"}

tools/calculator.ts in the scaffold declares id: "calculator" exactly. With VERYFRONT_DEBUG=1 the real failure is visible at boot:

● Found 1 tool files in /…/clean-app/tools
✗ Error loading file:///…/clean-app/tools/calculator.ts:
    err=VeryfrontError: Missing extension for contract "SchemaValidator".
        Install it with: deno add @veryfront/ext-schema-zod
● Found 1 agent files in /…/clean-app/agents
· [Discovery] Registered 0 tools, 1 agents, …

It is specific to the launcher. Same project, same 0.1.1228, cache cleared:

Launcher Result
veryfront dev (global npm install) broken — agents/ loads, tools/ does not
npm run dev (node_modules/.bin/veryfront) works
bun run dev (--runtime bun scaffold) works

Root cause

rewriteDiscoveryImports rewrites bare veryfront/* imports in discovered project files to the project's own node_modules/veryfront package (#3037 — discovery modules are emitted outside the project tree, so they must not resolve by chance).

That is correct when the CLI is the project's install. A globally installed CLI runs from a different install, so the project copy becomes a second framework instance in the same process — and nothing ever bootstraps its extension contracts. defineSchema() in that instance resolves SchemaValidator against an empty contract registry and throws. Tool discovery catches the error per file, so the project registers 0 tools; agents/assistant.ts imports only veryfront/agent, which needs no contract, so agents keep loading and the failure only surfaces later as Unknown tool reference.

ext-schema-zod is bundled in the package and is loaded — just into the CLI's instance, not the project copy's. Hence the misleading "install it with deno add" advice.

Fix

Prefer the framework install that is actually executing when it is a different npm install than the project's. The project-local preference from #3037 is unchanged everywhere it was already right:

  • project-local CLI launch — running install is the project's copy, no change;
  • source checkouts and jsr: / https: runtimes — not an npm install, so the rule never triggers and the existing fail-closed behaviour for a present-but-unreadable project package is preserved.

Regression test

src/discovery/import-rewriter.test.ts — Deno BDD, alongside the eight existing tests that pin this exact resolution decision.

  • binds discovery imports to the running framework install when the CLI runs from another install — fails before the fix, resolving to <project>/node_modules/veryfront/local/schemas.js instead of the running install.
  • keeps project-local veryfront exports when the running install is the project's own — pins the Expose programmatic project runtime discovery #3037 behaviour that must not regress.

It lives in veryfront-code rather than veryfront-e2e because the defect is pure module resolution: reproducible in-process, no browser, no deployment, no credentials — so it runs in the pre-push gate. The runtime resolver is injected through a new optional resolveSpecifier option (mirroring rewriteForDeno's existing seam) so a test can stand in for a globally installed CLI deterministically.

Verification

Beyond the unit test, the same change was applied to the published 0.1.1228 artifact and the finding's command re-run against a freshly scaffolded ai-agent project:

● Found 1 tool files in /…/clean-app/tools
· [tool] Registered "calculator" for scope __default__
· [Discovery] Registered 1 tools, 1 agents, …
… /api/ag-ui … agents=1 tools=1 errors=0

Zero occurrences of Unknown tool reference (was one per message). Full pre-push suite passed.

Summary by CodeRabbit

  • Bug Fixes

    • Improved discovery import resolution when the CLI runs from a different framework installation.
    • Ensured imports use the active framework installation when appropriate, while preserving project-local exports when installations match.
  • Tests

    • Added coverage for matching and differing framework installation scenarios.

A globally installed CLI never registered a project's tools/ directory, so
the default `ai-agent` template failed on every chat message with
`Unknown tool reference: calculator`.

Discovery rewrites bare `veryfront/*` imports in project files to the
project's own `node_modules/veryfront` package. That is correct when the CLI
is the project's own install, but a globally installed CLI runs from a
different install: the project copy is then a second framework instance whose
extension contracts were never bootstrapped. `defineSchema()` throws
`Missing extension for contract "SchemaValidator"`, tool discovery aborts for
that file, and the project registers 0 tools while agents/ still loads.

Prefer the framework install that is actually executing when it is a
different npm install than the project's. Source checkouts and jsr:/https:
runtimes are not npm installs, so the project-local preference is unchanged
there and for project-local CLI launches.
@coderabbitai

coderabbitai Bot commented Aug 11, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

Discovery import resolution

Layer / File(s) Summary
Runtime package resolution
src/discovery/import-rewriter.ts
rewriteDiscoveryImports accepts an optional specifier resolver. It compares runtime and project-local Veryfront installations and rewrites imports only when they differ.
Installation matching tests
src/discovery/import-rewriter.test.ts
Tests cover different installations and matching installations that retain project-local exports.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: kwakayama

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main discovery fix: binding project discovery to the running framework installation.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/dx-20260811-0742-3

Comment @coderabbitai help to get the list of available commands.

@kojiwakayama

Copy link
Copy Markdown
Contributor Author

CI triage: the red shard was an unrelated flake, not a regression from this PR

coverage shard 8/8 failed (which in turn failed tests (unit) and coverage gate, since the shard uploads no lcov.info). The shard job only reports "shards finished with failure", so for the record, the actual failing assertion was:

HTTP Bundle Cache ... returns a signal-less cache follower after its bounded wait
error: AssertionError: Values are not equal.
    -   false
    +   true
    at src/transforms/esm/http-cache.test.ts:803:11
FAILED | 577 passed (3267 steps) | 1 failed (1 step)

http-cache.test.ts:803 is assertEquals(followerSettled, true).

Why it is not this PR:

  • the diff is two files, both under src/discovery/ (import-rewriter.ts, import-rewriter.test.ts). Nothing in src/transforms/esm/ imports discovery;
  • src/discovery/import-rewriter.test.ts is not even in shard 8's file list — no new test file was added, so shard partitioning is unchanged;
  • that test landed ~5h before this branch, in fix: allow cold remote modules to finish fetching #3553 (43f5e28c7), and is the newest thing in src/transforms/esm/http-cache.test.ts.

Proof it is a flake: re-ran the failed jobs on the same commit (27abca5e), no code change — coverage shard 8/8, tests (unit) and coverage gate all pass. Locally the file passes 3/3 (1 passed (73 steps) | 0 failed).

Likely cause, for whoever owns #3553: the test drives the follower with FakeTime, then asserts settlement after a fixed time.tickAsync(HTTP_MODULE_FETCH_MAX_WAIT_MS) + a single time.runMicrotasks(). The follower's rejection path still crosses real (unfaked) I/O inside withIsolatedHttpCache's temp dir, so on a loaded runner it has not settled by the time the assertion runs. Awaiting followerOutcome before asserting, or polling followerSettled the way the test already polls the waiter count above it, would make it deterministic. Deliberately not fixing it here to keep this PR's diff on-topic — flagging it as a separate flake to fix.

Also note: the curl: (22) 404 in the shard log is the Deno setup step's checksum-manifest fallback, present on green shards too. Unrelated infra noise.

@kojiwakayama

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 11, 2026 •

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@kojiwakayama

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 11, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/discovery/import-rewriter.test.ts`:
- Around line 474-552: Replace the Deno-specific fixture setup and cleanup in
both tests around the discovery import assertions with runtime-neutral helpers
from `#veryfront/testing/deno-compat.ts`, including temporary-directory creation,
directory creation, file writes, and recursive removal. Preserve the existing
test scenarios and assertions so the regression coverage runs under Node, Bun,
and Deno.

In `@src/discovery/import-rewriter.ts`:
- Around line 512-525: Exclude bare veryfront framework specifiers from the
earlier generic package-rewriting pass so they remain unchanged when
veryfrontSpecifiers is collected. Update the generic rewrite logic in the
surrounding import-rewriting flow, preserving veryfront/schemas and related
framework specifiers for the framework-specific loop containing
resolveRuntimeSpecifierToFileUrl and rewriteResolvedSpecifierImports.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ea5a9aac-d0d8-4b78-82b3-d36c126e92c6

📥 Commits

Reviewing files that changed from the base of the PR and between fe5d5b8 and 27abca5.

📒 Files selected for processing (2)
  • src/discovery/import-rewriter.test.ts
  • src/discovery/import-rewriter.ts

Comment thread src/discovery/import-rewriter.test.ts
Comment thread src/discovery/import-rewriter.ts
@kojiwakayama
kojiwakayama added this pull request to the merge queue Aug 11, 2026
Merged via the queue into main with commit f231bc4 Aug 11, 2026
57 of 60 checks passed
@kojiwakayama
kojiwakayama deleted the fix/dx-20260811-0742-3 branch August 11, 2026 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant