Conversation
Both `packmind-cli` and `packmind` now point to the same entrypoint. `packmind-cli` remains primary. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
After installing the primary `packmind-cli` binary, the installer now creates a `packmind` symlink pointing to it for forward compatibility. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add `createForwardCompatSymlink()` to maintain a `packmind` symlink alongside the primary `packmind-cli` binary after self-update, mirroring the install script behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR introduces a Confidence Score: 4/5Not yet safe to merge — open P1 concerns from prior review rounds (binary-role swap on alias self-update, missing lstat guard before unlinkSync, set -eu aborting install on symlink failure) remain unaddressed in the code. All four issues flagged in the previous review threads are still present in the code unchanged. Two of them (binary-role swap and destructive unlinkSync without lstat check) are P1 defects that can cause silent data loss or a permanently inverted symlink layout for users who run packmind update. The install.sh abort risk is also P1. These warrant resolution before merging. apps/cli/src/infra/commands/updateHandler.ts and apps/cli/scripts/install.sh need the most attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph install["install.sh — fresh install"]
A["Download packmind-cli binary"] --> B["mv → INSTALL_DIR/packmind-cli"]
B --> C["chmod +x packmind-cli"]
C --> D["ln -sf packmind-cli INSTALL_DIR/packmind"]
end
subgraph update_primary["updateViaExecutableReplace — running as packmind-cli"]
E["Download to packmind-cli.update-tmp"] --> F["renameSync → packmind-cli"]
F --> G["chmodSync packmind-cli"]
G --> H["createForwardCompatSymlink(packmind-cli)"]
H --> I["unlinkSync packmind (existing alias)"]
I --> J["symlinkSync packmind-cli → packmind ✓"]
end
subgraph update_alias["updateViaExecutableReplace — running as packmind (alias)"]
K["Download to packmind.update-tmp"] --> L["renameSync → packmind (replaces symlink with real binary)"]
L --> M["chmodSync packmind"]
M --> N["createForwardCompatSymlink(packmind)"]
N --> O["unlinkSync packmind-cli (real binary!)"]
O --> P["symlinkSync packmind → packmind-cli ⚠️ roles swapped"]
end
install --> update_primary
install --> update_alias
Reviews (3): Last reviewed commit: "Revert "🐛 fix(types): accept .mdc exten..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| alias_path="${INSTALL_DIR}/${alias_name}" | ||
| ln -sf "$target_name" "$alias_path" | ||
| info "Created symlink: $alias_path -> $target_name" |
There was a problem hiding this comment.
set -eu
The script runs with set -eu (line 11), which means any non-zero exit from ln -sf will abort the entire script. If symlink creation fails for any reason (e.g. filesystem that doesn't support symlinks, a directory already named packmind, or insufficient write permissions), the script exits with a non-zero code — even though the primary packmind-cli binary was already successfully installed at this point.
This is inconsistent with the TypeScript createForwardCompatSymlink function, which explicitly wraps symlinkSync in a try/catch and treats the failure as non-critical.
Consider protecting the ln call against failure:
| alias_path="${INSTALL_DIR}/${alias_name}" | |
| ln -sf "$target_name" "$alias_path" | |
| info "Created symlink: $alias_path -> $target_name" | |
| ln -sf "$target_name" "$alias_path" || warn "Could not create packmind symlink (non-critical)" | |
| info "Created symlink: $alias_path -> $target_name" |
| } else if (currentBasename === aliasName) { | ||
| targetName = aliasName; | ||
| symlinkName = primaryName; | ||
| } else { | ||
| return; | ||
| } |
There was a problem hiding this comment.
packmind alias silently swaps binary roles
When the user runs packmind update (using the alias), deps.executablePath resolves to the path of the packmind symlink (e.g. /usr/local/bin/packmind). The update sequence then:
- Writes the new binary to
/usr/local/bin/packmind(replacing the symlink with a real file viarenameSync). - Calls
createForwardCompatSymlink('/usr/local/bin/packmind', ...), which detectscurrentBasename === aliasName, so itunlinks/usr/local/bin/packmind-cliand creates a symlinkpackmind-cli → packmind.
This permanently deletes the original packmind-cli real binary and replaces it with a symlink, silently swapping which binary is canonical. The layout after update via alias would be the inverse of what install.sh sets up (packmind-cli pointing to packmind instead of vice-versa). While both names keep working, repeated alternating updates between the two commands would repeatedly swap roles, and tooling that inspects the install layout could be confused.
A safer approach would be to unconditionally create packmind → packmind-cli regardless of which binary invoked the update:
export function createForwardCompatSymlink(
currentPath: string,
platform: string,
): void {
const dir = path.dirname(currentPath);
const ext = platform === 'win32' ? '.exe' : '';
const primaryName = `packmind-cli${ext}`;
const aliasName = `packmind${ext}`;
const currentBasename = path.basename(currentPath);
// Only operate when the real binary is the primary name
if (currentBasename !== primaryName) {
return;
}
const symlinkPath = path.join(dir, aliasName);
try {
unlinkSync(symlinkPath);
} catch {
// May not exist
}
try {
symlinkSync(primaryName, symlinkPath);
logInfoConsole(
`Created forward-compatible symlink: ${symlinkPath} -> ${primaryName}`,
);
} catch {
// Non-critical
}
}| describe('when running as packmind', () => { | ||
| it('creates packmind-cli symlink', () => { | ||
| createForwardCompatSymlink('/usr/local/bin/packmind', 'linux'); | ||
|
|
||
| expect(fs.symlinkSync).toHaveBeenCalledWith( | ||
| 'packmind', | ||
| '/usr/local/bin/packmind-cli', | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
unlinkSync assertion for the reverse case
The "when running as packmind-cli" block explicitly verifies that unlinkSync is called before the new symlink is written (test: "removes existing symlink before creating new one"). The symmetric "when running as packmind" block only checks that symlinkSync is invoked, leaving the cleanup/teardown path for packmind-cli untested.
Consider adding the corresponding assertion:
| describe('when running as packmind', () => { | |
| it('creates packmind-cli symlink', () => { | |
| createForwardCompatSymlink('/usr/local/bin/packmind', 'linux'); | |
| expect(fs.symlinkSync).toHaveBeenCalledWith( | |
| 'packmind', | |
| '/usr/local/bin/packmind-cli', | |
| ); | |
| }); | |
| }); | |
| describe('when running as packmind', () => { | |
| beforeEach(() => { | |
| createForwardCompatSymlink('/usr/local/bin/packmind', 'linux'); | |
| }); | |
| it('creates packmind-cli symlink', () => { | |
| expect(fs.symlinkSync).toHaveBeenCalledWith( | |
| 'packmind', | |
| '/usr/local/bin/packmind-cli', | |
| ); | |
| }); | |
| it('removes existing symlink before creating new one', () => { | |
| expect(fs.unlinkSync).toHaveBeenCalledWith('/usr/local/bin/packmind-cli'); | |
| }); | |
| }); |
vincent-psarga
left a comment
vincent-psarga
left a comment
There was a problem hiding this comment.
Shouldn't this file: https://raw.githubusercontent.com/PackmindHub/packmind/main/apps/cli/scripts/install.sh be updated too to create both packmind-cli and packmind executables?
|
|
||
| ## Added | ||
|
|
||
| - CLI can now also be invoked as `packmind` (alias for `packmind-cli`) |
There was a problem hiding this comment.
Drop this - it's already in cli Changelog
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ation" This reverts commit 7569c3b.
| const symlinkPath = path.join(dir, symlinkName); | ||
| try { | ||
| unlinkSync(symlinkPath); | ||
| } catch { | ||
| // May not exist | ||
| } |
There was a problem hiding this comment.
unlinkSync does not guard against removing a real binary
The unlinkSync(symlinkPath) call removes whatever exists at symlinkPath — symlink or real file — without first verifying it is a symlink. If a user happens to have an independently-installed binary named packmind (e.g. from an earlier npm global install of a package of that name) living in the same directory as packmind-cli, a packmind-cli update will silently delete that binary and replace it with a symlink.
Guarding with lstatSync before unlinking makes the intent explicit and prevents unintended data loss:
| const symlinkPath = path.join(dir, symlinkName); | |
| try { | |
| unlinkSync(symlinkPath); | |
| } catch { | |
| // May not exist | |
| } | |
| const symlinkPath = path.join(dir, symlinkName); | |
| try { | |
| const stat = lstatSync(symlinkPath); | |
| if (stat.isSymbolicLink()) { | |
| unlinkSync(symlinkPath); | |
| } | |
| } catch { | |
| // May not exist | |
| } |
(You would need to import lstatSync from 'fs' alongside the existing imports.)
|
3 participants
Summary
packmindas a secondbinentry inpackage.json, pointing to the samemain.cjsentrypointpackmindsymlink during install (install.sh) and maintain it on self-update (updateHandler.ts)packmind-cliremains the primary name, all output strings unchangedThis is the first step toward renaming the CLI to
packmind. By silently shipping the alias now, we ensure both names work before any deprecation or user-visible switch happens.What's changed
apps/cli/package.json"packmind": "./main.cjs"tobinapps/cli/scripts/install.shpackmindsymlink after installingpackmind-clibinaryapps/cli/src/infra/commands/updateHandler.tscreateForwardCompatSymlink()recreates the alias after self-updateapps/cli/src/infra/commands/updateHandler.spec.ts.exe, and error handlingCHANGELOG.MD+apps/cli/CHANGELOG.MD[Unreleased] > AddedWhat does NOT change
packmind-cli), version output, usage messagesBINARY_NAME