fix: restore server version flag#1322
Conversation
|
Claude (@claude) review |
|
Greptile (@greptileai) review |
|
Claude finished Nikhil (@shadowfax92)'s task in 1m 52s —— View job Code Review
Overall: Clean architectural improvement. Replacing Issues
|
✅ Tests passed — 1393/1397
|
Greptile SummaryThis PR fixes
Confidence Score: 4/5The change is focused and well-tested; the main risk is the shift from a Bun runtime-enforced addon block to a JS-level dlopen patch, but the new test suite explicitly verifies both the guard and the absence of extracted .node files. The bootstrap approach is clean and the tests cover the two critical properties (idempotency and no temp-file extraction). The one weak spot is the tautological constant test in native-addon-policy.test.ts, which would not catch the compile step silently using a different entrypoint. packages/browseros-agent/scripts/build/server/native-addon-policy.test.ts — the first test is a constant-equality check that provides little coverage of the actual bundling behaviour. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph OLD["Old boot path (--compile-exec-argv=--no-addons)"]
A1["bun build --compile index.ts\n--compile-exec-argv=--no-addons"] --> B1["Compiled binary"]
B1 -->|"argv: binary -- --version"| C1["isCompiledVersionRequest() → true\nprint VERSION, exit 0"]
B1 -->|"argv: binary"| D1["loadServerConfig() → Application.start()"]
B1 -.->|"argv: binary --version\n(consumed by Bun runtime)"| E1["Bun prints its own version ❌"]
end
subgraph NEW["New boot path (JS-level guard)"]
A2["bun build --compile compiled-bootstrap.ts"] --> B2["Compiled binary"]
B2 -->|"any argv"| C2["installNativeAddonGuard()\npatches process.dlopen"]
C2 --> D2["await import('./index')"]
D2 -->|"argv: binary --version"| E2["commander handles --version ✅"]
D2 -->|"argv: binary"| F2["loadServerConfig() → Application.start()"]
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
subgraph OLD["Old boot path (--compile-exec-argv=--no-addons)"]
A1["bun build --compile index.ts\n--compile-exec-argv=--no-addons"] --> B1["Compiled binary"]
B1 -->|"argv: binary -- --version"| C1["isCompiledVersionRequest() → true\nprint VERSION, exit 0"]
B1 -->|"argv: binary"| D1["loadServerConfig() → Application.start()"]
B1 -.->|"argv: binary --version\n(consumed by Bun runtime)"| E1["Bun prints its own version ❌"]
end
subgraph NEW["New boot path (JS-level guard)"]
A2["bun build --compile compiled-bootstrap.ts"] --> B2["Compiled binary"]
B2 -->|"any argv"| C2["installNativeAddonGuard()\npatches process.dlopen"]
C2 --> D2["await import('./index')"]
D2 -->|"argv: binary --version"| E2["commander handles --version ✅"]
D2 -->|"argv: binary"| F2["loadServerConfig() → Application.start()"]
end
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/browseros-agent/scripts/build/server/native-addon-policy.test.ts:23-27
**Tautological constant test**
This test asserts that `SERVER_BUNDLE_ENTRYPOINT` equals the literal string `'apps/server/src/compiled-bootstrap.ts'`, but that value is the constant itself — the test can only fail if someone changes the constant and then edits this assertion to match. A more meaningful check would verify that the file referenced by the constant actually exists on disk, or that the bundle step picks it up as an entry point.
Reviews (1): Last reviewed commit: "chore: register compiled server entrypoi..." | Re-trigger Greptile |
Greptile SummaryThis PR replaces Bun's
Confidence Score: 4/5Safe to merge; the core logic change is small, well-tested, and the only finding is a test-path robustness issue. The approach of patching process.dlopen is verified end-to-end by the updated test suite, and the --version restoration is confirmed by both the build test and Commander's own .version() handler. The one rough edge is that native-addon-policy.test.ts constructs the guard's file path with process.cwd() rather than import.meta.dir, which would silently produce wrong paths if the test runner's working directory is not packages/browseros-agent. packages/browseros-agent/scripts/build/server/native-addon-policy.test.ts — the process.cwd()-based path construction. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant OS as OS / Updater
participant Boot as compiled-bootstrap.ts
participant Guard as native-addon-guard.ts
participant Index as index.ts
participant Config as config.ts (Commander)
OS->>Boot: "execve browseros_server [--version | --help | (none)]"
Boot->>Guard: installNativeAddonGuard()
Guard-->>Boot: process.dlopen patched (throws on call)
Boot->>Index: await import('./index')
Index->>Config: loadServerConfig() → parseCliArgs(argv)
alt --version flag
Config-->>OS: print VERSION, exit 0
else normal start
Config-->>Index: ConfigResult OK
Index->>Index: new Application(config).start()
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant OS as OS / Updater
participant Boot as compiled-bootstrap.ts
participant Guard as native-addon-guard.ts
participant Index as index.ts
participant Config as config.ts (Commander)
OS->>Boot: "execve browseros_server [--version | --help | (none)]"
Boot->>Guard: installNativeAddonGuard()
Guard-->>Boot: process.dlopen patched (throws on call)
Boot->>Index: await import('./index')
Index->>Config: loadServerConfig() → parseCliArgs(argv)
alt --version flag
Config-->>OS: print VERSION, exit 0
else normal start
Config-->>Index: ConfigResult OK
Index->>Index: new Application(config).start()
end
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/browseros-agent/scripts/build/server/native-addon-policy.test.ts:8-11
Prefer `import.meta.dir` over `process.cwd()` so the path resolves correctly regardless of which working directory the test runner is launched from. If `bun test` is invoked from the repo root, `process.cwd()` won't point to `packages/browseros-agent` and every subprocess-spawning test in this file will fail with a module-not-found error.
```suggestion
const nativeAddonGuardPath = join(
import.meta.dir,
'../../../apps/server/src/lib/native-addon-guard.ts',
)
```
Reviews (2): Last reviewed commit: "chore: register compiled server entrypoi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const nativeAddonGuardPath = join( | ||
| process.cwd(), | ||
| 'apps/server/src/lib/native-addon-guard.ts', | ||
| ) |
There was a problem hiding this comment.
import.meta.dir over process.cwd() so the path resolves correctly regardless of which working directory the test runner is launched from. If bun test is invoked from the repo root, process.cwd() won't point to packages/browseros-agent and every subprocess-spawning test in this file will fail with a module-not-found error.
| const nativeAddonGuardPath = join( | |
| process.cwd(), | |
| 'apps/server/src/lib/native-addon-guard.ts', | |
| ) | |
| const nativeAddonGuardPath = join( | |
| import.meta.dir, | |
| '../../../apps/server/src/lib/native-addon-guard.ts', | |
| ) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros-agent/scripts/build/server/native-addon-policy.test.ts
Line: 8-11
Comment:
Prefer `import.meta.dir` over `process.cwd()` so the path resolves correctly regardless of which working directory the test runner is launched from. If `bun test` is invoked from the repo root, `process.cwd()` won't point to `packages/browseros-agent` and every subprocess-spawning test in this file will fail with a module-not-found error.
```suggestion
const nativeAddonGuardPath = join(
import.meta.dir,
'../../../apps/server/src/lib/native-addon-guard.ts',
)
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Summary
browseros_server --versionhandling for compiled production binariesTest plan
bun test scripts/build/server/native-addon-policy.test.tsbun test apps/server/tests/build.test.tsbun run build:server:test./dist/prod/server/darwin-arm64/resources/bin/browseros_server --version./dist/prod/server/darwin-arm64/resources/bin/browseros_server./dist/prod/server/darwin-arm64/resources/bin/browseros_server --helpbun run checkbun run test:mainbun run test