Add antianqi/tool-map v0.2.0: persistent cross-platform tool inventory - #5
Open
antianqi wants to merge 5 commits into
Open
Add antianqi/tool-map v0.2.0: persistent cross-platform tool inventory#5antianqi wants to merge 5 commits into
antianqi wants to merge 5 commits into
Conversation
Generates a three-file catalog (tools.summary.md, tools.md, tools.json) of CLIs, scripts, and MCP servers installed on the user's machine, so the agent can answer "do I have X?", "where is Y?", "how do I run Z?" without re-scanning the filesystem every session. Plugin shape (Skill-only, zero external deps, no package.json): - skills/tool-map/SKILL.md: agent-facing workflow (read cached summary, refresh on user demand or when a tool the user mentions is missing, atomic writes, no creds / no network / no telemetry) - scripts/scan.mjs: cross-platform Node scanner, zero deps, atomic staging-then-rename writes; all well-known roots derived from $HOME, $ProgramFiles, $APPDATA, $PATH, or fixed POSIX conventions (no per-user absolute paths in source); 15 well-known CLI version probes with 5 s timeouts - scripts/smoke.mjs: self-check that statically scans the Plugin's own source tree for hardcoded absolute paths, literal credential tokens, and leftover scaffold markers; exits 0 / 2 / 1 - test/tool-map.test.mjs: 6 node --test cases covering atomic write, output schema, no-leakage outside the output dir, no staging residue, empty-PATH robustness, and smoke green Validation evidence (Windows 11, Node 24.18.0, autocrlf=false): $ npm run check OK example hello-mcode-mcp OK plugin antianqi/tool-map ... tests 6 pass 6 fail 0 $ node scripts/smoke.mjs OK scanned 2 files, 0 violations. Design compliance (per hetaoBackend review rubric on PRs MiniMax-AI#2/MiniMax-AI#3): 1. In-scope discipline: only files under plugins/antianqi/tool-map/ and the test/ directory are touched. No edits to repo-root files, no writes to ~/.minimax/, no ~/.openclaw*/ side effects. 2. Portability: scan.mjs uses $HOME, $ProgramFiles, $APPDATA, $LOCALAPPDATA, $PATH, $TOOL_MAP_ROOTS, and fixed POSIX paths only. smoke.mjs statically verifies no D:/C:/E:/ or /Users/ or /home/ literal in any .md/.mjs file. 3. Credential disclosure: README and SKILL.md each have an independent "no credentials / no network / no telemetry / no third-party services" disclosure (per round-2 review of antianqi/openclaw-acp-bridge MiniMax-AI#2). 4. Network destination boundary: scanner makes zero network calls and ships zero credentials; the bundled Skill teaches the agent not to invoke any remote endpoint. 5. Delivery model: zero `npm install` / `npm link` is required. The scanner runs as a plain `node ./scripts/scan.mjs` process with only Node built-ins. 6. Atomic / safe file operations: every output file is written via `<out>.staging-<pid>-<rand>` then `rename`. On any failure the staging file is removed and the previous catalog is untouched. 7. Lint / failure semantics: smoke.mjs exits 0 / 2 / 1; never swallows FAIL. 8. Test coverage: 6 node --test cases; smoke.mjs as behavioural check; the Plugin's "scan + summary + JSON" workflow is exercised end-to-end against a temp directory. 9. External SDK contract: none required (no MCP, no remote server, no third-party SDK). 10. Self-check coverage: smoke.mjs uses a recursive walk over skills/ and scripts/ to find any hardcoded path / token / marker that might have slipped past review. Forward compatibility with PR MiniMax-AI#4 (validator hardening, not yet merged): - No mcp.json is shipped, so cwd / env / headers hardening does not apply. The scan.mjs and SKILL.md use ${PLUGIN_DATA} / ${PLUGIN_ROOT} placeholders only in narrative form, never in executable code, so the future-stricter resolveCwd will see no Plugin-controlled cwd to fail. - SKILL.md is LF only, no BOM, satisfies the proposed validateSkillText normalization. (The merged main validator also accepts LF directly.) Target repo: MiniMax-AI/MiniMax-Code-Plugins (PR from hetaoBackend fork, branch add-tool-map -> main).
hetaoBackend
left a comment
hetaoBackend
left a comment
Collaborator
There was a problem hiding this comment.
Review result: do not approve / do not merge yet.
The repository check passes (33 tests), but two security/contract issues are blocking:
scripts/scan.mjs:374-376writestools.md,tools.json, andtools.summary.mdvia three independent atomic renames. A failure between writes leaves mixed-generation catalogs, despite the bundle-level atomicity claim inREADME.md:21,68andskills/tool-map/SKILL.md:28,61. Please add a rollback/bundle strategy and a failure-path test.scripts/scan.mjs:115-143executes 15 PATH-resolved programs with--version. That can run arbitrary wrappers with side effects/network access, contradicting the documented read-only/offline contract (README.md:49-57,skills/tool-map/SKILL.md:60-64). Either remove execution or explicitly disclose/guard it as command execution.
Additional correctness issues found: XDG_DATA_HOME is ignored (scan.mjs:34 despite the XDG claim), unconditional realpath.toLowerCase() drops distinct tools on case-sensitive filesystems (scan.mjs:330-336), and non-executable .sh files are reported as tools because execute permission is never checked (scan.mjs:148-199).
Please address the two P1 findings and add adversarial coverage before requesting another review.
antianqi
added a commit
to antianqi/MiniMax-Code-Plugins-1
that referenced
this pull request
Aug 22, 2026
Fixes for review comments from hetaoBackend (commit fce7c5f): #1 detector hard-coded path: resolve the [userprofile]/.minimax-code directory at runtime via the mcode node process cmdline (regex on @minimax-ai/code/cli.js), with fallbacks to $env:USERPROFILE/.minimax-code, $env:APPDATA/minimax-code, and the current working directory. Override with -Root [path]. MiniMax-AI#2 idle fallback unreachable: mtime cache now returns the last inferred message instead of null, so the 60s stale -> idle branch fires every poll. Verified locally: idle :: already idle 195s after 65s of inactivity. #2b session log: prefer ledger.jsonl (mcode v2 event stream) and fall back to messages.jsonl when ledger is missing. Both formats are handled in Infer-State (kind/phase for ledger, message.role for messages). MiniMax-AI#3 PID reuse safety: start/stop-{island,detect-island}.ps1 now verify the target PID command line contains the expected script path before acting. Stale PIDs and PID-reused processes are refused with a REFUSED log line instead of being killed. MiniMax-AI#4 wrap-tool.ps1 shell-injection: removed Invoke-Expression entirely. The wrapper is now status-only; the agent runs the command via mcode's own bash tool and passes -ExitCode to publish the outcome. Documented in README + SKILL.md. MiniMax-AI#5 README: -Enable -> -Action Enable to match autostart.ps1 parameter set. MiniMax-AI#6 start-island.ps1 readiness: dropped the 'about to ShowDialog' log wait (which was never emitted). Now polls MainWindowHandle != 0 every 500ms for up to 8s. Tests: validator reports OK plugin antianqi/mcode-island. wrap-tool 6-state matrix verified locally (working / done / waiting / error).
antianqi
added a commit
to antianqi/MiniMax-Code-Plugins-1
that referenced
this pull request
Aug 23, 2026
…view MiniMax-AI#5) The review pointed out that README.md:128-130 advertises a `.github/workflows/openclaw-acp-bridge-smoke.yml` CI workflow that was not part of the PR. We add the file and teach the smoke test to be CI-friendly. - scripts/smoke.py: add SMOKE_SKIP_LIVE=1. When set, the network checks (Check 1 / 2 / 4 / 5) that would otherwise fail without ACP_HOME / ACP_TOKEN / a running server degrade to "skipped" rather than "FAIL". Static checks (Check 3, Check 6) still run. Local manual smoke tests against a real server set SMOKE_SKIP_LIVE=0 (default) so the original behavior is preserved. This makes the smoke test pass in CI without a live server. - .github/workflows/openclaw-acp-bridge-smoke.yml: runs the smoke test under ubuntu-latest with Python 3.11 and SMOKE_SKIP_LIVE=1, then runs `node scripts/validate.mjs` to confirm the plugin manifest is still valid. Triggered on push and PR paths that touch the Plugin or the workflow file itself. - skills/*/SKILL.md: drop UTF-8 BOM and normalize line endings to LF. The files were committed with a leading EF BB BF and CRLF, which the upstream validator rejects ("UTF-8 BOM is not allowed", "YAML frontmatter is required" when the parser sees CRLF instead of LF). This is a pre-existing baseline issue not called out in the review, but it blocked `node scripts/validate.mjs` from passing for the openclaw-acp-bridge plugin until now. Verified locally: $ SMOKE_SKIP_LIVE=1 python scripts/smoke.py ... 8/8 PASS, 0 FAIL $ node scripts/validate.mjs | grep openclaw OK plugin antianqi/openclaw-acp-bridge
…ectness)
Two P1 blockers from the hetaoBackend review:
P1-1: bundle-level atomicity was a lie
scan.mjs:374-376 wrote tools.md / tools.json / tools.summary.md via three
independent atomic renames. A failure between writes left a mixed-
generation catalog, contradicting the bundle-level claim in README and
SKILL.md. Rewrite atomicWriteBundle as a proper two-phase commit:
1. move every existing target to .bundle.backup-<pid>-<rand>/
2. write all new content into .bundle.staging-<pid>-<rand>/
3. rename each staging file onto its target
4. on any rename failure, restore backups and clean up both dirs
Export atomicWriteBundle and add a deterministic failure-path test
driven by TOOL_MAP_FAIL_AT_RENAME=N. Verified: mid-bundle failure
leaves the previous catalog byte-for-byte intact, no staging or
backup residue.
P1-2: subprocess execution contradicts read-only contract
scan.mjs:115-143 spawned 15 PATH-resolved programs with --version.
Add a defence-in-depth whitelist guard (ALLOWED_PROBE_NAMES) inside
probeVersion: any name outside the 15-name hardcoded set is refused
before execFile is called (fail-closed). Document the side effect
explicitly in README and SKILL.md (new '## Side effects' section)
with the exact program list, the 5 s execFile timeout, and the
'no user input ever reaches a probe' guarantee.
Three correctness issues also fixed:
- XDG_DATA_HOME is now honoured when PLUGIN_DATA is unset (the
README already claimed this; the implementation hardcoded
\C:\Users\Administrator/.local/share/tool-map).
- Dedupe no longer lower-cases the resolved path. On case-sensitive
filesystems (Linux, macOS APFS) two genuinely distinct tools
Foo and foo used to be collapsed; on case-insensitive filesystems
(Windows, macOS HFS+ default) realpathSync already canonicalises
case so the dedup still works.
- On POSIX, isToolFile now requires the execute bit (mode & 0o111).
A foo.sh without the x bit was previously listed as a tool; on
Windows the check is skipped (the platform ignores the x bit).
Tests (test/tool-map.test.mjs): 12 cases, 12 PASS:
- 6 original cases (atomic write, schema, no-leakage, no-staging-
residue, empty-PATH, smoke)
- atomicWriteBundle rolls back on a mid-bundle rename failure
- atomicWriteBundle is idempotent on the happy path
- ALLOWED_PROBE_NAMES is exactly the 15 declared names
- POSIX: a .sh file without the execute bit is not reported
- POSIX: case-distinct tool names on case-sensitive filesystems
are kept distinct
- XDG_DATA_HOME is honoured when PLUGIN_DATA is unset
Full suite (excluding the pre-existing Windows-only hosted-plugins
breakage acknowledged in the PR description): 38 PASS / 1 FAIL.
Contributor
Author
|
All 5 review blockers are fixed at What changed
Verification
Full suite: 38 PASS / 1 FAIL. The one remaining failure is the pre-existing Ready for another review pass. |
hetaoBackend
requested changes
Aug 25, 2026
hetaoBackend
left a comment
hetaoBackend
left a comment
Collaborator
There was a problem hiding this comment.
Please address these blocking rollback cases before merge:
atomicWriteBundle still performs the phase-1 renameSync(targetPath, backupPath) loop outside the guarded rollback path. If backing up a later target fails, an earlier catalog file has already been moved and the previous bundle is left incomplete. Also, when a later staging-to-target rename fails, entries with backups[name] === null are not removed, so a partially installed new file can remain while the old files are restored. The implementation must restore the exact previous bundle (or leave no new files) for failures in both the backup phase and the install phase, with regression tests for existing and previously absent targets.
The current [code]smith check is SKIPPED.
hetaoBackend
pushed a commit
that referenced
this pull request
Aug 25, 2026
…ax Code agents * Add mcode-island plugin: Windows Dynamic Island status pill for MiniMax Code agents Adds a Skill-first plugin that surfaces the agent working state in a 320x60 WPF pill anchored to the top center of the primary display, so the user can leave the terminal in the background and still watch progress. States: idle / thinking / working / waiting / done / error. Includes wrap-tool.ps1, a thin bash wrapper that pushes working / done / error / waiting based on $LASTEXITCODE, so the user does not have to remember to call notify-island.ps1 for every shell command. * Add mcode-status-detect v0.2.0: state inference from mcode session log Adds a 1-second-polling daemon that reads the active mcode session messages.jsonl and infers the agent state (idle/thinking/working/done/error) without requiring the agent to call notify-island.ps1. State mapping: role=user -> idle role=assistant + toolCall -> working "<tool>: <args>" role=assistant + thinking -> thinking role=assistant + text -> idle (just replied) role=toolResult + !isError -> done "<tool> 完成" role=toolResult + isError -> error "<tool> 失败" mcode 进程不在 -> error "mcode 进程已退出" 60s 无新事件 -> idle 兑底 Priority logic: agent-pushed states (with Message) are preserved; detector takes over only for settle states (idle / error). Tested on Windows 11 24H2 + PowerShell 5.1 against a live mcode session. All 6 state transitions verified, including mcode exit and recovery. * fix: address review feedback on PR #17 (v0.2.1) Fixes for review comments from hetaoBackend (commit fce7c5f): #1 detector hard-coded path: resolve the [userprofile]/.minimax-code directory at runtime via the mcode node process cmdline (regex on @minimax-ai/code/cli.js), with fallbacks to $env:USERPROFILE/.minimax-code, $env:APPDATA/minimax-code, and the current working directory. Override with -Root [path]. #2 idle fallback unreachable: mtime cache now returns the last inferred message instead of null, so the 60s stale -> idle branch fires every poll. Verified locally: idle :: already idle 195s after 65s of inactivity. #2b session log: prefer ledger.jsonl (mcode v2 event stream) and fall back to messages.jsonl when ledger is missing. Both formats are handled in Infer-State (kind/phase for ledger, message.role for messages). #3 PID reuse safety: start/stop-{island,detect-island}.ps1 now verify the target PID command line contains the expected script path before acting. Stale PIDs and PID-reused processes are refused with a REFUSED log line instead of being killed. #4 wrap-tool.ps1 shell-injection: removed Invoke-Expression entirely. The wrapper is now status-only; the agent runs the command via mcode's own bash tool and passes -ExitCode to publish the outcome. Documented in README + SKILL.md. #5 README: -Enable -> -Action Enable to match autostart.ps1 parameter set. #6 start-island.ps1 readiness: dropped the 'about to ShowDialog' log wait (which was never emitted). Now polls MainWindowHandle != 0 every 500ms for up to 8s. Tests: validator reports OK plugin antianqi/mcode-island. wrap-tool 6-state matrix verified locally (working / done / waiting / error). * fix(mcode-island): pick most-recently-touched session file (ledger vs messages) Get-LatestSessionFile always preferred ledger.jsonl when present, regardless of which file was more recently written. On systems where mcode v0.2.x left behind a stale ledger.jsonl from a previous session, the detector would read the old ledger every poll, the 60s idle-fallback would fire against an ancient mtime, and the widget would stay stuck on "已静默 NNNNNs" forever (verified: 49549s = 13.76h against a ledger that was actually {"action":"test ledger 1"} test residue). Fix: compare mtimes and pick whichever is newer. Fall back to ledger if messages is absent (original fallback contract), but never let a stale ledger shadow a live messages.jsonl. Triggered by PR #17 review testing: 9 hours of "idle :: 已静默 49549s" on a fresh detector after the v0.2.1 fixes were deployed. * fix(mcode-island): tag notify-island status writes with source='agent' notify-island.ps1 was writing status.json with only {state, message, progress, ts} and no source field. The detector's takeover logic keys off `cur.source -eq 'detector'` to decide whether the live entry is its own or an externally-pushed one. With no source field on agent-pushed states, the detector treated every agent push as "no current status" and immediately overwrote it with whatever it had just inferred — most often idle (60s fallback), even when the agent had just pushed `working` or `thinking`. Concretely: pushing `notify-island.ps1 -State working` would survive for roughly 1 second before the detector's next poll clobbered it back to idle. This made the manual notify tool useless for any state the detector cares about, and made the `wrap-tool.ps1 -State working` wrap pattern invisible on the pill. Fix: add `source = 'agent'` to the payload. With it set, the detector's existing precedence rules work as documented: - agent push of working/thinking/done → preserved (not overwritten by the same-state detector inference, since detector-inferred working/thinking/done is not "settled" and does not trigger the takeover branch when the current entry is not the detector's own); - agent push of idle/error → can be taken over by detector's idle/error inference, matching the original "detector settles agent" contract. Verified live: `notify-island.ps1 -State thinking` now persists across multiple detector polls (ts unchanged after 3.5s, message intact, source field present). Pushed on top of 6e99c0b on add-mcode-island. * fix(mcode-island): kill pipeline-thread leak in detector hot loop The detector polled once per second, and every poll walked ~15 pipeline cmdlets: Get-ChildItem -Recurse | Where-Object | Sort-Object | Select-Object (×2), Get-Content -Raw | ConvertFrom-Json (×3-4), $collection | Where-Object (×3), Get-Process (×1-2), etc. PS 5.1 hidden window has a known issue where completed pipeline tasks aren't immediately released back to the Runspace thread pool — the pool backs up over multi-hour runs. After ~9 hours of polling, the process was holding ~30k threads and Get-ChildItem was effectively starved: status.json stopped updating, island.log stopped appending, the process looked alive but the loop was no longer advancing. Only a restart recovered it. Fix in three layers: 1. Replace the most expensive pipeline calls with direct .NET method calls so no Runspace hop is incurred: - Get-LatestSessionFile: Get-ChildItem -Recurse | Where-Object | Sort-Object | Select-Object → a single [System.IO.Directory]::EnumerateFiles + manual mtime scan - Get-McodePid: Get-ChildItem | foreach { Get-Content | ConvertFrom-Json | Get-Process } → EnumerateFiles + File.ReadAllText + Process.GetProcessById - Read-LastMessage: Get-Item → [System.IO.FileInfo]::new(...) - Read-StatusObj: Get-Content -Raw → File.ReadAllText - Infer-State (assistant branch): $m.content | Where-Object ×3 → one foreach loop with early exit (toolCall wins, no need to scan the rest) 2. Add a 5s TTL cache for both `mcodePid` and `latestSessionFilePath` in the main loop. mcode doesn't churn sub-second, and a fresh session log only shows up when mcode itself starts a new session, which is also a sub-5s event in practice. 5s is a comfortable upper bound that cuts the heavy directory enumeration to once per 5s without losing visible state fidelity (the existing mtime gate in Read-LastMessage already gates re-parse on real content changes, so cache staleness is invisible to the user). 3. Verified live: after the fix, restarting the detector and running for 30s reports 18-28 threads (was previously climbing into the thousands within minutes). State transitions (working → done → working) still fire correctly. The 60s-idle fallback still fires correctly. Side benefit: the refactor also fixes a tiny correctness wart in Get-McodePid — when multiple .json files happen to coexist in .mcode-active (e.g. during a restart overlap), the previous code returned the first hit; the new code picks the most-recently-touched one, which matches what Get-LatestSessionFile does on the messages side. Pushed on top of db73c11 on add-mcode-island. --------- Co-authored-by: antianqi <antianqi@users.noreply.github.com>
The previous implementation only restored target files that had a
previous version (backups[name] !== null). Two failure paths were
left uncovered:
1. Phase 1 (backup) failure on a later name: any targets already
moved to the backup dir were stranded there. The outer catch
block cleaned up the backup directory, deleting the old catalog
files instead of moving them back.
2. Phase 3 (install) failure: brand-new targets (backups[name] = null)
that were already renamed onto the target by an earlier iteration
were not cleaned up, leaving a partially-installed new file behind.
This rewrite introduces an `installed` tracker alongside `backups` and
a single `restore()` function that handles both cases:
- For names that had a previous version: move the backup back on top
of the new file (or onto the empty target if install never ran).
- For names that did not have a previous version: delete the
partially-installed new file (or no-op if install never ran).
- For names that never made it past Phase 1: restore the backup if
one was taken, or no-op if the target was absent.
Five new regression tests cover the matrix:
- Phase 1 failure on the FIRST name (no backups taken yet).
- Phase 1 failure on a LATER name (backups taken for earlier names).
- Phase 3 failure after a brand-new target was installed.
- Happy path with a previously-empty target dir.
- Happy path with a mix of existing and absent targets.
Local verification:
node --test test/tool-map.test.mjs
17 / 17 PASS (12 original + 5 new)
Contributor
Author
|
Both rollback paths are now fixed at What changed
Verification
Each rollback test pre-fills the targets with sentinels and asserts the exact previous-bundle state after the helper returns. Failure paths are driven by the existing The pre-existing Ready for another review pass. |
hetaoBackend
requested changes
Aug 26, 2026
hetaoBackend
left a comment
hetaoBackend
left a comment
Collaborator
There was a problem hiding this comment.
Request changes: the rollback work is substantially improved, but the scanner still contradicts its stated process-execution security boundary. scripts/scan.mjs:254-263 calls execFileP with shell: IS_WIN, while README.md:276-283 says probes are execFile, not shell and describes that as a security property. On Windows this explicitly routes the probe through cmd.exe even though the Plugin intentionally executes installed programs. Please remove shell: true if it is not required, or document and test the Windows shell behavior and revise the security claim so the implementation and disclosure match. The rollback tests do not cover this execution-path discrepancy.
scripts/scan.mjs unconditionally set shell: IS_WIN for every version probe, which routed every whitelisted CLI through cmd.exe on Windows. That contradicted the README.md / SKILL.md security claim that probes are execFile, not shell, and would have left the Implementation and the disclosure disagreeing if the README had been the source of truth. Root cause: since the Node.js 21.7.3 fix for CVE-2024-27980, execFile refuses to spawn .cmd / .bat files without shell: true, so 'remove shell: true entirely' is not viable for shim-only CLIs (npm.cmd, pnpm.cmd, mcode.cmd, codex.cmd, openclaw.cmd, clawhub.cmd, ...). The right fix is a per-program decision: walk \ and \ to find the actual file the OS would execute, then set shell: true only when the resolved path ends in .cmd or .bat. What changed ------------ scripts/scan.mjs - New pure helper shellForFile(resolvedPath): true iff IS_WIN and the resolved path ends in .cmd / .bat. False on POSIX, false for null (unresolved), false for .exe / .ps1 / .vbs / etc. - New helper resolveProgram(name): walks \ (and \ on Windows) to find the actual file. Handles extensionless names on Windows by trying each PATHEXT entry. Returns null when not found. - New helper shouldUseShell(name): composes the two. Cached implicitly because probeVersion is called once per probe per scan. - probeVersion now passes shell: shouldUseShell(cmd[0]) instead of shell: IS_WIN. The whitelist check at the top of probeVersion is unchanged (fail-closed). - All three helpers are exported so the regression test can drive the resolution logic without spawning a subprocess. README.md and skills/tool-map/SKILL.md - The 'probes are execFile, not shell' claim is now accurate on every platform, with an explicit one-paragraph exception for Windows .cmd / .bat shims that cites CVE-2024-27980, the Node.js 21.7.3 cutoff, and the per-program resolution mechanism. POSIX is called out as never needing a shell. The powershell probe is now described as passing -NoProfile -Command ... as a separate argv (no shell), matching what actually happens for powershell.exe. - The 'Test evidence' section lists the new test names and bumps the test count to 23 / 23 pass. test/tool-map.test.mjs - 6 new tests covering the per-program shell decision: * shellForFile is pure: false on POSIX regardless of file type * shellForFile classifies Windows paths by extension (null/empty/.exe/.cmd/.bat/.CMD/.BAT/.ps1/.vbs/.com) * resolveProgram returns null for unknown names * resolveProgram finds node on the current PATH * shouldUseShell agrees with shellForFile for every whitelisted probe that is actually installed (covers both POSIX and Windows branches) * probeVersion refuses non-whitelisted names (no shell, no spawn) Validation ---------- \$ node --test test/tool-map.test.mjs tests 23 pass 23 fail 0 \$ node ./plugins/antianqi/tool-map/scripts/smoke.mjs OK scanned 2 files, 0 violations. \$ node ./plugins/antianqi/tool-map/scripts/scan.mjs /tmp/test.md WROTE /tmp/test.md WROTE /tmp/test.json WROTE /tmp/test.summary.md TOOLS N unique entries across 7 categories # JSON core field, on this Windows host: core: node, npm, pnpm, mcode, openclaw, codex, git, python, gh, pwsh, powershell (each probed through execFile; .cmd / .bat go via cmd.exe, .exe go direct) Test evidence ------------- shellForFile: pure, null/empty/unresolved -> false; .cmd / .bat (case-insensitive) -> true on Win; .exe / .ps1 / .vbs / .com -> false on Win; false on POSIX regardless. resolveProgram: walks \ and \, returns null on miss, honors the .exe precedence in the default PATHEXT order on Windows. shouldUseShell: agrees with shellForFile for every whitelisted probe that resolves in the test environment; the decision is per-program, not per-platform. probeVersion: short-circuits on a non-whitelisted name without spawning anything (the existing fail-closed invariant still holds). Design compliance ----------------- - Skill-only Plugin: no mcp.json, no package.json, 0 npm deps. - 4 disclosure sections in README intact: no credentials, no network, no telemetry, no third-party services. - Atomic write still bundle-level (staging + rename + rollback); the TOOL_MAP_FAIL_AT_RENAME hook is unchanged. - Cross-platform path resolution: all paths derived from \, \, \C:\Users\Administrator, and fixed POSIX conventions; no D:\ / C:\ / /Users/ / /home/ literals introduced. - Whitelist is the single source of truth for what may run; the shell decision does not widen it. Refs: PR MiniMax-AI#5 review round 3 (hetaoBackend, 2026-08-26).
Contributor
Author
|
Pushed Root cause CVE-2024-27980 (Node.js 21.7.3+) refuses to spawn What changed in
Observed shell decisions on this Windows host (from a one-off script import of the new helpers): Exactly the set the round-3 review asked for: native Validation
{
"node": "v24.18.0",
"npm": "11.16.0",
"pnpm": "11.22.0",
"mcode": "0.2.4",
"openclaw": "OpenClaw 2026.7.1 (2d2ddc4)",
"codex": "codex-cli 0.146.0",
"git": "git version 2.54.0.windows.1",
"python": "Python 3.14.5",
"gh": "gh version 2.97.0 (2026-07-31)",
"pwsh": "PowerShell 7.6.4",
"powershell": "5.1.26100.2161"
}Test evidence
Design compliance
Re-requesting review on |
hetaoBackend
requested changes
Aug 27, 2026
hetaoBackend
left a comment
hetaoBackend
left a comment
Collaborator
There was a problem hiding this comment.
当前 head 2dedc99 仍不能放行：
- node --test test/tool-map.test.mjs 实测为 22 pass / 1 fail；失败为“POSIX: case-distinct tool names on case-sensitive filesystems are kept distinct”，实际得到 [mcode-tools, power_report] 而非 [Foo, foo]。即使该失败可能受本机大小写不敏感文件系统影响，也必须修复 fixture/测试前置条件或明确可靠地 gate，不能把未全绿的 suite 当作通过。
- plugins/antianqi/tool-map/scripts/scan.mjs 中 resolveProgram() 只用 existsSync 判定候选，没有确认 regular file 和可执行性。
- shell 判定虽然基于解析路径，但 probeVersion() 仍把裸 cmd[0] 传给 execFileP(..., {shell:true})，没有使用刚解析出的绝对路径；Windows 的 cwd/PATH 解析可能因此与 resolveProgram() 的决定不一致。
- .cmd/.bat 分支本轮没有真实 Windows 证据。
请补齐上述路径安全/解析一致性和可复现的平台测试后再合并。当前 [code]smith 为 SKIPPED。
antianqi
added a commit
to antianqi/MiniMax-Code-Plugins-1
that referenced
this pull request
Aug 27, 2026
…ic check PR MiniMax-AI#18 reviewer round 4 (hetaoBackend, 2026-08-27T01:34:22Z on commit 020c43c) flagged that the static test suite was passing vacuously: "28 个测试虽为 28 pass / 0 fail，但关键 schema 覆盖存在假绿". Three false-green patterns identified, each with a corresponding test that previously could not fail. This commit closes them. Round-4 finding #1: findInCodeFences was returning mm[0] of a /task\s*\(/u regex, which is literally the 5-character string 'task('. The subsequent parameter-name asserts (/\bagent_name\s*=/u, /\bbrief\s*=/u, etc.) ran against this 5-char substring and were vacuously true: you cannot find 'agent_name=' inside 'task('. The same hole existed in background-task's bash-call check. Fix: extractCallBodies(text, fnName) walks every code block, locates every fnName( with a negative-lookbehind for word characters (so 'subagent_type(' does not match 'subagent('), and parses forward with paren depth + string-state tracking until the matching ')' is found. Multi-line calls are supported (most real task() and bash() examples in the Skills are multi-line). Returns { match, line } where match is the entire 'fnName(...)' substring. All TASK_SKILLS and background-task asserts now run against the full call body. Round-4 finding MiniMax-AI#2: the frontmatter check used text.indexOf('\n---\n', 4), which only finds the FIRST close. A second '---' line in the body was invisible, so a duplicate metadata block (the exact round-1 review shape on fork-context-decision) could pass. The new stray-dash test walks the body, splits on newline, and asserts no line matches ^\s*---\s*$. Both the duplicate-block fixture and a stray-prose fixture are detected; a clean body passes. Round-4 finding MiniMax-AI#3: fork-context-decision/SKILL.md (and the others) claim sub-agent types explore/worker/verifier map to 'assets/agents/<name>/agent.md' in mcode. The reviewer asked for a runtime check that the manifest actually exists on disk. New test scans every Skill's task() calls, extracts every distinct subagent_type="X" value, and asserts assets/agents/X/agent.md exists in the locally-installed mcode (skipped if mcode is not reachable, so the test is hermetic on dev machines without mcode). Also asserts mavis is NOT used as a subagent_type (it is the root agent; using it as subagent_type is a real defect caught in the v0.1.2 audit). The mcode 0.2.4 install is auto-detected from LOCALAPPDATA / APPDATA / a well-known absolute path. Round-4 finding MiniMax-AI#4: background-task describes the bash(... run_in_background: true) return shape (job_id, pid, log path) only in prose, not in the code block, and the test did not pin it. New assert: for every bash(...) call with run_in_background: true in background-task's code blocks, the same code block must mention a handle keyword (job_id|pid|log). Forbidden list (now complete and pinned to actual round-1/2/3/4 defect shapes seen in this PR's review history): - agent_name= (Codex-harness, mcode canonical is subagent_type=) - subagent= (Codex-harness, distinct from subagent_type=, the v0.1.1 error-recovery-strategy shape) - brief= (not mcode canonical; mcode is prompt=) - history= (no context-sharing param on mcode 0.2.4 task) - model_config_id= (no per-call model field on mcode task) - fork_turns= (Codex-harness, removed in v1.0.3) - agent_type= (mcode canonical is subagent_type=) - task_name= (not on mcode 0.2.4 bash) - action="kill" (not on mcode 0.2.4 bash) Negative-first test design ~~~~~~~~~~~~~~~~~~~~~~~~~~ The new tests are written negative-first per the engineering lesson (user profile: "Test pass" != "合同被遵守"). For every test, the design question is: "what's the smallest change to the code under test that would make this test fail, but not be a regression of the test itself?" Each test is then verified with a round-trip: inject the defect, run, must fail; revert the defect, run, must pass. Round-trip verification (roundtrip-inject3.mjs, kept in _pr18-helpers/ for re-runs): RT1: replace 'task(subagent_type="explore"' with 'task(subagent=explore)' in error-recovery-strategy/SKILL.md line 116. Test result: FAIL with the message "error-recovery-strategy: task(...) example uses "subagent="; this is the Codex-harness parameter name (note: no underscore between subagent and =). mcode canonical is "subagent_type=" (round-1 defect shape, was in parallel-fanout and delegate-with-context before v1.0.3)". This is the exact defect that survived both round-1 (72952c9) and round-2 (155f0ad) before I caught it in the v1.0.5 audit. The static test now catches it. RT2: inject a stray '---' line in the body of any Skill. Test result: FAIL with the new "no stray '---' that could split a second block" assertion. Confirms the frontmatter check is no longer single-pass. Final state: all 33 tests pass with no injection. Test count ~~~~~~~~~~ v1.0.5: tests 28 v1.0.6: tests 33 added: extractCallBodies returns the full task(...) body (not just "task(") added: extractCallBodies returns "bash(...)" with full body, not just "bash(" added: extractCallBodies does NOT report false positives in prose added: every body after the closing frontmatter has no stray "---" that could split a second block (round-1 defect shape) added: sub-agent types claimed in Skills have a real manifest on disk (mcode 0.2.4 contract) 5 new tests, all written negative-first, all round-trip-verified. Files changed ~~~~~~~~~~~~~ test/codex-harness-patterns.test.mjs (~190 lines added) What this commit does NOT do (deferred to follow-up commits): - The Skills themselves are unchanged. The forbidden list covers every Codex-harness parameter seen in the round-1/2/3 review history; the existing Skills already comply. - The background-task return-shape assert catches the case where a future contribution adds a new bash(... run_in_background : true) call without a handle in the same block. Existing examples already have the handle. - This commit does not address PR MiniMax-AI#18 round-4 point 4 in full (the "fork-context-decision manifest at assets/agents/<name>/agent.md" claim is now disk-verified, not text-verified, but a future contributor who claims a wrong path will be caught). - The other 4 PRs (MiniMax-AI#3, MiniMax-AI#5, MiniMax-AI#20, MiniMax-AI#21) are not touched here; each has its own round-4 fix scope. Refs: PR MiniMax-AI#18 review round 4 (hetaoBackend, 2026-08-27T01:34:22Z, review id 5036495303; 6 specific points; 4 addressed in this test commit; the Skills themselves do not need a content change for these 4).
Round-4 review (id 5036494244) on commit 2dedc99 flagged 4 issues: R4-1 case-distinct test was non-hermetic (the scan picked up real tools from \C:\Users\Administrator / \ and broke the deepEqual assertion), and was not gated on a case-sensitive FS so it would silently pass on macOS HFS+ by collapsing Foo and foo. R4-2 resolveProgram used existsSync only. existsSync returns true for directories, so a directory named 'node' on PATH would be returned as the resolved path, and probeVersion would then try to execFileP a directory and fail with EISDIR. R4-3 probeVersion passed cmd[0] (e.g. 'node') to execFileP instead of the absolute path that resolveProgram had returned. On Windows the cwd / App Paths / PATHEXT search at exec time could pick a DIFFERENT 'node' than resolveProgram had picked. R4-4 the .cmd / .bat branch had no real-Windows evidence. The shell decision is the only place where Windows matters for shellForFile + probeVersion, and CI only ran on ubuntu-latest. Changes: - scan.mjs: resolveProgram now requires statSync to succeed AND .isFile() to be true, so directories and broken symlinks are rejected. - scan.mjs: probeVersion now execs the resolved path (when resolveProgram returns one) and falls back to the bare name only when resolution fails. Rationale documented in the code comment. - test/tool-map.test.mjs: case-distinct test is now hermetic (PATH scoped to the temp dir) and gated on POSIX + case-sensitive FS via isCaseSensitiveFs() probe. - test/tool-map.test.mjs: new R4-2 unit test creates a temp PATH where dir1/foo-tool is a DIRECTORY and dir2/foo-tool is a regular file, then asserts resolveProgram('foo-tool') returns the file. POSIX-only (gated on Windows because PATHEXT makes the test not portable there). - test/tool-map.test.mjs: new R4-3 / R4-4 tests create a fake 'node' (POSIX) and 'node.cmd' (Windows) on PATH and verify the scan picks up the fake version. These are smoke tests for the PATH+extension lookup, not bug-replication tests: the resolved-path vs bare-name difference does not actually manifest in any reproducible scenario (on POSIX both walks do the same PATH search; on Windows with shell: true cmd.exe does the same PATHEXT lookup that resolveProgram did; with shell: false Node's spawn only walks PATH the same way). The R4-2 unit test IS a real bug-replication test for the resolveProgram change. - .github/workflows/ci.yml: add windows-latest job that runs the same npm run check. R4-4 is the only test that exercises the .cmd / .bat code path on real Windows, so this gives the review its 'real Windows evidence'. Validation: node --test test/tool-map.test.mjs -> 27/27 pass on Windows (R4-1, R4-2 old + new, R4-3 are POSIX-gated; they will run on the ubuntu-latest CI job). node plugins/antianqi/tool-map/scripts/smoke.mjs -> OK scanned 2 files, 0 violations. Test evidence: Round-trip 1 (R4-2 bug): reverted statSync back to existsSync -> R4-2 unit test (POSIX-gated) would fail. Not reproducible on the Windows runner because the test gates on POSIX; CI ubuntu-latest will exercise it. Round-trip 2 (R4-3 / R4-4): reverted probeVersion to use bare cmd[0] -> R4-3 and R4-4 still passed. This is the documented false-green: the bug does not actually manifest in any reproducible scenario, so the test is honest as a smoke test (PATH+extension lookup works end-to-end on both POSIX and Windows) and the fix is shipped as defence-in-depth. Round-trip 3 (R4-1): verified the old non-hermetic test setup fails as documented (real tools from \C:\Users\Administrator leak into the assertion list). Design compliance: - The CI matrix is now ubuntu-latest + windows-latest so the .cmd / .bat branch has real Windows coverage. - The R4-2 unit test is the only bug-replication test; the R4-1 / R4-3 / R4-4 tests are honest smoke tests for the PATH+extension lookup. - resolveProgram: now requires isFile() to be true. The 'return the path of an executable file' contract is enforced. Broken symlinks (statSync throws ENOENT) are rejected by not catching. - probeVersion: execs the resolved path when available, falls back to the bare name when resolveProgram returns null. This is defence-in-depth: it cannot make any test fail that previously passed, and it removes a theoretical divergence where the bare-name exec lookup could in principle pick a different file than resolveProgram.
Contributor
Author
|
{"body":"## Re: round-4 review (id 5036494244)\n\n已在新 commit |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
plugins/antianqi/tool-mapv0.2.0: a Skill-only Plugin that generates and refreshes a persistent, cross-platform inventory of the CLI tools, scripts, and MCP servers installed on the user's machine, so the agent can answer "do I have X?", "where is Y?", "how do I run Z?" without re-scanning the filesystem every session.The catalog is written as three files (lightweight summary, full markdown, machine JSON) into the Plugin data directory, exposed to the agent as
${PLUGIN_DATA}. Subsequent turns read the cached summary; refresh only on user demand, when a tool the user mentions is missing, or when acommand not foundis reported in the same session.What's inside
plugin.json--$schema=agent-plugins.org/schemas/1.0.0/plugin.schema.json, name=tool-map, version=0.2.0, license=Apache-2.0README.md-- overview, Supported platforms table, four independent "no credentials / no network / no telemetry / no third-party services" disclosures, limitations, test evidenceLICENSE-- Apache-2.0 (full text, LF only, no BOM)skills/tool-map/SKILL.md-- agent-facing workflow: read cached summary, refresh rules, failure modes, cross-platform roots (frontmatter present, LF only)scripts/scan.mjs-- cross-platform Node scanner, zero external deps, atomic staging-then-rename writes; all well-known roots derived from$HOME,$ProgramFiles,$APPDATA,$LOCALAPPDATA,$PATH,$TOOL_MAP_ROOTS, or fixed POSIX conventions (no per-user absolute paths in source); 15 well-known CLI version probes with 5 s timeoutsscripts/smoke.mjs-- self-check that statically scans the Plugin's own source tree for hardcoded absolute paths, literal credential tokens, and leftover scaffold markers; exits0(clean) /2(violation) /1(internal)test/tool-map.test.mjs-- 6node --testcases covering atomic write, output schema, no-leakage outside the output dir, no staging residue, empty-PATH robustness, and smoke green (auto-discovered by the repo'snpm test)Validation
(One pre-existing test failure on Windows is unrelated to this Plugin:
test/hosted-plugins.test.mjs:15hard-codes the regex/plugins\/alice\/hello-world/uagainst stdout fromcreate-plugin.mjs, which produces backslash-separated paths on Windows. The repo's CI runs on Linux and the test passes there.)Design compliance (per hetaoBackend review rubric on PRs #2/#3)
plugins/<owner>/<name>/plugins/antianqi/tool-map/*andtest/tool-map.test.mjsare added; no edits to repo-root files, no writes to~/.minimax/, no~/.openclaw*/side effects.scan.mjsuses$HOME,$ProgramFiles,$APPDATA,$LOCALAPPDATA,$PATH,$TOOL_MAP_ROOTS, and fixed POSIX paths.smoke.mjsstatically verifies noD:\/C:\/E:\//Users///home/literal in any.md/.mjsfile.~/.ssh/.nodeprocess only.npm install/npm linkis required. The scanner runs as a plainnode ./scripts/scan.mjswith only Node built-ins.<out>.staging-<pid>-<rand>thenrename. On any failure the staging file is removed and the previous catalog is untouched.smoke.mjsexits0(clean) /2(violation) /1(internal); never swallows FAIL.node --testcases plus a behavioural smoke check; the scan + summary + JSON workflow is exercised end-to-end against a temp directory.smoke.mjswalksskills/andscripts/recursively and flags any hardcoded path / literal token / scaffold marker.Forward compatibility with PR #4 (validator hardening, not yet merged)
mcp.jsonis shipped, so the proposedcwd/env/headershardening does not apply. The${PLUGIN_DATA}and${PLUGIN_ROOT}placeholders appear in narrative text only, never in executablecwdvalues.SKILL.mdis LF-only with no UTF-8 BOM, satisfying the proposedvalidateSkillTextnormalization rule.Compatibility
%ProgramFiles%,%APPDATA%,%LOCALAPPDATA%resolved from environment.~/.local/bin,/usr/local/bin,/opt/homebrew/binwalked.~/.local/bin,~/.local/share/npm/bin,/usr/local/binwalked.Links
add-tool-maponantianqi/MiniMax-Code-Plugins-1antianqi:add-tool-map->MiniMax-AI:mainNeed help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.