Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors the action into an exported NpmDiffRunner and extracts Git, lockfile parsing, comparison, and formatting into new modules; adds TypeScript types, tests, and config files, updates README links, and centralizes outputs and error/early-exit handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as NpmDiffRunner
participant Git as GitService
participant Parser as LockfileParser
participant Comparer as comparePackages
participant Formatter as formatMarkdown
participant Core as ActionsCore
Runner->>Git: getMergeBase(baseRef)
alt merge-base found / fallback
Git-->>Runner: baseRevision
end
Runner->>Git: getChangedFiles(baseRevision)
Git-->>Runner: changedFiles[]
Runner->>Git: getFileAtRevision(baseRevision, lockfilePath)
Git-->>Runner: baseLockfileContent
Runner->>Git: getFileAtRevision(HEAD, lockfilePath)
Git-->>Runner: headLockfileContent
Runner->>Parser: parseLockfile(baseLockfileContent, includeTransitive)
Parser-->>Runner: basePackages
Runner->>Parser: parseLockfile(headLockfileContent, includeTransitive)
Parser-->>Runner: headPackages
Runner->>Comparer: comparePackages(basePackages, headPackages)
Comparer-->>Runner: Changes
Runner->>Formatter: formatMarkdown(Changes, includeTransitive)
Formatter-->>Runner: markdownDiff
Runner->>Core: setOutput(has_changes, diff, added_count, removed_count, updated_count)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/comparer.ts (1)
31-41: Consider potential key collisions in resolvedChanges Map.When multiple paths resolve to the same package name (e.g.,
node_modules/pkgandnode_modules/other/node_modules/pkg), only the last occurrence is retained in the Map. This may be intentional to deduplicate, but could silently lose version information for nested dependencies with different versions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/comparer.ts` around lines 31 - 41, ResolvedChanges currently uses package name as the Map key which causes collisions for nested packages sharing the same name; change the data model so entries are uniquely keyed or allow multiple entries per name (e.g., use a compound key including the package path like `${name}::${path}` or make resolvedChanges a Map<string, Entry[]> and push each occurrence). Update the place where entries are set (the block that calls resolvedChanges.set(name, ...)) to include the package path in the stored object and/or adjust the key creation, and update any consumers of resolvedChanges to iterate arrays or split the compound key to recover name/path so nested versions are preserved (refer to resolvedChanges, the set calls, and any readers of resolvedChanges).src/lockfile.parser.ts (2)
51-53: Unsafe type cast from LockfileData to PackageInfo.The cast
data as unknown as PackageInfoforces an incompatible type conversion.LockfileDatahaspackages,dependencies, etc., whilePackageInfoexpectsversion,name, and dependency maps with string values. This works at runtime because downstream code only accesses specific properties, but it bypasses type safety.♻️ Suggested improvement
Consider extracting only the relevant fields:
} else if (data.dependencies || data.devDependencies || data.optionalDependencies) { core.debug('Detected lockfile v1 (dependencies found)'); - packages[''] = data as unknown as PackageInfo; + packages[''] = { + dependencies: data.dependencies as Record<string, string> | undefined, + devDependencies: data.devDependencies as Record<string, string> | undefined, + optionalDependencies: data.optionalDependencies as Record<string, string> | undefined, + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lockfile.parser.ts` around lines 51 - 53, The branch that handles lockfile v1 currently does an unsafe cast "data as unknown as PackageInfo"; instead, construct a proper PackageInfo for the '' key by extracting the actual fields you need (e.g., map data.version/name if present and transform data.dependencies, data.devDependencies, data.optionalDependencies into string-to-string maps) rather than forcing the type cast; update the else-if handling that references data and packages[''] to build and assign a PackageInfo object (preserving keys like version, name and normalized dependency maps) so type safety is restored for PackageInfo consumers.
75-77: Silent error handling hinders debugging.Returning an empty object on parse failure without logging makes troubleshooting lockfile issues difficult. Consider logging the error before returning.
♻️ Suggested improvement
- } catch { + } catch (err: unknown) { + if (err instanceof Error) { + core.debug(`Failed to parse lockfile at ${path}: ${err.message}`); + } return {}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lockfile.parser.ts` around lines 75 - 77, The catch block in the lockfile parser that currently does "catch { return {} }" should capture the error (e.g., "catch (err)") and log it before returning so failures are visible; update the catch to log the error (using the project's logger or console.error with a clear message like "Failed to parse lockfile:" plus err) and then return the empty object to preserve behavior.test/functional/action.test.js (1)
9-15: Use unique temp dirs to avoid parallel test collisions.Line 10 uses a fixed directory name. If tests run in parallel or cleanup fails, this can cause flakes. Prefer
fs.mkdtempSyncwithos.tmpdir()(apply similarly to the second test’s temp dir).♻️ Suggested refactor
-const path = require('path'); +const path = require('path'); +const os = require('os'); ... -const tempDir = path.join(root, 'temp-functional-test-pkgjson'); +const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'npm-diff-pkgjson-'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/functional/action.test.js` around lines 9 - 15, Replace the fixed temp directory creation with unique OS temp dirs: instead of using the hardcoded tempDir variable and fs.mkdirSync, use fs.mkdtempSync(path.join(os.tmpdir(), 'temp-functional-test-pkgjson-')) to create a unique temp directory (update any variable names accordingly, e.g., tempDir). Apply the same change to the second test's temp directory creation so both tests use os.tmpdir() + fs.mkdtempSync and avoid collisions; ensure existing fs.existsSync/fs.rmSync cleanup logic targets the newly created unique temp path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/formatter.ts`:
- Around line 10-16: The npmdiff and npmjs links use the raw package name so
scoped packages like `@types/node` break the URL; update the link generation in
the formatter to URL-encode the package name (use encodeURIComponent or
equivalent) when building the npmdiff link
(`https://npmdiff.dev/${name}/${v1}/${v2}/`) and the npm details link
(`https://www.npmjs.com/package/${name}?activeTab=versions`) so the '/' in
scoped names becomes %2F; keep the existing v1/v2 normalization but substitute
encodedName where `name` is used.
In `@src/git.service.ts`:
- Around line 45-47: The getFileAtRevision method interpolates the
user-controlled path into a shell command, creating a shell injection risk;
update getFileAtRevision to validate or sanitize the path parameter (or switch
to a safe exec API that accepts an args array) before calling this.execute:
ensure the path contains only allowed characters (e.g., alphanumerics, /, -, _,
., and no shell metacharacters like ; & | $ ` > < *) or perform proper escaping,
and reject/throw for invalid values so only safe paths reach
getFileAtRevision(revision: string, path: string) and the underlying
this.execute call.
- Around line 27-35: The getMergeBase method interpolates baseRef directly into
shell commands (in getMergeBase and the execute call), creating a
command-injection risk; validate baseRef against a restrictive pattern (e.g.
allow only letters, digits, underscore, dash, dot and slash such as
/^[\w\-./]+$/) and throw an error when it doesn't match, or refactor execute to
run git with argument array (no shell interpolation) and call it like
execute(['merge-base', `origin/${baseRef}`, 'HEAD']) so that baseRef is never
passed through a shell; implement the validation or switch to argument-based
execution in the getMergeBase function and/or the execute helper to mitigate
injection.
In `@test/unit/action.test.js`:
- Around line 50-66: The test overwrites GitService.prototype.getMergeBase,
GitService.prototype.getChangedFiles, GitService.prototype.getFileAtRevision and
module functions lockfileParser.parseLockfile, comparer.comparePackages,
formatter.formatMarkdown in beforeEach but only restores fs/core in afterEach;
save the original references for each of those functions (e.g.,
originalGetMergeBase, originalGetChangedFiles, originalGetFileAtRevision,
originalParseLockfile, originalComparePackages, originalFormatMarkdown) before
mocking and restore them in afterEach, or alternatively call
jest.restoreAllMocks if using jest spies, ensuring
GitService.prototype.getMergeBase/getChangedFiles/getFileAtRevision and
lockfileParser.parseLockfile, comparer.comparePackages, formatter.formatMarkdown
are returned to their originals to avoid leakage into other tests.
---
Nitpick comments:
In `@src/comparer.ts`:
- Around line 31-41: ResolvedChanges currently uses package name as the Map key
which causes collisions for nested packages sharing the same name; change the
data model so entries are uniquely keyed or allow multiple entries per name
(e.g., use a compound key including the package path like `${name}::${path}` or
make resolvedChanges a Map<string, Entry[]> and push each occurrence). Update
the place where entries are set (the block that calls resolvedChanges.set(name,
...)) to include the package path in the stored object and/or adjust the key
creation, and update any consumers of resolvedChanges to iterate arrays or split
the compound key to recover name/path so nested versions are preserved (refer to
resolvedChanges, the set calls, and any readers of resolvedChanges).
In `@src/lockfile.parser.ts`:
- Around line 51-53: The branch that handles lockfile v1 currently does an
unsafe cast "data as unknown as PackageInfo"; instead, construct a proper
PackageInfo for the '' key by extracting the actual fields you need (e.g., map
data.version/name if present and transform data.dependencies,
data.devDependencies, data.optionalDependencies into string-to-string maps)
rather than forcing the type cast; update the else-if handling that references
data and packages[''] to build and assign a PackageInfo object (preserving keys
like version, name and normalized dependency maps) so type safety is restored
for PackageInfo consumers.
- Around line 75-77: The catch block in the lockfile parser that currently does
"catch { return {} }" should capture the error (e.g., "catch (err)") and log it
before returning so failures are visible; update the catch to log the error
(using the project's logger or console.error with a clear message like "Failed
to parse lockfile:" plus err) and then return the empty object to preserve
behavior.
In `@test/functional/action.test.js`:
- Around line 9-15: Replace the fixed temp directory creation with unique OS
temp dirs: instead of using the hardcoded tempDir variable and fs.mkdirSync, use
fs.mkdtempSync(path.join(os.tmpdir(), 'temp-functional-test-pkgjson-')) to
create a unique temp directory (update any variable names accordingly, e.g.,
tempDir). Apply the same change to the second test's temp directory creation so
both tests use os.tmpdir() + fs.mkdtempSync and avoid collisions; ensure
existing fs.existsSync/fs.rmSync cleanup logic targets the newly created unique
temp path.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
test/unit/git.service.test.js (1)
1-23: Consider adding tests for the remaining GitService methods.The tests for
execute()cover the core scenarios well. However, theGitServiceclass has three additional public methods (getMergeBase,getChangedFiles,getFileAtRevision) that lack unit test coverage:
getMergeBase: fallback logic from origin to local refgetChangedFiles: parsing and filtering of diff outputgetFileAtRevision: ignoreErrors behavior for missing filesAs per coding guidelines:
test/**: "Verify that the tests are comprehensive and cover the changes."Would you like me to generate unit tests for the remaining methods?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/git.service.test.js` around lines 1 - 23, Add unit tests covering GitService.getMergeBase, GitService.getChangedFiles, and GitService.getFileAtRevision: for getMergeBase, mock git.execute to return an origin ref first and verify fallback to local ref when origin call fails; for getChangedFiles, mock git.execute to return a diff --name-only string including additions, deletions and subpaths and assert parsing/filtering produces the expected array (including path filters and deduping); for getFileAtRevision, test successful retrieval by mocking git.execute to return file contents and test ignoreErrors=true returns empty string when git.execute errors while ignoreErrors=false throws; use the existing test harness pattern (instantiate new GitService()) and stub/spy on execute to control outputs.src/lockfile.parser.ts (1)
75-77: Consider logging parse errors before returning empty object.Silent error swallowing makes debugging difficult when lockfile parsing fails unexpectedly. Consider adding a debug log to help diagnose issues.
🔧 Suggested improvement
- } catch { + } catch (err) { + core.debug(`Failed to parse lockfile at ${path}: ${err instanceof Error ? err.message : String(err)}`); return {}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lockfile.parser.ts` around lines 75 - 77, The catch block currently swallows parse errors and returns {} (the try/catch around lockfile parsing) — change the catch to capture the error (e.g., catch (err)) and log the error before returning, using the module’s existing logger (e.g., logger.debug or processLogger.debug) or console.debug if no logger is available; keep the existing return {} behavior after logging to preserve current flow.test/unit/formatter.test.js (1)
6-33: Add a scoped-package link test.Once link encoding is fixed, add a test to assert correct encoding for
@scope/pkg.✅ Suggested test
+test('getCompareLink encodes scoped package names', () => { + const link = getCompareLink('@scope/pkg', '1.0.0', '1.1.0'); + assert.strictEqual(link, '[Compare](https://npmdiff.dev/%40scope%2Fpkg/1.0.0/1.1.0/)'); +});As per coding guidelines,
test/**: Verify that the tests are comprehensive and cover the changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/formatter.test.js` around lines 6 - 33, Add a unit test that verifies link encoding for scoped packages (e.g. "@scope/pkg") using getCompareLink and formatMarkdown: call getCompareLink('@scope/pkg', '1.0.0', '1.1.0') and assert the returned Compare URL encodes the slash (or uses the expected encoded form), and add a case in the formatMarkdown test data with added/updated/removed entries for '@scope/pkg' to assert the generated table rows contain the correctly encoded npmjs/npmdiff links; update or add assertions similar to existing tests referencing getCompareLink and formatMarkdown to ensure scoped-package links are encoded properly.test/unit/lockfile.parser.test.js (1)
6-98: Add coverage for nested transitive filtering and parse errors.The parser explicitly drops nested
.../node_modules/.../node_modules/...entries and returns{}on malformed JSON, but those branches aren't exercised here. Adding tests will guard those behaviors from regressions.✅ Suggested tests
+test('parseLockfile excludes nested transitive deps', () => { + const content = JSON.stringify({ + packages: { + '': { dependencies: { direct: '1.0.0' } }, + 'node_modules/direct': { version: '1.0.0' }, + 'node_modules/direct/node_modules/transitive': { version: '2.0.0' }, + }, + }); + + const packages = parseLockfile('package-lock.json', content); + assert.ok(packages['node_modules/direct']); + assert.strictEqual(packages['node_modules/direct/node_modules/transitive'], undefined); +}); + +test('parseLockfile returns empty on invalid JSON', () => { + const packages = parseLockfile('package-lock.json', '{'); + assert.deepStrictEqual(packages, {}); +});As per coding guidelines,
test/**: Verify that the tests are comprehensive and cover the changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/lockfile.parser.test.js` around lines 6 - 98, Add two unit tests to test/unit/lockfile.parser.test.js targeting parseLockfile: (1) a test that includes nested paths like "node_modules/pkg/node_modules/nested" in a package-lock v3 "packages" object and asserts those nested entries are dropped (not returned) to exercise the explicit filtering of nested node_modules; (2) a test that passes malformed JSON (e.g. an invalid string) to parseLockfile and asserts it returns an empty object {} to cover the error-handling branch. Use the same test harness/assert style as the existing tests and reference parseLockfile in both tests.test/unit/comparer.test.js (1)
32-74: Add a test that locks in nestednode_modulesbehavior.To prevent regressions around nested transitive dependencies, add a case where the same package exists in both top-level and nested paths and only the nested one changes.
✅ Suggested test
+test('comparePackages keeps nested node_modules paths distinct', () => { + const base = { + 'node_modules/dep': { version: '1.0.0' }, + 'node_modules/a/node_modules/dep': { version: '1.0.0' }, + }; + const head = { + 'node_modules/dep': { version: '1.0.0' }, + 'node_modules/a/node_modules/dep': { version: '1.1.0' }, + }; + + const changes = comparePackages(base, head); + assert.ok(changes.updated.some((c) => c.name === 'a/node_modules/dep')); +});As per coding guidelines,
test/**: Verify that the tests are comprehensive and cover the changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/comparer.test.js` around lines 32 - 74, Add a new unit test in comparer.test.js that exercises comparePackages with a package present both at top-level ('': dependencies) and in a nested path ('node_modules/pkg' and 'node_modules/some-module/node_modules/pkg'), where only the nested package version or constraint changes; call comparePackages and assert that the reported update refers only to the nested package (identify by path or by distinguishing properties your comparePackages uses), and that the top-level package is not reported as changed; reference the comparePackages function and the test file to locate where to add this case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/comparer.ts`:
- Around line 12-75: The current logic collapses nested node_modules by using
the stripped `name` (last node_modules suffix) as the map key (see
resolvedChanges, `name = key.replace(...)`) which merges distinct paths like
`node_modules/a/node_modules/dep` with `node_modules/dep`; change to use the
full `key` as the Map key (e.g., `resolvedChanges.set(key, ...)`) and introduce
a separate displayName (compute from `key` but do not drop nested segments) for
user-facing output so nested packages remain distinct; update all lookups and
dedup checks (e.g., `resolvedChanges.has(...)`, any checks that used `depName`
or `name`) to use the full-path key while preserving existing `name`/displayName
fields for presentation, and ensure the constraint-change logic that checks
`resolvedChanges.has(depName)` uses the corresponding full-path keys or a
mapping from dependency names to full paths.
In `@src/lockfile.parser.ts`:
- Around line 51-62: The root issue is that the code assigns the whole
LockfileData object to packages[''] by casting data as PackageInfo; instead,
construct and assign a proper PackageInfo-shaped object for the root package
(packages['']) rather than casting the whole lockfile, and normalize each v1
dependency entry when populating packages[name] from v1Deps so they conform to
the PackageInfo structure (e.g., ensure fields like version, name, and
dependencies are set or derived). Update the logic around packages[''], v1Deps
and the loop that assigns packages[name] to create/validate PackageInfo objects
instead of blindly copying LockfileData or raw dependency values.
---
Duplicate comments:
In `@src/formatter.ts`:
- Around line 3-16: getCompareLink builds URLs using the raw package name
(stored in the local variable name) and raw sanitized versions v1/v2, which
breaks for scoped packages containing "/" — fix by URL-encoding the path
components: pass name through encodeURIComponent when interpolating into both
the compare URL and the npm details URL, and also encode v1 and v2 (or at least
ensure they are safe) before concatenation; update getCompareLink to use
encodeURIComponent(name) and encodeURIComponent(v1)/encodeURIComponent(v2) when
constructing the links.
In `@src/git.service.ts`:
- Around line 27-35: The getMergeBase method interpolates baseRef into a shell
command, creating a command-injection risk; validate baseRef against the safe
pattern (/^[\w\-.\/]+$/) before calling this.execute and throw or return an
explicit error when it fails validation, then use the validated value in the
existing calls to this.execute (both the origin and local fallback branches) to
ensure only safe ref strings are passed to git merge-base.
- Around line 45-47: getFileAtRevision currently interpolates the
user-controlled path into a shell string, allowing command injection; change it
to stop using shell interpolation and instead call the underlying executor with
a safe argument list or sanitized path (e.g., use execute with an args array or
a child_process.execFile/spawn style call), and/or validate/normalize the path
to reject traversal or unsafe characters; update the getFileAtRevision
implementation (and adjust the execute helper if needed) so it passes revision
and path as separate, non-interpolated arguments to git instead of embedding
them into a single shell string.
In `@test/unit/action.test.js`:
- Around line 12-66: Save the original references to the mocked functions before
overwriting them and restore them in afterEach: capture
GitService.prototype.getMergeBase, GitService.prototype.getChangedFiles,
GitService.prototype.getFileAtRevision and the module exports
lockfileParser.parseLockfile, comparer.comparePackages, formatter.formatMarkdown
in variables (e.g., originalGetMergeBase, originalGetChangedFiles,
originalGetFileAtRevision, originalParseLockfile, originalComparePackages,
originalFormatMarkdown) and then reassign those originals back in afterEach so
the tests do not leak mocked implementations.
---
Nitpick comments:
In `@src/lockfile.parser.ts`:
- Around line 75-77: The catch block currently swallows parse errors and returns
{} (the try/catch around lockfile parsing) — change the catch to capture the
error (e.g., catch (err)) and log the error before returning, using the module’s
existing logger (e.g., logger.debug or processLogger.debug) or console.debug if
no logger is available; keep the existing return {} behavior after logging to
preserve current flow.
In `@test/unit/comparer.test.js`:
- Around line 32-74: Add a new unit test in comparer.test.js that exercises
comparePackages with a package present both at top-level ('': dependencies) and
in a nested path ('node_modules/pkg' and
'node_modules/some-module/node_modules/pkg'), where only the nested package
version or constraint changes; call comparePackages and assert that the reported
update refers only to the nested package (identify by path or by distinguishing
properties your comparePackages uses), and that the top-level package is not
reported as changed; reference the comparePackages function and the test file to
locate where to add this case.
In `@test/unit/formatter.test.js`:
- Around line 6-33: Add a unit test that verifies link encoding for scoped
packages (e.g. "@scope/pkg") using getCompareLink and formatMarkdown: call
getCompareLink('@scope/pkg', '1.0.0', '1.1.0') and assert the returned Compare
URL encodes the slash (or uses the expected encoded form), and add a case in the
formatMarkdown test data with added/updated/removed entries for '@scope/pkg' to
assert the generated table rows contain the correctly encoded npmjs/npmdiff
links; update or add assertions similar to existing tests referencing
getCompareLink and formatMarkdown to ensure scoped-package links are encoded
properly.
In `@test/unit/git.service.test.js`:
- Around line 1-23: Add unit tests covering GitService.getMergeBase,
GitService.getChangedFiles, and GitService.getFileAtRevision: for getMergeBase,
mock git.execute to return an origin ref first and verify fallback to local ref
when origin call fails; for getChangedFiles, mock git.execute to return a diff
--name-only string including additions, deletions and subpaths and assert
parsing/filtering produces the expected array (including path filters and
deduping); for getFileAtRevision, test successful retrieval by mocking
git.execute to return file contents and test ignoreErrors=true returns empty
string when git.execute errors while ignoreErrors=false throws; use the existing
test harness pattern (instantiate new GitService()) and stub/spy on execute to
control outputs.
In `@test/unit/lockfile.parser.test.js`:
- Around line 6-98: Add two unit tests to test/unit/lockfile.parser.test.js
targeting parseLockfile: (1) a test that includes nested paths like
"node_modules/pkg/node_modules/nested" in a package-lock v3 "packages" object
and asserts those nested entries are dropped (not returned) to exercise the
explicit filtering of nested node_modules; (2) a test that passes malformed JSON
(e.g. an invalid string) to parseLockfile and asserts it returns an empty object
{} to cover the error-handling branch. Use the same test harness/assert style as
the existing tests and reference parseLockfile in both tests.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/git.service.ts (1)
10-10: Shell escaping may be incomplete for certain metacharacters.The current escaping (
arg.replace(/"/g, '\\"')) only handles double quotes but doesn't escape$, backticks, or backslashes, which can still be interpreted inside double-quoted strings. SincegetMergeBase,getFileAtRevision, and their callers validate inputs, this is mitigated for those paths, but any future direct calls toexecute()with untrusted input could be vulnerable.Consider using a more complete escaping approach or switching to
execFileSyncwith an array of arguments (no shell) for stronger guarantees.♻️ Optional: Use execFileSync to avoid shell interpolation entirely
-import { execSync, StdioOptions } from 'child_process'; +import { execFileSync, StdioOptions } from 'child_process'; import * as core from '@actions/core'; export class GitService { private readonly maxBuffer = 50 * 1024 * 1024; execute(args: string[], options: { stdio?: StdioOptions; ignoreErrors?: boolean } = {}): string { core.debug(`Executing command: git ${args.join(' ')}`); try { - return execSync(`git ${args.map((arg) => `"${arg.replace(/"/g, '\\"')}"`).join(' ')}`, { + return execFileSync('git', args, { maxBuffer: this.maxBuffer, stdio: options.stdio || ['pipe', 'pipe', 'ignore'], }) .toString() .trim();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/git.service.ts` at line 10, The current execute() call builds a shell command using execSync with ad-hoc escaping (arg.replace(...)) which misses metacharacters like $, backticks, and backslashes; update execute() to avoid shell interpolation by switching to execFileSync (or spawnSync) and pass the git binary and the args array directly, or implement a robust escaping routine that covers $, backticks, backslashes, and quotes; update callers such as getMergeBase and getFileAtRevision to continue passing raw args (no quoting) so execute() can safely forward them as an argument array to execFileSync/child_process.spawnSync.test/unit/git.service.test.js (1)
1-66: Missing test coverage forgetChangedFiles.The test file covers
execute,getMergeBase, andgetFileAtRevision, but there's no test forgetChangedFiles. Consider adding tests to verify:
- Correct parsing of diff output into file list
- Handling of empty diff output
- Filtering of empty lines
📝 Example test to add
test('GitService.getChangedFiles returns array of changed files', () => { const git = new GitService(); // This test would require a known commit range in the test repo // or mocking execute - consider integration test approach const files = git.getChangedFiles('HEAD~1'); assert.ok(Array.isArray(files)); });As per coding guidelines, tests should be comprehensive and cover the changes. The
getChangedFilesmethod is part of the newGitServiceclass but lacks dedicated test coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/git.service.test.js` around lines 1 - 66, Add unit tests for GitService.getChangedFiles: create tests that instantiate GitService and mock its execute method to return representative git diff output (e.g., lines with file paths separated by newlines, empty lines, and no output) and then assert getChangedFiles returns an array of file paths, returns an empty array for empty diff output, and filters out any empty lines; reference the GitService class and the getChangedFiles and execute methods when implementing these tests and name them clearly (e.g., "GitService.getChangedFiles returns array of changed files", "GitService.getChangedFiles returns empty array for no changes", "GitService.getChangedFiles filters empty lines").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/git.service.ts`:
- Around line 49-54: The getFileAtRevision method validates path but not
revision, which allows injection into the git "show" argument; add the same
sanity check for revision (e.g., ensure it matches /^[\w\-./]+$/ and does not
contain ':' or other unsafe chars) and throw an Error with a clear message if
invalid, then only call this.execute(['show', `${revision}:${path}`], {
ignoreErrors: true }) after both revision and path are validated.
- Around line 41-47: getChangedFiles interpolates the user-controlled
baseRevision into a git command without validation; update getChangedFiles to
validate or sanitize baseRevision the same way getMergeBase and
getFileAtRevision do (reuse their validation helper or add the same checks)
before calling this.execute(['diff', '--name-only', baseRevision, 'HEAD']); if
validation fails throw an error or return an empty array to avoid passing
untrusted input to the git command.
---
Nitpick comments:
In `@src/git.service.ts`:
- Line 10: The current execute() call builds a shell command using execSync with
ad-hoc escaping (arg.replace(...)) which misses metacharacters like $,
backticks, and backslashes; update execute() to avoid shell interpolation by
switching to execFileSync (or spawnSync) and pass the git binary and the args
array directly, or implement a robust escaping routine that covers $, backticks,
backslashes, and quotes; update callers such as getMergeBase and
getFileAtRevision to continue passing raw args (no quoting) so execute() can
safely forward them as an argument array to
execFileSync/child_process.spawnSync.
In `@test/unit/git.service.test.js`:
- Around line 1-66: Add unit tests for GitService.getChangedFiles: create tests
that instantiate GitService and mock its execute method to return representative
git diff output (e.g., lines with file paths separated by newlines, empty lines,
and no output) and then assert getChangedFiles returns an array of file paths,
returns an empty array for empty diff output, and filters out any empty lines;
reference the GitService class and the getChangedFiles and execute methods when
implementing these tests and name them clearly (e.g.,
"GitService.getChangedFiles returns array of changed files",
"GitService.getChangedFiles returns empty array for no changes",
"GitService.getChangedFiles filters empty lines").
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/comparer.ts`:
- Around line 85-90: The current suppression logic uses hasResolvedChange =
(depNameToPaths.get(depName) || []).some(path => resolvedChanges.has(path))
which includes nested node_modules paths and lets transitive updates hide direct
dependency constraint changes; change this to only consider
non-nested/direct/workspace paths when computing hasResolvedChange (e.g., filter
depNameToPaths.get(depName) to exclude nested paths like those containing
multiple "node_modules" segments or use a helper isDirectPath(path) that only
returns true for top-level/workspace locations) so the if (baseConstraint !==
headConstraint && !hasResolvedChange) check only gets suppressed by
direct/workspace resolvedChanges, not transitive ones.
In `@src/lockfile.parser.ts`:
- Around line 54-71: The root PackageInfo construction and v1 dependency
handling are incorrect: set packages[''].name and .version from top-level
data.name and data.version (not data.packages['']) and normalize all v1
dependency constraints to strings so comparePackages sees plain version strings;
when iterating v1Deps (the v1Deps object and loop that assigns packages[name] =
info), replace object assignments with a PackageInfo-like entry where
dependency/version fields use info.version (or String(info) if needed) so
packages[name] becomes a string constraint (or contains a string .version)
rather than an object.
In `@test/unit/comparer.test.js`:
- Around line 71-92: Add a new unit test for comparePackages that simulates a
transitive package update inside a nested node_modules (e.g.,
'node_modules/other/node_modules/pkg' version changes) while the root package
constraint for 'pkg' (in the '' dependencies) also changes; assert that
comparePackages returns both the transitive update (updated entry for 'pkg' with
oldVersion/newVersion) and a separate reported constraint change for the root
dependency constraint, using the existing test pattern (see comparer.test.js
test that uses comparePackages and checks changes.updated) to follow naming and
assertion style.
In `@test/unit/git.service.test.js`:
- Around line 12-19: Update the test "GitService.execute handles arguments with
spaces" so it actually passes an argument containing spaces to exercise quoting
logic: call GitService.execute with the -c form and a value that includes spaces
(e.g., git.execute(['-c', 'user.name=Alice Smith', 'version'])) and keep the
assertion that output matches /git version/; this verifies GitService.execute
handles arguments with embedded spaces.
In `@test/unit/lockfile.parser.test.js`:
- Around line 46-59: The test assumes v1 root dependencies remain objects with a
.version field, but parseLockfile normalizes v1 root dependencies to string
version constraints; update the assertions in test('parseLockfile handles
lockfile v1 dependencies') to expect the string '1.1.0' for both packages['pkg']
and packages[''].dependencies.pkg (remove the .version access) so the test
reflects the normalized output from parseLockfile.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/unit/git.service.test.js (2)
89-106: Consider adding happy path tests forgetMergeBase,getChangedFiles, andgetFileAtRevision.The current tests cover input validation (negative cases) thoroughly, but there are no tests verifying correct behavior with valid inputs:
getMergeBase: No test for successful execution or the fallback behavior (triesorigin/${baseRef}first, then falls back to local ref)getChangedFiles: No test verifying it returns a properly split, trimmed, and filtered array of filenamesgetFileAtRevision: No test verifying it returns file content or handles missing files gracefully (returns'')As per coding guidelines, "test/**: Verify that the tests are comprehensive and cover the changes."
Would you like me to generate additional test cases for these happy paths?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/git.service.test.js` around lines 89 - 106, Add "happy path" unit tests for getMergeBase, getChangedFiles, and getFileAtRevision: for getMergeBase create tests that mock the underlying git calls to succeed when resolving origin/${baseRef} and another test where origin/${baseRef} fails and the local ref is used (verify returned merge base value and that fallback was attempted); for getChangedFiles mock the git diff output and assert the method returns a trimmed, split array with empty lines filtered and no surrounding whitespace (include cases with trailing/leading spaces and blank lines); for getFileAtRevision mock the git show/cat-file output to assert it returns file content and another test where the file is missing to assert it returns an empty string. Reference the GitService class methods getMergeBase, getChangedFiles, and getFileAtRevision and use the same stubbing/mocking approach used elsewhere in tests (e.g., stubbing child_process exec or the GitService helper that executes git).
4-4: Inconsistent import: remove.tsextension.Other test files (e.g.,
test/unit/action.test.jsline 6) import the same module without the.tsextension:const { GitService } = require('../../src/git.service');Using the explicit
.tsextension here is inconsistent and may fail depending on the test runner configuration.🔧 Suggested fix
-const { GitService } = require('../../src/git.service.ts'); +const { GitService } = require('../../src/git.service');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/git.service.test.js` at line 4, The test imports GitService with an explicit ".ts" extension which is inconsistent and may break some runners; in the test that requires GitService, replace the require call that references "git.service.ts" with the same require without the ".ts" extension so it uses "git.service" (i.e., update the require for GitService to remove the ".ts" suffix) to match other tests and ensure consistent module resolution.src/lockfile.parser.ts (1)
89-91: Consider logging parse errors for debugging.The empty catch silently returns an empty object, which is resilient but could hide issues during development or debugging.
💡 Optional: add debug logging
- } catch { + } catch (error) { + core.debug(`Failed to parse lockfile ${path}: ${error}`); return {}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lockfile.parser.ts` around lines 89 - 91, The catch block in src/lockfile.parser.ts currently swallows errors and returns {} — update it to capture the exception (e.g., catch (err)) and emit a debug/error log with context (e.g., "Failed to parse lockfile") before returning {}; use the project logger if available (or console.debug/console.error) so the parse failure is visible while preserving the fallback return {}.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lockfile.parser.ts`:
- Around line 54-61: The root PackageInfo construction assigns
data.dependencies/devDependencies/optionalDependencies directly but LockfileData
v1 may have values as PackageInfo objects, not strings; normalize these maps
before assigning to packages[''] so each dependency value is a string
version/specifier (e.g., map PackageInfo|string -> string by extracting .version
or appropriate field), then assign the normalized records to packages[''];
update the code paths that feed comparePackages to ensure it receives string
constraints (references: packages[''] and comparePackages, and the LockfileData
type).
In `@test/unit/git.service.test.js`:
- Around line 12-17: The test currently passes an argument with spaces to
GitService.execute but then runs a command that ignores the config, so it
doesn't verify quoting; update the test to run a git subcommand that echoes the
configured value (e.g., replace the 'version' argument with a command like
'config' and key 'user.name') so the call to GitService.execute(['-c',
'user.name=Alice Smith', 'config', 'user.name']) returns the configured string
and assert that the output equals or contains "Alice Smith"; this will exercise
GitService.execute's quoting logic and reference the GitService.execute method
to locate the test change.
---
Nitpick comments:
In `@src/lockfile.parser.ts`:
- Around line 89-91: The catch block in src/lockfile.parser.ts currently
swallows errors and returns {} — update it to capture the exception (e.g., catch
(err)) and emit a debug/error log with context (e.g., "Failed to parse
lockfile") before returning {}; use the project logger if available (or
console.debug/console.error) so the parse failure is visible while preserving
the fallback return {}.
In `@test/unit/git.service.test.js`:
- Around line 89-106: Add "happy path" unit tests for getMergeBase,
getChangedFiles, and getFileAtRevision: for getMergeBase create tests that mock
the underlying git calls to succeed when resolving origin/${baseRef} and another
test where origin/${baseRef} fails and the local ref is used (verify returned
merge base value and that fallback was attempted); for getChangedFiles mock the
git diff output and assert the method returns a trimmed, split array with empty
lines filtered and no surrounding whitespace (include cases with
trailing/leading spaces and blank lines); for getFileAtRevision mock the git
show/cat-file output to assert it returns file content and another test where
the file is missing to assert it returns an empty string. Reference the
GitService class methods getMergeBase, getChangedFiles, and getFileAtRevision
and use the same stubbing/mocking approach used elsewhere in tests (e.g.,
stubbing child_process exec or the GitService helper that executes git).
- Line 4: The test imports GitService with an explicit ".ts" extension which is
inconsistent and may break some runners; in the test that requires GitService,
replace the require call that references "git.service.ts" with the same require
without the ".ts" extension so it uses "git.service" (i.e., update the require
for GitService to remove the ".ts" suffix) to match other tests and ensure
consistent module resolution.
… fixed data integrity and quality
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lockfile.parser.ts`:
- Around line 10-21: The normalizeV1Deps function misclassifies a PackageInfo
with an empty version string because the current truthy check on (info as
PackageInfo).version fails and the code casts the object to string producing
"[object Object]"; update the check inside normalizeV1Deps to detect the
presence of a version key regardless of its truthiness (e.g., use 'version' in
info or typeof (info as PackageInfo).version === 'string' or
hasOwnProperty('version')) and then assign normalized[name] = (info as
PackageInfo).version as string; otherwise keep the existing fallback that treats
info as a string.
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
1 participant
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.
This PR refactors the core action logic into a modular architecture to improve maintainability, readability, and testability.
Key Changes:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores