Skip to content

fix: address 14+ verified bugs from CLI audit#422

Open
Kamirus wants to merge 3 commits intomainfrom
fix/cli-audit-bugs
Open

fix: address 14+ verified bugs from CLI audit#422
Kamirus wants to merge 3 commits intomainfrom
fix/cli-audit-bugs

Conversation

@Kamirus
Copy link
Collaborator

@Kamirus Kamirus commented Mar 19, 2026

Summary

Comprehensive audit of the mops CLI uncovered and independently verified 14+ bugs across core commands (install, check, build, test, sync, update, sources, publish, remove, add). All findings were verified against the actual source code before fixing.

High-priority fixes

  • Path splitting on spaces — test, watch/error-checker, and watch/warning-checker used sources().join(" ").split(" ") to pass args to moc, breaking when any package path contains spaces. Switched to sourcesArgs().flat() (matching build.ts and check.ts).
  • compareVersions silently wrong — parseInt on missing version segments produced NaN, causing "1.0" to be treated as equal to "1.0.5". Fixed with parseInt(x) || 0.
  • sync broken with version-pinned deps — Config keys like base@1.0.0 never matched used package names like base. Applied getDepName() normalization.
  • update --lock ignore not working — lock option was not forwarded to intermediate add() calls.
  • getGithubCommit no error handling — Network failures or 404s returned objects without .sha, causing silent downstream failures or @undefined in URLs. Now throws with a descriptive error; update.ts wraps in try/catch to continue with other deps.
  • installFromGithub called process.exit(1) — Changed to return false so callers can handle failures gracefully. Updated installDep and add to propagate the result and exit with code 1.
  • add wrote broken deps to config — When installFromGithub failed, the dependency was still written to mops.toml. Now exits early on failure.

Medium-priority fixes

  • Dead code in build.ts — Unreachable loop after filter() removed. Replaced with cleaner invalid-name detection that reports all invalid names at once (previously silently ignored invalid names).
  • readLockFile crash on corrupted JSON — Added try/catch with actionable error message.
  • sources.ts path resolution — existsSync used relative path that resolved against wrong directory when cwd parameter differed.
  • installDep silent success for malformed deps — Now warns and returns false instead of silently returning true.
  • Integrity check prefix collision — startsWith(packageId) could match pkg@1.0.0-beta against pkg@1.0.0. Fixed with startsWith(packageId + "/").
  • remove missed GitHub transitive deps — dep.repo was not passed to getTransitiveDependenciesOf.

Code quality

  • Renamed puiblishingId typo to publishingId (8 occurrences)
  • Fixed == to === in build.ts
  • Used path.join instead of string concatenation
  • Escaped version string in regex (find-changelog-entry.ts)
  • Avoided in-place array mutation ([...vers].reverse())
  • Removed duplicate error log in vessel.ts (underlying downloadFromGithub already logs)

Documentation

  • Fixed typos: "Initalize" to "Initialize", "conflics" to "conflicts"
  • Removed invalid # comment in JSON example (format docs)
  • Split duplicate moc keys into separate TOML examples (toolchain docs)
  • Documented missing [package] fields: baseDir, readme, dfx

Behavior changes

  • mops build now reports invalid canister names instead of silently ignoring them (previously the names were silently filtered out, which was unintentional — the dead code proved the original intent was to report errors)

Test plan

  • All pre-commit hooks pass (lint-staged, prettier, eslint, TypeScript check, Svelte check)
  • Run existing test suite (npm test in cli/)
  • Manual smoke test: mops install, mops check, mops build, mops test
  • Verify mops sync works correctly with version-pinned dependencies
  • Verify mops update --lock ignore no longer updates the lock file during intermediate steps

Manual testing results

Test suite: 11 suites, 51 tests, 46 snapshots — all passing.

Smoke tests (run against dev entry point via npm run mops):

  • mops install — installs deps from cache/registry, verifies integrity ✓
  • mops check — type-checks canister entrypoints, reports success ✓
  • mops build — builds 2 canisters, validates candid compatibility, respects per-canister args (--incremental-gc, --release) ✓
  • mops build foo — builds a single named canister ✓
  • mops build nonexistent — exits with code 1, reports "not found in mops.toml configuration" (new stricter validation working) ✓
  • mops test — runs Motoko test files with the test library, reports pass/fail ✓

Targeted verifications:

  • mops sync with version-pinned dep ("base@0.11.1" = "0.11.1"):
    • Correctly recognizes pinned dep as "used" when mo:base is imported (no false "unused" report) ✓
    • Correctly detects pinned dep as "unused" when import is removed ✓
    • Correctly detects missing packages not in config ✓
  • mops update --lock ignore:
    • Lock file MD5 identical before and after update (verified with diff) ✓
    • mops.toml was updated from base = "0.11.0" to base = "0.16.0"
    • The --lock ignore option is properly forwarded to inner add() calls ✓

Bug fixes:
- Fix path splitting on spaces in test, watch error-checker, and warning-checker
  (use sourcesArgs().flat() instead of sources().join(" ").split(" "))
- Fix compareVersions silently treating short versions as equal (parseInt || 0)
- Fix sync incorrectly reporting version-pinned deps as missing/unused
- Forward --lock option from update command to add() calls
- Add error handling to getGithubCommit (check res.ok and json.sha)
- Fix installFromGithub using process.exit(1) instead of returning false
- Remove dead unreachable code in build.ts canister name validation
- Add try/catch around JSON.parse in readLockFile with actionable error message
- Fix sources.ts path resolution when cwd differs from process.cwd()
- Warn and return false for deps with no version, repo, or path
- Harden integrity check prefix matching (startsWith(packageId + "/"))
- Pass dep.repo to getTransitiveDependenciesOf in remove.ts

Code quality:
- Rename puiblishingId typo to publishingId in publish.ts
- Fix loose equality (== to ===) in build.ts
- Use path.join instead of string concatenation in resolve-packages.ts and integrity.ts
- Escape version string in find-changelog-entry.ts regex
- Avoid in-place array mutation with [...vers].reverse()

Documentation:
- Fix typos: "Initalize" -> "Initialize", "conflics" -> "conflicts"
- Remove invalid JSON comment in format docs
- Split duplicate moc keys into separate examples in toolchain docs
- Document missing mops.toml [package] fields: baseDir, readme, dfx

Made-with: Cursor
@Kamirus Kamirus requested a review from a team as a code owner March 19, 2026 14:06
- add.ts: check installFromGithub return value before writing to config
- update.ts: wrap getGithubCommit in try/catch to handle API failures gracefully
- vessel.ts: remove duplicate error log (downloadFromGithub already logs)
- CHANGELOG.md: add entries for all user-facing fixes

Made-with: Cursor
@github-actions
Copy link

github-actions bot commented Mar 19, 2026

Benchmark Results

bench/1-buffer-vector-add.bench.mo $({\color{green}-8.22\%})$

Add (second)

Add items one-by-one (second)

Instructions: ${\color{green}-8.22\%}$
Heap: ${\color{gray}0\%}$
Stable Memory: ${\color{gray}0\%}$
Garbage Collection: ${\color{gray}0\%}$

Instructions

10 10000 1000000
Buffer 9_557 $({\color{green}-29.67\%})$ 5_687_594 $({\color{green}-8.33\%})$ 525_783_888 $({\color{green}-8.25\%})$
Vector 13_525 $({\color{red}+18.47\%})$ 4_378_612 $({\color{green}-10.82\%})$ 417_864_498 $({\color{green}-10.74\%})$

Heap

10 10000 1000000
Buffer 272 B $({\color{gray}0\%})$ 272 B $({\color{gray}0\%})$ 272 B $({\color{gray}0\%})$
Vector 272 B $({\color{gray}0\%})$ 272 B $({\color{gray}0\%})$ 272 B $({\color{gray}0\%})$

Garbage Collection

10 10000 1000000
Buffer 1.09 KiB $({\color{gray}0\%})$ 143.28 KiB $({\color{gray}0\%})$ 12.02 MiB $({\color{gray}0\%})$
Vector 1.09 KiB $({\color{gray}0\%})$ 45.65 KiB $({\color{gray}0\%})$ 3.86 MiB $({\color{gray}0\%})$
bench/2-vector-buffer-add.bench.mo $({\color{green}-10.02\%})$

Add

Add items one-by-one

Instructions: ${\color{green}-10.02\%}$
Heap: ${\color{gray}0\%}$
Stable Memory: ${\color{gray}0\%}$
Garbage Collection: ${\color{gray}0\%}$

Instructions

10 10000 1000000
Vector 13_525 $({\color{green}-12.27\%})$ 4_378_966 $({\color{green}-10.82\%})$ 417_886_092 $({\color{green}-10.74\%})$
Buffer 9_557 $({\color{green}-9.75\%})$ 5_686_886 $({\color{green}-8.33\%})$ 525_781_056 $({\color{green}-8.25\%})$

Heap

10 10000 1000000
Vector 272 B $({\color{gray}0\%})$ 272 B $({\color{gray}0\%})$ 272 B $({\color{gray}0\%})$
Buffer 272 B $({\color{gray}0\%})$ 272 B $({\color{gray}0\%})$ 272 B $({\color{gray}0\%})$

Garbage Collection

10 10000 1000000
Vector 1.09 KiB $({\color{gray}0\%})$ 45.65 KiB $({\color{gray}0\%})$ 3.86 MiB $({\color{gray}0\%})$
Buffer 1.09 KiB $({\color{gray}0\%})$ 143.28 KiB $({\color{gray}0\%})$ 12.02 MiB $({\color{gray}0\%})$
bench/array.bench.mo $({\color{green}-17.47\%})$

Array

arr arr

Instructions: ${\color{green}-17.47\%}$
Heap: ${\color{gray}0\%}$
Stable Memory: ${\color{gray}0\%}$
Garbage Collection: ${\color{gray}0\%}$

Instructions

100k x1 reset1 100k x3 reset2 100k x4 reset3
Array 13_502_096 $({\color{green}-10.00\%})$ 3_335 $({\color{green}-25.14\%})$ 27_003_270 $({\color{green}-10.00\%})$ 3_809 $({\color{green}-24.92\%})$ 54_004_127 $({\color{green}-10.00\%})$ 4_283 $({\color{green}-24.74\%})$

Heap

100k x1 reset1 100k x3 reset2 100k x4 reset3
Array 390.9 KiB $({\color{gray}0\%})$ -390.37 KiB $({\color{gray}0\%})$ 390.9 KiB $({\color{gray}0\%})$ -390.37 KiB $({\color{gray}0\%})$ 390.9 KiB $({\color{gray}0\%})$ -390.37 KiB $({\color{gray}0\%})$

Garbage Collection

100k x1 reset1 100k x3 reset2 100k x4 reset3
Array 360 B $({\color{gray}0\%})$ 390.97 KiB $({\color{gray}0\%})$ 391 KiB $({\color{gray}0\%})$ 390.97 KiB $({\color{gray}0\%})$ 1.14 MiB $({\color{gray}0\%})$ 390.97 KiB $({\color{gray}0\%})$
No previous results found "/home/runner/work/mops/mops/.bench/prng.bench.json"
bench/prng.bench.mo $({\color{gray}0\%})$

Prng

Benchmark N next calls for different PRNGs

Instructions: ${\color{gray}0\%}$
Heap: ${\color{gray}0\%}$
Stable Memory: ${\color{gray}0\%}$
Garbage Collection: ${\color{gray}0\%}$

Instructions

10 100 1000 10000
Seiran128 1_694 15_194 150_194 1_500_194
SFC64 2_802 28_962 288_557 2_882_655
SFC32 2_383 23_825 237_026 2_379_333

Heap

10 100 1000 10000
Seiran128 272 B 272 B 272 B 272 B
SFC64 308 B 272 B 272 B 272 B
SFC32 280 B 280 B 272 B 272 B

Garbage Collection

10 100 1000 10000
Seiran128 296 B 296 B 296 B 296 B
SFC64 536 B 4.98 KiB 47.16 KiB 469.04 KiB
SFC32 376 B 1.78 KiB 15.39 KiB 156.11 KiB
bench/removeLast.bench.mo $({\color{green}-10.90\%})$

Remove items using removeLast

Vector and buffer are initialized with 100k items and then 70k items are removed one-by-one.

Instructions: ${\color{green}-10.90\%}$
Heap: ${\color{gray}0\%}$
Stable Memory: ${\color{gray}0\%}$
Garbage Collection: ${\color{gray}0\%}$

Instructions

remove 70k
Vector 27_707_716 $({\color{green}-13.98\%})$
Buffer 29_236_977 $({\color{green}-7.82\%})$

Heap

remove 70k
Vector -136.8 KiB $({\color{gray}0\%})$
Buffer -269.76 KiB $({\color{gray}0\%})$

Garbage Collection

remove 70k
Vector 139.45 KiB $({\color{gray}0\%})$
Buffer 540.43 KiB $({\color{gray}0\%})$
bench/stable-memory.bench.mo $({\color{green}-134.03\%})$

Stable Memory and Region

Grow Region and store blobs in it

Instructions: ${\color{green}-66.97\%}$
Heap: ${\color{green}-6.10\%}$
Stable Memory: ${\color{gray}0\%}$
Garbage Collection: ${\color{green}-60.97\%}$

Instructions

Region (fill 1/100) Region (fill 1/50) StableMemory
10 pages 2_627_029 $({\color{green}-95.97\%})$ 10_496_318 $({\color{green}-95.97\%})$ 2_693 $({\color{green}-5.87\%})$
100 pages 52_466_721 $({\color{green}-95.97\%})$ 104_914_650 $({\color{green}-95.97\%})$ 2_698 $({\color{green}-8.67\%})$
256 pages 134_274_022 $({\color{green}-95.97\%})$ 268_575_191 $({\color{green}-95.97\%})$ 3_246 $({\color{green}-12.37\%})$

Heap

Region (fill 1/100) Region (fill 1/50) StableMemory
10 pages 272 B $({\color{green}-8.11\%})$ 272 B $({\color{gray}0\%})$ 276 B $({\color{gray}0\%})$
100 pages 272 B $({\color{green}-11.69\%})$ 272 B $({\color{green}-11.69\%})$ 272 B $({\color{gray}0\%})$
256 pages 272 B $({\color{green}-11.69\%})$ 272 B $({\color{green}-11.69\%})$ 276 B $({\color{gray}0\%})$

Garbage Collection

Region (fill 1/100) Region (fill 1/50) StableMemory
10 pages 208.34 KiB $({\color{green}-91.44\%})$ 832.38 KiB $({\color{green}-91.45\%})$ 336 B $({\color{gray}0\%})$
100 pages 4.06 MiB $({\color{green}-91.46\%})$ 8.13 MiB $({\color{green}-91.46\%})$ 340 B $({\color{gray}0\%})$
256 pages 10.4 MiB $({\color{green}-91.46\%})$ 20.8 MiB $({\color{green}-91.46\%})$ 340 B $({\color{gray}0\%})$

Stable Memory

Region (fill 1/100) Region (fill 1/50) StableMemory
10 pages 8 MiB $({\color{gray}0\%})$ 8 MiB $({\color{gray}0\%})$ 8 MiB $({\color{gray}0\%})$
100 pages 8 MiB $({\color{gray}0\%})$ 8 MiB $({\color{gray}0\%})$ 0 B $({\color{gray}0\%})$
256 pages 16 MiB $({\color{gray}0\%})$ 16 MiB $({\color{gray}0\%})$ 16 MiB $({\color{gray}0\%})$

Also add CHANGELOG note about stricter build canister name validation.

Made-with: Cursor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant