Skip to content

Commit abc4201

Browse files
mcp-tool-shopclaude
andcommitted
fix(health-c): humanize CLI errors + harden trust boundaries (+41 tests)
Stage-C humanization across kernel/memories/rules. Trust-boundary: shape-guard parsed index.json so a valid-but-wrong file emits a structured INVALID_INDEX/READ_FAILED instead of a raw TypeError/stack (KER-B1, RUL-B1, MEM-B01); memories rejects a directory target (NOT_A_FILE) instead of EISDIR (MEM-B02). Observability: kernel distinguishes malformed-vs-missing layers in resolve + counts skipped usage lines (KER-B2/B5); memories moves unresolved-ref reporting off console.warn into the CLI render path (MEM-B03). Shared flagValue bug fixed in all three (a value-flag no longer swallows the next --flag) (KER-B6/MEM-B05/RUL-B6). rules: --lazy now plumbed through validate/stats (RUL-B2) and drift issues carry fix hints + line numbers (RUL-B4). Feature-sized items (doctor cmd, per-command help, rules exports map, split atomicity) deferred to the Feature pass. Tests: 228 -> 269 (kernel 97->117, memories 42->54, rules 89->98). Exclusive ownership; build gate green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 43155a8 commit abc4201

22 files changed

Lines changed: 1196 additions & 61 deletions

packages/kernel/src/cli-helpers.ts

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
/**
2+
* Pure, side-effect-free helpers for the ai-loadout CLI.
3+
*
4+
* These live in their own module (separate from cli.ts) so they can be unit
5+
* tested without executing the CLI's top-level argv parsing / process.exit.
6+
* Nothing here writes to stdout or exits the process — callers decide how to
7+
* report. The CLI wires these into its structured `fail(code, message, hint)`.
8+
*/
9+
10+
import type { LoadoutIndex } from "./types.js";
11+
12+
// ── Index shape validation (KER-B1) ──────────────────────────────
13+
//
14+
// `JSON.parse` happily returns null / numbers / arrays / wrong-shaped objects.
15+
// Before we hand a parsed value to validate/budget/dead/overlaps/merge — all of
16+
// which dereference `.entries` — confirm it actually looks like a loadout index.
17+
// This turns a downstream raw `TypeError: Cannot read properties of … (reading
18+
// 'entries')` into an actionable, structured failure at the trust boundary.
19+
20+
/**
21+
* Structural type-guard for a parsed loadout index.
22+
*
23+
* Minimal by design: we only assert what every consumer relies on — a plain
24+
* object with an `entries` array. Per-entry validation is the job of
25+
* `validateIndex`; this guard just stops the obviously-wrong file (null, a
26+
* number, an array, `{}`, a missing `entries`) before it crashes elsewhere.
27+
*/
28+
export function isLoadoutIndex(value: unknown): value is LoadoutIndex {
29+
return (
30+
typeof value === "object" &&
31+
value !== null &&
32+
!Array.isArray(value) &&
33+
Array.isArray((value as { entries?: unknown }).entries)
34+
);
35+
}
36+
37+
/**
38+
* Human-readable description of why a parsed value is not a loadout index.
39+
*
40+
* Returned so the CLI can put a precise reason in the failure hint instead of a
41+
* generic "invalid". `null` means the value *is* a valid index (no problem).
42+
*/
43+
export function describeIndexShapeProblem(value: unknown): string | null {
44+
if (value === null) return "got null";
45+
if (Array.isArray(value)) return "got a JSON array, expected an object";
46+
const t = typeof value;
47+
if (t !== "object") return `got a JSON ${t}, expected an object`;
48+
if (!Array.isArray((value as { entries?: unknown }).entries)) {
49+
return "object is missing an 'entries' array";
50+
}
51+
return null;
52+
}
53+
54+
// ── Flag value parsing (KER-B6) ──────────────────────────────────
55+
//
56+
// `--project --json` must NOT let `--project` swallow `--json` as its value.
57+
// A value that itself starts with `--` is treated as absent: the user almost
58+
// certainly forgot the value, and silently consuming the next flag produces
59+
// confusing, hard-to-debug behavior downstream.
60+
61+
/**
62+
* Read the value of `--<flag>` from args, supporting both `--flag value` and
63+
* `--flag=value` forms.
64+
*
65+
* Guard: if the token following `--flag` itself starts with `--`, the value is
66+
* treated as ABSENT (returns `undefined`) rather than swallowing the next flag.
67+
* The `--flag=` form is honored verbatim, even if the value starts with `--`,
68+
* because there the user was explicit about the pairing.
69+
*/
70+
export function getFlagValue(args: string[], flag: string): string | undefined {
71+
const prefix = `--${flag}=`;
72+
for (const a of args) {
73+
if (a.startsWith(prefix)) return a.slice(prefix.length);
74+
}
75+
const idx = args.indexOf(`--${flag}`);
76+
if (idx !== -1 && idx + 1 < args.length && !args[idx + 1].startsWith("--")) {
77+
return args[idx + 1];
78+
}
79+
return undefined;
80+
}
81+
82+
/**
83+
* Classify why `getFlagValue` returned undefined for a bare `--flag`.
84+
*
85+
* Lets the CLI tailor its error message:
86+
* - "missing" → `--flag` was the last token, or absent entirely.
87+
* - "swallowed"→ `--flag` was immediately followed by another `--flag`,
88+
* so we refused to consume it as a value.
89+
* - null → a value WAS present (no problem).
90+
*/
91+
export function diagnoseFlagValue(
92+
args: string[],
93+
flag: string
94+
): "missing" | "swallowed" | null {
95+
if (getFlagValue(args, flag) !== undefined) return null;
96+
const idx = args.indexOf(`--${flag}`);
97+
if (idx === -1) return "missing";
98+
const next = args[idx + 1];
99+
if (next !== undefined && next.startsWith("--")) return "swallowed";
100+
return "missing";
101+
}

packages/kernel/src/cli.ts

Lines changed: 65 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,17 @@ import { readFileSync, existsSync } from "node:fs";
1414
import { resolve, dirname, join } from "node:path";
1515
import { fileURLToPath } from "node:url";
1616
import type { LoadoutIndex } from "./types.js";
17-
import { readUsage, summarizeUsage, summaryToJSON } from "./usage.js";
17+
import { readUsage, readUsageWithStats, summarizeUsage, summaryToJSON } from "./usage.js";
1818
import { findDeadEntries, findKeywordOverlaps, analyzeBudget } from "./analysis.js";
1919
import { resolveLoadout, explainEntry } from "./resolve.js";
2020
import type { ResolveOptions } from "./resolve.js";
2121
import { validateIndex } from "./validate.js";
22+
import {
23+
isLoadoutIndex,
24+
describeIndexShapeProblem,
25+
getFlagValue,
26+
diagnoseFlagValue,
27+
} from "./cli-helpers.js";
2228

2329
// ── Colors ────────────────────────────────────────────────────
2430
const BOLD = "\x1b[1m";
@@ -49,36 +55,60 @@ function positionalArgs(args: string[]): string[] {
4955
return args.filter((a) => !a.startsWith("--"));
5056
}
5157

52-
function getFlagValue(args: string[], flag: string): string | undefined {
53-
const prefix = `--${flag}=`;
54-
for (const a of args) {
55-
if (a.startsWith(prefix)) return a.slice(prefix.length);
56-
}
58+
// getFlagValue / diagnoseFlagValue live in cli-helpers.ts (testable, pure).
59+
60+
/**
61+
* Read a path-valued flag, failing with a clear, actionable error when the
62+
* value was swallowed by a following flag (e.g. `--project --json`). A truly
63+
* absent flag returns undefined (these flags all have sensible defaults).
64+
*/
65+
function getPathFlag(args: string[], flag: string): string | undefined {
66+
if (diagnoseFlagValue(args, flag) === "swallowed") {
67+
fail(
68+
"MISSING_FLAG_VALUE",
69+
`--${flag} was given without a value (the next token "--${nextToken(args, flag)}" is a flag, not a value).`,
70+
`Provide a value, e.g. '--${flag} <path>' or '--${flag}=<path>'.`
71+
);
72+
}
73+
return getFlagValue(args, flag);
74+
}
75+
76+
function nextToken(args: string[], flag: string): string {
5777
const idx = args.indexOf(`--${flag}`);
58-
if (idx !== -1 && idx + 1 < args.length && !args[idx + 1].startsWith("--")) {
59-
return args[idx + 1];
60-
}
61-
return undefined;
78+
return idx !== -1 ? (args[idx + 1] ?? "").replace(/^--/, "") : "";
6279
}
6380

6481
function getResolveOpts(args: string[]): ResolveOptions {
6582
return {
66-
projectRoot: getFlagValue(args, "project"),
67-
globalDir: getFlagValue(args, "global"),
68-
orgPath: getFlagValue(args, "org"),
69-
sessionPath: getFlagValue(args, "session"),
83+
projectRoot: getPathFlag(args, "project"),
84+
globalDir: getPathFlag(args, "global"),
85+
orgPath: getPathFlag(args, "org"),
86+
sessionPath: getPathFlag(args, "session"),
7087
};
7188
}
7289

7390
function loadIndex(path: string): LoadoutIndex {
7491
if (!existsSync(path)) {
7592
fail("FILE_NOT_FOUND", `Index not found: ${path}`);
7693
}
94+
let parsed: unknown;
7795
try {
78-
return JSON.parse(readFileSync(path, "utf-8")) as LoadoutIndex;
96+
parsed = JSON.parse(readFileSync(path, "utf-8"));
7997
} catch (e) {
8098
fail("PARSE_ERROR", `Failed to parse index: ${path}`, (e as Error).message);
8199
}
100+
// Trust boundary: valid JSON is not necessarily a valid index. Assert the
101+
// shape here so a wrong-but-parseable file fails with a clear, actionable
102+
// message instead of a raw TypeError later in validate/budget/dead/overlaps.
103+
if (!isLoadoutIndex(parsed)) {
104+
const problem = describeIndexShapeProblem(parsed);
105+
fail(
106+
"INVALID_INDEX",
107+
`File is valid JSON but not a loadout index: ${path}`,
108+
`Expected an object with an 'entries' array${problem ? ` (${problem})` : ""}. Run 'ai-loadout validate' for details.`
109+
);
110+
}
111+
return parsed;
82112
}
83113

84114
function getVersion(): string {
@@ -135,9 +165,15 @@ function cmdUsage(args: string[]) {
135165
}
136166

137167
const jsonlPath = resolve(positional[0]);
138-
const events = readUsage(jsonlPath);
168+
const { events, skipped } = readUsageWithStats(jsonlPath);
139169
const json = hasFlag(args, "json");
140170

171+
// Surface dropped lines so a corrupt log doesn't silently undercount.
172+
// (Stay quiet in --json mode to keep stdout machine-parseable.)
173+
if (skipped > 0 && !json) {
174+
warn(`Skipped ${skipped} malformed line(s) in ${jsonlPath}`);
175+
}
176+
141177
if (events.length === 0) {
142178
if (json) { log("[]"); return; }
143179
info("No usage events found");
@@ -346,14 +382,25 @@ function cmdResolve(args: string[]) {
346382
for (const s of result.searched) {
347383
if (s.found) {
348384
ok(`${s.name.padEnd(10)} ${DIM}${s.path}${RESET}`);
385+
} else if (s.malformed) {
386+
// Present but unparseable — call it out distinctly from a missing file
387+
// so the user knows there's a corrupt file to fix, not one to create.
388+
warn(`${s.name.padEnd(10)} ${DIM}${s.path}${RESET} ${YELLOW}(malformed JSON — skipped)${RESET}`);
349389
} else {
350390
log(` ${DIM}${RESET} ${s.name.padEnd(10)} ${DIM}${s.path} (not found)${RESET}`);
351391
}
352392
}
353393

354394
if (result.layers.length === 0) {
355-
log(`\n ${YELLOW}No loadout indexes found.${RESET}`);
356-
log(` ${DIM}Create .claude/loadout/index.json or ~/.ai-loadout/index.json${RESET}\n`);
395+
// Echo the ACTUAL searched paths rather than a hardcoded default list, so
396+
// the message reflects any --global/--org/--project/--session overrides and
397+
// tells the user exactly where to put a file.
398+
const searchedPaths = result.searched.map((s) => s.path);
399+
log(`\n ${YELLOW}No loadout indexes found in any of:${RESET}`);
400+
for (const p of searchedPaths) {
401+
log(` ${DIM}${p}${RESET}`);
402+
}
403+
log(` ${DIM}Create one of the above to get started.${RESET}\n`);
357404
return;
358405
}
359406

packages/kernel/src/index.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ export { validateIndex } from "./validate.js";
3333
export { mergeIndexes } from "./merge.js";
3434

3535
// ── Usage ─────────────────────────────────────────────────────
36-
export { recordUsage, readUsage, summarizeUsage, summaryToJSON } from "./usage.js";
36+
export { recordUsage, readUsage, readUsageWithStats, summarizeUsage, summaryToJSON } from "./usage.js";
3737
export type { UsageSummary, UsageSummaryJSON } from "./usage.js";
3838

3939
// ── Analysis ──────────────────────────────────────────────────

packages/kernel/src/resolve.ts

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,15 @@ export interface SearchedLayer {
3232
name: string;
3333
path: string;
3434
found: boolean;
35+
/**
36+
* The file exists on disk but its JSON could not be parsed. Distinct from
37+
* `found: false` (file absent) so callers don't report a present-but-corrupt
38+
* layer as "not found" — that's misleading. Additive/optional: undefined
39+
* means "not malformed" (either absent, or found and parsed fine).
40+
*/
41+
malformed?: boolean;
42+
/** Parse error message when `malformed` is true (for diagnostics). */
43+
error?: string;
3544
}
3645

3746
/** Result of resolving the full layer stack. */
@@ -119,9 +128,15 @@ export function discoverLayers(opts?: ResolveOptions): {
119128
const raw = readFileSync(path, "utf-8");
120129
const index = JSON.parse(raw) as LoadoutIndex;
121130
layers.push({ name, path, index });
122-
} catch {
123-
// Malformed file — skip silently, same as missing
124-
searched[searched.length - 1].found = false;
131+
} catch (e) {
132+
// Malformed file — skip it (don't sink the whole resolve), but record
133+
// that it was malformed (not merely absent) so callers can report the
134+
// difference. A present-but-corrupt file shown as "not found" hides a
135+
// problem the user needs to fix.
136+
const rec = searched[searched.length - 1];
137+
rec.found = false;
138+
rec.malformed = true;
139+
rec.error = (e as Error).message;
125140
}
126141
}
127142
}
Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
import { describe, it } from "node:test";
2+
import assert from "node:assert/strict";
3+
import {
4+
isLoadoutIndex,
5+
describeIndexShapeProblem,
6+
getFlagValue,
7+
diagnoseFlagValue,
8+
} from "../cli-helpers.js";
9+
10+
// ── isLoadoutIndex (KER-B1) ──────────────────────────────────────
11+
12+
describe("isLoadoutIndex (KER-B1 trust boundary)", () => {
13+
it("accepts an object with an entries array", () => {
14+
assert.equal(isLoadoutIndex({ entries: [] }), true);
15+
assert.equal(isLoadoutIndex({ version: "1.0.0", entries: [{ id: "a" }] }), true);
16+
});
17+
18+
it("rejects valid-but-wrong JSON that would crash downstream", () => {
19+
// These all parse as valid JSON yet would throw a raw TypeError the moment
20+
// something reads `.entries`. The guard catches them at the boundary.
21+
assert.equal(isLoadoutIndex(null), false);
22+
assert.equal(isLoadoutIndex(42), false);
23+
assert.equal(isLoadoutIndex("a string"), false);
24+
assert.equal(isLoadoutIndex(true), false);
25+
assert.equal(isLoadoutIndex({}), false);
26+
assert.equal(isLoadoutIndex([]), false);
27+
assert.equal(isLoadoutIndex([{ entries: [] }]), false);
28+
assert.equal(isLoadoutIndex({ entries: "not an array" }), false);
29+
assert.equal(isLoadoutIndex({ version: "1.0.0" }), false); // missing entries
30+
});
31+
});
32+
33+
describe("describeIndexShapeProblem (KER-B1 actionable hint)", () => {
34+
it("returns null for a valid index (no problem)", () => {
35+
assert.equal(describeIndexShapeProblem({ entries: [] }), null);
36+
});
37+
38+
it("names the specific problem for each wrong shape", () => {
39+
assert.match(describeIndexShapeProblem(null)!, /null/);
40+
assert.match(describeIndexShapeProblem([])!, /array/);
41+
assert.match(describeIndexShapeProblem(42)!, /number/);
42+
assert.match(describeIndexShapeProblem("x")!, /string/);
43+
assert.match(describeIndexShapeProblem({})!, /entries/);
44+
assert.match(describeIndexShapeProblem({ entries: 5 })!, /entries/);
45+
});
46+
});
47+
48+
// ── getFlagValue (KER-B6) ────────────────────────────────────────
49+
50+
describe("getFlagValue (KER-B6 defensive flag parsing)", () => {
51+
it("reads --flag value", () => {
52+
assert.equal(getFlagValue(["--project", "/tmp/x"], "project"), "/tmp/x");
53+
});
54+
55+
it("reads --flag=value", () => {
56+
assert.equal(getFlagValue(["--project=/tmp/x"], "project"), "/tmp/x");
57+
});
58+
59+
it("honors --flag=value even when value starts with -- (explicit pairing)", () => {
60+
assert.equal(getFlagValue(["--project=--json"], "project"), "--json");
61+
});
62+
63+
it("does NOT swallow a following flag as the value", () => {
64+
// `--project --json` previously returned "--json"; now it returns undefined
65+
// because the user clearly forgot the value for --project.
66+
assert.equal(getFlagValue(["--project", "--json"], "project"), undefined);
67+
});
68+
69+
it("returns undefined when the flag is the last token", () => {
70+
assert.equal(getFlagValue(["resolve", "--project"], "project"), undefined);
71+
});
72+
73+
it("returns undefined when the flag is absent", () => {
74+
assert.equal(getFlagValue(["resolve", "--json"], "project"), undefined);
75+
});
76+
});
77+
78+
describe("diagnoseFlagValue (KER-B6 error classification)", () => {
79+
it("returns null when a value is present", () => {
80+
assert.equal(diagnoseFlagValue(["--project", "/tmp/x"], "project"), null);
81+
assert.equal(diagnoseFlagValue(["--project=/tmp/x"], "project"), null);
82+
});
83+
84+
it("classifies a swallowed following flag", () => {
85+
assert.equal(diagnoseFlagValue(["--project", "--json"], "project"), "swallowed");
86+
});
87+
88+
it("classifies an absent or trailing flag as missing", () => {
89+
assert.equal(diagnoseFlagValue(["resolve"], "project"), "missing");
90+
assert.equal(diagnoseFlagValue(["resolve", "--project"], "project"), "missing");
91+
});
92+
});

0 commit comments

Comments
 (0)