fix: stabilize LCO 1.6 for Hermes-first MCP - #792
Conversation
📝 WalkthroughWalkthroughThe PR makes Hermes the primary stdio MCP path, fixes notification and structured-content handling, makes ChangesHermes MCP stabilization
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
evaOS review status: completedPR: #792 - fix: stabilize LCO 1.6 for Hermes-first MCP evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #792 Review URL: #792 (review) |
coderabbitai
Bot
added
codex
![evaos-code-review-bot[bot]](https://avatars.githubusercontent.com/in/4184532?s=60&v=4){kind=small}
There was a problem hiding this comment.
Walkthrough
PR: #792 - fix: stabilize LCO 1.6 for Hermes-first MCP
Head: edcdc78c89364b3f3cd3aeacd56f906ae950e3f4 into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 5/5 (~70 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
README.md |
modified | +45/-14 | Documentation | Low |
docs/BETA_RELEASE_RUNBOOK.md |
modified | +26/-4 | Documentation | Low |
docs/CLAIM_AUDIT.md |
modified | +14/-1 | Documentation | Low |
docs/HERMES_ADAPTER_BOUNDARY.md |
modified | +15/-13 | Documentation | Low |
docs/RELEASE_CHECKLIST.md |
modified | +14/-0 | Documentation | Low |
docs/SETUP.md |
modified | +46/-1 | Documentation | Low |
docs/releases/CHANGELOG.md |
modified | +2/-1 | Documentation | Low |
docs/releases/RELEASE_NOTES_1.6.0.md |
modified | +15/-6 | Documentation | Low |
package.json |
modified | +1/-1 | Configuration | Low |
packages/cli/src/cli-mcp-product-smoke.ts |
modified | +63/-7 | Changed file | Moderate: validated P3 finding |
packages/cli/src/hermes-readiness.ts |
added | +177/-0 | Changed file | Moderate: validated P3 finding |
packages/cli/src/hermes-smoke.ts |
added | +160/-0 | Changed file | Low |
packages/cli/src/main.ts |
modified | +180/-0 | Changed file | Low |
packages/core/src/index.ts |
modified | +7/-1 | Changed file | Low |
packages/mcp-server/src/mcp-protocol.ts |
added | +15/-0 | Changed file | Low |
packages/mcp-server/src/server-runtime.ts |
modified | +22/-3 | Changed file | Low |
packages/mcp-server/src/tools.ts |
modified | +9/-2 | Changed file | Low |
packages/openclaw-plugin/openclaw.plugin.json |
modified | +2/-2 | Changed file | Low |
tests/cli-find.test.ts |
modified | +49/-2 | Test coverage | Low |
tests/cli-help.test.ts |
modified | +23/-0 | Test coverage | Low |
tests/cli-mcp-product-smoke.test.ts |
modified | +9/-9 | Test coverage | Low |
tests/hermes-readiness.test.ts |
added | +142/-0 | Test coverage | Low |
tests/hermes-smoke.test.ts |
added | +202/-0 | Test coverage | Elevated: large change |
tests/lco-first-docs.test.ts |
modified | +3/-3 | Test coverage | Low |
tests/mcp-hermes-contract.test.ts |
added | +145/-0 | Test coverage | Low |
1 additional changed files omitted from this walkthrough.
Review Signal
Validated inline findings: 2 (P0: 0, P1: 0, P2: 0, P3: 2).
Dropped findings before posting: 1. High-severity findings: 0.
Risk Taxonomy
- Flaky test risk: 1
- Migration: 1
Validation and Proof
2 required validation/proof recommendation(s) selected from changed files.
- required: TypeScript/web build or CI proof - Runtime TypeScript/web files or package/config files changed. Proof: npm run build; typecheck; focused Vitest; green GitHub check.
- required: CI/release smoke proof - CI, release, launchd, or package metadata changed. Proof: green GitHub check; release-status; coverage-audit; rollback note.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Call out evidence leakage, replay/collision risks, duplicate side effects, and brittle sanitizer logic.
Profile proof expectations: Look for focused sanitizer, signature, orchestration, or fixture proof.
Related Context
Related issues/PRs: #785, #786, #788, #790, #791, #673.
Suggested labels: docs, tests.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions: none
- Label suggestions: orchestration, security, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
![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: edcdc78c89
ℹ️ 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".
evaOS review status: completedPR: #792 - fix: stabilize LCO 1.6 for Hermes-first MCP evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #792 Review URL: #792 (review) |
There was a problem hiding this comment.
Walkthrough
PR: #792 - fix: stabilize LCO 1.6 for Hermes-first MCP
Head: ef7a13821405e45af347eca028071dff0005581a into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 5/5 (~70 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
README.md |
modified | +45/-14 | Documentation | Low |
docs/BETA_RELEASE_RUNBOOK.md |
modified | +26/-4 | Documentation | Low |
docs/CLAIM_AUDIT.md |
modified | +14/-1 | Documentation | Low |
docs/HERMES_ADAPTER_BOUNDARY.md |
modified | +15/-13 | Documentation | Low |
docs/RELEASE_CHECKLIST.md |
modified | +14/-0 | Documentation | Low |
docs/SETUP.md |
modified | +46/-1 | Documentation | Low |
docs/releases/CHANGELOG.md |
modified | +2/-1 | Documentation | Low |
docs/releases/RELEASE_NOTES_1.6.0.md |
modified | +15/-6 | Documentation | Low |
package.json |
modified | +1/-1 | Configuration | Low |
packages/cli/src/cli-mcp-product-smoke.ts |
modified | +69/-7 | Changed file | Moderate: validated P3 finding |
packages/cli/src/hermes-readiness.ts |
added | +189/-0 | Changed file | Low |
packages/cli/src/hermes-smoke.ts |
added | +160/-0 | Changed file | Moderate: validated P2 finding |
packages/cli/src/main.ts |
modified | +180/-0 | Changed file | Low |
packages/core/src/index.ts |
modified | +7/-1 | Changed file | Low |
packages/mcp-server/src/mcp-protocol.ts |
added | +15/-0 | Changed file | Low |
packages/mcp-server/src/server-runtime.ts |
modified | +22/-3 | Changed file | Moderate: validated P3 finding |
packages/mcp-server/src/tools.ts |
modified | +9/-2 | Changed file | Moderate: validated P2 finding |
packages/openclaw-plugin/openclaw.plugin.json |
modified | +2/-2 | Changed file | Low |
tests/cli-find.test.ts |
modified | +49/-2 | Test coverage | Low |
tests/cli-help.test.ts |
modified | +23/-0 | Test coverage | Low |
tests/cli-mcp-product-smoke.test.ts |
modified | +61/-9 | Test coverage | Low |
tests/hermes-readiness.test.ts |
added | +149/-0 | Test coverage | Low |
tests/hermes-smoke.test.ts |
added | +202/-0 | Test coverage | Elevated: large change |
tests/lco-first-docs.test.ts |
modified | +3/-3 | Test coverage | Low |
tests/mcp-hermes-contract.test.ts |
added | +145/-0 | Test coverage | Low |
1 additional changed files omitted from this walkthrough.
Review Signal
Validated inline findings: 4 (P0: 0, P1: 0, P2: 2, P3: 2).
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
- API compatibility: 1
- Flaky test risk: 1
- Runtime correctness: 1
- Security boundary: 1
Validation and Proof
2 required validation/proof recommendation(s) selected from changed files.
- required: TypeScript/web build or CI proof - Runtime TypeScript/web files or package/config files changed. Proof: npm run build; typecheck; focused Vitest; green GitHub check.
- required: CI/release smoke proof - CI, release, launchd, or package metadata changed. Proof: green GitHub check; release-status; coverage-audit; rollback note.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Call out evidence leakage, replay/collision risks, duplicate side effects, and brittle sanitizer logic.
Profile proof expectations: Look for focused sanitizer, signature, orchestration, or fixture proof.
Related Context
Related issues/PRs: #785, #786, #788, #790, #791, #673.
Suggested labels: docs, tests.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions: none
- Label suggestions: orchestration, security, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/cli-mcp-product-smoke.test.ts (1)
285-316: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInline
toolCallProbetype is stale, forcing a manual cast.The test's local narrowing type only declares
toolName/ok/contentItemCount/contentKinds/structuredContentPresent, so checkingstructuredContentObjectneeds a cast at L315. Widening the inline type to include the newerToolCallProbefields (structuredContentObject,structuredContentWrappedResult,reasonCodes,durationMs) would drop the cast and keep the test in sync with the real shape.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cli-mcp-product-smoke.test.ts` around lines 285 - 316, The inline toolCallProbe type in the test is missing newer ToolCallProbe fields. Update that type to include structuredContentObject, structuredContentWrappedResult, reasonCodes, and durationMs, then access structuredContentObject directly and remove the manual cast.packages/cli/src/cli-mcp-product-smoke.ts (1)
217-226: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winEarly
resolve()bypassesfinish(), leaving notification-silence fields unset.The
runtimeRootFactory()throw path resolves directly instead of routing throughfinish(), sonotificationSilenceReady/invalidNotificationResponseCountare never attached to this result. Downstream,createCliMcpProductSmokeReporttreats a missingnotificationSilenceReadyastrue(mcpProbe.notificationSilenceReady !== false), so a setup failure silently reports "notification silence proven" in evidence even though notification behavior was never exercised. Overallokstill fails (viamcpReady), but the individual field is misleading for anyone auditing the JSON evidence.🩹 Proposed fix
} catch { - resolve({ - ...packageDefect("mcp_isolated_runtime_setup_failed"), - tools: [], - toolCall: failedToolCall(toolCallName, "mcp_isolated_runtime_setup_failed") - }); + resolve({ + ...packageDefect("mcp_isolated_runtime_setup_failed"), + tools: [], + toolCall: failedToolCall(toolCallName, "mcp_isolated_runtime_setup_failed"), + notificationSilenceReady: false, + invalidNotificationResponseCount: 0 + }); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/cli-mcp-product-smoke.ts` around lines 217 - 226, Route the runtimeRootFactory() failure through finish() instead of resolving directly, ensuring the resulting mcp probe includes notificationSilenceReady and invalidNotificationResponseCount. Preserve the existing mcp_isolated_runtime_setup_failed defect, empty tools, and failed tool call while making the failure explicitly indicate notification silence was not proven.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/BETA_RELEASE_RUNBOOK.md`:
- Around line 253-274: Update the Hermes smoke documentation around the
“isolated Hermes client canary” to say it uses an isolated temporary database,
not a protected copy of the local LCO database. Keep the existing cleanup and
shared-evidence restrictions, and do not introduce a separate manual validation
flow.
In `@packages/cli/src/hermes-smoke.ts`:
- Around line 79-99: Update the Hermes smoke search measurement around
createCliMcpProductSmokeReport so the latency threshold reflects a warm
existing-index lco_find search: perform a throwaway lco_find warm-up in the same
MCP runtime before recording the representative find duration, or consistently
rename the threshold and related readiness/reporting symbols to describe cold
first-query latency.
- Line 1: Update createCliMcpProductSmokeReport so the recorded findLatencyMs
measures a warm search by performing a throwaway lco_find tools/call before
starting the timer, while keeping the existing timed search and report behavior
unchanged.
In `@packages/mcp-server/src/mcp-protocol.ts`:
- Around line 9-11: Update serializeMcpTextContent to catch JSON.stringify
failures for non-serializable results such as BigInt or circular references, and
return a safe fallback string instead of propagating the exception. Preserve the
existing pretty-printed JSON output and null fallback for successfully handled
values.
In `@packages/mcp-server/src/server-runtime.ts`:
- Line 77: Remove the unreachable responseAllowed guard from the catch block in
the server runtime error-handling flow. Keep the existing earlier early-return
logic and simplify the catch path so it proceeds directly with its error
handling.
In `@README.md`:
- Around line 371-373: Update the README section structure around the Hermes
configuration to make Hermes discoverable from a heading: rename the existing
“OpenClaw And MCP” heading to include Hermes, or create a dedicated Hermes
section while preserving the setup content and clear public entrypoints.
In `@tests/hermes-smoke.test.ts`:
- Around line 55-83: Add a separate Hermes smoke test using fakeHermesMcpServer
and configure it to proactively emit a genuine notification without an id
property, such as a log message, during the flow. Assert that
createHermesSmokeReport does not classify this message as
mcp_notification_response_invalid and preserves the intended
notificationSilenceReady outcome, while retaining the existing id:null violation
test unchanged.
In `@tests/release-claim-audit.test.ts`:
- Around line 194-201: Update the README version extraction logic near the
existing `Current stable:` matcher in `tests/release-claim-audit.test.ts` to
recognize `Published stable at this candidate snapshot:` followed by the version
value. Ensure subsequent release-claim assertions use the extracted README
version rather than falling back to `packageVersion`, or reuse a single shared
version source if that is the established pattern.
In `@VISION.md`:
- Around line 18-24: Update the Tier 1 Hermes candidate-release criteria in
VISION.md to replace “default indexed search” with “default existing-index
search without refresh.” Preserve the surrounding initialization, notification,
tool-registration, structured-result, and latency requirements, and align the
wording with the MCP tools configuration using indexDecision:
"skipped_by_default".
---
Outside diff comments:
In `@packages/cli/src/cli-mcp-product-smoke.ts`:
- Around line 217-226: Route the runtimeRootFactory() failure through finish()
instead of resolving directly, ensuring the resulting mcp probe includes
notificationSilenceReady and invalidNotificationResponseCount. Preserve the
existing mcp_isolated_runtime_setup_failed defect, empty tools, and failed tool
call while making the failure explicitly indicate notification silence was not
proven.
In `@tests/cli-mcp-product-smoke.test.ts`:
- Around line 285-316: The inline toolCallProbe type in the test is missing
newer ToolCallProbe fields. Update that type to include structuredContentObject,
structuredContentWrappedResult, reasonCodes, and durationMs, then access
structuredContentObject directly and remove the manual cast.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8c782ade-029c-41dc-950a-2226efe87c78
📒 Files selected for processing (28)
README.mdVISION.mddocs/BETA_RELEASE_RUNBOOK.mddocs/CLAIM_AUDIT.mddocs/HERMES_ADAPTER_BOUNDARY.mddocs/RELEASE_CHECKLIST.mddocs/SETUP.mddocs/releases/CHANGELOG.mddocs/releases/RELEASE_NOTES_1.6.0.mdopenclaw.plugin.jsonpackage.jsonpackages/cli/src/cli-mcp-product-smoke.tspackages/cli/src/hermes-readiness.tspackages/cli/src/hermes-smoke.tspackages/cli/src/main.tspackages/core/src/index.tspackages/mcp-server/src/mcp-protocol.tspackages/mcp-server/src/server-runtime.tspackages/mcp-server/src/tools.tspackages/openclaw-plugin/openclaw.plugin.jsontests/cli-find.test.tstests/cli-help.test.tstests/cli-mcp-product-smoke.test.tstests/hermes-readiness.test.tstests/hermes-smoke.test.tstests/lco-first-docs.test.tstests/mcp-hermes-contract.test.tstests/release-claim-audit.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: test
🧰 Additional context used
📓 Path-based instructions (2)
docs/SETUP.md
📄 CodeRabbit inference engine (AGENTS.md)
Keep detailed first-run instructions in
docs/SETUP.md.
Files:
docs/SETUP.md
README.md
📄 CodeRabbit inference engine (AGENTS.md)
README.md: KeepREADME.mdas the public landing page: product value, install, setup, first workflow, OpenClaw/MCP entrypoints, safety boundaries, and links.
Do not put active sprint ledgers, long issue history, exhaustive release-gate command inventories, or internal agent maintenance loops into the publicREADME.md; linkVISION.md, GitHub trackers, or runbooks instead.
Files:
README.md
🪛 ast-grep (0.45.0)
tests/mcp-hermes-contract.test.ts
[warning] 1-1: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
packages/cli/src/cli-mcp-product-smoke.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🪛 LanguageTool
VISION.md
[typographical] ~31-~31: To join two clauses or introduce examples, consider using an em dash.
Context: ...nly the compatibility surface. Tier 3 - Generic MCP (any harness). Any other M...
(DASH_RULE)
README.md
[style] ~180-~180: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...s the active prerelease train. - next is reserved for release candidates. - npm ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (34)
README.md (1)
10-11: LGTM!Also applies to: 106-107, 128-129, 145-146, 174-182, 307-307, 331-338, 384-386, 391-393
VISION.md (1)
7-9: LGTM!Also applies to: 26-30, 54-64, 77-84, 93-96
docs/HERMES_ADAPTER_BOUNDARY.md (1)
5-9: LGTM!Also applies to: 21-21, 30-34, 46-50
docs/SETUP.md (3)
314-316: LGTM!Also applies to: 337-340, 351-353, 355-362, 373-378
341-349: 🎯 Functional CorrectnessVerify the duplicated Hermes database-path examples consistently. YAML environment values containing
~are not automatically shell-expanded; confirm Hermes or LCO expands the path before opening the database.
docs/SETUP.md#L341-L349: document a supported absolute-path or expansion form if needed.README.md#L374-L382: keep the landing-page example identical to the verified setup form.
364-370: 🩺 Stability & AvailabilityEnsure both readiness examples document the required package-smoke producer. The readiness implementation requires
--package-smoke, while the shown Hermes smoke command produces onlyhermes-smoke.json.
docs/SETUP.md#L364-L370: add or reference the command that creates/tmp/lco-package-smoke/cli-mcp-smoke.json.README.md#L387-L388: add the same prerequisite or point to a guaranteed existing artifact.package.json (1)
4-4: LGTM!tests/lco-first-docs.test.ts (1)
92-99: LGTM!tests/mcp-hermes-contract.test.ts (2)
1-2: Static-analysischild_processwarning is a false positive.
spawnis invoked with a fixedprocess.execPathand an argument array (no shell concatenation of untrusted input) — already the safe pattern the rule recommends.Source: Linters/SAST tools
13-22: LGTM! Thorough end-to-end coverage of notification suppression, structured-content normalization, and error-code behavior over real stdio.Also applies to: 24-145
packages/mcp-server/src/mcp-protocol.ts (1)
1-7: LGTM!Also applies to: 13-15
packages/mcp-server/src/server-runtime.ts (2)
39-48: LGTM! Cleanly distinguishes notifications from requests and prevents notification-based tool dispatch, matching the#786objective.
232-240: LGTM!packages/mcp-server/src/tools.ts (1)
569-569: LGTM!indexDecisiontri-state cleanly matches the opt-in indexing contract and is exercised by the newcli-find.test.tscoverage.Also applies to: 582-588, 613-613
packages/core/src/index.ts (2)
17856-17856: LGTM!Also applies to: 17914-17914
17872-17876: 🎯 Functional CorrectnessNo change needed
indexDecision === "requested"cannot reach the fallback here:indexedis only set whenshouldIndexis true, andcreateRecallIndexSummary()reportsattempted: truewhenever either indexer runs. The fallback only coversskipped_by_flagandskipped_by_default.> Likely an incorrect or invalid review comment.packages/openclaw-plugin/openclaw.plugin.json (1)
202-202: LGTM! Description text matches the new opt-in indexing behavior implemented intools.ts.Also applies to: 2023-2023
openclaw.plugin.json (1)
202-202: LGTM! Matches the parallel update inpackages/openclaw-plugin/openclaw.plugin.json.Also applies to: 2023-2023
tests/cli-find.test.ts (1)
304-330: LGTM! Solid coverage distinguishing default-skip, explicit-flag-skip, and explicit-index behaviors, with a DB-level assertion backing the "no write" guarantee.Also applies to: 346-369, 383-427
docs/releases/CHANGELOG.md (1)
7-8: LGTM! Correctly keeps the unreleased1.6.0candidate out of the released-version list per the file's stated policy.docs/releases/RELEASE_NOTES_1.6.0.md (1)
3-9: LGTM! Release copy accurately reflects the Hermes notification/structured-content/indexing changes and correctly frames 1.6.0 as a candidate, not a published release.Also applies to: 40-42, 65-67, 80-81
packages/cli/src/cli-mcp-product-smoke.ts (3)
451-480: LGTM on structured-content/reason-code derivation.Object/wrapped-result detection, name-mismatch precedence, and the reason-code allowlist regex all look correct and consistent with the PR's structured-content contract.
7-113: LGTM ontoolCallArgumentsplumbing anddurationMscapture.Argument passthrough and duration measurement around the
tools/callwrite/response are wired correctly.Also applies to: 200-215, 364-390, 443-449
316-329: 🎯 Functional CorrectnessDrop this warning: the guard only counts response-shaped messages, and the MCP server only writes id-bearing responses to stdout.
> Likely an incorrect or invalid review comment.packages/cli/src/hermes-smoke.ts (1)
100-152: LGTM on blocker aggregation and report shape.Prefixing, dedupe, and gating (
ok = find.ok && extract.ok && blockers.length===0 && setupBlockers.length===0) look correct and consistent with the required-tools/notification/structured-content checks.packages/cli/src/hermes-readiness.ts (1)
45-153: LGTM!packages/cli/src/main.ts (1)
973-983: LGTM!Also applies to: 1081-1091, 2168-2197, 4510-4633
tests/hermes-smoke.test.ts (1)
1-53: LGTM!Also applies to: 85-155
tests/hermes-readiness.test.ts (1)
1-143: LGTM!tests/cli-help.test.ts (1)
654-676: LGTM!tests/cli-mcp-product-smoke.test.ts (1)
31-94: LGTM!Also applies to: 125-125
docs/BETA_RELEASE_RUNBOOK.md (1)
39-40: LGTM!Also applies to: 275-278
docs/CLAIM_AUDIT.md (1)
5-12: LGTM!Also applies to: 53-58
docs/RELEASE_CHECKLIST.md (1)
22-30: LGTM!Also applies to: 53-53, 72-73
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/cli-mcp-product-smoke.ts (1)
128-130: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFail closed when notification silence was not measured.
If
runtimeRootFactory()fails,probeMcpToolsListAndCall()resolves withoutnotificationSilenceReady;!== falsethen reports silence as ready despite no MCP process being observed. Require an explicittrue.Proposed fix
- const notificationSilenceReady = mcpProbe.notificationSilenceReady !== false; + const notificationSilenceReady = mcpProbe.notificationSilenceReady === true;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/cli-mcp-product-smoke.ts` around lines 128 - 130, Update the notificationSilenceReady assignment in the MCP smoke-test readiness flow to require mcpProbe.notificationSilenceReady === true, so missing measurements fail closed while explicit readiness remains accepted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/cli/src/cli-mcp-product-smoke.ts`:
- Around line 128-130: Update the notificationSilenceReady assignment in the MCP
smoke-test readiness flow to require mcpProbe.notificationSilenceReady === true,
so missing measurements fail closed while explicit readiness remains accepted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cee84641-9566-4e37-9017-2f6f490dbff7
📒 Files selected for processing (4)
packages/cli/src/cli-mcp-product-smoke.tspackages/cli/src/hermes-readiness.tstests/cli-mcp-product-smoke.test.tstests/hermes-readiness.test.ts
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.45.0)
packages/cli/src/cli-mcp-product-smoke.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🔇 Additional comments (4)
packages/cli/src/cli-mcp-product-smoke.ts (1)
323-334: Track sent request IDs instead of hardcoding1,2, and3.The response detector still rejects valid responses when this probe is reused with another ID scheme. Keep a set of emitted request IDs and validate against it.
packages/cli/src/hermes-readiness.ts (1)
3-3: LGTM!Also applies to: 21-21, 55-83, 98-98, 127-127, 139-139, 149-155
tests/cli-mcp-product-smoke.test.ts (1)
41-41: LGTM!Also applies to: 60-60, 69-69, 84-84, 147-194
tests/hermes-readiness.test.ts (1)
11-11: LGTM!Also applies to: 22-22, 35-35, 53-53, 79-79, 88-88, 104-104
evaOS review status: completedPR: #792 - fix: stabilize LCO 1.6 for Hermes-first MCP evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #792 Review URL: #792 (review) |
coderabbitai
Bot
added
documentation
There was a problem hiding this comment.
Walkthrough
PR: #792 - fix: stabilize LCO 1.6 for Hermes-first MCP
Head: bcbc5746b5960b866c5c0120da6867f077f9a1a5 into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 5/5 (~70 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
README.md |
modified | +46/-14 | Documentation | Low |
docs/BETA_RELEASE_RUNBOOK.md |
modified | +26/-4 | Documentation | Low |
docs/CLAIM_AUDIT.md |
modified | +14/-1 | Documentation | Low |
docs/HERMES_ADAPTER_BOUNDARY.md |
modified | +15/-13 | Documentation | Low |
docs/RELEASE_CHECKLIST.md |
modified | +14/-0 | Documentation | Moderate: validated P3 finding |
docs/SETUP.md |
modified | +56/-1 | Documentation | Low |
docs/releases/CHANGELOG.md |
modified | +2/-1 | Documentation | Low |
docs/releases/RELEASE_NOTES_1.6.0.md |
modified | +15/-6 | Documentation | Low |
package.json |
modified | +1/-1 | Configuration | Low |
packages/cli/src/cli-mcp-product-smoke.ts |
modified | +111/-18 | Changed file | Moderate: validated P3 finding |
packages/cli/src/hermes-readiness.ts |
added | +190/-0 | Changed file | Moderate: validated P3 finding |
packages/cli/src/hermes-smoke.ts |
added | +162/-0 | Changed file | Moderate: validated P3 finding |
packages/cli/src/main.ts |
modified | +187/-0 | Changed file | Low |
packages/core/src/index.ts |
modified | +7/-1 | Changed file | Low |
packages/mcp-server/src/mcp-protocol.ts |
added | +15/-0 | Changed file | Low |
packages/mcp-server/src/server-runtime.ts |
modified | +22/-3 | Changed file | Low |
packages/mcp-server/src/tools.ts |
modified | +10/-3 | Changed file | Moderate: validated P3 finding |
packages/openclaw-plugin/openclaw.plugin.json |
modified | +4/-4 | Changed file | Low |
tests/cli-find.test.ts |
modified | +49/-2 | Test coverage | Low |
tests/cli-help.test.ts |
modified | +25/-0 | Test coverage | Low |
tests/cli-mcp-product-smoke.test.ts |
modified | +101/-20 | Test coverage | Low |
tests/hermes-readiness.test.ts |
added | +187/-0 | Test coverage | Low |
tests/hermes-smoke.test.ts |
added | +215/-0 | Test coverage | Elevated: large change |
tests/lco-first-docs.test.ts |
modified | +3/-3 | Test coverage | Low |
tests/mcp-hermes-contract.test.ts |
added | +145/-0 | Test coverage | Low |
1 additional changed files omitted from this walkthrough.
Review Signal
Validated inline findings: 5 (P0: 0, P1: 0, P2: 0, P3: 5).
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
- API compatibility: 1
- Data loss: 1
- Flaky test risk: 1
- Proof gap: 1
- Release regression: 1
Validation and Proof
2 required validation/proof recommendation(s) selected from changed files.
- required: TypeScript/web build or CI proof - Runtime TypeScript/web files or package/config files changed. Proof: npm run build; typecheck; focused Vitest; green GitHub check.
- required: CI/release smoke proof - CI, release, launchd, or package metadata changed. Proof: green GitHub check; release-status; coverage-audit; rollback note.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Call out evidence leakage, replay/collision risks, duplicate side effects, and brittle sanitizer logic.
Profile proof expectations: Look for focused sanitizer, signature, orchestration, or fixture proof.
Related Context
Related issues/PRs: #785, #786, #788, #790, #791, #673.
Suggested labels: docs, tests.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions: none
- Label suggestions: orchestration, security, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/cli-mcp-product-smoke.ts (1)
266-275: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not mark notification silence proven before the probe reaches a completed call.
Line 266 reports
truefor any early failure with zero invalid messages, including failures beforenotifications/initializedis exercised. Gate this onpendingSuccess(or an explicit completed-notification-probe flag) as well.Proposed fix
- const notificationSilenceReady = invalidNotificationResponseCount === 0; + const notificationSilenceReady = pendingSuccess !== null + && invalidNotificationResponseCount === 0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/cli-mcp-product-smoke.ts` around lines 266 - 275, Update the notification probe result logic around notificationSilenceReady so it is true only when invalidNotificationResponseCount is zero and the probe has completed successfully, using pendingSuccess or the established completed-notification-probe state. Preserve the existing blocker and count reporting for early failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/cli/src/cli-mcp-product-smoke.ts`:
- Around line 266-275: Update the notification probe result logic around
notificationSilenceReady so it is true only when
invalidNotificationResponseCount is zero and the probe has completed
successfully, using pendingSuccess or the established
completed-notification-probe state. Preserve the existing blocker and count
reporting for early failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ec62b906-946d-417f-9c1a-d8bce807bfb6
📒 Files selected for processing (16)
README.mddocs/BETA_RELEASE_RUNBOOK.mddocs/RELEASE_CHECKLIST.mddocs/SETUP.mdopenclaw.plugin.jsonpackages/cli/src/cli-mcp-product-smoke.tspackages/cli/src/hermes-readiness.tspackages/cli/src/hermes-smoke.tspackages/cli/src/main.tspackages/mcp-server/src/tools.tspackages/openclaw-plugin/openclaw.plugin.jsonskills/lossless-openclaw-orchestrator/SKILL.mdtests/cli-help.test.tstests/cli-mcp-product-smoke.test.tstests/hermes-readiness.test.tstests/hermes-smoke.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: test
🧰 Additional context used
📓 Path-based instructions (3)
skills/lossless-openclaw-orchestrator/SKILL.md
📄 CodeRabbit inference engine (AGENTS.md)
Keep agent-facing workflow instructions in
skills/lossless-openclaw-orchestrator/SKILL.md.
Files:
skills/lossless-openclaw-orchestrator/SKILL.md
docs/SETUP.md
📄 CodeRabbit inference engine (AGENTS.md)
Keep detailed first-run instructions in
docs/SETUP.md.
Files:
docs/SETUP.md
README.md
📄 CodeRabbit inference engine (AGENTS.md)
README.md: KeepREADME.mdas the public landing page: product value, install, setup, first workflow, OpenClaw/MCP entrypoints, safety boundaries, and links.
Do not put active sprint ledgers, long issue history, exhaustive release-gate command inventories, or internal agent maintenance loops into the publicREADME.md; linkVISION.md, GitHub trackers, or runbooks instead.
Files:
README.md
🪛 ast-grep (0.45.0)
packages/cli/src/cli-mcp-product-smoke.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🪛 LanguageTool
skills/lossless-openclaw-orchestrator/SKILL.md
[style] ~172-~172: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...prepared_inboxwhen the user asks what needs attention. 2. Uselco_describe_ref` fo...
(EN_REPEATEDWORDS_NEED)
🔇 Additional comments (16)
docs/BETA_RELEASE_RUNBOOK.md (1)
265-269: Use isolated temporary database wording.
lco hermes smokeruns against isolated temporary runtime state, so “protected copy of the local LCO database” is misleading and may encourage operators to copy active data. Rename this to “isolated temporary database,” retaining the cleanup and evidence restrictions.README.md (1)
371-394: LGTM!docs/SETUP.md (1)
314-316: LGTM!Also applies to: 337-389
docs/RELEASE_CHECKLIST.md (1)
23-29: LGTM!Also applies to: 53-53, 72-73, 116-118
skills/lossless-openclaw-orchestrator/SKILL.md (1)
67-67: LGTM!Also applies to: 170-172
packages/openclaw-plugin/openclaw.plugin.json (1)
202-202: LGTM!Also applies to: 214-214, 2023-2023, 2035-2035
packages/mcp-server/src/tools.ts (1)
180-180: LGTM!Also applies to: 569-613
openclaw.plugin.json (1)
202-214: LGTM!Also applies to: 2023-2036
packages/cli/src/cli-mcp-product-smoke.ts (1)
14-14: LGTM!Also applies to: 35-36, 68-80, 92-92, 107-165, 183-228, 297-300, 320-435
packages/cli/src/hermes-smoke.ts (1)
26-26: LGTM!Also applies to: 69-153
packages/cli/src/main.ts (1)
1353-1357: LGTM!Also applies to: 2183-2196, 4575-4638
packages/cli/src/hermes-readiness.ts (1)
48-120: LGTM!Also applies to: 158-165
tests/hermes-smoke.test.ts (1)
20-20: LGTM!Also applies to: 64-69, 163-215
tests/cli-help.test.ts (1)
661-662: LGTM!tests/cli-mcp-product-smoke.test.ts (1)
18-29: LGTM!Also applies to: 33-100, 110-111, 156-218, 230-231, 261-266, 410-410, 792-793
tests/hermes-readiness.test.ts (1)
70-110: LGTM!Also applies to: 141-177
evaOS review status: completedPR: #792 - fix: stabilize LCO 1.6 for Hermes-first MCP evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #792 Review URL: #792 (review) |
coderabbitai
Bot
added
safety
There was a problem hiding this comment.
Walkthrough
PR: #792 - fix: stabilize LCO 1.6 for Hermes-first MCP
Head: 34a8130aafb585330f538dfecb635c312f9cefb0 into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 5/5 (~70 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
README.md |
modified | +46/-14 | Documentation | Low |
docs/BETA_RELEASE_RUNBOOK.md |
modified | +26/-4 | Documentation | Low |
docs/CLAIM_AUDIT.md |
modified | +14/-1 | Documentation | Low |
docs/HERMES_ADAPTER_BOUNDARY.md |
modified | +15/-13 | Documentation | Low |
docs/RELEASE_CHECKLIST.md |
modified | +14/-0 | Documentation | Low |
docs/SETUP.md |
modified | +56/-1 | Documentation | Low |
docs/releases/CHANGELOG.md |
modified | +2/-1 | Documentation | Low |
docs/releases/RELEASE_NOTES_1.6.0.md |
modified | +15/-6 | Documentation | Low |
package.json |
modified | +1/-1 | Configuration | Low |
packages/cli/src/cli-mcp-product-smoke.ts |
modified | +111/-18 | Changed file | Moderate: validated P2 finding |
packages/cli/src/hermes-readiness.ts |
added | +190/-0 | Changed file | Moderate: validated P2 finding |
packages/cli/src/hermes-smoke.ts |
added | +162/-0 | Changed file | Moderate: validated P3 finding |
packages/cli/src/main.ts |
modified | +187/-0 | Changed file | Low |
packages/core/src/index.ts |
modified | +7/-1 | Changed file | Low |
packages/mcp-server/src/mcp-protocol.ts |
added | +15/-0 | Changed file | Low |
packages/mcp-server/src/server-runtime.ts |
modified | +22/-3 | Changed file | Moderate: validated P2 finding |
packages/mcp-server/src/tools.ts |
modified | +10/-3 | Changed file | Low |
packages/openclaw-plugin/openclaw.plugin.json |
modified | +4/-4 | Changed file | Low |
tests/cli-find.test.ts |
modified | +49/-2 | Test coverage | Low |
tests/cli-help.test.ts |
modified | +25/-0 | Test coverage | Low |
tests/cli-mcp-product-smoke.test.ts |
modified | +102/-20 | Test coverage | Low |
tests/hermes-readiness.test.ts |
added | +187/-0 | Test coverage | Low |
tests/hermes-smoke.test.ts |
added | +215/-0 | Test coverage | Elevated: large change |
tests/lco-first-docs.test.ts |
modified | +3/-3 | Test coverage | Low |
tests/mcp-hermes-contract.test.ts |
added | +145/-0 | Test coverage | Low |
1 additional changed files omitted from this walkthrough.
Review Signal
Validated inline findings: 6 (P0: 0, P1: 0, P2: 3, P3: 3).
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
- API compatibility: 1
- Flaky test risk: 3
- Migration: 1
- Release regression: 1
Validation and Proof
2 required validation/proof recommendation(s) selected from changed files.
- required: TypeScript/web build or CI proof - Runtime TypeScript/web files or package/config files changed. Proof: npm run build; typecheck; focused Vitest; green GitHub check.
- required: CI/release smoke proof - CI, release, launchd, or package metadata changed. Proof: green GitHub check; release-status; coverage-audit; rollback note.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Call out evidence leakage, replay/collision risks, duplicate side effects, and brittle sanitizer logic.
Profile proof expectations: Look for focused sanitizer, signature, orchestration, or fixture proof.
Related Context
Related issues/PRs: #785, #786, #788, #790, #791, #673.
Suggested labels: docs, tests.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions: none
- Label suggestions: orchestration, security, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/cli/src/cli-mcp-product-smoke.ts (3)
417-436: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not return before draining the current stdout chunk.
If the tool result and an invalid notification response arrive in the same
dataevent,returnexits the callback and discards later entries inlines. The probe then finishes withinvalidNotificationResponseCount === 0and can incorrectly pass. RecordpendingSuccess, continue parsing the current chunk, and ignore only duplicate/late valid responses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/cli-mcp-product-smoke.ts` around lines 417 - 436, Update the stdout data handler around the tool-result branch identified by parsed.id === 3 so it records pendingSuccess and schedules completion without returning immediately. Continue iterating through the remaining lines in the current chunk, while ignoring only duplicate or late valid responses after success has been recorded, so invalid notification responses are still counted.
300-300: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTrack outstanding response IDs, not all historically sent IDs.
pendingResponseIdsis never removed after a response is consumed. A later response-like message with id1,2, or3is therefore accepted even when no request is outstanding, weakening the fail-closed notification proof. Remove IDs after the first valid response and add a duplicate/late-response regression test.Also applies to: 344-355
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/cli-mcp-product-smoke.ts` at line 300, Update the response-consumption logic around pendingResponseIds so each ID is removed immediately after its first valid response is accepted, ensuring only currently outstanding requests are tracked. Preserve fail-closed handling for duplicate or late responses, and add a regression test covering a response received after the corresponding ID has already been consumed.
92-92: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftThe 100 ms quiet period does not prove notification silence.
After 100 ms, the probe terminates the child and reports readiness; any delayed response to
notifications/initializedis never observed. This makes the release-gate signal “silent for 100 ms,” not “produced no response.” Use a protocol-aware drain/EOF strategy or a timeout tied to the configured probe deadline, and test delayed invalid responses.Also applies to: 418-435
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/cli-mcp-product-smoke.ts` at line 92, Replace the fixed NOTIFICATION_QUIET_PERIOD_MS-based readiness check in the MCP probe with protocol-aware draining through child-process EOF or the configured probe deadline, so delayed responses to notifications/initialized are observed before reporting silence. Update the cleanup/readiness flow around the probe process and add coverage for delayed invalid responses.tests/cli-mcp-product-smoke.test.ts (1)
151-194: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for an actual notification response.
This test proves that legal unsolicited
notifications/progressmessages are ignored, but it does not prove that an id-lessresult/erroremitted fornotifications/initializedfails the smoke check. Add that fake-server behavior and assertok === false,notificationSilenceReady === false, and a nonzero strict exit code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cli-mcp-product-smoke.test.ts` around lines 151 - 194, Add a regression case alongside the existing unsolicited-notification test using the fake MCP server helpers to emit an id-less result/error response for notifications/initialized. Run the qa-lab cli-mcp-smoke command in strict mode and assert the process exits nonzero, the parsed report has ok === false, and notificationSilenceReady === false.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/cli/src/cli-mcp-product-smoke.ts`:
- Around line 417-436: Update the stdout data handler around the tool-result
branch identified by parsed.id === 3 so it records pendingSuccess and schedules
completion without returning immediately. Continue iterating through the
remaining lines in the current chunk, while ignoring only duplicate or late
valid responses after success has been recorded, so invalid notification
responses are still counted.
- Line 300: Update the response-consumption logic around pendingResponseIds so
each ID is removed immediately after its first valid response is accepted,
ensuring only currently outstanding requests are tracked. Preserve fail-closed
handling for duplicate or late responses, and add a regression test covering a
response received after the corresponding ID has already been consumed.
- Line 92: Replace the fixed NOTIFICATION_QUIET_PERIOD_MS-based readiness check
in the MCP probe with protocol-aware draining through child-process EOF or the
configured probe deadline, so delayed responses to notifications/initialized are
observed before reporting silence. Update the cleanup/readiness flow around the
probe process and add coverage for delayed invalid responses.
In `@tests/cli-mcp-product-smoke.test.ts`:
- Around line 151-194: Add a regression case alongside the existing
unsolicited-notification test using the fake MCP server helpers to emit an
id-less result/error response for notifications/initialized. Run the qa-lab
cli-mcp-smoke command in strict mode and assert the process exits nonzero, the
parsed report has ok === false, and notificationSilenceReady === false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 70a463d6-7476-4e0d-b1d7-690f6285ffb6
📒 Files selected for processing (2)
packages/cli/src/cli-mcp-product-smoke.tstests/cli-mcp-product-smoke.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: test
- GitHub Check: CodeQL (javascript-typescript)
🧰 Additional context used
🪛 ast-grep (0.45.0)
packages/cli/src/cli-mcp-product-smoke.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🔇 Additional comments (2)
packages/cli/src/cli-mcp-product-smoke.ts (1)
14-14: LGTM!Also applies to: 107-129, 130-148, 165-165, 261-275, 396-403, 488-517, 520-523
tests/cli-mcp-product-smoke.test.ts (1)
247-247: LGTM!
evaOS review status: completedPR: #792 - fix: stabilize LCO 1.6 for Hermes-first MCP evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #792 Review URL: #792 (review) |
coderabbitai
Bot
added
the
orchestrator
There was a problem hiding this comment.
Walkthrough
PR: #792 - fix: stabilize LCO 1.6 for Hermes-first MCP
Head: 5ec7b90c68a25c138d21b3846834a1de376487cb into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 5/5 (~70 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
README.md |
modified | +46/-14 | Documentation | Low |
docs/BETA_RELEASE_RUNBOOK.md |
modified | +26/-4 | Documentation | Low |
docs/CLAIM_AUDIT.md |
modified | +14/-1 | Documentation | Low |
docs/HERMES_ADAPTER_BOUNDARY.md |
modified | +15/-13 | Documentation | Low |
docs/RELEASE_CHECKLIST.md |
modified | +14/-0 | Documentation | Low |
docs/SETUP.md |
modified | +56/-1 | Documentation | Low |
docs/releases/CHANGELOG.md |
modified | +2/-1 | Documentation | Low |
docs/releases/RELEASE_NOTES_1.6.0.md |
modified | +15/-6 | Documentation | Low |
package.json |
modified | +1/-1 | Configuration | Low |
packages/cli/src/cli-mcp-product-smoke.ts |
modified | +115/-19 | Changed file | Moderate: validated P3 finding |
packages/cli/src/hermes-readiness.ts |
added | +190/-0 | Changed file | Low |
packages/cli/src/hermes-smoke.ts |
added | +162/-0 | Changed file | Moderate: validated P2 finding |
packages/cli/src/main.ts |
modified | +187/-0 | Changed file | Low |
packages/core/src/index.ts |
modified | +7/-1 | Changed file | Low |
packages/mcp-server/src/mcp-protocol.ts |
added | +15/-0 | Changed file | Low |
packages/mcp-server/src/server-runtime.ts |
modified | +22/-3 | Changed file | Low |
packages/mcp-server/src/tools.ts |
modified | +10/-3 | Changed file | Moderate: validated P2 finding |
packages/openclaw-plugin/openclaw.plugin.json |
modified | +4/-4 | Changed file | Low |
tests/cli-find.test.ts |
modified | +49/-2 | Test coverage | Low |
tests/cli-help.test.ts |
modified | +25/-0 | Test coverage | Low |
tests/cli-mcp-product-smoke.test.ts |
modified | +227/-20 | Test coverage | Elevated: large change |
tests/hermes-readiness.test.ts |
added | +187/-0 | Test coverage | Low |
tests/hermes-smoke.test.ts |
added | +215/-0 | Test coverage | Elevated: large change |
tests/lco-first-docs.test.ts |
modified | +3/-3 | Test coverage | Low |
tests/mcp-hermes-contract.test.ts |
added | +145/-0 | Test coverage | Low |
1 additional changed files omitted from this walkthrough.
Review Signal
Validated inline findings: 3 (P0: 0, P1: 0, P2: 2, P3: 1).
Dropped findings before posting: 1. High-severity findings: 0.
Risk Taxonomy
- API compatibility: 1
- Flaky test risk: 2
Validation and Proof
2 required validation/proof recommendation(s) selected from changed files.
- required: TypeScript/web build or CI proof - Runtime TypeScript/web files or package/config files changed. Proof: npm run build; typecheck; focused Vitest; green GitHub check.
- required: CI/release smoke proof - CI, release, launchd, or package metadata changed. Proof: green GitHub check; release-status; coverage-audit; rollback note.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Call out evidence leakage, replay/collision risks, duplicate side effects, and brittle sanitizer logic.
Profile proof expectations: Look for focused sanitizer, signature, orchestration, or fixture proof.
Related Context
Related issues/PRs: #785, #786, #788, #790, #791, #673.
Suggested labels: docs, tests.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions: none
- Label suggestions: orchestration, security, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
Superseded by fixes through 5ec7b90; current-head CI, CodeQL, CodeRabbit status, and independent evaOS review are complete. No merge performed.
Exact-head stabilization evidenceCandidate:
Review dispositionsFixed now:
Accepted tradeoff / not a current gate failure:
Proof boundary: this establishes PR readiness and isolated Hermes-client compatibility for this SHA. It does not establish merge, npm/GitHub publication, installation into Eva's active profile, or live Eva runtime safety. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ec7b90c68
ℹ️ 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".
evaOS review status: completedPR: #792 - fix: stabilize LCO 1.6 for Hermes-first MCP evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #792 Review URL: #792 (review) |
There was a problem hiding this comment.
Walkthrough
PR: #792 - fix: stabilize LCO 1.6 for Hermes-first MCP
Head: 9bb2137f69095630b771ea5dac530a6663cbbadc into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 5/5 (~70 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
README.md |
modified | +49/-14 | Documentation | Low |
docs/BETA_RELEASE_RUNBOOK.md |
modified | +26/-4 | Documentation | Low |
docs/CLAIM_AUDIT.md |
modified | +14/-1 | Documentation | Low |
docs/HERMES_ADAPTER_BOUNDARY.md |
modified | +15/-13 | Documentation | Low |
docs/RELEASE_CHECKLIST.md |
modified | +14/-0 | Documentation | Low |
docs/SETUP.md |
modified | +56/-1 | Documentation | Low |
docs/releases/CHANGELOG.md |
modified | +2/-1 | Documentation | Low |
docs/releases/RELEASE_NOTES_1.6.0.md |
modified | +15/-6 | Documentation | Low |
package.json |
modified | +1/-1 | Configuration | Low |
packages/cli/src/cli-mcp-product-smoke.ts |
modified | +118/-19 | Changed file | Moderate: validated P2 finding |
packages/cli/src/hermes-readiness.ts |
added | +190/-0 | Changed file | Low |
packages/cli/src/hermes-smoke.ts |
added | +162/-0 | Changed file | Moderate: validated P3 finding |
packages/cli/src/main.ts |
modified | +187/-0 | Changed file | Low |
packages/core/src/index.ts |
modified | +7/-1 | Changed file | Low |
packages/mcp-server/src/mcp-protocol.ts |
added | +15/-0 | Changed file | Low |
packages/mcp-server/src/server-runtime.ts |
modified | +22/-3 | Changed file | Moderate: validated P3 finding |
packages/mcp-server/src/tools.ts |
modified | +10/-3 | Changed file | Low |
packages/openclaw-plugin/openclaw.plugin.json |
modified | +4/-4 | Changed file | Low |
tests/cli-find.test.ts |
modified | +49/-2 | Test coverage | Low |
tests/cli-help.test.ts |
modified | +25/-0 | Test coverage | Low |
tests/cli-mcp-product-smoke.test.ts |
modified | +257/-21 | Test coverage | Elevated: large change |
tests/hermes-readiness.test.ts |
added | +187/-0 | Test coverage | Low |
tests/hermes-smoke.test.ts |
added | +215/-0 | Test coverage | Elevated: large change |
tests/lco-first-docs.test.ts |
modified | +5/-3 | Test coverage | Low |
tests/mcp-hermes-contract.test.ts |
added | +145/-0 | Test coverage | Low |
1 additional changed files omitted from this walkthrough.
Review Signal
Validated inline findings: 4 (P0: 0, P1: 0, P2: 2, P3: 2).
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
- Flaky test risk: 2
- Runtime correctness: 2
Validation and Proof
2 required validation/proof recommendation(s) selected from changed files.
- required: TypeScript/web build or CI proof - Runtime TypeScript/web files or package/config files changed. Proof: npm run build; typecheck; focused Vitest; green GitHub check.
- required: CI/release smoke proof - CI, release, launchd, or package metadata changed. Proof: green GitHub check; release-status; coverage-audit; rollback note.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Call out evidence leakage, replay/collision risks, duplicate side effects, and brittle sanitizer logic.
Profile proof expectations: Look for focused sanitizer, signature, orchestration, or fixture proof.
Related Context
Related issues/PRs: #785, #786, #788, #790, #791, #673.
Suggested labels: docs, tests.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions: none
- Label suggestions: orchestration, security, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
Final delta evidence and review dispositionsCurrent head: The final delta fixes the two reproduced supported-path findings:
Current-head proof:
Terminal dispositions for the remaining final-delta threads:
Proof boundary: PR readiness and isolated Hermes-client compatibility for this SHA only; no merge, publication, active Eva install, or live Eva runtime claim. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bb2137f69
ℹ️ 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".
evaOS review status: completedPR: #792 - fix: stabilize LCO 1.6 for Hermes-first MCP evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #792 Review URL: #792 (review) |
There was a problem hiding this comment.
Walkthrough
PR: #792 - fix: stabilize LCO 1.6 for Hermes-first MCP
Head: aecd7add3b1e99d52c41ea5ec71059790dc50768 into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 5/5 (~70 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
README.md |
modified | +49/-14 | Documentation | Low |
docs/BETA_RELEASE_RUNBOOK.md |
modified | +26/-4 | Documentation | Low |
docs/CLAIM_AUDIT.md |
modified | +14/-1 | Documentation | Low |
docs/HERMES_ADAPTER_BOUNDARY.md |
modified | +15/-13 | Documentation | Low |
docs/RELEASE_CHECKLIST.md |
modified | +14/-0 | Documentation | Low |
docs/SETUP.md |
modified | +59/-1 | Documentation | Low |
docs/releases/CHANGELOG.md |
modified | +2/-1 | Documentation | Low |
docs/releases/RELEASE_NOTES_1.6.0.md |
modified | +15/-6 | Documentation | Low |
package.json |
modified | +1/-1 | Configuration | Low |
packages/cli/src/cli-mcp-product-smoke.ts |
modified | +118/-19 | Changed file | Moderate: validated P2 finding |
packages/cli/src/hermes-readiness.ts |
added | +190/-0 | Changed file | Moderate: validated P3 finding |
packages/cli/src/hermes-smoke.ts |
added | +162/-0 | Changed file | Moderate: validated P2 finding |
packages/cli/src/main.ts |
modified | +187/-0 | Changed file | Low |
packages/core/src/index.ts |
modified | +7/-1 | Changed file | Low |
packages/mcp-server/src/mcp-protocol.ts |
added | +15/-0 | Changed file | Low |
packages/mcp-server/src/server-runtime.ts |
modified | +22/-3 | Changed file | Moderate: validated P2 finding |
packages/mcp-server/src/tools.ts |
modified | +10/-3 | Changed file | Low |
packages/openclaw-plugin/openclaw.plugin.json |
modified | +4/-4 | Changed file | Low |
tests/cli-find.test.ts |
modified | +49/-2 | Test coverage | Low |
tests/cli-help.test.ts |
modified | +25/-0 | Test coverage | Low |
tests/cli-mcp-product-smoke.test.ts |
modified | +257/-21 | Test coverage | Elevated: large change |
tests/hermes-readiness.test.ts |
added | +187/-0 | Test coverage | Low |
tests/hermes-smoke.test.ts |
added | +215/-0 | Test coverage | Elevated: large change |
tests/lco-first-docs.test.ts |
modified | +7/-3 | Test coverage | Low |
tests/mcp-hermes-contract.test.ts |
added | +145/-0 | Test coverage | Low |
1 additional changed files omitted from this walkthrough.
Review Signal
Validated inline findings: 5 (P0: 0, P1: 0, P2: 3, P3: 2).
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
- API compatibility: 1
- Flaky test risk: 2
- Release regression: 1
- Runtime correctness: 1
Validation and Proof
2 required validation/proof recommendation(s) selected from changed files.
- required: TypeScript/web build or CI proof - Runtime TypeScript/web files or package/config files changed. Proof: npm run build; typecheck; focused Vitest; green GitHub check.
- required: CI/release smoke proof - CI, release, launchd, or package metadata changed. Proof: green GitHub check; release-status; coverage-audit; rollback note.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Call out evidence leakage, replay/collision risks, duplicate side effects, and brittle sanitizer logic.
Profile proof expectations: Look for focused sanitizer, signature, orchestration, or fixture proof.
Related Context
Related issues/PRs: #785, #786, #788, #790, #791, #673.
Suggested labels: docs, tests.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions: none
- Label suggestions: orchestration, security, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/lco-first-docs.test.ts`:
- Around line 59-60: Update the tilde assertions in the LCO documentation test
to extract and inspect only the Hermes subsection, rather than the entire MCP
section. In that Hermes block, make the LCO_DB_PATH pattern accept optional
single or double quotes around ~, while preserving validation that non-Hermes
examples are allowed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e0d06e3a-f1b5-45e8-b6d8-94c16a40f4b1
📒 Files selected for processing (5)
README.mddocs/SETUP.mdpackages/cli/src/cli-mcp-product-smoke.tstests/cli-mcp-product-smoke.test.tstests/lco-first-docs.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: test
🧰 Additional context used
📓 Path-based instructions (2)
docs/SETUP.md
📄 CodeRabbit inference engine (AGENTS.md)
Keep detailed first-run instructions in
docs/SETUP.md.
Files:
docs/SETUP.md
README.md
📄 CodeRabbit inference engine (AGENTS.md)
README.md: KeepREADME.mdas the public landing page: product value, install, setup, first workflow, OpenClaw/MCP entrypoints, safety boundaries, and links.
Do not put active sprint ledgers, long issue history, exhaustive release-gate command inventories, or internal agent maintenance loops into the publicREADME.md; linkVISION.md, GitHub trackers, or runbooks instead.
Files:
README.md
🪛 ast-grep (0.45.0)
packages/cli/src/cli-mcp-product-smoke.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🔇 Additional comments (6)
packages/cli/src/cli-mcp-product-smoke.ts (1)
106-173: LGTM!Also applies to: 243-305, 319-439, 491-524
tests/cli-mcp-product-smoke.test.ts (1)
18-18: LGTM!Also applies to: 33-119, 327-352
README.md (2)
371-372: Make Hermes setup discoverable from a heading.The new primary Hermes setup remains under
## OpenClaw And MCP, so the public landing page still frames the primary path as OpenClaw-only. Rename the heading to include Hermes or add a dedicated Hermes section.As per coding guidelines, keep
README.mdas the public landing page with clear setup and entrypoints.Source: Coding guidelines
383-385: LGTM!Also applies to: 387-397
docs/SETUP.md (1)
314-316: LGTM!Also applies to: 337-392
tests/lco-first-docs.test.ts (1)
96-103: LGTM!
Final exact-head review dispositionsCurrent head: The final docs-only delta fixed Current-head proof:
Terminal dispositions:
Proof boundary: PR readiness and isolated Hermes-client compatibility for this |
Current-head review completed and the only requested change was terminally dispositioned as a non-blocking test-maintainability refinement in issue comment 5108954468. All supported-path gates are green; no merge performed.
1 participant
Summary
Stabilizes the LCO 1.6 candidate for Hermes-first stdio MCP use:
{ "result": value }lco_findquery the existing index by default while retaining explicit refreshValidation
Candidate SHA:
edcdc78c89364b3f3cd3aeacd56f906ae950e3f4npm run buildlco qa-lab cli-mcp-smoke --strict: passlco hermes smoke --strict: pass, all 14 Eva tools present, default find 15 mslco release hermes-readiness --strict: passindex_skipped_by_defaultEval:
lco-hermes-1.6-stabilization-v1Evidence:
/Volumes/LEXAR/Codex/evidence/lco-hermes-first/2026-07-28/Scope boundary
This establishes PR readiness and isolated Hermes-client compatibility for the
named SHA. It does not merge this PR, merge #785, publish npm, create a tag or
GitHub Release, change Eva's active Hermes profile/database, run live Codex
control, or prove active-Eva/customer runtime safety.
PR #785 remains separate and has no file overlap with this candidate.
Closes #786
Closes #788
Closes #790
Tracks #791
Tracks #673
Summary by CodeRabbit
config.yamlexample.loo hermes smokeandloo release hermes-readinessfor evidence-based candidate validation.lco_find/loo_findrefresh/index only whenindex: true; default behavior skips indexing and records the reason.