feat: add local skills market and LAN web defaults#59
feat: add local skills market and LAN web defaults#59ymkiux merged 19 commits intoSakuraByteCore:mainfrom
Conversation
|
@coderabbitai re-review ！Stop making breaking changes, do a proper review！ |
📝 WalkthroughWalkthroughAdds a local "Skills Market": multi-target skills management (Codex/Claude/Agents) with target switching, local inspect/scan/import/export/delete (including ZIP and cross‑app), Web UI market tab/state and confirm-dialog gating, target-aware CLI/backend handlers, default web bind changed to 0.0.0.0, and supporting unit & E2E test updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Web UI (Market)
participant API as Backend API
participant FS as Local Skills Storage
rect rgba(100,149,237,0.5)
User->>UI: open Market / select target
UI->>UI: setSkillsTargetApp(target)
end
rect rgba(60,179,113,0.5)
UI->>API: POST /api (action: list-skills, targetApp)
API->>FS: read target skills directory (root)
FS-->>API: installed skills + root
API-->>UI: respond with skillsList + root
end
rect rgba(255,165,0,0.5)
UI->>API: POST /api (action: scan-unmanaged-skills, targetApp)
API->>FS: scan local roots for importable items
FS-->>API: importable items
API-->>UI: respond with skillsImportList
end
rect rgba(218,112,214,0.5)
User->>UI: trigger import/export/delete (with target)
UI->>API: POST /api (import/export/delete, targetApp, payload)
API->>FS: perform copy/delete or create ZIP
FS-->>API: operation result/status
API-->>UI: operation status
UI->>UI: refresh lists / update market state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Stylelint (17.5.0)web-ui/styles.cssJSONError: JSON Error in /package.json: 🔧 ast-grep (0.42.0)web-ui/app.jsComment |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-ui/modules/skills.methods.mjs (1)
160-161:⚠️ Potential issue | 🟡 MinorInconsistent return value when guard condition is met.
The guard clause at line 161 returns
undefined(implicit), while all other code paths returntrueorfalse. This inconsistency could cause issues inloadSkillsMarketOverviewwhere the return value is used to setskillsMarketImportLoadedOnce.🐛 Proposed fix
async scanImportableSkills(options = {}) { - if (this.skillsScanningImports || this.skillsImporting || this.skillsZipImporting || this.skillsExporting) return; + if (this.skillsScanningImports || this.skillsImporting || this.skillsZipImporting || this.skillsExporting) return false; const silent = !!(options && options.silent);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/modules/skills.methods.mjs` around lines 160 - 161, The guard clause in async scanImportableSkills currently returns implicitly undefined when this.skillsScanningImports || this.skillsImporting || this.skillsZipImporting || this.skillsExporting is true; change it to explicitly return false so all code paths return a boolean (true/false) consistent with other branches and with how loadSkillsMarketOverview uses the result to set skillsMarketImportLoadedOnce. Locate the scanImportableSkills function and update the early-return to return false rather than nothing.
🧹 Nitpick comments (6)
site/guide/getting-started.md (1)
30-30: Add a local-only override hint near the new LAN default.Line 30 is accurate, but adding a one-line note for
CODEXMATE_HOST=127.0.0.1would reduce accidental exposure on shared networks.✍️ Suggested doc tweak
-默认监听 `0.0.0.0:3737`，支持局域网访问，并尝试自动打开浏览器。 +默认监听 `0.0.0.0:3737`，支持局域网访问，并尝试自动打开浏览器。 +如需仅本机访问，可设置环境变量 `CODEXMATE_HOST=127.0.0.1`。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site/guide/getting-started.md` at line 30, Add a one-line local-only override hint immediately after the sentence that states the default listen address "0.0.0.0:3737": mention the environment variable CODEXMATE_HOST and show that setting CODEXMATE_HOST=127.0.0.1 will bind the server to localhost to avoid LAN exposure, keeping the note concise and placed next to the existing default-address sentence.web-ui/session-helpers.mjs (1)
64-68: Consider skipping market overview reload when already on the market tab.If repeated clicks can call
switchMainTab('market'), Line 64-68 may trigger unnecessary refresh work. Guarding by previous tab would avoid duplicate I/O.♻️ Suggested guard
- const shouldLoadSkillsMarketOnEnter = nextTab === 'market' + const shouldLoadSkillsMarketOnEnter = nextTab === 'market' + && previousTab !== 'market' && typeof this.loadSkillsMarketOverview === 'function';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/session-helpers.mjs` around lines 64 - 68, The current guard (shouldLoadSkillsMarketOnEnter) triggers loadSkillsMarketOverview when nextTab === 'market' even if the UI is already on the market tab; modify the condition to also check the current active tab to avoid redundant reloads. Update the logic that defines shouldLoadSkillsMarketOnEnter to require nextTab === 'market' && nextTab !== this.<currentMainTabState> && typeof this.loadSkillsMarketOverview === 'function' (replace <currentMainTabState> with the actual state variable that holds the current tab, e.g., this.mainTab or this.currentTab), so switchMainTab('market') clicks do nothing if the tab is already 'market', then call this.loadSkillsMarketOverview({ silent: true }) only when that combined condition is true.tests/unit/web-run-host.test.mjs (1)
112-115: HardenDEFAULT_WEB_HOSTextraction to avoid formatting-coupled failures.The current regex is fragile against quote-style/spacing changes in
cli.js. Consider a more tolerant pattern.♻️ Suggested patch
-const defaultHostMatch = cliContent.match(/const DEFAULT_WEB_HOST = '([^']+)';/); +const defaultHostMatch = cliContent.match(/const\s+DEFAULT_WEB_HOST\s*=\s*['"]([^'"]+)['"]\s*;/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/web-run-host.test.mjs` around lines 112 - 115, The current extraction of DEFAULT_WEB_HOST via cliContent.match is fragile to quote style and spacing changes; update the pattern used to find DEFAULT_WEB_HOST so it tolerates single or double quotes and optional whitespace and optional trailing semicolon (e.g. match const DEFAULT_WEB_HOST with \s* around =, capture a quote with a backreference and capture the host value) and then use the captured group for the host; keep the same error throw if the match fails. Ensure the code references DEFAULT_WEB_HOST, cliContent.match, and defaultHostMatch when implementing the new, more tolerant regex.README.en.md (1)
127-127: Add a short safety note for LAN binding defaults.Since the default host is now
0.0.0.0, consider explicitly documenting “trusted LAN only” and the quick fallback (CODEXMATE_HOST=127.0.0.1) for local-only exposure.Also applies to: 235-235
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.en.md` at line 127, Add a short safety note near the default listen address line that clarifies the server binds to 0.0.0.0:3737 (exposes to the LAN) and should only be used on trusted networks, and include the quick fallback to bind locally by setting CODEXMATE_HOST=127.0.0.1 (or mention setting host to 127.0.0.1) to restrict exposure; update both occurrences referenced (the default listen address line and the repeat at lines ~235) so the README explicitly warns about LAN binding and shows the local-only fallback.web-ui/index.html (1)
2136-2151: Target switch duplicated in modal and market panel.The target switch component (lines 2136-2151) is visually identical to the one in the market panel (lines 1307-1322). Consider extracting to a reusable Vue component if this pattern expands further.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/index.html` around lines 2136 - 2151, The market-target-switch markup is duplicated (same structure using class "market-target-switch"/"market-target-chip", binding to skillsTargetApp and calling setSkillsTargetApp) in two places; extract this into a reusable Vue component (e.g., MarketTargetSwitch) that accepts props for current target (skillsTargetApp) and an emit/callback for setSkillsTargetApp, replace both duplicated blocks with the new component, and ensure ARIA attributes and the click handler behavior are preserved by forwarding aria-pressed, role/tablist, and the silent option through the component API.web-ui/modules/skills.methods.mjs (1)
36-44: Minor: Unconditional state reset may clear user selections.
resetSkillsTargetState()is called unconditionally at line 41, clearing selections even when re-opening the modal for the same target. If this is intentional for a "fresh start" UX, consider adding a comment. Otherwise, the reset could be moved inside the target-change conditional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/modules/skills.methods.mjs` around lines 36 - 44, openSkillsManager always calls resetSkillsTargetState(), which clears user selections even when reopening the modal for the same target; either move the resetSkillsTargetState() call inside the branch that runs when the target actually changes (the targetApp !== this.skillsTargetApp block) so selections persist on re-open, or if the intended UX is to always start fresh, add an inline comment above resetSkillsTargetState() explaining the intentional behavior; locate this logic in openSkillsManager and update around the normalizeSkillsTargetApp/this.skillsTargetApp check and before setting showSkillsModal/refreshSkillsList.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli.js`:
- Around line 67-68: The default constant DEFAULT_WEB_HOST currently exposes the
unauthenticated web UI on all interfaces; change DEFAULT_WEB_HOST from '0.0.0.0'
to the loopback address '127.0.0.1' so the admin UI remains localhost-only by
default (leave DEFAULT_WEB_OPEN_HOST as-is); update any related references that
rely on DEFAULT_WEB_HOST to preserve behavior when the user explicitly supplies
--host or equivalent flags.
- Around line 1409-1416: resolveSkillTarget currently treats any explicit but
unsupported target (params.targetApp/params.target) as if it were missing and
silently falls back to the defaultApp (Codex); change it so that when an
explicit target string is provided but getSkillTargetByApp(raw) returns falsy
you surface an error instead of falling back. Update resolveSkillTarget to:
compute raw from params, if raw is empty use getSkillTargetByApp(defaultApp) ||
SKILL_TARGETS[0] || null, but if raw is non-empty and getSkillTargetByApp(raw)
is falsy throw or return a validation error indicating an unsupported target;
likewise apply the same explicit-vs-missing logic to the other helper(s) that
mirror this behavior (the other getSkillTarget... helpers mentioned in the
review) so typos no longer silently route to Codex.
In `@README.md`:
- Line 127: The README currently states the default listen address as "默认监听
`0.0.0.0:3737`", which exposes the UI to all network interfaces; change the
default back to `127.0.0.1` and update any startup/help text to require an
explicit opt-in (e.g., `--host 0.0.0.0`) to enable LAN access, and add a clear
security warning in the README next to the listen address explaining the risks
of binding to `0.0.0.0` and advising use of `127.0.0.1` or secure networks when
sensitive keys/config are present.
---
Outside diff comments:
In `@web-ui/modules/skills.methods.mjs`:
- Around line 160-161: The guard clause in async scanImportableSkills currently
returns implicitly undefined when this.skillsScanningImports ||
this.skillsImporting || this.skillsZipImporting || this.skillsExporting is true;
change it to explicitly return false so all code paths return a boolean
(true/false) consistent with other branches and with how
loadSkillsMarketOverview uses the result to set skillsMarketImportLoadedOnce.
Locate the scanImportableSkills function and update the early-return to return
false rather than nothing.
---
Nitpick comments:
In `@README.en.md`:
- Line 127: Add a short safety note near the default listen address line that
clarifies the server binds to 0.0.0.0:3737 (exposes to the LAN) and should only
be used on trusted networks, and include the quick fallback to bind locally by
setting CODEXMATE_HOST=127.0.0.1 (or mention setting host to 127.0.0.1) to
restrict exposure; update both occurrences referenced (the default listen
address line and the repeat at lines ~235) so the README explicitly warns about
LAN binding and shows the local-only fallback.
In `@site/guide/getting-started.md`:
- Line 30: Add a one-line local-only override hint immediately after the
sentence that states the default listen address "0.0.0.0:3737": mention the
environment variable CODEXMATE_HOST and show that setting
CODEXMATE_HOST=127.0.0.1 will bind the server to localhost to avoid LAN
exposure, keeping the note concise and placed next to the existing
default-address sentence.
In `@tests/unit/web-run-host.test.mjs`:
- Around line 112-115: The current extraction of DEFAULT_WEB_HOST via
cliContent.match is fragile to quote style and spacing changes; update the
pattern used to find DEFAULT_WEB_HOST so it tolerates single or double quotes
and optional whitespace and optional trailing semicolon (e.g. match const
DEFAULT_WEB_HOST with \s* around =, capture a quote with a backreference and
capture the host value) and then use the captured group for the host; keep the
same error throw if the match fails. Ensure the code references
DEFAULT_WEB_HOST, cliContent.match, and defaultHostMatch when implementing the
new, more tolerant regex.
In `@web-ui/index.html`:
- Around line 2136-2151: The market-target-switch markup is duplicated (same
structure using class "market-target-switch"/"market-target-chip", binding to
skillsTargetApp and calling setSkillsTargetApp) in two places; extract this into
a reusable Vue component (e.g., MarketTargetSwitch) that accepts props for
current target (skillsTargetApp) and an emit/callback for setSkillsTargetApp,
replace both duplicated blocks with the new component, and ensure ARIA
attributes and the click handler behavior are preserved by forwarding
aria-pressed, role/tablist, and the silent option through the component API.
In `@web-ui/modules/skills.methods.mjs`:
- Around line 36-44: openSkillsManager always calls resetSkillsTargetState(),
which clears user selections even when reopening the modal for the same target;
either move the resetSkillsTargetState() call inside the branch that runs when
the target actually changes (the targetApp !== this.skillsTargetApp block) so
selections persist on re-open, or if the intended UX is to always start fresh,
add an inline comment above resetSkillsTargetState() explaining the intentional
behavior; locate this logic in openSkillsManager and update around the
normalizeSkillsTargetApp/this.skillsTargetApp check and before setting
showSkillsModal/refreshSkillsList.
In `@web-ui/session-helpers.mjs`:
- Around line 64-68: The current guard (shouldLoadSkillsMarketOnEnter) triggers
loadSkillsMarketOverview when nextTab === 'market' even if the UI is already on
the market tab; modify the condition to also check the current active tab to
avoid redundant reloads. Update the logic that defines
shouldLoadSkillsMarketOnEnter to require nextTab === 'market' && nextTab !==
this.<currentMainTabState> && typeof this.loadSkillsMarketOverview ===
'function' (replace <currentMainTabState> with the actual state variable that
holds the current tab, e.g., this.mainTab or this.currentTab), so
switchMainTab('market') clicks do nothing if the tab is already 'market', then
call this.loadSkillsMarketOverview({ silent: true }) only when that combined
condition is true.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e3763db6-c585-47d0-a230-489fdad6b72c
📒 Files selected for processing (17)
README.en.mdREADME.mdcli.jssite/guide/getting-started.mdtests/unit/config-tabs-ui.test.mjstests/unit/run.mjstests/unit/session-tab-switch-performance.test.mjstests/unit/skills-market-runtime.test.mjstests/unit/skills-modal-ui.test.mjstests/unit/web-run-host.test.mjsweb-ui/app.jsweb-ui/index.htmlweb-ui/modules/config-mode.computed.mjsweb-ui/modules/skills.computed.mjsweb-ui/modules/skills.methods.mjsweb-ui/session-helpers.mjsweb-ui/styles.css
📜 Review details
🧰 Additional context used
🪛 LanguageTool
README.en.md
[grammar] ~57-~57: Ensure spelling is correct
Context: ...level delete (supports batch) Skills Market - Switch the skills install target b...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~58-~58: Ensure spelling is correct
Context: ...install target between Codex and Claude Code - Inspect local installed skills, root...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~59-~59: Ensure spelling is correct
Context: ...local installed skills, root paths, and status - Scan importable sources from Codex...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~61-~61: Ensure spelling is correct
Context: ...pp import, ZIP import/export, and batch delete Engineering Utilities - MCP stdio dom...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (34)
web-ui/modules/config-mode.computed.mjs (1)
52-52: Market tab label mapping looks correct.Line 52 cleanly extends
inspectorMainTabLabel()and preserves existing fallback behavior.tests/unit/run.mjs (1)
16-16: Good test-suite expansion in runner registration.The added imports integrate the new market/runtime and web-host test coverage without altering runner semantics.
Also applies to: 29-29
tests/unit/session-tab-switch-performance.test.mjs (1)
130-149: Nice targeted coverage for market-tab entry behavior.This test validates both state transition and the expected
loadSkillsMarketOverview({ silent: true })call contract.web-ui/app.js (1)
173-189: Skills market state initialization is consistent and ready for runtime wiring.
skillsTargetAppand the market loading flags are initialized with safe defaults and align with the skills methods/computed flow.tests/unit/web-run-host.test.mjs (1)
126-147: Coverage for host precedence/default behavior is solid.The three assertions cleanly validate default host, CLI override priority, and env override priority.
README.en.md (1)
57-61: Skills Market docs are clear and map well to the implemented UX.Scope, target switching, local scan behavior, and import/export/delete flows are documented consistently.
Also applies to: 199-204
tests/unit/skills-market-runtime.test.mjs (1)
11-127: Runtime test scaffolding and scenarios are well-targeted for the new market flow.This gives good confidence in target switching, state reset, and local overview load sequencing.
web-ui/modules/skills.computed.mjs (2)
3-8: LGTM! Clean target-aware computed properties.The
skillsTargetLabel()andskillsDefaultRootPath()computed properties correctly derive display values fromthis.skillsTargetApp, matching the target-switch behavior in the methods module.
87-105: LGTM! Well-structured market UI computed properties.
skillsMarketBusy()correctly aggregates all loading/transition flags using boolean coercion.- The preview properties (
skillsMarketInstalledPreview,skillsMarketImportPreview) follow the existing defensive pattern withArray.isArraychecks and provide consistent 6-item slices for the market tab UI.README.md (1)
57-61: LGTM! Skills Market documentation is comprehensive.The new Skills Market section clearly documents the local-first approach, target switching between Codex and Claude Code, and the cross-application import/export workflows. The emphasis on "不依赖远程在线市场" (no reliance on remote online marketplaces) aligns well with the local-first philosophy stated in the project description.
Also applies to: 199-203
tests/unit/skills-modal-ui.test.mjs (2)
43-59: LGTM! Comprehensive test coverage for Skills Market surface.The assertions properly validate:
- New computed properties (
skillsTargetLabel,skillsDefaultRootPath)- Method signature for target switching (
setSkillsTargetApp)- All expected API endpoints for local skills operations
- Absence of online market functionality (
skillsMarketRemoteCount,list-online-skills-market)The
doesNotMatchassertions effectively enforce that the online marketplace UI has been removed while local functionality is preserved.
75-77: LGTM! CSS class assertions align with local-only market design.The test correctly validates presence of
.market-target-chipfor the target switcher while asserting absence of.market-online-toolbarand.market-ecosystem-cardselectors, ensuring the online marketplace UI components are not present.web-ui/styles.css (2)
3671-3816: LGTM! Well-structured market UI styles following the design system.The new CSS classes for the Skills Market UI:
- Consistently use existing design tokens (
--color-*,--spacing-*,--radius-*)- Properly implement interactive states (
.market-target-chip.active,.market-action-card:hover:not(:disabled),.market-action-card:disabled)- Follow the established gradient and shadow patterns from the rest of the stylesheet
4325-4328: LGTM! Responsive breakpoints handle market grid collapse correctly.The responsive adjustments at 960px (compact target switch alignment) and 720px (single-column grid collapse for
.market-gridand.market-action-grid) ensure the Skills Market UI remains usable on smaller screens.Also applies to: 4355-4361
tests/unit/config-tabs-ui.test.mjs (4)
37-61: LGTM! Comprehensive market tab integration tests.The assertions thoroughly validate the market tab integration:
- Tab structure and ARIA accessibility attributes
- Vue bindings for tab visibility and selection state
- Target app switching handlers (
setSkillsTargetAppwith both'codex'and'claude')- Market overview loading behavior
- UI text content in Chinese
- Absence of online marketplace functionality via
doesNotMatchassertionsThis ensures the new local-only Skills Market is properly wired into the navigation system while confirming the removal of remote marketplace features.
175-183: LGTM! App state assertions validate local-only market design.The presence assertions for
skillsTargetApp,skillsMarketLoading,skillsMarketLocalLoadedOnce, andskillsMarketImportLoadedOncecombined with the absence assertions forskillsMarketRemoteLoading,skillsMarketRemoteLoadedOnce,skillsMarketRemoteItems,skillsMarketRemoteLatestOnly, andskillsMarketEcosystemseffectively enforce that the app state supports only local market functionality.
233-234: LGTM! Session helper assertions for market tab entry.The test validates that entering the market tab triggers
loadSkillsMarketOverview({ silent: true })via the deferred refresh mechanism, consistent with how other tabs (like trash) handle their initial data loading.
272-278: LGTM! CSS class assertions for market UI grid structure.The assertions validate:
- 2-column grid for
.market-gridand 3-column grid for.market-action-grid- Target switch styling (
.market-target-switch,.market-target-chip.active)- Wide panel variant (
.market-panel-wide)- Absence of online market styles (
.market-online-list,.market-ecosystem-grid)cli.js (1)
10300-10301: Nice loopback fallback for browser auto-open.Using
127.0.0.1for the browser URL while listening on an any-address bind avoids trying to open0.0.0.0in the browser.web-ui/modules/skills.methods.mjs (9)
3-5: LGTM!Simple and effective normalization that defaults unknown values to 'codex', providing a safe fallback.
7-17: LGTM!Comprehensive state reset that properly clears all skills-related state including the new one-time load flags for the market UI lifecycle.
19-34: LGTM!Good optimization to only reset state when the target actually changes, avoiding unnecessary re-renders and API calls.
61-101: LGTM!Well-structured error handling with consistent boolean return values. The API call correctly passes
targetAppmatching the backend'slist-skillshandler.
103-123: LGTM!Good coordination of one-time loading with proper guard against concurrent calls. The retry-on-failure behavior (flags stay false if load fails) is reasonable for transient errors.
198-239: LGTM!User action handler with proper busy guards and clear success/failure messaging. The API call correctly uses the generic
import-skillsendpoint withtargetAppparameter.
263-288: LGTM!Proper URL encoding for both the query parameter and the filename header. The endpoint matches the backend handler shown in context snippet 2.
325-369: LGTM!Export functionality correctly uses the generic
export-skillsendpoint withtargetAppparameter. The programmatic download link creation and cleanup is properly implemented.
371-420: LGTM!Delete functionality with proper confirmation dialog, clear warning message, and post-deletion refresh. The
delete-skillsendpoint is correctly used withtargetAppparameter.web-ui/index.html (6)
99-108: LGTM!New market tab follows the established pattern with proper ARIA attributes (
role="tab",aria-selected,aria-controls), keyboard accessibility (tabindex), and consistent event handlers.
228-246: LGTM!Side navigation entry follows the established pattern with proper ARIA attributes and dynamic meta information showing the current target and counts.
334-351: LGTM!Status strip correctly displays market-specific metrics. The
skillsImportConfiguredCountcomputed property is confirmed to exist inweb-ui/modules/skills.computed.mjs(context snippet 1).
484-488: LGTM!Explicitly passing
targetApp: 'codex'when opening skills manager from the Codex configuration section ensures correct context. The updated hint text appropriately directs users to the market page for Claude Code support.
1285-1457: LGTM!Well-structured market panel with proper accessibility (ARIA roles, labels), consistent styling patterns, and clear separation between overview and detailed management. The target selector correctly uses
role="tablist"andaria-pressedfor toggle buttons.
2304-2309: LGTM!Good architectural decision to move the ZIP input outside the modal. This ensures the file input persists in the DOM when the modal closes, preventing potential issues with the file selection flow being interrupted by modal state changes.
|
@coderabbitai re-review Please review the documentation changes again. |
|
@coderabbitai re-review ！Stop making breaking changes, do a proper review！ |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 8009
✅ Overall: Both files are well-synchronizedAll parallel changes are applied consistently across both READMEs:
🔍 Minor observations1. Slight wording divergence between two sections within each file (consistent across both READMEs) The "Skills Market" overview block (near the top) and the "Skills Market Tab" section (in the features detail area) describe the scan capability slightly differently:
These two phrasings describe the same underlying feature but from slightly different angles ("importable sources" vs. "unmanaged skills"). Since this wording is intentionally symmetric across both language files, this is a documentation style choice. No action required, just flagging for awareness. 2. Security consideration for Both READMEs now document The documentation changes are accurate, complete, and properly synchronized between the two language files. No factual inconsistencies or missing translations were found. 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
cli.js (2)
67-68:⚠️ Potential issue | 🔴 CriticalKeep the default web bind on loopback.
This makes an unauthenticated admin surface reachable from the LAN by default. The startup warning already confirms there is still no auth, so binding all interfaces should stay opt-in instead of the default.
🔒 Minimal fix
-const DEFAULT_WEB_HOST = '0.0.0.0'; +const DEFAULT_WEB_HOST = '127.0.0.1'; const DEFAULT_WEB_OPEN_HOST = '127.0.0.1';Also applies to: 10308-10310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 67 - 68, The default web bind currently uses DEFAULT_WEB_HOST = '0.0.0.0', which exposes an unauthenticated admin surface to the LAN; change DEFAULT_WEB_HOST to the loopback address ('127.0.0.1') so the server binds to localhost by default, keep or confirm DEFAULT_WEB_OPEN_HOST remains '127.0.0.1', and apply the same change to the other occurrences referenced (the duplicate at the other location, e.g., lines around 10308-10310) so all default bind settings use loopback instead of 0.0.0.0.
1409-1416:⚠️ Potential issue | 🟠 MajorReject unsupported
targetAppvalues instead of silently defaulting to Codex.An explicit typo currently falls back to the default target, so
delete-skills,import-skills,export-skills, and ZIP uploads can hit the wrong skills root. Only a missing target should default; an invalid explicit value should return目标宿主不支持.Also applies to: 9748-9756
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 1409 - 1416, resolveSkillTarget currently treats any explicit but invalid params.targetApp/target as missing and falls back to default; change it so that when an explicit target string is provided (non-empty) and getSkillTargetByApp(raw) returns falsy, the function returns the error marker '目标宿主不支持' instead of defaulting to Codex. Concretely, in resolveSkillTarget check whether raw is non-empty; if so, return getSkillTargetByApp(raw) || '目标宿主不支持'; otherwise fall back to getSkillTargetByApp(defaultApp) || SKILL_TARGETS[0] || null. Apply the same fix to the duplicate implementation referenced (the block around the other occurrence).
🧹 Nitpick comments (1)
web-ui/modules/skills.methods.mjs (1)
36-44: Unconditional state reset may clear user selections unexpectedly.
resetSkillsTargetState()is called unconditionally at line 41, even when the target hasn't changed. If a user closes and reopens the Skills modal for the same target, their previous selections will be cleared.If this is the intended UX (fresh state on every modal open), this is fine. Otherwise, consider only resetting when the target actually changes.
💡 Optional: Reset state only when target changes
async openSkillsManager(options = {}) { const targetApp = this.normalizeSkillsTargetApp(options && options.targetApp ? options.targetApp : this.skillsTargetApp); + const targetChanged = targetApp !== this.skillsTargetApp; if (targetApp !== this.skillsTargetApp) { this.skillsTargetApp = targetApp; } - this.resetSkillsTargetState(); + if (targetChanged) { + this.resetSkillsTargetState(); + } this.showSkillsModal = true; await this.refreshSkillsList({ silent: false }); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/modules/skills.methods.mjs` around lines 36 - 44, openSkillsManager currently calls resetSkillsTargetState unconditionally which clears selections even when reopening the modal for the same target; change the logic so resetSkillsTargetState is invoked only when the resolved targetApp differs from the current this.skillsTargetApp (i.e., after calling normalizeSkillsTargetApp compare targetApp !== this.skillsTargetApp and only then call resetSkillsTargetState and assign this.skillsTargetApp), leaving the rest of the method (showSkillsModal and refreshSkillsList) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli.js`:
- Around line 9758-9765: The route /api/import-codex-skills-zip must not be
retargetable by query string: in handleImportSkillsZipUpload replace the current
fallback logic that calls resolveSkillTargetAppFromRequest(req, options &&
options.targetApp ? options.targetApp : 'codex') with a call that forces the
literal 'codex' as the target (i.e., pass 'codex' directly so the request/query
cannot override it). Do the same change for the analogous call near lines
9793-9795 so both occurrences use explicit 'codex' when invoking
resolveSkillTargetAppFromRequest.
---
Duplicate comments:
In `@cli.js`:
- Around line 67-68: The default web bind currently uses DEFAULT_WEB_HOST =
'0.0.0.0', which exposes an unauthenticated admin surface to the LAN; change
DEFAULT_WEB_HOST to the loopback address ('127.0.0.1') so the server binds to
localhost by default, keep or confirm DEFAULT_WEB_OPEN_HOST remains '127.0.0.1',
and apply the same change to the other occurrences referenced (the duplicate at
the other location, e.g., lines around 10308-10310) so all default bind settings
use loopback instead of 0.0.0.0.
- Around line 1409-1416: resolveSkillTarget currently treats any explicit but
invalid params.targetApp/target as missing and falls back to default; change it
so that when an explicit target string is provided (non-empty) and
getSkillTargetByApp(raw) returns falsy, the function returns the error marker
'目标宿主不支持' instead of defaulting to Codex. Concretely, in resolveSkillTarget
check whether raw is non-empty; if so, return getSkillTargetByApp(raw) ||
'目标宿主不支持'; otherwise fall back to getSkillTargetByApp(defaultApp) ||
SKILL_TARGETS[0] || null. Apply the same fix to the duplicate implementation
referenced (the block around the other occurrence).
---
Nitpick comments:
In `@web-ui/modules/skills.methods.mjs`:
- Around line 36-44: openSkillsManager currently calls resetSkillsTargetState
unconditionally which clears selections even when reopening the modal for the
same target; change the logic so resetSkillsTargetState is invoked only when the
resolved targetApp differs from the current this.skillsTargetApp (i.e., after
calling normalizeSkillsTargetApp compare targetApp !== this.skillsTargetApp and
only then call resetSkillsTargetState and assign this.skillsTargetApp), leaving
the rest of the method (showSkillsModal and refreshSkillsList) unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e82df4b5-6277-4a9b-b4c6-7d4986bfaf6d
📒 Files selected for processing (17)
README.en.mdREADME.mdcli.jssite/guide/getting-started.mdtests/unit/config-tabs-ui.test.mjstests/unit/run.mjstests/unit/session-tab-switch-performance.test.mjstests/unit/skills-market-runtime.test.mjstests/unit/skills-modal-ui.test.mjstests/unit/web-run-host.test.mjsweb-ui/app.jsweb-ui/index.htmlweb-ui/modules/config-mode.computed.mjsweb-ui/modules/skills.computed.mjsweb-ui/modules/skills.methods.mjsweb-ui/session-helpers.mjsweb-ui/styles.css
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
🪛 LanguageTool
README.en.md
[grammar] ~57-~57: Ensure spelling is correct
Context: ...level delete (supports batch) Skills Market - Switch the skills install target b...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~58-~58: Ensure spelling is correct
Context: ...install target between Codex and Claude Code - Inspect local installed skills, root...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~59-~59: Ensure spelling is correct
Context: ...local installed skills, root paths, and status - Scan importable sources from Codex...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~61-~61: Ensure spelling is correct
Context: ...pp import, ZIP import/export, and batch delete Engineering Utilities - MCP stdio dom...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (37)
tests/unit/session-tab-switch-performance.test.mjs (1)
130-149: Market-tab behavior test is solid.This covers both the tab switch state change and the expected market overview preload call with the correct
{ silent: true }payload.site/guide/getting-started.md (1)
30-30: Doc update is clear and consistent.The default bind address and LAN-access statement are concise and aligned with the intended startup behavior.
web-ui/modules/config-mode.computed.mjs (1)
52-52: Main-tab label mapping formarketis correct.This cleanly extends the existing label resolver without affecting other tab branches.
web-ui/session-helpers.mjs (1)
64-68: Market-enter load hook is implemented safely.The function-type guard plus silent preload call fits the current tab-switch flow and avoids unsafe invocation when the handler is absent.
tests/unit/run.mjs (1)
16-16: Test runner updates look correct.Both new test modules are integrated using the same import/registration pattern, with no impact to runner error-handling or execution flow.
Also applies to: 29-29
web-ui/app.js (1)
173-189: Skills Market state defaults are correctly initialized.These added defaults prevent undefined-state reads during first render/load and align with the target-aware Skills Market flow.
tests/unit/web-run-host.test.mjs (2)
14-124: Function extraction and instantiation test harness is solid for this scope.The helper path cleanly isolates
resolveWebHostbehavior from CLI runtime side effects.
126-147: Host precedence coverage matches the LAN-default behavior change.The three assertions cover default, CLI override, and env override paths well.
web-ui/modules/skills.computed.mjs (2)
3-8: Target-aware label/default-path computed fields look correct.The
claudevs default (codex) mapping is clear and consistent.
87-105: Market busy/preview computed logic is safe and consistent.The boolean aggregation and bounded preview slices handle non-array state safely.
tests/unit/skills-market-runtime.test.mjs (4)
11-54: VM test harness setup is well-structured for runtime behavior checks.State initialization and computed binding are sufficient for these method-level tests.
56-78:refreshSkillsListtest validates target-aware API usage correctly.Good assertion set for action, params, and resulting state mutation.
80-103:setSkillsTargetAppreset/reload behavior is verified clearly.The test correctly confirms local state reset plus forced market overview reload.
105-127:loadSkillsMarketOvervieworchestration test is accurate.Order checks and final loading-state assertion provide good protection against regressions.
README.en.md (2)
127-127: Security consideration: Default LAN binding exposes the web UI to all network interfaces.This same concern was raised in README.md: binding to
0.0.0.0by default exposes the web UI (and potentially sensitive API keys/provider settings) to all devices on the network, which may be problematic on shared or untrusted networks.Consider adding a security note in the documentation warning users about this default, or keeping
127.0.0.1as the default with explicit opt-in for LAN access.Also applies to: 235-235
26-29: Documentation accurately describes the new Skills Market feature.The new sections clearly explain the local-first Skills Market capabilities including target switching between Codex and Claude Code, cross-app import, and ZIP distribution. The emphasis on local-first operation (not dependent on a remote marketplace) is well documented.
Also applies to: 57-61, 199-203
README.md (2)
236-236: Environment variable default change aligns with the documented binding behavior.The
CODEXMATE_HOSTdefault of0.0.0.0is consistent with the documented behavior at line 127. Note that the security implications of this default have been flagged in a previous review comment on line 127.
26-29: Documentation accurately describes the local Skills Market feature (Chinese version).The Chinese documentation mirrors the English version, clearly describing the local-first Skills Market capabilities. The content is consistent with the implementation.
Also applies to: 57-61, 199-203
web-ui/styles.css (2)
3671-3816: Well-structured CSS additions for the Skills Market UI.The new market-specific styles follow the established design system patterns, using existing CSS custom properties for spacing, colors, radii, transitions, and shadows. The component hierarchy is clear and consistent with other UI components in the stylesheet.
4325-4328: Responsive breakpoints are consistent with existing patterns.The responsive adjustments for
.market-target-switch-compact,.market-grid, and.market-action-gridalign with the existing media query breakpoints (960px and 720px) used throughout the stylesheet.Also applies to: 4355-4361
tests/unit/skills-modal-ui.test.mjs (2)
43-59: Test assertions correctly validate the skills module structure.The assertions verify:
- Computed properties (
skillsTargetLabel,skillsDefaultRootPath) are exported- Remote market functionality (
skillsMarketRemoteCount) is intentionally absentsetSkillsTargetApp(app, options = {})method signature matches the implementation- API endpoint usage (
list-skills,scan-unmanaged-skills,import-skills,export-skills,delete-skills) is correctly invoked- Online market API (
list-online-skills-market) is intentionally absentThese assertions align with the local-first Skills Market design.
75-77: Style assertions correctly enforce local-first UI patterns.The assertions verify that:
.market-target-chipCSS class is present (for target switching UI).market-online-toolbarand.market-ecosystem-cardare absent (confirming removal of remote/online market UI elements)This aligns with the PR's goal of implementing a local-first Skills Market.
web-ui/index.html (7)
99-108: LGTM! Market tab follows established navigation patterns.The new "技能市场" tab correctly mirrors the structure of existing tabs with proper
role="tab", dynamictabindex,:aria-selected, andaria-controlsbindings. The pointer/click handlers are consistently implemented.
228-246: LGTM! Side-rail market section is well-structured.The side navigation entry correctly uses
skillsTargetLabeland displays appropriate counters (skillsList.length,skillsImportList.length). ARIA attributes are properly configured.
334-351: LGTM! Market status strip provides useful context.The status chips display the target, local count, importable count, and directly-importable count. The use of
skillsImportConfiguredCountcorrectly references the computed property that filters items withhasSkillFile.
484-487: LGTM! Explicit target ensures predictable behavior.Passing
{ targetApp: 'codex' }explicitly from the Codex configuration panel ensures the Skills Manager always opens with the correct target context, regardless of the currentskillsTargetAppstate.
1285-1457: LGTM! Market panel is well-structured with proper target-aware functionality.The market panel correctly:
- Uses
loadSkillsMarketOverviewfor refresh actions- Implements target switching via
setSkillsTargetApp- Displays appropriate previews limited to first 6 items
- Provides clear action cards for local management, cross-app import, and ZIP distribution
The explanatory text clearly communicates that online/MCP registry features are removed.
2133-2151: LGTM! Target switch in Skills modal matches market panel.The compact target switch reuses the same
market-target-chipstyling and correctly callssetSkillsTargetAppwith{ silent: false }to provide user feedback on target changes.
2304-2309: LGTM! Moving the hidden input outside the modal is safe.The ZIP import
<input>element retains itsref,accept, and@changehandler. Placing it outside the modal block ensures it remains accessible when triggered from both the Skills modal and the market panel.tests/unit/config-tabs-ui.test.mjs (3)
37-61: LGTM! Comprehensive test coverage for market tab.The tests effectively verify:
- DOM structure (IDs, data attributes, ARIA bindings)
- Event handlers wiring
- Market-specific classes and state bindings
- Absence of removed online/MCP registry elements via
doesNotMatchThis ensures the market tab is correctly integrated and that removed features don't accidentally resurface.
175-183: LGTM! State initialization tests are thorough.The assertions correctly verify:
skillsTargetAppdefaults to'codex'- Market loading flags are initialized
- "Loaded once" tracking flags start as
false- Remote/online market state fields are absent
272-278: LGTM! CSS structure tests validate market layout.The assertions verify the expected grid layouts exist while confirming online market styles are absent, ensuring the UI matches the local-only design intent.
web-ui/modules/skills.methods.mjs (5)
3-5: LGTM! Simple and correct normalization.The function safely defaults to
'codex'for any input that isn't explicitly'claude', preventing invalid target values.
103-123: LGTM! Well-designed overview loading with proper concurrency guard.The method correctly:
- Guards against concurrent execution with
skillsMarketLoading- Tracks one-time loading via
skillsMarketLocalLoadedOnceandskillsMarketImportLoadedOnce- Supports both initial load and forced refresh scenarios
- Uses
finallyto ensure the loading flag is always cleared
160-161: Early return yieldsundefined, but callers handle this correctly.When the guard condition at line 161 triggers, the function returns
undefined. The callerloadSkillsMarketOverviewassigns this toimportLoaded, which becomes falsy and prevents settingskillsMarketImportLoadedOncetotrue. This is acceptable since the overview function has its own concurrency guard that prevents this scenario during normal operation.
371-420: LGTM! Delete operation includes proper confirmation dialog.The delete flow correctly:
- Validates selection before prompting
- Uses
requestConfirmDialogwith danger styling- Includes target label in confirmation message
- Refreshes both local list and import scan after deletion
263-288: LGTM! Target-aware ZIP upload endpoint.The endpoint change from
/api/import-codex-skills-zipto/api/import-skills-zip?targetApp=...properly encodes the target app parameter. The backend correctly extracts thetargetAppquery parameter viaresolveSkillTargetAppFromRequest, validates it withnormalizeSkillTargetApp, and falls back to 'codex' if not provided or invalid. This maintains backward compatibility while supporting dynamic target app selection.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cli.js (1)
9755-9765:⚠️ Potential issue | 🟠 MajorKeep the Codex-only ZIP endpoint pinned to Codex.
Lines 9759-9760 give any
targetAppquery precedence over the route-level default, so Line 9774 still lets/api/import-codex-skills-zip?targetApp=claudeimport into Claude. That breaks the endpoint contract and can write uploads into the wrong skills directory.🛡️ Minimal fix
async function handleImportSkillsZipUpload(req, res, options = {}) { if (req.method !== 'POST') { writeJsonResponse(res, 405, { error: 'Method Not Allowed' }); return; } try { - const targetApp = resolveSkillTargetAppFromRequest(req, options && options.targetApp ? options.targetApp : 'codex'); + const targetApp = options && options.targetApp + ? normalizeSkillTargetApp(options.targetApp) + : resolveSkillTargetAppFromRequest(req, 'codex'); if (!targetApp) { writeJsonResponse(res, 400, { error: '目标宿主不支持' }); return; }Also applies to: 9768-9790, 9807-9809
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 9755 - 9765, The helper resolveSkillTargetAppFromRequest currently honors a targetApp query param which lets `/api/import-codex-skills-zip` write to non-Codex targets; change it so when the caller passes a Codex-only default (fallbackApp === 'codex') the function ignores any targetApp query and always returns 'codex' (after normalizeSkillTargetApp) instead of letting parsed.searchParams override it; update resolveSkillTargetAppFromRequest to explicitly check for fallbackApp === 'codex' and return 'codex' immediately (using normalizeSkillTargetApp) before reading parsed.searchParams, still falling back safely on errors, and ensure normalizeSkillTargetApp is used consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cli.js`:
- Around line 9755-9765: The helper resolveSkillTargetAppFromRequest currently
honors a targetApp query param which lets `/api/import-codex-skills-zip` write
to non-Codex targets; change it so when the caller passes a Codex-only default
(fallbackApp === 'codex') the function ignores any targetApp query and always
returns 'codex' (after normalizeSkillTargetApp) instead of letting
parsed.searchParams override it; update resolveSkillTargetAppFromRequest to
explicitly check for fallbackApp === 'codex' and return 'codex' immediately
(using normalizeSkillTargetApp) before reading parsed.searchParams, still
falling back safely on errors, and ensure normalizeSkillTargetApp is used
consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5578611c-6b53-4543-bbcc-ef68caca59d1
📒 Files selected for processing (3)
README.mdcli.jstests/unit/web-run-host.test.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/web-run-host.test.mjs
- README.md
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
🔇 Additional comments (5)
cli.js (5)
68-68: Good hardening for any-address binds.Opening the browser on loopback instead of the listen address when the server is bound broadly is a nice safety improvement.
Also applies to: 10314-10314
120-126: Explicit skill-target validation looks solid.Rejecting unsupported explicit
targetApp/targetvalues here avoids silently routing typos to Codex while still preserving the existing default when the target is omitted.Also applies to: 1398-1424
1596-1669: Target-aware list/scan responses are consistent.Threading
targetApp,targetLabel, androotthrough the list/scan payloads should keep the UI state deterministic across Codex and Claude Code.Also applies to: 1716-1774
1776-1926: Unified target-aware skills operations are coherent.The import/export/delete flow keeps target metadata in the result payloads, blocks self-imports, and namespaces exported ZIPs by target app.
Also applies to: 1981-2259, 2269-2330
9940-9954: Dispatcher wiring matches the new generic skills API surface.These action mappings line up cleanly with the new target-aware helpers, so the frontend can keep forwarding
targetAppwithout extra branching.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
cli.js (1)
9774-9790:⚠️ Potential issue | 🟠 MajorPin route-scoped targets instead of treating them as fallback.
When this handler is used by
/api/import-codex-skills-zip,options.targetAppis only a fallback here, so?targetApp=claudestill imports into Claude’s skills directory. That breaks the Codex-only endpoint contract and can write uploads into the wrong app.🛠️ Minimal fix
- const targetApp = resolveSkillTargetAppFromRequest(req, options && options.targetApp ? options.targetApp : 'codex'); + const forcedTargetApp = normalizeSkillTargetApp(options && options.targetApp ? options.targetApp : ''); + const targetApp = forcedTargetApp || resolveSkillTargetAppFromRequest(req, 'codex'); if (!targetApp) { writeJsonResponse(res, 400, { error: '目标宿主不支持' }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 9774 - 9790, The handler currently uses options.targetApp only as a fallback when calling resolveSkillTargetAppFromRequest, allowing a query param like ?targetApp=claude to override the intended codex-only route; update the logic so the route pins the target to 'codex' instead of treating it as a fallback: call resolveSkillTargetAppFromRequest (or override its result) with a forced 'codex' target for this handler (i.e. ensure targetApp === 'codex' before proceeding to writeUploadZipStream/importSkillsFromZipFile), and keep the rest of the flow (fileName, writeUploadZipStream, importSkillsFromZipFile) unchanged.
🧹 Nitpick comments (1)
site/guide/getting-started.md (1)
30-30: Consider adding a local-only safety hint.Since Line 30 now defaults to
0.0.0.0, add a brief note that users on untrusted networks can bind to127.0.0.1for local-only access.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site/guide/getting-started.md` at line 30, Add a brief local-only safety hint next to the sentence that says the server defaults to listening on `0.0.0.0:3737` in getting-started.md: mention that on untrusted networks users can bind the server to `127.0.0.1` (localhost) to restrict access to the local machine and prevent LAN exposure, and optionally show the equivalent flag or config key used to change the bind address in your app/start command so readers know how to switch from `0.0.0.0` to `127.0.0.1`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.en.md`:
- Line 127: Update the README phrase that currently reads "Default listen
address is `0.0.0.0:3737`" to match actual runtime/tests by stating the default
listen address is `127.0.0.1:3737` (loopback) and clarify that browser auto-open
is enabled by default; alternatively, note that `0.0.0.0:3737` can be used to
expose on LAN if explicitly set. Ensure both occurrences of the sentence (the
one containing "Default listen address is `0.0.0.0:3737`" and the duplicate
later in the file) are updated to avoid conflicting documentation.
In `@web-ui/index.html`:
- Around line 279-281: Update the subtitle text that implies an "online
directory" to state the market is local-only: find the <p class="subtitle"
v-else-if="mainTab === 'market'"> element (and the other similar <p
class="subtitle" for the market tab later in the file) and replace the phrase
about connecting to or accessing an "online directory" with wording that
clarifies the Market only manages local Codex/Claude Skills and integrates with
the local ecosystem/catalog.
In `@web-ui/modules/skills.methods.mjs`:
- Around line 19-33: The setSkillsTargetApp method must be guarded against
concurrent skill operations: at its start, check a single in-flight marker
(create/use a boolean like this.skillsActionInProgress or
this._skillsActionLock, or detect existing flags such as
this.isImportingSkills/this.isExportingSkills/etc.) and if any in-flight
operation exists, abort the target switch (return false) or queue the requested
target for when operations finish; ensure all skill operations set and clear
that marker on start/finish so setSkillsTargetApp, resetSkillsTargetState and
loadSkillsMarketOverview will not operate on the wrong app mid-flight.
- Around line 36-44: openSkillsManager always calls resetSkillsTargetState which
clears skillsImportList and the loaded-once flags even when reopening the modal
for the same target; change the logic in openSkillsManager to only call
resetSkillsTargetState when the target actually changes (i.e., when normalized
targetApp !== this.skillsTargetApp before assignment), leaving skillsImportList
and the loaded-once flags intact for the same target, and still call
refreshSkillsList({ silent: false }) afterwards so the UI updates as needed.
---
Duplicate comments:
In `@cli.js`:
- Around line 9774-9790: The handler currently uses options.targetApp only as a
fallback when calling resolveSkillTargetAppFromRequest, allowing a query param
like ?targetApp=claude to override the intended codex-only route; update the
logic so the route pins the target to 'codex' instead of treating it as a
fallback: call resolveSkillTargetAppFromRequest (or override its result) with a
forced 'codex' target for this handler (i.e. ensure targetApp === 'codex' before
proceeding to writeUploadZipStream/importSkillsFromZipFile), and keep the rest
of the flow (fileName, writeUploadZipStream, importSkillsFromZipFile) unchanged.
---
Nitpick comments:
In `@site/guide/getting-started.md`:
- Line 30: Add a brief local-only safety hint next to the sentence that says the
server defaults to listening on `0.0.0.0:3737` in getting-started.md: mention
that on untrusted networks users can bind the server to `127.0.0.1` (localhost)
to restrict access to the local machine and prevent LAN exposure, and optionally
show the equivalent flag or config key used to change the bind address in your
app/start command so readers know how to switch from `0.0.0.0` to `127.0.0.1`.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ddaa99c9-cab7-44c7-9dca-994748d65362
📒 Files selected for processing (17)
README.en.mdREADME.mdcli.jssite/guide/getting-started.mdtests/unit/config-tabs-ui.test.mjstests/unit/run.mjstests/unit/session-tab-switch-performance.test.mjstests/unit/skills-market-runtime.test.mjstests/unit/skills-modal-ui.test.mjstests/unit/web-run-host.test.mjsweb-ui/app.jsweb-ui/index.htmlweb-ui/modules/config-mode.computed.mjsweb-ui/modules/skills.computed.mjsweb-ui/modules/skills.methods.mjsweb-ui/session-helpers.mjsweb-ui/styles.css
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
🪛 LanguageTool
README.en.md
[grammar] ~57-~57: Ensure spelling is correct
Context: ...level delete (supports batch) Skills Market - Switch the skills install target b...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~58-~58: Ensure spelling is correct
Context: ...install target between Codex and Claude Code - Inspect local installed skills, root...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~59-~59: Ensure spelling is correct
Context: ...local installed skills, root paths, and status - Scan importable sources from Codex...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~61-~61: Ensure spelling is correct
Context: ...pp import, ZIP import/export, and batch delete Engineering Utilities - MCP stdio dom...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (13)
tests/unit/session-tab-switch-performance.test.mjs (1)
130-149: Good coverage for market-tab preload behavior.This test correctly locks in the expected behavior when entering the market tab, including the silent overview preload call.
web-ui/app.js (1)
173-173: State initialization looks correct and complete.These defaults provide a clean initial state for target switching and market lazy-load guards without changing existing behavior.
Also applies to: 187-189
web-ui/modules/config-mode.computed.mjs (1)
52-52: Market tab label mapping is correctly wired.This keeps inspector labeling consistent once
mainTabswitches tomarket.web-ui/session-helpers.mjs (1)
64-68: Market tab preload integration looks correct.The new market preload follows the same guarded/silent pattern as other tab-entry loads and does not alter existing teardown logic.
tests/unit/run.mjs (1)
16-16: Good test-runner coverage expansion.Both new unit modules are wired into the runner using the existing import pattern.
Also applies to: 29-29
web-ui/modules/skills.computed.mjs (2)
3-8: Target label/root-path computed helpers are clean and deterministic.The codex/claude mapping is straightforward and consistent with target-aware runtime behavior.
87-105: Market busy + preview computed values are well-scoped.Boolean aggregation and top-6 previews are safe and avoid unnecessary UI over-rendering.
tests/unit/skills-modal-ui.test.mjs (2)
43-60: API-surface assertions are aligned with the new local skills market flow.The added checks correctly enforce computed/method presence and generic
*-skillsendpoint usage.
75-77: Style-hook assertions are consistent with the UI shift away from online market widgets.README.md (1)
127-130: Host default and security warning are clear and actionable.This wording makes the unauthenticated LAN exposure risk explicit while keeping loopback as the documented safe default.
Also applies to: 238-238
tests/unit/skills-market-runtime.test.mjs (1)
56-78: Good targeted runtime coverage for skills market orchestration.These tests exercise the critical target-aware flows (
list, target switch reset, overview load order) with clear assertions.Also applies to: 80-103, 105-127
tests/unit/web-run-host.test.mjs (1)
126-147: Host and target-resolution tests add strong regression protection.The added cases correctly lock precedence (
CLI > env > default) and explicit unsupported-target rejection.Also applies to: 194-213
cli.js (1)
1409-1423: Explicit unsupported targets now fail closed.Nice fix: an explicit but unknown
targetApp/targetno longer silently falls back to Codex, which prevents cross-app skill operations from landing in the wrong root.
|
@coderabbitai re-review ！Stop making breaking changes, do a proper review！ |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
cli.js (1)
67-68:⚠️ Potential issue | 🟠 MajorBinding the Web UI to all interfaces by default is still a safety blocker.
/apiin this same server can mutate providers, auth profiles, sessions, and skills without auth. Making the default host0.0.0.0exposes that admin surface to the LAN as soon ascodexmate runstarts; the startup warning later on does not reduce the exposure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 67 - 68, Change the default binding in cli.js so the admin web UI is not exposed to the LAN: update the DEFAULT_WEB_HOST constant from '0.0.0.0' to a loopback address (e.g., '127.0.0.1') and ensure DEFAULT_WEB_OPEN_HOST remains an explicit loopback (or 'localhost'); update any code paths that use DEFAULT_WEB_HOST (e.g., server startup or help text) so they use the new default and keep external-binding behavior opt-in via an explicit flag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli.js`:
- Around line 9774-9790: The fallback filename currently hardcodes
'codex-skills.zip' causing uploads for other targets (e.g., Claude) to get the
wrong default name; update the call to resolveUploadFileNameFromRequest to use
the resolved targetApp (or targetApp || 'codex') when building the fallback name
so the variable passed to resolveUploadFileNameFromRequest and later to
importSkillsFromZipFile reflects the actual target (refer to
normalizeSkillTargetApp, forcedTargetApp, targetApp,
resolveUploadFileNameFromRequest, and importSkillsFromZipFile); ensure you
compute targetApp first (forcedTargetApp ||
resolveSkillTargetAppFromRequest(...)) and then call
resolveUploadFileNameFromRequest(req, `${targetApp || 'codex'}-skills.zip`) so
importSkillsFromZipFile receives the correct fallbackName.
- Around line 120-126: SKILL_TARGETS and SKILL_IMPORT_SOURCES are constructed
from fixed CODEX_SKILLS_DIR/CLAUDE_SKILLS_DIR values and thus ignore env-aware
location resolution used elsewhere; change the construction of SKILL_TARGETS and
SKILL_IMPORT_SOURCES to call the existing env-aware resolver(s) (the same
helper(s) used elsewhere to locate Codex/Claude homes) instead of using the
static CODEX_SKILLS_DIR and CLAUDE_SKILLS_DIR constants, and ensure the
resulting arrays remain frozen (Object.freeze) and include the resolved dir
values for Codex, Claude and the Agents entry (AGENTS_SKILLS_DIR) so
scans/import/export/delete operate on the actual install locations.
---
Duplicate comments:
In `@cli.js`:
- Around line 67-68: Change the default binding in cli.js so the admin web UI is
not exposed to the LAN: update the DEFAULT_WEB_HOST constant from '0.0.0.0' to a
loopback address (e.g., '127.0.0.1') and ensure DEFAULT_WEB_OPEN_HOST remains an
explicit loopback (or 'localhost'); update any code paths that use
DEFAULT_WEB_HOST (e.g., server startup or help text) so they use the new default
and keep external-binding behavior opt-in via an explicit flag.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 941f9ee3-1b69-4dc4-bfbe-abfc17eb6c94
📒 Files selected for processing (11)
README.en.mdREADME.mdcli.jssite/guide/getting-started.mdtests/unit/config-tabs-ui.test.mjstests/unit/session-tab-switch-performance.test.mjstests/unit/skills-market-runtime.test.mjstests/unit/web-run-host.test.mjsweb-ui/index.htmlweb-ui/modules/skills.methods.mjsweb-ui/session-helpers.mjs
✅ Files skipped from review due to trivial changes (2)
- site/guide/getting-started.md
- tests/unit/session-tab-switch-performance.test.mjs
🚧 Files skipped from review as they are similar to previous changes (5)
- web-ui/session-helpers.mjs
- README.md
- web-ui/modules/skills.methods.mjs
- tests/unit/skills-market-runtime.test.mjs
- tests/unit/web-run-host.test.mjs
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
🪛 LanguageTool
README.en.md
[grammar] ~57-~57: Ensure spelling is correct
Context: ...level delete (supports batch) Skills Market - Switch the skills install target b...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~58-~58: Ensure spelling is correct
Context: ...install target between Codex and Claude Code - Inspect local installed skills, root...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~59-~59: Ensure spelling is correct
Context: ...local installed skills, root paths, and status - Scan importable sources from Codex...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~61-~61: Ensure spelling is correct
Context: ...pp import, ZIP import/export, and batch delete Engineering Utilities - MCP stdio dom...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (14)
README.en.md (2)
26-29: LGTM!The Skills Market documentation additions are comprehensive and well-structured:
- Feature bullet accurately describes local-first capabilities
- Comparison table row clarifies the value proposition
- Core Features section details all key functionality
- Architecture diagram updates correctly reflect the new SKILLS node and service changes
- Web UI section documents the new tab appropriately
The local-first emphasis is consistent throughout.
Also applies to: 38-38, 57-62, 85-85, 90-94, 112-112, 201-206
127-129: Documentation matches runtime host default.The runtime code confirms
DEFAULT_WEB_HOST = '0.0.0.0'(cli.js:67), which aligns with the documentation. The safety note appropriately documents the LAN exposure implications of this default.web-ui/index.html (7)
99-108: LGTM!The new market tab follows the established navigation pattern correctly:
- Consistent ARIA attributes (
role="tab",aria-selected,aria-controls,tabindex)- Pointer and click event handlers match existing tabs
- Data attribute
data-main-tab="market"enables consistent styling/scripting
228-246: LGTM!The sidebar market section is well implemented:
- Proper accessibility attributes
- Dynamic content displays relevant counts (
skillsList.length,skillsImportList.length)skillsTargetLabelprovides context-aware labeling
334-351: LGTM!The market status strip provides useful at-a-glance metrics with clear labeling.
484-486: Intentional behavior: Quick action explicitly targets Codex.The
{ targetApp: 'codex' }parameter ensures this quick action from the Codex config panel always opens the Skills manager for Codex, regardless of the currentskillsTargetAppstate. This differs from market panel actions (lines 1301, 1413) which use the currently selected target, which is the correct UX design.
1285-1457: LGTM!The market panel implementation is thorough:
- Target switching with proper
aria-pressedaccessibility- Preview grids with reasonable limits (6 items)
- Clear separation between installed skills, importable sources, and distribution actions
- Explicit "market explanation" section clarifies local-only scope
- Disabled states during busy operations prevent race conditions
2136-2151: LGTM!The target switch in the skills modal mirrors the market panel implementation, providing consistent UX for switching between Codex and Claude Code targets.
2304-2309: LGTM!Moving the ZIP file input outside the modal is safe. Per
web-ui/modules/skills.methods.mjs:258-278, bothtriggerSkillsZipImportandhandleSkillsZipImportChangeinclude null checks on the input element (if (input),if (el)), providing defensive handling if the ref is not available.tests/unit/config-tabs-ui.test.mjs (4)
37-63: LGTM!Comprehensive test coverage for the new market tab:
- Positive assertions verify all required DOM elements and Vue bindings
- Negative assertions (
doesNotMatch) confirm removal of online/remote market functionality- Tests for both presence of local market features and absence of deprecated remote features
177-185: LGTM!Good coverage of app state initialization:
- Verifies local market state fields exist (
skillsTargetApp,skillsMarketLoading, etc.)- Confirms remote/online market fields are not present, enforcing the local-first design
235-237: LGTM!These assertions correctly verify the deferred market loading logic in
switchMainTab. Perweb-ui/session-helpers.mjs:65, thepreviousTab !== 'market'guard prevents redundant reloads when re-entering the market tab.
275-281: LGTM!CSS assertions verify:
- Market grid layout classes exist
- Target switch styling exists
- Online/ecosystem market styles are absent (consistent with local-only design)
cli.js (1)
1409-1423: Nice fail-closed target handling.Unsupported explicit targets now stop instead of silently falling back to Codex, and the legacy Codex ZIP route is pinned to
codexinstead of treating it as a fallback.Also applies to: 9808-9810
|
@coderabbitai re-review ！Stop making breaking changes, do a proper review！ |
|
Re-review constraint reaffirmed: focus exclusively on correctness, safety, and quality; no breaking changes or refactors. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-ui/modules/skills.methods.mjs (1)
78-117:⚠️ Potential issue | 🟠 MajorKeep the overview cache flags in sync with direct refreshes.
loadSkillsMarketOverview()skips work based onskillsMarketLocalLoadedOnceandskillsMarketImportLoadedOnce, butrefreshSkillsList()andscanImportableSkills()can clearskillsList/skillsImportListon a failed manual refresh without clearing those flags. After one failed refresh or scan from the modal or market page, the next automatic overview load can skip that slice and keep showing stale empty data until the user refreshes again.🧭 Minimal fix
async refreshSkillsList(options = {}) { this.skillsLoading = true; try { const res = await api('list-skills', { targetApp: this.skillsTargetApp }); if (res.error) { this.skillsRootPath = ''; this.skillsList = []; this.skillsSelectedNames = []; + this.skillsMarketLocalLoadedOnce = false; this.showMessage(res.error, 'error'); return false; } const exists = res.exists !== false; if (!exists) { this.skillsRootPath = res.root || ''; this.skillsList = []; this.skillsSelectedNames = []; + this.skillsMarketLocalLoadedOnce = true; if (!options.silent) { this.showMessage(`${this.skillsTargetLabel} skills 目录不存在，已按空列表显示`, 'info'); } return true; } this.skillsRootPath = res.root || ''; this.skillsList = Array.isArray(res.items) ? res.items : []; const currentNames = new Set((Array.isArray(this.skillsList) ? this.skillsList : []) .map((item) => (item && typeof item.name === 'string' ? item.name.trim() : '')) .filter(Boolean)); this.skillsSelectedNames = (Array.isArray(this.skillsSelectedNames) ? this.skillsSelectedNames : []) .filter((name) => currentNames.has(name)); + this.skillsMarketLocalLoadedOnce = true; return true; } catch (e) { this.skillsRootPath = ''; this.skillsList = []; this.skillsSelectedNames = []; + this.skillsMarketLocalLoadedOnce = false; this.showMessage('加载 skills 失败', 'error'); return false; } finally { this.skillsLoading = false; } } ... async scanImportableSkills(options = {}) { if (this.skillsScanningImports || this.skillsImporting || this.skillsZipImporting || this.skillsExporting) return false; const silent = !!(options && options.silent); this.skillsScanningImports = true; try { const res = await api('scan-unmanaged-skills', { targetApp: this.skillsTargetApp }); if (res.error) { this.skillsImportList = []; this.skillsImportSelectedKeys = []; + this.skillsMarketImportLoadedOnce = false; if (!silent) { this.showMessage(res.error, 'error'); } return false; } this.skillsImportList = Array.isArray(res.items) ? res.items : []; const availableKeys = new Set(this.skillsImportSelectableKeys); this.skillsImportSelectedKeys = (Array.isArray(this.skillsImportSelectedKeys) ? this.skillsImportSelectedKeys : []) .filter((key) => availableKeys.has(key)); + this.skillsMarketImportLoadedOnce = true; if (!silent && this.skillsImportList.length === 0) { this.showMessage('未扫描到可导入 skill', 'info'); } else if (!silent) { this.showMessage(`扫描到 ${this.skillsImportList.length} 个可导入 skill`, 'success'); } return true; } catch (e) { this.skillsImportList = []; this.skillsImportSelectedKeys = []; + this.skillsMarketImportLoadedOnce = false; if (!silent) { this.showMessage('扫描可导入 skill 失败', 'error'); } return false; } finally { this.skillsScanningImports = false; } }Also applies to: 120-139, 177-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/modules/skills.methods.mjs` around lines 78 - 117, When refreshSkillsList() clears skillsList/skillsSelectedNames due to a non-existent directory or an error, also reset the overview cache flags so automatic overview reloads won't incorrectly skip updates: clear skillsMarketLocalLoadedOnce (and if applicable skillsMarketImportLoadedOnce) inside the "if (!exists) { ... }" branch and inside the catch block where you reset skillsRootPath/skillsList/skillsSelectedNames. Do the analogous change in scanImportableSkills(): whenever you clear skillsImportList on error or empty result, reset skillsMarketImportLoadedOnce so the market overview will re-fetch that slice. Use the exact symbols refreshSkillsList, scanImportableSkills, skillsMarketLocalLoadedOnce, skillsMarketImportLoadedOnce, skillsList, and skillsImportList to find and update the right places.
♻️ Duplicate comments (2)
cli.js (2)
9774-9790:⚠️ Potential issue | 🟡 MinorBuild the generic ZIP fallback name from the resolved target.
After
targetAppis resolved, Line 9780 still hardcodescodex-skills.zip. For root-levelSKILL.mdarchives, that fallback becomes the imported skill name, so/api/import-skills-zip?targetApp=claudecan still create a Codex-named skill by default.🩹 Minimal fix
- const fileName = resolveUploadFileNameFromRequest(req, 'codex-skills.zip'); + const fileName = resolveUploadFileNameFromRequest(req, `${targetApp}-skills.zip`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 9774 - 9790, The fallback ZIP filename is hardcoded to 'codex-skills.zip' when computing fileName and passed to importSkillsFromZipFile, which lets an upload default to creating a Codex-named skill even after resolving targetApp; update the code that sets fileName (the call to resolveUploadFileNameFromRequest used before writeUploadZipStream) to build the generic fallback using the resolved targetApp (forcedTargetApp/targetApp) instead of the fixed string—e.g., derive the fallback as `${targetApp}-skills.zip` or similar—so resolveUploadFileNameFromRequest and the subsequent importSkillsFromZipFile call receive a target-specific fallback name.
120-126:⚠️ Potential issue | 🟠 MajorUse the env-aware Codex/Claude homes for
SKILL_TARGETS.These targets are still pinned to
~/.codex/skillsand~/.claude/skills, while this file already treats Codex/Claude installs as relocatable elsewhere. OnCODEX_HOME/CLAUDE_HOME/XDG_CONFIG_HOMEsetups, the market will scan, import, export, and delete against the wrong tree, and imports can create stray default directories instead of the configured home.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 120 - 126, SKILL_TARGETS and SKILL_IMPORT_SOURCES must use the environment-aware skill locations instead of hardcoded ~/.codex/skills and ~/.claude/skills; update the SKILL_TARGETS entries to reference the same computed values/helpers used elsewhere to resolve Codex/Claude homes (e.g., the CODEX_SKILLS_DIR / CLAUDE_SKILLS_DIR resolution or the helper that reads CODEX_HOME/CLAUDE_HOME/XDG_CONFIG_HOME) so all operations (scan/import/export/delete) operate against the configured home and not default dirs.
🧹 Nitpick comments (1)
tests/unit/web-run-host.test.mjs (1)
103-103: Minor: duplicate return statement in generated code.
extractFunctionBySignature(line 103) appendsreturn ${funcName};, andinstantiateFunction(line 109) appends anotherreturn ${funcName};. This results in:function foo() {...} return foo; return foo; // unreachableThe second return is harmless (unreachable), but you could consolidate by removing one of them.
♻️ Optional: remove duplicate return
function extractFunctionBySignature(source, signature, funcName) { // ... const block = source.slice(startIndex, endIndex + 1).trim(); - return `${block}\nreturn ${funcName};`; + return block; }Also applies to: 109-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/web-run-host.test.mjs` at line 103, The generated code contains a duplicate "return ${funcName};" because both extractFunctionBySignature and instantiateFunction append the same return; remove the extra by choosing one location to emit the final return — e.g., stop appending `return ${funcName};` inside extractFunctionBySignature and let instantiateFunction produce it (or vice versa), ensuring funcName is still returned once and tests that rely on the returned value still pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli.js`:
- Around line 10315-10316: The current logic collapses any-address hosts to
DEFAULT_WEB_OPEN_HOST which becomes 127.0.0.1; when the original host is the
IPv6 any address ("::") auto-open should instead use the IPv6 loopback so the
browser targets an address that will be bound. Update the code that sets
openHost/openUrl (referencing isAnyAddressHost, host, openHost, openUrl,
formatHostForUrl, DEFAULT_WEB_OPEN_HOST, port) to special-case "::" (and
equivalently the IPv6 any constant) and substitute an IPv6 loopback (e.g. "::1")
for openHost before calling formatHostForUrl so the generated http URL uses the
IPv6 loopback form rather than 127.0.0.1.
In `@web-ui/index.html`:
- Around line 1307-1322: The wrapper element with class "market-target-switch"
currently uses role="tablist" but its children are buttons using aria-pressed
and the Vue state/handler skillsTargetApp and setSkillsTargetApp; update the
wrapper's ARIA role from "tablist" to "group" (for both instances of the
market-target-switch markup) so the semantics match the pressed-toggle pattern
while keeping the button markup, aria-pressed usage, and existing
setSkillsTargetApp/skillsTargetApp logic unchanged.
---
Outside diff comments:
In `@web-ui/modules/skills.methods.mjs`:
- Around line 78-117: When refreshSkillsList() clears
skillsList/skillsSelectedNames due to a non-existent directory or an error, also
reset the overview cache flags so automatic overview reloads won't incorrectly
skip updates: clear skillsMarketLocalLoadedOnce (and if applicable
skillsMarketImportLoadedOnce) inside the "if (!exists) { ... }" branch and
inside the catch block where you reset
skillsRootPath/skillsList/skillsSelectedNames. Do the analogous change in
scanImportableSkills(): whenever you clear skillsImportList on error or empty
result, reset skillsMarketImportLoadedOnce so the market overview will re-fetch
that slice. Use the exact symbols refreshSkillsList, scanImportableSkills,
skillsMarketLocalLoadedOnce, skillsMarketImportLoadedOnce, skillsList, and
skillsImportList to find and update the right places.
---
Duplicate comments:
In `@cli.js`:
- Around line 9774-9790: The fallback ZIP filename is hardcoded to
'codex-skills.zip' when computing fileName and passed to
importSkillsFromZipFile, which lets an upload default to creating a Codex-named
skill even after resolving targetApp; update the code that sets fileName (the
call to resolveUploadFileNameFromRequest used before writeUploadZipStream) to
build the generic fallback using the resolved targetApp
(forcedTargetApp/targetApp) instead of the fixed string—e.g., derive the
fallback as `${targetApp}-skills.zip` or similar—so
resolveUploadFileNameFromRequest and the subsequent importSkillsFromZipFile call
receive a target-specific fallback name.
- Around line 120-126: SKILL_TARGETS and SKILL_IMPORT_SOURCES must use the
environment-aware skill locations instead of hardcoded ~/.codex/skills and
~/.claude/skills; update the SKILL_TARGETS entries to reference the same
computed values/helpers used elsewhere to resolve Codex/Claude homes (e.g., the
CODEX_SKILLS_DIR / CLAUDE_SKILLS_DIR resolution or the helper that reads
CODEX_HOME/CLAUDE_HOME/XDG_CONFIG_HOME) so all operations
(scan/import/export/delete) operate against the configured home and not default
dirs.
---
Nitpick comments:
In `@tests/unit/web-run-host.test.mjs`:
- Line 103: The generated code contains a duplicate "return ${funcName};"
because both extractFunctionBySignature and instantiateFunction append the same
return; remove the extra by choosing one location to emit the final return —
e.g., stop appending `return ${funcName};` inside extractFunctionBySignature and
let instantiateFunction produce it (or vice versa), ensuring funcName is still
returned once and tests that rely on the returned value still pass.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b52fda8-8cd2-456a-b477-a68b643d61d2
📒 Files selected for processing (17)
README.en.mdREADME.mdcli.jssite/guide/getting-started.mdtests/unit/config-tabs-ui.test.mjstests/unit/run.mjstests/unit/session-tab-switch-performance.test.mjstests/unit/skills-market-runtime.test.mjstests/unit/skills-modal-ui.test.mjstests/unit/web-run-host.test.mjsweb-ui/app.jsweb-ui/index.htmlweb-ui/modules/config-mode.computed.mjsweb-ui/modules/skills.computed.mjsweb-ui/modules/skills.methods.mjsweb-ui/session-helpers.mjsweb-ui/styles.css
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
🪛 LanguageTool
README.en.md
[grammar] ~57-~57: Ensure spelling is correct
Context: ...level delete (supports batch) Skills Market - Switch the skills install target b...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~58-~58: Ensure spelling is correct
Context: ...install target between Codex and Claude Code - Inspect local installed skills, root...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~59-~59: Ensure spelling is correct
Context: ...local installed skills, root paths, and status - Scan importable sources from Codex...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~61-~61: Ensure spelling is correct
Context: ...pp import, ZIP import/export, and batch delete Engineering Utilities - MCP stdio dom...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (22)
web-ui/app.js (1)
173-173: State defaults look correct and safe.The new market state (
skillsTargetApp+ one-time load flags) is initialized consistently and should support the intended lazy-load flow without changing existing behavior.Also applies to: 187-189
web-ui/modules/config-mode.computed.mjs (1)
52-52: Good addition for inspector tab labeling.
marketnow has an explicit label, which avoids falling back to “未知” for the new tab.site/guide/getting-started.md (1)
30-31: Docs update is clear and operationally complete.The new default host guidance is paired with a local-only fallback, which keeps the setup instructions practical and safe.
web-ui/session-helpers.mjs (1)
64-69: No action needed—implementation is safe.Line 68's fire-and-forget call to
loadSkillsMarketOverviewis safe from unhandled rejections. BothrefreshSkillsList(lines 80–117) andscanImportableSkills(lines 181–211) inskills.methods.mjshave internal try-catch blocks that catch all errors and return boolean values.loadSkillsMarketOverviewitself has a try-finally structure with no uncaught error paths; errors in the called methods are already handled internally. The hardening suggestion is unnecessary.tests/unit/session-tab-switch-performance.test.mjs (2)
151-170: Test may fail: implementation lacks re-entry guard for market tab.The test expects
switchMainTab('market')to NOT callloadSkillsMarketOverviewwhen already on the'market'tab. However, examining theswitchMainTabimplementation inweb-ui/session-helpers.mjs(lines 64-67 in the context snippet), the condition is:const shouldLoadSkillsMarketOnEnter = nextTab === 'market' && typeof this.loadSkillsMarketOverview === 'function';This does not include a
previousTab !== 'market'guard, meaningloadSkillsMarketOverviewwill be called even when re-entering the same tab. The assertion at line 169 (calls = []) will fail.Note:
tests/unit/config-tabs-ui.test.mjs(line 236) asserts thatpreviousTab !== 'market'exists in the helper script, which suggests the implementation should have this guard but it appears missing from the provided context.[raise_major_issue, request_verification]
#!/bin/bash # Verify if the previousTab !== 'market' guard exists in the actual implementation rg -n "previousTab !== 'market'" web-ui/session-helpers.mjs
130-149: LGTM!The test for entering the market tab from a non-market tab correctly asserts that
loadSkillsMarketOverview({ silent: true })is called exactly once.tests/unit/run.mjs (1)
16-16: LGTM!New test modules for skills market runtime and web host configuration are correctly added to the test runner, following the existing import pattern.
Also applies to: 29-29
README.md (2)
127-129: Security note adequately addresses LAN exposure concern.The documentation now includes a clear security warning about the unauthenticated management UI being exposed on LAN by default, with instructions to use
127.0.0.1for local-only access. This addresses the previously flagged security concern.
57-62: LGTM!Skills Market documentation is comprehensive, covering target switching, local skills management, import/export capabilities, and batch operations. The feature sections are well-structured.
Also applies to: 201-206
web-ui/modules/skills.computed.mjs (2)
3-8: LGTM!Clean computed properties for target label and default root path. The ternary logic correctly maps
'claude'to "Claude Code" /~/.claude/skillsand defaults to "Codex" /~/.codex/skills.
87-105: LGTM!The
skillsMarketBusyaggregator properly combines all relevant loading/action flags with explicit boolean coercion. The preview computed properties correctly slice the first 6 items with safe array fallbacks.tests/unit/skills-modal-ui.test.mjs (2)
43-59: LGTM!The assertions comprehensively verify the skills module exports and API calls. The negative assertions correctly validate the removal of online/ecosystem market features, aligning with the local-first approach.
75-77: LGTM!Style assertions correctly verify the presence of new market target styling and absence of removed online/ecosystem style hooks.
README.en.md (2)
127-129: Documentation correctly reflects implementation.The tests in
web-run-host.test.mjs(lines 126-129) confirm thatresolveWebHostdefaults to'0.0.0.0', so the documentation stating0.0.0.0as the default is accurate. The safety note appropriately warns users about LAN exposure.[approve_code_changes, duplicate_comment]
Also applies to: 237-237
57-62: LGTM!Skills Market documentation mirrors the Chinese README and comprehensively documents the local-first market features. The static analysis hints flagging "Skills", "Code", "status", and "delete" as spelling errors are false positives.
Also applies to: 201-206
tests/unit/web-run-host.test.mjs (3)
14-90: LGTM!The
findMatchingBraceparser correctly handles JavaScript edge cases: string literals, template literals with embedded expressions (${}), line comments, and block comments. The depth tracking for template literal nesting is particularly well-handled.
126-147: LGTM!Tests correctly verify that
resolveWebHostdefaults to'0.0.0.0', CLI host takes precedence over environment, and environment host takes precedence over the default. This validates the LAN-by-default behavior.
194-224: LGTM!Skill target resolution tests properly verify fallback behavior, rejection of explicitly unsupported targets, and the request-based target parsing including query parameter handling.
tests/unit/config-tabs-ui.test.mjs (4)
37-63: LGTM!Comprehensive assertions for the market tab UI surface including tab bindings, ARIA attributes, action handlers, target switching, and proper negative assertions to verify removal of online/ecosystem market features.
235-237: Validates expected market tab entry guard.This assertion confirms that
switchMainTabshould includepreviousTab !== 'market'as a guard condition. This aligns with my earlier observation insession-tab-switch-performance.test.mjswhere the test expects no reload when already on the market tab.
177-185: LGTM!State initialization assertions correctly verify market-related state fields and confirm removal of remote/ecosystem market state (matching the local-first approach).
275-281: LGTM!Style assertions verify market grid layouts and target styling are present, while confirming online/ecosystem styles are correctly removed.
|
@coderabbitai re-review ！Stop making breaking changes, do a proper review！ |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
cli.js (2)
67-68:⚠️ Potential issue | 🔴 CriticalDo not re-expose the unauthenticated Web UI on every interface by default.
This reintroduces the earlier LAN-exposure issue. A plain
codexmate runnow publishes every mutable/apiroute on the network, including provider, auth, session, and skills writes, before the user has opted in to remote access.🔒 Minimal fix
-const DEFAULT_WEB_HOST = '0.0.0.0'; +const DEFAULT_WEB_HOST = '127.0.0.1'; const DEFAULT_WEB_OPEN_HOST = '127.0.0.1';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 67 - 68, Change the default hosts to bind only to localhost to avoid exposing the unauthenticated Web UI by default: update the constants DEFAULT_WEB_HOST and DEFAULT_WEB_OPEN_HOST in cli.js so they are both '127.0.0.1' (instead of DEFAULT_WEB_HOST = '0.0.0.0'), ensuring the server binds to loopback unless the user explicitly opts into remote access.
10347-10348:⚠️ Potential issue | 🟡 MinorUse
::1when auto-opening a server bound to::.This still collapses both any-address hosts to
127.0.0.1. On IPv6-only machines,codexmate run --host ::then opens an IPv4 URL that may not be listening.🩹 Minimal fix
- const openHost = isAnyAddressHost(host) ? DEFAULT_WEB_OPEN_HOST : host; + const openHost = host === '::' + ? '::1' + : (host === '0.0.0.0' ? DEFAULT_WEB_OPEN_HOST : host);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 10347 - 10348, The current logic maps any-address hosts to a single DEFAULT_WEB_OPEN_HOST which becomes 127.0.0.1 even for IPv6 any-addresses; update the selection so that when isAnyAddressHost(host) is true you pick an IPv6 loopback (::1) if the original host is IPv6 (e.g., host === '::' or host.includes(':')), otherwise fall back to the IPv4 loopback (127.0.0.1); change the code that sets openHost (referencing isAnyAddressHost, DEFAULT_WEB_OPEN_HOST, formatHostForUrl, openHost, host, port) to use a DEFAULT_WEB_OPEN_HOST_IPV6 ('::1') for IPv6 any-addresses so openUrl uses the correct loopback address.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli.js`:
- Around line 175-205: getCodexSkillsDir and getClaudeSkillsDir currently append
'skills' after resolving an existing config directory, which can pick
~/.config/codex or ~/.config/claude even when the real skills live in
~/.codex/skills or ~/.claude/skills; change each to resolve the concrete skills
directory by passing candidate paths that already include 'skills' to
resolveExistingDir (e.g. CODEX_HOME/CODEX_HOME/skills,
XDG_CONFIG_HOME/codex/skills, ~/.codex/skills for getCodexSkillsDir and
analogous candidates for getClaudeSkillsDir) and then return the resolved path
directly instead of appending 'skills' afterward; keep using the same env vars
(CODEX_HOME, CLAUDE_HOME, CLAUDE_CONFIG_DIR, XDG_CONFIG_HOME) and the same
fallback constants (CONFIG_DIR, CLAUDE_DIR) when building the candidate list so
resolveExistingDir picks the actual skills tree.
---
Duplicate comments:
In `@cli.js`:
- Around line 67-68: Change the default hosts to bind only to localhost to avoid
exposing the unauthenticated Web UI by default: update the constants
DEFAULT_WEB_HOST and DEFAULT_WEB_OPEN_HOST in cli.js so they are both
'127.0.0.1' (instead of DEFAULT_WEB_HOST = '0.0.0.0'), ensuring the server binds
to loopback unless the user explicitly opts into remote access.
- Around line 10347-10348: The current logic maps any-address hosts to a single
DEFAULT_WEB_OPEN_HOST which becomes 127.0.0.1 even for IPv6 any-addresses;
update the selection so that when isAnyAddressHost(host) is true you pick an
IPv6 loopback (::1) if the original host is IPv6 (e.g., host === '::' or
host.includes(':')), otherwise fall back to the IPv4 loopback (127.0.0.1);
change the code that sets openHost (referencing isAnyAddressHost,
DEFAULT_WEB_OPEN_HOST, formatHostForUrl, openHost, host, port) to use a
DEFAULT_WEB_OPEN_HOST_IPV6 ('::1') for IPv6 any-addresses so openUrl uses the
correct loopback address.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3021cfe8-0f49-4c55-bc24-2425b835a2f4
📒 Files selected for processing (2)
cli.jstests/unit/web-run-host.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/web-run-host.test.mjs
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
|
@coderabbitai re-review ！Stop making breaking changes, do a proper review！ |
|
✏️ Learnings added
🧠 Learnings used✅ Actions performedFull review triggered. |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
cli.js (2)
67-68:⚠️ Potential issue | 🔴 CriticalThis reintroduces LAN-wide exposure for the unauthenticated admin UI.
codexmate runnow binds a mutating Web UI to all interfaces by default, while startup still warns that the surface has no auth. That should stay opt-in via--host/CODEXMATE_HOST; the warning alone does not mitigate the exposure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 67 - 68, The default web host should not expose the unauthenticated admin UI to the LAN; change the default so the mutating Web UI binds to localhost unless explicitly requested. Revert or set DEFAULT_WEB_HOST to '127.0.0.1' (keep DEFAULT_WEB_OPEN_HOST as '127.0.0.1' if needed) and only switch to '0.0.0.0' when the user passes --host or sets CODEXMATE_HOST explicitly; update the startup logic that chooses the bind address (the code that reads DEFAULT_WEB_HOST / DEFAULT_WEB_OPEN_HOST and the CLI/env handling for --host / CODEXMATE_HOST) so that LAN binding is opt-in rather than the default.
175-205:⚠️ Potential issue | 🟠 MajorResolve the concrete
.../skillsdirectories here.These helpers still return
.../skillsunder the first matching config home instead of resolving the concrete existing skills-directory candidates. On machines that have~/.config/codex/~/.config/claudebut keep populated skills in~/.codex/skills/~/.claude/skills, scan/import/export/delete will target the wrong tree and can create a second empty skills directory.🩹 Minimal fix
function getCodexSkillsDir() { - const envCodexHome = typeof process.env.CODEX_HOME === 'string' ? process.env.CODEX_HOME.trim() : ''; - if (envCodexHome) { - return path.join(envCodexHome, 'skills'); - } - const xdgConfig = typeof process.env.XDG_CONFIG_HOME === 'string' ? process.env.XDG_CONFIG_HOME.trim() : ''; - if (xdgConfig) { - return path.join(xdgConfig, 'codex', 'skills'); - } - const resolvedConfigDir = resolveExistingDir([ - path.join(os.homedir(), '.config', 'codex') - ], CONFIG_DIR); - return path.join(resolvedConfigDir, 'skills'); + const candidates = []; + const envCodexHome = typeof process.env.CODEX_HOME === 'string' ? process.env.CODEX_HOME.trim() : ''; + if (envCodexHome) { + candidates.push(path.join(envCodexHome, 'skills')); + } + const xdgConfig = typeof process.env.XDG_CONFIG_HOME === 'string' ? process.env.XDG_CONFIG_HOME.trim() : ''; + if (xdgConfig) { + candidates.push(path.join(xdgConfig, 'codex', 'skills')); + } + candidates.push(path.join(os.homedir(), '.config', 'codex', 'skills')); + candidates.push(CODEX_SKILLS_DIR); + return resolveExistingDir(candidates, CODEX_SKILLS_DIR); } function getClaudeSkillsDir() { - const envClaudeHome = typeof process.env.CLAUDE_HOME === 'string' && process.env.CLAUDE_HOME.trim() - ? process.env.CLAUDE_HOME.trim() - : (typeof process.env.CLAUDE_CONFIG_DIR === 'string' ? process.env.CLAUDE_CONFIG_DIR.trim() : ''); - if (envClaudeHome) { - return path.join(envClaudeHome, 'skills'); - } - const xdgConfig = typeof process.env.XDG_CONFIG_HOME === 'string' ? process.env.XDG_CONFIG_HOME.trim() : ''; - if (xdgConfig) { - return path.join(xdgConfig, 'claude', 'skills'); - } - const resolvedConfigDir = resolveExistingDir([ - path.join(os.homedir(), '.config', 'claude') - ], CLAUDE_DIR); - return path.join(resolvedConfigDir, 'skills'); + const candidates = []; + const envClaudeHome = typeof process.env.CLAUDE_HOME === 'string' && process.env.CLAUDE_HOME.trim() + ? process.env.CLAUDE_HOME.trim() + : (typeof process.env.CLAUDE_CONFIG_DIR === 'string' ? process.env.CLAUDE_CONFIG_DIR.trim() : ''); + if (envClaudeHome) { + candidates.push(path.join(envClaudeHome, 'skills')); + } + const xdgConfig = typeof process.env.XDG_CONFIG_HOME === 'string' ? process.env.XDG_CONFIG_HOME.trim() : ''; + if (xdgConfig) { + candidates.push(path.join(xdgConfig, 'claude', 'skills')); + } + candidates.push(path.join(os.homedir(), '.config', 'claude', 'skills')); + candidates.push(CLAUDE_SKILLS_DIR); + return resolveExistingDir(candidates, CLAUDE_SKILLS_DIR); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 175 - 205, The helpers getCodexSkillsDir and getClaudeSkillsDir currently return the first matching config home with '/skills' appended rather than resolving which concrete skills directory already exists; update both functions to build a list of candidate skill paths (e.g. env path + '/skills', XDG_CONFIG_HOME + '/codex|claude/skills', os.homedir() + '/.codex/skills', os.homedir() + '/.config/codex|claude/skills') and pass that list into resolveExistingDir (using the existing CONFIG_DIR / CLAUDE_DIR fallback constants) so resolveExistingDir returns the actual existing skills directory; ensure the env and XDG branches also check for an existing skills dir (use trimmed env values + '/skills' as candidates) rather than immediately returning path.join(..., 'skills').
🤖 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 89-94: The README diagram's OPENCLAW node was changed to list
"~/.openclaw/*.json5 + workspace/AGENTS.md" but the Web UI and config-file docs
still refer to "~/.openclaw/openclaw.json"; update either the diagram or the
docs for consistency by restoring "~/.openclaw/openclaw.json" in the OPENCLAW
node or by updating the Web UI/config-file sections to reference the new
"~/.openclaw/*.json5" pattern (search for OPENCLAW, "~/.openclaw/openclaw.json",
and the Web UI/config-file headings to make the same choice across the README).
In `@web-ui/index.html`:
- Around line 1307-1322: Add the disabled binding to the market target switch
buttons so they reflect the same busy state used by other market actions: update
the two button elements that render the target chips (the ones using :class with
market-target-chip and the click handler setSkillsTargetApp and the reactive
state skillsTargetApp) to include :disabled="skillsMarketBusy". This ensures the
UI disables the Codex and Claude Code target buttons while skillsMarketBusy is
true, matching the behavior of refresh/scan/ZIP/manager buttons.
In `@web-ui/modules/skills.methods.mjs`:
- Around line 177-178: The guards preventing concurrent ops miss the
skillsDeleting flag; update the boolean checks in scanImportableSkills and the
other entrypoints referenced (the similar guard blocks around lines for
import/export handlers) to include this.skillsDeleting (e.g., if
(this.skillsScanningImports || this.skillsImporting || this.skillsZipImporting
|| this.skillsExporting || this.skillsDeleting) return false;) so deletes cannot
race with scan/import/zip-import/export operations; apply the same addition to
each equivalent guard check noted in the review.
In `@web-ui/session-helpers.mjs`:
- Around line 64-69: The call to this.loadSkillsMarketOverview({ silent: true })
can reject and currently has no error handling, so update the tab-switch logic
(the shouldLoadSkillsMarketOnEnter branch) to handle the Promise returned by
loadSkillsMarketOverview by either awaiting it (if the surrounding function is
async) or attaching a .catch(...) that swallows or logs the error; ensure you
reference the same call site (the shouldLoadSkillsMarketOnEnter check and the
loadSkillsMarketOverview invocation) and do not change behavior other than
preventing unhandled promise rejections.
In `@web-ui/styles.css`:
- Around line 3671-3815: The stylesheet uses the shared skills-panel styles but
the custom properties --radius-md, --font-weight-primary, and --font-size-large
are not defined, causing fallbacks; add definitions for these tokens (e.g., in
:root or a shared variables block loaded before the market styles) so classes
like .skills-panel, .skills-panel-title, .skills-summary-value and the market
components pick up the correct radius, font weight, and large font size; ensure
the names match exactly (--radius-md, --font-weight-primary, --font-size-large)
and provide values consistent with other token naming (e.g., numeric px/rem for
sizes and valid font-weight values).
---
Duplicate comments:
In `@cli.js`:
- Around line 67-68: The default web host should not expose the unauthenticated
admin UI to the LAN; change the default so the mutating Web UI binds to
localhost unless explicitly requested. Revert or set DEFAULT_WEB_HOST to
'127.0.0.1' (keep DEFAULT_WEB_OPEN_HOST as '127.0.0.1' if needed) and only
switch to '0.0.0.0' when the user passes --host or sets CODEXMATE_HOST
explicitly; update the startup logic that chooses the bind address (the code
that reads DEFAULT_WEB_HOST / DEFAULT_WEB_OPEN_HOST and the CLI/env handling for
--host / CODEXMATE_HOST) so that LAN binding is opt-in rather than the default.
- Around line 175-205: The helpers getCodexSkillsDir and getClaudeSkillsDir
currently return the first matching config home with '/skills' appended rather
than resolving which concrete skills directory already exists; update both
functions to build a list of candidate skill paths (e.g. env path + '/skills',
XDG_CONFIG_HOME + '/codex|claude/skills', os.homedir() + '/.codex/skills',
os.homedir() + '/.config/codex|claude/skills') and pass that list into
resolveExistingDir (using the existing CONFIG_DIR / CLAUDE_DIR fallback
constants) so resolveExistingDir returns the actual existing skills directory;
ensure the env and XDG branches also check for an existing skills dir (use
trimmed env values + '/skills' as candidates) rather than immediately returning
path.join(..., 'skills').
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5c0856cd-f644-45f0-8f69-2b6d23179aed
📒 Files selected for processing (17)
README.en.mdREADME.mdcli.jssite/guide/getting-started.mdtests/unit/config-tabs-ui.test.mjstests/unit/run.mjstests/unit/session-tab-switch-performance.test.mjstests/unit/skills-market-runtime.test.mjstests/unit/skills-modal-ui.test.mjstests/unit/web-run-host.test.mjsweb-ui/app.jsweb-ui/index.htmlweb-ui/modules/config-mode.computed.mjsweb-ui/modules/skills.computed.mjsweb-ui/modules/skills.methods.mjsweb-ui/session-helpers.mjsweb-ui/styles.css
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
🪛 LanguageTool
README.en.md
[grammar] ~57-~57: Ensure spelling is correct
Context: ...level delete (supports batch) Skills Market - Switch the skills install target b...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~58-~58: Ensure spelling is correct
Context: ...install target between Codex and Claude Code - Inspect local installed skills, root...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~59-~59: Ensure spelling is correct
Context: ...local installed skills, root paths, and status - Scan importable sources from Codex...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~61-~61: Ensure spelling is correct
Context: ...pp import, ZIP import/export, and batch delete Engineering Utilities - MCP stdio dom...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (14)
README.en.md (1)
127-130: Host default and safety caveat are clearly documented.These updates are consistent and reduce operator confusion around LAN exposure vs local-only binding.
Also applies to: 237-237
web-ui/modules/config-mode.computed.mjs (1)
52-52: Market tab label wiring looks correct.The new
mainTab === 'market'branch is consistent with the existing computed-label structure.tests/unit/session-tab-switch-performance.test.mjs (1)
130-170: Good regression coverage for market tab entry behavior.The new tests validate both initial market entry loading and the no-reload case when already on
market.site/guide/getting-started.md (1)
30-31: Startup guide host-binding note is clear and actionable.The default LAN bind plus explicit local-only override is documented succinctly.
web-ui/app.js (1)
173-189: State initialization for Skills Market is consistent and safe.The new target/app and market lifecycle flags are correctly initialized and fit the existing state model without introducing behavior regressions.
tests/unit/run.mjs (1)
16-29: New test modules are integrated correctly into the unit runner.Both imports follow the existing loading pattern and are placed correctly before test execution.
web-ui/modules/skills.computed.mjs (2)
3-8: Target-aware computed mappings are correct.The label/root-path derivation for Codex vs Claude is straightforward and behavior-safe.
87-105: Market computed helpers are well-bounded and defensive.Busy-state aggregation and preview slicing correctly handle non-array fallbacks and keep UI preview scope controlled.
tests/unit/skills-modal-ui.test.mjs (3)
43-47: Computed/method export assertions are correctly updated for target-aware skills flow.These checks align with the current module surface and guard against accidental API drift.
54-59: Endpoint assertions correctly enforce local market API usage.The updated expectations match the unified local skills operations and explicitly prevent regression to the removed online market call.
75-77: CSS hook assertions are consistent with the current market UI structure.The presence/absence checks reflect the intended style contract after market UI changes.
web-ui/modules/skills.methods.mjs (1)
81-83: Top-leveltargetAppwiring matches the backend contract.These calls now pass
targetAppexactly where the generic skills handlers expect it, so the new list/scan/import/export/delete flow should resolve the intended host correctly.Also applies to: 182-184, 231-234, 353-356, 410-413
tests/unit/skills-market-runtime.test.mjs (1)
11-190: Good lightweight coverage for the new target-aware runtime paths.By composing the real methods and computed getters, this suite pins the generic
targetApprequest shape, the busy-state no-op paths, the same-target reopen behavior, and the market overview sequencing without needing the full UI shell.tests/unit/web-run-host.test.mjs (1)
126-154: No issues found—test assertions are satisfied by the current implementation.The verification confirms that
cli.jshas been updated correctly:
DEFAULT_WEB_HOSTis'0.0.0.0'(line 67)resolveWebHost()returns the expected default (lines 215–225)handleImportSkillsZipUpload()computesforcedTargetAppand uses target-specific ZIP fallback names like${targetApp}-skills.zip(lines 9806–9812)All assertions on lines 127, 152, 308, 352, 355, and 366 are correctly implemented.
|
Triggering a proper full re-review now. 🧠 Learnings used✅ Actions performedFull review triggered. |
|
Triggering a proper re-review now with this enforced as a hard constraint on every single comment. 🧠 Learnings used✅ Actions performedFull review triggered. |
|
Triggering a proper full re-review now with this enforced as a hard rule on every single comment. Re-review instruction: Apply one strict, non-negotiable constraint throughout the entire review of PR 🧠 Learnings used✅ Actions performedFull review triggered. |
|
Triggering a proper re-review now. Re-review instruction: Apply one strict, non-negotiable constraint throughout the entire review of PR 🧠 Learnings used✅ Actions performedFull review triggered. |
|
Triggering a proper full re-review now with this enforced as a hard rule on every single comment. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-ui/modules/skills.methods.mjs (1)
104-144:⚠️ Potential issue | 🟠 MajorRefresh/scan must keep
skillsMarket*LoadedOncetruthful.Both the market page and the modal call
refreshSkillsList()/scanImportableSkills()directly, but these methods never updateskillsMarketLocalLoadedOnce/skillsMarketImportLoadedOnce. After a slice has loaded once, a later manual refresh or scan failure clears the visible data while leaving the flagtrue, so the nextloadSkillsMarketOverview()can skip that slice and keep the empty/stale result until the user retries manually. Set/clear the matching loaded-once flag inside each method based on that call’s outcome.Also applies to: 203-239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/modules/skills.methods.mjs` around lines 104 - 144, refreshSkillsList and scanImportableSkills currently don't update the "loaded once" flags, causing loadSkillsMarketOverview to skip stale/empty slices; inside refreshSkillsList (and similarly scanImportableSkills) set skillsMarketLocalLoadedOnce = true on a successful load (when exists true and items processed) and set it to false on branches that clear data (exists === false, res.error, or the catch/failure path); likewise set/clear skillsMarketImportLoadedOnce in scanImportableSkills based on that method's success/failure, so the flags always reflect the real outcome of each call and allow loadSkillsMarketOverview to reattempt loading when appropriate.
♻️ Duplicate comments (2)
cli.js (1)
67-68:⚠️ Potential issue | 🔴 CriticalDon't expose the unauthenticated Web UI on all interfaces by default.
DEFAULT_WEB_HOST = '0.0.0.0'makes a stockcodexmate runreachable from the LAN even though this server still exposes write surfaces for providers, auth profiles, sessions, and skills without auth. The startup warning later on is advisory only; it does not reduce that remote attack surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 67 - 68, Change the default host so the unauthenticated web UI is not bound to all interfaces: replace DEFAULT_WEB_HOST = '0.0.0.0' with a loopback-only address (e.g., '127.0.0.1') so the server is not reachable from the LAN by default; keep DEFAULT_WEB_OPEN_HOST as-is if it’s used for intentional open bindings, and update any startup/help text or tests referencing DEFAULT_WEB_HOST to reflect the new safer default.web-ui/app.js (1)
3578-3584:⚠️ Potential issue | 🟠 MajorFail closed if the disable predicate throws.
deleteSelectedSkills()passes a liveconfirmDisabledpredicate for a destructive dialog. If that predicate throws here, the catch path currently returnsfalseand re-enables the confirm button instead of keeping it blocked.🔒 Minimal safety fix
isConfirmDialogDisabled() { if (typeof this.confirmDialogDisableWhen === 'function') { try { return !!this.confirmDialogDisableWhen.call(this); } catch (_) { - return false; + return true; } } return !!this.confirmDialogConfirmDisabled; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/app.js` around lines 3578 - 3584, The isConfirmDialogDisabled() handler currently returns false on exception which re-enables the confirm button; change the catch path to return true so exceptions cause the dialog to remain disabled (fail-closed). Locate isConfirmDialogDisabled and update its catch block to return true (optionally log the error) so live predicates like the one passed from deleteSelectedSkills() cannot re-enable the destructive confirm when they throw.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli.js`:
- Around line 9859-9868: When returning early for non-POST requests (the
req.method !== 'POST' branch) or when targetApp is missing (the !targetApp
branch), ensure the incoming request body is drained before sending the error
response to avoid leaving the socket open; call req.resume() (or otherwise
consume the stream) prior to writeJsonResponse in those early-return branches so
that the listeners created in writeUploadZipStream() are not required to drain
the body.
In `@web-ui/modules/skills.methods.mjs`:
- Around line 241-242: The guard that blocks concurrent skill mutations must
also include the skillsLoading flag to prevent stale-list races; update the
early-return condition in importSelectedSkills (and the analogous guards in
deleteSelectedSkills, importSkillsFromZip / importZipSkills, and
exportSelectedSkills) so they check this.skillsLoading alongside
this.skillsDeleting, this.skillsScanningImports, this.skillsImporting,
this.skillsZipImporting, and this.skillsExporting, returning early when any are
true.
---
Outside diff comments:
In `@web-ui/modules/skills.methods.mjs`:
- Around line 104-144: refreshSkillsList and scanImportableSkills currently
don't update the "loaded once" flags, causing loadSkillsMarketOverview to skip
stale/empty slices; inside refreshSkillsList (and similarly
scanImportableSkills) set skillsMarketLocalLoadedOnce = true on a successful
load (when exists true and items processed) and set it to false on branches that
clear data (exists === false, res.error, or the catch/failure path); likewise
set/clear skillsMarketImportLoadedOnce in scanImportableSkills based on that
method's success/failure, so the flags always reflect the real outcome of each
call and allow loadSkillsMarketOverview to reattempt loading when appropriate.
---
Duplicate comments:
In `@cli.js`:
- Around line 67-68: Change the default host so the unauthenticated web UI is
not bound to all interfaces: replace DEFAULT_WEB_HOST = '0.0.0.0' with a
loopback-only address (e.g., '127.0.0.1') so the server is not reachable from
the LAN by default; keep DEFAULT_WEB_OPEN_HOST as-is if it’s used for
intentional open bindings, and update any startup/help text or tests referencing
DEFAULT_WEB_HOST to reflect the new safer default.
In `@web-ui/app.js`:
- Around line 3578-3584: The isConfirmDialogDisabled() handler currently returns
false on exception which re-enables the confirm button; change the catch path to
return true so exceptions cause the dialog to remain disabled (fail-closed).
Locate isConfirmDialogDisabled and update its catch block to return true
(optionally log the error) so live predicates like the one passed from
deleteSelectedSkills() cannot re-enable the destructive confirm when they throw.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5c9da4fe-a602-48ef-a770-09a12327aeac
📒 Files selected for processing (19)
README.en.mdREADME.mdcli.jssite/guide/getting-started.mdtests/e2e/run.jstests/unit/config-tabs-ui.test.mjstests/unit/readme-docs-consistency.test.mjstests/unit/run.mjstests/unit/session-tab-switch-performance.test.mjstests/unit/skills-market-runtime.test.mjstests/unit/skills-modal-ui.test.mjstests/unit/web-run-host.test.mjsweb-ui/app.jsweb-ui/index.htmlweb-ui/modules/config-mode.computed.mjsweb-ui/modules/skills.computed.mjsweb-ui/modules/skills.methods.mjsweb-ui/session-helpers.mjsweb-ui/styles.css
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
🪛 LanguageTool
README.en.md
[grammar] ~57-~57: Ensure spelling is correct
Context: ...level delete (supports batch) Skills Market - Switch the skills install target b...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~58-~58: Ensure spelling is correct
Context: ...install target between Codex and Claude Code - Inspect local installed skills, root...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~59-~59: Ensure spelling is correct
Context: ...local installed skills, root paths, and status - Scan importable sources from Codex...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~61-~61: Ensure spelling is correct
Context: ...pp import, ZIP import/export, and batch delete Engineering Utilities - MCP stdio dom...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (20)
web-ui/modules/config-mode.computed.mjs (1)
52-52: LGTM!The new
'market'tab label follows the existing pattern and integrates cleanly with the other tab labels.tests/e2e/run.js (1)
43-46: LGTM! Proper E2E test isolation.Setting these environment variables to empty strings ensures E2E tests won't inadvertently access real user config directories. The CLI code (per context snippets at
cli.js:175-189andcli.js:190-214) safely handles empty strings via.trim()+ truthiness checks, correctly falling back to the temporary home directory.tests/unit/readme-docs-consistency.test.mjs (1)
1-25: LGTM! Good documentation consistency guard.The test correctly verifies that both README files contain the expected OpenClaw state diagram entry, preventing documentation drift between localized versions. The regex properly escapes special characters.
One note: the
test()function is used as a global, which is fine for Node.js's built-in test runner (node --test) or similar frameworks, but ensure your test runner provides this global.web-ui/modules/skills.computed.mjs (2)
3-8: LGTM!Clean implementation of target-aware label and path getters following the existing pattern.
86-107: LGTM!The new market-related computed properties are well-implemented:
skillsMarketBusy()correctly aggregates all 7 loading/busy flags (all confirmed initialized inweb-ui/app.jsper context snippet)- Preview getters safely handle non-array inputs and limit to 6 items
web-ui/styles.css (3)
40-40: LGTM!The new design tokens (
--font-size-large,--font-weight-primary,--radius-md) are well-placed in the token scale and address the previously flagged missing token definitions.Also applies to: 46-46, 64-64
3674-3818: LGTM!The market UI styles are well-structured:
- Layout classes follow existing grid patterns
- Target chip uses consistent pill styling with proper active state
- Preview and action card classes maintain visual consistency with existing skills panel styles
- Hover/disabled states are properly handled with appropriate transitions
4328-4331: LGTM!Responsive breakpoints are consistent with existing media queries in the file, and the grid collapse behavior at 720px provides appropriate mobile layout.
Also applies to: 4358-4364
tests/unit/session-tab-switch-performance.test.mjs (3)
130-149: LGTM!Test correctly verifies that
loadSkillsMarketOverview({ silent: true })is invoked when switching from a non-market tab to the market tab. The mock VM setup and assertion logic align with the implementation inweb-ui/session-helpers.mjs(lines 64-69).
151-179: LGTM! Rejection swallowing test is well-structured.The test correctly verifies that rejected promises from
loadSkillsMarketOverviewdon't surface as unhandled rejections. UsingsetImmediateto yield to the event loop after the synchronousswitchMainTabcall is a reasonable approach to allow the microtask queue to flush before checking for unhandled rejections.
181-200: LGTM!Test correctly verifies the guard condition
previousTab !== 'market'by confirming thatloadSkillsMarketOverviewis not called when already on the market tab.site/guide/getting-started.md (1)
30-32: Documentation now aligns with runtime behavior.The updated documentation correctly reflects the
DEFAULT_WEB_HOST = '0.0.0.0'setting fromcli.js(line 67), and appropriately includes a security warning about LAN exposure. The instruction to use127.0.0.1for localhost-only access is helpful.tests/unit/run.mjs (1)
16-17: LGTM!New test module imports follow the established pattern and extend coverage for the skills market runtime, README documentation consistency, and web host configuration behavior.
Also applies to: 30-30
web-ui/session-helpers.mjs (1)
64-69: LGTM! Clean fire-and-forget pattern with proper rejection handling.The implementation correctly:
- Guards against redundant loads when already on the market tab (
previousTab !== 'market')- Defensively checks that
loadSkillsMarketOverviewexists as a function- Uses
void Promise.resolve(...).catch(() => {})to suppress rejections without blocking tab switchingThis aligns with the existing patterns in this file (e.g., trash count priming at lines 57-62) and is covered by the corresponding unit tests.
README.md (6)
26-29: LGTM!Clear and concise addition of the local-first Skills Market capability, emphasizing the local-only nature (no remote market dependency).
57-61: LGTM!The Skills Market feature section accurately documents the implementation: target switching between Codex and Claude Code, local skills inspection, cross-app import scanning, and ZIP distribution support.
85-85: LGTM!Architecture diagram updates correctly reflect the new Skills Market service and the SKILLS state node aggregating
~/.codex/skills,~/.claude/skills, and~/.agents/skillsdirectories.Also applies to: 90-94, 112-112
127-129: Security warning is appropriate for the new default.The documentation correctly reflects the
DEFAULT_WEB_HOST = '0.0.0.0'change and includes a clear security warning about LAN exposure of the unauthenticated management interface. The guidance to use127.0.0.1for sensitive environments is helpful.
201-205: LGTM!The Skills Market tab documentation accurately describes the UI capabilities: target switching, local skills display, unmanaged skills scanning, cross-app import, and ZIP distribution.
238-238: Environment variable documentation aligns with implementation.The
CODEXMATE_HOSTdefault value of0.0.0.0matches theDEFAULT_WEB_HOSTconstant incli.js(line 67), and the note about explicitly setting127.0.0.1for localhost-only access is consistent with the security guidance above.
|
To permanently enforce this constraint and prevent future violations, I recommend adding it directly to the CodeRabbit configuration for this repository. You can do this by adding a reviews:
instructions: |
Do not suggest any breaking changes, API/interface/behavior alterations, or refactoring of any kind.
Limit all feedback exclusively to correctness, safety, and quality issues within the existing design.
Do not suggest changes that alter existing APIs, interfaces, or behaviors.This will embed the constraint at the configuration level so it applies to every review automatically, without relying on per-comment instructions. You can read more at https://docs.coderabbit.ai/guides/review-instructions. Triggering a proper re-review now. Re-review instruction: Apply one strict, non-negotiable constraint throughout the entire review — no breaking changes, no API/interface/behavior alterations, no refactoring suggestions of any kind. Every comment must be limited exclusively to correctness, safety, or quality issues within the existing design as implemented in this PR. Do not suggest any change that alters an existing API, interface, or behavior. 🧠 Learnings used✅ Actions performedFull review triggered. |
|
Triggering a proper full re-review now with this enforced as a hard rule on every single comment. 🧠 Learnings used✅ Actions performedFull review triggered. |
|
Triggering a proper full re-review now with this as a hard, non-negotiable rule on every single comment. 🧠 Learnings used✅ Actions performedFull review triggered. |
|
To permanently stop this loop, I strongly recommend encoding the constraint directly in your CodeRabbit configuration so it applies to every review automatically. Create or update reviews:
instructions: |
Do not suggest any breaking changes, API/interface/behavior alterations, or refactoring of any kind.
Limit all feedback exclusively to correctness, safety, and quality issues within the existing design.
Do not suggest changes that alter existing APIs, interfaces, or behaviors.This will make the constraint permanent at the configuration level rather than relying on per-comment instructions. See https://docs.coderabbit.ai/guides/review-instructions for details. Triggering a full re-review now. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
web-ui/app.js (1)
3578-3587:⚠️ Potential issue | 🟠 MajorFail closed when the confirm-disable predicate throws.
Line 3583 returns
falseon guard errors, which re-enables the confirm button exactly when the safety check fails. That weakens the new function-based confirm guard for destructive dialogs.🔒 Minimal safety fix
isConfirmDialogDisabled() { if (typeof this.confirmDialogDisableWhen === 'function') { try { return !!this.confirmDialogDisableWhen.call(this); } catch (_) { - return false; + return true; } } return !!this.confirmDialogConfirmDisabled; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/app.js` around lines 3578 - 3587, The current isConfirmDialogDisabled() swallows exceptions from the user-supplied predicate confirmDialogDisableWhen and returns false (re-enabling the confirm control) on error; change the error path to "fail closed" by returning true when the predicate throws so destructive actions stay disabled on predicate failure—update the catch block inside isConfirmDialogDisabled() to return true (and optionally log the exception) while keeping the normal boolean coercions for confirmDialogDisableWhen and confirmDialogConfirmDisabled intact.web-ui/modules/skills.methods.mjs (1)
241-242:⚠️ Potential issue | 🟠 Major
skillsLoadingis still missing from the mutating entrypoint guards.If
refreshSkillsList()is already in flight, import/delete can still start from stale modal state, and the older list response can land after the mutation’s own refresh and put just-changed items back into the UI until the next reload.🩹 Minimal fix
- if (this.skillsDeleting || this.skillsScanningImports || this.skillsImporting || this.skillsZipImporting || this.skillsExporting) return; + if (this.skillsLoading || this.skillsDeleting || this.skillsScanningImports || this.skillsImporting || this.skillsZipImporting || this.skillsExporting) return;- if (this.skillsDeleting || this.skillsScanningImports || this.skillsZipImporting || this.skillsImporting || this.skillsExporting) return; + if (this.skillsLoading || this.skillsDeleting || this.skillsScanningImports || this.skillsZipImporting || this.skillsImporting || this.skillsExporting) return;- if (this.skillsDeleting || this.skillsZipImporting || this.skillsExporting || this.skillsImporting) return; + if (this.skillsLoading || this.skillsDeleting || this.skillsZipImporting || this.skillsExporting || this.skillsImporting) return;Also applies to: 333-335, 414-419
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/modules/skills.methods.mjs` around lines 241 - 242, The guard that prevents concurrent mutations in importSelectedSkills is missing skillsLoading, allowing a refreshSkillsList() in-flight to race with imports/deletes; update the initial boolean guard in async importSelectedSkills to include this.skillsLoading in the OR chain (i.e., if (this.skillsLoading || this.skillsDeleting || this.skillsScanningImports || this.skillsImporting || this.skillsZipImporting || this.skillsExporting) return;), and apply the same change to the other mutating entrypoints referenced around the other locations (the methods around the 333-335 and 414-419 ranges) so all mutation starters bail out while skillsLoading is true.cli.js (2)
9858-9868:⚠️ Potential issue | 🟠 MajorDrain rejected upload requests before returning 400/405.
Line 9860 and Line 9867 return before
writeUploadZipStream()attaches any body listeners, so the incoming upload can stay unread and keep the socket busy until timeout.🩹 Minimal fix
async function handleImportSkillsZipUpload(req, res, options = {}) { if (req.method !== 'POST') { + req.resume(); writeJsonResponse(res, 405, { error: 'Method Not Allowed' }); return; } try { const forcedTargetApp = normalizeSkillTargetApp(options && options.targetApp ? options.targetApp : ''); const targetApp = forcedTargetApp || resolveSkillTargetAppFromRequest(req, 'codex'); if (!targetApp) { + req.resume(); writeJsonResponse(res, 400, { error: '目标宿主不支持' }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 9858 - 9868, In handleImportSkillsZipUpload, early returns for non-POST and missing targetApp leave the request body unread; before returning a 405 or 400 call the same drain routine used for uploads (e.g., invoke writeUploadZipStream or otherwise attach 'data'/'end' listeners or call req.resume()) to consume the incoming body and free the socket; update the branches that call writeJsonResponse for Method Not Allowed and 目标宿主不支持 to drain the request (use resolveSkillTargetAppFromRequest and writeUploadZipStream identifiers to locate the logic).
67-68:⚠️ Potential issue | 🔴 CriticalDon't expose the unauthenticated Web UI on all interfaces by default.
Line 67 binds an admin surface to
0.0.0.0, so every mutating/apihandler in this file becomes LAN-reachable without auth. The warning at Lines 10415-10417 is only informational; it does not reduce the exposure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 67 - 68, DEFAULT_WEB_HOST is set to '0.0.0.0' which exposes the unauthenticated Web UI to the LAN; change the default to the loopback address and require an explicit opt-in to open to all interfaces. Update the constant DEFAULT_WEB_HOST from '0.0.0.0' to '127.0.0.1' (or switch code paths that bind admin surfaces to use DEFAULT_WEB_OPEN_HOST) so admin/mutating endpoints bind to loopback by default, and ensure any code that relies on DEFAULT_WEB_HOST (e.g., the web server bind/listen call) only binds to 0.0.0.0 when an explicit flag is provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/index.html`:
- Around line 1297-1303: The click handlers currently bind the method reference
so the native MouseEvent can be passed into openSkillsManager; change the two
`@click` handlers in the market buttons to call openSkillsManager explicitly (e.g.
use openSkillsManager() or openSkillsManager({}) if the implementation expects
an options object) so the method receives no DOM event or a proper options
object; locate the handlers by the unique method name openSkillsManager in
web-ui/index.html and update both market buttons that use
`@click`="openSkillsManager".
In `@web-ui/modules/skills.methods.mjs`:
- Around line 67-87: openSkillsManager() can change this.skillsTargetApp and
call refreshSkillsList() while other operations are running; reuse the same
busy-state guard used by setSkillsTargetApp() instead of directly mutating
state. Modify openSkillsManager to either call/await
setSkillsTargetApp(targetApp) (which performs the busy check and state reset) or
early-return when this.skillsBusy is true, and only call
this.resetSkillsTargetState(), set this.showSkillsModal, and await
this.refreshSkillsList() after setSkillsTargetApp completes; keep
showUnsupportedSkillsTargetMessage and the existing fallback behavior when
normalizeSkillsTargetApp throws.
In `@web-ui/session-helpers.mjs`:
- Around line 64-69: The fire-and-forget call to loadSkillsMarketOverview can
throw synchronously before Promise.resolve can wrap it; modify the
shouldLoadSkillsMarketOnEnter branch so you first call loadSkillsMarketOverview
inside a try/catch to guard synchronous exceptions (use the
this.loadSkillsMarketOverview({ silent: true }) call inside try), then pass the
returned value to Promise.resolve(...).catch(...) to handle asynchronous
rejections; reference shouldLoadSkillsMarketOnEnter and loadSkillsMarketOverview
when making this change.
---
Duplicate comments:
In `@cli.js`:
- Around line 9858-9868: In handleImportSkillsZipUpload, early returns for
non-POST and missing targetApp leave the request body unread; before returning a
405 or 400 call the same drain routine used for uploads (e.g., invoke
writeUploadZipStream or otherwise attach 'data'/'end' listeners or call
req.resume()) to consume the incoming body and free the socket; update the
branches that call writeJsonResponse for Method Not Allowed and 目标宿主不支持 to drain
the request (use resolveSkillTargetAppFromRequest and writeUploadZipStream
identifiers to locate the logic).
- Around line 67-68: DEFAULT_WEB_HOST is set to '0.0.0.0' which exposes the
unauthenticated Web UI to the LAN; change the default to the loopback address
and require an explicit opt-in to open to all interfaces. Update the constant
DEFAULT_WEB_HOST from '0.0.0.0' to '127.0.0.1' (or switch code paths that bind
admin surfaces to use DEFAULT_WEB_OPEN_HOST) so admin/mutating endpoints bind to
loopback by default, and ensure any code that relies on DEFAULT_WEB_HOST (e.g.,
the web server bind/listen call) only binds to 0.0.0.0 when an explicit flag is
provided.
In `@web-ui/app.js`:
- Around line 3578-3587: The current isConfirmDialogDisabled() swallows
exceptions from the user-supplied predicate confirmDialogDisableWhen and returns
false (re-enabling the confirm control) on error; change the error path to "fail
closed" by returning true when the predicate throws so destructive actions stay
disabled on predicate failure—update the catch block inside
isConfirmDialogDisabled() to return true (and optionally log the exception)
while keeping the normal boolean coercions for confirmDialogDisableWhen and
confirmDialogConfirmDisabled intact.
In `@web-ui/modules/skills.methods.mjs`:
- Around line 241-242: The guard that prevents concurrent mutations in
importSelectedSkills is missing skillsLoading, allowing a refreshSkillsList()
in-flight to race with imports/deletes; update the initial boolean guard in
async importSelectedSkills to include this.skillsLoading in the OR chain (i.e.,
if (this.skillsLoading || this.skillsDeleting || this.skillsScanningImports ||
this.skillsImporting || this.skillsZipImporting || this.skillsExporting)
return;), and apply the same change to the other mutating entrypoints referenced
around the other locations (the methods around the 333-335 and 414-419 ranges)
so all mutation starters bail out while skillsLoading is true.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b33e6467-bd26-4afe-b692-5513a510aa6b
📒 Files selected for processing (19)
README.en.mdREADME.mdcli.jssite/guide/getting-started.mdtests/e2e/run.jstests/unit/config-tabs-ui.test.mjstests/unit/readme-docs-consistency.test.mjstests/unit/run.mjstests/unit/session-tab-switch-performance.test.mjstests/unit/skills-market-runtime.test.mjstests/unit/skills-modal-ui.test.mjstests/unit/web-run-host.test.mjsweb-ui/app.jsweb-ui/index.htmlweb-ui/modules/config-mode.computed.mjsweb-ui/modules/skills.computed.mjsweb-ui/modules/skills.methods.mjsweb-ui/session-helpers.mjsweb-ui/styles.css
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
🪛 LanguageTool
README.en.md
[grammar] ~57-~57: Ensure spelling is correct
Context: ...level delete (supports batch) Skills Market - Switch the skills install target b...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~58-~58: Ensure spelling is correct
Context: ...install target between Codex and Claude Code - Inspect local installed skills, root...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~59-~59: Ensure spelling is correct
Context: ...local installed skills, root paths, and status - Scan importable sources from Codex...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~61-~61: Ensure spelling is correct
Context: ...pp import, ZIP import/export, and batch delete Engineering Utilities - MCP stdio dom...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (7)
web-ui/modules/config-mode.computed.mjs (1)
52-52: Main-tab label coverage is correct formarket.Nice catch adding the explicit
marketlabel path; this avoids falling back to'未知'in inspector state.tests/unit/run.mjs (1)
16-17: Test runner wiring looks good.These imports correctly register the new suites in the shared collector and keep execution flow unchanged.
Also applies to: 30-30
tests/unit/session-tab-switch-performance.test.mjs (1)
130-200: Strong regression coverage for the new market-tab path.These tests validate enter-only loading, rejection swallowing, and idempotent behavior when already on
market—exactly the risky edges here.site/guide/getting-started.md (1)
30-32: Good security framing for LAN-default behavior.The local-only override and warning are clear and reduce accidental exposure risk in real usage.
README.md (1)
127-129: Host default documentation now includes the critical safety context.The LAN-default explanation plus explicit
127.0.0.1override guidance is clear and actionable.Also applies to: 238-238
tests/unit/readme-docs-consistency.test.mjs (1)
14-24: Cross-language README invariant check is a solid guardrail.Good addition ensuring both docs keep the same OpenClaw runtime-config diagram contract.
tests/e2e/run.js (1)
43-46: Verify empty-string home overrides can't bypass tmp-home isolation.Setting these env vars to
''is safe only if path resolvers treat empty string as "unset". If not, some paths may resolve unexpectedly and leak writes outside the intended temp sandbox. The resolver code and path-handling logic must be inspected to confirm that empty strings are normalized (trimmed or checked for length) and fall back to HOME/defaults rather than using''as a literal directory input.
|
To permanently break this loop and enforce the constraint at the configuration level (rather than relying on per-comment instructions), I strongly recommend adding a reviews:
instructions: |
Do not suggest any breaking changes, API/interface/behavior alterations, or refactoring of any kind.
Limit all feedback exclusively to correctness, safety, and quality issues within the existing design.
Do not suggest changes that alter existing APIs, interfaces, or behaviors.This will encode the constraint permanently so it applies automatically to every future review. See CodeRabbit review instructions docs for more details. Triggering a proper full re-review now with this as a hard, non-negotiable rule on every single comment. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
web-ui/session-helpers.mjs (1)
64-69:⚠️ Potential issue | 🟠 MajorGuard synchronous throws when entering the market tab.
Line 68 only suppresses promise rejections; a synchronous throw from
loadSkillsMarketOverview(...)can still escape and break tab switching.🛠️ Non-breaking fix
- void Promise.resolve(this.loadSkillsMarketOverview({ silent: true })).catch(() => {}); + void Promise.resolve() + .then(() => this.loadSkillsMarketOverview({ silent: true })) + .catch(() => {});In JavaScript, does Promise.resolve(fn()) catch synchronous exceptions thrown by fn before Promise.resolve runs? Please cite MDN.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/session-helpers.mjs` around lines 64 - 69, The current call to this.loadSkillsMarketOverview(...) can throw synchronously before Promise.resolve(...) wraps it (so synchronous throws escape); change the invocation so the function is executed inside the promise chain (e.g., use Promise.resolve().then(() => this.loadSkillsMarketOverview({ silent: true })).catch(() => {})) to guarantee both sync throws and async rejections are caught; update the block that computes shouldLoadSkillsMarketOnEnter and the call site that uses loadSkillsMarketOverview to use this pattern.web-ui/app.js (1)
3578-3584:⚠️ Potential issue | 🟠 MajorFail closed when the confirm-disable predicate throws.
Line 3583 currently enables the confirm button if
confirmDialogDisableWhenthrows. That bypasses the guard exactly when its check fails, which is risky for destructive dialogs.🔒 Minimal safety fix
isConfirmDialogDisabled() { if (typeof this.confirmDialogDisableWhen === 'function') { try { return !!this.confirmDialogDisableWhen.call(this); } catch (_) { - return false; + return true; } } return !!this.confirmDialogConfirmDisabled; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/app.js` around lines 3578 - 3584, The current isConfirmDialogDisabled() uses confirmDialogDisableWhen.call(this) but returns false on exceptions, which enables confirmation when the predicate fails; change the catch path to return true so the dialog remains disabled on errors. Locate isConfirmDialogDisabled and the use of confirmDialogDisableWhen, wrap the call in the existing try/catch and replace the catch return value from false to true to "fail closed" (keep the !! coercion for the try path).cli.js (1)
9858-9885:⚠️ Potential issue | 🟠 MajorDrain the upload request on early exits.
handleImportSkillsZipUpload()still returns before the incoming body is consumed in the 405/unsupported-target branches, and thecatchpath also misses the case wherewriteUploadZipStream()rejects before attaching any listeners (for example, an oversizedcontent-length). That leaves unread upload bytes on the socket and can tie up keep-alive connections.🩹 Minimal fix
async function handleImportSkillsZipUpload(req, res, options = {}) { if (req.method !== 'POST') { + req.resume(); writeJsonResponse(res, 405, { error: 'Method Not Allowed' }); return; } try { const forcedTargetApp = normalizeSkillTargetApp(options && options.targetApp ? options.targetApp : ''); const targetApp = forcedTargetApp || resolveSkillTargetAppFromRequest(req, 'codex'); if (!targetApp) { + req.resume(); writeJsonResponse(res, 400, { error: '目标宿主不支持' }); return; } const fileName = resolveUploadFileNameFromRequest(req, `${targetApp}-skills.zip`); const upload = await writeUploadZipStream( @@ }); writeJsonResponse(res, 200, result || {}); } catch (e) { + if (!req.complete && !req.destroyed) { + req.resume(); + } const message = e && e.message ? e.message : '上传失败'; writeJsonResponse(res, 400, { error: message }); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 9858 - 9885, handleImportSkillsZipUpload returns early in the 405 and unsupported-target branches and can leave the request body unread if writeUploadZipStream rejects before attaching listeners; drain the incoming request stream in those early-exit paths and in the catch block before sending the JSON response. Concretely, in handleImportSkillsZipUpload, call req.resume() (and optionally attach a no-op 'error' handler) immediately before each writeJsonResponse that returns early (the 405 branch and the "目标宿主不支持" branch) and also before the writeJsonResponse in the catch block so any in-flight upload bytes are consumed/dropped even when writeUploadZipStream fails.web-ui/modules/skills.methods.mjs (1)
67-87:⚠️ Potential issue | 🟠 Major
openSkillsManager()still lacks busy-state guard.This method can change
skillsTargetAppand callrefreshSkillsList()while import/delete/export work is in flight. UnlikesetSkillsTargetApp()which has a comprehensive busy check (Lines 34-44), this entry point bypasses those guards.A past review flagged this issue but the guard was not added in the visible code.
🛡️ Proposed fix
async openSkillsManager(options = {}) { + if ( + this.skillsLoading + || this.skillsDeleting + || this.skillsScanningImports + || this.skillsImporting + || this.skillsZipImporting + || this.skillsExporting + || this.skillsMarketLoading + ) { + return false; + } let targetApp; try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/modules/skills.methods.mjs` around lines 67 - 87, Add the same busy-state guard used in setSkillsTargetApp() to openSkillsManager(): at the start of openSkillsManager(), check the component's skills busy flag using the same logic as setSkillsTargetApp() and bail out (return false) if an operation is in flight so you don't change skillsTargetApp or call refreshSkillsList() while import/delete/export are running; mirror the exact check and early-return behavior from setSkillsTargetApp() to prevent concurrent state changes when openSkillsManager(), skillsTargetApp, and refreshSkillsList() run.
🧹 Nitpick comments (3)
web-ui/modules/skills.methods.mjs (1)
368-369:exportSelectedSkills()guard is missingskillsScanningImports.Other mutating operations (
importSelectedSkills,importSkillsFromZipFile,deleteSelectedSkills) includeskillsScanningImportsin their guards. Export doesn't, creating an inconsistency. While export is less likely to conflict with scanning, the asymmetry could cause confusion during maintenance.♻️ Proposed fix for consistency
async exportSelectedSkills() { - if (this.skillsDeleting || this.skillsExporting || this.skillsZipImporting || this.skillsImporting) return; + if (this.skillsDeleting || this.skillsScanningImports || this.skillsExporting || this.skillsZipImporting || this.skillsImporting) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/modules/skills.methods.mjs` around lines 368 - 369, The exportSelectedSkills() method's early-return guard is missing the skillsScanningImports flag, causing an inconsistency with other mutating methods; update the guard in exportSelectedSkills() to include skillsScanningImports alongside skillsDeleting, skillsExporting, skillsZipImporting, and skillsImporting so it short-circuits when scanning imports is in progress (mirror the same check used in importSelectedSkills, importSkillsFromZipFile, and deleteSelectedSkills).tests/unit/web-run-host.test.mjs (1)
1-12: BOM character at file start.Line 1 contains a UTF-8 BOM (
\uFEFF). While most modern tooling handles this, it can cause issues with some parsers or when the file is concatenated. Consider removing it for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/web-run-host.test.mjs` around lines 1 - 12, The file tests/unit/web-run-host.test.mjs starts with a UTF-8 BOM (U+FEFF) before the first import (the line beginning with "import assert from 'assert';"), remove that leading BOM character so the file begins directly with the import statement; simply delete the invisible character at the top of the file (or re-save without BOM) to eliminate the \uFEFF and prevent parser issues.tests/unit/config-tabs-ui.test.mjs (1)
53-59: Target switch button regex is fragile but adequate for contract testing.The regex captures buttons with specific
:classand@clickpatterns. The assertion for exactly 4 buttons (2 targets × 2 layouts: standard and compact switch groups) is a reasonable contract check. However, minor HTML whitespace or attribute reordering could break this.Consider extracting the button count via a more lenient pattern if this becomes flaky:
const chipButtons = html.match(/class="\['market-target-chip'/g) || []; assert.strictEqual(chipButtons.length, 4);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/config-tabs-ui.test.mjs` around lines 53 - 59, The current test's targetSwitchButtons regex in tests/unit/config-tabs-ui.test.mjs is fragile (variable targetSwitchButtons) because it depends on attribute order/whitespace; replace it with a more lenient approach that simply counts elements by the unique class token "market-target-chip" (i.e., match occurrences of that class string in html and assert the count equals 4) and keep the existing loop that asserts :disabled="skillsMarketBusy" if still needed; update references to targetSwitchButtons to use the new chip-count result and adjust the assertion accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/index.html`:
- Around line 1297-1303: The new market action buttons (those calling
loadSkillsMarketOverview and openSkillsManager and using skillsMarketBusy) lack
the existing init-state guard; update their :disabled bindings to also disable
when the global init flags are active (e.g., loading || initError) so they
mirror the config-side Skills entry guard; locate template buttons that call
loadSkillsMarketOverview, openSkillsManager, scan/refresh/ZIP/modal skill
actions and change :disabled="skillsMarketBusy" to :disabled="skillsMarketBusy
|| loading || initError" (or the equivalent init-state flag names used
elsewhere) to prevent clicks during failed/initializing state.
- Around line 1419-1421: The UI copy implies an import completes, but the button
only starts scanning via scanImportableSkills({ silent: false }); update the
text for the elements with class names market-action-title and
market-action-copy so the title and description reflect that this triggers a
scan (e.g., “扫描可导入项” / “扫描其他应用目录以查找可导入技能” or similar) to match the handler
scanImportableSkills rather than implying an immediate import.
---
Duplicate comments:
In `@cli.js`:
- Around line 9858-9885: handleImportSkillsZipUpload returns early in the 405
and unsupported-target branches and can leave the request body unread if
writeUploadZipStream rejects before attaching listeners; drain the incoming
request stream in those early-exit paths and in the catch block before sending
the JSON response. Concretely, in handleImportSkillsZipUpload, call req.resume()
(and optionally attach a no-op 'error' handler) immediately before each
writeJsonResponse that returns early (the 405 branch and the "目标宿主不支持" branch)
and also before the writeJsonResponse in the catch block so any in-flight upload
bytes are consumed/dropped even when writeUploadZipStream fails.
In `@web-ui/app.js`:
- Around line 3578-3584: The current isConfirmDialogDisabled() uses
confirmDialogDisableWhen.call(this) but returns false on exceptions, which
enables confirmation when the predicate fails; change the catch path to return
true so the dialog remains disabled on errors. Locate isConfirmDialogDisabled
and the use of confirmDialogDisableWhen, wrap the call in the existing try/catch
and replace the catch return value from false to true to "fail closed" (keep the
!! coercion for the try path).
In `@web-ui/modules/skills.methods.mjs`:
- Around line 67-87: Add the same busy-state guard used in setSkillsTargetApp()
to openSkillsManager(): at the start of openSkillsManager(), check the
component's skills busy flag using the same logic as setSkillsTargetApp() and
bail out (return false) if an operation is in flight so you don't change
skillsTargetApp or call refreshSkillsList() while import/delete/export are
running; mirror the exact check and early-return behavior from
setSkillsTargetApp() to prevent concurrent state changes when
openSkillsManager(), skillsTargetApp, and refreshSkillsList() run.
In `@web-ui/session-helpers.mjs`:
- Around line 64-69: The current call to this.loadSkillsMarketOverview(...) can
throw synchronously before Promise.resolve(...) wraps it (so synchronous throws
escape); change the invocation so the function is executed inside the promise
chain (e.g., use Promise.resolve().then(() => this.loadSkillsMarketOverview({
silent: true })).catch(() => {})) to guarantee both sync throws and async
rejections are caught; update the block that computes
shouldLoadSkillsMarketOnEnter and the call site that uses
loadSkillsMarketOverview to use this pattern.
---
Nitpick comments:
In `@tests/unit/config-tabs-ui.test.mjs`:
- Around line 53-59: The current test's targetSwitchButtons regex in
tests/unit/config-tabs-ui.test.mjs is fragile (variable targetSwitchButtons)
because it depends on attribute order/whitespace; replace it with a more lenient
approach that simply counts elements by the unique class token
"market-target-chip" (i.e., match occurrences of that class string in html and
assert the count equals 4) and keep the existing loop that asserts
:disabled="skillsMarketBusy" if still needed; update references to
targetSwitchButtons to use the new chip-count result and adjust the assertion
accordingly.
In `@tests/unit/web-run-host.test.mjs`:
- Around line 1-12: The file tests/unit/web-run-host.test.mjs starts with a
UTF-8 BOM (U+FEFF) before the first import (the line beginning with "import
assert from 'assert';"), remove that leading BOM character so the file begins
directly with the import statement; simply delete the invisible character at the
top of the file (or re-save without BOM) to eliminate the \uFEFF and prevent
parser issues.
In `@web-ui/modules/skills.methods.mjs`:
- Around line 368-369: The exportSelectedSkills() method's early-return guard is
missing the skillsScanningImports flag, causing an inconsistency with other
mutating methods; update the guard in exportSelectedSkills() to include
skillsScanningImports alongside skillsDeleting, skillsExporting,
skillsZipImporting, and skillsImporting so it short-circuits when scanning
imports is in progress (mirror the same check used in importSelectedSkills,
importSkillsFromZipFile, and deleteSelectedSkills).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c68db2cd-5754-4418-8240-19dfa4e53507
📒 Files selected for processing (19)
README.en.mdREADME.mdcli.jssite/guide/getting-started.mdtests/e2e/run.jstests/unit/config-tabs-ui.test.mjstests/unit/readme-docs-consistency.test.mjstests/unit/run.mjstests/unit/session-tab-switch-performance.test.mjstests/unit/skills-market-runtime.test.mjstests/unit/skills-modal-ui.test.mjstests/unit/web-run-host.test.mjsweb-ui/app.jsweb-ui/index.htmlweb-ui/modules/config-mode.computed.mjsweb-ui/modules/skills.computed.mjsweb-ui/modules/skills.methods.mjsweb-ui/session-helpers.mjsweb-ui/styles.css
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
🔇 Additional comments (25)
web-ui/modules/config-mode.computed.mjs (1)
52-52: Market tab label mapping is correct.This explicitly covers
marketand avoids falling back to未知.web-ui/modules/skills.computed.mjs (1)
3-8: Computed additions look safe and consistent.The new target helpers, busy-state aggregation, and capped preview lists are all bounded and null-safe.
Also applies to: 87-105
tests/e2e/run.js (1)
43-46: Good E2E environment isolation.Clearing these home/config env vars helps prevent accidental dependency on developer machine state.
site/guide/getting-started.md (1)
30-32: Docs update is clear and safety-conscious.The default LAN bind note plus localhost fallback guidance is explicit and actionable.
tests/unit/run.mjs (1)
16-17: Unit test registration updates look correct.New suites are properly imported into the existing runner pipeline.
Also applies to: 30-30
tests/unit/session-tab-switch-performance.test.mjs (1)
130-200: Market tab behavior tests are well-targeted.Nice coverage for first-enter load, rejection handling, and no-op reload on repeated market tab selection.
tests/unit/readme-docs-consistency.test.mjs (1)
14-24: Cross-README consistency check is a good guardrail.Validating the same OpenClaw runtime-config invariant in both language docs helps prevent drift.
tests/unit/web-run-host.test.mjs (6)
14-90: Robust brace-matching implementation.The
findMatchingBraceparser correctly handles strings, template literals with embedded expressions, line/block comments, and nested braces. This is essential for reliably extracting function bodies fromcli.js.
126-147: Host resolution tests are well-structured.Tests verify the precedence chain (CLI host → environment → default) and the new
0.0.0.0default for LAN access. The use ofinstantiateFunctionwith controlled bindings allows isolated testing of the extracted logic.
167-245: Skills directory resolution tests correctly verify environment variable precedence.Tests confirm
CODEX_HOME/CLAUDE_CONFIG_DIRtake priority overXDG_CONFIG_HOME, and that resolvers produce concrete/skillspaths rather than parent config directories. The mockedresolveExistingDircaptures arguments for verification.
328-348: Target resolution tests cover fallback and precedence behavior.Tests verify: (1) omitted target falls back to default, (2) explicit unsupported targets return
nullinstead of silently falling back, (3)targetApptakes precedence overtarget. This aligns with the backend contract.
538-596: Import safety test correctly validates nested-path rejection.The test verifies that
importSkillsrejects when the resolved target path is nested inside the source skill directory, returning the expected Chinese error message and making zero copy calls. TheisPathInsidemock precisely gates the rejection.
670-720: Cleanup verification ensures tempDir is always removed.The test confirms
importSkillsFromZipFilecallsrmSyncwith{ recursive: true, force: true }even when the target app is unsupported, preventing orphaned temp directories.tests/unit/config-tabs-ui.test.mjs (3)
37-74: Market tab DOM and accessibility assertions are thorough.The tests correctly verify:
- Tab/panel wiring (
id,aria-controls,v-show)- Target switch buttons with
:disabled="skillsMarketBusy"gating- Accessibility semantics (
role="group"with appropriatearia-label, notrole="tablist")- Absence of removed online/remote market elements
The negative assertions (
doesNotMatch) effectively guard against regression of the removed MCP registry UI.
188-196: State property assertions validate local-only market metadata.The positive assertions for
skillsTargetApp,skillsMarketLoading, andskillsMarketLocalLoadedOnce/skillsMarketImportLoadedOncecombined with negative assertions for remote/ecosystem fields confirm the PR's removal of online market functionality.
246-248: Deferred market refresh test validates promise-safe error handling.The assertion for
void Promise.resolve(...).catch(() => {})confirms the fire-and-forget pattern that prevents unhandled rejections when the market overview load fails during tab entry.web-ui/modules/skills.methods.mjs (4)
33-65:setSkillsTargetApp()busy-state guard is comprehensive.The guard correctly blocks target switching during all mutating operations: loading, deleting, scanning, importing (both methods), exporting, and market loading. This prevents race conditions where handlers capture the old target but toasts/refreshes use the new one.
146-166:loadSkillsMarketOverview()correctly tracks one-time completion per slice.The method:
- Guards against re-entry with
skillsMarketLoading- Conditionally refreshes based on
forceRefreshor*LoadedOnceflags- Updates flags only on successful operations
- Returns combined success status
414-419: Delete operation provides clear user feedback when scan is running.Rather than silently returning,
deleteSelectedSkills()shows an error message whenskillsScanningImportsis true. This is better UX than the silent guards elsewhere.
427-434: Confirm dialogconfirmDisabledcallback enables live gating.The
confirmDisabled: () => this.skillsDeleting || this.skillsScanningImportspattern allows the confirm button to disable dynamically if a scan starts while the dialog is open. This prevents the user from confirming a delete during a concurrent scan.tests/unit/skills-market-runtime.test.mjs (5)
11-54: Well-designed VM builder for isolated unit testing.The
buildVmhelper:
- Injects a controllable
apiimplementation- Provides sensible defaults for all state properties
- Wires computed properties from
skills.computed.mjsviaObject.defineProperty- Allows targeted overrides for specific test scenarios
This pattern enables testing business logic without the full Vue runtime.
105-121: Busy-state blocking test validates guard behavior.This test confirms
setSkillsTargetAppreturnsfalseand doesn't callloadSkillsMarketOverviewwhenskillsImportingis true. Combined with the implementation's comprehensive guard (checking all busy flags), this provides confidence in race-condition prevention.
179-207: Same-target reopen test validates selective state reset.The test confirms that reopening the modal for the same target:
- Preserves
skillsImportListand*LoadedOnceflags (avoiding redundant API calls)- Clears user selections (
skillsSelectedNames,skillsImportSelectedKeys)- Resets filters (
skillsKeyword,skillsStatusFilter)This matches the implementation's conditional
resetSkillsTargetState()call.
338-355: Confirm dialog disabled state test verifies live callback evaluation.The test captures the
confirmDisabledfunction passed torequestConfirmDialog, then verifies:
- It returns
falsewhenskillsScanningImportsisfalse- It returns
trueafter settingskillsScanningImports = trueThis confirms the callback evaluates against live VM state rather than a captured snapshot.
263-287: Deletion busy-state guard test is thorough.The test verifies that
importSelectedSkills,importSkillsFromZipFile, andexportSelectedSkillsall:
- Make zero API/upload calls when
skillsDeletingis true- Produce no error messages (silent early return)
This validates the guard additions from past review feedback.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
web-ui/app.js (1)
3578-3584:⚠️ Potential issue | 🟠 MajorFail closed if the disable predicate throws.
An exception currently returns
false, which enables the confirm button and can let destructive actions proceed without the intended guard. Keep the dialog disabled on evaluation errors instead.🔒 Minimal safety fix
isConfirmDialogDisabled() { if (typeof this.confirmDialogDisableWhen === 'function') { try { return !!this.confirmDialogDisableWhen.call(this); } catch (_) { - return false; + return true; } } return !!this.confirmDialogConfirmDisabled; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/app.js` around lines 3578 - 3584, In isConfirmDialogDisabled(), the current catch block returns false on exceptions which enables the confirm button; change the error handling so that any exception from confirmDialogDisableWhen (or a non-function) results in returning true (keep the dialog disabled) — i.e., after detecting typeof this.confirmDialogDisableWhen === 'function' call it inside try/catch and on any catch return true to fail closed, ensuring confirmDialogDisableWhen and isConfirmDialogDisabled are updated accordingly.web-ui/index.html (2)
1298-1303:⚠️ Potential issue | 🟡 MinorMirror the existing init-state guard on market controls.
This panel stays mounted under the global
loading/initErrorbanner because it usesv-show, but these controls only bindskillsMarketBusy. That leaves refresh, target-switch, scan, and ZIP actions clickable during a failed or incomplete init.🛡️ Minimal guard fix
- <button class="btn-tool btn-tool-compact" `@click`="loadSkillsMarketOverview({ forceRefresh: true, silent: false })" :disabled="skillsMarketBusy"> + <button class="btn-tool btn-tool-compact" `@click`="loadSkillsMarketOverview({ forceRefresh: true, silent: false })" :disabled="loading || !!initError || skillsMarketBusy"> - :disabled="skillsMarketBusy" + :disabled="loading || !!initError || skillsMarketBusy"Apply the same guard to the market target chips, local refresh, scan, and action-card buttons in this panel.
Also applies to: 1307-1323, 1363-1365, 1388-1390, 1415-1425
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/index.html` around lines 1298 - 1303, The market controls (e.g., the buttons bound to loadSkillsMarketOverview and openSkillsManager, the market target chips, local refresh/scan buttons, and action-card buttons) currently only check skillsMarketBusy and remain interactive during global init/loading errors; update these controls to mirror the global init-state guard by also checking the global init flags (the same guard used for the top-level loading/initError banner) — e.g., add the same condition used by the banner to each control's :disabled or v-if/v-show logic so they are disabled/hidden when global loading or initError is true (apply this change to the refresh button, target-switch chips, scan, ZIP actions, and the other control blocks referenced around the lines noted).
1419-1421:⚠️ Potential issue | 🟡 MinorThe “跨应用导入” card still describes a scan as an import.
This handler only runs
scanImportableSkills({ silent: false }); the actual import still happens later in the manager/modal flow. The current title and copy overstate what the click does.✏️ Minimal copy fix
- <span class="market-action-title">跨应用导入</span> - <span class="market-action-copy">扫描其他应用目录并导入到当前 {{ skillsTargetLabel }}</span> + <span class="market-action-title">扫描导入来源</span> + <span class="market-action-copy">扫描其他应用目录，随后在 Skills 管理中导入到当前 {{ skillsTargetLabel }}</span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/index.html` around lines 1419 - 1421, The button currently claims to perform an import but only invokes scanImportableSkills({ silent: false }); update the UI copy for the element with class "market-action-card" (the button that calls scanImportableSkills) so the title and secondary text accurately reflect that it only scans; e.g., change the title from "跨应用导入" to something like "扫描可导入技能" and the description from "扫描其他应用目录并导入到当前 {{ skillsTargetLabel }}" to indicate it will "扫描其他应用目录以查找可导入项目，导入将在管理/模态流程中完成" (or equivalent wording in Chinese), ensuring the click behavior and text are consistent with the scan-only action.web-ui/modules/skills.methods.mjs (2)
67-87:⚠️ Potential issue | 🟠 MajorGuard
openSkillsManager()the same way assetSkillsTargetApp().
openSkillsManager()still changesskillsTargetAppand startsrefreshSkillsList()without the busy-state check already used insetSkillsTargetApp(). A reopen or target switch during import/export/delete/ZIP work can still make the late toast and follow-up refresh land on the wrong target.Based on learnings: the maintainer asked review feedback to stay within correctness, safety, and quality without suggesting breaking API/interface/behavior changes.🛡️ Minimal guard
async openSkillsManager(options = {}) { + if ( + this.skillsLoading + || this.skillsDeleting + || this.skillsScanningImports + || this.skillsImporting + || this.skillsZipImporting + || this.skillsExporting + || this.skillsMarketLoading + ) { + return false; + } let targetApp; try { targetApp = this.normalizeSkillsTargetApp(options && options.targetApp ? options.targetApp : this.skillsTargetApp);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/modules/skills.methods.mjs` around lines 67 - 87, openSkillsManager currently changes skillsTargetApp and calls refreshSkillsList without checking the component's busy state; update openSkillsManager to perform the same busy guard used in setSkillsTargetApp (check the same busy flag/condition used there, e.g. an isBusy or similar property/method) and if busy, abort (return false or show the same toast) to prevent target switches/refreshes during import/export/delete/zip operations; keep all other behavior (normalizing target via normalizeSkillsTargetApp, resetting state, showing modal, awaiting refreshSkillsList) unchanged when not busy.
242-242:⚠️ Potential issue | 🟠 MajorMutations can still race with
refreshSkillsList().These guards still omit
skillsLoading. Lines 2290-2305 inweb-ui/index.htmlleave the ZIP/import/delete actions enabled during a list refresh, so this race is user-reachable: an olderlist-skillsresponse can land after the mutation's own refresh and overwrite the updated state. For delete,confirmDisabledshould mirror the same busy state.Based on learnings: the maintainer asked review feedback to stay within correctness, safety, and quality without suggesting breaking API/interface/behavior changes.🛡️ Minimal guard update
- if (this.skillsDeleting || this.skillsScanningImports || this.skillsImporting || this.skillsZipImporting || this.skillsExporting) return; + if (this.skillsLoading || this.skillsDeleting || this.skillsScanningImports || this.skillsImporting || this.skillsZipImporting || this.skillsExporting) return; - if (this.skillsDeleting || this.skillsScanningImports || this.skillsZipImporting || this.skillsImporting || this.skillsExporting) return; + if (this.skillsLoading || this.skillsDeleting || this.skillsScanningImports || this.skillsZipImporting || this.skillsImporting || this.skillsExporting) return; - if (this.skillsDeleting || this.skillsZipImporting || this.skillsExporting || this.skillsImporting) return; + if (this.skillsLoading || this.skillsDeleting || this.skillsZipImporting || this.skillsExporting || this.skillsImporting) return; ... - confirmDisabled: () => this.skillsDeleting || this.skillsScanningImports, + confirmDisabled: () => this.skillsLoading || this.skillsDeleting || this.skillsScanningImports,Also applies to: 334-334, 415-418, 432-432
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/modules/skills.methods.mjs` at line 242, The guard that blocks mutations (currently checking skillsDeleting, skillsScanningImports, skillsImporting, skillsZipImporting, skillsExporting) misses skillsLoading and thus allows races with refreshSkillsList; update all affected guards (including the one used for confirmDisabled) to also include skillsLoading so mutation actions and confirmDisabled are disabled while a list refresh is in progress, and ensure any helper that composes these booleans (e.g., the UI check used at lines referenced) is updated consistently.cli.js (1)
9858-9885:⚠️ Potential issue | 🟠 MajorDrain rejected upload requests before responding.
Line 9859 and Line 9867 return without consuming the request body, and Line 9883 has the same problem when
writeUploadZipStream()rejects on its header-size precheck before attaching anyreqlisteners. On Node's HTTP server, that can leave unread upload data on the socket and tie up keep-alive connections under repeated bad requests.🛠️ Minimal fix
async function handleImportSkillsZipUpload(req, res, options = {}) { if (req.method !== 'POST') { + req.resume(); writeJsonResponse(res, 405, { error: 'Method Not Allowed' }); return; } try { const forcedTargetApp = normalizeSkillTargetApp(options && options.targetApp ? options.targetApp : ''); const targetApp = forcedTargetApp || resolveSkillTargetAppFromRequest(req, 'codex'); if (!targetApp) { + req.resume(); writeJsonResponse(res, 400, { error: '目标宿主不支持' }); return; } const fileName = resolveUploadFileNameFromRequest(req, `${targetApp}-skills.zip`); const upload = await writeUploadZipStream( @@ }); writeJsonResponse(res, 200, result || {}); } catch (e) { + if (!req.complete && !req.destroyed) { + req.resume(); + } const message = e && e.message ? e.message : '上传失败'; writeJsonResponse(res, 400, { error: message }); } }Based on learnings: the maintainer wants correctness/safety fixes that preserve current behavior, so this keeps the existing upload contract intact.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 9858 - 9885, The handler handleImportSkillsZipUpload can return or throw before the incoming request body is consumed, which can leak sockets; before every early return (when method !== 'POST' or no targetApp) and in the catch path where writeUploadZipStream may reject before attaching listeners, ensure the request body is drained (e.g., call a small helper like drainRequestBody(req) or at minimum req.resume() and attach a noop 'data'/'end' listener) prior to calling writeJsonResponse or rethrowing; update references inside handleImportSkillsZipUpload and its error-handling around writeUploadZipStream, resolveUploadFileNameFromRequest, resolveSkillTargetAppFromRequest and writeJsonResponse to drain the request first so behavior is preserved but sockets are not left tied up.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/styles.css`:
- Around line 3699-3709: The .market-target-chip CSS forces cursor: pointer and
lacks a disabled style so chips bound with :disabled="skillsMarketBusy" still
look clickable; add a disabled rule targeting both the disabled pseudo-class and
attribute (e.g., .market-target-chip:disabled, .market-target-chip[disabled])
that removes pointer interactivity (cursor: default or not-allowed and
pointer-events: none) and adjusts visual state (muted color, reduced opacity or
different background/border/box-shadow) to match other disabled action cards.
Ensure you update the same stylesheet selectors so the Vue binding
:disabled="skillsMarketBusy" visibly disables the chip.
---
Duplicate comments:
In `@cli.js`:
- Around line 9858-9885: The handler handleImportSkillsZipUpload can return or
throw before the incoming request body is consumed, which can leak sockets;
before every early return (when method !== 'POST' or no targetApp) and in the
catch path where writeUploadZipStream may reject before attaching listeners,
ensure the request body is drained (e.g., call a small helper like
drainRequestBody(req) or at minimum req.resume() and attach a noop 'data'/'end'
listener) prior to calling writeJsonResponse or rethrowing; update references
inside handleImportSkillsZipUpload and its error-handling around
writeUploadZipStream, resolveUploadFileNameFromRequest,
resolveSkillTargetAppFromRequest and writeJsonResponse to drain the request
first so behavior is preserved but sockets are not left tied up.
In `@web-ui/app.js`:
- Around line 3578-3584: In isConfirmDialogDisabled(), the current catch block
returns false on exceptions which enables the confirm button; change the error
handling so that any exception from confirmDialogDisableWhen (or a non-function)
results in returning true (keep the dialog disabled) — i.e., after detecting
typeof this.confirmDialogDisableWhen === 'function' call it inside try/catch and
on any catch return true to fail closed, ensuring confirmDialogDisableWhen and
isConfirmDialogDisabled are updated accordingly.
In `@web-ui/index.html`:
- Around line 1298-1303: The market controls (e.g., the buttons bound to
loadSkillsMarketOverview and openSkillsManager, the market target chips, local
refresh/scan buttons, and action-card buttons) currently only check
skillsMarketBusy and remain interactive during global init/loading errors;
update these controls to mirror the global init-state guard by also checking the
global init flags (the same guard used for the top-level loading/initError
banner) — e.g., add the same condition used by the banner to each control's
:disabled or v-if/v-show logic so they are disabled/hidden when global loading
or initError is true (apply this change to the refresh button, target-switch
chips, scan, ZIP actions, and the other control blocks referenced around the
lines noted).
- Around line 1419-1421: The button currently claims to perform an import but
only invokes scanImportableSkills({ silent: false }); update the UI copy for the
element with class "market-action-card" (the button that calls
scanImportableSkills) so the title and secondary text accurately reflect that it
only scans; e.g., change the title from "跨应用导入" to something like "扫描可导入技能" and
the description from "扫描其他应用目录并导入到当前 {{ skillsTargetLabel }}" to indicate it
will "扫描其他应用目录以查找可导入项目，导入将在管理/模态流程中完成" (or equivalent wording in Chinese),
ensuring the click behavior and text are consistent with the scan-only action.
In `@web-ui/modules/skills.methods.mjs`:
- Around line 67-87: openSkillsManager currently changes skillsTargetApp and
calls refreshSkillsList without checking the component's busy state; update
openSkillsManager to perform the same busy guard used in setSkillsTargetApp
(check the same busy flag/condition used there, e.g. an isBusy or similar
property/method) and if busy, abort (return false or show the same toast) to
prevent target switches/refreshes during import/export/delete/zip operations;
keep all other behavior (normalizing target via normalizeSkillsTargetApp,
resetting state, showing modal, awaiting refreshSkillsList) unchanged when not
busy.
- Line 242: The guard that blocks mutations (currently checking skillsDeleting,
skillsScanningImports, skillsImporting, skillsZipImporting, skillsExporting)
misses skillsLoading and thus allows races with refreshSkillsList; update all
affected guards (including the one used for confirmDisabled) to also include
skillsLoading so mutation actions and confirmDisabled are disabled while a list
refresh is in progress, and ensure any helper that composes these booleans
(e.g., the UI check used at lines referenced) is updated consistently.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 96ecc0dd-3a69-4676-b6ea-9598fc8a95dd
📒 Files selected for processing (19)
README.en.mdREADME.mdcli.jssite/guide/getting-started.mdtests/e2e/run.jstests/unit/config-tabs-ui.test.mjstests/unit/readme-docs-consistency.test.mjstests/unit/run.mjstests/unit/session-tab-switch-performance.test.mjstests/unit/skills-market-runtime.test.mjstests/unit/skills-modal-ui.test.mjstests/unit/web-run-host.test.mjsweb-ui/app.jsweb-ui/index.htmlweb-ui/modules/config-mode.computed.mjsweb-ui/modules/skills.computed.mjsweb-ui/modules/skills.methods.mjsweb-ui/session-helpers.mjsweb-ui/styles.css
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
🪛 LanguageTool
README.en.md
[grammar] ~57-~57: Ensure spelling is correct
Context: ...level delete (supports batch) Skills Market - Switch the skills install target b...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~58-~58: Ensure spelling is correct
Context: ...install target between Codex and Claude Code - Inspect local installed skills, root...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~59-~59: Ensure spelling is correct
Context: ...local installed skills, root paths, and status - Scan importable sources from Codex...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~61-~61: Ensure spelling is correct
Context: ...pp import, ZIP import/export, and batch delete Engineering Utilities - MCP stdio dom...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (14)
tests/e2e/run.js (1)
43-46: Good E2E isolation for skills/config path resolution.Explicitly blanking these env vars keeps tests deterministic and prevents machine-specific config from leaking into path selection logic.
web-ui/session-helpers.mjs (1)
64-69: Synchronous throws fromloadSkillsMarketOverviewcan escape the catch handler.Per MDN,
Promise.resolve(fn())evaluatesfn()synchronously before wrapping. IfloadSkillsMarketOverviewthrows synchronously, the exception propagates immediately and interrupts tab switching. The.catch(() => {})only handles async rejections.🛠️ Recommended fix
- void Promise.resolve(this.loadSkillsMarketOverview({ silent: true })).catch(() => {}); + void Promise.resolve() + .then(() => this.loadSkillsMarketOverview({ silent: true })) + .catch(() => {});web-ui/modules/config-mode.computed.mjs (1)
52-52: LGTM!The new
'market'tab label mapping follows the established pattern and correctly integrates with the Skills Market tab addition.tests/unit/run.mjs (1)
16-17: LGTM!The new test module imports follow the existing pattern and correctly register the Skills Market, README consistency, and web host tests with the test runner.
Also applies to: 30-30
site/guide/getting-started.md (1)
30-32: LGTM!The updated documentation correctly reflects the new default host binding (
0.0.0.0:3737) and includes appropriate security guidance warning users about LAN exposure of the unauthenticated management interface.tests/unit/session-tab-switch-performance.test.mjs (1)
130-200: LGTM!The three new test cases thoroughly cover the
markettab entry flow:
- Verifies
loadSkillsMarketOverview({ silent: true })is called when entering from another tab- Confirms async rejections are swallowed without causing
unhandledRejection- Validates idempotency when already on the
markettabThe tests correctly mirror the implementation's guard condition (
previousTab !== 'market').tests/unit/readme-docs-consistency.test.mjs (1)
1-25: LGTM!The test effectively guards against README documentation drift between English and Chinese versions by asserting both files contain the identical OpenClaw runtime config file pattern.
tests/unit/skills-modal-ui.test.mjs (1)
35-82: LGTM!The updated assertions comprehensively validate:
- Confirm dialog disabled state management (
confirmDialogConfirmDisabled,confirmDialogDisableWhen,isConfirmDialogDisabled())- New skills computed properties (
skillsTargetLabel,skillsDefaultRootPath) and removal of remote market (skillsMarketRemoteCount)- Target-aware API routing (
list-skills,scan-unmanaged-skills, etc.) and removal of online market API- UI elements for local-only market (
.market-target-chip) and removal of online componentsThe
doesNotMatchassertions effectively guard against regression to the removed MCP registry market UI.web-ui/app.js (2)
147-148: Good per-dialog state wiring.The disable state is now declared reactively, initialized on open, and cleared on resolve, which prevents one confirm dialog's disabled state from leaking into the next.
Also applies to: 3559-3572, 3598-3599
175-175: The concern in this review comment does not align with the actual implementation. TheresetSkillsTargetState()method explicitly resets bothskillsMarketLocalLoadedOnceandskillsMarketImportLoadedOnceflags tofalsewhenever the target changes (lines 29–30 in skills.methods.mjs). BothsetSkillsTargetApp()(line 56) andopenSkillsManager()(line 78) invoke this reset immediately upon detecting a target change, ensuring that subsequent calls toloadSkillsMarketOverview()will always perform fresh loads for the new target rather than reusing the previous target's cached data.> Likely an incorrect or invalid review comment.web-ui/modules/skills.computed.mjs (2)
3-8: Clear target mapping.The label/root-path getters stay explicit for the two supported apps and provide a safe Codex default when the target is unset.
87-104: Busy and preview computed state look solid.Including
skillsMarketLoadingin the aggregate busy flag and slicing from guarded array fallbacks keeps the market UI stable before data has loaded.README.en.md (2)
26-29: The Skills Market docs are well synchronized.The new capability is described consistently across the overview, feature list, architecture block, and Web UI section, so readers get the same mental model everywhere.
Also applies to: 38-38, 57-61, 85-94, 201-205
127-129: Good safety callout for the new LAN default.Documenting the
0.0.0.0default together with the127.0.0.1override makes the exposure trade-off explicit instead of leaving it implicit.Also applies to: 237-237
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
web-ui/session-helpers.mjs (1)
68-68:⚠️ Potential issue | 🟠 MajorGuard synchronous throws in the fire-and-forget market preload.
Line 68 can still throw synchronously before promise wrapping, which may interrupt tab switching.
🛠️ Minimal fix
- void Promise.resolve(this.loadSkillsMarketOverview({ silent: true })).catch(() => {}); + void Promise.resolve() + .then(() => this.loadSkillsMarketOverview({ silent: true })) + .catch(() => {});In JavaScript, does Promise.resolve(fn()) catch synchronous exceptions thrown by fn before Promise.resolve is called?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/session-helpers.mjs` at line 68, The fire-and-forget call to loadSkillsMarketOverview (void Promise.resolve(this.loadSkillsMarketOverview({ silent: true })).catch(() => {})) can still throw synchronously before Promise.resolve is invoked; wrap the call so synchronous exceptions are caught — e.g., capture the call in a try/catch and then Promise.resolve the returned promise (or use Promise.resolve().then(() => this.loadSkillsMarketOverview(...))). Update the invocation that references loadSkillsMarketOverview to ensure both sync throws and async rejections are swallowed.web-ui/styles.css (1)
3699-3709:⚠️ Potential issue | 🟡 MinorAdd an explicit disabled style for target chips.
When chips are disabled by busy state, they still present as clickable because
cursor: pointerremains and no disabled visual override exists.🎯 Minimal CSS patch
.market-target-chip { border: 1px solid rgba(160, 145, 130, 0.28); border-radius: 999px; background: rgba(255, 255, 255, 0.92); color: var(--color-text-secondary); padding: 8px 14px; font-size: var(--font-size-caption); font-weight: var(--font-weight-secondary); cursor: pointer; transition: border-color var(--transition-fast) var(--ease-smooth), background var(--transition-fast) var(--ease-smooth), color var(--transition-fast) var(--ease-smooth), box-shadow var(--transition-fast) var(--ease-smooth); } + +.market-target-chip:disabled, +.market-target-chip[disabled] { + cursor: not-allowed; + opacity: 0.64; + pointer-events: none; +}#!/bin/bash # Verify chip selectors and disabled usage are present/absent as expected. rg -n "market-target-chip" web-ui/styles.css rg -n ":disabled=\"skillsMarketBusy\"|market-target-chip" --iglob "*.html" --iglob "*.js" --iglob "*.mjs"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles.css` around lines 3699 - 3709, The .market-target-chip currently keeps cursor: pointer and no visual disabled state; add explicit disabled styles targeting .market-target-chip[disabled], .market-target-chip.disabled and .market-target-chip[aria-disabled="true"] to override interactivity (set cursor: default or not-allowed, pointer-events: none), and provide a visual override (reduced opacity or muted background/border and remove box-shadow/highlight) while preserving transitions; update styles for color, background and border so busy/disabled chips look non-clickable and consistent with existing variables.cli.js (1)
9858-9885:⚠️ Potential issue | 🟠 MajorDrain or close the upload stream on every pre-processing error path.
The
405/ unsupported-target returns here, plus theContent-Lengthprecheck inwriteUploadZipStream()(Lines 8593-8597), can all fail before a body consumer is guaranteed to be attached. Sending JSON without draining the request leaves oversized or malformed uploads holding the socket open until timeout.🩹 Minimal fix
async function handleImportSkillsZipUpload(req, res, options = {}) { + const failRequest = (statusCode, payload) => { + if (!req.complete && !req.destroyed) { + req.resume(); + res.setHeader('Connection', 'close'); + } + writeJsonResponse(res, statusCode, payload); + }; + if (req.method !== 'POST') { - writeJsonResponse(res, 405, { error: 'Method Not Allowed' }); + failRequest(405, { error: 'Method Not Allowed' }); return; } try { const forcedTargetApp = normalizeSkillTargetApp(options && options.targetApp ? options.targetApp : ''); const targetApp = forcedTargetApp || resolveSkillTargetAppFromRequest(req, 'codex'); if (!targetApp) { - writeJsonResponse(res, 400, { error: '目标宿主不支持' }); + failRequest(400, { error: '目标宿主不支持' }); return; } const fileName = resolveUploadFileNameFromRequest(req, `${targetApp}-skills.zip`); const upload = await writeUploadZipStream( req, @@ writeJsonResponse(res, 200, result || {}); } catch (e) { const message = e && e.message ? e.message : '上传失败'; - writeJsonResponse(res, 400, { error: message }); + failRequest(400, { error: message }); } }Run this to verify the preflight rejection path and the handler’s current error exits together:
#!/bin/bash sed -n '8591,8598p' cli.js printf '\n----\n' sed -n '9858,9887p' cli.js🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 9858 - 9885, The handler handleImportSkillsZipUpload currently returns JSON on early error paths (non-POST, missing targetApp) and in the catch without consuming or closing the incoming request body, which leaves upload sockets open; add a small drain helper (e.g., drainRequest(req) that calls req.resume() and removes/ignores further data) or call req.resume() directly before every early return and before writeJsonResponse in the catch; update the non-POST branch, the missing targetApp branch, and the catch block to call this drain helper (or req.resume()) prior to calling writeJsonResponse so writeUploadZipStream and content-length prechecks cannot leave the socket hanging.web-ui/app.js (1)
3578-3584:⚠️ Potential issue | 🟠 MajorUse fail-closed behavior when disable predicate throws.
At Line 3583, exceptions currently return
false(enabled), which can bypass destructive-action guarding. This should default to disabled on evaluation errors.🔒 Minimal safety fix
isConfirmDialogDisabled() { if (typeof this.confirmDialogDisableWhen === 'function') { try { return !!this.confirmDialogDisableWhen.call(this); } catch (_) { - return false; + return true; } } return !!this.confirmDialogConfirmDisabled; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/app.js` around lines 3578 - 3584, The isConfirmDialogDisabled() method currently returns false when confirmDialogDisableWhen throws, which leaves the dialog enabled on errors; update the catch block in isConfirmDialogDisabled to return true (fail-closed) so that any exception from the confirmDialogDisableWhen predicate results in the dialog being disabled; locate the isConfirmDialogDisabled method and the confirmDialogDisableWhen reference to make this change (optionally add a process or console log of the caught error while ensuring the return value is true).web-ui/index.html (1)
1419-1421:⚠️ Potential issue | 🟡 MinorKeep this action card copy aligned with its handler.
This button only calls
scanImportableSkills({ silent: false }), so the current title/copy still reads as if the import finishes here instead of after the scan result is reviewed in Skills 管理.✏️ Minimal copy fix
- <span class="market-action-title">跨应用导入</span> - <span class="market-action-copy">扫描其他应用目录并导入到当前 {{ skillsTargetLabel }}</span> + <span class="market-action-title">扫描导入来源</span> + <span class="market-action-copy">扫描其他应用目录，随后在 Skills 管理中导入到当前 {{ skillsTargetLabel }}</span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/index.html` around lines 1419 - 1421, Update the button copy to reflect that it only scans for importable skills (does not perform the import) by changing the market-action-title/market-action-copy text for the element using the scanImportableSkills({ silent: false }) handler; keep the dynamic skillsTargetLabel but make the copy say it will "扫描并列出可导入项以在 Skills 管理 中审核" (or similar) so the UI accurately communicates that the action runs a scan and that actual importing happens later during review.web-ui/modules/skills.methods.mjs (2)
241-242:⚠️ Potential issue | 🟠 MajorBlock skill mutations while
refreshSkillsList()is still running.These entrypoints still ignore
skillsLoading. That leaves the footer actions callable against staleskillsList/skillsImportListduring a refresh, and a late list response can then overwrite the mutation's own follow-up refresh. The delete confirm callback should also disable if loading starts after the dialog opens.🛡️ Minimal guard update
- if (this.skillsDeleting || this.skillsScanningImports || this.skillsImporting || this.skillsZipImporting || this.skillsExporting) return; + if (this.skillsLoading || this.skillsDeleting || this.skillsScanningImports || this.skillsImporting || this.skillsZipImporting || this.skillsExporting) return; ... - if (this.skillsDeleting || this.skillsScanningImports || this.skillsZipImporting || this.skillsImporting || this.skillsExporting) return; + if (this.skillsLoading || this.skillsDeleting || this.skillsScanningImports || this.skillsZipImporting || this.skillsImporting || this.skillsExporting) return; ... - if (this.skillsDeleting || this.skillsExporting || this.skillsZipImporting || this.skillsImporting) return; + if (this.skillsLoading || this.skillsDeleting || this.skillsExporting || this.skillsZipImporting || this.skillsImporting) return; ... - if (this.skillsDeleting || this.skillsZipImporting || this.skillsExporting || this.skillsImporting) return; + if (this.skillsLoading || this.skillsDeleting || this.skillsZipImporting || this.skillsExporting || this.skillsImporting) return; ... - confirmDisabled: () => this.skillsDeleting || this.skillsScanningImports, + confirmDisabled: () => this.skillsLoading || this.skillsDeleting || this.skillsScanningImports,Also applies to: 334-335, 369-369, 414-432
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/modules/skills.methods.mjs` around lines 241 - 242, Several mutation entrypoints (e.g., importSelectedSkills) are missing the skillsLoading guard so actions can run against a stale list and be clobbered by a late refresh; update each entrypoint guard to include this.skillsLoading (add skillsLoading to the boolean OR check in importSelectedSkills and the other mutation methods referenced around lines 334-335, 369, and 414-432) so mutations are blocked while refreshSkillsList() is running, and also update the delete-confirm callback UI/handler to disable or no-op when this.skillsLoading becomes true after the dialog opens so a late loading state cannot allow the deletion to proceed.
67-87:⚠️ Potential issue | 🟠 MajorGuard
openSkillsManager()while market work is already in flight.This method still writes
this.skillsTargetAppdirectly and starts a new list refresh with no busy check. The config-side launcher does not disableskillsMarketLoading, so opening the modal during an overview refresh can switch targets mid-request and mix local/import state across hosts.🛡️ Minimal guard
async openSkillsManager(options = {}) { + if ( + this.skillsLoading + || this.skillsDeleting + || this.skillsScanningImports + || this.skillsImporting + || this.skillsZipImporting + || this.skillsExporting + || this.skillsMarketLoading + ) { + return false; + } let targetApp; try { targetApp = this.normalizeSkillsTargetApp(options && options.targetApp ? options.targetApp : this.skillsTargetApp);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/modules/skills.methods.mjs` around lines 67 - 87, openSkillsManager can mutate skillsTargetApp and call refreshSkillsList while a market request is already running, causing mixed state; add a guard at the start of openSkillsManager to check this.skillsMarketLoading and abort (return false) when true so we don't normalize/assign this.skillsTargetApp, call resetSkillsTargetState, or start refreshSkillsList while loading; use the existing symbols openSkillsManager, this.skillsMarketLoading, this.skillsTargetApp, resetSkillsTargetState, and refreshSkillsList to locate where to add the early-return guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli.js`:
- Around line 176-184: The fallback path returned by the
CODEX_HOME/XDG_CONFIG_HOME branches (variables envCodexHome and xdgConfig) may
contain an unexpanded tilde and isn't normalized because resolveExistingDir only
expands its candidates; update the branches that compute target
(path.join(envCodexHome, 'skills') and path.join(xdgConfig, 'codex', 'skills'))
to expand and normalize the fallback before returning (e.g., expand ~ to the
home dir and run path.resolve/path.normalize) so resolveExistingDir never
returns a literal unexpanded fallback; ensure the same change is applied to the
other analogous block around lines 194-201.
In `@README.md`:
- Around line 60-61: Replace inconsistent scan terminology in README.md by
choosing one unified phrase (either "可导入来源" or "未托管的 skills") and update all
occurrences to that term; specifically, edit the overview sentence that
currently uses "可导入来源" and the tab/detail text that uses "未托管的 skills" so both
use the same chosen term, ensuring surrounding context (e.g., descriptions of
scanning, ZIP import/export, cross-app import) reads naturally and any
pluralization or casing matches other headings and usage in the file.
In `@tests/unit/config-tabs-ui.test.mjs`:
- Around line 246-248: The test assertions expect behavior that no longer
exists: update the assertions in tests/unit/config-tabs-ui.test.mjs so they
match the current session-helper contract in web-ui/session-helpers.mjs — assert
that the helperScript contains the conditional "nextTab === 'market' && typeof
this.loadSkillsMarketOverview === 'function'" and that it directly calls
"this.loadSkillsMarketOverview({ silent: true });", and remove the assertions
checking "previousTab !== 'market'" and the "void
Promise.resolve(...).catch(...)" pattern.
---
Duplicate comments:
In `@cli.js`:
- Around line 9858-9885: The handler handleImportSkillsZipUpload currently
returns JSON on early error paths (non-POST, missing targetApp) and in the catch
without consuming or closing the incoming request body, which leaves upload
sockets open; add a small drain helper (e.g., drainRequest(req) that calls
req.resume() and removes/ignores further data) or call req.resume() directly
before every early return and before writeJsonResponse in the catch; update the
non-POST branch, the missing targetApp branch, and the catch block to call this
drain helper (or req.resume()) prior to calling writeJsonResponse so
writeUploadZipStream and content-length prechecks cannot leave the socket
hanging.
In `@web-ui/app.js`:
- Around line 3578-3584: The isConfirmDialogDisabled() method currently returns
false when confirmDialogDisableWhen throws, which leaves the dialog enabled on
errors; update the catch block in isConfirmDialogDisabled to return true
(fail-closed) so that any exception from the confirmDialogDisableWhen predicate
results in the dialog being disabled; locate the isConfirmDialogDisabled method
and the confirmDialogDisableWhen reference to make this change (optionally add a
process or console log of the caught error while ensuring the return value is
true).
In `@web-ui/index.html`:
- Around line 1419-1421: Update the button copy to reflect that it only scans
for importable skills (does not perform the import) by changing the
market-action-title/market-action-copy text for the element using the
scanImportableSkills({ silent: false }) handler; keep the dynamic
skillsTargetLabel but make the copy say it will "扫描并列出可导入项以在 Skills 管理 中审核" (or
similar) so the UI accurately communicates that the action runs a scan and that
actual importing happens later during review.
In `@web-ui/modules/skills.methods.mjs`:
- Around line 241-242: Several mutation entrypoints (e.g., importSelectedSkills)
are missing the skillsLoading guard so actions can run against a stale list and
be clobbered by a late refresh; update each entrypoint guard to include
this.skillsLoading (add skillsLoading to the boolean OR check in
importSelectedSkills and the other mutation methods referenced around lines
334-335, 369, and 414-432) so mutations are blocked while refreshSkillsList() is
running, and also update the delete-confirm callback UI/handler to disable or
no-op when this.skillsLoading becomes true after the dialog opens so a late
loading state cannot allow the deletion to proceed.
- Around line 67-87: openSkillsManager can mutate skillsTargetApp and call
refreshSkillsList while a market request is already running, causing mixed
state; add a guard at the start of openSkillsManager to check
this.skillsMarketLoading and abort (return false) when true so we don't
normalize/assign this.skillsTargetApp, call resetSkillsTargetState, or start
refreshSkillsList while loading; use the existing symbols openSkillsManager,
this.skillsMarketLoading, this.skillsTargetApp, resetSkillsTargetState, and
refreshSkillsList to locate where to add the early-return guard.
In `@web-ui/session-helpers.mjs`:
- Line 68: The fire-and-forget call to loadSkillsMarketOverview (void
Promise.resolve(this.loadSkillsMarketOverview({ silent: true })).catch(() =>
{})) can still throw synchronously before Promise.resolve is invoked; wrap the
call so synchronous exceptions are caught — e.g., capture the call in a
try/catch and then Promise.resolve the returned promise (or use
Promise.resolve().then(() => this.loadSkillsMarketOverview(...))). Update the
invocation that references loadSkillsMarketOverview to ensure both sync throws
and async rejections are swallowed.
In `@web-ui/styles.css`:
- Around line 3699-3709: The .market-target-chip currently keeps cursor: pointer
and no visual disabled state; add explicit disabled styles targeting
.market-target-chip[disabled], .market-target-chip.disabled and
.market-target-chip[aria-disabled="true"] to override interactivity (set cursor:
default or not-allowed, pointer-events: none), and provide a visual override
(reduced opacity or muted background/border and remove box-shadow/highlight)
while preserving transitions; update styles for color, background and border so
busy/disabled chips look non-clickable and consistent with existing variables.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7c8753ca-9ce1-4a19-b174-efd7e2c97552
📒 Files selected for processing (19)
README.en.mdREADME.mdcli.jssite/guide/getting-started.mdtests/e2e/run.jstests/unit/config-tabs-ui.test.mjstests/unit/readme-docs-consistency.test.mjstests/unit/run.mjstests/unit/session-tab-switch-performance.test.mjstests/unit/skills-market-runtime.test.mjstests/unit/skills-modal-ui.test.mjstests/unit/web-run-host.test.mjsweb-ui/app.jsweb-ui/index.htmlweb-ui/modules/config-mode.computed.mjsweb-ui/modules/skills.computed.mjsweb-ui/modules/skills.methods.mjsweb-ui/session-helpers.mjsweb-ui/styles.css
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
🪛 LanguageTool
README.en.md
[grammar] ~57-~57: Ensure spelling is correct
Context: ...level delete (supports batch) Skills Market - Switch the skills install target b...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~58-~58: Ensure spelling is correct
Context: ...install target between Codex and Claude Code - Inspect local installed skills, root...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~59-~59: Ensure spelling is correct
Context: ...local installed skills, root paths, and status - Scan importable sources from Codex...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~61-~61: Ensure spelling is correct
Context: ...pp import, ZIP import/export, and batch delete Engineering Utilities - MCP stdio dom...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (15)
web-ui/modules/config-mode.computed.mjs (1)
49-53:markettab label handling looks correct.This closes the fallback-to-
未知gap for a valid tab state.site/guide/getting-started.md (1)
30-32: Docs update is clear and safety-aware.Default LAN bind plus localhost-only fallback guidance is explicit and actionable.
web-ui/modules/skills.computed.mjs (1)
3-8: Computed additions are solid and defensive.Target mapping, busy-state aggregation, and preview slicing all include safe fallbacks for missing/non-array state.
Also applies to: 87-105
web-ui/styles.css (1)
40-65: Token and responsive additions look good.The shared tokens are now defined, and market-grid/action-grid collapse behavior is correctly handled at smaller breakpoints.
Also applies to: 4329-4331, 4358-4364
tests/e2e/run.js (1)
43-46: Deterministic E2E env isolation looks correct.Neutralizing these host-config env vars alongside
HOME/USERPROFILEkeeps the suite isolated from machine-local skill paths.tests/unit/run.mjs (1)
16-17: New unit suites are wired into the runner correctly.The added dynamic imports follow the existing registration model and preserve the runner’s execution/error semantics.
Also applies to: 30-30
tests/unit/session-tab-switch-performance.test.mjs (1)
130-200: Market-tab switching tests are solid and targeted.Good coverage for enter/re-enter behavior and rejected async preload handling without leaking unhandled rejections.
tests/unit/readme-docs-consistency.test.mjs (1)
14-24: README parity check is implemented correctly.Asserting the same invariant in both
README.mdandREADME.en.mdis a good CI guard against future drift.tests/unit/skills-modal-ui.test.mjs (3)
35-39: Confirm-dialog disable-state assertions are well aligned.These checks correctly enforce the new disabled gating path in script and template bindings.
46-63: Target-aware skills module assertions look correct.The added expectations for target label/root path and unified local API actions match the local-first Skills Market contract.
68-83: Layout/style checks are consistent with the market UI update.The new
.market-target-chipand confirm-button disabled binding checks are good safeguards, and the removed online-market selectors are validated.web-ui/app.js (2)
147-148: State initialization is complete and consistent.The new reactive fields are initialized safely and match the Skills Market/computed usage pattern.
Also applies to: 175-175, 189-191
3559-3572: Confirm dialog disable-state wiring/reset looks correct.
requestConfirmDialog()correctly handles both static and functional disable conditions, andresolveConfirmDialog()fully resets the related state.Also applies to: 3598-3599
README.md (2)
85-95: Architecture/state diagram update is coherent with the new Skills Market scope.The runtime/state nodes and
CORE --> SKILLSflow are clearly reflected and internally consistent.Also applies to: 112-112
127-129: LAN default now includes explicit safety guidance.The localhost override guidance is clear and placed where users will see it during setup.
Also applies to: 238-238
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
web-ui/session-helpers.mjs (1)
64-69:⚠️ Potential issue | 🟠 MajorGuard synchronous throws in the market preload call.
At Line 68,
Promise.resolve(this.loadSkillsMarketOverview(...))does not protect against synchronous exceptions thrown during argument evaluation, so tab switching can still fail before.catch()runs.Proposed non-breaking fix
- void Promise.resolve(this.loadSkillsMarketOverview({ silent: true })).catch(() => {}); + void Promise.resolve() + .then(() => this.loadSkillsMarketOverview({ silent: true })) + .catch(() => {});In JavaScript, does Promise.resolve(fn()) catch synchronous exceptions thrown by fn() before it returns? Please cite MDN.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/session-helpers.mjs` around lines 64 - 69, The current call to Promise.resolve(this.loadSkillsMarketOverview({ silent: true })) can still throw synchronously before Promise.resolve runs; wrap the invocation of loadSkillsMarketOverview in a try/catch so synchronous exceptions are captured and then normalize the result into a promise for the existing .catch handling. Specifically, inside the shouldLoadSkillsMarketOnEnter branch, call this.loadSkillsMarketOverview({ silent: true }) inside a try block, assign the result to a variable (e.g., result), then pass that variable into Promise.resolve(result).catch(...); in the catch block, handle or swallow the synchronous error consistently with the existing async .catch behavior — this ensures loadSkillsMarketOverview and the shouldLoadSkillsMarketOnEnter logic are protected against both sync and async failures.README.en.md (1)
60-61:⚠️ Potential issue | 🟡 MinorUnify scan terminology across sections for clarity.
The feature overview uses “importable sources,” while the tab details use “unmanaged skills” for the same scan concept. Using one term in both places would reduce ambiguity.
Also applies to: 204-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.en.md` around lines 60 - 61, Replace inconsistent terminology in README.en.md by choosing one term and using it everywhere: either change the phrase "importable sources" to "unmanaged skills" (or change "unmanaged skills" to "importable sources") so the feature overview and the tab details use the same wording; update the two occurrences shown in the diff ("Scan importable sources from `Codex` / `Claude Code` / `Agents`" and the corresponding tab description that uses "unmanaged skills") and any other matching lines in the file so the wording is consistent across the document.README.md (1)
60-61:⚠️ Potential issue | 🟡 Minor扫描术语建议统一，避免“可导入来源/未托管 skills”双表述。
当前两个段落在描述同一扫描对象时用词不同，读者可能误解为两个独立概念。建议统一为同一术语。
Also applies to: 204-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 60 - 61, Choose a single term to describe the scan target and make README text consistent by replacing all occurrences of the two different phrases—“可导入来源” and “未托管 skills”—with the chosen term; update the lines that currently read "扫描 `Codex` / `Claude Code` / `Agents` 可导入来源" and the other occurrences around lines 204-205 so both use the same terminology, and ensure related lines like "支持跨应用导入、ZIP 导入 / 导出、批量删除" still read naturally with the unified term.web-ui/app.js (1)
3578-3584:⚠️ Potential issue | 🟠 MajorFail-closed when confirm-disable predicate throws.
At Line 3583, an exception in
confirmDialogDisableWhencurrently returnsfalse, which enables the confirm button on guard-evaluation failure.🔒 Minimal safety fix
isConfirmDialogDisabled() { if (typeof this.confirmDialogDisableWhen === 'function') { try { return !!this.confirmDialogDisableWhen.call(this); } catch (_) { - return false; + return true; } } return !!this.confirmDialogConfirmDisabled; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/app.js` around lines 3578 - 3584, The isConfirmDialogDisabled() guard currently returns false if confirmDialogDisableWhen() throws, which enables confirmation on errors; change the catch branch in isConfirmDialogDisabled to return true (fail-closed) so that any exception from the confirmDialogDisableWhen predicate disables the confirm action; locate the isConfirmDialogDisabled method and update its catch to return true (optionally add a debug log using process logger or console if available) to ensure safety on predicate failure.cli.js (1)
9858-9868:⚠️ Potential issue | 🟠 MajorDrain the upload stream before these early returns.
Line 9860 and Line 9867 return before
writeUploadZipStream()attaches any body listeners. If a client already started sending the ZIP body, the unread bytes stay on the socket until timeout.🩹 Minimal fix
async function handleImportSkillsZipUpload(req, res, options = {}) { if (req.method !== 'POST') { + req.resume(); writeJsonResponse(res, 405, { error: 'Method Not Allowed' }); return; } try { const forcedTargetApp = normalizeSkillTargetApp(options && options.targetApp ? options.targetApp : ''); const targetApp = forcedTargetApp || resolveSkillTargetAppFromRequest(req, 'codex'); if (!targetApp) { + req.resume(); writeJsonResponse(res, 400, { error: '目标宿主不支持' }); return; }Node.js http IncomingMessage should I call req.resume() before returning an early error response when not consuming the request body?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 9858 - 9868, In handleImportSkillsZipUpload, drain the incoming request body before any early return so the socket isn't left with unread bytes: when checking req.method !== 'POST' and after resolving targetApp (before returning the 400), ensure you call a drain action on req (e.g., req.resume() or otherwise consume the stream) so the client upload is flushed even though writeUploadZipStream() listeners aren’t attached yet; update the branches around normalizeSkillTargetApp and resolveSkillTargetAppFromRequest to perform this drain prior to writeJsonResponse and return.web-ui/styles.css (1)
3699-3709:⚠️ Potential issue | 🟡 MinorAdd a real disabled state for the target chips.
index.htmlnow disables these buttons with:disabled="skillsMarketBusy", but this rule still forcescursor: pointerand never visually mutes the control. During busy periods the chips keep looking clickable unlike the other disabled market actions.🩹 Minimal CSS fix
.market-target-chip { border: 1px solid rgba(160, 145, 130, 0.28); border-radius: 999px; background: rgba(255, 255, 255, 0.92); color: var(--color-text-secondary); padding: 8px 14px; font-size: var(--font-size-caption); font-weight: var(--font-weight-secondary); cursor: pointer; transition: border-color var(--transition-fast) var(--ease-smooth), background var(--transition-fast) var(--ease-smooth), color var(--transition-fast) var(--ease-smooth), box-shadow var(--transition-fast) var(--ease-smooth); } + +.market-target-chip:disabled, +.market-target-chip[disabled] { + cursor: not-allowed; + opacity: 0.64; + box-shadow: none; + pointer-events: none; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles.css` around lines 3699 - 3709, The .market-target-chip rule lacks a disabled state, so when the element is marked disabled (e.g., :disabled="skillsMarketBusy") it still shows cursor:pointer and normal styling; add a disabled selector (e.g., .market-target-chip:disabled and/or .market-target-chip[disabled]) that mutes the visual appearance and interaction by setting cursor to default (or not-allowed), reducing opacity or changing background/color to the secondary/disabled tokens, removing box-shadow and disabling pointer events while preserving transitions; update the disabled selector to target the same class names used in the template (.market-target-chip) so busy chips look visually disabled and non-interactive.web-ui/index.html (1)
1419-1421:⚠️ Potential issue | 🟡 MinorMake this card describe the scan step, not the import step.
This button only starts
scanImportableSkills({ silent: false }), but the current title/copy still says the import completes here. That is misleading because the actual import still happens later inSkills 管理.✏️ Minimal copy fix
- <span class="market-action-title">跨应用导入</span> - <span class="market-action-copy">扫描其他应用目录并导入到当前 {{ skillsTargetLabel }}</span> + <span class="market-action-title">扫描导入来源</span> + <span class="market-action-copy">扫描其他应用目录，随后在 Skills 管理中导入到当前 {{ skillsTargetLabel }}</span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/index.html` around lines 1419 - 1421, The button currently labeled as performing an import is misleading because it only calls scanImportableSkills({ silent: false }); update the UI copy for the button with class "market-action-card" (and its child spans "market-action-title" and "market-action-copy") to describe the scanning step — e.g., change the title to something like "扫描可导入技能" and the copy to indicate it will scan other app directories and list results for import in "Skills 管理" (retain binding to skillsTargetLabel and disable behavior using skillsMarketBusy).web-ui/modules/skills.methods.mjs (1)
67-87:⚠️ Potential issue | 🟠 Major
openSkillsManager()can still switch targets mid-flight.This method changes
this.skillsTargetApp(line 77) and startsrefreshSkillsList()(line 86) without the busy-state guard thatsetSkillsTargetApp()already has. If import/delete/export/scan work is still running, those handlers will later toast and refresh against the new target instead of the one they actually mutated.🛡️ Suggested fix: add busy-state guard
async openSkillsManager(options = {}) { + if ( + this.skillsLoading + || this.skillsDeleting + || this.skillsScanningImports + || this.skillsImporting + || this.skillsZipImporting + || this.skillsExporting + || this.skillsMarketLoading + ) { + return false; + } let targetApp; try { targetApp = this.normalizeSkillsTargetApp(options && options.targetApp ? options.targetApp : this.skillsTargetApp);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/modules/skills.methods.mjs` around lines 67 - 87, openSkillsManager() can change this.skillsTargetApp and call refreshSkillsList() while other skill operations are in progress, causing later handlers to operate against the wrong target; mirror the busy-state guard used in setSkillsTargetApp(): check the same busy flag (or call the same isBusy guard) at the start of openSkillsManager(), bail out or await until not busy, and only then proceed to normalize targetApp, update this.skillsTargetApp and call resetSkillsTargetState()/refreshSkillsList(); ensure the busy check uses the same shared flag/method as setSkillsTargetApp to prevent mid-flight target switches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli.js`:
- Around line 1441-1456: The function resolveSkillTarget currently treats
explicitly provided but empty selectors (e.g., targetApp: '' or target: '') as
absent and falls back to the default; add an explicit rejection: if
hasAnyExplicitTarget is true and the resolved raw value is empty, return null.
Keep the existing invalid-target check (hasAnyExplicitTarget && raw &&
!getSkillTargetByApp(raw)) but add a prior check (hasAnyExplicitTarget && !raw)
to return null so explicit-but-empty selectors do not fall back to default;
update the logic in resolveSkillTarget accordingly.
---
Duplicate comments:
In `@cli.js`:
- Around line 9858-9868: In handleImportSkillsZipUpload, drain the incoming
request body before any early return so the socket isn't left with unread bytes:
when checking req.method !== 'POST' and after resolving targetApp (before
returning the 400), ensure you call a drain action on req (e.g., req.resume() or
otherwise consume the stream) so the client upload is flushed even though
writeUploadZipStream() listeners aren’t attached yet; update the branches around
normalizeSkillTargetApp and resolveSkillTargetAppFromRequest to perform this
drain prior to writeJsonResponse and return.
In `@README.en.md`:
- Around line 60-61: Replace inconsistent terminology in README.en.md by
choosing one term and using it everywhere: either change the phrase "importable
sources" to "unmanaged skills" (or change "unmanaged skills" to "importable
sources") so the feature overview and the tab details use the same wording;
update the two occurrences shown in the diff ("Scan importable sources from
`Codex` / `Claude Code` / `Agents`" and the corresponding tab description that
uses "unmanaged skills") and any other matching lines in the file so the wording
is consistent across the document.
In `@README.md`:
- Around line 60-61: Choose a single term to describe the scan target and make
README text consistent by replacing all occurrences of the two different
phrases—“可导入来源” and “未托管 skills”—with the chosen term; update the lines that
currently read "扫描 `Codex` / `Claude Code` / `Agents` 可导入来源" and the other
occurrences around lines 204-205 so both use the same terminology, and ensure
related lines like "支持跨应用导入、ZIP 导入 / 导出、批量删除" still read naturally with the
unified term.
In `@web-ui/app.js`:
- Around line 3578-3584: The isConfirmDialogDisabled() guard currently returns
false if confirmDialogDisableWhen() throws, which enables confirmation on
errors; change the catch branch in isConfirmDialogDisabled to return true
(fail-closed) so that any exception from the confirmDialogDisableWhen predicate
disables the confirm action; locate the isConfirmDialogDisabled method and
update its catch to return true (optionally add a debug log using process logger
or console if available) to ensure safety on predicate failure.
In `@web-ui/index.html`:
- Around line 1419-1421: The button currently labeled as performing an import is
misleading because it only calls scanImportableSkills({ silent: false }); update
the UI copy for the button with class "market-action-card" (and its child spans
"market-action-title" and "market-action-copy") to describe the scanning step —
e.g., change the title to something like "扫描可导入技能" and the copy to indicate it
will scan other app directories and list results for import in "Skills 管理"
(retain binding to skillsTargetLabel and disable behavior using
skillsMarketBusy).
In `@web-ui/modules/skills.methods.mjs`:
- Around line 67-87: openSkillsManager() can change this.skillsTargetApp and
call refreshSkillsList() while other skill operations are in progress, causing
later handlers to operate against the wrong target; mirror the busy-state guard
used in setSkillsTargetApp(): check the same busy flag (or call the same isBusy
guard) at the start of openSkillsManager(), bail out or await until not busy,
and only then proceed to normalize targetApp, update this.skillsTargetApp and
call resetSkillsTargetState()/refreshSkillsList(); ensure the busy check uses
the same shared flag/method as setSkillsTargetApp to prevent mid-flight target
switches.
In `@web-ui/session-helpers.mjs`:
- Around line 64-69: The current call to
Promise.resolve(this.loadSkillsMarketOverview({ silent: true })) can still throw
synchronously before Promise.resolve runs; wrap the invocation of
loadSkillsMarketOverview in a try/catch so synchronous exceptions are captured
and then normalize the result into a promise for the existing .catch handling.
Specifically, inside the shouldLoadSkillsMarketOnEnter branch, call
this.loadSkillsMarketOverview({ silent: true }) inside a try block, assign the
result to a variable (e.g., result), then pass that variable into
Promise.resolve(result).catch(...); in the catch block, handle or swallow the
synchronous error consistently with the existing async .catch behavior — this
ensures loadSkillsMarketOverview and the shouldLoadSkillsMarketOnEnter logic are
protected against both sync and async failures.
In `@web-ui/styles.css`:
- Around line 3699-3709: The .market-target-chip rule lacks a disabled state, so
when the element is marked disabled (e.g., :disabled="skillsMarketBusy") it
still shows cursor:pointer and normal styling; add a disabled selector (e.g.,
.market-target-chip:disabled and/or .market-target-chip[disabled]) that mutes
the visual appearance and interaction by setting cursor to default (or
not-allowed), reducing opacity or changing background/color to the
secondary/disabled tokens, removing box-shadow and disabling pointer events
while preserving transitions; update the disabled selector to target the same
class names used in the template (.market-target-chip) so busy chips look
visually disabled and non-interactive.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5faf8416-703b-419d-84b9-5bd977b99838
📒 Files selected for processing (19)
README.en.mdREADME.mdcli.jssite/guide/getting-started.mdtests/e2e/run.jstests/unit/config-tabs-ui.test.mjstests/unit/readme-docs-consistency.test.mjstests/unit/run.mjstests/unit/session-tab-switch-performance.test.mjstests/unit/skills-market-runtime.test.mjstests/unit/skills-modal-ui.test.mjstests/unit/web-run-host.test.mjsweb-ui/app.jsweb-ui/index.htmlweb-ui/modules/config-mode.computed.mjsweb-ui/modules/skills.computed.mjsweb-ui/modules/skills.methods.mjsweb-ui/session-helpers.mjsweb-ui/styles.css
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
🔇 Additional comments (26)
web-ui/modules/skills.computed.mjs (1)
3-8: Computed additions look solid and defensively implemented.The new market computed values handle missing/invalid state safely and keep UI preview/busy logic bounded and predictable.
Also applies to: 87-105
web-ui/modules/config-mode.computed.mjs (1)
52-52: Explicit market tab label mapping is correct.This removes the unintended
'未知'fallback for a valid tab state.tests/e2e/run.js (1)
43-46: E2E env isolation change looks good.Clearing these home/config env vars in test context reduces host-environment leakage across runs.
tests/unit/run.mjs (1)
16-17: Test runner updates are consistent.New modules are integrated using the existing import/register pattern without altering execution semantics.
Also applies to: 30-30
site/guide/getting-started.md (1)
30-32: Host default and security warning are clearly documented.The guidance now explicitly communicates LAN exposure implications and the localhost-only fallback.
tests/unit/session-tab-switch-performance.test.mjs (1)
130-149: Market-tab switch tests add valuable regression coverage.The new cases meaningfully verify one-time load-on-enter behavior and rejected-promise handling.
Also applies to: 151-179, 181-200
tests/unit/readme-docs-consistency.test.mjs (1)
14-24: Cross-language README consistency check is a good safeguard.Asserting the same invariant in both
README.mdandREADME.en.mdshould prevent silent drift.tests/unit/skills-modal-ui.test.mjs (2)
35-63: Good regression coverage for local-first skills wiring.These assertions correctly lock in the confirm-dialog disable hooks and local skills API lifecycle calls, while preventing regressions back to online-market paths.
68-83: Template/style checks are well-targeted for this change.Validating
:disabled="isConfirmDialogDisabled()"plus the local-market CSS presence/absence is a solid guardrail for the updated UI behavior.README.en.md (1)
127-130: Security guidance is clear and actionable with the LAN default.The default host statement is paired with a concrete localhost fallback instruction, which improves safety communication.
Also applies to: 237-237
README.md (1)
127-129:0.0.0.0默认监听与安全提示配套完整。默认值、风险提示、以及切回本机监听的操作指引写得很清楚。
Also applies to: 238-238
tests/unit/config-tabs-ui.test.mjs (1)
246-248: Re-verify market-enter assertions against current helper contract.These expectations were previously called out as potentially out of sync with
web-ui/session-helpers.mjs. Please confirm current helper logic still includes bothpreviousTab !== 'market'and thePromise.resolve(...).catch(...)wrapper before keeping these assertions.#!/bin/bash set -euo pipefail echo "== session helper contract ==" rg -n -C3 "shouldLoadSkillsMarketOnEnter|loadSkillsMarketOverview|previousTab" web-ui/session-helpers.mjs echo echo "== current test expectations ==" rg -n -C3 "shouldLoadSkillsMarketOnEnter|loadSkillsMarketOverview|previousTab|Promise\\.resolve" tests/unit/config-tabs-ui.test.mjsweb-ui/index.html (1)
1297-1303: Verify thatskillsMarketBusycovers global init failure/loading.These controls only bind
:disabled="skillsMarketBusy", while Line 485 explicitly guards the config-side Skills entry withloading || !!initError. IfskillsMarketBusystill excludes the global init flags, the market page remains clickable under the loading/error banner.#!/bin/bash set -euo pipefail index_file="$(fd -i 'index.html' web-ui | head -n 1)" echo "== market control bindings ==" sed -n '1297,1425p' "$index_file" echo echo "== skillsMarketBusy definition and references ==" rg -n -C5 '\bskillsMarketBusy\b' web-uiExpected result: the
skillsMarketBusydefinition should clearly includeloadingandinitError(or an equivalent global-init guard). If it does not, these bindings need the same explicit guard used at Line 485.Also applies to: 1307-1323, 1363-1365, 1388-1390, 1415-1425
tests/unit/web-run-host.test.mjs (5)
1-12: LGTM! Test setup correctly extracts cli.js for isolated unit testing.The approach of reading
cli.jssource and extracting functions via brace-matching enables testing internal helpers with controlled mocks without exposing them as public exports.
14-90: Well-implemented brace matcher with proper handling of edge cases.The parser correctly handles nested template literals, escape sequences, and both comment styles. The template depth tracking ensures
${...}expressions don't prematurely close the function body.
538-596: Good safety coverage: nested path rejection test.This test verifies the critical safety behavior that import operations reject target paths nested inside source directories. The assertions confirm no copy occurs,
ensureDiris never called, and an appropriate error is recorded.
670-720: Important resource cleanup verification.The test confirms that
importSkillsFromZipFilecleans uptempDirviarmSync(..., { recursive: true, force: true })even when the target app is unsupported, preventing temporary file accumulation.
722-856: Comprehensive precedence testing for import flows.These tests verify the critical behavior that
targetApptakes precedence overtarget, and that raw invalid targets are preserved for downstream validation rather than silently coerced.web-ui/modules/skills.methods.mjs (4)
11-19: Good: Strict target validation now rejects typos and case variants.
normalizeSkillsTargetAppnow only accepts exact'codex'or'claude'and throws on anything else, preventing silent fallback that could redirect operations to the wrong host.
33-65: Comprehensive busy-state guard insetSkillsTargetApp.The method correctly blocks target switching while any skills action is in flight, preventing race conditions where operations would toast/refresh against the wrong target.
414-468: Good defensive check: delete blocked with user feedback during scan.The separate check for
skillsScanningImportsat lines 416-419 provides a clear error message to the user explaining why the operation can't proceed, rather than silently returning.
104-144: LGTM:refreshSkillsListproperly handles all error states.The method correctly returns
falseon API error,trueon success (including empty list with missing directory), and updates state appropriately in each case. The target-aware API call at lines 107-109 ensures operations scope to the correct host.tests/unit/skills-market-runtime.test.mjs (4)
11-54: Well-designed test harness with computed property support.The
buildVmhelper properly installs computed getters (lines 42-51) so tests can verify reactive behavior likeskillsTargetLabelandskillsMarketBusywithout duplicating the computed definitions.
105-121: Good: Tests verify busy-state guard blocks target switch.This test confirms that
setSkillsTargetAppreturnsfalseand preserves the original target whenskillsImportingis active, matching the implementation's guard logic.
338-355: Good: Tests verifyconfirmDisabledreflects live scanning state.The test at lines 352-354 confirms that the
confirmDisabledpredicate passed torequestConfirmDialogcorrectly evaluates against the VM's currentskillsScanningImportsstate, ensuring the confirm button disables dynamically.
179-207: Good coverage: same-target reopen preserves loaded state but clears selections.This test verifies the intended UX where reopening the modal for the same target clears user selections (
skillsSelectedNames,skillsImportSelectedKeys,skillsKeyword,skillsStatusFilter) but preserves the loaded-once flags and import list to avoid redundant API calls.
There was a problem hiding this comment.
♻️ Duplicate comments (7)
web-ui/styles.css (1)
3699-3709:⚠️ Potential issue | 🟡 MinorAdd an explicit disabled style for target chips.
Line [3707] forces
cursor: pointer, but there is still no.market-target-chip:disabled(or[disabled]) rule. Disabled chips can still look interactive during busy states.🩹 Minimal non-breaking CSS fix
.market-target-chip { border: 1px solid rgba(160, 145, 130, 0.28); border-radius: 999px; background: rgba(255, 255, 255, 0.92); color: var(--color-text-secondary); padding: 8px 14px; font-size: var(--font-size-caption); font-weight: var(--font-weight-secondary); cursor: pointer; transition: border-color var(--transition-fast) var(--ease-smooth), background var(--transition-fast) var(--ease-smooth), color var(--transition-fast) var(--ease-smooth), box-shadow var(--transition-fast) var(--ease-smooth); } + +.market-target-chip:disabled, +.market-target-chip[disabled] { + cursor: not-allowed; + pointer-events: none; + opacity: 0.64; + box-shadow: none; +}#!/bin/bash # Verify disabled-state selector coverage for market target chips. rg -n --fixed-strings ".market-target-chip:disabled" web-ui/styles.css rg -n --fixed-strings ".market-target-chip[disabled]" web-ui/styles.css🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles.css` around lines 3699 - 3709, The .market-target-chip always sets cursor: pointer but lacks disabled-state styling; add explicit selectors .market-target-chip:disabled and .market-target-chip[disabled] to override interactive styles when disabled—set cursor to default (or not-allowed), reduce opacity or adjust color to var(--color-text-disabled), and disable pointer-events and transition effects as needed so disabled chips appear non-interactive; update any focus/hover rules (e.g., :hover, :focus) for .market-target-chip to avoid applying interactive styles when the element matches :disabled or [disabled].cli.js (2)
9858-9868:⚠️ Potential issue | 🟠 MajorDrain the upload body on early error returns.
The branches on Line 9859 and Line 9866 return before
writeUploadZipStream()attaches any listeners, so unread upload bytes remain buffered on the socket.🩹 Minimal fix
async function handleImportSkillsZipUpload(req, res, options = {}) { if (req.method !== 'POST') { + req.resume(); writeJsonResponse(res, 405, { error: 'Method Not Allowed' }); return; } try { const forcedTargetApp = normalizeSkillTargetApp(options && options.targetApp ? options.targetApp : ''); const targetApp = forcedTargetApp || resolveSkillTargetAppFromRequest(req, 'codex'); if (!targetApp) { + req.resume(); writeJsonResponse(res, 400, { error: '目标宿主不支持' }); return; }#!/bin/bash set -e echo "=== handleImportSkillsZipUpload ===" sed -n '9858,9887p' cli.js echo echo "=== writeUploadZipStream (where request listeners are attached) ===" sed -n '8591,8658p' cli.js🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 9858 - 9868, The function handleImportSkillsZipUpload returns early on non-POST and missing targetApp branches without draining the incoming request body, leaving upload bytes buffered; update those early-return paths in handleImportSkillsZipUpload to consume/drain the request stream before responding by invoking the same drain logic used by writeUploadZipStream (e.g., call a drainRequestBody helper or reuse writeUploadZipStream to attach the data/end/error listeners to req) so the socket buffer is cleared prior to writeJsonResponse; ensure you reference writeUploadZipStream or the new drainRequestBody helper when implementing the fix so the same listener behavior is used.
1441-1456:⚠️ Potential issue | 🟠 MajorReject explicitly empty target selectors.
Line 1449 still collapses
targetApp: ''/target: ''into the default target, solist/import/export/deletecan silently hit Codex when the caller actually sent an invalid selector.🩹 Minimal fix
function resolveSkillTarget(params = {}, defaultApp = 'codex') { const hasExplicitTargetApp = !!(params && typeof params === 'object' && Object.prototype.hasOwnProperty.call(params, 'targetApp')); const hasExplicitTarget = !!(params && typeof params === 'object' && Object.prototype.hasOwnProperty.call(params, 'target')); const hasAnyExplicitTarget = hasExplicitTargetApp || hasExplicitTarget; const rawTargetApp = hasExplicitTargetApp ? params.targetApp : ''; const rawTarget = hasExplicitTarget ? params.target : ''; const raw = rawTargetApp || rawTarget || ''; + if (hasAnyExplicitTarget && !raw) { + return null; + } if (hasAnyExplicitTarget && raw && !getSkillTargetByApp(raw)) { return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 1441 - 1456, The resolver collapses explicit empty selectors into the default; update resolveSkillTarget so that if the caller explicitly provides targetApp or target but the resolved raw value is empty or doesn't map to a known target, the function returns null. Concretely, in resolveSkillTarget adjust the conditional that currently reads "if (hasAnyExplicitTarget && raw && !getSkillTargetByApp(raw))" to treat empty raw as invalid (e.g., "if (hasAnyExplicitTarget && (!raw || !getSkillTargetByApp(raw)))"), referencing params.targetApp / params.target and getSkillTargetByApp to locate the logic to change.web-ui/app.js (1)
3578-3584:⚠️ Potential issue | 🟠 MajorFail-open confirm disable guard can allow unintended destructive confirmation.
At Line 3583, errors in
confirmDialogDisableWhenreturnfalse(enabled). This should fail closed and keep confirm disabled.🔒 Minimal safety fix
isConfirmDialogDisabled() { if (typeof this.confirmDialogDisableWhen === 'function') { try { return !!this.confirmDialogDisableWhen.call(this); } catch (_) { - return false; + return true; } } return !!this.confirmDialogConfirmDisabled; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/app.js` around lines 3578 - 3584, The guard isConfirmDialogDisabled currently treats exceptions from confirmDialogDisableWhen as returning false (enabling the confirm), which is unsafe; change the catch behavior so that if calling this.confirmDialogDisableWhen throws or returns a non-boolean, the method fails closed by returning true (i.e., keep the confirm disabled). Locate isConfirmDialogDisabled and update the try/catch to return true on any caught error and coerce/validate the result of confirmDialogDisableWhen.call(this) to a boolean before returning.README.md (1)
60-61:⚠️ Potential issue | 🟡 MinorUnify scan terminology between overview and tab details.
Line 60 uses “可导入来源”, while Line 204 uses “未托管的 skills”. Using one term in both places will avoid ambiguity.
Also applies to: 204-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 60 - 61, Summary: The README uses two different phrases — “可导入来源” and “未托管的 skills” — for the same concept; replace them with a single unified term. Fix: pick one canonical phrase (e.g., “可导入来源”) and update all occurrences so both the overview line currently using “可导入来源” and the tab detail lines that use “未托管的 skills” use that same phrase; search for the exact tokens “可导入来源” and “未托管的 skills” and make the replacements so wording is consistent across the README. Ensure grammar and punctuation around the replaced phrase remain correct.web-ui/modules/skills.methods.mjs (2)
67-87:⚠️ Potential issue | 🟠 MajorGuard
openSkillsManager()while market loading is still in flight.
openSkillsManager()can still retarget and start a freshrefreshSkillsList()whileloadSkillsMarketOverview()is running. The config-side trigger at Line 485 does not disable onskillsMarketLoading, so late responses can repopulateskillsList/skillsImportListfor the wrong host afterskillsTargetAppchanges.🩹 Minimal guard
async openSkillsManager(options = {}) { + if ( + this.skillsLoading + || this.skillsDeleting + || this.skillsScanningImports + || this.skillsImporting + || this.skillsZipImporting + || this.skillsExporting + || this.skillsMarketLoading + ) { + return false; + } let targetApp; try { targetApp = this.normalizeSkillsTargetApp(options && options.targetApp ? options.targetApp : this.skillsTargetApp);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/modules/skills.methods.mjs` around lines 67 - 87, openSkillsManager can change skillsTargetApp and call refreshSkillsList while an in-flight market load (loadSkillsMarketOverview) is still running; add a guard at the top of openSkillsManager to detect this.skillsMarketLoading and avoid retargeting/refreshing during the load (either return false or await the existing load before proceeding). Specifically, update openSkillsManager to check this.skillsMarketLoading and if true: show a short-notice (or return false) instead of changing this.skillsTargetApp and calling refreshSkillsList, ensuring in-flight load results don't repopulate skillsList/skillsImportList for the wrong host.
241-242:⚠️ Potential issue | 🟠 MajorBlock import, ZIP import, and delete while
skillsLoadingis true.These entrypoints still overlap with
refreshSkillsList(). The modal footer actions at Line 2292, Line 2298, and Line 2301 remain reachable duringskillsLoading, so an older refresh response can land after the action’s own follow-up refresh and restore stale pre-change state.🩹 Minimal guard
async importSelectedSkills() { - if (this.skillsDeleting || this.skillsScanningImports || this.skillsImporting || this.skillsZipImporting || this.skillsExporting) return; + if (this.skillsLoading || this.skillsDeleting || this.skillsScanningImports || this.skillsImporting || this.skillsZipImporting || this.skillsExporting) return; const selectedSet = new Set(Array.isArray(this.skillsImportSelectedKeys) ? this.skillsImportSelectedKeys : []);async importSkillsFromZipFile(file) { - if (this.skillsDeleting || this.skillsScanningImports || this.skillsZipImporting || this.skillsImporting || this.skillsExporting) return; + if (this.skillsLoading || this.skillsDeleting || this.skillsScanningImports || this.skillsZipImporting || this.skillsImporting || this.skillsExporting) return; const maxSize = 20 * 1024 * 1024;async deleteSelectedSkills() { - if (this.skillsDeleting || this.skillsZipImporting || this.skillsExporting || this.skillsImporting) return; + if (this.skillsLoading || this.skillsDeleting || this.skillsZipImporting || this.skillsExporting || this.skillsImporting) return; if (this.skillsScanningImports) { this.showMessage('正在扫描导入源，请稍后再试', 'error'); return;Also applies to: 333-335, 415-418
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/modules/skills.methods.mjs` around lines 241 - 242, The various action entrypoints (e.g., importSelectedSkills) currently guard against overlapping operations using flags like skillsDeleting, skillsScanningImports, skillsImporting, skillsZipImporting, and skillsExporting but omit skillsLoading, allowing actions to run while refreshSkillsList() is in-flight and potentially restore stale state; update the guard in importSelectedSkills and the other affected entrypoints referenced (around the other occurrences at the 333-335 and 415-418 blocks) to also check this.skillsLoading and return early when true, and ensure any modal-footer action handlers that trigger these flows include the same skillsLoading check so actions are blocked while refreshSkillsList() is pending.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cli.js`:
- Around line 9858-9868: The function handleImportSkillsZipUpload returns early
on non-POST and missing targetApp branches without draining the incoming request
body, leaving upload bytes buffered; update those early-return paths in
handleImportSkillsZipUpload to consume/drain the request stream before
responding by invoking the same drain logic used by writeUploadZipStream (e.g.,
call a drainRequestBody helper or reuse writeUploadZipStream to attach the
data/end/error listeners to req) so the socket buffer is cleared prior to
writeJsonResponse; ensure you reference writeUploadZipStream or the new
drainRequestBody helper when implementing the fix so the same listener behavior
is used.
- Around line 1441-1456: The resolver collapses explicit empty selectors into
the default; update resolveSkillTarget so that if the caller explicitly provides
targetApp or target but the resolved raw value is empty or doesn't map to a
known target, the function returns null. Concretely, in resolveSkillTarget
adjust the conditional that currently reads "if (hasAnyExplicitTarget && raw &&
!getSkillTargetByApp(raw))" to treat empty raw as invalid (e.g., "if
(hasAnyExplicitTarget && (!raw || !getSkillTargetByApp(raw)))"), referencing
params.targetApp / params.target and getSkillTargetByApp to locate the logic to
change.
In `@README.md`:
- Around line 60-61: Summary: The README uses two different phrases — “可导入来源”
and “未托管的 skills” — for the same concept; replace them with a single unified
term. Fix: pick one canonical phrase (e.g., “可导入来源”) and update all occurrences
so both the overview line currently using “可导入来源” and the tab detail lines that
use “未托管的 skills” use that same phrase; search for the exact tokens “可导入来源” and
“未托管的 skills” and make the replacements so wording is consistent across the
README. Ensure grammar and punctuation around the replaced phrase remain
correct.
In `@web-ui/app.js`:
- Around line 3578-3584: The guard isConfirmDialogDisabled currently treats
exceptions from confirmDialogDisableWhen as returning false (enabling the
confirm), which is unsafe; change the catch behavior so that if calling
this.confirmDialogDisableWhen throws or returns a non-boolean, the method fails
closed by returning true (i.e., keep the confirm disabled). Locate
isConfirmDialogDisabled and update the try/catch to return true on any caught
error and coerce/validate the result of confirmDialogDisableWhen.call(this) to a
boolean before returning.
In `@web-ui/modules/skills.methods.mjs`:
- Around line 67-87: openSkillsManager can change skillsTargetApp and call
refreshSkillsList while an in-flight market load (loadSkillsMarketOverview) is
still running; add a guard at the top of openSkillsManager to detect
this.skillsMarketLoading and avoid retargeting/refreshing during the load
(either return false or await the existing load before proceeding).
Specifically, update openSkillsManager to check this.skillsMarketLoading and if
true: show a short-notice (or return false) instead of changing
this.skillsTargetApp and calling refreshSkillsList, ensuring in-flight load
results don't repopulate skillsList/skillsImportList for the wrong host.
- Around line 241-242: The various action entrypoints (e.g.,
importSelectedSkills) currently guard against overlapping operations using flags
like skillsDeleting, skillsScanningImports, skillsImporting, skillsZipImporting,
and skillsExporting but omit skillsLoading, allowing actions to run while
refreshSkillsList() is in-flight and potentially restore stale state; update the
guard in importSelectedSkills and the other affected entrypoints referenced
(around the other occurrences at the 333-335 and 415-418 blocks) to also check
this.skillsLoading and return early when true, and ensure any modal-footer
action handlers that trigger these flows include the same skillsLoading check so
actions are blocked while refreshSkillsList() is pending.
In `@web-ui/styles.css`:
- Around line 3699-3709: The .market-target-chip always sets cursor: pointer but
lacks disabled-state styling; add explicit selectors
.market-target-chip:disabled and .market-target-chip[disabled] to override
interactive styles when disabled—set cursor to default (or not-allowed), reduce
opacity or adjust color to var(--color-text-disabled), and disable
pointer-events and transition effects as needed so disabled chips appear
non-interactive; update any focus/hover rules (e.g., :hover, :focus) for
.market-target-chip to avoid applying interactive styles when the element
matches :disabled or [disabled].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d55fb903-9fc1-4d94-9955-61a6d109e0ba
📒 Files selected for processing (19)
README.en.mdREADME.mdcli.jssite/guide/getting-started.mdtests/e2e/run.jstests/unit/config-tabs-ui.test.mjstests/unit/readme-docs-consistency.test.mjstests/unit/run.mjstests/unit/session-tab-switch-performance.test.mjstests/unit/skills-market-runtime.test.mjstests/unit/skills-modal-ui.test.mjstests/unit/web-run-host.test.mjsweb-ui/app.jsweb-ui/index.htmlweb-ui/modules/config-mode.computed.mjsweb-ui/modules/skills.computed.mjsweb-ui/modules/skills.methods.mjsweb-ui/session-helpers.mjsweb-ui/styles.css
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
🔇 Additional comments (29)
tests/e2e/run.js (1)
39-49: Good test-environment isolation for Skills path resolution.This env override block cleanly prevents host-specific Codex/Claude config directories from leaking into E2E runs while preserving the existing test flow.
web-ui/styles.css (2)
40-64: Token additions are correct and close the shared-styles gap.Defining
--font-size-large,--font-weight-primary, and--radius-mdresolves the invalid custom-property fallbacks for the new market/skills panels.
4329-4331: Responsive market layout rules look solid.The compact alignment and single-column fallbacks for market grid/action cards are consistent and reduce small-screen layout breakage risk.
Also applies to: 4358-4364
cli.js (3)
1664-1702: LGTM: list/scan responses are now target-aware.Surfacing
targetApp/targetLabeland excluding the active target from unmanaged sources gives the new market tab the right state when switching between Codex and Claude.Also applies to: 1749-1812
2200-2321: LGTM: export/delete generalized without dropping the Codex shims.The generic paths still keep the old Codex wrapper entry points, so existing callers stay stable while the UI gets target metadata back.
Also applies to: 2331-2392
10031-10045: LGTM: the generic skills actions are additive.Keeping
list-skills/import-skills/export-skillsalongside the existing Codex-specific actions is the right compatibility shape for this feature.web-ui/session-helpers.mjs (1)
64-69: Guard synchronous throws in the market overview fire-and-forget call.The past review correctly identified that
Promise.resolve(this.loadSkillsMarketOverview(...))evaluates the function call synchronously. IfloadSkillsMarketOverviewthrows synchronously before returning a promise, the exception escapes and can interrupt tab switching.🛠️ Recommended fix
if (shouldLoadSkillsMarketOnEnter) { - void Promise.resolve(this.loadSkillsMarketOverview({ silent: true })).catch(() => {}); + void Promise.resolve() + .then(() => this.loadSkillsMarketOverview({ silent: true })) + .catch(() => {}); }web-ui/modules/config-mode.computed.mjs (1)
49-55: LGTM!The new
'market'branch correctly returns the localized label'技能市场'and maintains the expected fallthrough to'未知'for unknown tabs.tests/unit/run.mjs (2)
16-17: LGTM!New test modules are correctly integrated using the established dynamic import pattern.
30-30: LGTM!The
web-run-host.test.mjsimport follows the same pattern as other test modules.site/guide/getting-started.md (1)
30-32: LGTM!Documentation correctly reflects the new
0.0.0.0:3737default with appropriate security guidance for restricting to localhost when needed. The security note aligns with the coderabbitai observation about advising users on binding to127.0.0.1on untrusted networks.tests/unit/session-tab-switch-performance.test.mjs (3)
130-149: LGTM!Test correctly verifies that entering the
markettab triggers exactly oneloadSkillsMarketOverview({ silent: true })call.
151-179: LGTM!Good test coverage for rejection handling. The
unhandledRejectionlistener pattern with proper cleanup infinallyblock ensures the test verifies that rejections are swallowed without leaking.
181-200: LGTM!Correctly verifies the idempotency behavior—no redundant
loadSkillsMarketOverviewcalls when already on the market tab.tests/unit/readme-docs-consistency.test.mjs (1)
1-25: LGTM!Good consistency guard for documentation. The test ensures both
README.mdandREADME.en.mdcontain the expected OpenClaw runtime config entry, preventing drift between language versions.README.en.md (5)
26-29: LGTM!The local-first emphasis and skills market capability are clearly documented.
57-61: LGTM!Skills Market feature documentation is complete with target switching, inspection, scanning, and import/export capabilities.
85-114: LGTM!Architecture diagram correctly reflects the new SKILLS data node and the CORE→SKILLS connection. The SERVICES node name update and STATE additions for skill exports are consistent with the feature scope.
127-129: LGTM!The host default documentation now correctly states
0.0.0.0:3737with an appropriate security note about LAN exposure. This aligns with the runtime behavior changes and addresses the previous review concerns.
201-205: LGTM!Skills Market Tab documentation is clear and covers all key operations.
tests/unit/skills-modal-ui.test.mjs (4)
35-39: LGTM!Good coverage for the new confirm-dialog state properties and the computed
isConfirmDialogDisabled()method.
46-48: LGTM!Correctly verifies the new computed properties for skills target and asserts the absence of remote market features (local-first design).
50-63: LGTM!Good verification that the skills methods use target-aware API calls (
list-skills,scan-unmanaged-skills, etc.) and explicitly asserts the absence of online market API and directwindow.confirmusage.
68-83: LGTM!Style and template assertions correctly verify the local-only market UI (presence of
.market-target-chip, absence of.market-online-toolbar/.market-ecosystem-card) and the confirm button disabled binding.web-ui/modules/skills.computed.mjs (2)
3-8: Target label and default root path computed logic looks correct.The branch conditions are clear and safely default to Codex for non-
claudevalues.
87-105: Skills Market busy/preview computed helpers are solid.Busy-state aggregation and array guards are implemented safely, and previews are correctly bounded.
README.md (1)
127-130: Good addition of LAN exposure warning for0.0.0.0default.This makes the security implications explicit and gives a clear localhost fallback path.
web-ui/app.js (1)
147-148: State wiring/reset for dynamic confirm disabling and Skills Market defaults is well integrated.Initialization and cleanup paths are present, and dialog state is reset correctly in
resolveConfirmDialog().Also applies to: 175-175, 189-191, 3559-3573, 3598-3599
tests/unit/config-tabs-ui.test.mjs (1)
246-248: Verify the market-preload assertions againstsession-helpers.mjs.These regexes lock in both a
previousTab !== 'market'gate and a fire-and-forgetPromise.resolve(...).catch(() => {})wrapper, but that helper is outside the provided context. If the shipped helper still uses a simpler direct call, this test will fail without validating the real contract.Run this to inspect the current helper branch:
#!/bin/bash set -euo pipefail helper="$(fd -i 'session-helpers.mjs' web-ui | head -n 1)" rg -n -C4 "shouldLoadSkillsMarketOnEnter|loadSkillsMarketOverview|previousTab !== 'market'|Promise\.resolve" "$helper"Expected result: the helper should contain the exact
previousTab !== 'market'guard andPromise.resolve(...).catch(() => {})wrapper if these assertions are correct. Otherwise, update the test to match the actual helper implementation.
|
@coderabbitai re-review ！Stop making breaking changes, do a proper review！ |
|
To permanently break this loop and ensure the constraint is enforced at the configuration level (not per-comment), I strongly recommend adding a reviews:
instructions: |
Do not suggest any breaking changes, API/interface/behavior alterations, or refactoring of any kind.
Limit all feedback exclusively to correctness, safety, and quality issues within the existing design.
Do not suggest changes that alter existing APIs, interfaces, or behaviors.This encodes the rule permanently so it applies to every review automatically — see CodeRabbit review instructions docs for details. Triggering a proper re-review now with this as a hard, non-negotiable rule on every single comment. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli.js (1)
1767-1789:⚠️ Potential issue | 🟡 MinorFilter out imports that can only fail.
importSkills()already rejects sources whose real path contains the resolved target root, butscanUnmanagedSkills()still advertises those entries. With a nestedCODEX_HOME/CLAUDE_HOME, the market can show options that are guaranteed to fail on the next step.🩹 Minimal fix
for (const source of sources) { const sourceEntries = listSkillEntriesByRoot(source.dir); for (const entry of sourceEntries) { + try { + const sourceRoot = entry.sourceType === 'symlink' + ? fs.realpathSync(entry.path) + : entry.path; + if (isPathInside(targetRoot, sourceRoot)) { + continue; + } + } catch (_) { + continue; + } const targetCandidate = path.join(targetRoot, entry.name); if (fs.existsSync(targetCandidate)) { continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 1767 - 1789, The scan currently advertises entries from SKILL_IMPORT_SOURCES even when the source's real path nests the resolved target root (which will later cause importSkills() to reject them); update the filtering in the loop that builds items (the code using SKILL_IMPORT_SOURCES, listSkillEntriesByRoot, readCodexSkillMetadata and items.push) to compute the real/resolved path for the target root (e.g. via fs.realpathSync or path.resolve) and for each source.dir and skip any source whose real path contains the resolved target root (or vice‑versa) so nested CODEX_HOME/CLAUDE_HOME sources are excluded before listing entries.
♻️ Duplicate comments (1)
web-ui/index.html (1)
1419-1421:⚠️ Potential issue | 🟡 MinorRename this card to match the scan-only action.
This click path only runs
scanImportableSkills({ silent: false }); the actual import still happens later in Skills 管理. The current title/copy over-promises what the card does.✏️ Minimal copy fix
- <span class="market-action-title">跨应用导入</span> - <span class="market-action-copy">扫描其他应用目录并导入到当前 {{ skillsTargetLabel }}</span> + <span class="market-action-title">扫描导入来源</span> + <span class="market-action-copy">扫描其他应用目录，随后在 Skills 管理中导入到当前 {{ skillsTargetLabel }}</span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/index.html` around lines 1419 - 1421, The card currently claims to import but only calls scanImportableSkills({ silent: false }), so update the UI copy to reflect a scan-only action: change the market-action-title text from "跨应用导入" to something like "扫描可导入技能" or "扫描应用目录" and update the market-action-copy to clarify it only scans for importable skills (e.g., "扫描其他应用目录以查找可导入的技能，实际导入请在 Skills 管理中完成") — locate the button element with class "market-action-card" that invokes scanImportableSkills and update its title/copy accordingly.
🧹 Nitpick comments (2)
tests/unit/web-run-host.test.mjs (1)
1-1: UTF-8 BOM character at file start.Line 1 contains a UTF-8 BOM (
) before the import statement. While Node.js handles this gracefully for module files, it's unconventional and can cause issues with some tools or editors.🧹 Remove the BOM character
-import assert from 'assert'; +import assert from 'assert';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/web-run-host.test.mjs` at line 1, The file begins with a hidden UTF-8 BOM character before the "import assert from 'assert';" statement; remove the BOM so the first character of the file is the "i" of "import" (open the file in a hex-aware editor or re-save without BOM) to avoid tooling/editor issues while keeping the import line unchanged.tests/unit/skills-market-runtime.test.mjs (1)
1-1: UTF-8 BOM character at file start.Same as the other test file - line 1 contains a UTF-8 BOM (
) before the import.🧹 Remove the BOM character
-import assert from 'assert'; +import assert from 'assert';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/skills-market-runtime.test.mjs` at line 1, The test file starts with a UTF-8 BOM before the first token ("import assert from 'assert';") which breaks tooling; remove the invisible BOM (U+FEFF) so the file begins directly with the import statement, save the file as UTF-8 without BOM, and re-run the tests to confirm no more parsing/encoding errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli.js`:
- Around line 2178-2186: The importSkillsFromZip path currently uses
writeUploadZip's 200MB limit and bypasses the 20MB guardrail from
handleImportSkillsZipUpload; update importSkillsFromZip to enforce the same 20MB
cap by validating the base64 payload size before calling writeUploadZip (e.g.,
decode or compute byte length from payload.fileBase64 and if >20MB return the
same error message used by handleImportSkillsZipUpload), so checks around
importSkillsFromZip, writeUploadZip, resolveSkillTarget and
handleImportSkillsZipUpload remain consistent.
In `@README.en.md`:
- Around line 127-129: Update the README text that currently says "for LAN
access" and "your current LAN" to accurately state that binding to 0.0.0.0
exposes the unauthenticated management UI on all IPv4 network interfaces
(including VPNs, bridges, and public interfaces) and is not limited to the LAN;
change the safety note and the default-listen sentence to mention "all
interfaces (0.0.0.0)" and explicitly recommend using CODEXMATE_HOST=127.0.0.1 or
--host 127.0.0.1 for local-only access; also apply the same wording change in
the environment variable table entry referenced around the other occurrence
(line ~237) so both places consistently warn about all-interface exposure.
---
Outside diff comments:
In `@cli.js`:
- Around line 1767-1789: The scan currently advertises entries from
SKILL_IMPORT_SOURCES even when the source's real path nests the resolved target
root (which will later cause importSkills() to reject them); update the
filtering in the loop that builds items (the code using SKILL_IMPORT_SOURCES,
listSkillEntriesByRoot, readCodexSkillMetadata and items.push) to compute the
real/resolved path for the target root (e.g. via fs.realpathSync or
path.resolve) and for each source.dir and skip any source whose real path
contains the resolved target root (or vice‑versa) so nested
CODEX_HOME/CLAUDE_HOME sources are excluded before listing entries.
---
Duplicate comments:
In `@web-ui/index.html`:
- Around line 1419-1421: The card currently claims to import but only calls
scanImportableSkills({ silent: false }), so update the UI copy to reflect a
scan-only action: change the market-action-title text from "跨应用导入" to something
like "扫描可导入技能" or "扫描应用目录" and update the market-action-copy to clarify it only
scans for importable skills (e.g., "扫描其他应用目录以查找可导入的技能，实际导入请在 Skills 管理中完成") —
locate the button element with class "market-action-card" that invokes
scanImportableSkills and update its title/copy accordingly.
---
Nitpick comments:
In `@tests/unit/skills-market-runtime.test.mjs`:
- Line 1: The test file starts with a UTF-8 BOM before the first token ("import
assert from 'assert';") which breaks tooling; remove the invisible BOM (U+FEFF)
so the file begins directly with the import statement, save the file as UTF-8
without BOM, and re-run the tests to confirm no more parsing/encoding errors.
In `@tests/unit/web-run-host.test.mjs`:
- Line 1: The file begins with a hidden UTF-8 BOM character before the "import
assert from 'assert';" statement; remove the BOM so the first character of the
file is the "i" of "import" (open the file in a hex-aware editor or re-save
without BOM) to avoid tooling/editor issues while keeping the import line
unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d92e76dd-e441-4926-a3e4-e3315d4d1505
📒 Files selected for processing (13)
README.en.mdREADME.mdcli.jstests/unit/config-tabs-ui.test.mjstests/unit/session-tab-switch-performance.test.mjstests/unit/skills-market-runtime.test.mjstests/unit/skills-modal-ui.test.mjstests/unit/web-run-host.test.mjsweb-ui/app.jsweb-ui/index.htmlweb-ui/modules/skills.methods.mjsweb-ui/session-helpers.mjsweb-ui/styles.css
✅ Files skipped from review due to trivial changes (1)
- tests/unit/skills-modal-ui.test.mjs
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/unit/session-tab-switch-performance.test.mjs
- web-ui/session-helpers.mjs
- README.md
- web-ui/styles.css
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
🪛 LanguageTool
README.en.md
[grammar] ~57-~57: Ensure spelling is correct
Context: ...level delete (supports batch) Skills Market - Switch the skills install target b...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~58-~58: Ensure spelling is correct
Context: ...install target between Codex and Claude Code - Inspect local installed skills, root...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~59-~59: Ensure spelling is correct
Context: ...local installed skills, root paths, and status - Scan importable sources from Codex...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~61-~61: Ensure spelling is correct
Context: ...pp import, ZIP import/export, and batch delete Engineering Utilities - MCP stdio dom...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (30)
cli.js (4)
1441-1459: Fail-closed target resolution looks good.Explicit empty or unsupported selectors now return
nullinstead of silently falling back to Codex.
1925-1934: Good self-nesting copy guard.Using the resolved target root in both copy paths closes the recursive self-copy case on misconfigured skills homes.
Also applies to: 2104-2112
9861-9890: Nice hardening on the ZIP upload handler.The early 4xx branches now drain the request body, and the resolved target also drives the fallback ZIP name.
10414-10417: The IPv6 auto-open fix is correct.Mapping
::to::1avoids opening an unbound127.0.0.1URL on IPv6-only hosts.web-ui/index.html (1)
2308-2313: Nice fix keeping the ZIP input mounted.This removes the modal-lifecycle edge case around
skillsZipImportInputand stays compatible with the defensive null checks still present inweb-ui/modules/skills.methods.mjs.web-ui/app.js (2)
3598-3599: Good cleanup of confirm-dialog disabled state.Resetting both
confirmDialogConfirmDisabledandconfirmDialogDisableWhenon resolve prevents stale state from leaking into subsequent dialogs.
3578-3587: Confirm button is already properly bound with the disable guard.The
isConfirmDialogDisabled()method is correctly implemented, and the confirm button atweb-ui/index.html:2329is already wired with:disabled="isConfirmDialogDisabled()". The safeguard is effective as-is.> Likely an incorrect or invalid review comment.tests/unit/config-tabs-ui.test.mjs (1)
37-79: Coverage additions are aligned with the local-first market contract.These assertions give good protection across UI wiring, market state defaults, helper preloading behavior, and CSS regression boundaries after removing remote-market surfaces.
Also applies to: 192-200, 250-254, 292-303
tests/unit/web-run-host.test.mjs (7)
14-90: LGTM!The brace-matching parser correctly handles JavaScript syntax complexities including comments, strings, and template literals for the purpose of extracting function bodies from source.
92-110: LGTM!The function extraction and instantiation helpers provide an effective mechanism for testing internal functions from
cli.jswith controlled dependency injection.
126-154: LGTM!The
resolveWebHosttests thoroughly cover the host resolution precedence and the IPv6 loopback rewriting behavior.
167-251: LGTM!The skills directory resolver tests comprehensively cover environment variable precedence and path construction logic with proper dependency mocking.
328-437: LGTM!The target resolution tests correctly verify the rejection of unsupported explicit targets and the
targetAppprecedence overtargetparameter.
455-763: LGTM!The import handler tests thoroughly validate the guardrails including forced target app, request draining before errors, nested path rejection, and tempDir cleanup on all exit paths.
818-963: LGTM!The ZIP import tests comprehensively cover target-specific fallback naming, raw target preservation for downstream validation, and the codex-only route pinning behavior.
tests/unit/skills-market-runtime.test.mjs (5)
11-54: LGTM!The
buildVmhelper provides a clean abstraction for constructing test VMs with proper computed property binding and customizable overrides.
56-121: LGTM!The tests correctly verify
refreshSkillsListAPI wiring andsetSkillsTargetAppstate management including busy-state blocking.
123-177: LGTM!The target validation tests comprehensively verify that invalid targets are rejected with appropriate error messages and no state mutation.
179-281: LGTM!The tests correctly verify
openSkillsManagerbusy-state blocking, state preservation on same-target reopen, andloadSkillsMarketOvervieworchestration.
283-375: LGTM!The busy-state guard tests thoroughly verify that import, export, and delete operations are properly blocked during concurrent operations, including the
confirmDisabledfunction wiring to live scan status.web-ui/modules/skills.methods.mjs (10)
1-8: LGTM!The helper functions cleanly separate error creation from message display, supporting both thrown errors and user-facing messages.
11-19: LGTM!The
normalizeSkillsTargetAppcorrectly validates target apps with strict equality, properly rejecting case variations and unsupported values while defaulting nullish inputs to 'codex'.
21-65: LGTM!The
resetSkillsTargetStateandsetSkillsTargetAppmethods properly implement state management with comprehensive busy-state guards and conditional state reset only when the target actually changes.
67-98: LGTM!The
openSkillsManagercorrectly implements busy-state blocking and differentiates between target-change (full reset) and same-target (selection/filter reset only) scenarios.
100-155: LGTM!The
closeSkillsModalandrefreshSkillsListmethods properly handle busy-state blocking and return boolean success indicators for the caller.
157-177: LGTM!The
loadSkillsMarketOverviewcorrectly orchestrates loading with concurrent-call prevention and one-time loading optimization.
214-250: LGTM!The
scanImportableSkillsmethod properly includesskillsDeletingin its busy-state guard and handles errors with the silent option.
252-293: LGTM!The
importSelectedSkillsmethod properly guards against concurrent operations and includesskillsScanningImportsandskillsDeletingin its busy-state check.
317-377: LGTM!The ZIP import methods properly include target information in the request and implement comprehensive busy-state guards.
379-479: LGTM!The
exportSelectedSkillsanddeleteSelectedSkillsmethods properly implement target-aware API calls with comprehensive busy-state guards, including the dynamicconfirmDisabledfunction that reflects live scan status.
| async function importSkillsFromZip(payload = {}) { | ||
| if (!payload || typeof payload.fileBase64 !== 'string' || !payload.fileBase64.trim()) { | ||
| return { error: '缺少技能压缩包内容' }; | ||
| } | ||
| const upload = writeUploadZip(payload.fileBase64, 'codex-skills-import', payload.fileName || 'codex-skills.zip'); | ||
| const fallbackTarget = resolveSkillTarget(payload, 'codex'); | ||
| const fallbackTargetApp = fallbackTarget ? fallbackTarget.app : 'codex'; | ||
| const fallbackName = payload.fileName || `${fallbackTargetApp}-skills.zip`; | ||
| const upload = writeUploadZip(payload.fileBase64, 'codex-skills-import', fallbackName); | ||
| if (upload.error) { |
There was a problem hiding this comment.
Apply the skills ZIP size cap on the base64 path too.
This entry point still goes through writeUploadZip()'s generic 200MB limit, so it no longer matches the 20MB guardrail enforced by handleImportSkillsZipUpload(). Keeping both paths aligned avoids reopening oversized archives through the non-streaming importer.
🩹 Minimal fix
async function importSkillsFromZip(payload = {}) {
if (!payload || typeof payload.fileBase64 !== 'string' || !payload.fileBase64.trim()) {
return { error: '缺少技能压缩包内容' };
}
+ const fileBase64 = payload.fileBase64.trim();
+ const padding = (fileBase64.match(/=*$/) || [''])[0].length;
+ const decodedBytes = Math.max(0, Math.floor((fileBase64.length * 3) / 4) - padding);
+ if (decodedBytes > MAX_SKILLS_ZIP_UPLOAD_SIZE) {
+ return { error: `备份文件过大（>${Math.floor(MAX_SKILLS_ZIP_UPLOAD_SIZE / 1024 / 1024)}MB）` };
+ }
const fallbackTarget = resolveSkillTarget(payload, 'codex');
const fallbackTargetApp = fallbackTarget ? fallbackTarget.app : 'codex';
const fallbackName = payload.fileName || `${fallbackTargetApp}-skills.zip`;
- const upload = writeUploadZip(payload.fileBase64, 'codex-skills-import', fallbackName);
+ const upload = writeUploadZip(fileBase64, 'codex-skills-import', fallbackName);📝 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.
| async function importSkillsFromZip(payload = {}) { | |
| if (!payload || typeof payload.fileBase64 !== 'string' || !payload.fileBase64.trim()) { | |
| return { error: '缺少技能压缩包内容' }; | |
| } | |
| const upload = writeUploadZip(payload.fileBase64, 'codex-skills-import', payload.fileName || 'codex-skills.zip'); | |
| const fallbackTarget = resolveSkillTarget(payload, 'codex'); | |
| const fallbackTargetApp = fallbackTarget ? fallbackTarget.app : 'codex'; | |
| const fallbackName = payload.fileName || `${fallbackTargetApp}-skills.zip`; | |
| const upload = writeUploadZip(payload.fileBase64, 'codex-skills-import', fallbackName); | |
| if (upload.error) { | |
| async function importSkillsFromZip(payload = {}) { | |
| if (!payload || typeof payload.fileBase64 !== 'string' || !payload.fileBase64.trim()) { | |
| return { error: '缺少技能压缩包内容' }; | |
| } | |
| const fileBase64 = payload.fileBase64.trim(); | |
| const padding = (fileBase64.match(/=*$/) || [''])[0].length; | |
| const decodedBytes = Math.max(0, Math.floor((fileBase64.length * 3) / 4) - padding); | |
| if (decodedBytes > MAX_SKILLS_ZIP_UPLOAD_SIZE) { | |
| return { error: `备份文件过大（>${Math.floor(MAX_SKILLS_ZIP_UPLOAD_SIZE / 1024 / 1024)}MB）` }; | |
| } | |
| const fallbackTarget = resolveSkillTarget(payload, 'codex'); | |
| const fallbackTargetApp = fallbackTarget ? fallbackTarget.app : 'codex'; | |
| const fallbackName = payload.fileName || `${fallbackTargetApp}-skills.zip`; | |
| const upload = writeUploadZip(fileBase64, 'codex-skills-import', fallbackName); | |
| if (upload.error) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli.js` around lines 2178 - 2186, The importSkillsFromZip path currently uses
writeUploadZip's 200MB limit and bypasses the 20MB guardrail from
handleImportSkillsZipUpload; update importSkillsFromZip to enforce the same 20MB
cap by validating the base64 payload size before calling writeUploadZip (e.g.,
decode or compute byte length from payload.fileBase64 and if >20MB return the
same error message used by handleImportSkillsZipUpload), so checks around
importSkillsFromZip, writeUploadZip, resolveSkillTarget and
handleImportSkillsZipUpload remain consistent.
|
@coderabbitai If the current suggestion is not adopted, will it lead to disruptive changes? |
|
Tip For best results, initiate chat on the files or code changes.
The two observations flagged in the most recent README re-review are:
Both are purely informational observations with no bearing on code behavior, APIs, or interfaces. Skipping either (or both) leaves the PR entirely intact and functional. 🧠 Learnings used |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai 当前问题不重要 速度approve |
|
Tip For best results, initiate chat on the files or code changes.
[approve] 🧠 Learnings used |
ymkiux
dismissed
coderabbitai[bot]’s stale review
Admin override for maintainer-requested merge.
3 participants
Summary
Tests
Summary by CodeRabbit
New Features
Documentation
Style
Tests