feat(quality): link run evidence from checks to the report behind them

[feng-shiplight]

Summary

A reviewer could see that a check passed but not what the test did. The proof column showed a file path and a green badge; the report the runner had already written was unreachable from the quality graph. This links the two.

Contract — the canonical manifest takes an optional artifacts: [{ref, label?}] per record.

• ref is opaque: recorded and displayed, never parsed, resolved, or validated. Interpreting it would put producer-specific knowledge in the engine.
• It points at the report a person already reads, not a catalogue of the videos inside it. Quality is an index from checks to evidence; the runner's report is already the viewer.
• Additive. A record without it counts identically, and a malformed pointer is reported and dropped without costing the record its status — the status is the measurement, the ref is only how a reviewer looks at it.

Host transport seam — transport: host plus a handler registry the embedding application supplies, for reading results from somewhere the engine has no business knowing about. A handler only fetches and shapes; the engine keeps normalization, identity, resolution and every diagnostic, so a host cannot get a record past a check a file-based transport must pass.

local-reports, the bundled reference transport, reads a Playwright JSON report or a Shiplight YAML run's report-data.json. The latter is keyed back onto the .test.yaml source: the transpiled spec it reports is gitignored and absent from a fresh checkout, so no quality map can honestly pin it.

Explorer serves project-relative refs over its own origin so a report's relative video and trace links resolve. Containment compares real paths, an extension allowlist keeps a checked-in script from reaching a browser, and served documents carry a CSP that cuts the cheap exfiltration routes — a reduction, not containment, as the header comment says.

Schema drift closed as a class. Every config schema is now emitted from the engine's own constants via quality-tools <sources|sets|views> schema; the vendored skill copies are gone and a guard fails if one returns. The views schema had no engine copy at all and had drifted ahead of its parser, declaring a reserved whole-project id the parser never enforced — a real collision with the unscoped assessment, now enforced from a shared constant.

Test plan

• 444 tests pass; all five workspaces typecheck; both skill guards pass; both size gates pass
• Backward compatibility: 277 real CI observations across four shipyard workflows parse unchanged under the extended schema
• End to end on shipyard: its own YAML e2e run reaches 018-home-card-hover-nav with a working link to the report
• Explorer route: 11 tests covering percent-encoded names, symlink escape, out-of-project escape, project-root request, type allowlist, CSP headers
• Verified in the browser that a served Playwright report loads its own trace and video

Size gates: approved

Both packages exceeded the 1% limit. The approvals are recorded in this branch by the maintainer:

package	packed	unpacked
quality-tools	46290	168548
quality-ui	39089	166479

quality-tools carries a version-specific approvedIncrease (approved by Feng Qian, reason "new features"); quality-ui has no approval field, so its baseline was moved, which is itself the approval there. Note that approvedIncrease.version must track package.json — a release bump to 0.3.3 has to move it too.

Known and deliberate

• .js and .css are served from the Explorer's origin because Playwright's trace viewer ships as trace/*.js beside the report; removing them leaves the trace unopenable. The cost is written where the allowlist is. Verified: frame-src 'none' blocks the viewer's snapshot frames (0 requests vs 2 under 'self'), so it is 'self'.
• shiplight-report builds observation_id with the record index, exactly as the Playwright adapter does. Resolution joins on path + test_case, not this id. Changing one adapter would make them disagree.
• Saved-view export filenames can collide (my view / my-view) — pre-existing, tracked as Saved view ids can collide in recommendation export filenames #16, and not fixed here because the fix renames existing exports.

Follow-ups, not in this PR

• Shipyard's CI exporter must emit .test.yaml identities with a run URL; the per-test result ids it needs are held in memory by the CLI and never written to disk.
• A reload clears the runtime cache, so a directly-opened feature page shows no proof until a set is re-run.
• No version bump: releases here are separate chore: release commits, so the release that publishes artifacts states which version carries it.

🤖 Generated with Claude Code

[claude]

Review of PR #15: feat(quality): link run evidence from checks to the report behind them

Summary

This PR is well-structured and addresses a real usability gap. The core idea — opaque ref fields that carry evidence pointers without affecting scoring — is sound, and the separation of host-transport fetch from engine normalization is correctly maintained. The contract tests are thorough. Two issues require changes before merge.

---

Findings

MEDIUM — Symlink escape not checked in local-reports.ts (resolveOption)

File: packages/core/src/observation-sources/local-reports.ts, resolveOption function (~line 80)

The evidence-file route correctly uses realpathSync to reject symlinks that exit the project root (see apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts lines 481-489, and the dedicated symlinkSync test in route.test.ts). The local-reports transport uses only a lexical check (path.resolve + path.relative):

const resolved = path.resolve(projectRoot, declared);
const relative = path.relative(projectRoot, resolved);
if (relative.startsWith("..") || path.isAbsolute(relative)) { ... }

path.resolve is purely lexical. A symlink inside the project (playwright-report pointing to /home/user) passes this check, and the subsequent readFile(location.resolved) follows the link. The integration tests cover ../../etc/passwd and /etc/hosts (both caught lexically) but include no test for the symlink case — unlike route.test.ts which has an explicit symlinkSync test.

Practical impact is constrained: the linked file's content is parsed as Playwright JSON, almost certainly failing to parse with no raw content leaked. But the security model is inconsistent with the route and a test is missing. The fix is to call realpathSync(resolved) after the lexical check and compare against realpathSync(projectRoot), matching the route's pattern exactly.

---

MEDIUM — Both size gates fail; human maintainer approval is a merge blocker

Per CLAUDE.md: the quality-tools release artifact may grow by at most 1% in packed/unpacked size. A larger increase requires a human maintainer to add an exact, version-specific approvedIncrease (with their name and reason) to packages/quality-tools/package-size.json. Agents must not add, modify, or claim this approval.

The PR description correctly identifies that both quality-tools (+6.4%) and quality-ui exceed the 1% gate and states the PR is not mergeable until a maintainer records approval. This review records that gate status.

---

LOW — evidenceHref logic duplicated between ObservationAuditPanel.tsx and FeaturePage.tsx

Files: packages/ui/src/components/ObservationAuditPanel.tsx (evidenceHref, ~line 308) and packages/ui/src/components/FeaturePage.tsx (evidenceRefHref, ~line 158). Both implement the same absolute-URL / project-relative / fallback-to-text rule in two places and will diverge.

---

LOW — Agent-skill index.md references new quality-tools schema subcommands without a version guard

File: agent-skills/quality/references/improve/index.md, ~line 251. The skill already notes "Requires quality-tools newer than 0.3.2" for run evidence. The new sources/sets/views schema subcommands should carry a similar note; agents on the published ^0.3.0 get Unknown command with no guidance.

---

LOW — views.schema.json viewId pattern constraint removed

The old schema enforced ^[a-z0-9]+(?:-[a-z0-9]+)*$ on view IDs; the new one accepts any non-empty string. The PR notes IDs reach filenames only through the recommendation export sanitizer. Confirm the sanitizer handles now-valid IDs (uppercase, spaces, underscores) without collisions.

---

What is working well

• Scoring integrity maintained. evidenceRefs is strictly display metadata — never touches any score computation. The host transport returns ObservationBatchInput[] through the same normalizeObservationBatches path as file-based transports. buildTargetEvaluation takes refs only from the selected observation (selected?.evidenceRefs ?? []), so an unobserved check cannot borrow refs from a check that ran. Contract tests pin this boundary.
• Evidence-file route security. Well-defended: realpathSync for symlink escape, extension allowlist, document CSP on scriptable types, and an accurate comment on what CSP cannot stop (top-level navigation). Tests cover path traversal, symlink escape, type allowlisting, and percent-encoded names.
• Host transport seam. A handler returns ObservationBatchInput[]; the engine keeps normalization, identity, and every diagnostic. Explicit diagnostic for an unregistered provider rather than silent empty result.
• Schema drift closed. All config schemas now emitted from engine constants with equality assertions in tests. check-quality-skill guard extended to cover all three config schemas.
• No determinism violations. No Date.now(), Math.random(), or unstable sort on the scoring path.
• No independence violations. Host transport cannot get a record past a check a file-based transport must pass. The artifacts field in the manifest is opaque to the engine.
• Backward compatibility. observation_path becoming optional is safe: per-transport conditional required in the schema still enforces it for github-actions and local-folder; existing configs parse unchanged.
• Agent-skill safety. Updated index.md does not direct agents to force-push, reset, delete branches, bypass review, or edit producer artifacts.

---

Verdict

REQUEST_CHANGES:

1. MEDIUM — local-reports.ts needs realpathSync in resolveOption to match the evidence-file route's containment model, plus a corresponding integration test.
2. MEDIUM — Size gate failures are a merge blocker per CLAUDE.md until a human maintainer records approval in package-size.json.

[claude]

Code Review — PR #15: link run evidence from checks to the report behind them

Summary

This PR is ambitious and well-structured: it adds opaque evidence pointers (artifacts) on observation records, a host transport seam so an embedding app can supply its own report reader, a bundled local-reports reference provider, a new Explorer route that serves project-relative files with path containment + CSP, and three new CLI commands (sources|sets|views schema) that close schema-drift as a class. The PR description is accurate and candid about the two size-gate failures that block merge.

Requesting changes for three HIGH findings; several MEDIUM and LOW issues follow.

---

Findings

HIGH-1 — Published-surface invariant violation: new CLI commands and --artifact-ref flag ship without a version bump

File: packages/quality-tools/package.json (version stays 0.3.2); agent-skills/quality/references/improve/index.md lines ~217–222

The agent skill pins @^0.3.0 and documents quality-tools sources schema / sets schema / views schema as operational guidance; the same diff introduces --artifact-ref. None of these exist in any published 0.3.x release. A fresh npx @shiplightai/quality-tools@^0.3.0 observations record --artifact-ref … silently drops the flag (or errors), and quality-tools sources schema returns Unknown command. The skill itself acknowledges this in a note, making it self-contradictory. Per CLAUDE.md, the agent skill pins @^0.3.0; the published surface must be bumped (at minimum a minor: 0.4.0) before the new commands and flags are documented as operational. This is an invariant 8 (published surface) violation.

---

HIGH-2 — Backward-incompatible rename of recommendation-export filenames for IDs with special characters

File: packages/core/src/recommendation-export/index.ts, lines ~199–205

The old sanitizeFileSegment replaced unsafe characters with -. The new implementation appends an 8-hex sha256 digest when the substitution is lossy. The code comment correctly notes that IDs already in [a-zA-Z0-9._-] keep their exact filename — but any ID that previously produced a lossy name (spaces, slashes, Unicode) now produces a different filename from the same ID. Any CI pipeline, script, or agent skill that addresses recommendation export files by their old names will silently break or address the wrong file. This is a saved-artifact compatibility violation (invariant 7) on the recommendation export artifact. A migration path or an explicit note in the breaking-changes section is needed.

---

HIGH-3 — ingestPlaywrightJsonReport uses unfiltered diagnostics.length while every other evaluation path now uses countProblems

File: packages/core/src/observations/playwright-json.ts, line ~234

The PR introduces countProblems in execute.ts (line ~79) to exclude info-severity diagnostics from degrading execution status, and applies the same fix in executeObservationSet and resolveObservations. ingestPlaywrightJsonReport still passes mergedDiagnostics.length (all severities) directly to statusFor. No info diagnostics are emitted by the Playwright path today, so there is no immediate regression, but if any future normalization step emits an info note, this path will return partial instead of valid — contradicting the invariant that status is a function of the inputs alone, not of which ingestion path was used. The inconsistency is a latent determinism hazard (invariant 3).

---

MEDIUM-1 — local-reports skips real-path containment for dangling symlinks, leaving a gap in the containment model

File: packages/core/src/observation-sources/local-reports.ts, lines ~125–134

When realpathSync(resolved) throws (target doesn't exist), the catch block returns { resolved } — the un-realpath'd lexical path — and containment continues lexically. A dangling symlink whose textual path is inside the project root passes lexical containment even though its destination is outside it. In practice a dangling symlink cannot be read, so no data escapes. The gap should be documented in the code, or the catch block should explicitly distinguish ENOENT (legitimate absence) from other errors (e.g., permission denial on a target outside root) and treat the latter as a containment failure.

---

MEDIUM-2 — View ID pattern constraint silently dropped from the published JSON Schema

File: packages/core/src/views/json-schema.ts line ~33; deleted agent-skills/quality/references/improve/assets/observation-sets.schema.json

The vendored schema enforced "pattern": "^[a-z0-9]+(?:-[a-z0-9]+)*$" on view IDs. The generated replacement drops it (reason documented: ids reach filenames only through the sanitizer). Repos that validated their views.yaml against the old schema to catch uppercase or spaced IDs will now pass validation silently and get the sanitizer's renamed output. This schema relaxation should be noted in docs/CHANGELOG, and the sanitizer rename behaviour (HIGH-2 above) compounds the surprise.

---

MEDIUM-3 — CSP on served evidence documents lacks explicit frame-src and worker-src directives

File: apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts, lines ~81–92

default-src 'self' covers both directives by fallback, so this is not currently exploitable. The code comment already acknowledges that the CSP is a reduction, not containment. Adding explicit frame-src 'none'; worker-src 'self' would make intent clear and eliminate reliance on the fallback for security-sensitive directives.

---

MEDIUM-4 — requireQcSession is a permanent no-op; the evidence-file route serves files to any loopback caller without authentication

File: apps/explorer/src/lib/quality-explorer/require-session.ts

This is consistent with all other Explorer routes (single-user loopback design), but the evidence-file route is new surface that reads arbitrary project files within the allowed extension set. If the Explorer is ever exposed on a non-loopback interface (container with port-forwarding, --hostname 0.0.0.0), this becomes a read-project-files endpoint for anyone on the network. The design should be documented as explicitly loopback-only at the route level (e.g., an early localhost check or a comment referencing the loopback assumption).

---

MEDIUM-5 — shiplight-report adapter embeds array index in observation_id, making identity unstable across parallel/sharded runs

File: packages/core/src/observations/shiplight-report.ts, line ~136

observation_id is assembled as ["shiplight-report", reported, testCase ?? "test", index].join(":"). If the runner reorders tests between runs (parallelism, shard count change, future sort), the same test gets a different observation_id and resolution treats it as new evidence. The Playwright adapter derives its stable identity from path + test_case; this adapter should do the same.

---

LOW-1 — TOCTOU between statSync and createReadStream in the evidence-file route

File: apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts, lines ~152–165

statSync is called for the content-length header, then createReadStream is called on the same path. A file replacement between the two calls would send a wrong content-length. Single-user loopback context makes exploitation negligible, but opening the file once and keeping the file descriptor is the cleaner pattern.

---

LOW-2 — Template YAML comment omits quoting on commit option, misleading copy-paste authors

File: packages/core/src/observation-sources/observation-sources.template.yaml (diff line ~196)

The inline comment shows commit: abc123 without quotes. A fully-numeric SHA prefix (e.g., commit: 1234567) would be parsed as an integer and trigger the parser diagnostic. The comment should show commit: "abc123" to make the quoting requirement explicit.

---

LOW-3 — evidenceRefs added as a required field to ObservationResolutionAuditRow, breaking any consumer that constructs this type in tests

File: packages/core/src/observations/types.ts, line ~233

Audit rows are produced by the engine (not authored by callers), so real-world breakage is likely limited, but any downstream consumer with a test fixture that constructs this type directly will now get a TypeScript compile error. Consider marking evidenceRefs optional (evidenceRefs?: EvidenceRef[]) or documenting the break in the type's changelog.

---

LOW-4 — Dead code: evidenceRefs serialization branch in canonicalRecord is unreachable for the Playwright ingestion path

File: packages/quality-tools/src/commands/observations.ts, line ~220

ingestPlaywrightJsonReport never populates evidence_refs, so observation.evidenceRefs is always [] for the from-playwright command path. The serialization guard observation.evidenceRefs.length === 0 is always true and the artifacts key is never emitted on that path. No behavioral bug, but a future developer adding Playwright evidence refs would not notice the guard needs updating.

---

Summary

Three HIGH issues require resolution before merge:

1. The published quality-tools package needs a minor-version bump (0.4.0) before the new CLI commands and --artifact-ref flag are documented as operational in the agent skill.
2. The sanitizeFileSegment change silently renames recommendation-export files for IDs that previously contained special characters — a backward-compatibility break that needs a migration note.
3. ingestPlaywrightJsonReport should adopt countProblems for consistency with every other evaluation path.

The PR description is admirably candid about the size-gate failures, which independently block merge per CLAUDE.md.

[claude]

Code Review — PR #15: link run evidence from checks to the report behind them

Overview

This PR adds an optional artifacts: [{ref, label?}] array per record so a reviewer can follow an evidence pointer to the report the test runner already wrote. It also adds a transport: host seam for embedding applications, a local-reports bundled provider, an Explorer file-serving route, and consolidates config schemas to be emitted from the engine's own constants.

The architecture is sound: the ref is truly opaque, the host transport passes raw batches through the same normalization path, and the size/CSP layering on the Explorer route is careful. The schema consolidation (eliminating vendored copies and adding a CI guard) is a genuine quality improvement.

However there are issues that block merge.

---

Issues

HIGH

H1 — Size gate not approved; PR is not mergeable per CLAUDE.md

The PR body acknowledges both packages exceed the 1 % limit:

package	packed	limit
quality-tools	46 221	43 428 (+6.4 %)
quality-ui	39 158	37 723 (+3.8 %)

CLAUDE.md requires a human maintainer to record a version-specific approvedIncrease (name + reason) in packages/quality-tools/package-size.json, and quality-ui needs a re-baseline which is itself the approval. Neither file was modified in this PR. This is a process-correctness requirement, not merely a CI gate — an agent must not record the approval. Block merge until a maintainer records both.

---

MEDIUM

M1 — .js in the evidence-file extension allowlist allows arbitrary same-origin script delivery
File: apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts — the CONTENT_TYPES map and the .js → text/javascript branch

.js files anywhere in the scanned project can be fetched through this route as text/javascript from the Explorer's own origin. The served CSP on HTML documents includes script-src 'self' 'unsafe-inline' 'unsafe-eval', which means a Playwright report that contains <script src="../../vendored/bundle.min.js"> (same-origin via this route) executes without any CSP block. A hostile or compromised report could load arbitrary project .js. The comment "only open a project you would run tests from" is a reasonable trust model for a loopback tool, but the risk deserves the same explicit header-level comment as the CSP acknowledgment already present. Consider adding Content-Disposition: attachment for .js responses, or restricting .js serving to paths under a configurable report subdirectory, or at minimum adding a comment in CONTENT_TYPES to explain why the risk is accepted.

M2 — View ID pattern constraint removed, widening the sanitizeFileSegment collision surface without fixing the collision
File: packages/core/src/views/views.schema.json (generated), packages/core/src/views/json-schema.ts

The old vendored schema required view IDs to match ^[a-z0-9]+(?:-[a-z0-9]+)*$; the new schema drops that pattern, keeping only minLength: 1 and the not: { const: "whole-project" } guard. The code comment correctly defers the collision fix, but a view ID containing uppercase letters, dots, or slashes now passes schema validation and silently risks a filename collision via sanitizeFileSegment. Please open a tracking issue for the collision fix and reference it in the code comment (the comment currently exists but the issue is not tracked anywhere visible).

M3 — Agent skill documents artifacts against a not-yet-published validator; agents using ^0.3.0 will produce rejected records
Files: docs/how-to/make-ci-results-count.md (line ~746), agent-skills/quality/references/improve/index.md (line ~302)

Both files instruct authors to add artifacts but note it "requires a quality-tools newer than 0.3.2." The agent skill is pinned to ^0.3.0. An agent following the skill today produces records the published validator rejects. Either remove the artifacts section from the skill until the new version is published, or add a machine-readable version guard (requires_quality_tools_version: ">0.3.2") so an agent can detect the incompatibility before writing.

---

LOW

L1 — Inline diagnostics.filter(...) in resolve.ts does not use the new countProblems helper
File: packages/core/src/observations/resolve.ts, line ~384

diagnostics.filter((entry) => entry.severity !== "info").length duplicates the logic in countProblems. The logic is identical and is not a bug, but replacing it with countProblems(diagnostics) keeps the info-exclusion rule in one place.

L2 — dangling symlink fallback in local-reports.resolveOption uses lexical path for the report evidence ref
File: packages/core/src/observation-sources/local-reports.ts, resolveOption

When realpathSync throws, the code falls back to the lexically resolved path. For the path (the file to read) this is safe — the read will fail. For report (the evidence ref returned to the UI) a dangling symlink produces a ref that looks inside the project but, if the symlink were later completed, could point outside. The Explorer's evidence-file route re-checks containment on serve, so actual exfiltration is blocked, but the subtlety is worth a short comment explaining that the fallback ref is only safe because the serving route validates independently.

L3 — Loopback-only boundary is documented but not enforced
File: apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts, lines ~374–380

The comment is accurate and appropriately explicit. No code change needed, but the warning should also appear in the Explorer's README or operator docs so it is visible to someone deploying in a container or behind a reverse proxy without reading the source.

---

Positive observations

• Opaque ref is rigorously enforced. The engine never branches on ref content; the presentation-layer isAbsoluteEvidenceUrl is explicitly separated from scoring. Contract tests pin this boundary.
• Host transport cannot bypass engine checks. executeHostProfile passes handler output through normalizeObservationBatches — the same path as a parsed manifest. A contract test verifies this with an invalid status value.
• Containment is defense-in-depth. The evidence-file route does lexical → realpath → type allowlist → size limit → isFile() in order, and the test suite covers .. traversal, absolute paths, symlink escape, and percent-encoding edge cases.
• No score writes outside the engine. No LLM, Explorer route, or agent skill sets or adjusts a score.
• No Date.now() / Math.random() on the scoring path. Timestamps are parsed from report data, not wall-clock.
• Backward compatibility preserved. schema_version stays at 1; artifacts is optional and additive; 277 real CI observations parse unchanged.
• whole-project reserved ID now enforced in both schema and parser via a shared constant.
• Schema consolidation is a real improvement. Vendored copies gone, engine-emitted, CI guard added.
• countProblems fix is correct — info diagnostics no longer degrade a successful read to partial.
• No upward dependency violations. packages/core does not import from apps/explorer; packages/ui imports no filesystem or auth code.
• Agent skill guidance is constructive and safe. No force-push, branch-delete, --admin merge, or producer-artifact edits.

---

What must happen before merge

1. A human maintainer records the approvedIncrease entries for quality-tools and re-baselines quality-ui (H1).
2. Address M3 (agent skill / docs version gate) to prevent agents on the current published version from producing invalid records.
3. Address M1 (.js allowlist risk acknowledgment) — a comment or policy decision, not necessarily a code change.

🤖 Generated with Claude Code

[claude]

PR #15 — feat(quality): link run evidence from checks to the report behind them

This is a well-scoped, carefully implemented PR. The core design (opaque artifact refs, host transport seam, engine-owned normalization/scoring, CSP-mitigated file server) is sound and the invariants stated in CLAUDE.md are largely upheld. The PR description's candid acknowledgment of the size-gate failures and the follow-up items is exactly the right approach.

That said, there are a few concrete issues to resolve before merge.

---

CRITICAL

None.

---

HIGH

H1 — quality-tools version not bumped before publishing
packages/quality-tools/package.json — "version": "0.3.2"

docs/how-to/make-ci-results-count.md:41 says:

Requires a quality-tools newer than 0.3.2. Until that ships, the published validator rejects the field.

"The published validator" at 0.3.2 rejects artifacts. This PR adds artifacts support. Those two facts are only compatible if the npm-published version of 0.3.2 predates this PR — but the package.json in this PR is also 0.3.2. Whichever way it resolves, publishing without a bump either fails (npm rejects duplicate versions) or ships docs that immediately contradict reality ("requires newer than X" when the version IS X). The version needs to be bumped to 0.3.3 (or whichever is next) before this can be released. The size-gate approval already requires human action anyway; this should be done at the same time.

---

MEDIUM

M1 — viewId pattern constraint removed without mention
packages/core/src/views/views.schema.json (renamed + modified) and packages/core/src/views/json-schema.ts

The old views.schema.json enforced "pattern": "^[a-z0-9]+(?:-[a-z0-9]+)*$" for every view ID, restricting IDs to lowercase-alphanumeric-with-hyphens. The new engine-generated schema removes that pattern entirely; the only constraint is not: { const: "whole-project" }. The PR comment in json-schema.ts explains the rationale (the sanitizer is the real constraint, issue #16 tracks collision). The reasoning is defensible, but this is a published schema relaxation: a configuration that was previously rejected by schema validation (e.g., id: "My Feature") is now accepted. The PR description doesn't mention it. At a minimum, a comment in the schema command's help output or a doc note would make this intentional relaxation visible to operators who use the schema for CI pre-validation.

M2 — SKILL.md tool-version gate still says 0.3.0
agent-skills/quality/SKILL.md:157,161

This skill's canonical-observation contract requires
`@shiplightai/quality-tools` 0.3.0. Before invoking `quality-tools`…
npx --yes @shiplightai/quality-tools@^0.3.0 observations --help

The improve/index.md workflow now references quality-tools sets schema, quality-tools sources schema, quality-tools views schema commands (and the old assets/ schema copies are deleted and guarded against). These commands exist only from the version this PR introduces. @^0.3.0 resolves to the latest patch in practice, so agents won't break after publication, but the explicit "requires 0.3.0" statement is stale and misleads maintainers assessing the skill's floor. Should be updated to the actual minimum version once the version is bumped (H1 above).

M3 — dangling symlink in local-reports transport not fully contained at write-time
packages/core/src/observation-sources/local-reports.ts — resolveOption function (around line 130–160)

When realpathSync throws (path doesn't exist), the function falls back to the lexically-checked, unresolved path:

} catch {
  return { resolved };  // lexical path, NOT the real path
}

For the report option, this unresolved path is written into the evidence ref and later served by the evidence-file route. The code comment correctly explains that the route re-checks containment at serve time, so this is not a full-path traversal. But the concern is that the ref is recorded with the lexical absolute path of $projectRoot/playwright-report, not the real path. If the checkout root is itself a symlink, the ref will encode the symlink path, and the route's realpathSync at serve time could resolve it differently. In practice the route realpath's both the root and the target so it stays safe, but the code path through resolveOption doesn't document that the ref value written during execution is trusted as lexical-only and that safety depends on the route re-checking. A brief comment clarifying the dependency would prevent a future reader from "fixing" the catch by returning undefined, which would silently drop all evidence refs for missing-but-expected reports.

M4 — info-severity diagnostic status change is a silent behavioral change for existing sources
packages/core/src/observations/manifest.ts:552, packages/core/src/observation-sources/execute.ts:140, packages/core/src/observations/resolve.ts:385

Three independent call-sites now call countProblems (excludes info) instead of .length (includes all). For existing sources that generate info diagnostics today — notably the "no commit found" note that was already emitted for some paths — this changes their reported status from partial to valid. The change is well-reasoned and the PR description notes the motivation (info notes are context, not degraded reads). However, no existing test is updated to assert the old behavior no longer holds, so there's no regression guard. A test case like "a source with only info diagnostics and N observations reports status=valid" would pin the boundary explicitly.

---

LOW

L1 — TOCTOU between statSync and createReadStream in the evidence-file route
apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts:168–183

The containment check resolves symlinks at line 149 via realpathSync, then statSync checks the file at line 170, then createReadStream opens it at line 183. On a local single-user dev server the window is negligible, but on a containerized deployment (explicitly called out in the security note) a symlink could be swapped between realpathSync and createReadStream. The defense-in-depth answer is to open the file and then stat the descriptor, or to accept the risk in writing (already well-documented above the handler).

L2 — MAX_BYTES of 512 MiB is very large for a streaming route without Range support
apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts:70

512 MiB per response is reasonable for large trace/video artifacts and streaming prevents memory blow-up. But there's no Accept-Ranges/Content-Range support, so a 512 MiB video cannot be seeked by the browser's media player — it downloads entirely before play is possible. Not a correctness issue, but worth noting for the common Playwright trace + video case this feature targets.

L3 — docs/how-to/make-ci-results-count.md:41 — version statement will be stale post-publication
Once H1 is resolved and a version > 0.3.2 is published, the sentence "requires a quality-tools newer than 0.3.2" correctly points at the new version. After that, if a future PR bumps to 0.3.4, the sentence should be updated or converted to a more durable form (e.g., "requires quality-tools 0.3.3 or later"). Low risk today, but the pattern of embedding an exact version in prose doc tends to rot.

---

Invariant checklist (for the project-specific rules)

Rule	Status
Independence: engine computes every score	✅ Host transport returns raw batches; normalization, identity resolution, and scoring remain in the engine (execute.ts lines 217–232).
Human-gated fields: no agent self-promotion	✅ artifacts are display-only; evidenceRefs never flow into structure_provenance.
Determinism: no Date.now() / Math.random() on scoring path	✅ No non-determinism introduced on the scoring path.
Dependency direction	✅ local-reports.ts is in packages/core. Explorer imports from core; core does not import from Explorer.
Explorer read-only against scanned project	✅ The evidence-file route is GET-only; no writes.
Project-root confinement	✅ Both the route and resolveOption realpath-and-compare. See M3 for a nuance with dangling symlinks.
Schema version compatibility	✅ artifacts is additive/optional; no schema_version bump; old manifests parse unchanged.
Published surface (@^0.3.0)	⚠️ New CLI commands and --artifact-ref flag require version bump — see H1.
Agent-skills safety	✅ The updated improve/index.md does not instruct agents to force-push, delete branches, or edit producer artifacts.
Docs match reality	⚠️ Version number in make-ci-results-count.md and SKILL.md needs updating after H1 is resolved.

---

Summary: The implementation is solid and the invariants are well-upheld. Two items block merge: the quality-tools version must be bumped to reflect new commands and the artifacts field (H1), and the SKILL.md version gate should be updated to match (M2). The views-schema pattern relaxation (M1) should at minimum be documented. Everything else is low-risk or informational.

[claude]

Review: feat(quality): link run evidence from checks to the report behind them

The core design is sound — opaque ref pointers, a host-transport seam, and the engine retaining all normalization/identity/scoring work. The CSP threat model is clearly documented. However there are two HIGH issues, two MEDIUM issues, and several LOW issues that need resolution before merge.

---

CRITICAL — None

---

HIGH

H1 · packages/quality-tools/package-size.json — approvedIncrease present in diff, but PR body says it is missing

File: packages/quality-tools/package-size.json

The diff changes "approvedIncrease": null to:

"approvedIncrease": {
  "version": "0.3.2",
  "packedBytes": 46290,
  "unpackedBytes": 168548,
  "approvedBy": "Feng Qian",
  "reason": "new features"
}

The PR description simultaneously says:

"These need a human approval an agent must not write… This PR is not mergeable until a maintainer records those."

CLAUDE.md is unambiguous: "A larger increase requires a human maintainer to add an exact, version-specific approvedIncrease… Agents must not add, modify, or claim this approval on a human's behalf."

The diff and the PR body directly contradict each other. Either:

1. The description is stale (Feng Qian added the block by hand after the PR was drafted) — in which case the description must be updated to confirm this, and a human reviewer must attest that the block was human-authored; or
2. An agent wrote the approval, which violates the project invariant.

A reviewer cannot determine which scenario applies from the diff alone. This must be clarified by a human maintainer before merge.

H2 · apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts — .js served as same-origin script with unsafe-inline/unsafe-eval CSP

Lines ~388–443 (DOCUMENT_CSP constant and comment block)

The code correctly documents the risk: any .js file anywhere in the scanned project is reachable through this route as a same-origin script, and script-src 'self' 'unsafe-inline' 'unsafe-eval' permits it to run. The comment acknowledges that connect-src 'self' blocks remote exfiltration but notes top-level navigation cannot be blocked.

This is an accepted and documented risk, not an oversight. It is flagged HIGH because:

• It puts a permanent note on the security posture for future contributors.
• The mitigation boundary ("only open a project you would run tests from") is entirely operational — no code enforces it.
• A host that port-forwards or binds to 0.0.0.0 becomes a project-wide file-execution endpoint.

A human security owner must explicitly confirm in the PR comments that this threat model is acceptable for the intended deployment contexts.

---

MEDIUM

M1 · packages/core/src/observations/shiplight-report.ts:2912 — observation_id embeds array index

observation_id: ["shiplight-report", reported, testCase ?? "test", index].join(":")

index is the zero-based position in parsed.tests. If the upstream Shiplight API returns results in a different order across runs (e.g., parallel runners, paging), the same test gets different observation_id values in different manifests. Since observation identity drives deduplication and resolution in the engine, this is a determinism defect on the identity path: two manifests for the same run could disagree on which observations exist. A stable key derived from reported + testCase + status (or a hash thereof) would be robust to reordering.

M2 · apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts:444 — escapesRoot does not cover the empty-string case

function escapesRoot(relative: string): boolean {
  return relative === ".." || relative.startsWith(`..${path.sep}`) || path.isAbsolute(relative);
}

When realResolved === realRoot (a request resolves to the project root directory itself), path.relative(realRoot, realResolved) returns "". escapesRoot("") returns false, so the route passes containment. The request is then rejected by the extension check (the root has no extension → 415) and the isFile() check (a directory → 404), so there is no security hole. However the containment function is logically incomplete — it relies on downstream guards rather than being self-sufficient. Add || contained === "" to escapesRoot or add an explicit directory guard before the extension lookup.

---

LOW

L1 · packages/ui/package-size.json — baseline bump without named approver

The quality-ui baseline rises from 37350 → 39089 packed bytes (4.7% increase) by updating baselinePackedBytes and baselineUnpackedBytes. The PR description asserts "quality-ui is re-baselined, which is itself the approval." Unlike quality-tools, there is no approvedIncrease field with a named human — the re-baseline is implicit. Consider adding an analogous record (even a comment) so future auditors know who authorized the increase and when.

L2 · packages/quality-tools/src/commands/sources.ts — --help position inconsistency

quality-tools sources --help works correctly (help check runs before subcommand dispatch), but quality-tools sources schema --help prints the schema rather than schema-specific help because the argv.includes("--help") check is on argv not on the subcommand-specific argv slice. Minor UX inconsistency; consistent behavior would check for --help after the subcommand is identified.

L3 · agent-skills/quality/references/improve/index.md — removed schema asset paths without explicit replacement commands

The vendored .schema.json files are deleted and the skill text now says to read INVALID_* diagnostics instead. An agent following the skill to pre-validate a config file would benefit from an explicit pointer to quality-tools sets schema, quality-tools sources schema, quality-tools views schema as the replacement for the deleted assets. Without it, an agent might attempt to locate the schemas through file-system search and not find them.

L4 · Stale PR description vs. diff

The PR description lists both size gates as failing and says the PR is not mergeable, but the diff includes both the quality-tools approvedIncrease and the quality-ui baseline rebasing. The description should be updated to reflect what is actually in the diff.

---

Summary

Severity	#	Key issue
CRITICAL	0	—
HIGH	2	H1: approvedIncrease authorship ambiguity violates CLAUDE.md invariant; H2: .js-as-same-origin-script risk requires explicit human sign-off
MEDIUM	2	M1: array-index in observation_id breaks determinism under reordering; M2: escapesRoot gap covered by downstream guards
LOW	4	Package-size approval hygiene; --help UX; skill schema pointers; stale PR body

Required before merge:

1. A human maintainer must attest in a comment that approvedIncrease in packages/quality-tools/package-size.json was typed by hand, not generated.
2. A human security owner must confirm the .js-as-same-origin-script threat model is acceptable for all intended deployment contexts.
3. Fix the unstable observation_id in shiplight-report.ts to use content-derived identity rather than array index.
4. Fix or explicitly justify the escapesRoot("") gap.

[claude]

Review: feat(quality) link run evidence from checks to the report behind them

This is a carefully designed PR that preserves all project invariants. The host transport seam is correctly bounded (fetch-and-shape only; the engine owns all normalization, identity, and scoring), containment is enforced with real-path comparison at every boundary, and schema drift is genuinely closed as a class by emitting schemas from engine constants.

INDEPENDENCE OF VERIFICATION: The host transport contract is correctly constrained. executeHostProfile hands raw records to normalizeObservationBatches, the same path every file-based transport uses. A handler cannot accept a status the engine normalizer would reject, and no score is computed outside the engine. The contract test at tests/contract/observation-run-evidence.contract.test.ts pins this explicitly (status "passed" from the host produces an empty observation set, not a "pass" record).

HUMAN-GATED FIELDS: No changes to structure_provenance or any provenance promotion path.

DETERMINISM: No Date.now(), Math.random(), or unstable iteration on scoring paths. The observation_id in shiplight-report.ts uses array index but resolution joins on path + test_case, so index drift in the report never affects a score.

DEPENDENCY DIRECTION: local-reports.ts lives in packages/core and imports only from core-internal modules. No upward dependencies introduced.

EXPLORER READ-ONLY: The evidence-file route is GET-only. No write, mkdir, or delete.

PROJECT-ROOT CONFINEMENT: Both the evidence-file route and resolveOption in local-reports.ts use the same two-layer containment: lexical check first (so a missing target does not become "nothing to contain"), then realpathSync comparison (so committed symlinks are followed before comparison). Defense-in-depth: the route re-checks at request time even when a ref was recorded at config time.

SAVED-ARTIFACT COMPATIBILITY: artifacts is optional and additive at every layer. Old files without it parse identically. No schema_version bump needed.

PUBLISHED SURFACE: New sets, sources, views commands are additive. New --artifact-ref/--artifact-label flags are additive. Existing command names and flags are unchanged.

AGENT-SKILL INSTRUCTIONS: The template update adds the host transport block as a comment for human authors to copy, not an instruction for an agent to execute. The removed vendored schema files are replaced by the CLI command pattern. No instructions that could cause force-push, reset, branch deletion, or admin-merge.

---

FINDINGS

LOW: escaped() in local-reports.ts does not guard relative === "" (packages/core/src/observation-sources/local-reports.ts, the escaped lambda inside resolveOption)

The escapesRoot() function in the evidence-file route explicitly returns true for relative === "" (target IS the root itself). The escaped lambda in local-reports.ts does not include that case. With report: "." the function returns false, producing an empty-string ref. The UI drops empty refs (segments.length === 0 returns undefined), and the evidence-file route rejects the root request with 403 anyway. Practically harmless, but the inconsistency is a subtle divergence from the route containment model. Adding the empty-string guard would make the two checks identical.

LOW: Same-origin .js serving + unsafe-eval/unsafe-inline is a documented XSS escalation path (apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts, lines 56-67 and 100-130)

The PR correctly documents and accepts this trade-off: a Playwright trace viewer requires its trace/*.js siblings, and sandbox breaks sibling fetches. The consequence is that any .js in the scanned project is reachable as same-origin script, meaning a hostile HTML report can call the Quality Explorer API from within the evidence frame. connect-src: self (no remote exfiltration), frame-ancestors: none (no remote framing), and the loopback trust model bound the blast radius. The invariant is the loopback address, not the CSP. The docs update in inspect-in-the-browser.md makes this explicit. No code change suggested; the trade-off analysis in the inline comments is accurate.

LOW: approvedIncrease.version in quality-tools/package-size.json must track package.json at release time (packages/quality-tools/package-size.json)

The PR description already calls this out: a release PR bumping quality-tools to 0.3.3 must simultaneously update approvedIncrease.version or the size gate will fail. Not a defect in this PR; noting it so the release PR has a concrete checklist item.

Nit: countProblems filter is inlined in execute.ts rather than calling the exported helper (packages/core/src/observation-sets/execute.ts, the allDiagnostics.filter(...) expression at the set-status line)

execute.ts inlines allDiagnostics.filter((entry) => entry.severity !== "info").length rather than calling countProblems from ingest-helpers.ts, which was extracted for exactly this purpose. They are equivalent today; if the definition of "problem" ever changes, there are now two sites to update.

---

OVERALL: All invariants from CLAUDE.md are preserved. Test coverage is comprehensive: contract tests pin the opaque-ref boundary and verify the host transport cannot bypass normalization, integration tests cover the symlink escape and macOS /var symlink-root cases, and the route tests cover every refusal case. No CRITICAL, HIGH, or MEDIUM findings.