Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Command matching may misidentify unrelated processes and does not verify the wallet name; regression coverage is also missing.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Improves bitcore-cli wallet lock-file detection for varied CLI invocation forms and stale-lock cleanup.
Changes:
- Broadens process-command matching.
- Adds stale-lock warnings and explanatory comments.
- Updates wallet lock ownership validation.
File summaries
| File | Reviewed changes |
|---|---|
packages/bitcore-cli/src/wallet.ts |
Updates process matching and stale wallet-lock detection. |
Review details
Suppressed comments (3)
packages/bitcore-cli/src/wallet.ts:269
- Removing
this.namefrom the ownership check means a stale.myWallet.LOCKis treated as live when its PID has been reused by a runningbitcore-cli otherWallet. The lock filename identifies the wallet whose lock is stale, but it does not prove that the current process owns it; parse the arguments and require the wallet-name token while still allowing options before it.
if (!command.includes('bitcore-cli ') && !/^\s*(node\s)?.*(src\/)?cli\.js /m.test(command)) {
packages/bitcore-cli/src/wallet.ts:266
- This comment has an extra article; change “for the this wallet” to “for this wallet.”
// Furthermore, the lock file name includes the wallet name, so the lock should indeed be for the this wallet.
packages/bitcore-cli/src/wallet.ts:269
- This check no longer verifies that the running CLI was opened for
this.name, and the.*cli.jspattern also accepts unrelated processes. If the PID in a stale lock is later reused bybitcore-cli -H ... otherWalletor by anothernode /path/to/cli.jsprocess, this wallet's lock is treated as active indefinitely even though that process does not own it. Parse the command line and require both the actual Bitcore CLI executable and this wallet argument (including the options-before-name form), or use a stronger lock ownership token.
if (!command.includes('bitcore-cli ') && !/^\s*(node\s)?.*(src\/)?cli\.js /m.test(command)) {
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Active legacy lock files may be incorrectly treated as stale; the parsing API type also needs alignment.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
packages/bitcore-cli/src/wallet.ts:269
- Matching only
argv[1]does not prove that the PID still owns this wallet: after PID reuse, anotherbitcore-cliprocess using the samecli.jspath but a different wallet will pass this check and keep the stale lock in place, preventing the intended wallet from opening. Persist and compare the wallet identity (or robustly parse the live argv) in addition to the script path.
const isSameProcess = !!runningScriptPath && !!lockedScriptPath && path.resolve(runningScriptPath) === path.resolve(lockedScriptPath);
packages/bitcore-cli/src/wallet.ts:269
path.resolveresolves relative paths against the contender's current working directory, not the working directory of the process that wrote the lock. Thus relative invocations from different directories can either compare equal for unrelatedcli.jsfiles or fail to identify the same script. Store a canonical/absolute script path (or the lock owner's cwd) and compare against that.
const isSameProcess = !!runningScriptPath && !!lockedScriptPath && path.resolve(runningScriptPath) === path.resolve(lockedScriptPath);
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
| const pid = fs.readFileSync(lockFilename, 'utf-8')?.trim(); | ||
| const lockFileData = fs.readFileSync(lockFilename, 'utf-8')?.trim(); | ||
| const [pid, lockedFullCommand] = lockFileData.split('\n'); | ||
| const lockedScriptPath: string = CWCUtils.tryParse(lockedFullCommand)?.[1]; |
Comment on lines
+181
to
+184
| export function tryParse(json: string, fallback?: any) { | ||
| try { | ||
| if (typeof json === 'object') return json; // already parsed | ||
| return JSON.parse(json); |
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
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.
Description
The wallet lock file was overwritten if another process was running the wallet with options before the name.
e.g.:
Similarly, it would be overwritten if the cli.js file was being run directly in a variety of ways.
e.g.:
Changelog
Testing Notes
Checklist