Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces dynamic prebuilt-language download/publish with a deterministic bootstrapSeed.json locate-and-copy flow, removes unzipper/node-wget-js/@types, always invokes ad4m with --language-language-only 'false', and simplifies startup sequencing and error reporting. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (test-runner)
participant Installer as installSystemLanguages
participant FS as Filesystem (bootstrapSeed.json)
participant AD4M as ad4m binary
CLI->>Installer: start installSystemLanguages(relativePath)
Installer->>FS: search candidate bootstrapSeed.json paths
FS-->>Installer: path found / not found
alt seed found
Installer->>FS: copy seed -> package root bootstrapSeed.json
Installer->>AD4M: ensure binary exists
Installer->>AD4M: kill holochain & lair-keystore processes
CLI->>AD4M: spawn with --language-language-only 'false'
AD4M-->>CLI: start result
else seed missing
Installer-->>CLI: throw descriptive error (missing bootstrapSeed)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test-runner/src/installSystemLanguages.ts`:
- Around line 16-17: The UI execution path calls startServer without seeding
bootstrap data, causing startServer (which reads bootstrapSeed.json) to fail;
update the UI startup flow to invoke installSystemLanguages(...) before calling
startServer so the bootstrap seed is copied (same behavior as runtest() path),
i.e., ensure installSystemLanguages is executed in the UI branch prior to
startServer to create test-runner/bootstrapSeed.json.
HexaField
force-pushed
the
fix/ad4m-test-use-bootstrap-seed
branch
from
3b38090 to
513172b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test-runner/src/cli.ts (1)
105-105:defaultLangPathparameter is now dead instartServer's body.The change on line 162 removes the last use of
defaultLangPathinsidestartServer. The parameter can be dropped from the signature (it is optional, so callers passing it won't break, but it silently does nothing).♻️ Proposed cleanup
-export function startServer(relativePath: string, bundle: string, meta: string, languageType: string, port: number, defaultLangPath?: string, callback?: any): Promise<any> { +export function startServer(relativePath: string, bundle: string, meta: string, languageType: string, port: number, callback?: any): Promise<any> {Update the call-site in
run()(line 286) accordingly:- await startServer(relativePath, args.bundle!, args.meta!, 'expression', 4000, args.defaultLangPath, () => { + await startServer(relativePath, args.bundle!, args.meta!, 'expression', 4000, () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-runner/src/cli.ts` at line 105, Remove the now-unused optional parameter defaultLangPath from the startServer function signature and all internal references; update its declaration export function startServer(relativePath: string, bundle: string, meta: string, languageType: string, port: number, callback?: any): Promise<any> and adjust any callers—specifically update the run() call-site (previously passing defaultLangPath on line where run invokes startServer) to stop supplying that argument so call-sites match the new signature; ensure TypeScript types and any exported usages compile after the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test-runner/src/installSystemLanguages.ts`:
- Around line 36-55: installSystemLanguages.ts fails at runtime because
bootstrapSeed.json from tests/js/bootstrapSeed.json is not copied into the
test-runner package, so the candidate paths in the candidates array (used in the
seedPath resolution) won't exist for installed users; fix by adding a build or
prepare step to copy tests/js/bootstrapSeed.json into the test-runner output
directory (so ../bootstrapSeed.json exists next to the compiled
installSystemLanguages.js), update package.json build/prepare scripts
accordingly, and ensure package.json "files" includes the copied
bootstrapSeed.json so npm publishes it.
---
Duplicate comments:
In `@test-runner/src/cli.ts`:
- Around line 285-303: The UI branch inside the args.ui conditional calls
startServer without ensuring the bootstrap seed exists; before invoking
startServer in this block, call installSystemLanguages with the same parameters
used elsewhere (e.g., relativePath and args.defaultLangPath) so the bootstrap
seed is created before startServer reads it; update the CLI path to invoke
installSystemLanguages(...) prior to startServer(...) to mirror the non-UI
initialization flow.
---
Nitpick comments:
In `@test-runner/src/cli.ts`:
- Line 105: Remove the now-unused optional parameter defaultLangPath from the
startServer function signature and all internal references; update its
declaration export function startServer(relativePath: string, bundle: string,
meta: string, languageType: string, port: number, callback?: any): Promise<any>
and adjust any callers—specifically update the run() call-site (previously
passing defaultLangPath on line where run invokes startServer) to stop supplying
that argument so call-sites match the new signature; ensure TypeScript types and
any exported usages compile after the change.
513172b to
e0a5a0b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test-runner/src/installSystemLanguages.ts (1)
23-28: Binary path doesn't account for Windows.exeextension.Line 23 hardcodes
ad4mwithout consideringad4m.exeon Windows.getAd4mHostBinarysaves asad4m.exeon Windows, sofs.existsSync(binaryPath)will always befalseon Windows, triggering a redundant download attempt (which internally finds the existing binary and returns early).Since
binaryPathisn't used after this block, this is harmless — but the "already exists" check is dead code on Windows. A small fix for clarity:Suggested fix
- let binaryPath = path.join(ad4mDataDirectory(relativePath), 'binary', `ad4m`); + const isWin = process.platform === 'win32'; + let binaryPath = path.join(ad4mDataDirectory(relativePath), 'binary', isWin ? 'ad4m.exe' : 'ad4m');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-runner/src/installSystemLanguages.ts` around lines 23 - 28, The exists-check for the host binary uses a hardcoded name "ad4m" and misses the Windows executable "ad4m.exe", making the fs.existsSync(binaryPath) check always false on Windows; update the logic in installSystemLanguages.ts so the initial binaryPath resolution (which uses ad4mDataDirectory and the variable binaryPath) checks for the platform-specific filename — either determine the executable name via process.platform === 'win32' or test both path.join(ad4mDataDirectory(relativePath), 'binary', 'ad4m') and the same with '.exe' before calling getAd4mHostBinary(relativePath) — reference the binaryPath variable, ad4mDataDirectory(relativePath) call, and the getAd4mHostBinary function when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test-runner/package.json`:
- Line 38: The project depends on get-port v7+ which is ESM-only and breaks the
CommonJS build; fix by either pinning get-port to the last CJS-compatible
release (e.g., change the dependency entry in package.json for "get-port" to a
v5.x version) or convert the test-runner to ESM output (add "type": "module" to
package.json and update tsconfig.json "module" to "ESNext") and update import
usage in test-runner/src/helpers/index.ts (the import getPort from "get-port"
usage) accordingly so runtime no longer attempts require() on an ESM package.
In `@test-runner/src/installSystemLanguages.ts`:
- Around line 20-21: The call to deleteAllAd4mData in installSystemLanguages is
passing a raw relativePath which leads to fs.existsSync('') and no deletion; fix
by ensuring deleteAllAd4mData gets the same absolute path produced by
ad4mDataDirectory — either change installSystemLanguages to call
deleteAllAd4mData(ad4mDataDirectory(relativePath)) or modify deleteAllAd4mData
to resolve its input with ad4mDataDirectory before checking fs.existsSync;
update other callers (e.g., startServer/test helpers) to pass/expect the
resolved path accordingly so deletion consistently targets the home-based AD4M
data directory.
---
Duplicate comments:
In `@test-runner/package.json`:
- Around line 6-11: package.json currently lists "bootstrapSeed.json" in "files"
but the build script ("build": "tsc") doesn't copy it and it only appears at
runtime via installSystemLanguages; add a prepare (or prepublishOnly) script to
copy tests/js/bootstrapSeed.json into the package root before publish so
external consumers have it available, e.g. add a "prepare" script that runs a
small copy step (shell cp or node script) to move tests/js/bootstrapSeed.json ->
bootstrapSeed.json; update package.json scripts and ensure
installSystemLanguages continues to reference bootstrapSeed.json in the package
root.
---
Nitpick comments:
In `@test-runner/src/installSystemLanguages.ts`:
- Around line 23-28: The exists-check for the host binary uses a hardcoded name
"ad4m" and misses the Windows executable "ad4m.exe", making the
fs.existsSync(binaryPath) check always false on Windows; update the logic in
installSystemLanguages.ts so the initial binaryPath resolution (which uses
ad4mDataDirectory and the variable binaryPath) checks for the platform-specific
filename — either determine the executable name via process.platform === 'win32'
or test both path.join(ad4mDataDirectory(relativePath), 'binary', 'ad4m') and
the same with '.exe' before calling getAd4mHostBinary(relativePath) — reference
the binaryPath variable, ad4mDataDirectory(relativePath) call, and the
getAd4mHostBinary function when making this change.
e0a5a0b to
ea20c5c
Compare
Contributor
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 `@test-runner/src/cli.ts`:
- Line 107: In startServer replace the call that passes the raw relativePath
into deleteAllAd4mData so it correctly resolves the user's home directory;
instead of deleteAllAd4mData(relativePath) call
deleteAllAd4mData(ad4mDataDirectory(relativePath)) (same fix as in
installSystemLanguages) so deleteAllAd4mData checks the actual homedir path
returned by ad4mDataDirectory rather than the CWD-relative string.
In `@test-runner/src/installSystemLanguages.ts`:
- Around line 20-21: The call to deleteAllAd4mData in installSystemLanguages
passes a literal relativePath which leads to fs.existsSync('') or a CWD-relative
check and no deletion; change the call site to pass the resolved data directory
by invoking ad4mDataDirectory(relativePath) so call
deleteAllAd4mData(ad4mDataDirectory(relativePath)) instead, ensuring the
function checks the actual homedir()-based path; keep the default relativePath =
'' as-is but always resolve it via ad4mDataDirectory before calling
deleteAllAd4mData.
- Around line 36-55: The package is missing bootstrapSeed.json at runtime
because the TypeScript build (tsc) doesn't copy tests/js/bootstrapSeed.json into
the package root so candidates[0] (../bootstrapSeed.json) is absent; add a build
step that copies tests/js/bootstrapSeed.json to the package root before
publishing/building (e.g., add a "prebuild" or "prepare" script in package.json
that runs a copy command or small Node script to place bootstrapSeed.json at
test-runner/bootstrapSeed.json), so installSystemLanguages.ts's candidates array
can find ../bootstrapSeed.json at runtime; ensure this script runs in CI/publish
too.
ea20c5c to
e77472e
Compare
Contributor
Author
|
Thanks — understood. The If you'd prefer a different approach — e.g. making |
Contributor
Author
|
Thanks — understood. The If you would prefer a different approach — e.g. making |
Member
|
@HexaField, yeah can you please make the script call |
…guage bundles The test runner previously downloaded pre-built system language bundles from perspect3vism repos during postinstall. These bundles were CJS and required conversion to ESM for the executor's Deno runtime. The bundles were also stale and no longer maintained. Now the test runner uses the bootstrap seed (tests/js/bootstrapSeed.json) which contains the language-language bundle inline (already ESM, built by Deno) and hashes for all system languages. At runtime, the language-language fetches other system languages by hash from the bootstrap store (Cloudflare). Changes: - Simplify installSystemLanguages to just copy the bootstrap seed - Remove CJS→ESM bundle transform logic (no longer needed) - Remove stale language bundle downloads from postinstall - Remove unused dependencies (node-wget-js, unzipper) - Always boot executor with --language-language-only false
HexaField
force-pushed
the
fix/ad4m-test-use-bootstrap-seed
branch
from
e77472e to
4f9927e
Compare
Contributor
Author
|
Updated — ⬡ |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test-runner/package.json`:
- Line 20: The prepublishOnly script uses the Unix-only cp command to copy
../tests/js/bootstrapSeed.json which breaks on Windows; replace that cp
invocation with a cross-platform Node-based copy using the already-declared
fs-extra module (e.g., call fs-extra.copy or copySync to copy
../tests/js/bootstrapSeed.json to ./bootstrapSeed.json). Update the
"prepublishOnly" npm script to invoke a short Node script (inline node -e or a
small script file) that requires fs-extra and calls its copy/copySync function
so the file copy is platform agnostic; reference the script name
"prepublishOnly", the source file bootstrapSeed.json, and fs-extra.copy/copySync
when implementing the change.
In `@test-runner/src/installSystemLanguages.ts`:
- Around line 23-28: The code builds binaryPath using
ad4mDataDirectory(relativePath) + 'binary' + 'ad4m' which misses the Windows
.exe name and causes existsSync to always fail; also the reassignment to
binaryPath after calling getAd4mHostBinary is unused and can be removed. Fix by
checking for the platform-specific filename when constructing binaryPath (e.g.,
if process.platform === 'win32' use 'ad4m.exe' or test both 'ad4m' and
'ad4m.exe' with fs.existsSync), call getAd4mHostBinary(relativePath) only if
neither exists, and drop the dead reassignment of binaryPath after the async
call; reference symbols: binaryPath, getAd4mHostBinary, ad4mDataDirectory, and
relativePath.
lucksus
approved these changes
Feb 19, 2026
Member
lucksus
left a comment
lucksus
left a comment
There was a problem hiding this comment.
Approving althouth I found those findAndKillProcess('holochain') calls which should be removed - super old, from before Rust refactor. This is now all in one process ad4m-executor. But that is not an addition in this PR, so just these changes are good.
| } | ||
| }); | ||
| await findAndKillProcess('holochain') | ||
| await findAndKillProcess('lair-keystore') |
Member
There was a problem hiding this comment.
that is also super old. we are not spawning holochain (no lair-keystore) as external processes. all inside the ad4m-executor. these lines should go away.
Member
There was a problem hiding this comment.
@HexaField I will merge this now, but please note my comments above for future work on the test-runner.
HexaField
added a commit
to HexaField/ad4m
that referenced
this pull request
Feb 19, 2026
…and perspect3vism refs
- Replace findAndKillProcess('holochain') and findAndKillProcess('lair-keystore')
with findAndKillProcess('ad4m') — everything is a single ad4m-executor process
since the Rust refactor
- Remove stale --ipfsPort flag from spawn args (IPFS removed)
- Remove dead defaultLangPath branch (both branches were identical)
- Update binary download URL from perspect3vism to coasys
- Update README and example package.json URLs from perspect3vism to coasys
- Remove unused get-port import from cli.ts
Ref: coasys#678 (review)
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
2 participants
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.
Summary
The test runner (
@coasys/ad4m-test) previously downloaded pre-built system language bundles fromperspect3vismrepos duringpostinstall. These bundles were CJS and required conversion to ESM for the executor's Deno runtime. As discussed with @lucksus, these bundles are stale and no longer maintained.Changes
This PR simplifies the test runner to use the bootstrap seed (
tests/js/bootstrapSeed.json) which already contains:At runtime, the language-language fetches other system languages by hash from the bootstrap store (
https://bootstrap-store-gateway.perspect3vism.workers.dev).Specific changes:
installSystemLanguages.ts— simplified to just copy the bootstrap seed from the repoget-builtin-test-langs.js(postinstall) — replaced with no-op (bundles fetched at runtime)cli.ts— always boot with--language-language-only false(language-language handles everything)package.json— removed unused deps (node-wget-js,unzipper,@types/unzipper)What's removed:
What stays:
tests/js/bootstrapSeed.json(maintained in this repo)Context
Follow-up to #670. Per conversation with @lucksus: the perspect3vism bundle repos are abandoned, and the correct approach is to use the bootstrap seed which the language-language uses to fetch system languages from Cloudflare.
cc @lucksus @data-bot-coasys
Summary by CodeRabbit
Chores
Bug Fixes