fix: address PR #74 CodeRabbit review (6 findings, all real)

SutuSebastian · SutuSebastian · commit 33a23b191597 · 2026-05-05T15:45:30.000+03:00
Fact-checked each finding against the codebase; all valid. Two were
critical correctness bugs (T2, T3, T5) that would have shipped silently
broken behavior.

T2 (Major) — `scripts/detect-pm.mjs` wrong package-name keys

`codemapInDevDependencies` checked `manifest?.dependencies?.codemap` but
the published package is `@stainless-code/codemap` — the project-installed
branch was dead code for all real consumers; `dlx-pinned` / `dlx-latest`
also pulled the unscoped `codemap` package (a different npm namespace
entirely). Fixed:

- Lookup checks both scoped (`@stainless-code/codemap`) and bare
  (`codemap`) keys — the latter for monorepos that alias via
  `"codemap": "workspace:*"`.
- `'execute'` (dlx) commandArgs now use the scoped published name so
  npm/pnpm/yarn/bun resolve the right registry entry.
- `'execute-local'` keeps `["codemap"]` because that's the bin alias
  per `package.json#bin`, regardless of the scoped name.
- Tests updated: scoped-dev-dep / bare-dev-dep / scoped-dlx-pinned
  cases. Old tests that asserted dlx-with-unscoped-`codemap@&lt;v&gt;` were
  themselves testing a bug.

T3 (Critical) — no-op `expect()` in formatAuditSarif test

`expect(run.results.every(...))` without a chained matcher creates and
discards the expectation. If formatAuditSarif reverted to severity
`note`, the test would still pass. Added `.toBe(true)`.

T5 (Major) — `require()` in ESM module

`readStdinSync` in `cmd-pr-comment.ts` used `require("node:fs").readSync`
but the file loads via `await import()` (ESM); `require` is undefined at
runtime. `codemap pr-comment -` would have crashed for every user. My
unit tests passed file paths, not stdin — caught nothing. Imported
`readSync` from `node:fs` at the top, used directly.

T4 (Major) — pr-comment SARIF renderer dropped runs[1+]

`renderSarifComment` only read `doc.runs?.[0]?.results`. Valid SARIF
allows multiple runs (merged / multi-tool docs). Now flattens via
`(doc.runs ?? []).flatMap(run =&gt; run.results ?? [])`. New test
`aggregates results across multi-run SARIF docs (not just runs[0])`.

T1 (Minor) — `mode: command` without `command:` input falls through

The validate step accepted `mode=command` but didn't guard against
empty `command:`. Run step's if/elif only handled `audit` + `recipe`,
so `$EXEC` would invoke with empty `$ARGS`. Added an explicit guard
mirroring the `mode=recipe` pattern.

T6 (Minor) — `--ci` doc said "no-findings warning"; actual is "no-locatable-rows"

Both `.agents/rules/codemap.md` and `templates/agents/rules/codemap.md`
described the suppressed warning incorrectly. Aligned wording with the
implementation in `printFormattedQuery`.
diff --git a/.agents/rules/codemap.md b/.agents/rules/codemap.md
@@ -37,7 +37,7 @@ A local database (default **`.codemap/index.db`**) indexes structure: symbols, i
 | Impact (blast-radius walker)   | —                  | `bun src/index.ts impact <target> [--direction up\|down\|both] [--depth N] [--via <b>] [--limit N] [--summary] [--json]` — replaces hand-composed `WITH RECURSIVE` queries                                                                                                                                 |
 | Coverage ingest                | —                  | `bun src/index.ts ingest-coverage <path> [--json]` — Istanbul (`coverage-final.json`) or LCOV (`lcov.info`); format auto-detected. Joinable to `symbols` for "untested AND dead" queries.                                                                                                                  |
 | SARIF / GH annotations         | —                  | `bun src/index.ts query --recipe deprecated-symbols --format sarif` · `… --format annotations`                                                                                                                                                                                                             |
-| `--ci` aggregate flag          | —                  | `bun src/index.ts query -r deprecated-symbols --ci` (or `audit --base origin/main --ci`) — aliases `--format sarif` + non-zero exit when findings/additions surfaced + suppresses no-findings stderr warning. Mutually exclusive with `--json` / `--format <other>`.                                       |
+| `--ci` aggregate flag          | —                  | `bun src/index.ts query -r deprecated-symbols --ci` (or `audit --base origin/main --ci`) — aliases `--format sarif` + non-zero exit when findings/additions surfaced + suppresses the no-locatable-rows stderr warning. Mutually exclusive with `--json` / `--format <other>`.                             |
 | PR-comment renderer            | —                  | `bun src/index.ts pr-comment <input.json>` (or `-` for stdin) — renders an audit JSON envelope or SARIF doc as a markdown PR-summary comment. Pipe to `gh pr comment <PR> -F -`. Useful for private repos without GHAS, aggregate audit deltas, or bot-context seeding.                                    |
 | Mermaid graph (≤50 edges)      | —                  | `bun src/index.ts query --format mermaid 'SELECT from_path AS "from", to_path AS "to" FROM dependencies LIMIT 50'` — recipes / SQL must alias columns to `{from, to, label?, kind?}`; rejects unbounded inputs.                                                                                            |
 | Diff preview                   | —                  | `bun src/index.ts query --format diff '<SQL returning file_path, line_start, before_pattern, after_pattern>'` — read-only unified diff; `--format diff-json` returns structured hunks for agents.                                                                                                          |
diff --git a/action.yml b/action.yml
@@ -180,6 +180,11 @@ runs:
           exit 1
         fi
 
+        if [ "$MODE" = "command" ] && [ -z "$COMMAND" ]; then
+          echo "::error::codemap action: mode='command' requires the 'command' input (raw CLI args)."
+          exit 1
+        fi
+
     - name: Run codemap
       if: steps.gate.outputs.skip != 'true'
       id: run
diff --git a/scripts/detect-pm.mjs b/scripts/detect-pm.mjs
@@ -41,21 +41,23 @@ async function main() {
     agent = detected?.agent ?? "npm";
   }
 
-  // Per Q3's three-branch resolution (docs/plans/github-marketplace-action.md).
+  // Per Q3 (docs/plans/github-marketplace-action.md). `execute-local` resolves
+  // the `codemap` bin alias; `execute` (dlx) needs the scoped registry name.
+  const PUBLISHED_NAME = "@stainless-code/codemap";
   let intent;
   let commandArgs;
   let installMethod;
   if (versionInput !== "") {
     intent = "execute";
-    commandArgs = [`codemap@${versionInput}`];
+    commandArgs = [`${PUBLISHED_NAME}@${versionInput}`];
     installMethod = "dlx-pinned";
   } else if (codemapInDevDependencies(workingDir)) {
     intent = "execute-local";
     commandArgs = ["codemap"];
     installMethod = "project-installed";
   } else {
     intent = "execute";
-    commandArgs = ["codemap@latest"];
+    commandArgs = [`${PUBLISHED_NAME}@latest`];
     installMethod = "dlx-latest";
   }
 
@@ -82,16 +84,24 @@ async function main() {
   );
 }
 
-/** False on read errors / missing manifest. */
+// Scoped published name + bare bin name (workspace aliases use the latter).
+const CODEMAP_DEP_KEYS = ["@stainless-code/codemap", "codemap"];
+
 function codemapInDevDependencies(workingDir) {
   try {
     const manifestPath = join(workingDir, "package.json");
     if (!existsSync(manifestPath)) return false;
     const manifest = JSON.parse(readFileSync(manifestPath, "utf8"));
-    return Boolean(
-      manifest?.dependencies?.codemap ||
-      manifest?.devDependencies?.codemap ||
-      manifest?.optionalDependencies?.codemap,
+    const buckets = [
+      manifest?.dependencies,
+      manifest?.devDependencies,
+      manifest?.optionalDependencies,
+    ];
+    return buckets.some(
+      (b) =>
+        b !== null &&
+        b !== undefined &&
+        CODEMAP_DEP_KEYS.some((k) => b[k] !== undefined),
     );
   } catch {
     return false;
diff --git a/scripts/detect-pm.test.mjs b/scripts/detect-pm.test.mjs
@@ -86,30 +86,43 @@ describe("scripts/detect-pm.mjs", () => {
     expect(out.install_method).toBe("dlx-latest");
   });
 
-  it("uses execute-local when codemap is in devDependencies", () => {
-    const dir = makeFixture("dev-dep-fixture", {
+  it("uses execute-local when @stainless-code/codemap is in devDependencies (scoped name)", () => {
+    const dir = makeFixture("scoped-dev-dep-fixture", {
       "package.json": JSON.stringify({
-        devDependencies: { codemap: "^1.0.0" },
+        devDependencies: { "@stainless-code/codemap": "^1.0.0" },
       }),
       "package-lock.json": "{}",
     });
     const out = runDetect({ WORKING_DIRECTORY: dir });
     expect(out.agent).toBe("npm");
     expect(out.install_method).toBe("project-installed");
+    // bin alias is `codemap` regardless of the scoped package name
     expect(out.exec).toContain("codemap");
-    expect(out.exec).not.toContain("codemap@");
+    expect(out.exec).not.toContain("@stainless-code/codemap@");
   });
 
-  it("uses dlx-pinned when version input is set (overrides project install)", () => {
+  it("uses execute-local when bare `codemap` key is set (workspace alias case)", () => {
+    const dir = makeFixture("bare-dev-dep-fixture", {
+      "package.json": JSON.stringify({
+        devDependencies: { codemap: "workspace:*" },
+      }),
+      "package-lock.json": "{}",
+    });
+    const out = runDetect({ WORKING_DIRECTORY: dir });
+    expect(out.install_method).toBe("project-installed");
+  });
+
+  it("uses dlx-pinned with scoped published name when version input is set", () => {
     const dir = makeFixture("pinned-fixture", {
       "package.json": JSON.stringify({
-        devDependencies: { codemap: "^1.0.0" },
+        devDependencies: { "@stainless-code/codemap": "^1.0.0" },
       }),
       "package-lock.json": "{}",
     });
     const out = runDetect({ WORKING_DIRECTORY: dir, VERSION: "1.2.3" });
     expect(out.install_method).toBe("dlx-pinned");
-    expect(out.exec).toContain("codemap@1.2.3");
+    // dlx must use the scoped name so the right registry entry resolves
+    expect(out.exec).toContain("@stainless-code/codemap@1.2.3");
   });
 
   it("respects PACKAGE_MANAGER override", () => {
diff --git a/src/application/output-formatters.test.ts b/src/application/output-formatters.test.ts
@@ -610,7 +610,9 @@ describe("formatAuditSarif", () => {
     ]);
     expect(run.results).toHaveLength(3);
     // Severity = warning (audit deltas are more actionable than per-recipe `note`)
-    expect(run.results.every((r: { level: string }) => r.level === "warning"));
+    expect(
+      run.results.every((r: { level: string }) => r.level === "warning"),
+    ).toBe(true);
     // Locations auto-detected per row
     expect(
       run.results[0].locations[0].physicalLocation.artifactLocation.uri,
diff --git a/src/application/pr-comment-engine.test.ts b/src/application/pr-comment-engine.test.ts
@@ -217,4 +217,33 @@ describe("renderSarifComment", () => {
     expect(r.findings_count).toBe(75);
     expect(r.markdown).toContain("… and 25 more");
   });
+
+  it("aggregates results across multi-run SARIF docs (not just runs[0])", () => {
+    // SARIF spec allows multiple runs (merged / multi-tool); aggregator
+    // shouldn't drop entries from runs[1+].
+    const r = renderSarifComment({
+      version: "2.1.0",
+      runs: [
+        {
+          tool: {
+            driver: { name: "codemap", rules: [{ id: "rule.a", name: "a" }] },
+          },
+          results: [{ ruleId: "rule.a", message: { text: "from run 0" } }],
+        },
+        {
+          tool: {
+            driver: { name: "other", rules: [{ id: "rule.b", name: "b" }] },
+          },
+          results: [
+            { ruleId: "rule.b", message: { text: "from run 1" } },
+            { ruleId: "rule.b", message: { text: "another from run 1" } },
+          ],
+        },
+      ],
+    });
+    expect(r.findings_count).toBe(3);
+    expect(r.markdown).toContain("from run 0");
+    expect(r.markdown).toContain("from run 1");
+    expect(r.markdown).toContain("another from run 1");
+  });
 });
diff --git a/src/application/pr-comment-engine.ts b/src/application/pr-comment-engine.ts
@@ -146,7 +146,8 @@ export function renderSarifComment(doc: SarifDocument): RenderedComment {
   lines.push("## codemap findings");
   lines.push("");
 
-  const results = doc.runs?.[0]?.results ?? [];
+  // SARIF supports multi-run docs (merged / multi-tool); flatten so we don't under-report.
+  const results = (doc.runs ?? []).flatMap((run) => run.results ?? []);
   if (results.length === 0) {
     lines.push("✅ No findings.");
     return {
diff --git a/src/cli/cmd-pr-comment.ts b/src/cli/cmd-pr-comment.ts
@@ -1,4 +1,4 @@
-import { readFileSync } from "node:fs";
+import { readFileSync, readSync } from "node:fs";
 
 import {
   detectCommentInputShape,
@@ -199,7 +199,7 @@ function readStdinSync(): string {
   while (true) {
     let n: number;
     try {
-      n = require("node:fs").readSync(0, buffer, 0, buffer.length, null);
+      n = readSync(0, buffer, 0, buffer.length, null);
     } catch {
       break;
     }
diff --git a/templates/agents/rules/codemap.md b/templates/agents/rules/codemap.md
@@ -42,7 +42,7 @@ Install **[@stainless-code/codemap](https://www.npmjs.com/package/@stainless-cod
 | Impact (blast-radius walker)      | `codemap impact <target> [--direction up\|down\|both] [--depth N] [--via <b>] [--limit N] [--summary] [--json]` — replaces hand-composed `WITH RECURSIVE` queries                                                                                                                                 |
 | Coverage ingest                   | `codemap ingest-coverage <path> [--json]` — Istanbul (`coverage-final.json`) or LCOV (`lcov.info`); format auto-detected. Joinable to `symbols` for "untested AND dead" queries.                                                                                                                  |
 | SARIF / GH annotations            | `codemap query --recipe deprecated-symbols --format sarif` · `… --format annotations`                                                                                                                                                                                                             |
-| `--ci` aggregate flag             | `codemap query -r deprecated-symbols --ci` (or `audit --base origin/main --ci`) — aliases `--format sarif` + non-zero exit when findings/additions surfaced + suppresses no-findings stderr warning. Mutually exclusive with `--json` / `--format <other>`.                                       |
+| `--ci` aggregate flag             | `codemap query -r deprecated-symbols --ci` (or `audit --base origin/main --ci`) — aliases `--format sarif` + non-zero exit when findings/additions surfaced + suppresses the no-locatable-rows stderr warning. Mutually exclusive with `--json` / `--format <other>`.                             |
 | PR-comment renderer               | `codemap pr-comment <input.json>` (or `-` for stdin) — renders an audit JSON envelope or SARIF doc as a markdown PR-summary comment. Pipe to `gh pr comment <PR> -F -`. Useful for private repos without GHAS, aggregate audit deltas, or bot-context seeding.                                    |
 | Mermaid graph (≤50 edges)         | `codemap query --format mermaid 'SELECT from_path AS "from", to_path AS "to" FROM dependencies LIMIT 50'` — recipes / SQL must alias columns to `{from, to, label?, kind?}`; rejects unbounded inputs.                                                                                            |
 | Diff preview                      | `codemap query --format diff '<SQL returning file_path, line_start, before_pattern, after_pattern>'` — read-only unified diff; `--format diff-json` returns structured hunks for agents.                                                                                                          |
