Mcp version#2
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a Gemini CLI extension for BrowserNERD: release CI, packaging/install tooling, a Node CLI wrapper, gemini config and mangle facts, command prompts and hooks, skill docs, many tests (unit, smoke, e2e), and supporting scripts and manifests. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer (push tag)
participant GH as GitHub Actions
participant Runner as Runner (build host)
participant Go as Go toolchain (1.21)
participant Node as Node/npm (20)
participant GHRel as GitHub Release
Dev->>GH: push tag matching "v*"
GH->>Runner: trigger workflow
Runner->>Go: setup Go 1.21 & build cross-platform binaries
Go-->>Runner: produce binaries into `bin/`
Runner->>Node: setup Node 20, install deps (if package-lock.json)
Node-->>Runner: package npm artifacts per platform
Runner->>GHRel: create Release, upload built artifacts and notes
GHRel-->>Dev: publish release with artifacts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 426ffddee9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const prebuiltName = os === 'win32' | ||
| ? `browsernerd-windows-${arch}.exe` | ||
| : `browsernerd-${os}-${arch}`; |
There was a problem hiding this comment.
Use the release artifact naming convention here, because process.arch returns x64 on most desktop environments while the release workflow builds *-amd64 binaries (.github/workflows/release.yml lines 35-38). With the current string interpolation, x86_64 users look for files like browsernerd-linux-x64/browsernerd-windows-x64.exe, fail to find prebuilt binaries, and unnecessarily fall back to go build, which breaks installation on systems that do not have Go installed.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (2)
hooks/after-tool-call.js (1)
7-27: Missingtry/catchin the async hook — unhandled exceptions will surface to the Gemini CLI.
JSON.stringifycan throw on circular references, andcontext.result.content.push(...)can throw ifcontentis frozen or sealed. Wrapping the body in atry/catchensures a faulty hook never breaks the calling tool pipeline.🔧 Proposed fix
export default async function afterToolCall(context) { if (!context || !context.tool || !context.result) return context; + try { // We only care about tracking failures in active interactions if (context.tool.name === 'browser-act' || context.tool.name === 'browser-reason') { ... } + } catch (e) { + console.warn('[BrowserNERD hook] afterToolCall error (ignored):', e?.message); + } return context; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/after-tool-call.js` around lines 7 - 27, The async hook afterToolCall lacks error handling and can crash the caller; wrap the body of afterToolCall in a try/catch so JSON.stringify(context.result) and the mutation of context.result.content are guarded, catch and log the error (e.g., console.error) and return the original context on failure; additionally, before pushing to context.result.content verify Array.isArray(context.result.content) and that the array is writable (or fallback to replacing content with a new array or appending a safe text field) to avoid exceptions when content is frozen/sealed..github/workflows/release.yml (1)
31-31:mkdir -p releasecreates an unused directory.Artifacts are built into
bin/(lines 35–38) and uploaded frombin/(lines 44–48). Therelease/directory is created but never populated or referenced, making this dead code.🔧 Proposed fix
- mkdir -p release cd mcp-server🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml at line 31, Remove the dead directory creation: delete the "mkdir -p release" step since artifacts are built into and uploaded from "bin/"; ensure the workflow only creates/populates the "bin/" directory (or if you intended to use "release/" instead, change the build and upload steps to target "release/") and keep artifact upload steps (the actions referencing "bin/") consistent with that choice.
🤖 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/release.yml:
- Line 27: The workflow currently silences all npm ci failures via "run: npm ci
|| true"; remove that and make the step conditional on the presence of a
lockfile so genuine errors aren't hidden: replace the "run: npm ci || true" line
with a small shell snippet that checks for package-lock.json (or
npm-shrinkwrap.json if used) and runs "npm ci" only when present, otherwise runs
"npm install" (or fails the job explicitly), ensuring you no longer swallow
corrupt-lockfile or network errors.
- Line 19: CI uses Go 1.21 which is EOL; update the go-version key in the GitHub
Actions release workflow by changing the go-version: '1.21' entry to at least
'1.25' (preferably '1.26') so the workflow uses a supported Go toolchain; locate
the go-version field in .github/workflows/release.yml and replace the version
string accordingly, then run the workflow locally or via a dry run to verify
compatibility.
In `@bin/cli.js`:
- Around line 19-26: The exit handler currently swallows signal-based
terminations by only using (code) and calling process.exit(code || 0); change
the child.on('exit') callback to accept (code, signal) and handle both cases: if
signal is non-null, propagate the same signal to the current process (e.g.,
process.kill(process.pid, signal)) so the parent reflects the child's signal
termination; otherwise call process.exit(code || 0). Update the child.on('exit',
...) handler to use (code, signal) and implement this conditional propagation.
In `@commands/browser/init.toml`:
- Line 4: The shell snippet that writes the YAML uses echo with literal "\n"
which produces a single-line malformed YAML; replace the echo invocation with a
POSIX-safe printf call that supplies the YAML as a format string containing
explicit "\n" sequences and redirect that output into .browsernerd/config.yaml
(keep the existing mkdir -p and && chaining), ensuring the format string is
quoted so newlines are emitted correctly and no shell-specific echo flags are
relied upon.
In `@commands/browser/look.toml`:
- Line 9: The template line "3. {{args}}" in commands/browser/look.toml can pass
a literal empty token when no args are supplied; change it to provide a fallback
default so the model gets a meaningful prompt (e.g., replace the bare variable
with a default form such as {{args:-What other information can you infer from
this screenshot?}}) or update the template documentation to require/describe the
expected invocation; target the "3. {{args}}" entry in
commands/browser/look.toml (or the template variable named args) and implement
the default or doc change accordingly.
In `@GEMINI.md`:
- Around line 1-46: The fenced code block containing examples that use //
comments (examples including the "operations" array and operation types like
"session_create", "await_stable", "click", "type", "fill_form") is marked as
```json which makes the inline comments invalid; change the opening fence to
```jsonc for that block so the JSON-with-comments examples are correctly marked
and rendered (update the code fence surrounding the block that includes the
sample with {"operations": [...]}, not the snippet contents).
In `@hooks/after-tool-call.js`:
- Around line 12-13: The check that uses output = JSON.stringify(context.result)
and if (output.includes('crash') || output.includes('"status":"error"') ||
output.includes('"type":"Error"')) is causing false positives; instead parse
context.result (wrap JSON.parse in try/catch if necessary) and inspect known
error-shaped properties (e.g., result.status === 'error', result.type ===
'Error', result.error?.message or result.error?.type) and only look for the
substring "crash" inside those error-specific fields (like error.message) rather
than calling output.includes('crash') on the entire serialized result; update
the conditional in hooks/after-tool-call.js to use the parsed object and these
targeted checks, with a safe fallback to the previous behavior only if parsing
fails.
In `@mcp-server/gemini-config.yaml`:
- Around line 1-28: Replace the empty-string default in the YAML config for
browser.launch (currently [""]) with an empty list or remove the browser.launch
key entirely so callers don't try to execute an empty command; update any code
that expects browser.launch to handle a missing or empty-array value (e.g.,
treat browser.launch === [] or undefined as "no custom launcher
configured")—refer to the browser.launch setting in the config and any
config-parsing logic that reads it.
In `@mcp-server/schemas/browser.mg`:
- Around line 1051-1055: Add missing Decl statements for agent_client/1 and
triage_hint/1 and correct the caused_by arity so the body uses the declared
caused_by/3; specifically, add a Decl for agent_client("gemini_cli"), add a Decl
for triage_hint(Action), and change the triage_hint body to call
caused_by(SessionId, ConsoleMessage, ReqId) (or otherwise pass three arguments
to caused_by) so the predicate identities match the existing Decl
caused_by(SessionId, ConsoleMessage, RequestId) and triage_hint becomes
queryable.
- Line 1050: Replace the invalid C-style comment "// --- GEMINI CLI INTEGRATION
---" with a Mangle comment using "#" and add missing predicate declarations for
agent_client and triage_hint (include their correct arities, e.g., Decl
agent_client/1 or /2 and Decl triage_hint/1 or /2 as appropriate for their use
in the file); also resolve the caused_by arity mismatch by making the rule call
match the declared predicate signature (either change the call to
caused_by(SessionId, ConsoleMessage, RequestId) by supplying the missing
SessionId/ConsoleMessage variables where this rule invokes caused_by, or if the
rule truly only has two args, update the declaration to Decl caused_by/2) so the
declaration for caused_by and its usages have the same arity.
In `@scripts/install.js`:
- Around line 44-49: The catch block in scripts/install.js that handles
build/rename failures currently only logs warnings; modify the catch to set a
non-zero exit status so npm/CI detect failure by assigning process.exitCode = 1
(or calling process.exit(1)) before returning/finishing; locate the try/catch
around the build/rename logic in install.js and add the exit-code assignment
inside the existing catch (the block that logs "BrowserNERD compilation failed
or Go is not installed.") so the script fails correctly when the binary is
missing.
In `@skills/browser-debugging/SKILL.md`:
- Line 14: Step 4 currently refers to the tool name `browser-mangle` but the MCP
server registers the Mangle database tool as `n`; update the step to replace the
`browser-mangle` reference with `n` so the tool name matches the registered
identifier (ensure the sentence in SKILL.md that mentions `browser-mangle` now
reads `n` to align with other steps like the React check using
`browser-observe`).
---
Nitpick comments:
In @.github/workflows/release.yml:
- Line 31: Remove the dead directory creation: delete the "mkdir -p release"
step since artifacts are built into and uploaded from "bin/"; ensure the
workflow only creates/populates the "bin/" directory (or if you intended to use
"release/" instead, change the build and upload steps to target "release/") and
keep artifact upload steps (the actions referencing "bin/") consistent with that
choice.
In `@hooks/after-tool-call.js`:
- Around line 7-27: The async hook afterToolCall lacks error handling and can
crash the caller; wrap the body of afterToolCall in a try/catch so
JSON.stringify(context.result) and the mutation of context.result.content are
guarded, catch and log the error (e.g., console.error) and return the original
context on failure; additionally, before pushing to context.result.content
verify Array.isArray(context.result.content) and that the array is writable (or
fallback to replacing content with a new array or appending a safe text field)
to avoid exceptions when content is frozen/sealed.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.github/workflows/release.ymlGEMINI.mdbin/cli.jscommands/browser/init.tomlcommands/browser/launch.tomlcommands/browser/look.tomlgemini-extension.jsonhooks/after-tool-call.jsmcp-server/gemini-config.yamlmcp-server/schemas/browser.mgpackage.jsonscripts/install.jsskills/browser-debugging/SKILL.mdskills/e2e-test-engineer/SKILL.mdskills/form-filler/SKILL.md
| - name: Set up Go | ||
| uses: actions/setup-go@v5 | ||
| with: | ||
| go-version: '1.21' |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Go latest stable version 2026 Go 1.22 1.23 1.24 release support
💡 Result:
As of Feb 23, 2026, the latest stable Go version is Go 1.26.0. [1][2]
Support status (Go 1.22–1.26)
Go’s release policy is: each major Go release is supported until there are two newer major releases (with critical bug/security fixes delivered as minor revisions while supported). [3]
That means today:
- Supported: Go 1.26 (released Feb 10, 2026) and Go 1.25 (released Aug 12, 2025) [2][4][5]
- Not supported (EOL): Go 1.24 (released Feb 11, 2025), Go 1.23 (released Aug 13, 2024), Go 1.22 (released Feb 6, 2024) [3][6][7]
Next major release
Go releases are on a ~6-month cadence, and Go 1.27 is expected in August 2026. [8]
Sources:
[1] https://go.dev/VERSION
[2] https://go.dev/blog/go1.26
[3] https://go.dev/doc/devel/release
[4] https://go.dev/blog/go1.25
[5] https://tip.golang.org/doc/go1.26
[6] https://go.dev/blog/go1.24
[7] https://go.dev/blog/go1.23
[8] https://tip.golang.org/doc/next
Update Go version to 1.25 or 1.26; Go 1.21 is end-of-life.
Go 1.21 (released late 2023) is no longer supported as of February 2026. The current supported versions are Go 1.26 (released Feb 2026) and Go 1.25 (released Aug 2025). EOL versions do not receive security patches or bug fixes. Use at least Go 1.25.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yml at line 19, CI uses Go 1.21 which is EOL;
update the go-version key in the GitHub Actions release workflow by changing the
go-version: '1.21' entry to at least '1.25' (preferably '1.26') so the workflow
uses a supported Go toolchain; locate the go-version field in
.github/workflows/release.yml and replace the version string accordingly, then
run the workflow locally or via a dry run to verify compatibility.
| node-version: '20' | ||
|
|
||
| - name: Install dependencies | ||
| run: npm ci || true # ignore if no package-lock.json |
There was a problem hiding this comment.
|| true silences all npm ci failures, not just a missing lockfile.
A corrupt package-lock.json, network error, or version conflict will be silently swallowed and the build will continue against an incomplete or absent node_modules.
🔧 Proposed fix
- run: npm ci || true # ignore if no package-lock.json
+ run: |
+ if [ -f package-lock.json ]; then npm ci; fi📝 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.
| run: npm ci || true # ignore if no package-lock.json | |
| run: | | |
| if [ -f package-lock.json ]; then npm ci; fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yml at line 27, The workflow currently silences
all npm ci failures via "run: npm ci || true"; remove that and make the step
conditional on the presence of a lockfile so genuine errors aren't hidden:
replace the "run: npm ci || true" line with a small shell snippet that checks
for package-lock.json (or npm-shrinkwrap.json if used) and runs "npm ci" only
when present, otherwise runs "npm install" (or fails the job explicitly),
ensuring you no longer swallow corrupt-lockfile or network errors.
| child.on('error', (err) => { | ||
| console.error('Failed to start BrowserNERD process:', err); | ||
| process.exit(1); | ||
| }); | ||
|
|
||
| child.on('exit', (code) => { | ||
| process.exit(code || 0); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine bin/cli.js
fd "cli.js" | head -5Repository: theRebelliousNerd/browserNerd
Length of output: 83
🏁 Script executed:
# Read the specific lines mentioned in the review
cat -n bin/cli.js | sed -n '15,30p'Repository: theRebelliousNerd/browserNerd
Length of output: 476
🌐 Web query:
Node.js child process exit event signal null code semantics
💡 Result:
In Node.js, the ChildProcess 'exit' event is called as:
child.on('exit', (code, signal) => { /* ... */ });Semantics:
codeis the child’s numeric exit status only if it exited “normally” (e.g., returned frommain, calledprocess.exit(n)). Otherwise it isnull. Acodeof0conventionally means success; non‑zero usually indicates an error. [1]signalis the signal name (string like'SIGTERM','SIGKILL') only if the OS terminated the child due to a signal. Otherwise it isnull. [1]- Exactly one of
codeorsignalwill be non‑nullfor'exit'. [1]
Related gotcha:
'exit'can fire before the child’s stdio streams are fully closed; if you need to wait for stdio to finish, use the'close'event instead (it has the samecode/signalnull rules). [1]
Sources: Node.js child_process documentation for 'exit' / 'close'. [1]
[1] (nodejs.org)
Citations:
Propagate child termination signals.
process.exit(code || 0) masks signal-based terminations as success; when the child exits due to a signal, code is null, so null || 0 evaluates to 0. Handle both exit codes and signals explicitly using (code, signal) to preserve failure semantics.
🔧 Proposed fix
-child.on('exit', (code) => {
- process.exit(code || 0);
-});
+child.on('exit', (code, signal) => {
+ if (code !== null) {
+ process.exit(code);
+ }
+ if (signal) {
+ process.kill(process.pid, signal);
+ return;
+ }
+ process.exit(1);
+});📝 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.
| child.on('error', (err) => { | |
| console.error('Failed to start BrowserNERD process:', err); | |
| process.exit(1); | |
| }); | |
| child.on('exit', (code) => { | |
| process.exit(code || 0); | |
| }); | |
| child.on('error', (err) => { | |
| console.error('Failed to start BrowserNERD process:', err); | |
| process.exit(1); | |
| }); | |
| child.on('exit', (code, signal) => { | |
| if (code !== null) { | |
| process.exit(code); | |
| } | |
| if (signal) { | |
| process.kill(process.pid, signal); | |
| return; | |
| } | |
| process.exit(1); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/cli.js` around lines 19 - 26, The exit handler currently swallows
signal-based terminations by only using (code) and calling process.exit(code ||
0); change the child.on('exit') callback to accept (code, signal) and handle
both cases: if signal is non-null, propagate the same signal to the current
process (e.g., process.kill(process.pid, signal)) so the parent reflects the
child's signal termination; otherwise call process.exit(code || 0). Update the
child.on('exit', ...) handler to use (code, signal) and implement this
conditional propagation.
| prompt = """Initialize a new BrowserNERD workspace. | ||
|
|
||
| I am executing a shell command to initialize the `.browsernerd` configuration. | ||
| !{mkdir -p .browsernerd && echo "browser:\n headless: false\n viewport_width: 1280\n viewport_height: 720\n\ndocker:\n enabled: false\n log_window: 60s\n" > .browsernerd/config.yaml} |
There was a problem hiding this comment.
echo does not expand \n in double quotes on POSIX sh/bash, producing a malformed single-line YAML file.
Without the -e flag, echo "foo\nbar" writes the literal five-character string foo\nbar to stdout. The resulting config.yaml will be one flat line with literal \n characters, which is invalid YAML that tools consuming it (including the BrowserNERD server) will fail to parse.
Use printf, which reliably interprets \n format specifiers across all POSIX-compliant shells:
🔧 Proposed fix
-!{mkdir -p .browsernerd && echo "browser:\n headless: false\n viewport_width: 1280\n viewport_height: 720\n\ndocker:\n enabled: false\n log_window: 60s\n" > .browsernerd/config.yaml}
+!{mkdir -p .browsernerd && printf "browser:\n headless: false\n viewport_width: 1280\n viewport_height: 720\n\ndocker:\n enabled: false\n log_window: 60s\n" > .browsernerd/config.yaml}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@commands/browser/init.toml` at line 4, The shell snippet that writes the YAML
uses echo with literal "\n" which produces a single-line malformed YAML; replace
the echo invocation with a POSIX-safe printf call that supplies the YAML as a
format string containing explicit "\n" sequences and redirect that output into
.browsernerd/config.yaml (keep the existing mkdir -p and && chaining), ensuring
the format string is quoted so newlines are emitted correctly and no
shell-specific echo flags are relied upon.
| Based on this screenshot and the current context, please tell me: | ||
| 1. Are there any obvious CSS/layout issues? | ||
| 2. What are the primary calls to action (CTAs)? | ||
| 3. {{args}} |
There was a problem hiding this comment.
{{args}} in question 3 is left bare with no default.
If the user invokes /browser:look with no arguments, the model receives 3. {{args}} literally. Consider providing a fallback default (e.g. {{args:-What other information can you infer from this screenshot?}}) or documenting the expected invocation pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@commands/browser/look.toml` at line 9, The template line "3. {{args}}" in
commands/browser/look.toml can pass a literal empty token when no args are
supplied; change it to provide a fallback default so the model gets a meaningful
prompt (e.g., replace the bare variable with a default form such as {{args:-What
other information can you infer from this screenshot?}}) or update the template
documentation to require/describe the expected invocation; target the "3.
{{args}}" entry in commands/browser/look.toml (or the template variable named
args) and implement the default or doc change accordingly.
| server: | ||
| name: browsernerd-mcp | ||
| version: 0.0.8 | ||
| log_file: data/browsernerd-mcp.log | ||
|
|
||
| browser: | ||
| auto_start: false | ||
| launch: [""] | ||
| default_navigation_timeout: 15s | ||
| default_attach_timeout: 10s | ||
| session_store: data/sessions.json | ||
| enable_dom_ingestion: true | ||
| enable_header_ingestion: true | ||
|
|
||
| mcp: | ||
| sse_port: 0 | ||
| progressive_only: true | ||
|
|
||
| mangle: | ||
| enable: true | ||
| schema_path: schemas/browser.mg | ||
| disable_builtin_rules: false | ||
| fact_buffer_limit: 2048 | ||
|
|
||
| recorder: | ||
| enabled: true | ||
| trace_dir: data/traces | ||
| max_rotated_files: 3 |
There was a problem hiding this comment.
Avoid empty launch command defaults.
browser.launch is [""] (Line 8); if the launcher reads the first entry, it may attempt to execute an empty command. Prefer an empty list or omit the field.
🔧 Proposed fix
- launch: [""]
+ launch: []📝 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.
| server: | |
| name: browsernerd-mcp | |
| version: 0.0.8 | |
| log_file: data/browsernerd-mcp.log | |
| browser: | |
| auto_start: false | |
| launch: [""] | |
| default_navigation_timeout: 15s | |
| default_attach_timeout: 10s | |
| session_store: data/sessions.json | |
| enable_dom_ingestion: true | |
| enable_header_ingestion: true | |
| mcp: | |
| sse_port: 0 | |
| progressive_only: true | |
| mangle: | |
| enable: true | |
| schema_path: schemas/browser.mg | |
| disable_builtin_rules: false | |
| fact_buffer_limit: 2048 | |
| recorder: | |
| enabled: true | |
| trace_dir: data/traces | |
| max_rotated_files: 3 | |
| server: | |
| name: browsernerd-mcp | |
| version: 0.0.8 | |
| log_file: data/browsernerd-mcp.log | |
| browser: | |
| auto_start: false | |
| launch: [] | |
| default_navigation_timeout: 15s | |
| default_attach_timeout: 10s | |
| session_store: data/sessions.json | |
| enable_dom_ingestion: true | |
| enable_header_ingestion: true | |
| mcp: | |
| sse_port: 0 | |
| progressive_only: true | |
| mangle: | |
| enable: true | |
| schema_path: schemas/browser.mg | |
| disable_builtin_rules: false | |
| fact_buffer_limit: 2048 | |
| recorder: | |
| enabled: true | |
| trace_dir: data/traces | |
| max_rotated_files: 3 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcp-server/gemini-config.yaml` around lines 1 - 28, Replace the empty-string
default in the YAML config for browser.launch (currently [""]) with an empty
list or remove the browser.launch key entirely so callers don't try to execute
an empty command; update any code that expects browser.launch to handle a
missing or empty-array value (e.g., treat browser.launch === [] or undefined as
"no custom launcher configured")—refer to the browser.launch setting in the
config and any config-parsing logic that reads it.
| } catch (e) { | ||
| console.warn('\n======================================================'); | ||
| console.warn('⚠️ WARNING: BrowserNERD compilation failed or Go is not installed.'); | ||
| console.warn('To use this extension, please ensure Go 1.21+ is installed.'); | ||
| console.warn('Then manually run `go build` inside the mcp-server/ directory.'); | ||
| console.warn('======================================================\n'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n scripts/install.js | head -60Repository: theRebelliousNerd/browserNerd
Length of output: 2327
🏁 Script executed:
cat -n package.json | grep -A 10 -B 2 "scripts"Repository: theRebelliousNerd/browserNerd
Length of output: 340
🏁 Script executed:
# Check if there are any other error handling mechanisms or exit calls after line 50
tail -n +50 scripts/install.jsRepository: theRebelliousNerd/browserNerd
Length of output: 74
Fail the install step when build/rename fails.
The catch block (lines 44–50) logs warnings for compilation or Go errors but doesn't set a non-zero exit code. Since the script is invoked as the postinstall and build hook in package.json, a silent exit with status 0 causes npm and CI systems to report success even when the binary is missing.
Add process.exitCode = 1; to the catch block so failures are properly detected:
🔧 Proposed fix
} catch (e) {
console.warn('\n======================================================');
console.warn('⚠️ WARNING: BrowserNERD compilation failed or Go is not installed.');
console.warn('To use this extension, please ensure Go 1.21+ is installed.');
console.warn('Then manually run `go build` inside the mcp-server/ directory.');
console.warn('======================================================\n');
+ process.exitCode = 1;
}📝 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.
| } catch (e) { | |
| console.warn('\n======================================================'); | |
| console.warn('⚠️ WARNING: BrowserNERD compilation failed or Go is not installed.'); | |
| console.warn('To use this extension, please ensure Go 1.21+ is installed.'); | |
| console.warn('Then manually run `go build` inside the mcp-server/ directory.'); | |
| console.warn('======================================================\n'); | |
| } catch (e) { | |
| console.warn('\n======================================================'); | |
| console.warn('⚠️ WARNING: BrowserNERD compilation failed or Go is not installed.'); | |
| console.warn('To use this extension, please ensure Go 1.21+ is installed.'); | |
| console.warn('Then manually run `go build` inside the mcp-server/ directory.'); | |
| console.warn('======================================================\n'); | |
| process.exitCode = 1; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/install.js` around lines 44 - 49, The catch block in
scripts/install.js that handles build/rename failures currently only logs
warnings; modify the catch to set a non-zero exit status so npm/CI detect
failure by assigning process.exitCode = 1 (or calling process.exit(1)) before
returning/finishing; locate the try/catch around the build/rename logic in
install.js and add the exit-code assignment inside the existing catch (the block
that logs "BrowserNERD compilation failed or Go is not installed.") so the
script fails correctly when the binary is missing.
| 2. If the user complains about a crash, use `browser-reason` with `topic="why_failed"` and `since_navigation=true`. | ||
| 3. If an element isn't interactable or a click fails, use `browser-reason` with `topic="blocking_issue"`. | ||
| 4. Inspect network errors by looking at the Mangle facts provided in `browser-mangle`. | ||
| 5. Check React state using `browser-observe` with `mode: "react"`. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -rn "browser-mangle|browser_mangle|browserMangle" --type goRepository: theRebelliousNerd/browserNerd
Length of output: 1688
🏁 Script executed:
sed -n '10,20p' skills/browser-debugging/SKILL.mdRepository: theRebelliousNerd/browserNerd
Length of output: 722
Replace browser-mangle with the correct tool name n on line 13.
The MCP server exposes the Mangle database tool as n (not browser-mangle). Step 4 should reference n for network error inspection to match the registered tool names used elsewhere in the system.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/browser-debugging/SKILL.md` at line 14, Step 4 currently refers to the
tool name `browser-mangle` but the MCP server registers the Mangle database tool
as `n`; update the step to replace the `browser-mangle` reference with `n` so
the tool name matches the registered identifier (ensure the sentence in SKILL.md
that mentions `browser-mangle` now reads `n` to align with other steps like the
React check using `browser-observe`).
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
scripts/package.sh (2)
51-58: Use absolute paths instead ofcd+ relative../../offsets; verifyzipavailability.The
cdinto the release directory and then../../release/offset works only when the script is run from the repo root. Capturing the working directory as an absolute path is more robust and avoids silent failures if the script is ever invoked from a different directory.
zipis also not guaranteed to be installed on all Linux hosts. Prefertaror add an explicit availability check before using it.♻️ Proposed fix — absolute paths + zip availability check
+REPO_ROOT="$(pwd)" + mkdir -p release -cd "release_build/${PLATFORM}.${ARCH}.${NAME}" if [ "$PLATFORM" = "win32" ]; then - zip -r "../../release/${PLATFORM}.${ARCH}.${NAME}.zip" ./* + if ! command -v zip &>/dev/null; then + echo "ERROR: 'zip' is not installed. Cannot package for win32." >&2 + exit 1 + fi + (cd "${REPO_ROOT}/release_build/${PLATFORM}.${ARCH}.${NAME}" && \ + zip -r "${REPO_ROOT}/release/${PLATFORM}.${ARCH}.${NAME}.zip" ./*) else - tar -czvf "../../release/${PLATFORM}.${ARCH}.${NAME}.tar.gz" ./* + (cd "${REPO_ROOT}/release_build/${PLATFORM}.${ARCH}.${NAME}" && \ + tar -czvf "${REPO_ROOT}/release/${PLATFORM}.${ARCH}.${NAME}.tar.gz" ./*) fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/package.sh` around lines 51 - 58, Replace the fragile cd+relative paths by constructing absolute paths: capture the current/desired repo root into a variable (e.g., REPO_ROOT) and compute BUILD_DIR="$REPO_ROOT/release_build/${PLATFORM}.${ARCH}.${NAME}" and OUT_DIR="$REPO_ROOT/release"; mkdir -p "$BUILD_DIR" and "$OUT_DIR" and use those absolute paths when creating archives instead of cd and ../../ offsets; for the Windows branch check availability of zip (command -v zip) before using it and either fall back to tar or exit with a clear error message if zip is missing; update the packaging commands that reference the archive names using the same variables (PLATFORM/ARCH/NAME) so they operate on BUILD_DIR and OUT_DIR directly.
20-29:set -e+ silentcpfailures give unhelpful errors; inconsistent with warn-and-continue for the binary.If any of the
cpcalls on lines 20–29 fail (missing file/directory),set -eterminates the script immediately with only a generic shell error — no context about which asset was missing. The binary (lines 45–49), by contrast, gets an explicit warning and the script continues. Adding guard messages at the asset-copy stage makes failures actionable.♻️ Proposed fix — explicit existence checks with descriptive errors
+for asset in gemini-extension.json GEMINI.md package.json; do + if [ ! -f "$asset" ]; then + echo "ERROR: Required asset '$asset' not found. Run from the repo root." >&2 + exit 1 + fi +done +for dir in commands hooks skills scripts; do + if [ ! -d "$dir" ]; then + echo "ERROR: Required directory '$dir/' not found. Run from the repo root." >&2 + exit 1 + fi +done + cp gemini-extension.json "$OUT_DIR/" cp GEMINI.md "$OUT_DIR/" cp package.json "$OUT_DIR/" cp -r commands/ "$OUT_DIR/" cp -r hooks/ "$OUT_DIR/" cp -r skills/ "$OUT_DIR/" cp -r scripts/ "$OUT_DIR/"Also applies to: 45-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/package.sh` around lines 20 - 29, The asset-copy block currently uses plain cp under set -e so failures produce vague shell errors; update the cp lines (the various cp/gcp of gemini-extension.json, GEMINI.md, package.json, commands/, hooks/, skills/, scripts/, mcp-server/gemini-config.yaml and the binary section later) to perform explicit existence checks before copying: for required assets test -e or -d and emit a descriptive error message (including the missing filename) and exit non‑zero, and for the optional binary (the mkdir -p + cp bin/cli.js block) mirror the existing warn-and-continue behavior by checking existence and printing a warning if missing instead of letting set -e abort. Ensure messages reference the target asset name so failures are actionable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/package.sh`:
- Around line 4-10: The script currently only checks PLATFORM and ARCH for
emptiness but must validate them against explicit allowlists and prevent path
traversal: add arrays of allowed PLATFORM values (e.g. linux, darwin, win32) and
allowed ARCH values (e.g. x86_64, arm64) and reject any value not present;
additionally reject values containing '/' or '..' (or any path-separator) to
prevent traversal; update the early validation around PLATFORM and ARCH to exit
non-zero with a clear error if validation fails, and ensure all later uses of
these variables (e.g. OUT_DIR construction, and commands referencing OUT_DIR,
and any rm -rf, mkdir -p, cd that use PLATFORM/ARCH/OUT_DIR) are safely quoted
and only use the validated values when building paths.
- Around line 35-36: The script unconditionally maps any ARCH value to
GOARCH="amd64" which can silently bundle the wrong binary; update the ARCH
validation in package.sh so you only accept known architectures (e.g., "amd64"
and "arm64") and set GOARCH accordingly (use ARCH to choose GOARCH mapping), and
if ARCH is not in the allowlist emit a clear warning message (and optionally
exit non‑zero) instead of silently falling back — refer to the GOARCH variable
and the ARCH environment variable in the existing logic to locate where to add
the allowlist check and warning.
- Line 27: The package script currently flattens the config path by copying
mcp-server/gemini-config.yaml directly into $OUT_DIR (cp -r
mcp-server/gemini-config.yaml "$OUT_DIR/"), but gemini-extension.json expects
the file at ${extensionPath}/mcp-server/gemini-config.yaml; update the script to
preserve the mcp-server subdirectory when copying (for example, copy the whole
mcp-server directory into $OUT_DIR or create $OUT_DIR/mcp-server and copy the
file there) so the config path matches what gemini-extension.json (line 12)
expects.
---
Nitpick comments:
In `@scripts/package.sh`:
- Around line 51-58: Replace the fragile cd+relative paths by constructing
absolute paths: capture the current/desired repo root into a variable (e.g.,
REPO_ROOT) and compute
BUILD_DIR="$REPO_ROOT/release_build/${PLATFORM}.${ARCH}.${NAME}" and
OUT_DIR="$REPO_ROOT/release"; mkdir -p "$BUILD_DIR" and "$OUT_DIR" and use those
absolute paths when creating archives instead of cd and ../../ offsets; for the
Windows branch check availability of zip (command -v zip) before using it and
either fall back to tar or exit with a clear error message if zip is missing;
update the packaging commands that reference the archive names using the same
variables (PLATFORM/ARCH/NAME) so they operate on BUILD_DIR and OUT_DIR
directly.
- Around line 20-29: The asset-copy block currently uses plain cp under set -e
so failures produce vague shell errors; update the cp lines (the various cp/gcp
of gemini-extension.json, GEMINI.md, package.json, commands/, hooks/, skills/,
scripts/, mcp-server/gemini-config.yaml and the binary section later) to perform
explicit existence checks before copying: for required assets test -e or -d and
emit a descriptive error message (including the missing filename) and exit
non‑zero, and for the optional binary (the mkdir -p + cp bin/cli.js block)
mirror the existing warn-and-continue behavior by checking existence and
printing a warning if missing instead of letting set -e abort. Ensure messages
reference the target asset name so failures are actionable.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
.github/workflows/release.ymlpackage.jsonscripts/package.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/release.yml
- package.json
| PLATFORM=$1 | ||
| ARCH=$2 | ||
|
|
||
| if [ -z "$PLATFORM" ] || [ -z "$ARCH" ]; then | ||
| echo "Usage: npm run package -- <platform> <arch>" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Validate PLATFORM and ARCH against an allowlist to prevent silent misbehaviour and path traversal.
The check only guards against empty values. An unrecognised value (e.g. x64, aarch64, a typo, or a CI parameter-injection string) will pass through silently, produce a malformed binary path, and potentially result in a release archive that ships the wrong (or no) binary — all without any error.
Additionally, PLATFORM and ARCH are later used unquoted or interpolated in rm -rf "$OUT_DIR", mkdir -p "$OUT_DIR", and cd — making path traversal (e.g. ../../../) exploitable in automated pipelines.
🛡️ Proposed fix — allowlist + traversal guard
-if [ -z "$PLATFORM" ] || [ -z "$ARCH" ]; then
- echo "Usage: npm run package -- <platform> <arch>"
- exit 1
-fi
+VALID_PLATFORMS="win32 darwin linux"
+VALID_ARCHES="x64 arm64 ia32"
+
+if [ -z "$PLATFORM" ] || [ -z "$ARCH" ]; then
+ echo "Usage: npm run package -- <platform> <arch>"
+ exit 1
+fi
+
+if ! echo "$VALID_PLATFORMS" | grep -wq "$PLATFORM"; then
+ echo "Error: unsupported platform '$PLATFORM'. Expected one of: $VALID_PLATFORMS"
+ exit 1
+fi
+
+if ! echo "$VALID_ARCHES" | grep -wq "$ARCH"; then
+ echo "Error: unsupported arch '$ARCH'. Expected one of: $VALID_ARCHES"
+ exit 1
+fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/package.sh` around lines 4 - 10, The script currently only checks
PLATFORM and ARCH for emptiness but must validate them against explicit
allowlists and prevent path traversal: add arrays of allowed PLATFORM values
(e.g. linux, darwin, win32) and allowed ARCH values (e.g. x86_64, arm64) and
reject any value not present; additionally reject values containing '/' or '..'
(or any path-separator) to prevent traversal; update the early validation around
PLATFORM and ARCH to exit non-zero with a clear error if validation fails, and
ensure all later uses of these variables (e.g. OUT_DIR construction, and
commands referencing OUT_DIR, and any rm -rf, mkdir -p, cd that use
PLATFORM/ARCH/OUT_DIR) are safely quoted and only use the validated values when
building paths.
| cp -r hooks/ "$OUT_DIR/" | ||
| cp -r skills/ "$OUT_DIR/" | ||
| cp -r scripts/ "$OUT_DIR/" | ||
| cp -r mcp-server/gemini-config.yaml "$OUT_DIR/" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the package.sh script for context
fd package.sh -type fRepository: theRebelliousNerd/browserNerd
Length of output: 243
🏁 Script executed:
# Find all references to gemini-config.yaml
rg -n "gemini-config.yaml" -A2 -B2Repository: theRebelliousNerd/browserNerd
Length of output: 648
🏁 Script executed:
# Also check for any references to mcp-server directory structure
rg -n "mcp-server/" -A1 -B1 | head -50Repository: theRebelliousNerd/browserNerd
Length of output: 2864
🏁 Script executed:
# Look for any references to how the config is loaded at runtime
rg -n "gemini-config" --type=js --type=ts --type=go -A3 -B1Repository: theRebelliousNerd/browserNerd
Length of output: 55
Path flattening in cp -r breaks the config location expected by the extension.
cp -r mcp-server/gemini-config.yaml "$OUT_DIR/" copies the file to $OUT_DIR/gemini-config.yaml, not $OUT_DIR/mcp-server/gemini-config.yaml. The -r flag has no effect on a single file, but the real issue is that gemini-extension.json explicitly expects the config at ${extensionPath}/mcp-server/gemini-config.yaml (line 12). The packaged release will fail at runtime when the extension tries to load the config.
Preserve the subdirectory:
-cp -r mcp-server/gemini-config.yaml "$OUT_DIR/"
+mkdir -p "$OUT_DIR/mcp-server"
+cp mcp-server/gemini-config.yaml "$OUT_DIR/mcp-server/"Or copy the entire directory if additional files are expected:
-cp -r mcp-server/gemini-config.yaml "$OUT_DIR/"
+cp -r mcp-server/ "$OUT_DIR/"📝 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.
| cp -r mcp-server/gemini-config.yaml "$OUT_DIR/" | |
| mkdir -p "$OUT_DIR/mcp-server" | |
| cp mcp-server/gemini-config.yaml "$OUT_DIR/mcp-server/" |
| cp -r mcp-server/gemini-config.yaml "$OUT_DIR/" | |
| cp -r mcp-server/ "$OUT_DIR/" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/package.sh` at line 27, The package script currently flattens the
config path by copying mcp-server/gemini-config.yaml directly into $OUT_DIR (cp
-r mcp-server/gemini-config.yaml "$OUT_DIR/"), but gemini-extension.json expects
the file at ${extensionPath}/mcp-server/gemini-config.yaml; update the script to
preserve the mcp-server subdirectory when copying (for example, copy the whole
mcp-server directory into $OUT_DIR or create $OUT_DIR/mcp-server and copy the
file there) so the config path matches what gemini-extension.json (line 12)
expects.
| GOARCH="amd64" | ||
| if [ "$ARCH" = "arm64" ]; then GOARCH="arm64"; fi |
There was a problem hiding this comment.
Silent amd64 fallback for unknown arch can silently bundle the wrong binary.
Any arch value that isn't arm64 maps to amd64 with no warning. If a caller passes e.g. ia32, the script picks up the amd64 binary, names the output archive ia32, and exits without any indication that the wrong binary was bundled. Combined with the missing allowlist validation (see above), a misspelled arch causes a silently broken release.
🐛 Proposed fix — emit a warning on unrecognised arch
GOARCH="amd64"
-if [ "$ARCH" = "arm64" ]; then GOARCH="arm64"; fi
+if [ "$ARCH" = "arm64" ]; then
+ GOARCH="arm64"
+elif [ "$ARCH" != "x64" ] && [ "$ARCH" != "ia32" ]; then
+ echo "WARNING: Unrecognised arch '$ARCH', defaulting GOARCH to amd64"
+fi📝 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.
| GOARCH="amd64" | |
| if [ "$ARCH" = "arm64" ]; then GOARCH="arm64"; fi | |
| GOARCH="amd64" | |
| if [ "$ARCH" = "arm64" ]; then | |
| GOARCH="arm64" | |
| elif [ "$ARCH" != "x64" ] && [ "$ARCH" != "ia32" ]; then | |
| echo "WARNING: Unrecognised arch '$ARCH', defaulting GOARCH to amd64" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/package.sh` around lines 35 - 36, The script unconditionally maps any
ARCH value to GOARCH="amd64" which can silently bundle the wrong binary; update
the ARCH validation in package.sh so you only accept known architectures (e.g.,
"amd64" and "arm64") and set GOARCH accordingly (use ARCH to choose GOARCH
mapping), and if ARCH is not in the allowlist emit a clear warning message (and
optionally exit non‑zero) instead of silently falling back — refer to the GOARCH
variable and the ARCH environment variable in the existing logic to locate where
to add the allowlist check and warning.
- Add 6 Node.js test files: extension-structure, skills, commands, hooks, mcp-server-smoke, e2e-lifecycle - Add 6 Go tests: gemini_config_test.go (3), gemini_schema_test.go (3) - Fix Mangle schema browser.mg: use # comments instead of //, proper Decl statements - Update package.json with Jest devDependency, test scripts, and npm metadata - 74/82 Node.js tests pass (8 E2E tests require Chrome), 6/6 Go tests pass
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (8)
mcp-server/internal/config/gemini_config_test.go (2)
56-71:TestGeminiConfigValidatesre-validates whatLoadalready validates.Per
config.goLine 175,Load()already callscfg.Validate()internally and returns its error. CallingValidate()again on Line 68 is redundant. This test effectively only checks thatLoadsucceeds (same asTestGeminiConfigLoads).If the intent is to test
Validate()in isolation, consider constructing aConfigstruct manually and callingValidate()on it, which would provide independent value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp-server/internal/config/gemini_config_test.go` around lines 56 - 71, TestGeminiConfigValidates redundantly calls cfg.Validate() after Load() because Load already performs validation; remove the duplicate validation call from TestGeminiConfigValidates (the cfg.Validate() invocation) so the test only verifies Load succeeds, or alternatively replace the test body to construct a Config instance manually and call Validate() to test validation in isolation; reference the TestGeminiConfigValidates test function, the Load function, and the Validate method when making the change.
73-98:TestGeminiConfigProgressiveToolCountdoesn't actually assert tool count.The test name claims to validate "exactly 6 progressive disclosure tools," but it only checks that
progressive_onlyistrue(already asserted inTestGeminiConfigLoads) and logs the expected tool list. There's no assertion that the server would actually expose 6 tools.Either rename the test to reflect what it does (e.g.,
TestGeminiConfigProgressiveOnlyEnabled) or add an actual tool-count assertion by instantiating the MCP server and querying its registered tools.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp-server/internal/config/gemini_config_test.go` around lines 73 - 98, The test TestGeminiConfigProgressiveToolCount currently only checks cfg.MCP.IsProgressiveOnly() and logs expectedTools but does not assert the actual exposed tool count; either rename the test to reflect it only verifies progressive_only, or modify it to instantiate the MCP server with the loaded cfg (e.g., create the server via the project’s MCP server constructor using cfg), retrieve the server's registered tool list (e.g., RegisteredTools() or similar API), and assert that len(registeredTools) == len(expectedTools) and that the tool names equal the expectedTools slice to enforce the "exactly 6" requirement.mcp-server/internal/mangle/gemini_schema_test.go (1)
12-27: Consider adding a skip guard for missing schema file.Unlike the config tests which gracefully skip when
gemini-config.yamlis absent, these tests will fail with an opaqueNewEngineerror if../../schemas/browser.mgdoesn't exist. A skip guard would improve the experience in partial checkout or CI environments.Proposed guard (add before NewEngine call in each test)
schemaPath := "../../schemas/browser.mg" if _, err := os.Stat(schemaPath); os.IsNotExist(err) { t.Skipf("Schema not found at %s", schemaPath) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp-server/internal/mangle/gemini_schema_test.go` around lines 12 - 27, Add a skip guard before calling NewEngine in TestGeminiMangleSchemaLoads: check the schema file path ("../../schemas/browser.mg") with os.Stat and call t.Skipf("Schema not found at %s", schemaPath) when os.IsNotExist(err) to avoid calling NewEngine on missing files; place this check just before the NewEngine(cfg) invocation in the TestGeminiMangleSchemaLoads function so the config.MangleConfig/engine.Ready assertions only run when the schema exists.tests/extension-structure.test.js (1)
51-61: Duplicated MCP server lookup.
manifest.mcpServers[Object.keys(manifest.mcpServers)[0]]is repeated in two tests. Consider extracting it into a shared variable inbeforeAllfor clarity.Proposed refactor
let manifest; + let firstServer; beforeAll(() => { const raw = fs.readFileSync(path.join(ROOT, 'gemini-extension.json'), 'utf8'); manifest = JSON.parse(raw); + const serverNames = Object.keys(manifest.mcpServers || {}); + firstServer = serverNames.length > 0 ? manifest.mcpServers[serverNames[0]] : undefined; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/extension-structure.test.js` around lines 51 - 61, The two tests duplicate the MCP server lookup manifest.mcpServers[Object.keys(manifest.mcpServers)[0]]; refactor by declaring a shared variable (e.g., let server) and assigning it in a beforeAll hook (assign server = manifest.mcpServers[Object.keys(manifest.mcpServers)[0]]), then update the tests to use that shared server variable so the lookup is performed once and the tests are clearer.tests/e2e-lifecycle.test.js (1)
56-56: Suppressing stderr discards diagnostic output on test failures.When a test fails, server-side error logs would be invaluable for debugging. Consider accumulating stderr and logging it in
afterAllor on failure.Proposed change
- this.proc.stderr.on('data', () => {}); // suppress + this.stderrLog = ''; + this.proc.stderr.on('data', (chunk) => { this.stderrLog += chunk.toString(); });Then in
afterAll:afterAll(() => { if (client.stderrLog) console.error('Server stderr:', client.stderrLog); client.stop(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e-lifecycle.test.js` at line 56, The test currently suppresses server stderr by attaching an empty handler to this.proc.stderr which discards diagnostics; change the handler to accumulate stderr into a string property (e.g., this.proc.stderrLog) by appending chunk.toString() in the 'data' listener on this.proc.stderr, remove the no-op listener, and in afterAll (or test failure cleanup) check and print client.stderrLog (or this.proc.stderrLog) via console.error before calling client.stop() so server error output is available on failures; reference the this.proc.stderr 'data' listener, the stderrLog property, the afterAll cleanup and client.stop() for where to apply the changes.tests/mcp-server-smoke.test.js (1)
73-74: Fixed 2-second startup delay is fragile.
setTimeout(done, 2000)assumes the server starts within 2 seconds. On slow CI machines this may not be enough; on fast machines it wastes time. Consider pollingproc.stdoutfor initial output or usingproc.on('spawn', ...)as a readiness signal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mcp-server-smoke.test.js` around lines 73 - 74, The fixed 2s delay using setTimeout(done, 2000) is fragile; replace it with an active readiness check: listen for the server process readiness (use proc.on('spawn', ...) as an immediate readiness signal or proc.stdout.on('data', ...) and poll the stdout for the server's startup log line) and call done only when that readiness event/log is observed; update the test to remove setTimeout(done, 2000) and instead wait on either proc.on('spawn', ...) or a matching stdout message before invoking done.tests/hooks.test.js (1)
36-71: String-based structural validation is intentional but inherently fragile.All assertions rely on
toContain/toMatchagainst raw file content. Any minor refactor ofafter-tool-call.js(e.g., renaming the parameter fromcontexttoctx, changing quote styles, or reformatting) will silently break these tests. This is fine for a "contract snapshot" approach, but consider adding a brief comment in the file noting that these tests must be updated in lockstep with the hook implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks.test.js` around lines 36 - 71, Add a short inline comment at the top of the hook implementation (the file referenced by these tests that contains the "export default async function" implementation) stating that the test-suite uses fragile string-based assertions and must be updated in lockstep when making structural changes—mention examples like renaming the "context" parameter, changing quote styles around 'browser-act'/'browser-reason', reformatting that would affect checks for "async function", 'context', 'crash', '"status":"error"', '[System injected via hook]', 'return context', and '!context'. This will alert future editors that modifications to the function signature, quoted literals, or formatting require corresponding updates to tests/hooks.test.js.tests/commands.test.js (1)
65-68: Kebab-case regex allows trailing hyphens.
/^[a-z][a-z0-9-]*$/permits names likefoo-orbar--baz. A stricter pattern would be:/^[a-z][a-z0-9]*(-[a-z0-9]+)*$/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/commands.test.js` around lines 65 - 68, The current test "command file name uses kebab-case" uses a permissive regex for the variable name that allows trailing or consecutive hyphens; update the expect(name).toMatch call to use a stricter pattern that enforces: name starts with a lowercase letter, may have letters/digits, and any hyphen must be followed by at least one alphanumeric segment (i.e., no trailing or consecutive hyphens) so the test rejects "foo-" and "bar--baz".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcp-server/internal/config/gemini_config_test.go`:
- Around line 26-28: The test in gemini_config_test.go currently asserts
cfg.Server.Version == "0.0.8", which will break on version bumps; replace this
hardcoded check in the test that references cfg.Server.Version with one of two
fixes: (A) load the expected version dynamically from the same source as
production (e.g., parse gemini-config.yaml or package.json) and compare
cfg.Server.Version to that loaded value, or (B) relax the assertion to verify
cfg.Server.Version is non-empty and matches a semver pattern (use a regexp like
^v?\d+\.\d+\.\d+(-[A-Za-z0-9.-]+)?(\+[A-Za-z0-9.-]+)?$) so the test no longer
relies on a specific literal version string. Ensure you update the test helper
or import regexp where needed and keep the assertion against cfg.Server.Version.
In `@mcp-server/internal/mangle/gemini_schema_test.go`:
- Around line 98-112: The test currently skips on query errors and never asserts
when the query succeeds but returns zero results; update
TestGeminiTriageHintRule so that after calling engine.Query(ctx,
`triage_hint(SessionId, Action).`) you keep the existing t.Skip on error but
when err == nil assert that len(results) > 0 (use t.Fatalf or
t.Errorf+t.FailNow) and optionally validate the Action content (e.g., check that
the returned Action string contains an expected substring); reference the test
name TestGeminiTriageHintRule, the engine.Query call and the
triage_hint(SessionId, Action) query when making the change.
In `@tests/commands.test.js`:
- Around line 14-25: The module-level fs.readdirSync calls (COMMANDS_DIR ->
commandGroups -> allCommands population) will throw if the commands directory is
missing; move this logic into a setup block or guard it by checking existence
(e.g., fs.existsSync(COMMANDS_DIR)) or wrapping the readdirSync in a try/catch
so the test file doesn't crash at import time. Specifically, protect the code
that computes commandGroups and populates allCommands (references: COMMANDS_DIR,
commandGroups, allCommands, fs.readdirSync, fs.statSync) by either relocating it
into a beforeAll/describe setup or by early-returning/setting allCommands to []
when the directory is absent or unreadable.
In `@tests/e2e-lifecycle.test.js`:
- Around line 220-221: The test comment and assertion disagree: the comment
claims quick_status should be under 500 tokens (~2000 chars) but the assertion
checks expect(resultStr.length).toBeLessThan(5000). Update either the comment or
the assertion so they match; specifically change the numeric threshold in the
assertion to reflect ~2000 characters (e.g., toBeLessThan(2000)) if you want to
enforce the 500-token claim, or update the comment to document the 5000-char
threshold; locate the check using resultStr and the test name in
tests/e2e-lifecycle.test.js to make the edit.
- Around line 243-248: The assertion is vacuous because hasElements uses
resultStr.includes('a'), which matches almost any string; update the hasElements
check in the test (the hasElements variable inside the if (typeof result ===
'object') block) to look for more specific link/anchor indicators in resultStr —
for example check for '<a ' or 'href=' for HTML anchors, or specific JSON link
markers like '"link"' or '"type":"link"' — so replace the includes('a') check
with one or more of these targeted includes/regex checks while leaving the
hasRefs logic unchanged.
- Around line 50-54: The test currently accumulates stdout into a JS string
(this.buffer += chunk.toString('utf8')) which makes this.buffer.length count
characters not bytes, breaking Content-Length byte framing; change the buffer to
a Buffer (initialize this.buffer = Buffer.alloc(0)), append incoming chunks with
this.buffer = Buffer.concat([this.buffer, chunk]) inside the
this.proc.stdout.on('data', ...) handler, and update _drain() to read
Content-Length using this.buffer.length (bytes), slice message bytes with
this.buffer.slice(0, contentLength).toString('utf8') when decoding, and remove
any string-based concatenation logic so framing is byte-accurate.
In `@tests/extension-structure.test.js`:
- Around line 15-22: The beforeAll block reads gemini-extension.json
unconditionally which throws ENOENT and prevents the "exists at repository root"
test from running; update the tests/extension-structure.test.js to first assert
the file exists (use fs.existsSync in the "exists at repository root" test or
move that check into a beforeAll guard) and only call fs.readFileSync/JSON.parse
if the file exists, and apply the same pattern to the GEMINI.md and package.json
blocks (guard reads with fs.existsSync or reorder so existence is asserted
before reading) so the test runner reports a clear missing-file failure instead
of throwing in setup.
In `@tests/hooks.test.js`:
- Around line 28-34: The beforeAll hook currently calls
fs.readFileSync(hookPath) which will throw ENOENT if the file is missing and
prevent the 'file exists' test from running; change the setup so you check
existence first (e.g., using fs.existsSync(hookPath) inside beforeAll or move
the fs.readFileSync into a separate step after verifying existence) and only
call fs.readFileSync(hookPath) when the file exists, referencing the existing
beforeAll, fs.readFileSync, fs.existsSync, hookPath and the 'file exists' test
to locate where to apply the guard.
In `@tests/mcp-server-smoke.test.js`:
- Around line 98-113: The fallback assertion is vacuous because
expect(responses.length).toBeGreaterThanOrEqual(0) always passes; update the
fallback in the setTimeout callback that inspects parseJsonRpcResponse(stdout)
(the responses variable and initResponse check) to assert a real failure: either
change the assertion to expect(responses.length).toBeGreaterThan(0) or
explicitly fail the test with done.fail('no initialize response received') so
the test fails when initResponse is missing.
- Around line 30-44: The parseJsonRpcResponse function in
tests/mcp-server-smoke.test.js is using a fragile regex (parseJsonRpcResponse)
that fails on nested objects; replace it with Content-Length framing parsing
similar to McpClient._drain: read headers to find "Content-Length", parse that
many bytes for the JSON payload, repeat until buffer exhausted, and JSON.parse
each complete body (or reuse/extract a shared helper from McpClient._drain).
Ensure the new parser handles multiple messages in a buffer, preserves newlines,
and throws or returns partial/incomplete data appropriately instead of silencing
JSON.parse errors.
- Around line 118-119: The test uses jsonRpcRequest which always sets an id, but
JSON-RPC notifications must omit id; add a notify helper (mirroring
e2e-lifecycle.test.js) that builds a JSON-RPC 2.0 object without an id and
returns it, then replace the call to jsonRpcRequest('notifications/initialized',
{}) with notify('notifications/initialized', {}) (or call the new helper where
initialized is sent) so the initialized message is a proper notification;
reference the existing jsonRpcRequest helper and the e2e-lifecycle.test.js
notify pattern when implementing.
In `@tests/skills.test.js`:
- Around line 13-16: Guard the call to fs.readdirSync(SKILLS_DIR) so loading the
test file doesn't throw when SKILLS_DIR is missing: check existence with
fs.existsSync(SKILLS_DIR) (or wrap fs.readdirSync in a try/catch) and set
skillDirs to an empty array if the directory is absent or reading fails; update
the initialization of skillDirs (the const skillDirs = ... expression) to use
that guard while preserving the filtering by fs.statSync and path.join.
- Around line 33-76: Several tests call content.split(...) when SKILL.md may be
missing, causing TypeErrors; before running any assertions that use content (in
the tests "frontmatter contains a name field", "frontmatter contains a
description field", "frontmatter name matches directory name", "body content is
at least 100 characters long", and "body references at least one BrowserNERD
tool") add a guard/assertion to ensure the file exists or content is defined
(e.g., expect(fs.existsSync(skillFile)).toBe(true) or
expect(content).toBeDefined()) and return/fail early if not, so the tests fail
once with a clear missing-file error instead of throwing in content.split.
Ensure you update the various tests that reference content.split('---')
accordingly.
---
Nitpick comments:
In `@mcp-server/internal/config/gemini_config_test.go`:
- Around line 56-71: TestGeminiConfigValidates redundantly calls cfg.Validate()
after Load() because Load already performs validation; remove the duplicate
validation call from TestGeminiConfigValidates (the cfg.Validate() invocation)
so the test only verifies Load succeeds, or alternatively replace the test body
to construct a Config instance manually and call Validate() to test validation
in isolation; reference the TestGeminiConfigValidates test function, the Load
function, and the Validate method when making the change.
- Around line 73-98: The test TestGeminiConfigProgressiveToolCount currently
only checks cfg.MCP.IsProgressiveOnly() and logs expectedTools but does not
assert the actual exposed tool count; either rename the test to reflect it only
verifies progressive_only, or modify it to instantiate the MCP server with the
loaded cfg (e.g., create the server via the project’s MCP server constructor
using cfg), retrieve the server's registered tool list (e.g., RegisteredTools()
or similar API), and assert that len(registeredTools) == len(expectedTools) and
that the tool names equal the expectedTools slice to enforce the "exactly 6"
requirement.
In `@mcp-server/internal/mangle/gemini_schema_test.go`:
- Around line 12-27: Add a skip guard before calling NewEngine in
TestGeminiMangleSchemaLoads: check the schema file path
("../../schemas/browser.mg") with os.Stat and call t.Skipf("Schema not found at
%s", schemaPath) when os.IsNotExist(err) to avoid calling NewEngine on missing
files; place this check just before the NewEngine(cfg) invocation in the
TestGeminiMangleSchemaLoads function so the config.MangleConfig/engine.Ready
assertions only run when the schema exists.
In `@tests/commands.test.js`:
- Around line 65-68: The current test "command file name uses kebab-case" uses a
permissive regex for the variable name that allows trailing or consecutive
hyphens; update the expect(name).toMatch call to use a stricter pattern that
enforces: name starts with a lowercase letter, may have letters/digits, and any
hyphen must be followed by at least one alphanumeric segment (i.e., no trailing
or consecutive hyphens) so the test rejects "foo-" and "bar--baz".
In `@tests/e2e-lifecycle.test.js`:
- Line 56: The test currently suppresses server stderr by attaching an empty
handler to this.proc.stderr which discards diagnostics; change the handler to
accumulate stderr into a string property (e.g., this.proc.stderrLog) by
appending chunk.toString() in the 'data' listener on this.proc.stderr, remove
the no-op listener, and in afterAll (or test failure cleanup) check and print
client.stderrLog (or this.proc.stderrLog) via console.error before calling
client.stop() so server error output is available on failures; reference the
this.proc.stderr 'data' listener, the stderrLog property, the afterAll cleanup
and client.stop() for where to apply the changes.
In `@tests/extension-structure.test.js`:
- Around line 51-61: The two tests duplicate the MCP server lookup
manifest.mcpServers[Object.keys(manifest.mcpServers)[0]]; refactor by declaring
a shared variable (e.g., let server) and assigning it in a beforeAll hook
(assign server = manifest.mcpServers[Object.keys(manifest.mcpServers)[0]]), then
update the tests to use that shared server variable so the lookup is performed
once and the tests are clearer.
In `@tests/hooks.test.js`:
- Around line 36-71: Add a short inline comment at the top of the hook
implementation (the file referenced by these tests that contains the "export
default async function" implementation) stating that the test-suite uses fragile
string-based assertions and must be updated in lockstep when making structural
changes—mention examples like renaming the "context" parameter, changing quote
styles around 'browser-act'/'browser-reason', reformatting that would affect
checks for "async function", 'context', 'crash', '"status":"error"', '[System
injected via hook]', 'return context', and '!context'. This will alert future
editors that modifications to the function signature, quoted literals, or
formatting require corresponding updates to tests/hooks.test.js.
In `@tests/mcp-server-smoke.test.js`:
- Around line 73-74: The fixed 2s delay using setTimeout(done, 2000) is fragile;
replace it with an active readiness check: listen for the server process
readiness (use proc.on('spawn', ...) as an immediate readiness signal or
proc.stdout.on('data', ...) and poll the stdout for the server's startup log
line) and call done only when that readiness event/log is observed; update the
test to remove setTimeout(done, 2000) and instead wait on either
proc.on('spawn', ...) or a matching stdout message before invoking done.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
mcp-server/internal/config/gemini_config_test.gomcp-server/internal/mangle/gemini_schema_test.gomcp-server/schemas/browser.mgpackage.jsontests/commands.test.jstests/e2e-lifecycle.test.jstests/extension-structure.test.jstests/hooks.test.jstests/mcp-server-smoke.test.jstests/skills.test.js
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- mcp-server/schemas/browser.mg
| if cfg.Server.Version != "0.0.8" { | ||
| t.Errorf("expected version '0.0.8', got %q", cfg.Server.Version) | ||
| } |
There was a problem hiding this comment.
Hardcoded version "0.0.8" will break on every version bump.
This assertion couples the test to a specific version string. Consider reading the expected version from gemini-config.yaml or package.json dynamically, or at minimum just asserting the version is non-empty and matches semver format.
Proposed alternative
- if cfg.Server.Version != "0.0.8" {
- t.Errorf("expected version '0.0.8', got %q", cfg.Server.Version)
- }
+ if cfg.Server.Version == "" {
+ t.Error("expected a non-empty server version")
+ }📝 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.
| if cfg.Server.Version != "0.0.8" { | |
| t.Errorf("expected version '0.0.8', got %q", cfg.Server.Version) | |
| } | |
| if cfg.Server.Version == "" { | |
| t.Error("expected a non-empty server version") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcp-server/internal/config/gemini_config_test.go` around lines 26 - 28, The
test in gemini_config_test.go currently asserts cfg.Server.Version == "0.0.8",
which will break on version bumps; replace this hardcoded check in the test that
references cfg.Server.Version with one of two fixes: (A) load the expected
version dynamically from the same source as production (e.g., parse
gemini-config.yaml or package.json) and compare cfg.Server.Version to that
loaded value, or (B) relax the assertion to verify cfg.Server.Version is
non-empty and matches a semver pattern (use a regexp like
^v?\d+\.\d+\.\d+(-[A-Za-z0-9.-]+)?(\+[A-Za-z0-9.-]+)?$) so the test no longer
relies on a specific literal version string. Ensure you update the test helper
or import regexp where needed and keep the assertion against cfg.Server.Version.
| // The triage_hint rule should fire because: | ||
| // - agent_client("gemini_cli") is a static fact in the schema | ||
| // - caused_by should derive from the console_event + net_response with status >= 400 | ||
| results, err := engine.Query(ctx, `triage_hint(SessionId, Action).`) | ||
| if err != nil { | ||
| // Some Mangle engines may not support all query forms; log and skip | ||
| t.Logf("triage_hint query returned error (may be expected if caused_by timing is strict): %v", err) | ||
| t.Skip("Skipping triage_hint validation due to query limitations") | ||
| } | ||
|
|
||
| t.Logf("triage_hint results: %+v", results) | ||
| // If the causal chain fired, we should get a result with a helpful action string | ||
| if len(results) > 0 { | ||
| t.Logf("triage_hint rule fired successfully with %d result(s)", len(results)) | ||
| } |
There was a problem hiding this comment.
TestGeminiTriageHintRule cannot fail — it skips on error and has no assertion on empty results.
After AddFacts succeeds, the test either:
- Query errors → skips (Line 105)
- Results > 0 → logs success, no assertion on content
- Results == 0 → falls through with no assertion at all
The rule logic is never actually validated. Consider asserting that results are non-empty when the query succeeds, or asserting specific content in the action string:
Proposed fix
t.Logf("triage_hint results: %+v", results)
- // If the causal chain fired, we should get a result with a helpful action string
- if len(results) > 0 {
- t.Logf("triage_hint rule fired successfully with %d result(s)", len(results))
+ if len(results) == 0 {
+ t.Error("Expected triage_hint rule to fire with prerequisite facts, got 0 results")
}📝 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.
| // The triage_hint rule should fire because: | |
| // - agent_client("gemini_cli") is a static fact in the schema | |
| // - caused_by should derive from the console_event + net_response with status >= 400 | |
| results, err := engine.Query(ctx, `triage_hint(SessionId, Action).`) | |
| if err != nil { | |
| // Some Mangle engines may not support all query forms; log and skip | |
| t.Logf("triage_hint query returned error (may be expected if caused_by timing is strict): %v", err) | |
| t.Skip("Skipping triage_hint validation due to query limitations") | |
| } | |
| t.Logf("triage_hint results: %+v", results) | |
| // If the causal chain fired, we should get a result with a helpful action string | |
| if len(results) > 0 { | |
| t.Logf("triage_hint rule fired successfully with %d result(s)", len(results)) | |
| } | |
| // The triage_hint rule should fire because: | |
| // - agent_client("gemini_cli") is a static fact in the schema | |
| // - caused_by should derive from the console_event + net_response with status >= 400 | |
| results, err := engine.Query(ctx, `triage_hint(SessionId, Action).`) | |
| if err != nil { | |
| // Some Mangle engines may not support all query forms; log and skip | |
| t.Logf("triage_hint query returned error (may be expected if caused_by timing is strict): %v", err) | |
| t.Skip("Skipping triage_hint validation due to query limitations") | |
| } | |
| t.Logf("triage_hint results: %+v", results) | |
| if len(results) == 0 { | |
| t.Error("Expected triage_hint rule to fire with prerequisite facts, got 0 results") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcp-server/internal/mangle/gemini_schema_test.go` around lines 98 - 112, The
test currently skips on query errors and never asserts when the query succeeds
but returns zero results; update TestGeminiTriageHintRule so that after calling
engine.Query(ctx, `triage_hint(SessionId, Action).`) you keep the existing
t.Skip on error but when err == nil assert that len(results) > 0 (use t.Fatalf
or t.Errorf+t.FailNow) and optionally validate the Action content (e.g., check
that the returned Action string contains an expected substring); reference the
test name TestGeminiTriageHintRule, the engine.Query call and the
triage_hint(SessionId, Action) query when making the change.
| const commandGroups = fs.readdirSync(COMMANDS_DIR).filter(d => | ||
| fs.statSync(path.join(COMMANDS_DIR, d)).isDirectory() | ||
| ); | ||
|
|
||
| const allCommands = []; | ||
| commandGroups.forEach(group => { | ||
| const groupDir = path.join(COMMANDS_DIR, group); | ||
| const tomlFiles = fs.readdirSync(groupDir).filter(f => f.endsWith('.toml')); | ||
| tomlFiles.forEach(f => { | ||
| allCommands.push({ group, file: f, fullPath: path.join(groupDir, f) }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Module-level readdirSync crashes the entire test file if commands/ is missing.
Unlike other test files where beforeAll failures are scoped to a describe block, this code runs at import time. If the commands directory doesn't exist, every test in this file (including unrelated future additions) fails with an unhandled ENOENT before Jest even registers the test cases.
Proposed guard
+const commandsExist = fs.existsSync(COMMANDS_DIR);
+
// Discover all command groups and their .toml files
-const commandGroups = fs.readdirSync(COMMANDS_DIR).filter(d =>
+const commandGroups = commandsExist ? fs.readdirSync(COMMANDS_DIR).filter(d =>
fs.statSync(path.join(COMMANDS_DIR, d)).isDirectory()
-);
+) : [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/commands.test.js` around lines 14 - 25, The module-level fs.readdirSync
calls (COMMANDS_DIR -> commandGroups -> allCommands population) will throw if
the commands directory is missing; move this logic into a setup block or guard
it by checking existence (e.g., fs.existsSync(COMMANDS_DIR)) or wrapping the
readdirSync in a try/catch so the test file doesn't crash at import time.
Specifically, protect the code that computes commandGroups and populates
allCommands (references: COMMANDS_DIR, commandGroups, allCommands,
fs.readdirSync, fs.statSync) by either relocating it into a beforeAll/describe
setup or by early-returning/setting allCommands to [] when the directory is
absent or unreadable.
|
|
||
| this.proc.stdout.on('data', (chunk) => { | ||
| this.buffer += chunk.toString('utf8'); | ||
| this._drain(); | ||
| }); |
There was a problem hiding this comment.
Content-Length framing compares byte length against character length.
On Line 52, chunk.toString('utf8') converts to a JS string, so this.buffer.length on Line 78 returns character count. However, Content-Length in the MCP protocol specifies byte count. For responses containing multi-byte UTF-8 characters, the framing will silently corrupt message boundaries.
Fix by keeping the buffer as a Buffer (like the smoke test does) or by converting the Content-Length comparison to character-aware logic.
Proposed fix: keep buffer as Buffer
constructor() {
this.proc = null;
- this.buffer = '';
+ this.buffer = Buffer.alloc(0);
this.pending = new Map();
}
...
this.proc.stdout.on('data', (chunk) => {
- this.buffer += chunk.toString('utf8');
+ this.buffer = Buffer.concat([this.buffer, chunk]);
this._drain();
});
...
_drain() {
- // Parse Content-Length framed JSON-RPC messages
while (true) {
- const headerEnd = this.buffer.indexOf('\r\n\r\n');
+ const headerEnd = this.buffer.indexOf('\r\n\r\n');
if (headerEnd === -1) break;
- const header = this.buffer.substring(0, headerEnd);
+ const header = this.buffer.slice(0, headerEnd).toString('utf8');
const match = header.match(/Content-Length:\s*(\d+)/i);
if (!match) {
- this.buffer = this.buffer.substring(headerEnd + 4);
+ this.buffer = this.buffer.slice(headerEnd + 4);
continue;
}
const len = parseInt(match[1], 10);
const bodyStart = headerEnd + 4;
if (this.buffer.length < bodyStart + len) break; // incomplete
- const body = this.buffer.substring(bodyStart, bodyStart + len);
- this.buffer = this.buffer.substring(bodyStart + len);
+ const body = this.buffer.slice(bodyStart, bodyStart + len).toString('utf8');
+ this.buffer = this.buffer.slice(bodyStart + len);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e-lifecycle.test.js` around lines 50 - 54, The test currently
accumulates stdout into a JS string (this.buffer += chunk.toString('utf8'))
which makes this.buffer.length count characters not bytes, breaking
Content-Length byte framing; change the buffer to a Buffer (initialize
this.buffer = Buffer.alloc(0)), append incoming chunks with this.buffer =
Buffer.concat([this.buffer, chunk]) inside the this.proc.stdout.on('data', ...)
handler, and update _drain() to read Content-Length using this.buffer.length
(bytes), slice message bytes with this.buffer.slice(0,
contentLength).toString('utf8') when decoding, and remove any string-based
concatenation logic so framing is byte-accurate.
| // Token efficiency claim: quick_status should be under 500 tokens (~2000 chars) | ||
| expect(resultStr.length).toBeLessThan(5000); |
There was a problem hiding this comment.
Comment/assertion mismatch on token efficiency.
Comment says "under 500 tokens (~2000 chars)" but the assertion checks < 5000 — a 2.5× discrepancy. Align the threshold with the documented claim or update the comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e-lifecycle.test.js` around lines 220 - 221, The test comment and
assertion disagree: the comment claims quick_status should be under 500 tokens
(~2000 chars) but the assertion checks
expect(resultStr.length).toBeLessThan(5000). Update either the comment or the
assertion so they match; specifically change the numeric threshold in the
assertion to reflect ~2000 characters (e.g., toBeLessThan(2000)) if you want to
enforce the 500-token claim, or update the comment to document the 5000-char
threshold; locate the check using resultStr and the test name in
tests/e2e-lifecycle.test.js to make the edit.
| function parseJsonRpcResponse(buffer) { | ||
| const str = buffer.toString('utf8'); | ||
| // Find all JSON objects in the buffer (skip Content-Length headers) | ||
| const results = []; | ||
| const regex = /\{[^]*?"jsonrpc"\s*:\s*"2\.0"[^]*?\}/g; | ||
| let match; | ||
| while ((match = regex.exec(str)) !== null) { | ||
| try { | ||
| results.push(JSON.parse(match[0])); | ||
| } catch (e) { | ||
| // partial JSON, skip | ||
| } | ||
| } | ||
| return results; | ||
| } |
There was a problem hiding this comment.
Regex [^]*? will mis-parse nested JSON objects.
The pattern \{[^]*?"jsonrpc"\s*:\s*"2\.0"[^]*?\} uses non-greedy [^]*? before the closing }, so it stops at the first } encountered — truncating any response that contains nested objects (which most MCP responses will). JSON.parse in the catch block silently discards these, potentially making every response unparseable.
Also flagged by Biome: [^] is a negated empty character class. While valid in JS (matches any char including newline), prefer [\s\S] for clarity, or better yet, use Content-Length framing like in e2e-lifecycle.test.js's McpClient._drain().
Proposed fix: use Content-Length framing
function parseJsonRpcResponse(buffer) {
const str = buffer.toString('utf8');
- // Find all JSON objects in the buffer (skip Content-Length headers)
const results = [];
- const regex = /\{[^]*?"jsonrpc"\s*:\s*"2\.0"[^]*?\}/g;
- let match;
- while ((match = regex.exec(str)) !== null) {
- try {
- results.push(JSON.parse(match[0]));
- } catch (e) {
- // partial JSON, skip
+ let remaining = str;
+ while (remaining.length > 0) {
+ const headerEnd = remaining.indexOf('\r\n\r\n');
+ if (headerEnd === -1) break;
+ const header = remaining.substring(0, headerEnd);
+ const m = header.match(/Content-Length:\s*(\d+)/i);
+ if (!m) { remaining = remaining.substring(headerEnd + 4); continue; }
+ const len = parseInt(m[1], 10);
+ const bodyStart = headerEnd + 4;
+ if (remaining.length < bodyStart + len) break;
+ const body = remaining.substring(bodyStart, bodyStart + len);
+ remaining = remaining.substring(bodyStart + len);
+ try { results.push(JSON.parse(body)); } catch (e) {
+ // skip malformed
}
}
return results;
}Note: e2e-lifecycle.test.js already implements correct Content-Length framing in McpClient._drain() — consider extracting a shared helper.
🧰 Tools
🪛 Biome (2.4.4)
[error] 34-34: The regular expression includes this negated empty character class.
(lint/correctness/noEmptyCharacterClassInRegex)
[error] 34-34: The regular expression includes this negated empty character class.
(lint/correctness/noEmptyCharacterClassInRegex)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/mcp-server-smoke.test.js` around lines 30 - 44, The
parseJsonRpcResponse function in tests/mcp-server-smoke.test.js is using a
fragile regex (parseJsonRpcResponse) that fails on nested objects; replace it
with Content-Length framing parsing similar to McpClient._drain: read headers to
find "Content-Length", parse that many bytes for the JSON payload, repeat until
buffer exhausted, and JSON.parse each complete body (or reuse/extract a shared
helper from McpClient._drain). Ensure the new parser handles multiple messages
in a buffer, preserves newlines, and throws or returns partial/incomplete data
appropriately instead of silencing JSON.parse errors.
| setTimeout(() => { | ||
| const responses = parseJsonRpcResponse(stdout); | ||
| const initResponse = responses.find(r => r.result && r.result.serverInfo); | ||
|
|
||
| if (!initResponse) { | ||
| // Server may use different response format, just check we got something | ||
| expect(responses.length).toBeGreaterThanOrEqual(0); | ||
| done(); | ||
| return; | ||
| } | ||
|
|
||
| expect(initResponse.result.serverInfo).toBeDefined(); | ||
| expect(initResponse.result.serverInfo.name).toContain('browsernerd'); | ||
| done(); | ||
| }, 3000); | ||
| }, 10000); |
There was a problem hiding this comment.
Vacuous assertion on fallback path — test always passes when server doesn't respond.
Line 104: expect(responses.length).toBeGreaterThanOrEqual(0) is always true since length is never negative. If the server fails to respond to initialize, this test silently passes instead of detecting the failure.
Proposed fix
if (!initResponse) {
- // Server may use different response format, just check we got something
- expect(responses.length).toBeGreaterThanOrEqual(0);
+ // Server responded but with a different format; at minimum we got a response
+ expect(responses.length).toBeGreaterThan(0);
done();
return;
}📝 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.
| setTimeout(() => { | |
| const responses = parseJsonRpcResponse(stdout); | |
| const initResponse = responses.find(r => r.result && r.result.serverInfo); | |
| if (!initResponse) { | |
| // Server may use different response format, just check we got something | |
| expect(responses.length).toBeGreaterThanOrEqual(0); | |
| done(); | |
| return; | |
| } | |
| expect(initResponse.result.serverInfo).toBeDefined(); | |
| expect(initResponse.result.serverInfo.name).toContain('browsernerd'); | |
| done(); | |
| }, 3000); | |
| }, 10000); | |
| setTimeout(() => { | |
| const responses = parseJsonRpcResponse(stdout); | |
| const initResponse = responses.find(r => r.result && r.result.serverInfo); | |
| if (!initResponse) { | |
| // Server responded but with a different format; at minimum we got a response | |
| expect(responses.length).toBeGreaterThan(0); | |
| done(); | |
| return; | |
| } | |
| expect(initResponse.result.serverInfo).toBeDefined(); | |
| expect(initResponse.result.serverInfo.name).toContain('browsernerd'); | |
| done(); | |
| }, 3000); | |
| }, 10000); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/mcp-server-smoke.test.js` around lines 98 - 113, The fallback assertion
is vacuous because expect(responses.length).toBeGreaterThanOrEqual(0) always
passes; update the fallback in the setTimeout callback that inspects
parseJsonRpcResponse(stdout) (the responses variable and initResponse check) to
assert a real failure: either change the assertion to
expect(responses.length).toBeGreaterThan(0) or explicitly fail the test with
done.fail('no initialize response received') so the test fails when initResponse
is missing.
| // Send initialized notification first | ||
| const notifyMsg = jsonRpcRequest('notifications/initialized', {}); |
There was a problem hiding this comment.
notifications/initialized should not include a JSON-RPC id field.
Per the JSON-RPC 2.0 spec, notifications must omit id. The jsonRpcRequest helper always sets id, making this a notification with an id—technically a regular request the server may try to respond to.
The e2e-lifecycle.test.js file handles this correctly with a separate notify() method that omits id.
Proposed fix: add a notification helper
+function jsonRpcNotification(method, params = {}) {
+ const msg = JSON.stringify({ jsonrpc: '2.0', method, params });
+ return `Content-Length: ${Buffer.byteLength(msg)}\r\n\r\n${msg}`;
+}Then on Line 119:
- const notifyMsg = jsonRpcRequest('notifications/initialized', {});
+ const notifyMsg = jsonRpcNotification('notifications/initialized', {});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/mcp-server-smoke.test.js` around lines 118 - 119, The test uses
jsonRpcRequest which always sets an id, but JSON-RPC notifications must omit id;
add a notify helper (mirroring e2e-lifecycle.test.js) that builds a JSON-RPC 2.0
object without an id and returns it, then replace the call to
jsonRpcRequest('notifications/initialized', {}) with
notify('notifications/initialized', {}) (or call the new helper where
initialized is sent) so the initialized message is a proper notification;
reference the existing jsonRpcRequest helper and the e2e-lifecycle.test.js
notify pattern when implementing.
| // Discover all skills dynamically | ||
| const skillDirs = fs.readdirSync(SKILLS_DIR).filter(d => | ||
| fs.statSync(path.join(SKILLS_DIR, d)).isDirectory() | ||
| ); |
There was a problem hiding this comment.
Guard readdirSync so the test file can load cleanly when skills/ is missing.
Right now fs.readdirSync(SKILLS_DIR) can throw before Jest even runs the “exists” test, so failures show up as a load error rather than a clear assertion.
✅ Suggested fix
-const skillDirs = fs.readdirSync(SKILLS_DIR).filter(d =>
- fs.statSync(path.join(SKILLS_DIR, d)).isDirectory()
-);
+const skillDirs = fs.existsSync(SKILLS_DIR)
+ ? fs.readdirSync(SKILLS_DIR).filter(d =>
+ fs.statSync(path.join(SKILLS_DIR, d)).isDirectory()
+ )
+ : [];📝 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.
| // Discover all skills dynamically | |
| const skillDirs = fs.readdirSync(SKILLS_DIR).filter(d => | |
| fs.statSync(path.join(SKILLS_DIR, d)).isDirectory() | |
| ); | |
| // Discover all skills dynamically | |
| const skillDirs = fs.existsSync(SKILLS_DIR) | |
| ? fs.readdirSync(SKILLS_DIR).filter(d => | |
| fs.statSync(path.join(SKILLS_DIR, d)).isDirectory() | |
| ) | |
| : []; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/skills.test.js` around lines 13 - 16, Guard the call to
fs.readdirSync(SKILLS_DIR) so loading the test file doesn't throw when
SKILLS_DIR is missing: check existence with fs.existsSync(SKILLS_DIR) (or wrap
fs.readdirSync in a try/catch) and set skillDirs to an empty array if the
directory is absent or reading fails; update the initialization of skillDirs
(the const skillDirs = ... expression) to use that guard while preserving the
filtering by fs.statSync and path.join.
| beforeAll(() => { | ||
| if (fs.existsSync(skillFile)) { | ||
| content = fs.readFileSync(skillFile, 'utf8'); | ||
| } | ||
| }); | ||
|
|
||
| test('has a SKILL.md file', () => { | ||
| expect(fs.existsSync(skillFile)).toBe(true); | ||
| }); | ||
|
|
||
| test('SKILL.md starts with YAML frontmatter (---)', () => { | ||
| expect(content).toBeDefined(); | ||
| expect(content.trimStart().startsWith('---')).toBe(true); | ||
| }); | ||
|
|
||
| test('frontmatter contains a name field', () => { | ||
| const frontmatter = content.split('---')[1]; | ||
| expect(frontmatter).toContain('name:'); | ||
| }); | ||
|
|
||
| test('frontmatter contains a description field', () => { | ||
| const frontmatter = content.split('---')[1]; | ||
| expect(frontmatter).toContain('description:'); | ||
| }); | ||
|
|
||
| test('frontmatter name matches directory name', () => { | ||
| const frontmatter = content.split('---')[1]; | ||
| const nameMatch = frontmatter.match(/name:\s*(.+)/); | ||
| expect(nameMatch).not.toBeNull(); | ||
| expect(nameMatch[1].trim()).toBe(skillName); | ||
| }); | ||
|
|
||
| test('body content is at least 100 characters long', () => { | ||
| const parts = content.split('---'); | ||
| // parts[0] is empty, parts[1] is frontmatter, parts[2]+ is body | ||
| const body = parts.slice(2).join('---').trim(); | ||
| expect(body.length).toBeGreaterThanOrEqual(100); | ||
| }); | ||
|
|
||
| test('body references at least one BrowserNERD tool', () => { | ||
| const body = content.split('---').slice(2).join('---'); | ||
| const tools = ['browser-observe', 'browser-act', 'browser-reason', 'browser-mangle', 'launch-browser']; | ||
| const hasAtLeastOne = tools.some(t => body.includes(t)); | ||
| expect(hasAtLeastOne).toBe(true); |
There was a problem hiding this comment.
Avoid TypeErrors when SKILL.md is missing.
If the file doesn’t exist, content remains undefined and content.split(...) will throw in multiple tests. That turns a single “missing file” failure into multiple noisy errors. Consider gating the content tests on file existence.
✅ Suggested refactor
- beforeAll(() => {
- if (fs.existsSync(skillFile)) {
- content = fs.readFileSync(skillFile, 'utf8');
- }
- });
+ const skillFileExists = fs.existsSync(skillFile);
+
+ beforeAll(() => {
+ if (skillFileExists) {
+ content = fs.readFileSync(skillFile, 'utf8');
+ }
+ });
@@
- test('SKILL.md starts with YAML frontmatter (---)', () => {
- expect(content).toBeDefined();
- expect(content.trimStart().startsWith('---')).toBe(true);
- });
+ const describeIf = skillFileExists ? describe : describe.skip;
+
+ describeIf('SKILL.md content', () => {
+ test('starts with YAML frontmatter (---)', () => {
+ expect(content.trimStart().startsWith('---')).toBe(true);
+ });
+ // move the remaining content-based tests here
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/skills.test.js` around lines 33 - 76, Several tests call
content.split(...) when SKILL.md may be missing, causing TypeErrors; before
running any assertions that use content (in the tests "frontmatter contains a
name field", "frontmatter contains a description field", "frontmatter name
matches directory name", "body content is at least 100 characters long", and
"body references at least one BrowserNERD tool") add a guard/assertion to ensure
the file exists or content is defined (e.g.,
expect(fs.existsSync(skillFile)).toBe(true) or expect(content).toBeDefined())
and return/fail early if not, so the tests fail once with a clear missing-file
error instead of throwing in content.split. Ensure you update the various tests
that reference content.split('---') accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
README.md (1)
608-628: Extension URLs and command syntax are valid; consider moving this section.The install command
gemini extensions install https://github.com/gemini-cli-extensions/nanobananais correct, and the extension is listed in the official Gemini CLI extension registry. The/generateand/iconslash-command invocations and their flags are also correct per the extension's spec.Minor doc-organization nit: this section currently sits between the "Comparison with Alternatives" table and the "Architecture" diagram, which interrupts the technical documentation flow. Consider moving it after "Contributing" so the core reference sections remain contiguous.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 608 - 628, Move the "## 🍌 Graphics & Assets by Nano Banana" section (the Nano Banana install and gemini CLI examples) so it no longer interrupts core docs; specifically cut the entire Nano Banana block and paste it after the existing "Contributing" section in README.md to keep the "Comparison with Alternatives" and "Architecture" sections contiguous.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 3-6: Replace the external placeholder image src in the README's
img tag with a relative path to a committed asset (e.g., change the img src to
"assets/banner.png" and add that image file to the repo) to avoid relying on
placehold.co, and update the adjacent comment text ("See the \"Graphics by Nano
Banana\"") to match the actual section heading "Graphics & Assets by Nano
Banana"; locate the README.md img element and the comment text near it to make
these two edits (ensure the asset is committed under assets/banner.png and the
README references that relative path).
---
Nitpick comments:
In `@README.md`:
- Around line 608-628: Move the "## 🍌 Graphics & Assets by Nano Banana" section
(the Nano Banana install and gemini CLI examples) so it no longer interrupts
core docs; specifically cut the entire Nano Banana block and paste it after the
existing "Contributing" section in README.md to keep the "Comparison with
Alternatives" and "Architecture" sections contiguous.
| <p align="center"> | ||
| <!-- Tip: Generate a custom banner using the Nano Banana Gemini CLI extension! See the "Graphics by Nano Banana" section below. --> | ||
| <img src="https://placehold.co/1000x300/0d1117/00ADD8/png?text=BrowserNERD&font=roboto" alt="BrowserNERD Banner" width="100%"> | ||
| </p> |
There was a problem hiding this comment.
Replace the third-party placeholder banner and fix the stale comment text.
Two issues here:
-
Placeholder image dependency —
placehold.cois an external CDN. If it throttles hotlinking or goes offline, the banner will break for everyone viewing the README on GitHub. Commit an actual asset to the repo (e.g.,assets/banner.png) and reference it with a relative path. -
Comment text mismatch — Line 4 says "See the 'Graphics by Nano Banana' section below" but the actual section heading (Line 608) is "Graphics & Assets by Nano Banana".
📝 Proposed fix
<p align="center">
- <!-- Tip: Generate a custom banner using the Nano Banana Gemini CLI extension! See the "Graphics by Nano Banana" section below. -->
- <img src="https://placehold.co/1000x300/0d1117/00ADD8/png?text=BrowserNERD&font=roboto" alt="BrowserNERD Banner" width="100%">
+ <!-- Tip: Generate a custom banner using the Nano Banana Gemini CLI extension! See the "Graphics & Assets by Nano Banana" section below. -->
+ <img src="assets/banner.png" alt="BrowserNERD Banner" width="100%">
</p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 3 - 6, Replace the external placeholder image src in
the README's img tag with a relative path to a committed asset (e.g., change the
img src to "assets/banner.png" and add that image file to the repo) to avoid
relying on placehold.co, and update the adjacent comment text ("See the
\"Graphics by Nano Banana\"") to match the actual section heading "Graphics &
Assets by Nano Banana"; locate the README.md img element and the comment text
near it to make these two edits (ensure the asset is committed under
assets/banner.png and the README references that relative path).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b52864335
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cp -r hooks/ "$OUT_DIR/" | ||
| cp -r skills/ "$OUT_DIR/" | ||
| cp -r scripts/ "$OUT_DIR/" | ||
| cp -r mcp-server/gemini-config.yaml "$OUT_DIR/" |
There was a problem hiding this comment.
The release packaging script copies only mcp-server/gemini-config.yaml into the archive root, but the extension manifest starts the server with cwd and --config under ${extensionPath}/mcp-server. In packaged releases this leaves the configured working directory and config path missing, so launching the MCP server from the installed artifact fails immediately.
Useful? React with 👍 / 👎.
| prompt = """Initialize a new BrowserNERD workspace. | ||
|
|
||
| I am executing a shell command to initialize the `.browsernerd` configuration. | ||
| !{mkdir -p .browsernerd && echo "browser:\n headless: false\n viewport_width: 1280\n viewport_height: 720\n\ndocker:\n enabled: false\n log_window: 60s\n" > .browsernerd/config.yaml} |
There was a problem hiding this comment.
The /browser:init command writes YAML via echo "...\n...", but standard shell echo does not reliably expand \n, so the created .browsernerd/config.yaml contains literal backslash-n sequences instead of line breaks. That makes the generated workspace config malformed and causes config loading/parsing to fail for users who initialize via this command.
Useful? React with 👍 / 👎.
- Fix critical bug: browser-act session_fork passed "source_session_id" but ForkSessionTool.Execute() reads "session_id", causing all fork operations via browser-act to silently fail - Fix timer leak: replace time.After with time.NewTimer + defer Stop() in SubscribeRuleTool to prevent memory leaks on early completion - Add 41 new unit tests across two files covering pure-logic helpers at 0% coverage (no Chrome needed): asInt, asInt64, buildObserveSummary, buildMangleSummary, suggestObserveNextStep, compact*, filterInteractive, classifyJSError, formatJSError, selectRecentSessionFacts, and 25+ more - Add 6 Claude Code agent definitions for testing/debugging workflows: go-test-runner, integration-tester, mcp-smoke-tester, mangle-debugger, coverage-analyzer, code-reviewer - MCP package unit coverage: 43.1% → 52.1% (+9pp) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
.claude/agents/mcp-smoke-tester.md (1)
11-33: Make the commands portable (avoid user-specific absolute paths).The hard-coded
/mnt/c/Users/brock/...paths will break for other contributors and CI. Consider using repo-relative paths (from the MCP server directory) or a placeholder variable for the repo root.♻️ Suggested edit (portable commands)
- cd /mnt/c/Users/brock/Documents/Coding\ Projects/BrowserNerd\ MCP/mcp-server && go build -o bin/browsernerd ./cmd/server + cd path/to/browsernerd/mcp-server && go build -o bin/browsernerd ./cmd/server @@ - ls /mnt/c/Users/brock/Documents/Coding\ Projects/BrowserNerd\ MCP/mcp-server/config.yaml + ls config.yaml @@ - cd /mnt/c/Users/brock/Documents/Coding\ Projects/BrowserNerd\ MCP/mcp-server && python scripts/mcp_smoke.py --exe bin/browsernerd --config config.yaml --debug smoke --url https://example.com + cd path/to/browsernerd/mcp-server && python scripts/mcp_smoke.py --exe bin/browsernerd --config config.yaml --debug smoke --url https://example.com @@ - cd /mnt/c/Users/brock/Documents/Coding\ Projects/BrowserNerd\ MCP/mcp-server && python scripts/mcp_smoke.py --exe bin/browsernerd --config config.yaml list + cd path/to/browsernerd/mcp-server && python scripts/mcp_smoke.py --exe bin/browsernerd --config config.yaml list🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/agents/mcp-smoke-tester.md around lines 11 - 33, Update the hard-coded absolute paths in .claude/agents/mcp-smoke-tester.md to use repo-relative paths or a repo-root placeholder so the commands are portable; replace occurrences of "/mnt/c/Users/brock/Documents/Coding Projects/BrowserNerd MCP/mcp-server" with a variable like REPO_ROOT or with relative paths (e.g., change "cd /.../mcp-server && go build -o bin/browsernerd ./cmd/server" to "cd <repo-root>/mcp-server && go build -o bin/browsernerd ./cmd/server" or simply "cd mcp-server && go build ..."), and similarly update the ls/cp and python script invocations (the three commands referencing bin/browsernerd, config.yaml, and scripts/mcp_smoke.py) so they work from the repository root or accept an env var placeholder..claude/agents/mangle-debugger.md (2)
29-35: Make command examples repo-relative (avoid user-specific absolute paths).The
cd /mnt/c/Users/...path is environment-specific and will break for most contributors. Prefer a repo-relative path or a$REPO_ROOTplaceholder.♻️ Proposed fix
- cd /mnt/c/Users/brock/Documents/Coding\ Projects/BrowserNerd\ MCP/mcp-server && SKIP_LIVE_TESTS=1 go test -v -count=1 ./internal/mangle/... + cd mcp-server && SKIP_LIVE_TESTS=1 go test -v -count=1 ./internal/mangle/...- cd /mnt/c/Users/brock/Documents/Coding\ Projects/BrowserNerd\ MCP/mcp-server && SKIP_LIVE_TESTS=1 go test -v -count=1 -run "Fact|Rule|Query" ./internal/mcp/... + cd mcp-server && SKIP_LIVE_TESTS=1 go test -v -count=1 -run "Fact|Rule|Query" ./internal/mcp/...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/agents/mangle-debugger.md around lines 29 - 35, The shell examples use a user-specific absolute path; make them repo-relative or use a $REPO_ROOT placeholder so they work for all contributors. Replace occurrences of "cd /mnt/c/Users/…/mcp-server && SKIP_LIVE_TESTS=1 go test -v -count=1 ./internal/mangle/..." and the fact-tools example with either "cd $REPO_ROOT && SKIP_LIVE_TESTS=1 go test -v -count=1 ./internal/mangle/..." (and similarly for "./internal/mcp/... -run \"Fact|Rule|Query\"") or simply drop the cd and run "SKIP_LIVE_TESTS=1 go test -v -count=1 ./internal/..." from the repo root; update both examples in .claude/agents/mangle-debugger.md.
7-7: Vary the sentence openings for readability.Three consecutive sentences begin with “You …” here. Consider rephrasing one or two to avoid repetition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/agents/mangle-debugger.md at line 7, The three consecutive sentences starting with "You ..." in the opening line ("You are an expert in BrowserNerd's Mangle-based reasoning system. You debug predicate definitions, rule evaluation, fact buffer issues, and schema correctness. You understand both the Go implementation and the `.mg` schema files.") are repetitive; rephrase one or two to vary sentence openings — e.g., change the first to "An expert in BrowserNerd's Mangle-based reasoning system," or rewrite the second as "Debugging responsibilities include predicate definitions, rule evaluation, fact buffer issues, and schema correctness," and/or make the third passive like "Both the Go implementation and the `.mg` schema files are well understood" to improve readability while keeping the same meaning.mcp-server/internal/mcp/progressive_helpers_test.go (1)
463-485: StrengthenargsContainsto handle all common value types in args.The function currently handles strings and
[]map[string]interface{}, but doesn't traverse[]string,[]interface{}, or nested maps. While existing test cases only search for values that the current implementation catches, the helper would fail if args contained an"operations"field as a[]string(like the test recommendation at line 400) and a test usedwantContainsto search it. Consider adding recursive traversal for[]string,[]interface{}, and nested map types to prevent future brittleness.💡 Suggested update
func argsContains(args map[string]interface{}, substr string) bool { - // Walk keys and string values for a simple substring check. - for k, v := range args { - if strings.Contains(strings.ToLower(k), substr) { - return true - } - switch val := v.(type) { - case string: - if strings.Contains(strings.ToLower(val), substr) { - return true - } - case []map[string]interface{}: - for _, inner := range val { - for _, iv := range inner { - if s, ok := iv.(string); ok && strings.Contains(strings.ToLower(s), substr) { - return true - } - } - } - } - } - return false + needle := strings.ToLower(substr) + var walk func(v interface{}) bool + walk = func(v interface{}) bool { + switch val := v.(type) { + case string: + return strings.Contains(strings.ToLower(val), needle) + case map[string]interface{}: + for k, v2 := range val { + if strings.Contains(strings.ToLower(k), needle) || walk(v2) { + return true + } + } + case []interface{}: + for _, v2 := range val { + if walk(v2) { + return true + } + } + case []string: + for _, s := range val { + if strings.Contains(strings.ToLower(s), needle) { + return true + } + } + case []map[string]interface{}: + for _, m := range val { + if walk(m) { + return true + } + } + } + return false + } + for k, v := range args { + if strings.Contains(strings.ToLower(k), needle) || walk(v) { + return true + } + } + return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp-server/internal/mcp/progressive_helpers_test.go` around lines 463 - 485, The argsContains helper is too narrow—extend argsContains to recursively traverse and check common value types: handle map[string]interface{} (nested maps), []string, []interface{} (iterating and recursing), in addition to the existing string and []map[string]interface{} cases; for each visited key/value convert strings to lowercase and use strings.Contains against substr, and for slice or map elements recurse into the same checking logic so any nested string (or slice of strings) will be found (update the argsContains function to implement this recursive traversal).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/agents/coverage-analyzer.md:
- Around line 12-29: In .claude/agents/coverage-analyzer.md the four bash
examples hardcode a developer-local absolute WSL path; update each "cd
/mnt/c/Users/brock/.../mcp-server && ..." line to use a repo-relative path
(e.g., "cd mcp-server && ...") so steps 1–4 (the unit, per-package, HTML and
full coverage commands) work for any contributor/CI and avoid leaking the
username/PII.
In @.claude/agents/go-test-runner.md:
- Around line 11-24: The three shell examples embed a hardcoded WSL path
(/mnt/c/Users/brock/Documents/Coding\ Projects/BrowserNerd\ MCP/mcp-server)
which breaks portability and leaks a username; update each command (the lines
that start with "cd /mnt/c/Users/brock... && go build ./..." and the two "cd
/mnt/c/Users/brock... && SKIP_LIVE_TESTS=1 go test..." examples) to use a
repo-relative path (e.g., cd mcp-server && ...) or a standard environment
variable (e.g., "${REPO_ROOT}/mcp-server") instead, ensuring the rest of the
command (go build ./..., SKIP_LIVE_TESTS=1 go test -v -count=1 ./..., and
package-specific ./internal/mangle/...) remains unchanged.
---
Nitpick comments:
In @.claude/agents/mangle-debugger.md:
- Around line 29-35: The shell examples use a user-specific absolute path; make
them repo-relative or use a $REPO_ROOT placeholder so they work for all
contributors. Replace occurrences of "cd /mnt/c/Users/…/mcp-server &&
SKIP_LIVE_TESTS=1 go test -v -count=1 ./internal/mangle/..." and the fact-tools
example with either "cd $REPO_ROOT && SKIP_LIVE_TESTS=1 go test -v -count=1
./internal/mangle/..." (and similarly for "./internal/mcp/... -run
\"Fact|Rule|Query\"") or simply drop the cd and run "SKIP_LIVE_TESTS=1 go test
-v -count=1 ./internal/..." from the repo root; update both examples in
.claude/agents/mangle-debugger.md.
- Line 7: The three consecutive sentences starting with "You ..." in the opening
line ("You are an expert in BrowserNerd's Mangle-based reasoning system. You
debug predicate definitions, rule evaluation, fact buffer issues, and schema
correctness. You understand both the Go implementation and the `.mg` schema
files.") are repetitive; rephrase one or two to vary sentence openings — e.g.,
change the first to "An expert in BrowserNerd's Mangle-based reasoning system,"
or rewrite the second as "Debugging responsibilities include predicate
definitions, rule evaluation, fact buffer issues, and schema correctness,"
and/or make the third passive like "Both the Go implementation and the `.mg`
schema files are well understood" to improve readability while keeping the same
meaning.
In @.claude/agents/mcp-smoke-tester.md:
- Around line 11-33: Update the hard-coded absolute paths in
.claude/agents/mcp-smoke-tester.md to use repo-relative paths or a repo-root
placeholder so the commands are portable; replace occurrences of
"/mnt/c/Users/brock/Documents/Coding Projects/BrowserNerd MCP/mcp-server" with a
variable like REPO_ROOT or with relative paths (e.g., change "cd /.../mcp-server
&& go build -o bin/browsernerd ./cmd/server" to "cd <repo-root>/mcp-server && go
build -o bin/browsernerd ./cmd/server" or simply "cd mcp-server && go build
..."), and similarly update the ls/cp and python script invocations (the three
commands referencing bin/browsernerd, config.yaml, and scripts/mcp_smoke.py) so
they work from the repository root or accept an env var placeholder.
In `@mcp-server/internal/mcp/progressive_helpers_test.go`:
- Around line 463-485: The argsContains helper is too narrow—extend argsContains
to recursively traverse and check common value types: handle
map[string]interface{} (nested maps), []string, []interface{} (iterating and
recursing), in addition to the existing string and []map[string]interface{}
cases; for each visited key/value convert strings to lowercase and use
strings.Contains against substr, and for slice or map elements recurse into the
same checking logic so any nested string (or slice of strings) will be found
(update the argsContains function to implement this recursive traversal).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.claude/agents/code-reviewer.md.claude/agents/coverage-analyzer.md.claude/agents/go-test-runner.md.claude/agents/integration-tester.md.claude/agents/mangle-debugger.md.claude/agents/mcp-smoke-tester.mdmcp-server/internal/mcp/fact_tools.gomcp-server/internal/mcp/progressive_helpers_test.gomcp-server/internal/mcp/progressive_tools.gomcp-server/internal/mcp/resources_test.go
✅ Files skipped from review due to trivial changes (4)
- mcp-server/internal/mcp/resources_test.go
- .claude/agents/code-reviewer.md
- mcp-server/internal/mcp/fact_tools.go
- .claude/agents/integration-tester.md
| ```bash | ||
| cd /mnt/c/Users/brock/Documents/Coding\ Projects/BrowserNerd\ MCP/mcp-server && SKIP_LIVE_TESTS=1 go test -coverprofile=coverage_unit.out -covermode=count ./... | ||
| ``` | ||
|
|
||
| 2. **View per-package coverage**: | ||
| ```bash | ||
| cd /mnt/c/Users/brock/Documents/Coding\ Projects/BrowserNerd\ MCP/mcp-server && go tool cover -func=coverage_unit.out | tail -20 | ||
| ``` | ||
|
|
||
| 3. **Generate HTML report** (if requested): | ||
| ```bash | ||
| cd /mnt/c/Users/brock/Documents/Coding\ Projects/BrowserNerd\ MCP/mcp-server && go tool cover -html=coverage_unit.out -o coverage.html | ||
| ``` | ||
|
|
||
| 4. **Run full coverage** (with integration tests, needs Chrome): | ||
| ```bash | ||
| cd /mnt/c/Users/brock/Documents/Coding\ Projects/BrowserNerd\ MCP/mcp-server && unset SKIP_LIVE_TESTS && go test -coverprofile=coverage_full.out -covermode=count -timeout 120s ./... | ||
| ``` |
There was a problem hiding this comment.
Remove hardcoded developer-local absolute paths — agent is unusable for any other contributor.
All four cd commands hard-wire a WSL path specific to the original author's machine (/mnt/c/Users/brock/...). Any team member (or CI runner) that invokes this agent will get an immediate cd: no such file or directory and every subsequent command will silently run in the wrong directory. The embedded username is also a minor PII leak that should be kept out of committed docs.
Replace each occurrence with a path relative to the repository root so the agent works regardless of where the repo is checked out.
🔧 Proposed fix — use relative paths
1. **Run unit-only coverage**:
```bash
- cd /mnt/c/Users/brock/Documents/Coding\ Projects/BrowserNerd\ MCP/mcp-server && SKIP_LIVE_TESTS=1 go test -coverprofile=coverage_unit.out -covermode=count ./...
+ cd mcp-server && SKIP_LIVE_TESTS=1 go test -coverprofile=coverage_unit.out -covermode=count ./...
```
2. **View per-package coverage**:
```bash
- cd /mnt/c/Users/brock/Documents/Coding\ Projects/BrowserNerd\ MCP/mcp-server && go tool cover -func=coverage_unit.out | tail -20
+ cd mcp-server && go tool cover -func=coverage_unit.out | tail -20
```
3. **Generate HTML report** (if requested):
```bash
- cd /mnt/c/Users/brock/Documents/Coding\ Projects/BrowserNerd\ MCP/mcp-server && go tool cover -html=coverage_unit.out -o coverage.html
+ cd mcp-server && go tool cover -html=coverage_unit.out -o coverage.html
```
4. **Run full coverage** (with integration tests, needs Chrome):
```bash
- cd /mnt/c/Users/brock/Documents/Coding\ Projects/BrowserNerd\ MCP/mcp-server && unset SKIP_LIVE_TESTS && go test -coverprofile=coverage_full.out -covermode=count -timeout 120s ./...
+ cd mcp-server && unset SKIP_LIVE_TESTS && go test -coverprofile=coverage_full.out -covermode=count -timeout 120s ./...
```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/agents/coverage-analyzer.md around lines 12 - 29, In
.claude/agents/coverage-analyzer.md the four bash examples hardcode a
developer-local absolute WSL path; update each "cd
/mnt/c/Users/brock/.../mcp-server && ..." line to use a repo-relative path
(e.g., "cd mcp-server && ...") so steps 1–4 (the unit, per-package, HTML and
full coverage commands) work for any contributor/CI and avoid leaking the
username/PII.
| 1. **Build first** to catch compile errors: | ||
| ```bash | ||
| cd /mnt/c/Users/brock/Documents/Coding\ Projects/BrowserNerd\ MCP/mcp-server && go build ./... | ||
| ``` | ||
|
|
||
| 2. **Run unit tests** (no browser needed): | ||
| ```bash | ||
| cd /mnt/c/Users/brock/Documents/Coding\ Projects/BrowserNerd\ MCP/mcp-server && SKIP_LIVE_TESTS=1 go test -v -count=1 ./... | ||
| ``` | ||
|
|
||
| 3. **Run specific package** if directed: | ||
| ```bash | ||
| cd /mnt/c/Users/brock/Documents/Coding\ Projects/BrowserNerd\ MCP/mcp-server && SKIP_LIVE_TESTS=1 go test -v -count=1 ./internal/mangle/... | ||
| ``` |
There was a problem hiding this comment.
Hardcoded personal WSL path breaks portability and leaks a username.
All three cd commands embed /mnt/c/Users/brock/Documents/Coding\ Projects/BrowserNerd\ MCP/mcp-server — a path that exists only on one developer's WSL environment. Any other contributor or CI runner will fail immediately with "no such file or directory." The username brock also constitutes personal information committed to a public repository.
Replace the absolute prefix with a path relative to the repo root (the agent is already invoked from that context), or use a well-known environment variable:
🔧 Proposed fix — use repo-relative paths
1. **Build first** to catch compile errors:
```bash
- cd /mnt/c/Users/brock/Documents/Coding\ Projects/BrowserNerd\ MCP/mcp-server && go build ./...
+ cd mcp-server && go build ./...
```
2. **Run unit tests** (no browser needed):
```bash
- cd /mnt/c/Users/brock/Documents/Coding\ Projects/BrowserNerd\ MCP/mcp-server && SKIP_LIVE_TESTS=1 go test -v -count=1 ./...
+ cd mcp-server && SKIP_LIVE_TESTS=1 go test -v -count=1 ./...
```
3. **Run specific package** if directed:
```bash
- cd /mnt/c/Users/brock/Documents/Coding\ Projects/BrowserNerd\ MCP/mcp-server && SKIP_LIVE_TESTS=1 go test -v -count=1 ./internal/mangle/...
+ cd mcp-server && SKIP_LIVE_TESTS=1 go test -v -count=1 ./internal/mangle/...
```📝 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.
| 1. **Build first** to catch compile errors: | |
| ```bash | |
| cd /mnt/c/Users/brock/Documents/Coding\ Projects/BrowserNerd\ MCP/mcp-server && go build ./... | |
| ``` | |
| 2. **Run unit tests** (no browser needed): | |
| ```bash | |
| cd /mnt/c/Users/brock/Documents/Coding\ Projects/BrowserNerd\ MCP/mcp-server && SKIP_LIVE_TESTS=1 go test -v -count=1 ./... | |
| ``` | |
| 3. **Run specific package** if directed: | |
| ```bash | |
| cd /mnt/c/Users/brock/Documents/Coding\ Projects/BrowserNerd\ MCP/mcp-server && SKIP_LIVE_TESTS=1 go test -v -count=1 ./internal/mangle/... | |
| ``` | |
| 1. **Build first** to catch compile errors: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/agents/go-test-runner.md around lines 11 - 24, The three shell
examples embed a hardcoded WSL path (/mnt/c/Users/brock/Documents/Coding\
Projects/BrowserNerd\ MCP/mcp-server) which breaks portability and leaks a
username; update each command (the lines that start with "cd
/mnt/c/Users/brock... && go build ./..." and the two "cd /mnt/c/Users/brock...
&& SKIP_LIVE_TESTS=1 go test..." examples) to use a repo-relative path (e.g., cd
mcp-server && ...) or a standard environment variable (e.g.,
"${REPO_ROOT}/mcp-server") instead, ensuring the rest of the command (go build
./..., SKIP_LIVE_TESTS=1 go test -v -count=1 ./..., and package-specific
./internal/mangle/...) remains unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a59a0bdfd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const prebuiltName = os === 'win32' | ||
| ? `browsernerd-windows-${arch}.exe` | ||
| : `browsernerd-${os}-${arch}`; |
There was a problem hiding this comment.
postinstall looks for browsernerd-windows-${arch}.exe on Windows, but the packaging path produces browsernerd-win32-x64.exe (from BIN_DST="...browsernerd-${PLATFORM}-${ARCH}" with PLATFORM=win32 in the release workflow). This mismatch means Windows installs never detect the shipped prebuilt binary and always fall back to go build, so users without Go end up with a non-working install despite downloading a release bundle.
Useful? React with 👍 / 👎.
| prompt = """Please analyze the visual state of the browser. | ||
| I am using BrowserNERD to capture a screenshot of the current page. | ||
|
|
||
| !{gemini mcp call browsernerd browser-observe '{"mode": "screenshot", "view": "summary"}'} |
There was a problem hiding this comment.
The /browser:look command invokes browser-observe with {"mode":"screenshot"} but does not pass a session_id. In BrowserObserveTool.Execute, screenshot mode explicitly returns an error when session_id is empty, so this command reliably fails instead of producing the screenshot-based analysis it promises.
Useful? React with 👍 / 👎.
| {"operations": [{"type": "session_create", "url": "https://example.com"}, {"type": "await_stable", "timeout_ms": 10000}]} | ||
|
|
||
| // Example: Single Click | ||
| {"operations": [{"type": "click", "ref": "btn_123"}]} |
There was a problem hiding this comment.
This guide tells agents to send browser-act operations like {"type":"click"}, but browser-act only accepts types such as interact, fill, and key (and returns unknown operation type otherwise). Because this file is the extension context prompt, these incorrect examples steer normal agent flows toward invalid tool calls and failed actions.
Useful? React with 👍 / 👎.
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores
Tests