Skip to content

Commit 3d54878

Browse files
committed
fix(audit): address PR #33 CodeRabbit feedback (1 Major + 3 Minor)
All 4 verified correct against the actual code; all applied. Major: - audit-engine.ts: validate baseline rows_json is an array before diffing. JSON.parse("null") / "{}" / "true" all parse successfully but produce non-array values that crash computeDelta on .length / .map. New explicit check returns a structured AuditError naming the actual type so the user knows what to re-save. Tests cover both the null and object cases. Minor: - README.md: add a fourth example line showing the prefix + per-delta override composition (`--baseline base --files-baseline hotfix-files`). The behavior is implemented + tested in resolveAuditBaselines but the README only showed the standalone forms. - cmd-audit.ts: harden consumeFlagValue's two-token path to reject empty-string values (`--flag ""`) and whitespace-only values (`--flag " "`). The `--flag=` path was already strict; the positional path silently accepted these and the failure surfaced later as a less-clear baseline-not-found error. Two new parser tests cover both cases. - SKILL.md (mirrored across .agents/ and templates/agents/ per Rule 10): add the "no slot resolves → exit 1" failure mode to the Audit subsection. The "silently absent" wording read like --baseline <prefix> could produce an empty envelope; clarified that audit errors instead of doing nothing.
1 parent 752f6c2 commit 3d54878

7 files changed

Lines changed: 79 additions & 4 deletions

File tree

.agents/skills/codemap/SKILL.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ Replace placeholders (`'...'`) with your module path, file glob, or symbol name.
4848

4949
**Audit (`bun src/index.ts audit`)** — separate top-level command for structural-drift verdicts. Composes B.6 baselines into a per-delta `{head, deltas}` envelope; v1 ships `files` / `dependencies` / `deprecated`. Two snapshot-source shapes:
5050

51-
- **`--baseline <prefix>`** — auto-resolves `<prefix>-files` / `<prefix>-dependencies` / `<prefix>-deprecated` in `query_baselines`. Slots that don't exist are silently absent (the convention-following user just saves what they need).
51+
- **`--baseline <prefix>`** — auto-resolves `<prefix>-files` / `<prefix>-dependencies` / `<prefix>-deprecated` in `query_baselines`. Slots that don't exist are silently absent (the convention-following user just saves what they need). **If no slot resolves at all** (every auto-resolved name is missing AND no `--<delta>-baseline` flag is passed), audit exits 1 — never produces an empty envelope.
5252
- **`--<delta>-baseline <name>`** — explicit per-delta override (e.g. `--files-baseline X --dependencies-baseline Y`). Names must exist or audit exits 1. Composes with `--baseline` (per-delta flag overrides one slot).
5353

5454
Each emitted delta carries its own `base` metadata so mixed-baseline audits are first-class. `--summary` collapses each delta to `{added: N, removed: N}`. `--no-index` skips the auto-incremental-index prelude (default is to re-index first so `head` reflects current source). v1 ships no `verdict` / threshold config — `codemap audit --json | jq -e '.deltas.dependencies.added | length <= 50'` is the CI exit-code idiom until v1.x ships native thresholds. Each delta pins a canonical SQL projection and validates baseline column-set membership before diffing — schema-bump-resilient (extras dropped, missing columns surface a clean re-save command).

README.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@ codemap query --save-baseline=base-deprecated -r deprecated-symbols
101101
codemap audit --baseline base # auto-resolves base-{files,dependencies,deprecated}
102102
codemap audit --json --summary --baseline base # counts-only — useful for CI dashboards
103103
codemap audit --files-baseline base-files # explicit per-delta — runs only the slots provided
104+
codemap audit --baseline base --files-baseline hotfix-files # mixed — auto-resolve deps + deprecated; override files
104105
codemap audit --baseline base --no-index # skip the auto-incremental-index prelude (frozen-DB CI)
105106
# Recipes that define per-row action templates append "actions" hints (kebab-case verb +
106107
# description) in --json output; ad-hoc SQL never carries actions. Inspect via --recipes-json.

src/application/audit-engine.test.ts

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,53 @@ describe("runAudit (engine)", () => {
142142
}
143143
});
144144

145+
it("rejects a baseline whose rows_json parses to non-array (null)", () => {
146+
const db = freshDb();
147+
try {
148+
upsertQueryBaseline(db, {
149+
name: "null-rows",
150+
recipe_id: null,
151+
sql: "SELECT 1",
152+
rows_json: "null",
153+
row_count: 0,
154+
git_ref: null,
155+
created_at: 1,
156+
});
157+
const result = runAudit({ db, baselines: { files: "null-rows" } });
158+
expect(result).toHaveProperty("error");
159+
if ("error" in result) {
160+
expect(result.error).toContain('"null-rows"');
161+
expect(result.error).toContain("invalid rows_json");
162+
expect(result.error).toContain("null");
163+
}
164+
} finally {
165+
db.close();
166+
}
167+
});
168+
169+
it("rejects a baseline whose rows_json parses to non-array (object)", () => {
170+
const db = freshDb();
171+
try {
172+
upsertQueryBaseline(db, {
173+
name: "object-rows",
174+
recipe_id: null,
175+
sql: "SELECT 1",
176+
rows_json: "{}",
177+
row_count: 0,
178+
git_ref: null,
179+
created_at: 1,
180+
});
181+
const result = runAudit({ db, baselines: { files: "object-rows" } });
182+
expect(result).toHaveProperty("error");
183+
if ("error" in result) {
184+
expect(result.error).toContain("invalid rows_json");
185+
expect(result.error).toContain("object");
186+
}
187+
} finally {
188+
db.close();
189+
}
190+
});
191+
145192
it("propagates a column-mismatch error from a delta", () => {
146193
const db = freshDb();
147194
try {

src/application/audit-engine.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,13 @@ export function runAudit(opts: {
160160

161161
let baselineRows: unknown[];
162162
try {
163-
baselineRows = JSON.parse(baseline.rows_json) as unknown[];
163+
const parsed = JSON.parse(baseline.rows_json) as unknown;
164+
if (!Array.isArray(parsed)) {
165+
return {
166+
error: `codemap audit: baseline "${baseline.name}" (delta "${spec.key}") has invalid rows_json (expected JSON array, got ${parsed === null ? "null" : typeof parsed}) — drop and re-save.`,
167+
};
168+
}
169+
baselineRows = parsed;
164170
} catch {
165171
return {
166172
error: `codemap audit: baseline "${baseline.name}" (delta "${spec.key}") has corrupt rows_json — drop and re-save.`,

src/cli/cmd-audit.test.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,18 @@ describe("parseAuditRest", () => {
132132
if (r.kind === "error") expect(r.message).toContain("--files-baseline");
133133
});
134134

135+
it("errors when --baseline gets an empty-string value (two-token form)", () => {
136+
const r = parseAuditRest(["audit", "--baseline", ""]);
137+
expect(r.kind).toBe("error");
138+
if (r.kind === "error") expect(r.message).toContain("--baseline");
139+
});
140+
141+
it("errors when --files-baseline gets a whitespace-only value", () => {
142+
const r = parseAuditRest(["audit", "--files-baseline", " "]);
143+
expect(r.kind).toBe("error");
144+
if (r.kind === "error") expect(r.message).toContain("--files-baseline");
145+
});
146+
135147
it("errors on unknown options", () => {
136148
const r = parseAuditRest(["audit", "--unknown", "x", "--baseline", "n"]);
137149
expect(r.kind).toBe("error");

src/cli/cmd-audit.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,16 @@ function consumeFlagValue(
128128
return { kind: "value", value: v, next: i + 1 };
129129
}
130130
const next = rest[i + 1];
131-
if (next === undefined || next.startsWith("-")) {
131+
// `next === ""` catches the two-token empty-string case (`--flag ""`); the
132+
// `--flag=` case is already caught above. Trim-zero check covers whitespace-
133+
// only values (`--flag " "`) — those would silently sneak through to a
134+
// baseline lookup that fails further downstream with a less clear error.
135+
if (
136+
next === undefined ||
137+
next === "" ||
138+
next.trim().length === 0 ||
139+
next.startsWith("-")
140+
) {
132141
return {
133142
kind: "error",
134143
message: `codemap audit: "${flagName}" requires a value.`,

templates/agents/skills/codemap/SKILL.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ Replace placeholders (`'...'`) with your module path, file glob, or symbol name.
4848

4949
**Audit (`codemap audit`)** — separate top-level command for structural-drift verdicts. Composes B.6 baselines into a per-delta `{head, deltas}` envelope; v1 ships `files` / `dependencies` / `deprecated`. Two snapshot-source shapes:
5050

51-
- **`--baseline <prefix>`** — auto-resolves `<prefix>-files` / `<prefix>-dependencies` / `<prefix>-deprecated` in `query_baselines`. Slots that don't exist are silently absent (the convention-following user just saves what they need).
51+
- **`--baseline <prefix>`** — auto-resolves `<prefix>-files` / `<prefix>-dependencies` / `<prefix>-deprecated` in `query_baselines`. Slots that don't exist are silently absent (the convention-following user just saves what they need). **If no slot resolves at all** (every auto-resolved name is missing AND no `--<delta>-baseline` flag is passed), audit exits 1 — never produces an empty envelope.
5252
- **`--<delta>-baseline <name>`** — explicit per-delta override (e.g. `--files-baseline X --dependencies-baseline Y`). Names must exist or audit exits 1. Composes with `--baseline` (per-delta flag overrides one slot).
5353

5454
Each emitted delta carries its own `base` metadata so mixed-baseline audits are first-class. `--summary` collapses each delta to `{added: N, removed: N}`. `--no-index` skips the auto-incremental-index prelude (default is to re-index first so `head` reflects current source). v1 ships no `verdict` / threshold config — `codemap audit --json | jq -e '.deltas.dependencies.added | length <= 50'` is the CI exit-code idiom until v1.x ships native thresholds. Each delta pins a canonical SQL projection and validates baseline column-set membership before diffing — schema-bump-resilient (extras dropped, missing columns surface a clean re-save command).

0 commit comments

Comments
 (0)