fix: resolve 3 failing tests in installer and uninstall scripts

[cluster2600]

Summary

• uninstall.sh: check directory writability rather than symlink target
• install.sh: check npm prefix writability instead of assuming NodeSource needs sudo

Brings the test suite from 188/194 passing to 194/194.

Test plan

• All 194 tests passing
• Manual verification on Linux + macOS

Summary by CodeRabbit

• Chores
  • Improved installer privilege detection to more accurately determine when elevated permissions are needed for global package linking, improving reliability across environments.
  • Refined uninstaller removal logic to better decide when elevation is required for deleting protected files, reducing failed removals and unnecessary privilege use.
  • Ignored local research artifacts by default to keep version control clean.
  • Made container runtime installs deterministic by using the lockfile for production dependency installation.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adjusted install and uninstall script privilege checks: install.sh now bases sudo usage for npm link on npm config get prefix writability; uninstall.sh only skips sudo when the target’s parent directory is writable. Added research/ to .gitignore. Docker runtime uses package-lock.json and npm ci.

Changes

Cohort / File(s)	Summary
Install privilege logic scripts/install.sh	Read NPM_GLOBAL_PREFIX via npm config get prefix; set SUDO="sudo" when prefix is non-empty, not writable by the user, and user is not root; apply $SUDO to npm link accordingly.
Uninstall removal checks scripts/uninstall.sh	Changed unprivileged removal guard to require the parent directory be writable ([ -w "$(dirname "$path")" ]) — file writability alone no longer allows direct rm -f.
Docker runtime stage Dockerfile	Runtime stage now copies nemoclaw/package-lock.json as well and uses npm ci --omit=dev instead of npm install --omit=dev for deterministic installs.
VCS ignore list .gitignore	Added research/ to project-specific ignore entries.

Sequence Diagram(s)

sequenceDiagram
  participant Installer as "install.sh"
  participant NPM as "npm config"
  participant FS as Filesystem
  participant SUDO as sudo

  Installer->>NPM: npm config get prefix
  NPM-->>Installer: PREFIX
  Installer->>FS: is PREFIX non-empty?
  FS-->>Installer: yes / no
  Installer->>FS: is PREFIX writable by user?
  FS-->>Installer: yes / no
  alt PREFIX writable or empty or user is root
    Installer->>FS: run npm link (no sudo)
    FS-->>Installer: link result
  else PREFIX not writable & user not root
    Installer->>SUDO: sudo npm link
    SUDO-->>FS: link with elevated perms
    FS-->>Installer: link result
  end

Loading

sequenceDiagram
  participant Remover as "uninstall.sh"
  participant FS as Filesystem
  participant SUDO as sudo

  Remover->>FS: is parent directory writable?
  FS-->>Remover: yes / no
  alt parent directory writable
    Remover->>FS: rm -f target (no sudo)
    FS-->>Remover: removed / failed
  else parent not writable
    alt non-interactive & sudo available
      Remover->>SUDO: sudo rm -f target
      SUDO-->>FS: removed / failed
    else
      Remover->>Remover: skip removal / abort
    end
  end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I nibble lines in bash at dawn,
Checking prefixes on the lawn,
If parents guard the files so tight,
I call for sudo in gentle light,
Then hop away with trailing yarn. 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main objective: fixing 3 failing tests in the installer and uninstall scripts, which directly corresponds to the changes made to scripts/install.sh and uninstall.sh.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Charjags]

Solid fixes both changes improve correctness of the privilege escalation logic:

install.sh Checking npm config get prefix writability instead of hardcoding NODE_MGR = "nodesource" is strictly better. This handles nvm, fnm, Homebrew, and any other Node manager where the global prefix is user-writable, without needing to enumerate package managers. The || true guard and -n check are good defensive coding.

uninstall.sh Correct fix. On Unix, the ability to delete a file depends on write permission to the parent directory (the directory entry), not the file itself. Checking [ -w "$path" ] was the wrong predicate. a read-only file in a writable directory can still be rm -f'd. Narrowing to just [ -w "$(dirname "$path")" ] matches POSIX semantics.

Minor note: research/results.tsv looks like experiment tracking data. maintainers may want to decide if this belongs in the repo long-term or in a separate tracking system.

[cjagwani]

LGTM

[cluster2600]

Sorry about that — research/results.tsv was a local experiment tracking file that got committed by mistake. Removed it and added research/ to .gitignore.

[cjagwani]

@cluster2600 all commits must have verified signatures. Please update commit ce7677d with a signature. Until then merging is blocked!

[cluster2600]

Sorry mate, commit is now signed.

[coderabbitai]

🧹 Nitpick comments (1)

Dockerfile (1)

5-8: Consider: Build stage could also benefit from npm ci for full determinism.

The build stage currently uses npm install without the lockfile, meaning dev dependencies (like TypeScript) are resolved based on semver ranges rather than exact locked versions. For fully reproducible builds, consider copying package-lock.json here as well and using npm ci.

This is optional and outside the scope of this PR's fixes—just noting it for potential future improvement.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Dockerfile` around lines 5 - 8, The Dockerfile's build stage uses RUN npm
install which can produce non-deterministic installs; to make builds
reproducible, update the COPY and RUN steps so the lockfile is included and use
npm ci instead of npm install—specifically, ensure the COPY that currently
references package.json also copies package-lock.json (the existing COPY lines
for package.json/tsconfig.json) and replace the RUN npm install && npm run build
invocation with RUN npm ci && npm run build.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@Dockerfile`:
- Around line 5-8: The Dockerfile's build stage uses RUN npm install which can
produce non-deterministic installs; to make builds reproducible, update the COPY
and RUN steps so the lockfile is included and use npm ci instead of npm
install—specifically, ensure the COPY that currently references package.json
also copies package-lock.json (the existing COPY lines for
package.json/tsconfig.json) and replace the RUN npm install && npm run build
invocation with RUN npm ci && npm run build.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b04ddcf3-0ffe-453b-b32d-be60b4ab5b8b

📥 Commits

Reviewing files that changed from the base of the PR and between e208eb1 and a68320c.

📒 Files selected for processing (4)

• .gitignore
• Dockerfile
• scripts/install.sh
• uninstall.sh

✅ Files skipped from review due to trivial changes (2)

• .gitignore
• scripts/install.sh

🚧 Files skipped from review as they are similar to previous changes (1)

• uninstall.sh