Skip to content

fix: resolve code review issues across all 10 packages#40

Merged
lete114 merged 13 commits into
mainfrom
fix/review-batch-1
Jun 20, 2026
Merged

fix: resolve code review issues across all 10 packages#40
lete114 merged 13 commits into
mainfrom
fix/review-batch-1

Conversation

@lete114

@lete114 lete114 commented Jun 20, 2026

Copy link
Copy Markdown
Member

Crypto

  • Replace Math.random() with rngChacha20 CSPRNG in defaultRandomBytes
  • Remove try-catch/Math.random fallback (confirmed working in QuickJS)
  • Set HKDF info domain separator to 'mcbe-mods-crypto-v1'
  • Document HKDF vs PBKDF2 limitation

IPC / Discover / RPC

  • Add configurable maxInflightIds option (default 1000) with oldest-entry eviction
  • Change #sentIds from Set to Map<string,number> with timestamp guard
  • Add input validation for namespace, channel, method name, compressor

Log

  • Add level label to output format
  • Validate constructor options.level before fallback
  • Add child() name validation
  • Wrap debug thunk calls in try-catch

Utils

  • Add guards: splitGroups groupSize<=0, getRandomRangeValue min>max, ms2ticks negative/zero, base64 regex
  • AddXP negative now returns without changes
  • unique with zero/negative length returns empty array
  • Rename Experience.ts → experience.ts for case-sensitive git

Protocol

  • Fix once() handler: wrap in try-catch
  • Add sourceType check before emitting
  • Improve JSDoc and error handling

Bedrock-URL

  • Strip query/hash before baseDir for relative URLs
  • Add Symbol.iterator + entries/keys/values to URLSearchParams

Compress

  • Fix package description, byte-length tests
  • Add @throws JSDoc documentation

Runtime

  • Re-export normalizeServiceType from barrel index

Root

  • Add .gitattributes with LF normalization
  • Set core.ignorecase false for case-sensitive git
  • Fix CI: remove double build, use native pnpm commands
  • Fix release: remove hashFiles always-true condition
  • Add engines field to root and all packages
  • Fix eslint config, README, workspace entries

Test improvements

  • Add ~30 new tests for edge cases and validation guards
  • Clean up scriptEventListeners across all IPC/Discover tests
  • Properly dispose local instances in afterEach hooks
  • Expand runtime test coverage to verify all 23 exports
  • Fix test assertions for new level label format

lete114 added 13 commits June 20, 2026 16:58
@lete114
fix: review batch 1 - input validation and iteration protocol
ad11da9 
- utils(splitGroups): guard groupSize <= 0 (C2)
- utils(getRandomRangeValue): guard min > max (C3)
- utils(ms2ticks): guard negative/zero params (I28)
- bedrock-url: strip query/hash before baseDir for relative URLs (C4)
- bedrock-url(URLSearchParams): add Symbol.iterator + generator methods (C5)
- root: add .gitattributes with LF normalization (C1)
- root: set core.ignorecase false for case-sensitive git (C1)
- bedrock-url(test): add tests for C4/C5 fixes
- utils: rename Experience.ts -> experience.ts for case-sensitive git
@lete114
fix: review batch 2 - memory leak, validation, and output format
2b775c8 
- ipc(ipc): change #sentIds from Set to Map with timestamp guard (I8)
- ipc(chunk): guard chunkSize <= 0 with RangeError (I16)
- ipc(types): update chunkSize doc to UTF-16 code units (I15)
- discover: time-based #sentIds Map with stale cleanup (I17)
- discover: carry serviceType in query param for heartbeat (I18)
- discover: input validation for register/query (I19)
- log(log): add level label to output format (I20)
- log(log): constructor name validation (I21)
- log(log): try-catch debug thunk exceptions (I22)
- log(test): update assertions for new level label format
@lete114
fix: review batch 3 - race condition, CI, mock alignment, test coverage
f4772bd 
- rpc(rpc): fix invoke() ordering - pending.set before post (C9)
- rpc(rpc): method name validation in invoke/handle (I25)
- ci: remove double build, test scripts now just vitest (C10)
- ci: replace @antfu/ni with native pnpm commands (C11)
- release: remove hashFiles always-true condition (C13)
- root+packages: add engines field (I31)
- protocol(setup): align ScriptEventSource mock with real API (I23)
- runtime(test): expand test coverage to 23 exports (I29)
@lete114
fix: review batch 4 - crypto CSPRNG, minor items, lint fixes
80db886 
- crypto(cipher): replace Math.random() with rngChacha20 CSPRNG (C6)
- crypto(cipher): document HKDF vs PBKDF2 limitation (C7)
- compress: fix description, add @throws, byte-length tests (M2-M5)
- discover: TTL >=, error object, dispose events, export (M1-M4,M7-M8)
- ipc: guards for namespace, channel, compressor, disposed (M1-M4)
- ipc: README fix, benchmark sink, new tests (M5-M9)
- log: level validation, declare global, new tests (M1,M3-M5)
- log: README child example fix (M6)
- protocol: once() try-catch, sourceType check, JSDoc (M1-M5)
- rpc: simplify types, error guard, comments, ms2ticks ceil (M1-M6)
- utils: base64 regex, addXP guard, splitGroups guard, unique guard (M2-M3,M5,M7)
- root: typecheck, workspace, CI, gitignore, npmrc, eslint, README (M1-M5,M7,M9-M13,M17)
- lint: fix style/sort/brace issues across packages
@lete114
fix: align @mcbe-mods/compress devDep to workspace:^
d00d1de 
- ipc: catalog: -> workspace:^ for @mcbe-mods/compress
- workspace: remove unused default catalog entry
- ci: restore lts/* node-version, remove timeout-minutes
@lete114
fix: revert unnecessary config changes
ba60a09 
- package.json: restore tsc --noEmit, remove npx prefix
- release.yml: lts/* instead of 22.x
- .gitignore: remove unused entries (.env, *.tsbuildinfo, .pnpm-debug.log)
- .npmrc: delete (auto-install-peers is pnpm default)
@lete114
fix: change hostname format to mDNS-SD style
be66029 
- normalizeServiceType: strip \d+. prefix instead of -N suffix
- hostname format: n.type.discover (e.g. 2.\_svc.\_tcp.discover)
- remove t param (no longer needed, hostname encodes type precisely)
@lete114
refactor: make maxInflightIds configurable, replace throw with eviction
7e9f8cc
@lete114
refactor: make maxInflightIds configurable, replace throw with eviction
d81741a 
- ipc/discover/rpc: add maxInflightIds option with default 1000
- ipc: replace throw on overflow with oldest-entry eviction
- rpc: Set<string> -> Map<string,number> with timestamp + eviction
- discover: use options.maxInflightIds instead of hardcoded constant
@lete114
refactor(crypto): remove try-catch and Math.random fallback from defa…
f2eb117 
…ultRandomBytes

rngChacha20 confirmed working, no longer need fallback path.
Replace IIFE with plain function + lazy-initialized module-level rng.
@lete114
fix: set HKDF info domain separator to 'mcbe-mods-crypto-v1'
55a97c8
@lete114
fix(test): clean up scriptEventListeners across tests and dispose loc…
b6f9059 
…al instances
@lete114
fix: address final code review items
3fddf66 
- Experience.addXP: add console.warn on negative value
- Log: validate options.level before fallback, add child() name guard
- ms2ticks: clarify error message (non-negative vs positive)
- runtime: re-export normalizeServiceType
- tests: add validation guard tests for addXP, getRandomRangeValue, splitGroups
@lete114
lete114 merged commit 679eecd into main Jun 20, 2026
4 checks passed
@lete114
lete114 deleted the fix/review-batch-1 branch June 20, 2026 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@lete114