feat: npm publish readiness — version, update-check, install scripts (#286–#289)#292
feat: npm publish readiness — version, update-check, install scripts (#286–#289)#292flyingrobots wants to merge 8 commits intomainfrom
Conversation
WalkthroughRepository transferred from Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Process
participant UpdateCheck as Update Checker
participant Cache as ~/.gitmind/update-check.json
participant Registry as npm Registry
participant User as User (stderr)
CLI->>UpdateCheck: triggerCheck() [fire-and-forget]
Note over UpdateCheck: Non-blocking background task
CLI->>UpdateCheck: getNotification()
UpdateCheck->>Cache: readCache()
alt Cache hit & not stale
Cache-->>UpdateCheck: return cached data
else Cache miss or stale
UpdateCheck-->>CLI: return null (previous version)
UpdateCheck->>Registry: fetchLatest(signal) [async]
Registry-->>UpdateCheck: latest version
UpdateCheck->>Cache: writeCache(data)
end
UpdateCheck-->>CLI: notification string (if newer)
CLI->>User: output command result
alt notification exists & !jsonMode
CLI->>User: write notification to stderr
end
Note over CLI: On process exit
CLI->>UpdateCheck: abort signal
UpdateCheck->>Registry: [abort fetch if pending]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Rationale: Multiple interacting systems with new logic (update checker with cache TTL, semver comparison, abort signaling), CLI lifecycle integration, shell script validation, and heterogeneous changes across ~40 files. While schema updates are repetitive and low-effort, the update-check module introduces density around async patterns, error handling, and I/O abstraction. CLI integration requires careful verification of lifecycle timing, signal propagation, and JSON mode suppression. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2842b3a58
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "files": [ | ||
| "bin/", | ||
| "src/", | ||
| "docs/contracts/extension-manifest.schema.json", |
There was a problem hiding this comment.
The new files whitelist omits docs/contracts/cli/**, so published tarballs no longer contain the CLI contract schemas even though the project documents those paths for validating --json output; with this commit, packaging includes docs/contracts/extension-manifest.schema.json but not docs/contracts/cli/*.schema.json, which will cause ENOENT for consumers loading schemas from an installed package.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| // Cancel background fetch — don't hold the process open | ||
| updateController.abort(); |
There was a problem hiding this comment.
Aborting updateController unconditionally at shutdown cancels the background check for fast commands before writeCache can run (the checker is explicitly signal-aware), so commands that complete quickly will repeatedly skip cache refresh and users may never see upgrade notifications because getNotification() reads only cached results.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/git-mind.js (1)
31-129: 🛠️ Refactor suggestion | 🟠 MajorAll terminal output must use chalk/figures throughout the file.
The guideline violation is confirmed:
bin/git-mind.jsmatches thebin/**pattern and must use chalk + figures for all output formatting. Currently:
printUsage()(lines 31–129) outputs raw text viaconsole.log()- Line 191:
console.log(VERSION)is unformatted- Multiple
console.error()calls (lines 204, 234, 271–272, 284, 305, 323, 341, 380, 428, 441, 454, 464, 472, 473, 503, 504, 518) lack formatting- Line 531: update notification written to stderr without styling
Dependencies (
chalk@^5.3.0,figures@^6.0.1) are available. Refactor all output calls to apply chalk for colors/text styles and figures for symbols.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/git-mind.js` around lines 31 - 129, The file uses plain console.log/console.error for all user output; replace all unstyled output with chalk and figures-based formatting: update printUsage() to build the usage string using chalk (bold headers, dim flags, cyan inline values) and figures (bullet arrows, check/cross symbols) instead of raw console.log, replace the standalone VERSION print (console.log(VERSION)) with a styled chalk output, and change every console.error call mentioned (errors around the code paths invoking printUsage, CLI command handlers and the listed lines) to use chalk.red / chalk.yellow with appropriate figures.error/figures.warning symbols; also ensure the update-notification written to stderr uses chalk styling and a figures symbol. Locate and modify calls by referencing printUsage(), VERSION, and each console.error invocation in the CLI command handlers to apply consistent chalk+figures formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.sh`:
- Around line 63-66: The install script currently uses eval to expand PREFIX
which permits command injection; change the expansion to perform safe tilde
handling without eval by detecting when PREFIX begins with "~" and replacing
that leading "~" with the value of HOME (e.g., set PREFIX to HOME concatenated
with PREFIX without the leading "~" when PREFIX starts with "~"), leave other
values untouched, then call npm install -g --prefix "$PREFIX" "$PACKAGE" as
before; update the code around the PREFIX assignment and the npm install call
(references: variable PREFIX and the npm install -g --prefix invocation) to use
this safe tilde-expansion logic.
- Around line 13-18: The --prefix branch in the argument-parsing loop (while [
$# -gt 0 ]; do ... case "$1" in --prefix) must validate that a following value
exists before doing PREFIX="$2" and shift 2; update the branch to check that $#
is at least 2 and that $2 is not another option (e.g., does not start with "-"),
and if validation fails emit a clear error message to stderr and exit non-zero
instead of shifting into an empty/next flag; keep using the same symbols
(PREFIX, --prefix, shift 2, the while/case block) so the fix is localized to
that branch.
In `@src/update-check.js`:
- Around line 120-122: Remove the unused variable declaration "let policy =
null;" in src/update-check.js to satisfy the no-unused-vars lint rule; locate
the declaration (commented with /** `@type`
{import('@git-stunts/alfred').Policy|null} */ and the subsequent let policy =
null;) and delete both the JSDoc/type annotation and the variable so there are
no remaining references to "policy".
In `@src/version.js`:
- Around line 9-10: The module currently does synchronous file I/O at import
(pkgPath / readFileSync / JSON.parse) without any error handling; wrap the
readFileSync + JSON.parse in a try/catch in src/version.js (around where pkgPath
and pkg are computed) and on any error (ENOENT, SyntaxError, etc.) do not
throw—log a concise warning (console.warn or a logger) including the error, and
export a safe fallback version (e.g., '0.0.0' or null) so importing modules
(bin/git-mind.js, src/index.js) won't crash at load; also add tests in
test/version.test.js for the error path to assert the fallback is returned and a
warning is emitted.
In `@test/Dockerfile`:
- Around line 1-27: The Dockerfile currently runs tests as root; create a
non-root user and switch to it before CMD to avoid running processes as UID 0.
Add commands after WORKDIR /app (and after files are copied and dependencies
installed) to create a user/group (e.g., addgroup --system testuser && adduser
--system --ingroup testuser testuser), chown -R /app to that user, and then add
a USER testuser line so the subsequent CMD (the existing CMD ["npx", "vitest",
...]) runs unprivileged; ensure any needed directories (like node_modules if
created earlier) are owned by that user.
- Line 3: Update the RUN instruction in the Dockerfile to add
--no-install-recommends to apt-get install and pin explicit package versions for
git, python3, make, and g++ (use apt-cache policy <pkg> on a node:22-slim
container to determine current candidate versions) while preserving the apt-get
update and the rm -rf /var/lib/apt/lists/* cleanup; this addresses Hadolint
DL3008/DL3015 and Trivy DS-0029 by preventing installation of recommended
packages and ensuring reproducible builds.
- Line 1: Update the Dockerfile to pin the base image, create a non-root user,
and lock apt installs: replace the floating FROM line with the digest-pinned
form (FROM
node:22@sha256:373f9e53a753877bcbd21e6e7884682f6b1988ee63a4c4129e9051bb96546081),
add steps to create and switch to a non-root user (e.g., adduser/ useradd and a
USER directive such as USER app or USER 1000 to avoid running tests as root),
and change any apt-get install commands to use --no-install-recommends plus
explicit package=version pins and cleanup (apt-get update && apt-get install -y
--no-install-recommends pkg=<version> ... && rm -rf /var/lib/apt/lists/*) so
builds are reproducible and minimal.
In `@test/update-check.test.js`:
- Around line 114-178: Replace the flaky setTimeout waits in the tests that call
triggerCheck() with Vitest fake timers: wrap each test's triggerCheck() call
between vi.useFakeTimers() and vi.useRealTimers(), then after triggerCheck()
call vi.runAllTimersAsync() to deterministically drive the fire-and-forget
_doCheck() promise to completion; update tests referencing triggerCheck() (and
any expectations that read written/fetched flags or cache state) to run timers
before asserting so the microtask queue is settled reliably.
In `@test/version.test.js`:
- Around line 24-36: The tests call execFileSync(process.execPath, [BIN,
'--version']) and execFileSync(..., [BIN, '-v']) without a timeout, so a hung
child (e.g. stalled update-check) can block CI; update both execFileSync
invocations to pass a timeout option (e.g., 5000 ms) alongside encoding to
ensure the subprocess is killed if it hangs, keeping the same expectations for
VERSION.
- Around line 24-36: The tests call execFileSync(process.execPath, [BIN,
'--version']) and execFileSync(process.execPath, [BIN, '-v']) without a timeout,
which can hang; add a timeout option (e.g. { encoding: 'utf-8', timeout: 5000 })
to both execFileSync invocations so the subprocess will be killed after a
reasonable period, keeping the assertions against VERSION the same and
preventing CI hangs.
- Around line 14-16: The regex in the test "VERSION is a valid semver string"
that matches the constant VERSION is unanchored and will accept values like
"1.0.0abc"; update the assertion to either enforce a strict semver by anchoring
the pattern (ensure the regex applied to VERSION ends with $) or, if
pre-release/metadata is allowed, change the test description to reflect that
intent (e.g., include "allows pre-release/metadata") and use an appropriate
regex; locate the test by the it block text "VERSION is a valid semver string"
and the constant NAME VERSION to apply the fix.
- Around line 14-16: The test for VERSION uses an unanchored regex; update the
assertion so VERSION is fully validated as semver by matching the whole string
and allowing optional prerelease/build metadata. Replace the current
expect(VERSION).toMatch(/^\d+\.\d+\.\d+/) with a regex anchored at both ends
(e.g. add ^...$) that includes optional hyphen-prefixed prerelease and
plus-prefixed build parts, keeping the test case named "VERSION is a valid
semver string" and referencing the VERSION constant.
- Line 11: The test uses import.meta.dirname to build BIN which is incompatible
with Node 20.0–20.10; update the BIN resolution to use a safe fallback: compute
a dir variable that uses import.meta.dirname when available and otherwise
derives a directory from import.meta.url via fileURLToPath + path.dirname
(importing fileURLToPath from 'url' and dirname from 'path'), then use that dir
with join to construct BIN; update the symbol referencing BIN and ensure imports
for fileURLToPath/dirname are added so tests run on full Node 20.
In `@uninstall.sh`:
- Around line 12-18: The --prefix case in uninstall.sh currently does shift 2
without validating that a value exists; update the case handling for "--prefix"
(the while/case block and PREFIX assignment) to first check that "$2" is
non-empty and not another option, and if invalid print a clear error message and
exit non-zero instead of performing shift 2; after validation assign PREFIX="$2"
and then shift 2 as before so you avoid a shell error when a value is missing.
- Around line 44-46: The script currently uses eval to expand PREFIX which risks
command injection; replace the eval-based expansion with safe tilde expansion by
detecting a leading tilde in PREFIX and replacing it with the user's HOME (e.g.
transform PREFIX when it starts with "~" into "$HOME/..."); keep the existing
npm uninstall -g --prefix "$PREFIX" "$PACKAGE" call and ensure PREFIX remains
quoted afterwards to preserve spaces and prevent word-splitting (refer to the
PREFIX variable and the npm uninstall invocation to locate the change).
---
Outside diff comments:
In `@bin/git-mind.js`:
- Around line 31-129: The file uses plain console.log/console.error for all user
output; replace all unstyled output with chalk and figures-based formatting:
update printUsage() to build the usage string using chalk (bold headers, dim
flags, cyan inline values) and figures (bullet arrows, check/cross symbols)
instead of raw console.log, replace the standalone VERSION print
(console.log(VERSION)) with a styled chalk output, and change every
console.error call mentioned (errors around the code paths invoking printUsage,
CLI command handlers and the listed lines) to use chalk.red / chalk.yellow with
appropriate figures.error/figures.warning symbols; also ensure the
update-notification written to stderr uses chalk styling and a figures symbol.
Locate and modify calls by referencing printUsage(), VERSION, and each
console.error invocation in the CLI command handlers to apply consistent
chalk+figures formatting.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (40)
.reuse/dep5CHANGELOG.mdCONTRIBUTING.mdGUIDE.mdbin/git-mind.jsdocs/contracts/cli/at.schema.jsondocs/contracts/cli/content-delete.schema.jsondocs/contracts/cli/content-meta.schema.jsondocs/contracts/cli/content-set.schema.jsondocs/contracts/cli/content-show.schema.jsondocs/contracts/cli/diff.schema.jsondocs/contracts/cli/doctor.schema.jsondocs/contracts/cli/export-data.schema.jsondocs/contracts/cli/export-file.schema.jsondocs/contracts/cli/extension-add.schema.jsondocs/contracts/cli/extension-list.schema.jsondocs/contracts/cli/extension-remove.schema.jsondocs/contracts/cli/extension-validate.schema.jsondocs/contracts/cli/import.schema.jsondocs/contracts/cli/merge.schema.jsondocs/contracts/cli/node-detail.schema.jsondocs/contracts/cli/node-list.schema.jsondocs/contracts/cli/review-batch.schema.jsondocs/contracts/cli/review-list.schema.jsondocs/contracts/cli/set.schema.jsondocs/contracts/cli/status.schema.jsondocs/contracts/cli/suggest.schema.jsondocs/contracts/cli/unset.schema.jsondocs/contracts/cli/view-lens.schema.jsondocs/contracts/cli/view-progress.schema.jsoninstall.shpackage.jsonsrc/format-pr.jssrc/index.jssrc/update-check.jssrc/version.jstest/Dockerfiletest/update-check.test.jstest/version.test.jsuninstall.sh
| # Parse arguments | ||
| while [ $# -gt 0 ]; do | ||
| case "$1" in | ||
| --prefix) | ||
| PREFIX="$2" | ||
| shift 2 |
There was a problem hiding this comment.
Validate --prefix has a value before shifting.
Missing a value causes a hard failure instead of a clean error.
Suggested fix
--prefix)
- PREFIX="$2"
- shift 2
+ if [ -z "$2" ]; then
+ echo "Error: --prefix requires a path" >&2
+ exit 1
+ fi
+ PREFIX="$2"
+ shift 2
;;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Parse arguments | |
| while [ $# -gt 0 ]; do | |
| case "$1" in | |
| --prefix) | |
| PREFIX="$2" | |
| shift 2 | |
| # Parse arguments | |
| while [ $# -gt 0 ]; do | |
| case "$1" in | |
| --prefix) | |
| if [ -z "$2" ]; then | |
| echo "Error: --prefix requires a path" >&2 | |
| exit 1 | |
| fi | |
| PREFIX="$2" | |
| shift 2 | |
| ;; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install.sh` around lines 13 - 18, The --prefix branch in the argument-parsing
loop (while [ $# -gt 0 ]; do ... case "$1" in --prefix) must validate that a
following value exists before doing PREFIX="$2" and shift 2; update the branch
to check that $# is at least 2 and that $2 is not another option (e.g., does not
start with "-"), and if validation fails emit a clear error message to stderr
and exit non-zero instead of shifting into an empty/next flag; keep using the
same symbols (PREFIX, --prefix, shift 2, the while/case block) so the fix is
localized to that branch.
| if [ -n "$PREFIX" ]; then | ||
| # Expand ~ if present | ||
| PREFIX=$(eval echo "$PREFIX") | ||
| npm install -g --prefix "$PREFIX" "$PACKAGE" |
There was a problem hiding this comment.
Stop using eval for prefix expansion.
eval enables command injection with user-supplied input. Use safe tilde expansion instead.
Suggested fix
if [ -n "$PREFIX" ]; then
# Expand ~ if present
- PREFIX=$(eval echo "$PREFIX")
+ case "$PREFIX" in
+ "~"|"~/"*) PREFIX="${HOME}${PREFIX#\~}";;
+ esac
npm install -g --prefix "$PREFIX" "$PACKAGE"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ -n "$PREFIX" ]; then | |
| # Expand ~ if present | |
| PREFIX=$(eval echo "$PREFIX") | |
| npm install -g --prefix "$PREFIX" "$PACKAGE" | |
| if [ -n "$PREFIX" ]; then | |
| # Expand ~ if present | |
| case "$PREFIX" in | |
| "~"|"~/"*) PREFIX="${HOME}${PREFIX#\~}";; | |
| esac | |
| npm install -g --prefix "$PREFIX" "$PACKAGE" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install.sh` around lines 63 - 66, The install script currently uses eval to
expand PREFIX which permits command injection; change the expansion to perform
safe tilde handling without eval by detecting when PREFIX begins with "~" and
replacing that leading "~" with the value of HOME (e.g., set PREFIX to HOME
concatenated with PREFIX without the leading "~" when PREFIX starts with "~"),
leave other values untouched, then call npm install -g --prefix "$PREFIX"
"$PACKAGE" as before; update the code around the PREFIX assignment and the npm
install call (references: variable PREFIX and the npm install -g --prefix
invocation) to use this safe tilde-expansion logic.
src/update-check.js
Outdated
| /** @type {import('@git-stunts/alfred').Policy|null} */ | ||
| let policy = null; | ||
|
|
There was a problem hiding this comment.
Remove unused policy to fix lint failure.
CI already fails with no-unused-vars. Drop the dead variable.
Suggested fix
- /** `@type` {import('@git-stunts/alfred').Policy|null} */
- let policy = null;
-
return {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** @type {import('@git-stunts/alfred').Policy|null} */ | |
| let policy = null; |
🧰 Tools
🪛 GitHub Actions: CI
[error] 121-121: ESLint: 'policy' is defined but never used. (no-unused-vars)
🪛 GitHub Check: test (22)
[failure] 121-121:
'policy' is assigned a value but never used. Allowed unused vars must match /^_/u
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/update-check.js` around lines 120 - 122, Remove the unused variable
declaration "let policy = null;" in src/update-check.js to satisfy the
no-unused-vars lint rule; locate the declaration (commented with /** `@type`
{import('@git-stunts/alfred').Policy|null} */ and the subsequent let policy =
null;) and delete both the JSDoc/type annotation and the variable so there are
no remaining references to "policy".
| const pkgPath = fileURLToPath(new URL('../package.json', import.meta.url)); | ||
| const pkg = JSON.parse(readFileSync(pkgPath, 'utf-8')); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check src/version.js to understand the module context and usage
head -20 src/version.jsRepository: flyingrobots/git-mind
Length of output: 516
🏁 Script executed:
# Check the package.json structure, particularly the files field
cat package.jsonRepository: flyingrobots/git-mind
Length of output: 1654
🏁 Script executed:
# Look for .npmignore which could override npm defaults
fd -g '.npmignore' --max-depth 2Repository: flyingrobots/git-mind
Length of output: 272
🏁 Script executed:
# Verify the import pattern and how version.js is used
rg -l 'version.js' --type jsRepository: flyingrobots/git-mind
Length of output: 114
🏁 Script executed:
cat test/version.test.jsRepository: flyingrobots/git-mind
Length of output: 1081
Unguarded synchronous I/O at module load will crash silently — add defensive error handling.
Lines 9–10 run at import time with zero error handling. Any ENOENT (running from non-standard working trees, bundled outputs, or broken symlinks) or SyntaxError from corrupted package.json will propagate as an unhandled top-level exception, killing the CLI with a cryptic stack trace. Since version.js is imported early and unconditionally by bin/git-mind.js and src/index.js, this is a reliability hazard.
The files array in package.json does not whitelist package.json itself, but npm includes it by default during publish — so the shipped package is fine. The real risk is development environments, bundled distributions, or unusual runtime paths.
Test coverage gap: test/version.test.js validates happy-path values but has no tests for error scenarios.
🛡️ Proposed fix — defensive read with sensible fallback
-const pkgPath = fileURLToPath(new URL('../package.json', import.meta.url));
-const pkg = JSON.parse(readFileSync(pkgPath, 'utf-8'));
+let pkg = { version: '0.0.0', name: '@flyingrobots/git-mind' };
+try {
+ const pkgPath = fileURLToPath(new URL('../package.json', import.meta.url));
+ pkg = JSON.parse(readFileSync(pkgPath, 'utf-8'));
+} catch {
+ // package.json unreadable — fall back to compile-time defaults above
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/version.js` around lines 9 - 10, The module currently does synchronous
file I/O at import (pkgPath / readFileSync / JSON.parse) without any error
handling; wrap the readFileSync + JSON.parse in a try/catch in src/version.js
(around where pkgPath and pkg are computed) and on any error (ENOENT,
SyntaxError, etc.) do not throw—log a concise warning (console.warn or a logger)
including the error, and export a safe fallback version (e.g., '0.0.0' or null)
so importing modules (bin/git-mind.js, src/index.js) won't crash at load; also
add tests in test/version.test.js for the error path to assert the fallback is
returned and a warning is emitted.
| FROM node:22-slim | ||
|
|
||
| RUN apt-get update && apt-get install -y git python3 make g++ && rm -rf /var/lib/apt/lists/* | ||
|
|
||
| # Disable upgrade check — no network egress in test container | ||
| ENV GIT_MIND_DISABLE_UPGRADE_CHECK=1 | ||
| ENV NO_COLOR=1 | ||
|
|
||
| WORKDIR /app | ||
|
|
||
| # Install deps first (layer cache) | ||
| COPY package.json package-lock.json ./ | ||
| RUN npm ci | ||
|
|
||
| # Copy source | ||
| COPY bin/ bin/ | ||
| COPY src/ src/ | ||
| COPY test/ test/ | ||
| COPY docs/contracts/ docs/contracts/ | ||
| COPY extensions/ extensions/ | ||
|
|
||
| # Git identity for tests that init repos | ||
| RUN git config --global user.email "test@test.com" && \ | ||
| git config --global user.name "Test" | ||
|
|
||
| # Run only integration tests (files that spawn bin/git-mind.js) | ||
| CMD ["npx", "vitest", "run", "test/contracts.integration.test.js", "test/content.test.js", "test/version.test.js"] |
There was a problem hiding this comment.
No USER directive — container runs every test process as root (UID 0).
Trivy DS-0002 and Checkov CKV_DOCKER_3 both flag this as an error. Git operations and file-I/O in tests executing as root can mask permission bugs that would surface for a real unprivileged user install. Add a non-root user before CMD.
🛡️ Proposed fix: add a non-root user
RUN git config --global user.email "test@test.com" && \
git config --global user.name "Test"
+RUN useradd -m testrunner && chown -R testrunner /app
+USER testrunner
+
# Run only integration tests (files that spawn bin/git-mind.js)
CMD ["npx", "vitest", "run", ...]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| FROM node:22-slim | |
| RUN apt-get update && apt-get install -y git python3 make g++ && rm -rf /var/lib/apt/lists/* | |
| # Disable upgrade check — no network egress in test container | |
| ENV GIT_MIND_DISABLE_UPGRADE_CHECK=1 | |
| ENV NO_COLOR=1 | |
| WORKDIR /app | |
| # Install deps first (layer cache) | |
| COPY package.json package-lock.json ./ | |
| RUN npm ci | |
| # Copy source | |
| COPY bin/ bin/ | |
| COPY src/ src/ | |
| COPY test/ test/ | |
| COPY docs/contracts/ docs/contracts/ | |
| COPY extensions/ extensions/ | |
| # Git identity for tests that init repos | |
| RUN git config --global user.email "test@test.com" && \ | |
| git config --global user.name "Test" | |
| # Run only integration tests (files that spawn bin/git-mind.js) | |
| CMD ["npx", "vitest", "run", "test/contracts.integration.test.js", "test/content.test.js", "test/version.test.js"] | |
| FROM node:22-slim | |
| RUN apt-get update && apt-get install -y git python3 make g++ && rm -rf /var/lib/apt/lists/* | |
| # Disable upgrade check — no network egress in test container | |
| ENV GIT_MIND_DISABLE_UPGRADE_CHECK=1 | |
| ENV NO_COLOR=1 | |
| WORKDIR /app | |
| # Install deps first (layer cache) | |
| COPY package.json package-lock.json ./ | |
| RUN npm ci | |
| # Copy source | |
| COPY bin/ bin/ | |
| COPY src/ src/ | |
| COPY test/ test/ | |
| COPY docs/contracts/ docs/contracts/ | |
| COPY extensions/ extensions/ | |
| # Git identity for tests that init repos | |
| RUN git config --global user.email "test@test.com" && \ | |
| git config --global user.name "Test" | |
| RUN useradd -m testrunner && chown -R testrunner /app | |
| USER testrunner | |
| # Run only integration tests (files that spawn bin/git-mind.js) | |
| CMD ["npx", "vitest", "run", "test/contracts.integration.test.js", "test/content.test.js", "test/version.test.js"] |
🧰 Tools
🪛 Checkov (3.2.334)
[low] 1-27: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[low] 1-27: Ensure that a user for the container has been created
(CKV_DOCKER_3)
🪛 Hadolint (2.14.0)
[warning] 3-3: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version>
(DL3008)
[info] 3-3: Avoid additional packages by specifying --no-install-recommends
(DL3015)
🪛 Trivy (0.69.1)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
[info] 1-1: No HEALTHCHECK defined
Add HEALTHCHECK instruction in your Dockerfile
Rule: DS-0026
(IaC/Dockerfile)
[error] 3-3: 'apt-get' missing '--no-install-recommends'
'--no-install-recommends' flag is missed: 'apt-get update && apt-get install -y git python3 make g++ && rm -rf /var/lib/apt/lists/*'
Rule: DS-0029
(IaC/Dockerfile)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/Dockerfile` around lines 1 - 27, The Dockerfile currently runs tests as
root; create a non-root user and switch to it before CMD to avoid running
processes as UID 0. Add commands after WORKDIR /app (and after files are copied
and dependencies installed) to create a user/group (e.g., addgroup --system
testuser && adduser --system --ingroup testuser testuser), chown -R /app to that
user, and then add a USER testuser line so the subsequent CMD (the existing CMD
["npx", "vitest", ...]) runs unprivileged; ensure any needed directories (like
node_modules if created earlier) are owned by that user.
| import { join } from 'node:path'; | ||
| import { VERSION, NAME } from '../src/version.js'; | ||
|
|
||
| const BIN = join(import.meta.dirname, '..', 'bin', 'git-mind.js'); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
import.meta.dirname Node.js when was it added release version
💡 Result:
import.meta.dirname was added to Node.js in:
- Node v21.2.0 (released Nov 14, 2023) (nodejs.org)
- Node v20.11.0 (released Jan 10, 2024) — backported to the v20 LTS line (nodejs.org)
Citations:
- 1: https://nodejs.org/download/release/v22.11.0/docs/api/esm.html?utm_source=openai
- 2: https://nodejs.org/download/release/v20.19.0/docs/api/esm.html?utm_source=openai
🏁 Script executed:
# Verify the install.sh version gating claim
fd install.sh -type f | head -5Repository: flyingrobots/git-mind
Length of output: 235
🏁 Script executed:
# Search for install.sh
fd install.shRepository: flyingrobots/git-mind
Length of output: 88
🏁 Script executed:
# Read install.sh to check Node version gating
cat install.shRepository: flyingrobots/git-mind
Length of output: 2131
import.meta.dirname incompatible with Node 20.0–20.10, contradicts stated runtime guideline.
The guideline mandates Node.js >= 20, but import.meta.dirname requires >= 20.11.0 (backported from v21.2.0). While install.sh enforces >= 22 in practice, the stated runtime floor creates a version mismatch. Test files running in isolated CI/dev environments may bypass the install script and encounter undefined references.
Apply the defensive fallback:
Fallback for full Node 20 support
-import { join } from 'node:path';
+import { join, dirname } from 'node:path';
+import { fileURLToPath } from 'node:url';
-const BIN = join(import.meta.dirname, '..', 'bin', 'git-mind.js');
+const __dirname = import.meta.dirname ?? dirname(fileURLToPath(import.meta.url));
+const BIN = join(__dirname, '..', 'bin', 'git-mind.js');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/version.test.js` at line 11, The test uses import.meta.dirname to build
BIN which is incompatible with Node 20.0–20.10; update the BIN resolution to use
a safe fallback: compute a dir variable that uses import.meta.dirname when
available and otherwise derives a directory from import.meta.url via
fileURLToPath + path.dirname (importing fileURLToPath from 'url' and dirname
from 'path'), then use that dir with join to construct BIN; update the symbol
referencing BIN and ensure imports for fileURLToPath/dirname are added so tests
run on full Node 20.
| it('VERSION is a valid semver string', () => { | ||
| expect(VERSION).toMatch(/^\d+\.\d+\.\d+/); | ||
| }); |
There was a problem hiding this comment.
Semver regex is unanchored — add $ or acknowledge pre-release intent.
/^\d+\.\d+\.\d+/ (no trailing $) matches "1.0.0abc", "1.0.0-anything", etc. If the intent is to enforce a clean release version, anchor it. If pre-release suffixes are acceptable, rename the test to reflect that.
✅ Proposed fix
- it('VERSION is a valid semver string', () => {
- expect(VERSION).toMatch(/^\d+\.\d+\.\d+/);
- });
+ it('VERSION is a valid semver string', () => {
+ // Allows pre-release labels (e.g. 1.0.0-beta.1) but rejects non-numeric junk
+ expect(VERSION).toMatch(/^\d+\.\d+\.\d+(-[\w.]+)?(\+[\w.]+)?$/);
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/version.test.js` around lines 14 - 16, The regex in the test "VERSION is
a valid semver string" that matches the constant VERSION is unanchored and will
accept values like "1.0.0abc"; update the assertion to either enforce a strict
semver by anchoring the pattern (ensure the regex applied to VERSION ends with
$) or, if pre-release/metadata is allowed, change the test description to
reflect that intent (e.g., include "allows pre-release/metadata") and use an
appropriate regex; locate the test by the it block text "VERSION is a valid
semver string" and the constant NAME VERSION to apply the fix.
Semver regex is unanchored — "1.0.0-rc.1" and "1.0.0abc" both pass silently.
/^\d+\.\d+\.\d+/ has no $ end anchor. The test description says "valid semver string" but the assertion accepts any string that merely starts with a three-part numeric prefix. Anchor it or at least match the optional pre-release/build-metadata segments explicitly.
✅ Proposed fix
- expect(VERSION).toMatch(/^\d+\.\d+\.\d+/);
+ expect(VERSION).toMatch(/^\d+\.\d+\.\d+(-[\w.]+)?(\+[\w.]+)?$/);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('VERSION is a valid semver string', () => { | |
| expect(VERSION).toMatch(/^\d+\.\d+\.\d+/); | |
| }); | |
| it('VERSION is a valid semver string', () => { | |
| expect(VERSION).toMatch(/^\d+\.\d+\.\d+(-[\w.]+)?(\+[\w.]+)?$/); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/version.test.js` around lines 14 - 16, The test for VERSION uses an
unanchored regex; update the assertion so VERSION is fully validated as semver
by matching the whole string and allowing optional prerelease/build metadata.
Replace the current expect(VERSION).toMatch(/^\d+\.\d+\.\d+/) with a regex
anchored at both ends (e.g. add ^...$) that includes optional hyphen-prefixed
prerelease and plus-prefixed build parts, keeping the test case named "VERSION
is a valid semver string" and referencing the VERSION constant.
| it('prints version and exits with --version', () => { | ||
| const out = execFileSync(process.execPath, [BIN, '--version'], { | ||
| encoding: 'utf-8', | ||
| }).trim(); | ||
| expect(out).toBe(VERSION); | ||
| }); | ||
|
|
||
| it('prints version and exits with -v', () => { | ||
| const out = execFileSync(process.execPath, [BIN, '-v'], { | ||
| encoding: 'utf-8', | ||
| }).trim(); | ||
| expect(out).toBe(VERSION); | ||
| }); |
There was a problem hiding this comment.
execFileSync has no timeout — a hung subprocess blocks CI forever.
Both CLI invocations carry no timeout option. The PR introduces a fire-and-forget update-check that initiates a network fetch; if it fires before the --version early-exit path and then stalls (e.g., network unavailable, 3 s AbortController window), the subprocess doesn't exit until that timer fires — and if the abort logic has any bug, it hangs forever, taking the test runner with it.
⏱️ Proposed fix
const out = execFileSync(process.execPath, [BIN, '--version'], {
encoding: 'utf-8',
+ timeout: 5000,
}).trim(); const out = execFileSync(process.execPath, [BIN, '-v'], {
encoding: 'utf-8',
+ timeout: 5000,
}).trim();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/version.test.js` around lines 24 - 36, The tests call
execFileSync(process.execPath, [BIN, '--version']) and execFileSync(..., [BIN,
'-v']) without a timeout, so a hung child (e.g. stalled update-check) can block
CI; update both execFileSync invocations to pass a timeout option (e.g., 5000
ms) alongside encoding to ensure the subprocess is killed if it hangs, keeping
the same expectations for VERSION.
execFileSync has no timeout — subprocess tests can hang forever.
Both CLI invocations lack a timeout option. If the bin entry point doesn't short-circuit --version before the update-check fires (or any other async leak lands on the event loop and delays process exit), these tests block indefinitely in CI.
⏱️ Proposed fix
- const out = execFileSync(process.execPath, [BIN, '--version'], {
- encoding: 'utf-8',
- }).trim();
+ const out = execFileSync(process.execPath, [BIN, '--version'], {
+ encoding: 'utf-8',
+ timeout: 5000,
+ }).trim();- const out = execFileSync(process.execPath, [BIN, '-v'], {
- encoding: 'utf-8',
- }).trim();
+ const out = execFileSync(process.execPath, [BIN, '-v'], {
+ encoding: 'utf-8',
+ timeout: 5000,
+ }).trim();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('prints version and exits with --version', () => { | |
| const out = execFileSync(process.execPath, [BIN, '--version'], { | |
| encoding: 'utf-8', | |
| }).trim(); | |
| expect(out).toBe(VERSION); | |
| }); | |
| it('prints version and exits with -v', () => { | |
| const out = execFileSync(process.execPath, [BIN, '-v'], { | |
| encoding: 'utf-8', | |
| }).trim(); | |
| expect(out).toBe(VERSION); | |
| }); | |
| it('prints version and exits with --version', () => { | |
| const out = execFileSync(process.execPath, [BIN, '--version'], { | |
| encoding: 'utf-8', | |
| timeout: 5000, | |
| }).trim(); | |
| expect(out).toBe(VERSION); | |
| }); | |
| it('prints version and exits with -v', () => { | |
| const out = execFileSync(process.execPath, [BIN, '-v'], { | |
| encoding: 'utf-8', | |
| timeout: 5000, | |
| }).trim(); | |
| expect(out).toBe(VERSION); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/version.test.js` around lines 24 - 36, The tests call
execFileSync(process.execPath, [BIN, '--version']) and
execFileSync(process.execPath, [BIN, '-v']) without a timeout, which can hang;
add a timeout option (e.g. { encoding: 'utf-8', timeout: 5000 }) to both
execFileSync invocations so the subprocess will be killed after a reasonable
period, keeping the assertions against VERSION the same and preventing CI hangs.
| # Parse arguments | ||
| while [ $# -gt 0 ]; do | ||
| case "$1" in | ||
| --prefix) | ||
| PREFIX="$2" | ||
| shift 2 | ||
| ;; |
There was a problem hiding this comment.
Validate --prefix has a value before shifting.
Missing a value causes shift 2 to abort with a shell error instead of a clear message.
Suggested fix
--prefix)
- PREFIX="$2"
- shift 2
+ if [ -z "$2" ]; then
+ echo "Error: --prefix requires a path" >&2
+ exit 1
+ fi
+ PREFIX="$2"
+ shift 2
;;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Parse arguments | |
| while [ $# -gt 0 ]; do | |
| case "$1" in | |
| --prefix) | |
| PREFIX="$2" | |
| shift 2 | |
| ;; | |
| # Parse arguments | |
| while [ $# -gt 0 ]; do | |
| case "$1" in | |
| --prefix) | |
| if [ -z "$2" ]; then | |
| echo "Error: --prefix requires a path" >&2 | |
| exit 1 | |
| fi | |
| PREFIX="$2" | |
| shift 2 | |
| ;; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@uninstall.sh` around lines 12 - 18, The --prefix case in uninstall.sh
currently does shift 2 without validating that a value exists; update the case
handling for "--prefix" (the while/case block and PREFIX assignment) to first
check that "$2" is non-empty and not another option, and if invalid print a
clear error message and exit non-zero instead of performing shift 2; after
validation assign PREFIX="$2" and then shift 2 as before so you avoid a shell
error when a value is missing.
| if [ -n "$PREFIX" ]; then | ||
| PREFIX=$(eval echo "$PREFIX") | ||
| npm uninstall -g --prefix "$PREFIX" "$PACKAGE" |
There was a problem hiding this comment.
Stop using eval for prefix expansion.
eval enables command injection with user-supplied input. Use safe tilde expansion instead.
Suggested fix
if [ -n "$PREFIX" ]; then
- PREFIX=$(eval echo "$PREFIX")
+ case "$PREFIX" in
+ "~"|"~/"*) PREFIX="${HOME}${PREFIX#\~}";;
+ esac
npm uninstall -g --prefix "$PREFIX" "$PACKAGE"
else📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ -n "$PREFIX" ]; then | |
| PREFIX=$(eval echo "$PREFIX") | |
| npm uninstall -g --prefix "$PREFIX" "$PACKAGE" | |
| if [ -n "$PREFIX" ]; then | |
| case "$PREFIX" in | |
| "~"|"~/"*) PREFIX="${HOME}${PREFIX#\~}";; | |
| esac | |
| npm uninstall -g --prefix "$PREFIX" "$PACKAGE" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@uninstall.sh` around lines 44 - 46, The script currently uses eval to expand
PREFIX which risks command injection; replace the eval-based expansion with safe
tilde expansion by detecting a leading tilde in PREFIX and replacing it with the
user's HOME (e.g. transform PREFIX when it starts with "~" into "$HOME/...");
keep the existing npm uninstall -g --prefix "$PREFIX" "$PACKAGE" call and ensure
PREFIX remains quoted afterwards to preserve spaces and prevent word-splitting
(refer to the PREFIX variable and the npm uninstall invocation to locate the
change).
Add src/update-check.js for non-blocking npm registry version checks with 24h cache TTL and 3s timeout. Wire --version/-v flag and update notification into CLI. Notification on stderr, suppressed in --json mode.
POSIX-compatible scripts supporting npm install -g and --prefix ~/.local. Checks Node >= 22 and npm availability. Uninstall includes optional --clean-cache to remove ~/.gitmind/ cache directory.
- createUpdateChecker(ports) with DI for fetchLatest, readCache, writeCache - defaultPorts() wires real fs + fetch + @git-stunts/alfred (lazy-loaded) - Alfred policy: 3s timeout wrapping 1 retry with decorrelated jitter - External AbortSignal threaded through retry for CLI cancellation - GIT_MIND_DISABLE_UPGRADE_CHECK env guard at composition root - Tests use injected fakes — no network, no filesystem - Split test scripts: npm test (unit), npm run test:integration (Docker)
CLI integration tests (contracts, content, version) now run in a Docker container with GIT_MIND_DISABLE_UPGRADE_CHECK=1 set at the image level. npm test runs unit tests only; npm run test:integration runs Docker.
Repository transferred to flyingrobots org. Updated npm package scope, GitHub URLs, schema $id fields, docs, and git remote.
Dead code — `registryPolicy` is created fresh in each `fetchLatest` call.
flyingrobots
force-pushed
the
feat/npm-publish-readiness
branch
from
e2842b3 to
720b6c4
Compare
1 participant
Summary
fileswhitelist,publishConfig, and version export for npm publish readiness (feat: npm install -g readiness — files field, publishConfig, --version flag #286)--versionflag and update-check notifications with hexagonal architecture and Alfred resilience (feat: update check notifications — non-blocking version detection #287)install.shanduninstall.shfor~/.local/binsupport (feat: install.sh and uninstall.sh for ~/.local/bin support #288)@neuroglyph/git-mindto@flyingrobots/git-mind(chore: transfer repo from neuroglyph to flyingrobots org #289)policyvariable lint violation (Dogfood git-mind suggest action with Claude CLI #293)Problem Statement
Package is not publishable to npm — missing files whitelist, publishConfig, version export, install scripts, and correct package name. Users cannot install via npm or run update checks.
ADR Compliance (Required)
Relevant ADR(s)
Compliance Declaration
Architecture Laws Checklist (Hard Gates)
Canonical Truth & Context
--at,--observer,--trust) or deterministically defaulted.Determinism & Provenance
Artifact Hygiene
Contracts & Compatibility
Extension/Effects Safety (if applicable)
Scope Control
Backward Compatibility
--versionflag (additive). Package renamed from@neuroglyph/git-mindto@flyingrobots/git-mind(breaking for existing installs).npm install @flyingrobots/git-mindinstead of old package name--versionflag prints version and exits. Update-check notification on CLI startup.Test Plan (Required)
Unit
npm test -- test/version.test.js test/update-check.test.jsIntegration
Determinism
npm testContract/Schema
Policy Gates
Security / Trust Impact
Performance Impact
Observability / Debuggability
Operational Notes
Linked Issues / Milestones
Reviewer Quick Verdict Block (for maintainers)
MUST (Hard Gates)
SHOULD (Quality)
Verdict