Skip to content

Commit 5e46a7c

Browse files
phodalQoder-AI
andcommitted
fix(harness-component-snapshot): keep bounded diffs and usage errors useful
Post-merge review of pull request 69 found that a bounded diff sorted entries by component ID only, so `unchanged` entries consumed the limit and a small `--limit` could truncate away the only real difference while still reporting `truncated: true`. Entries now order `changed`, `added`, and `removed` before `unchanged`, falling back to the canonical component ID order so results stay deterministic. Counts already covered every component and are unchanged. CLI usage failures collapsed every argument problem into one opaque message. The sanitization was protecting real privacy, because an unrecognized command or option is caller-supplied argv that can hold a private path, but it also discarded safe detail. Usage diagnostics now name the specific reason and, for missing-value, duplicate, and missing-required failures, the allowlisted flag, while never echoing an unrecognized token or any option value. Two review findings are documentation rather than behavior: the v1 `relationships` set is fully derivable from `components`, which is now recorded in `contract.mjs` and HCS-AC-5 as a deliberate extension point instead of looking like accidental redundancy; and byte-level revisions differ across operating systems when line-ending normalization differs, which the spec now carries as an explicit risk against cross-platform `--population-key` diffs. `.gitignore` re-includes `test/fixtures/**/.qoder/`. Those fixtures were only tracked because they had been force-added, so new fixture assets under that path were invisible to `git status`. Symlink-tolerance consistency and the hard-coded read surface in `assertProjectReadBoundaries` are recorded in the spec as deferred findings rather than changed here, since each is a design decision beyond this slice. Validated with `npm test` at 1290 passing, 0 failing, 0 skipped, doc links 6/6, `npm run pack:verify` at 458 npm and 480 runtime-zip entries, and a manual `diff --limit 1` check confirming the changed entry now survives truncation. Co-authored-by: QoderAI (Qwen 3.8 Max) <qoder_ai@qoder.com>
1 parent 692f415 commit 5e46a7c

8 files changed

Lines changed: 110 additions & 18 deletions

File tree

.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
.DS_Store
22
.qoder/
3+
!test/fixtures/**/.qoder/
34
node_modules/
45
dist/
56
outputs/

docs/specs/2026-08-02-harness-component-snapshot-v1.md

Lines changed: 47 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,10 @@ contract before generalizing across providers.
5757
- **HCS-AC-5 (minimal graph):** Each component has one typed `declared-in`
5858
relationship to its privacy-safe workspace artifact reference. Relationship
5959
validation rejects unknown component IDs, absolute targets, and mismatched
60-
provenance instead of inferring semantic dependencies.
60+
provenance instead of inferring semantic dependencies. In v1 this set is fully
61+
derivable from `components`; it stays a serialized, validated field because the
62+
typed relationship shape is the extension point for later relationship types,
63+
so adding one must not be a breaking schema change.
6164
- **HCS-AC-6 (tamper-safe validation):** Validation recomputes component
6265
identity, relationship integrity, rollback references, ordering, and the
6366
snapshot digest. Stale or edited snapshots fail with stable diagnostic codes.
@@ -66,8 +69,11 @@ contract before generalizing across providers.
6669
counts. Revision changes are named `content`; route identity changes appear
6770
as one removal and one addition. Activation and provenance remain fixed,
6871
explicit evidence dimensions in this v1 producer and therefore cannot be
69-
reported as changed without a future contract revision. Returned entries
70-
obey an explicit bounded limit and report truncation. Different populations
72+
reported as changed without a future contract revision. Counts always describe
73+
every component, while returned entries obey an explicit bounded limit and
74+
report truncation. Entries order `changed`, `added`, and `removed` before
75+
`unchanged`, then by canonical component ID, so a small limit cannot hide the
76+
actual differences behind redundant unchanged entries. Different populations
7177
fail closed.
7278
- **HCS-AC-8 (non-authorizing rollback reference):** Every component exposes a
7379
parseable reference binding provider, scope, population-qualified component
@@ -81,9 +87,12 @@ contract before generalizing across providers.
8187
`--population-key`, `validate`, `diff`, and `resolve` with parser-safe JSON
8288
stdout and usage/runtime failures on stderr. Runtime diagnostics expose a
8389
stable code without echoing caller-selected paths or untrusted snapshot
84-
fields. Public API failures use stable sanitized messages without appending
85-
filesystem or parser error details. Global and leaf help remain help-only
86-
human text and do not inspect a workspace.
90+
fields. Usage diagnostics name the specific failure reason and may name an
91+
allowlisted flag, but never echo an unrecognized command, unrecognized option,
92+
or any option value, because those are caller-supplied argv that can hold a
93+
private path. Public API failures use stable sanitized messages without
94+
appending filesystem or parser error details. Global and leaf help remain
95+
help-only human text and do not inspect a workspace.
8796
- **HCS-AC-10 (portable evidence):** Fixtures cover Windows, macOS, and Linux
8897
route forms, stable input ordering, provider/scope isolation, secret and home
8998
sentinels, hook activation, tampering, bounded diff states, and rollback
@@ -148,7 +157,9 @@ from accepted host evidence to the component snapshot contract.
148157
rollback-reference mismatches.
149158
- **HCS-AC-9:** `node --test test/harness-component-snapshot-cli.test.mjs`
150159
verifies help, strict parsing, JSON stdout, path-safe stderr failures, and all
151-
four direct CLI operations. Contract tests also verify that malformed Hook
160+
four direct CLI operations. A dedicated case asserts that usage failures name
161+
an allowlisted flag while never echoing an unrecognized command, unrecognized
162+
option, or option value. Contract tests also verify that malformed Hook
152163
input cannot expose parser details, caller paths, or private sentinels through
153164
the public API error message.
154165
- **HCS-AC-10:** `node --test test/doc-link-graph.test.mjs`,
@@ -165,6 +176,25 @@ from accepted host evidence to the component snapshot contract.
165176
follow-up additionally replaced per-read workspace-root resolution with a
166177
snapshot-scoped boundary and removed parser/filesystem details from public
167178
Hook configuration errors.
179+
- **Post-merge review follow-up (2026-08-05):** A maintainer review after the
180+
merge of pull request 69 corrected the observed repository-wide count above: on
181+
the merged tree the run reports 1,289 passing with 0 failures and 0 skips on
182+
macOS, not 1,288 passing with 1 skip. Four review findings were then applied on
183+
`main`: bounded diff entries
184+
now order significant statuses before `unchanged`; usage diagnostics name their
185+
reason and any allowlisted flag; the v1 relationship redundancy and its
186+
extension-point rationale are recorded in `contract.mjs` and HCS-AC-5; and
187+
`.gitignore` re-includes `test/fixtures/**/.qoder/` so fixture assets are no
188+
longer invisible to `git status`. Re-verification passed 1,290 tests with 0
189+
failures and 0 skips, documentation links 6/6, and package verification at 458
190+
npm and 480 runtime-zip entries.
191+
- **Deferred review findings (2026-08-05):** Two findings remain open rather than
192+
fixed here, because each is a design decision beyond this slice. Symbolic-link
193+
tolerance is inconsistent: probed collector roots and Skill trees reject any
194+
symlink, while `workspaceFileEvidence` accepts one whose realpath stays inside
195+
the workspace. `assertProjectReadBoundaries` also hard-codes the read surface of
196+
`qoderWorkspaceRuleSources`, so a new rule source added in `agent-customize`
197+
would silently narrow the fail-closed guarantee without failing a test.
168198
- **Risk — privacy:** A route or source field could expose an absolute home.
169199
Mitigation: accept only normalized workspace-relative routes and serialize
170200
no inventory path fields.
@@ -182,6 +212,16 @@ from accepted host evidence to the component snapshot contract.
182212
files cannot cross-diff or resolve rollback references. Repositories that
183213
relocate or compare across operating systems must provide the same opaque
184214
population key; only its domain-separated digest enters artifacts.
215+
- **Risk — line-ending normalization across operating systems:** A revision is a
216+
digest of raw file bytes, so the same commit checked out with `core.autocrlf`
217+
on Windows and with LF on Linux produces different revisions for every text
218+
component. A cross-operating-system diff that shares one explicit
219+
`--population-key` would then report each component as a `content` change.
220+
Mitigation for this repository: `.gitattributes` pins `eol=lf` for `.md`,
221+
`.json`, `.mjs`, `.js`, `.yaml`, and `.yml`. Snapshotted consumer projects
222+
carry no such guarantee, so cross-operating-system comparison requires a
223+
matching checkout normalization; byte-identical revisions are not claimed
224+
across differing line-ending configurations.
185225
- **Risk — incomplete rollback:** A privacy-safe snapshot cannot itself restore
186226
omitted source bytes. The v1 reference is resolvable and non-authorizing;
187227
executable restore remains gated on a future content-store or Git provenance

scripts/harness-component-snapshot/README.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,11 @@ node scripts/harness-component-snapshot/cli.mjs resolve --snapshot before.json -
2020
valid snapshot. Its result always sets `mutationAuthorized` to `false`; this
2121
capability neither stores source bodies nor restores files.
2222

23+
`diff` counts always describe every component, while `entries` is bounded by
24+
`--limit`. Entries list `changed`, `added`, and `removed` before `unchanged`, so
25+
a small limit truncates redundant unchanged entries rather than the actual
26+
differences; `truncated` and `totalEntries` report what was dropped.
27+
2328
Without `--population-key`, the population reference is a privacy-safe digest
2429
of the canonical workspace boundary, so two unrelated workspaces cannot be
2530
cross-diffed accidentally. Use the same opaque key only when two locations are

scripts/harness-component-snapshot/cli.mjs

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -26,30 +26,35 @@ Operational commands emit one JSON document on stdout; help emits this text.
2626

2727
const COMMANDS = new Set(["create", "validate", "diff", "resolve"]);
2828

29+
// Usage reasons stay explicit, but only allowlisted flag names may appear in
30+
// them. Unrecognized commands and options are caller-supplied argv that can
31+
// hold a private path, so they are reported without their value.
2932
class UsageError extends Error {
30-
constructor(message) {
31-
super(message);
33+
constructor(reason, safeFlag) {
34+
super(safeFlag ? `${reason}: ${safeFlag}` : reason);
3235
this.code = "INVALID_USAGE";
3336
this.exitCode = 64;
37+
this.usageReason = reason;
38+
this.usageFlag = safeFlag;
3439
}
3540
}
3641

3742
function parseOptions(argv, allowed) {
3843
const options = {};
3944
for (let index = 0; index < argv.length; index += 1) {
4045
const flag = argv[index];
41-
if (!flag.startsWith("--") || !allowed.has(flag)) throw new UsageError(`unknown option: ${flag}`);
46+
if (!flag.startsWith("--") || !allowed.has(flag)) throw new UsageError("unrecognized option");
4247
const value = argv[index + 1];
43-
if (value === undefined || value.startsWith("--")) throw new UsageError(`missing value for ${flag}`);
44-
if (Object.hasOwn(options, flag)) throw new UsageError(`duplicate option: ${flag}`);
48+
if (value === undefined || value.startsWith("--")) throw new UsageError("missing option value", flag);
49+
if (Object.hasOwn(options, flag)) throw new UsageError("duplicate option", flag);
4550
options[flag] = value;
4651
index += 1;
4752
}
4853
return options;
4954
}
5055

5156
function requireOption(options, flag) {
52-
if (!options[flag]) throw new UsageError(`missing required option: ${flag}`);
57+
if (!options[flag]) throw new UsageError("missing required option", flag);
5358
return options[flag];
5459
}
5560

@@ -70,7 +75,7 @@ export async function runHarnessComponentSnapshotCli(argv) {
7075
return;
7176
}
7277
const [command, ...rest] = argv;
73-
if (!COMMANDS.has(command)) throw new UsageError(`unknown command: ${command}`);
78+
if (!COMMANDS.has(command)) throw new UsageError("unrecognized command");
7479
if (rest.length === 1 && (rest[0] === "--help" || rest[0] === "-h")) {
7580
process.stdout.write(HELP);
7681
return;
@@ -107,7 +112,7 @@ if (isMain) {
107112
runHarnessComponentSnapshotCli(process.argv.slice(2)).catch((error) => {
108113
const code = error.code ?? "SNAPSHOT_FAILED";
109114
const message = error instanceof UsageError
110-
? "invalid component snapshot arguments"
115+
? error.message
111116
: "component snapshot operation failed";
112117
process.stderr.write(`${code}: ${message}\n`);
113118
process.exitCode = error.exitCode ?? 1;

scripts/harness-component-snapshot/contract.mjs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,10 @@ function validateCoverage(snapshot) {
249249
}
250250
}
251251

252+
// In v1 every relationship is exactly one `declared-in` edge per component, so
253+
// the set is fully derivable from `components`. It stays a serialized, validated
254+
// field because the typed relationship shape is the stable extension point for
255+
// later relationship types; adding one must not be a breaking schema change.
252256
function validateRelationships(snapshot) {
253257
if (!Array.isArray(snapshot.relationships)) fail("INVALID_RELATIONSHIPS", "snapshot relationships must be an array");
254258
const byId = new Map(snapshot.components.map((component) => [component.id, component]));

scripts/harness-component-snapshot/diff.mjs

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,16 @@ import {
1010
validateHarnessComponentSnapshot,
1111
} from "./contract.mjs";
1212

13+
// Significant statuses come first so a small limit truncates redundant
14+
// `unchanged` entries instead of hiding the actual differences. Ordering stays
15+
// deterministic because ties fall back to the canonical component ID order.
16+
const STATUS_ORDER = Object.freeze(["changed", "added", "removed", "unchanged"]);
17+
18+
function compareEntries(left, right) {
19+
const statusDelta = STATUS_ORDER.indexOf(left.status) - STATUS_ORDER.indexOf(right.status);
20+
return statusDelta || compareCodeUnits(left.componentId, right.componentId);
21+
}
22+
1323
function boundedLimit(value) {
1424
const limit = value === undefined ? 200 : Number(value);
1525
if (!Number.isInteger(limit) || limit < 0 || limit > MAX_DIFF_LIMIT) {
@@ -56,7 +66,7 @@ export function diffHarnessComponentSnapshots(before, after, options = {}) {
5666
componentId,
5767
beforeById.get(componentId),
5868
afterById.get(componentId),
59-
));
69+
)).sort(compareEntries);
6070
const counts = Object.fromEntries(["added", "removed", "changed", "unchanged"]
6171
.map((status) => [status, allEntries.filter((entry) => entry.status === status).length]));
6272
const diff = {

test/harness-component-snapshot-cli.test.mjs

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,19 +33,38 @@ test("direct CLI provides global and leaf help without performing work", () => {
3333
const result = run(args);
3434
assert.equal(result.status, 64, `${args.join(" ")} unexpectedly succeeded`);
3535
assert.equal(result.stdout, "");
36-
assert.equal(result.stderr, "INVALID_USAGE: invalid component snapshot arguments\n");
36+
assert.match(result.stderr, /^INVALID_USAGE: unrecognized (command|option)\n$/u);
3737
}
3838

3939
const privatePath = path.join(os.tmpdir(), "PRIVATE-USAGE-PATH-SENTINEL", "snapshot.json");
4040
for (const args of [[privatePath], ["validate", privatePath]]) {
4141
const result = run(args);
4242
assert.equal(result.status, 64);
4343
assert.equal(result.stdout, "");
44-
assert.equal(result.stderr, "INVALID_USAGE: invalid component snapshot arguments\n");
44+
assert.match(result.stderr, /^INVALID_USAGE: unrecognized (command|option)\n$/u);
4545
assert.equal(result.stderr.includes(privatePath), false);
46+
assert.equal(result.stderr.includes("PRIVATE-USAGE-PATH-SENTINEL"), false);
4647
}
4748
});
4849

50+
test("direct CLI names allowlisted flags in usage failures without echoing their values", async (t) => {
51+
const { workspace } = await fixtureWorkspace(t);
52+
const privateValue = path.join(os.tmpdir(), "PRIVATE-FLAG-VALUE-SENTINEL", "snapshot.json");
53+
54+
const missingValue = run(["create", "--workspace"]);
55+
assert.equal(missingValue.status, 64);
56+
assert.equal(missingValue.stderr, "INVALID_USAGE: missing option value: --workspace\n");
57+
58+
const duplicate = run(["create", "--workspace", workspace, "--workspace", privateValue]);
59+
assert.equal(duplicate.status, 64);
60+
assert.equal(duplicate.stderr, "INVALID_USAGE: duplicate option: --workspace\n");
61+
assert.equal(duplicate.stderr.includes(privateValue), false);
62+
63+
const missingRequired = run(["validate"]);
64+
assert.equal(missingRequired.status, 64);
65+
assert.equal(missingRequired.stderr, "INVALID_USAGE: missing required option: --snapshot\n");
66+
});
67+
4968
test("direct CLI creates parser-safe private JSON and rejects unknown options", async (t) => {
5069
const { workspace } = await fixtureWorkspace(t);
5170
const created = run([

test/harness-component-snapshot.test.mjs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -476,6 +476,14 @@ test("bounded diff reports added, removed, changed, and unchanged independently"
476476
assert.equal(bounded.entries.length, 2);
477477
assert.equal(bounded.totalEntries, full.totalEntries);
478478
assert.equal(bounded.truncated, true);
479+
assert.deepEqual(bounded.counts, full.counts);
480+
assert.equal(bounded.entries.some((entry) => entry.status === "unchanged"), false);
481+
482+
const significant = diffHarnessComponentSnapshots(before, after, { limit: 3 });
483+
assert.deepEqual(
484+
significant.entries.map((entry) => entry.status),
485+
["changed", "added", "removed"],
486+
);
479487
assert.throws(() => diffHarnessComponentSnapshots(before, after, { limit: 1001 }), { code: "INVALID_DIFF_LIMIT" });
480488
});
481489

0 commit comments

Comments
 (0)