From 627ce25b1b1db88b082d0c037c4944e084ee0c9e Mon Sep 17 00:00:00 2001 From: Feng Qian Date: Fri, 4 Sep 2026 11:06:02 -0700 Subject: [PATCH 1/7] feat(quality): link run evidence from checks to the report behind them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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, and the report the runner had already written was unreachable from the quality graph. Observations now carry opaque pointers to that report, and the feature page links them under each check's proof. The 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. - The pointer is 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. 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. Reading results from somewhere the engine has no business knowing about now has a seam: `transport: host` plus a handler registry the embedding application supplies. 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` is the bundled reference transport and the local half of the story. It reads a Playwright JSON report, or a Shiplight YAML run's `report-data.json` — the latter keyed back onto the `.test.yaml` source, because the transpiled spec it reports is gitignored and absent from a fresh checkout, so no quality map can honestly pin it. Quality Explorer serves project-relative refs over its own origin so a report's relative video and trace links resolve. Containment compares real paths, the extension allowlist keeps a checked-in script from ever reaching a browser, and served documents carry a CSP that cuts the cheap exfiltration routes — a reduction, not containment, as the header comment says. Also closes a drift class rather than one instance: every config schema is now emitted from the engine's own constants via `quality-tools 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. Verified against real repos: 277 CI observations across four shipyard workflows still parse unchanged, and shipyard's own YAML e2e run reaches its check with a working link to the report. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NhxpEed9pbcjT7MLtnDbiH --- .../assets/observation-sets.schema.json | 47 -- .../assets/observation-sources.schema.json | 109 ---- .../assets/observation-sources.template.yaml | 26 +- .../quality/references/improve/index.md | 86 ++- .../evidence-file/[...ref]/route.ts | 179 ++++++ .../evidence-file/route.test.ts | 133 +++++ .../src/app/quality-explorer/layout.tsx | 3 + .../quality-explorer/data-access/local-fs.ts | 19 +- docs/how-to/make-ci-results-count.md | 37 ++ packages/core/package.json | 103 +++- packages/core/src/observation-sets/execute.ts | 10 +- packages/core/src/observation-sets/index.ts | 1 + .../core/src/observation-sets/json-schema.ts | 67 +++ .../observation-sets.schema.json | 40 +- packages/core/src/observation-sets/parse.ts | 5 +- packages/core/src/observation-sets/types.ts | 5 + .../core/src/observation-sources/execute.ts | 135 ++++- .../core/src/observation-sources/index.ts | 2 + .../src/observation-sources/json-schema.ts | 105 ++++ .../src/observation-sources/local-reports.ts | 228 ++++++++ .../observation-sources.schema.json | 147 ++++- .../core/src/observation-sources/parse.ts | 94 ++- .../core/src/observation-sources/serialize.ts | 14 +- .../core/src/observation-sources/types.ts | 52 +- packages/core/src/observations/evaluate.ts | 5 +- packages/core/src/observations/index.ts | 1 + packages/core/src/observations/manifest.ts | 93 ++- packages/core/src/observations/normalize.ts | 27 +- .../core/src/observations/playwright-json.ts | 52 +- .../quality-observations.schema.json | 21 + packages/core/src/observations/resolve.ts | 5 +- .../core/src/observations/shiplight-report.ts | 160 ++++++ packages/core/src/observations/types.ts | 37 ++ packages/core/src/operations/index.ts | 12 + .../core/src/recommendation-export/index.ts | 6 +- packages/core/src/views/index.ts | 1 + packages/core/src/views/json-schema.ts | 59 ++ packages/core/src/views/parse.ts | 17 + packages/core/src/views/types.ts | 5 + .../core/src/views}/views.schema.json | 31 +- packages/core/tsup.config.ts | 2 +- packages/quality-tools/src/cli.ts | 13 + .../quality-tools/src/commands/analyze.ts | 5 + .../src/commands/observations.test.ts | 120 ++++ .../src/commands/observations.ts | 39 +- .../quality-tools/src/commands/sources.ts | 85 +++ packages/ui/src/components/FeaturePage.tsx | 137 ++++- .../components/ObservationAuditPanel.test.tsx | 151 +++++ .../src/components/ObservationAuditPanel.tsx | 89 ++- .../src/components/ObservationSourcesView.tsx | 21 +- packages/ui/src/components/ProjectScanner.tsx | 26 + packages/ui/src/components/Settings.tsx | 3 +- packages/ui/src/components/scan-cache.tsx | 55 +- packages/ui/src/host.tsx | 12 +- scripts/check-quality-skill.sh | 11 + .../observation-run-evidence.contract.test.ts | 536 ++++++++++++++++++ .../local-reports-run-evidence.test.ts | 448 +++++++++++++++ 57 files changed, 3602 insertions(+), 330 deletions(-) delete mode 100644 agent-skills/quality/references/improve/assets/observation-sets.schema.json delete mode 100644 agent-skills/quality/references/improve/assets/observation-sources.schema.json create mode 100644 apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts create mode 100644 apps/explorer/src/app/api/quality-explorer/evidence-file/route.test.ts create mode 100644 packages/core/src/observation-sets/json-schema.ts create mode 100644 packages/core/src/observation-sources/json-schema.ts create mode 100644 packages/core/src/observation-sources/local-reports.ts create mode 100644 packages/core/src/observations/shiplight-report.ts create mode 100644 packages/core/src/views/json-schema.ts rename {agent-skills/quality/references/improve/assets => packages/core/src/views}/views.schema.json (61%) create mode 100644 packages/quality-tools/src/commands/sources.ts create mode 100644 packages/ui/src/components/ObservationAuditPanel.test.tsx create mode 100644 tests/contract/observation-run-evidence.contract.test.ts create mode 100644 tests/integration/local-reports-run-evidence.test.ts diff --git a/agent-skills/quality/references/improve/assets/observation-sets.schema.json b/agent-skills/quality/references/improve/assets/observation-sets.schema.json deleted file mode 100644 index 1ed6172..0000000 --- a/agent-skills/quality/references/improve/assets/observation-sets.schema.json +++ /dev/null @@ -1,47 +0,0 @@ -{ - "$schema": "http://json-schema.org/draft-07/schema#", - "$id": "https://shiplight.dev/schemas/quality/observation-sets.schema.json", - "title": "Quality Observation Sets", - "type": "object", - "additionalProperties": false, - "required": ["observation_sets"], - "properties": { - "observation_sets": { - "type": "array", - "minItems": 1, - "items": { - "$ref": "#/definitions/observationSet" - } - } - }, - "definitions": { - "nonEmptyString": { - "type": "string", - "minLength": 1 - }, - "profileReference": { - "type": "object", - "additionalProperties": false, - "required": ["profile_id"], - "properties": { - "profile_id": { "$ref": "#/definitions/nonEmptyString" } - } - }, - "observationSet": { - "type": "object", - "additionalProperties": false, - "required": ["id", "name", "profiles"], - "properties": { - "id": { "$ref": "#/definitions/nonEmptyString" }, - "name": { "$ref": "#/definitions/nonEmptyString" }, - "description": { "$ref": "#/definitions/nonEmptyString" }, - "profiles": { - "type": "array", - "minItems": 1, - "uniqueItems": true, - "items": { "$ref": "#/definitions/profileReference" } - } - } - } - } -} diff --git a/agent-skills/quality/references/improve/assets/observation-sources.schema.json b/agent-skills/quality/references/improve/assets/observation-sources.schema.json deleted file mode 100644 index c87bdb1..0000000 --- a/agent-skills/quality/references/improve/assets/observation-sources.schema.json +++ /dev/null @@ -1,109 +0,0 @@ -{ - "$schema": "http://json-schema.org/draft-07/schema#", - "$id": "https://shiplight.dev/schemas/quality/observation-sources.schema.json", - "title": "Quality Observation Source Profiles", - "type": "object", - "additionalProperties": false, - "required": ["profiles"], - "properties": { - "profiles": { - "type": "array", - "minItems": 1, - "items": { - "$ref": "#/definitions/profile" - } - } - }, - "definitions": { - "nonEmptyString": { - "type": "string", - "minLength": 1 - }, - "sourceRef": { - "type": "object", - "additionalProperties": false, - "minProperties": 1, - "properties": { - "path": { "$ref": "#/definitions/nonEmptyString" }, - "url": { "$ref": "#/definitions/nonEmptyString" }, - "label": { "$ref": "#/definitions/nonEmptyString" } - } - }, - "auth": { - "type": "object", - "additionalProperties": false, - "properties": { - "required_env": { - "type": "array", - "items": { "$ref": "#/definitions/nonEmptyString" } - } - } - }, - "github": { - "type": "object", - "additionalProperties": false, - "required": ["repo", "workflow", "artifact_names"], - "properties": { - "repo": { "$ref": "#/definitions/nonEmptyString" }, - "workflow": { "$ref": "#/definitions/nonEmptyString" }, - "artifact_names": { - "type": "array", - "minItems": 1, - "items": { "$ref": "#/definitions/nonEmptyString" } - }, - "branch": { "$ref": "#/definitions/nonEmptyString" } - } - }, - "localFolder": { - "type": "object", - "additionalProperties": false, - "required": ["path"], - "properties": { - "path": { "$ref": "#/definitions/nonEmptyString" } - } - }, - "profile": { - "type": "object", - "additionalProperties": false, - "required": ["id", "name", "transport", "observation_path"], - "properties": { - "id": { "$ref": "#/definitions/nonEmptyString" }, - "name": { "$ref": "#/definitions/nonEmptyString" }, - "description": { "$ref": "#/definitions/nonEmptyString" }, - "transport": { - "enum": ["github-actions", "local-folder"] - }, - "observation_path": { "$ref": "#/definitions/nonEmptyString" }, - "source_refs": { - "type": "array", - "items": { "$ref": "#/definitions/sourceRef" } - }, - "auth": { "$ref": "#/definitions/auth" }, - "github": { "$ref": "#/definitions/github" }, - "local_folder": { "$ref": "#/definitions/localFolder" } - }, - "allOf": [ - { - "if": { - "properties": { - "transport": { "const": "github-actions" } - } - }, - "then": { - "required": ["github"] - } - }, - { - "if": { - "properties": { - "transport": { "const": "local-folder" } - } - }, - "then": { - "required": ["local_folder"] - } - } - ] - } - } -} diff --git a/agent-skills/quality/references/improve/assets/observation-sources.template.yaml b/agent-skills/quality/references/improve/assets/observation-sources.template.yaml index c443e11..0115ba0 100644 --- a/agent-skills/quality/references/improve/assets/observation-sources.template.yaml +++ b/agent-skills/quality/references/improve/assets/observation-sources.template.yaml @@ -22,20 +22,40 @@ profiles: - "quality-observations-*" # branch: "main" - # For a local source: + # For a local source that reads a canonical file already on disk: # - change `transport` above to `"local-folder"` # - remove both `auth` and `github` # - add this block: # local_folder: # path: "artifacts/quality" + # For results the reading application fetches itself — a local test report, + # a platform that already holds the run — use the host transport. The + # provider name is resolved by whoever reads this repo, so which providers + # exist depends on the application, not on Quality: + # - change `transport` above to `"host"` + # - remove `observation_path`, `auth`, and `github` + # - add this block: + # host: + # provider: "local-reports" + # options: + # path: "playwright-report/report.json" + # report: "playwright-report/index.html" + # Authoring rules: -# - One profile represents one workflow or local folder. -# - observation_path is relative to the downloaded artifact or local_folder. +# - One profile represents one workflow, local folder, or host provider. +# - observation_path is relative to the downloaded artifact or local_folder. A +# host profile omits it: it addresses no file. # - GitHub artifact_names may select several uploaded archives from one run. # Every matching observation_path uses the same canonical contract; Quality # merges their observations. # - A local-folder profile reads exactly one observation_path. +# - A host profile names `host.provider`. An application that does not register +# that provider reports it as a diagnostic rather than reading nothing, so a +# profile written for one reader stays legible to another. +# - `local-reports` is the provider Quality ships: it reads a Playwright JSON +# report from the working tree and points each result at the HTML report a +# reviewer opens. It records no commit unless `options.commit` pins one. # - Raw JUnit, Playwright, telemetry, or custom reports are producer inputs. # Convert them in the workflow with `quality-tools observations`; source # configuration contains no parser or format selection. diff --git a/agent-skills/quality/references/improve/index.md b/agent-skills/quality/references/improve/index.md index 59326a8..0ee03ed 100644 --- a/agent-skills/quality/references/improve/index.md +++ b/agent-skills/quality/references/improve/index.md @@ -94,7 +94,8 @@ Classify each gap before editing: 3. **Evidence strength:** a mapped method cannot establish the full claim or lacks the required execution context/gate. 4. **Source acquisition:** credentials, repository/workflow selection, artifact - names, or local-folder path prevent results from loading. + names, local-folder path, or an unregistered host provider prevent results + from loading. 5. **Artifact emission:** the workflow emits no canonical observation file or emits it at the wrong path. 6. **Producer format:** the canonical file has an invalid version, envelope, @@ -179,7 +180,7 @@ acquisition and resolution have been ruled out. ``` -- Observation config: compare with the configuration schemas in `assets/`, then +- Observation config: compare with the schemas the engine emits (see above), then run the relevant assessment. Engine diagnostics verify acquisition and graph joins. - Implementation or verification-method changes: run their owning verification command before @@ -209,23 +210,39 @@ or requires a human decision. - `.quality/config/observation-sets.yaml` - `.quality/config/views.yaml` -Use the configuration templates and schemas under `assets/`. Use -`quality-observations.template.json` as the canonical output example; obtain its -current schema from `quality-tools observations schema`, not a bundled copy. +Use the configuration templates under `assets/`. Obtain the current schemas +from the engine rather than a bundled copy, which cannot be checked against the +contract and drifts the moment it moves: -### Sources +- observation manifest: `quality-tools observations schema` +- observation sources: `quality-tools sources schema` +- observation sets: `quality-tools sets schema` +- saved views: `quality-tools views schema` -One profile represents one acquisition integration, such as one GitHub Actions -workflow or one local result folder. It answers only: +Use `quality-observations.template.json` as the canonical output example. -- which transport fetches results: `github-actions` or `local-folder` -- which `observation_path` contains canonical `quality-observations.json` - content +### Sources -A source never selects a parser. Raw JUnit, Playwright, telemetry, or custom -gate output must be converted by its producer before the source reads it. Do -not create a source profile until the canonical file exists or its emit step is -being added in the same authorized change. +One profile represents one acquisition integration, such as one GitHub Actions +workflow, one local result folder, or one provider the reading application +supplies. It answers only: + +- which transport fetches results: `github-actions`, `local-folder`, or `host` +- for the two file transports, which `observation_path` contains canonical + `quality-observations.json` content +- for `host`, which `host.provider` the reading application resolves + +A file-based source never selects a parser. Raw JUnit, Playwright, telemetry, +or custom gate output must be converted by its producer before the source reads +it. Do not create a source profile until the canonical file exists or its emit +step is being added in the same authorized change. + +A `host` profile is the exception, and only because the reading application — +not this configuration — owns the fetch. Its provider may read a native report +directly. The engine still normalizes, resolves, and diagnoses every record it +returns, so a host provider gets no record past a check a canonical file must +pass. Which providers resolve depends on who reads the repo; one that is not +registered is reported as a diagnostic rather than read as nothing. A local-folder profile reads one file. A GitHub Actions profile may select several uploaded artifacts from one workflow run; every matching @@ -329,6 +346,38 @@ Follow this sequence. Do not ask the user to choose a parser or config shape. GitHub Actions metadata comes from `GITHUB_SHA`, `GITHUB_REF_NAME`, and `GITHUB_RUN_ID`. Outside GitHub Actions, supply `--commit`; `--branch`, `--run-id`, `--run-url`, and `--observed-at` are optional. + + **Run evidence** (requires a `quality-tools` newer than 0.3.2; the published + validator rejects the field until then). A record may carry pointers to what + the run left behind, so a reviewer opening a check can see what the test did: + + ```json + { + "path": "tests/e2e/checkout.test.yaml", + "test_case": "guest can pay", + "status": "pass", + "artifacts": [ + { "ref": "https://app.example.test/runs/8412?test=99231", + "label": "Run 8412" } + ] + } + ``` + + Not to be confused with a workflow's uploaded artifacts, which are how the + canonical file travels. These are pointers INSIDE it. + + - `ref` is opaque. Quality records and displays it; it never parses, + resolves, or validates it. An absolute `http(s)` ref is linked as it + stands; anything else is a path into the project, which only a reader that + has the project can open. + - Point at the report a person already knows how to read — the run page, the + HTML report — not at each video and screenshot. Quality is an index from + checks to evidence, not a viewer. + - Optional and additive. A record without it still counts exactly the same; + only the link to look at the result is missing. + - A malformed entry is reported and dropped without costing the record its + observed status. The status is the measurement; the ref is only how a + reviewer looks at it. 4. **Schema-validate before upload.** ```bash @@ -346,9 +395,10 @@ Follow this sequence. Do not ask the user to choose a parser or config shape. every one uses the same contract. Raw native reports may remain alongside the canonical file for diagnosis; the quality engine never parses them. 6. **Configure the transport.** Copy - `assets/observation-sources.template.yaml`. Set `transport`, - `observation_path`, and either `github` or `local_folder`. Source - configuration contains no parser list or format selection. + `assets/observation-sources.template.yaml`. Set `transport`, then either + `observation_path` plus `github` or `local_folder` for a file transport, or + `host.provider` for a host transport. File-transport configuration contains + no parser list or format selection. 7. **Add the profile to an observation set.** 8. **Run `assess`.** Verify source acquisition first, then verify every observation resolves to the intended evidence identity. Use the engine's diff --git a/apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts b/apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts new file mode 100644 index 0000000..dd44857 --- /dev/null +++ b/apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts @@ -0,0 +1,179 @@ +import { createReadStream, realpathSync, statSync } from "node:fs"; +import path from "node:path"; +import { Readable } from "node:stream"; +import { requireQcSession } from "@/lib/quality-explorer/require-session"; +import { problemResponse as problem } from "@/lib/quality-explorer/route-problem"; +import { qualityProjectRoot } from "@/lib/quality-explorer/project-root"; + +export const runtime = "nodejs"; + +// Serves a run-evidence file out of the opened project so a reviewer can open +// the report the producer already wrote. +// +// The path is in the URL rather than a query parameter on purpose. A Playwright +// HTML report inlines its own summary but fetches attachments from a sibling +// `data/` directory using RELATIVE urls, so the report only works if its own +// address has the same shape as its folder — `.../evidence-file/report/index.html` +// resolves `data/x.webm` to `.../evidence-file/report/data/x.webm`, while a +// `?ref=` query would resolve it against the route and 404. +// +// Trust boundary: this serves files from the local project the user themselves +// opened, on a loopback-bound single-user dev server, and only the extensions +// below. It is not a general static file server — an unlisted extension is +// refused rather than guessed at, which is what keeps a checked-in `.command` +// or `.sh` in a scanned repo from ever being handed to a browser. +const CONTENT_TYPES: Readonly> = { + ".html": "text/html; charset=utf-8", + ".htm": "text/html; charset=utf-8", + ".css": "text/css; charset=utf-8", + ".js": "text/javascript; charset=utf-8", + ".json": "application/json; charset=utf-8", + ".txt": "text/plain; charset=utf-8", + ".log": "text/plain; charset=utf-8", + ".md": "text/plain; charset=utf-8", + ".csv": "text/csv; charset=utf-8", + ".png": "image/png", + ".jpg": "image/jpeg", + ".jpeg": "image/jpeg", + ".gif": "image/gif", + ".webp": "image/webp", + ".svg": "image/svg+xml", + ".ico": "image/x-icon", + ".webm": "video/webm", + ".mp4": "video/mp4", + ".mov": "video/quicktime", + ".ogg": "audio/ogg", + ".mp3": "audio/mpeg", + ".pdf": "application/pdf", + ".zip": "application/zip", + ".woff": "font/woff", + ".woff2": "font/woff2", + ".ttf": "font/ttf", +}; + +// `.js` and `.css` are here because a report is a web page and needs its own +// assets; they are served, never executed by this process. +const MAX_BYTES = 512 * 1024 * 1024; + +// Served HTML and SVG come from a scanned repo, which is not a trusted author, +// and they run on this application's own origin — so without a policy they +// could call its API and post what they read to a remote host. +// +// `sandbox` is NOT the answer here: an opaque origin turns the report's own +// sibling requests into cross-origin ones, breaking the trace and video that +// are the whole point, and the CORS header needed to restore them would open +// this local file server to every site the viewer has open. +// +// So the origin is kept and the cheap exfiltration routes are cut instead. +// `'self'` lets the report load its own assets and fetch its own `data/` +// directory, while every remote destination for fetch, XHR, beacon, image, +// script and form post is refused. `unsafe-inline`/`unsafe-eval` are required +// by the report's own bundle, and withholding them would only break honest +// reports. +// +// This is a reduction, NOT containment, and the difference matters: CSP cannot +// stop a top-level navigation, since the `navigate-to` directive that would +// have was dropped from the spec. A hostile page in a scanned repo can still +// read what this origin serves and then put it in a URL it navigates to. +// Closing that needs a separate origin for evidence, which one loopback dev +// server cannot provide — so the real boundary remains "only open a project you +// would run tests from". +const DOCUMENT_CSP = [ + "default-src 'self'", + "script-src 'self' 'unsafe-inline' 'unsafe-eval'", + "style-src 'self' 'unsafe-inline'", + "img-src 'self' data: blob:", + "media-src 'self' data: blob:", + "font-src 'self' data:", + "connect-src 'self'", + "form-action 'none'", + "frame-ancestors 'none'", + "base-uri 'none'", + "object-src 'none'" +].join("; "); + +const SCRIPTABLE_TYPES = new Set([".html", ".htm", ".svg"]); + +export async function GET( + _request: Request, + context: { params: Promise<{ ref: readonly string[] }> }, +): Promise { + const unauthorized = await requireQcSession(); + if (unauthorized) return unauthorized; + + const { ref } = await context.params; + if (!Array.isArray(ref) || ref.length === 0) { + return problem(400, "invalid-evidence-ref", "An evidence file path is required."); + } + + const projectRoot = qualityProjectRoot(); + // Segments are used AS GIVEN. Next has already percent-decoded each one, so + // decoding again is not a no-op on a name that legitimately contains `%`: a + // Playwright artifact folder named after a test title like `100% progress` + // arrives correctly decoded and a second pass throws, while a file actually + // named `a%20b.png` silently resolves to `a b.png` — a different file. + const relative = ref.join("/"); + + // Containment, not sanitisation: resolve first, then require the result to sit + // under the project root. Stripping `..` textually misses encoded forms; + // comparing the resolved path does not. + // + // `path.resolve` is purely lexical, so it is not enough on its own: a repo + // containing `report -> /Users/someone` would pass a lexical check and then + // serve files from outside the project. Both the real path AND the project + // root are realpath'd before comparison, so a symlinked checkout still + // resolves against its own real location rather than failing spuriously. + const resolved = path.resolve(projectRoot, relative); + let realResolved: string; + let realRoot: string; + try { + realRoot = realpathSync(projectRoot); + realResolved = realpathSync(resolved); + } catch { + return problem(404, "evidence-ref-not-found", "That evidence file could not be read."); + } + + const contained = path.relative(realRoot, realResolved); + if (contained.startsWith("..") || path.isAbsolute(contained)) { + return problem(403, "evidence-ref-outside-project", "That path is outside the opened project."); + } + + const contentType = CONTENT_TYPES[path.extname(realResolved).toLowerCase()]; + if (contentType === undefined) { + return problem( + 415, + "evidence-ref-unsupported-type", + "Quality Explorer does not serve that file type as run evidence.", + ); + } + + let size: number; + try { + const stats = statSync(realResolved); + if (!stats.isFile()) { + return problem(404, "evidence-ref-not-found", "That evidence file could not be read."); + } + size = stats.size; + } catch { + return problem(404, "evidence-ref-not-found", "That evidence file could not be read."); + } + + if (size > MAX_BYTES) { + return problem(413, "evidence-ref-too-large", "That evidence file is too large to serve."); + } + + const stream = Readable.toWeb(createReadStream(realResolved)) as ReadableStream; + return new Response(stream, { + headers: { + "content-type": contentType, + "content-length": String(size), + // The declared type is the one that was matched from the extension, so + // sniffing could only ever disagree with it. + "x-content-type-options": "nosniff", + "cache-control": "no-store", + ...(SCRIPTABLE_TYPES.has(path.extname(realResolved).toLowerCase()) + ? { "content-security-policy": DOCUMENT_CSP } + : {}), + }, + }); +} diff --git a/apps/explorer/src/app/api/quality-explorer/evidence-file/route.test.ts b/apps/explorer/src/app/api/quality-explorer/evidence-file/route.test.ts new file mode 100644 index 0000000..d42de9a --- /dev/null +++ b/apps/explorer/src/app/api/quality-explorer/evidence-file/route.test.ts @@ -0,0 +1,133 @@ +import { mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +// The evidence-file route serves run-evidence out of the opened project. Its +// whole job is deciding what NOT to serve, so the cases below are mostly about +// refusal: a path that escapes the project, a type that is not display +// evidence, and names that percent-decoding can quietly turn into other files. + +let root: string; + +function projectFile(relative: string, contents: string): void { + const target = path.join(root, relative); + mkdirSync(path.dirname(target), { recursive: true }); + writeFileSync(target, contents, "utf8"); +} + +async function get(...segments: readonly string[]): Promise { + const { GET } = await import("./[...ref]/route"); + return GET(new Request("http://127.0.0.1/evidence"), { + params: Promise.resolve({ ref: segments }), + }); +} + +beforeEach(() => { + root = mkdtempSync(path.join(tmpdir(), "qc-evidence-route-")); + process.env.QUALITY_PROJECT_ROOT = root; + // project-root caches its resolution per module instance. + vi.resetModules(); +}); + +afterEach(() => { + rmSync(root, { force: true, recursive: true }); + delete process.env.QUALITY_PROJECT_ROOT; +}); + +describe("evidence-file route", () => { + it("serves a report with its content type and a document CSP", async () => { + projectFile("playwright-report/index.html", "

report

"); + + const response = await get("playwright-report", "index.html"); + + expect(response.status).toBe(200); + expect(response.headers.get("content-type")).toContain("text/html"); + expect(response.headers.get("x-content-type-options")).toBe("nosniff"); + // Served HTML comes from a scanned repo, which is not a trusted author. + expect(response.headers.get("content-security-policy")).toContain("default-src 'self'"); + expect(await response.text()).toBe("

report

"); + }); + + it("does not send a document CSP on a non-scriptable asset", async () => { + projectFile("playwright-report/data/video.webm", "binary"); + + const response = await get("playwright-report", "data", "video.webm"); + + expect(response.status).toBe(200); + expect(response.headers.get("content-type")).toBe("video/webm"); + expect(response.headers.get("content-security-policy")).toBeNull(); + }); + + it("uses path segments as given rather than decoding them a second time", async () => { + // Next has already percent-decoded each segment. Decoding again is not a + // no-op on a name that legitimately contains `%`: this file would resolve + // to `a b.txt` instead, and a name like `100% progress` would throw. + projectFile("probe/a%20b.txt", "literal-percent-name"); + projectFile("probe/a b.txt", "space-name"); + + expect(await (await get("probe", "a%20b.txt")).text()).toBe("literal-percent-name"); + expect(await (await get("probe", "a b.txt")).text()).toBe("space-name"); + }); + + it("serves a name containing a percent sign rather than failing to decode it", async () => { + // Playwright names artifact folders after the test title, so `%` in a title + // reaches the filesystem. + projectFile("probe/100% progress.txt", "percent-in-name"); + + const response = await get("probe", "100% progress.txt"); + + expect(response.status).toBe(200); + expect(await response.text()).toBe("percent-in-name"); + }); + + it("refuses a path that escapes the project root", async () => { + // Pointed at a file that really exists outside the root, so the refusal is + // the containment check rather than the target simply being absent. + const outside = mkdtempSync(path.join(tmpdir(), "qc-evidence-outside-")); + writeFileSync(path.join(outside, "secret.txt"), "secret", "utf8"); + + try { + const response = await get("..", path.basename(outside), "secret.txt"); + expect(response.status).toBe(403); + } finally { + rmSync(outside, { force: true, recursive: true }); + } + }); + + it("reports an escape to a path that does not exist as not found", async () => { + expect((await get("..", "..", "nope", "absent.txt")).status).toBe(404); + }); + + it("refuses a symlink that resolves outside the project root", async () => { + // Lexical containment cannot see this: `path.resolve` never follows links, + // so the check has to compare real paths. + const outside = mkdtempSync(path.join(tmpdir(), "qc-evidence-outside-")); + writeFileSync(path.join(outside, "secret.txt"), "secret", "utf8"); + symlinkSync(outside, path.join(root, "escape")); + + try { + expect((await get("escape", "secret.txt")).status).toBe(403); + } finally { + rmSync(outside, { force: true, recursive: true }); + } + }); + + it("refuses a file type that is not display evidence", async () => { + // An allowlist, so a checked-in `.command` or `.sh` in a scanned repo is + // never handed to a browser. + projectFile("scripts/run.sh", "#!/bin/sh\necho hi"); + + expect((await get("scripts", "run.sh")).status).toBe(415); + }); + + it("reports a missing file as not found", async () => { + expect((await get("playwright-report", "absent.html")).status).toBe(404); + }); + + it("refuses a directory", async () => { + mkdirSync(path.join(root, "playwright-report"), { recursive: true }); + + expect((await get("playwright-report")).status).toBe(415); + }); +}); diff --git a/apps/explorer/src/app/quality-explorer/layout.tsx b/apps/explorer/src/app/quality-explorer/layout.tsx index da90861..f325de1 100644 --- a/apps/explorer/src/app/quality-explorer/layout.tsx +++ b/apps/explorer/src/app/quality-explorer/layout.tsx @@ -15,6 +15,9 @@ const host: QcUiHost = { routeBase: "/quality-explorer", apiBase: "/api/quality-explorer", setProject: setQcProjectAction, + // This host reads the project from the local filesystem, so it can serve the + // report a run-evidence ref points at. See the evidence-file route. + servesEvidenceFiles: true, }; export default function QualityExplorerLayout({ diff --git a/apps/explorer/src/lib/quality-explorer/data-access/local-fs.ts b/apps/explorer/src/lib/quality-explorer/data-access/local-fs.ts index 6525857..646de17 100644 --- a/apps/explorer/src/lib/quality-explorer/data-access/local-fs.ts +++ b/apps/explorer/src/lib/quality-explorer/data-access/local-fs.ts @@ -7,9 +7,22 @@ import { readMarkdownArtifactOp, scanOp, } from "@shiplightai/quality-core/operations"; +import { + createLocalReportsTransport, + LOCAL_REPORTS_PROVIDER, +} from "@shiplightai/quality-core"; import type { QcDataAccess } from "./types"; import { qualityProjectRoot } from "../project-root"; +// The host transports this application serves. Registration is deliberately the +// host's act, not the engine's: a repo can declare any provider, and which ones +// actually resolve is a property of who is reading the repo. Quality Explorer +// serves the bundled local-reports provider and nothing else — a profile naming +// a platform provider gets an explicit diagnostic here rather than silence. +const hostTransports = { + [LOCAL_REPORTS_PROVIDER]: createLocalReportsTransport(), +}; + // Draft/preview/publish sync has no meaning in local `qc` mode (no origin, no PR flow) — // mirror the box's `qc-sync-unavailable` 400 so routes surface it consistently. const syncUnavailable = (): never => { @@ -35,8 +48,10 @@ export const localFsDataAccess: QcDataAccess = { saveObservationSets: () => readOnly(), saveObservationSources: () => readOnly(), saveViews: () => readOnly(), - executeObservationSet: (input) => executeObservationSetOp({ ...input, projectPath: qualityProjectRoot() }), - executeObservationSource: (input) => executeObservationSourceOp({ ...input, projectPath: qualityProjectRoot() }), + executeObservationSet: (input) => + executeObservationSetOp({ ...input, projectPath: qualityProjectRoot(), hostTransports }), + executeObservationSource: (input) => + executeObservationSourceOp({ ...input, projectPath: qualityProjectRoot(), hostTransports }), syncStatus: () => syncUnavailable(), pull: () => syncUnavailable(), publish: () => syncUnavailable(), diff --git a/docs/how-to/make-ci-results-count.md b/docs/how-to/make-ci-results-count.md index 8f45c8c..d077328 100644 --- a/docs/how-to/make-ci-results-count.md +++ b/docs/how-to/make-ci-results-count.md @@ -36,6 +36,43 @@ Without that permission, the agent should propose the workflow change and stop. This work serializes a result that already happened. It must not change the test command, retry behavior, gate, or status. +## Link the run evidence (optional) + +Requires a `quality-tools` newer than 0.3.2. Until that ships, the published +validator rejects the field. + +A result can carry a pointer to what the run left behind, so a reviewer opening +a check can see what the test actually did rather than only whether it passed: + +```json +{ + "path": "tests/e2e/checkout.test.yaml", + "test_case": "guest can pay", + "status": "pass", + "artifacts": [ + { "ref": "https://app.example.test/runs/8412?test=99231", "label": "Run 8412" } + ] +} +``` + +Point at the report a person already knows how to read — the run page, the HTML +report the runner wrote. Quality links to it and stops there; it is an index +from checks to evidence, not a viewer, and it will not enumerate videos and +screenshots for you. + +`ref` is opaque: Quality records and displays it, and never parses or resolves +it. An absolute `http(s)` ref is linked as it stands. Anything else is read as a +path inside the project, which only a reader holding that project can open — so +a local report path shows as a link in Quality Explorer and as plain text in a +hosted reader. + +This is additive. A result without it counts exactly the same; only the link is +missing. And a malformed pointer never costs you the result it was attached to: +it is reported and dropped, and the observed status stands. + +Do not confuse these with the workflow artifacts that carry the canonical file +itself. These are pointers inside it. + ## Decision that remains yours You decide whether the workflow may be edited and which result sources belong in diff --git a/packages/core/package.json b/packages/core/package.json index 354cee4..e7910fc 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -12,26 +12,95 @@ "import": "./dist/index.js", "default": "./dist/index.js" }, - "./analytics": { "types": "./dist/analytics.d.ts", "import": "./dist/analytics.js", "default": "./dist/analytics.js" }, - "./evidence-view": { "types": "./dist/evidence-view.d.ts", "import": "./dist/evidence-view.js", "default": "./dist/evidence-view.js" }, - "./fix-prompts": { "types": "./dist/fix-prompts.d.ts", "import": "./dist/fix-prompts.js", "default": "./dist/fix-prompts.js" }, - "./observation-sets": { "types": "./dist/observation-sets.d.ts", "import": "./dist/observation-sets.js", "default": "./dist/observation-sets.js" }, - "./gap-triage": { "types": "./dist/gap-triage.d.ts", "import": "./dist/gap-triage.js", "default": "./dist/gap-triage.js" }, - "./observation-sources": { "types": "./dist/observation-sources.d.ts", "import": "./dist/observation-sources.js", "default": "./dist/observation-sources.js" }, - "./observations": { "types": "./dist/observations.d.ts", "import": "./dist/observations.js", "default": "./dist/observations.js" }, - "./operations": { "types": "./dist/operations.d.ts", "import": "./dist/operations.js", "default": "./dist/operations.js" }, - "./recommendation-export": { "types": "./dist/recommendation-export.d.ts", "import": "./dist/recommendation-export.js", "default": "./dist/recommendation-export.js" }, - "./owner-view": { "types": "./dist/owner-view.d.ts", "import": "./dist/owner-view.js", "default": "./dist/owner-view.js" }, - "./project-map": { "types": "./dist/project-map.d.ts", "import": "./dist/project-map.js", "default": "./dist/project-map.js" }, - "./project-index": { "types": "./dist/project-index.d.ts", "import": "./dist/project-index.js", "default": "./dist/project-index.js" }, - "./priority": { "types": "./dist/priority.d.ts", "import": "./dist/priority.js", "default": "./dist/priority.js" }, - "./assessment": { "types": "./dist/assessment.d.ts", "import": "./dist/assessment.js", "default": "./dist/assessment.js" }, - "./views": { "types": "./dist/views.d.ts", "import": "./dist/views.js", "default": "./dist/views.js" }, - "./views/server": { "types": "./dist/views-server.d.ts", "import": "./dist/views-server.js", "default": "./dist/views-server.js" }, - "./workspace": { "types": "./dist/workspace.d.ts", "import": "./dist/workspace.js", "default": "./dist/workspace.js" }, + "./analytics": { + "types": "./dist/analytics.d.ts", + "import": "./dist/analytics.js", + "default": "./dist/analytics.js" + }, + "./evidence-view": { + "types": "./dist/evidence-view.d.ts", + "import": "./dist/evidence-view.js", + "default": "./dist/evidence-view.js" + }, + "./fix-prompts": { + "types": "./dist/fix-prompts.d.ts", + "import": "./dist/fix-prompts.js", + "default": "./dist/fix-prompts.js" + }, + "./observation-sets": { + "types": "./dist/observation-sets.d.ts", + "import": "./dist/observation-sets.js", + "default": "./dist/observation-sets.js" + }, + "./gap-triage": { + "types": "./dist/gap-triage.d.ts", + "import": "./dist/gap-triage.js", + "default": "./dist/gap-triage.js" + }, + "./observation-sources": { + "types": "./dist/observation-sources.d.ts", + "import": "./dist/observation-sources.js", + "default": "./dist/observation-sources.js" + }, + "./observations": { + "types": "./dist/observations.d.ts", + "import": "./dist/observations.js", + "default": "./dist/observations.js" + }, + "./operations": { + "types": "./dist/operations.d.ts", + "import": "./dist/operations.js", + "default": "./dist/operations.js" + }, + "./recommendation-export": { + "types": "./dist/recommendation-export.d.ts", + "import": "./dist/recommendation-export.js", + "default": "./dist/recommendation-export.js" + }, + "./owner-view": { + "types": "./dist/owner-view.d.ts", + "import": "./dist/owner-view.js", + "default": "./dist/owner-view.js" + }, + "./project-map": { + "types": "./dist/project-map.d.ts", + "import": "./dist/project-map.js", + "default": "./dist/project-map.js" + }, + "./project-index": { + "types": "./dist/project-index.d.ts", + "import": "./dist/project-index.js", + "default": "./dist/project-index.js" + }, + "./priority": { + "types": "./dist/priority.d.ts", + "import": "./dist/priority.js", + "default": "./dist/priority.js" + }, + "./assessment": { + "types": "./dist/assessment.d.ts", + "import": "./dist/assessment.js", + "default": "./dist/assessment.js" + }, + "./views": { + "types": "./dist/views.d.ts", + "import": "./dist/views.js", + "default": "./dist/views.js" + }, + "./views/server": { + "types": "./dist/views-server.d.ts", + "import": "./dist/views-server.js", + "default": "./dist/views-server.js" + }, + "./workspace": { + "types": "./dist/workspace.d.ts", + "import": "./dist/workspace.js", + "default": "./dist/workspace.js" + }, "./observation-sets.schema.json": "./dist/observation-sets.schema.json", "./observation-sources.schema.json": "./dist/observation-sources.schema.json", "./quality-observations.schema.json": "./dist/quality-observations.schema.json", + "./views.schema.json": "./dist/views.schema.json", "./package.json": "./package.json" }, "files": [ diff --git a/packages/core/src/observation-sets/execute.ts b/packages/core/src/observation-sets/execute.ts index 536631f..bbc4e45 100644 --- a/packages/core/src/observation-sets/execute.ts +++ b/packages/core/src/observation-sets/execute.ts @@ -1,6 +1,7 @@ import { createDiagnostic } from "../diagnostics/diagnostic"; import { executeObservationSourceProfile, + type HostObservationTransportRegistry, type ObservationSourceExecutionSelection, type ObservationSourceProfile } from "../observation-sources"; @@ -21,6 +22,7 @@ interface ExecuteObservationSetInput { readonly env?: NodeJS.ProcessEnv; readonly selection?: ObservationSetExecutionSelection; readonly fetchImpl?: typeof fetch; + readonly hostTransports?: HostObservationTransportRegistry; } function statusFor( @@ -148,6 +150,7 @@ export async function executeObservationSet( projectRoot: input.projectRoot, env: input.env, fetchImpl: input.fetchImpl, + hostTransports: input.hostTransports, selection: profileSelection({ globalSelection: input.selection, profileOverride: selectionMap.overrides.get(profile.id) @@ -177,7 +180,12 @@ export async function executeObservationSet( return { setId: input.observationSet.id, setName: input.observationSet.name, - status: statusFor(merged.observations.length, allDiagnostics.length), + // Same rule as a single source execution: info-severity notes are context, + // not a partial read. + status: statusFor( + merged.observations.length, + allDiagnostics.filter((entry) => entry.severity !== "info").length + ), profiles: profileResults, observations: merged.observations, diagnostics: allDiagnostics, diff --git a/packages/core/src/observation-sets/index.ts b/packages/core/src/observation-sets/index.ts index 93e592d..08b044d 100644 --- a/packages/core/src/observation-sets/index.ts +++ b/packages/core/src/observation-sets/index.ts @@ -1,4 +1,5 @@ export * from "./types"; export * from "./parse"; +export * from "./json-schema"; export * from "./serialize"; export * from "./execute"; diff --git a/packages/core/src/observation-sets/json-schema.ts b/packages/core/src/observation-sets/json-schema.ts new file mode 100644 index 0000000..9d8c25c --- /dev/null +++ b/packages/core/src/observation-sets/json-schema.ts @@ -0,0 +1,67 @@ +// Emitted from this package's own contract constants, like the observation +// manifest and source-profile schemas, so the published schema and the parser +// that enforces it cannot disagree. The checked-in JSON beside this module is +// generated from here and asserted equal in tests. +import { RESERVED_OBSERVATION_SET_ID } from "./types"; + +const nonEmptyString = { $ref: "#/definitions/nonEmptyString" } as const; + +// Matched case-insensitively because YAML authors write ids in whatever case +// they like, and the parser lowercases before comparing. +function caseInsensitivePattern(value: string): string { + return `^${[...value].map((c) => `[${c.toUpperCase()}${c.toLowerCase()}]`).join("")}$`; +} + +export function buildObservationSetsJsonSchema(): Record { + return { + $schema: "http://json-schema.org/draft-07/schema#", + $id: "https://shiplight.dev/schemas/quality/observation-sets.schema.json", + title: "Quality Observation Sets", + type: "object", + additionalProperties: false, + required: ["observation_sets"], + properties: { + observation_sets: { + type: "array", + minItems: 1, + items: { $ref: "#/definitions/observationSet" } + } + }, + definitions: { + nonEmptyString: { type: "string", minLength: 1 }, + profileReference: { + type: "object", + additionalProperties: false, + required: ["profile_id"], + properties: { profile_id: nonEmptyString } + }, + observationSet: { + type: "object", + additionalProperties: false, + required: ["id", "name", "profiles"], + properties: { + id: { + allOf: [ + nonEmptyString, + // `static` names the assessment that has no runtime observations + // at all, so a set claiming it would shadow that scope. + { not: { pattern: caseInsensitivePattern(RESERVED_OBSERVATION_SET_ID) } } + ] + }, + name: nonEmptyString, + description: nonEmptyString, + profiles: { + type: "array", + minItems: 1, + uniqueItems: true, + items: { $ref: "#/definitions/profileReference" } + } + } + } + } + }; +} + +export function serializeObservationSetsJsonSchema(): string { + return `${JSON.stringify(buildObservationSetsJsonSchema(), null, 2)}\n`; +} diff --git a/packages/core/src/observation-sets/observation-sets.schema.json b/packages/core/src/observation-sets/observation-sets.schema.json index 9f1b53d..5cacefa 100644 --- a/packages/core/src/observation-sets/observation-sets.schema.json +++ b/packages/core/src/observation-sets/observation-sets.schema.json @@ -4,7 +4,9 @@ "title": "Quality Observation Sets", "type": "object", "additionalProperties": false, - "required": ["observation_sets"], + "required": [ + "observation_sets" + ], "properties": { "observation_sets": { "type": "array", @@ -22,29 +24,49 @@ "profileReference": { "type": "object", "additionalProperties": false, - "required": ["profile_id"], + "required": [ + "profile_id" + ], "properties": { - "profile_id": { "$ref": "#/definitions/nonEmptyString" } + "profile_id": { + "$ref": "#/definitions/nonEmptyString" + } } }, "observationSet": { "type": "object", "additionalProperties": false, - "required": ["id", "name", "profiles"], + "required": [ + "id", + "name", + "profiles" + ], "properties": { "id": { "allOf": [ - { "$ref": "#/definitions/nonEmptyString" }, - { "not": { "pattern": "^[Ss][Tt][Aa][Tt][Ii][Cc]$" } } + { + "$ref": "#/definitions/nonEmptyString" + }, + { + "not": { + "pattern": "^[Ss][Tt][Aa][Tt][Ii][Cc]$" + } + } ] }, - "name": { "$ref": "#/definitions/nonEmptyString" }, - "description": { "$ref": "#/definitions/nonEmptyString" }, + "name": { + "$ref": "#/definitions/nonEmptyString" + }, + "description": { + "$ref": "#/definitions/nonEmptyString" + }, "profiles": { "type": "array", "minItems": 1, "uniqueItems": true, - "items": { "$ref": "#/definitions/profileReference" } + "items": { + "$ref": "#/definitions/profileReference" + } } } } diff --git a/packages/core/src/observation-sets/parse.ts b/packages/core/src/observation-sets/parse.ts index e7bc5ff..3833595 100644 --- a/packages/core/src/observation-sets/parse.ts +++ b/packages/core/src/observation-sets/parse.ts @@ -1,5 +1,6 @@ import { readFileSync } from "node:fs"; import { parseDocument } from "yaml"; +import { RESERVED_OBSERVATION_SET_ID } from "./types"; import type { ObservationSet, ObservationSetDiagnostic, @@ -128,12 +129,12 @@ function observationSetFrom( return undefined; } - if (id.toLowerCase() === "static") { + if (id.toLowerCase() === RESERVED_OBSERVATION_SET_ID) { diagnostics.push( diagnostic(source, { severity: "error", code: "RESERVED_OBSERVATION_SET_ID", - message: "Observation set id static is reserved for assessments without runtime observations.", + message: `Observation set id ${RESERVED_OBSERVATION_SET_ID} is reserved for assessments without runtime observations.`, yamlPath: `$.observation_sets[${index}].id` }) ); diff --git a/packages/core/src/observation-sets/types.ts b/packages/core/src/observation-sets/types.ts index b3ef489..4aa7016 100644 --- a/packages/core/src/observation-sets/types.ts +++ b/packages/core/src/observation-sets/types.ts @@ -1,3 +1,8 @@ +// The id `static` names the assessment that ran with no runtime observations, +// so no saved set may claim it. Shared with the JSON Schema so the parser and +// the published contract reserve exactly the same name. +export const RESERVED_OBSERVATION_SET_ID = "static"; + export type ObservationSetDiagnosticSeverity = "error" | "warning" | "info"; export type ObservationSetParseStatus = "parsed" | "invalid"; diff --git a/packages/core/src/observation-sources/execute.ts b/packages/core/src/observation-sources/execute.ts index 70b196b..1fb7084 100644 --- a/packages/core/src/observation-sources/execute.ts +++ b/packages/core/src/observation-sources/execute.ts @@ -4,8 +4,10 @@ import AdmZip from "adm-zip"; import { createDiagnostic } from "../diagnostics/diagnostic"; import type { ScanDiagnostic } from "../diagnostics/diagnostic"; import { + INTERNAL_OBSERVATION_CONTEXT, ingestObservationManifest, mergeObservationIngestionResults, + normalizeObservationBatches, qualityObservationIdentity, type ObservationIngestionResult } from "../observations"; @@ -13,6 +15,7 @@ import { evaluateObservationSourceProfileEnv } from "./env"; import type { ExecutedObservationSourceArtifact, ExecutedObservationSourceRun, + HostObservationTransportRegistry, ObservationSourceExecutionResult, ObservationSourceExecutionSelection, ObservationSourceProfile, @@ -25,6 +28,7 @@ interface ExecuteObservationSourceProfileInput { readonly env?: NodeJS.ProcessEnv; readonly selection?: ObservationSourceExecutionSelection; readonly fetchImpl?: typeof fetch; + readonly hostTransports?: HostObservationTransportRegistry; } interface DownloadedArtifactEntry { @@ -67,6 +71,14 @@ interface GitHubArtifactsResponse { readonly artifacts?: readonly GitHubArtifactResponse[]; } +// Counts only diagnostics that describe something going wrong. An `info` +// note — "this report records no commit", say — is context for the reader, not +// a degraded read, and reporting it as `partial` would say a run half-failed +// when every record arrived intact. +function countProblems(diagnostics: readonly ScanDiagnostic[]): number { + return diagnostics.filter((entry) => entry.severity !== "info").length; +} + function statusFor(observationCount: number, diagnosticsCount: number): ObservationIngestionResult["status"] { if (observationCount === 0 && diagnosticsCount > 0) { return "invalid"; @@ -128,7 +140,7 @@ function finalizeExecution(input: { profileId: input.profile.id, profileName: input.profile.name, transport: input.profile.transport, - status: statusFor(merged.observations.length, diagnostics.length), + status: statusFor(merged.observations.length, countProblems(diagnostics)), envStatus: input.envStatus, observations: merged.observations, diagnostics, @@ -137,6 +149,85 @@ function finalizeExecution(input: { }; } +async function executeHostProfile( + input: ExecuteObservationSourceProfileInput, + envStatus: ReturnType +): Promise { + const provider = input.profile.host?.provider; + if (provider === undefined) { + return finalizeExecution({ + profile: input.profile, + envStatus, + diagnostics: [ + createDiagnostic({ + severity: "error", + code: "INVALID_OBSERVATION_SOURCE", + message: `Observation source profile ${input.profile.id} uses transport host but declares no host.provider.` + }) + ] + }); + } + + const transport = input.hostTransports?.[provider]; + if (transport === undefined) { + // A repo can legitimately declare a provider this reader cannot serve — the + // OSS CLI reading a config written for the hosted app, say. Say which + // providers ARE registered so the reader can tell "wrong host" from "typo". + const registered = Object.keys(input.hostTransports ?? {}); + return finalizeExecution({ + profile: input.profile, + envStatus, + diagnostics: [ + createDiagnostic({ + severity: "error", + code: "INVALID_OBSERVATION_SOURCE", + message: `Observation source profile ${input.profile.id} needs host provider ${provider}, which this application does not register. Registered providers: ${joinedList(registered)}.` + }) + ] + }); + } + + let result: Awaited>; + try { + result = await transport({ + profile: input.profile, + selection: input.selection, + projectRoot: input.projectRoot, + env: input.env + }); + } catch (error) { + return finalizeExecution({ + profile: input.profile, + envStatus, + diagnostics: [ + createDiagnostic({ + severity: "error", + code: "INVALID_OBSERVATION_SOURCE", + message: `Observation source profile ${input.profile.id} host provider ${provider} failed: ${error instanceof Error ? error.message : String(error)}` + }) + ] + }); + } + + // The host handed us raw records; from here the engine owns everything. It + // normalizes and diagnoses them exactly as it would a parsed manifest, so a + // host transport cannot get a record past a check a file-based one must pass. + const ingestion = normalizeObservationBatches( + result.batches.map((batch) => ({ + ...batch, + context: batch.context ?? INTERNAL_OBSERVATION_CONTEXT + })) + ); + + return finalizeExecution({ + profile: input.profile, + envStatus, + ingestionResults: [ingestion], + diagnostics: result.diagnostics ?? [], + selectedRun: result.selectedRun + }); +} + function missingEnvDiagnostics(profile: ObservationSourceProfile, env: NodeJS.ProcessEnv): readonly ScanDiagnostic[] { const missing = profile.requiredEnv.filter((name) => { const value = env[name]; @@ -205,7 +296,8 @@ function joinedList(values: readonly string[]): string { async function executeLocalFolderProfile( input: ExecuteObservationSourceProfileInput, - envStatus: ReturnType + envStatus: ReturnType, + observationPath: string ): Promise { const diagnostics: ScanDiagnostic[] = []; const ingestionResults: ObservationIngestionResult[] = []; @@ -226,7 +318,7 @@ async function executeLocalFolderProfile( }); } - const resolvedPath = path.resolve(folderRoot, input.profile.observationPath); + const resolvedPath = path.resolve(folderRoot, observationPath); let rawText: string; try { rawText = await readFile(resolvedPath, "utf8"); @@ -235,7 +327,7 @@ async function executeLocalFolderProfile( createDiagnostic({ severity: "warning", code: "MISSING_OBSERVATION_ARTIFACT_MATCH", - message: `Observation source profile ${input.profile.id} could not read canonical observation file ${input.profile.observationPath} under ${folderRoot}: ${error instanceof Error ? error.message : String(error)}` + message: `Observation source profile ${input.profile.id} could not read canonical observation file ${observationPath} under ${folderRoot}: ${error instanceof Error ? error.message : String(error)}` }) ); @@ -249,7 +341,7 @@ async function executeLocalFolderProfile( } artifacts.push({ - declaredObservationPath: input.profile.observationPath, + declaredObservationPath: observationPath, sourcePath: resolvedPath }); ingestionResults.push( @@ -531,7 +623,8 @@ function duplicateObservationIdentityDetails( async function executeGitHubActionsProfile( input: ExecuteObservationSourceProfileInput, - envStatus: ReturnType + envStatus: ReturnType, + observationPath: string ): Promise { const diagnostics: ScanDiagnostic[] = []; const artifacts: ExecutedObservationSourceArtifact[] = []; @@ -562,7 +655,7 @@ async function executeGitHubActionsProfile( } const downloads = await downloadGitHubArtifacts(input.profile, runId, fetchImpl, token); - const matches = matchingDownloadedEntries(downloads, input.profile.observationPath); + const matches = matchingDownloadedEntries(downloads, observationPath); const ambiguityDetails = ambiguousDownloadedMatchDetails( matches, input.profile.github?.artifactNames ?? [] @@ -573,7 +666,7 @@ async function executeGitHubActionsProfile( severity: "warning", code: "MISSING_OBSERVATION_ARTIFACT_MATCH", message: [ - `Observation source profile ${input.profile.id} selected GitHub Actions run ${runId} for workflow ${input.profile.github?.workflow ?? "(unknown workflow)"}, but could not find canonical observation path ${input.profile.observationPath}.`, + `Observation source profile ${input.profile.id} selected GitHub Actions run ${runId} for workflow ${input.profile.github?.workflow ?? "(unknown workflow)"}, but could not find canonical observation path ${observationPath}.`, `Configured artifact_names: ${joinedList(input.profile.github?.artifactNames ?? [])}.`, `Downloaded matching artifacts: ${joinedList(downloads.map((download) => download.name))}.`, `Downloaded artifact entries: ${joinedList(downloads.flatMap((download) => download.entries.map((entry) => `${download.name}/${entry.path}`)))}.` @@ -585,7 +678,7 @@ async function executeGitHubActionsProfile( createDiagnostic({ severity: "warning", code: "AMBIGUOUS_OBSERVATION_ARTIFACT_MATCH", - message: `Observation source profile ${input.profile.id} matched canonical observation path ${input.profile.observationPath} ambiguously: ${ambiguityDetails.join("; ")}.` + message: `Observation source profile ${input.profile.id} matched canonical observation path ${observationPath} ambiguously: ${ambiguityDetails.join("; ")}.` }) ); } else { @@ -610,7 +703,7 @@ async function executeGitHubActionsProfile( for (const match of matches) { artifacts.push({ - declaredObservationPath: input.profile.observationPath, + declaredObservationPath: observationPath, matchedArtifactName: match.artifact.name, matchedObservationPath: match.entry.path }); @@ -702,9 +795,27 @@ export async function executeObservationSourceProfile( }); } + if (input.profile.transport === "host") { + return executeHostProfile(input, envStatus); + } + + if (input.profile.observationPath === undefined) { + return finalizeExecution({ + profile: input.profile, + envStatus, + diagnostics: [ + createDiagnostic({ + severity: "error", + code: "INVALID_OBSERVATION_SOURCE", + message: `Observation source profile ${input.profile.id} has no observation_path, which transport ${input.profile.transport} requires.` + }) + ] + }); + } + if (input.profile.transport === "local-folder") { - return executeLocalFolderProfile(input, envStatus); + return executeLocalFolderProfile(input, envStatus, input.profile.observationPath); } - return executeGitHubActionsProfile(input, envStatus); + return executeGitHubActionsProfile(input, envStatus, input.profile.observationPath); } diff --git a/packages/core/src/observation-sources/index.ts b/packages/core/src/observation-sources/index.ts index 589207e..ebee1fe 100644 --- a/packages/core/src/observation-sources/index.ts +++ b/packages/core/src/observation-sources/index.ts @@ -1,5 +1,7 @@ export * from "./types"; export * from "./parse"; +export * from "./json-schema"; export * from "./env"; export * from "./execute"; +export * from "./local-reports"; export * from "./serialize"; diff --git a/packages/core/src/observation-sources/json-schema.ts b/packages/core/src/observation-sources/json-schema.ts new file mode 100644 index 0000000..2d4abd8 --- /dev/null +++ b/packages/core/src/observation-sources/json-schema.ts @@ -0,0 +1,105 @@ +// Emitted from this package's own contract constants, exactly like the +// canonical observation manifest schema, so the published schema and the parser +// that enforces it cannot disagree. The checked-in JSON file beside this module +// is generated from here and asserted equal in tests. +import { OBSERVATION_SOURCE_TRANSPORTS } from "./types"; + +const nonEmptyString = { $ref: "#/definitions/nonEmptyString" } as const; + +function transportRequires( + transport: string, + required: readonly string[] +): Record { + return { + if: { properties: { transport: { const: transport } } }, + then: { required: [...required] } + }; +} + +export function buildObservationSourceProfilesJsonSchema(): Record { + return { + $schema: "http://json-schema.org/draft-07/schema#", + $id: "https://shiplight.dev/schemas/quality/observation-sources.schema.json", + title: "Quality Observation Source Profiles", + type: "object", + additionalProperties: false, + required: ["profiles"], + properties: { + profiles: { + type: "array", + minItems: 1, + items: { $ref: "#/definitions/profile" } + } + }, + definitions: { + nonEmptyString: { type: "string", minLength: 1 }, + sourceRef: { + type: "object", + additionalProperties: false, + minProperties: 1, + properties: { path: nonEmptyString, url: nonEmptyString, label: nonEmptyString } + }, + auth: { + type: "object", + additionalProperties: false, + properties: { required_env: { type: "array", items: nonEmptyString } } + }, + github: { + type: "object", + additionalProperties: false, + required: ["repo", "workflow", "artifact_names"], + properties: { + repo: nonEmptyString, + workflow: nonEmptyString, + artifact_names: { type: "array", minItems: 1, items: nonEmptyString }, + branch: nonEmptyString + } + }, + localFolder: { + type: "object", + additionalProperties: false, + required: ["path"], + properties: { path: nonEmptyString } + }, + host: { + type: "object", + additionalProperties: false, + required: ["provider"], + properties: { + provider: nonEmptyString, + // Values are strings so config stays a flat, reviewable block rather + // than a place to smuggle structure past a reader. + options: { type: "object", additionalProperties: nonEmptyString } + } + }, + profile: { + type: "object", + additionalProperties: false, + // `observation_path` is required per-transport below, not here: a host + // transport addresses no file. + required: ["id", "name", "transport"], + properties: { + id: nonEmptyString, + name: nonEmptyString, + description: nonEmptyString, + transport: { enum: [...OBSERVATION_SOURCE_TRANSPORTS] }, + observation_path: nonEmptyString, + source_refs: { type: "array", items: { $ref: "#/definitions/sourceRef" } }, + auth: { $ref: "#/definitions/auth" }, + github: { $ref: "#/definitions/github" }, + local_folder: { $ref: "#/definitions/localFolder" }, + host: { $ref: "#/definitions/host" } + }, + allOf: [ + transportRequires("github-actions", ["github", "observation_path"]), + transportRequires("local-folder", ["local_folder", "observation_path"]), + transportRequires("host", ["host"]) + ] + } + } + }; +} + +export function serializeObservationSourceProfilesJsonSchema(): string { + return `${JSON.stringify(buildObservationSourceProfilesJsonSchema(), null, 2)}\n`; +} diff --git a/packages/core/src/observation-sources/local-reports.ts b/packages/core/src/observation-sources/local-reports.ts new file mode 100644 index 0000000..7c80241 --- /dev/null +++ b/packages/core/src/observation-sources/local-reports.ts @@ -0,0 +1,228 @@ +import { readFile } from "node:fs/promises"; +import path from "node:path"; +import { createDiagnostic } from "../diagnostics/diagnostic"; +import type { ScanDiagnostic } from "../diagnostics/diagnostic"; +import { buildPlaywrightObservationBatch, buildShiplightObservationBatch } from "../observations"; +import type { ObservationEvidenceRefInput } from "../observations"; +import type { HostObservationTransport, ObservationSourceProfile } from "./types"; + +/** + * The bundled reference host transport, and the one that makes run evidence + * work with no platform at all: it reads a native test report from the working + * tree and hands the engine its results, pointing each one at the HTML report + * the runner already wrote. + * + * It deliberately does NOT enumerate videos, traces, or screenshots. Quality is + * an index from checks to evidence, not a viewer — the runner's report is + * already the viewer, the reviewer already knows it, and rebuilding a worse one + * inside the quality UI would earn nothing. So the evidence pointer is the + * report, and the report handles presentation. + * + * It is also the worked example every other host transport is written against, + * including the platform ones that live outside this repo. + * + * Registration is still the host's decision. This module only offers the + * factory; nothing here registers itself. + * + * Config: + * + * - id: local-playwright + * name: Local Playwright run + * transport: host + * host: + * provider: local-reports + * options: + * path: playwright-report/report.json # results to read + * report: playwright-report/index.html # what a reviewer opens + * format: playwright-json # optional; see below + * commit: # optional, see below + * + * `format` selects which report the run left behind: + * + * playwright-json Playwright's JSON reporter output. + * shiplight-report A Shiplight YAML run's `report-data.json`, which keys its + * results on the TRANSPILED spec; that adapter maps them + * back to the `.test.yaml` source a quality map can pin. + */ + +export const LOCAL_REPORTS_PROVIDER = "local-reports"; + +const SUPPORTED_FORMATS = new Set(["playwright-json", "shiplight-report"]); + +const REPORT_LABEL = "Test report"; + +function invalid(message: string): ScanDiagnostic { + return createDiagnostic({ severity: "error", code: "INVALID_OBSERVATION_SOURCE", message }); +} + +function missingReport(message: string): ScanDiagnostic { + return createDiagnostic({ + severity: "warning", + code: "MISSING_OBSERVATION_ARTIFACT_MATCH", + message + }); +} + +function resolveOption( + profile: ObservationSourceProfile, + option: "path" | "report", + projectRoot: string | undefined +): { readonly resolved?: string; readonly diagnostic?: ScanDiagnostic } { + const declared = profile.host?.options[option]; + if (declared === undefined || declared.length === 0) { + return { + diagnostic: invalid( + `Observation source profile ${profile.id} needs host.options.${option} naming a file under the project.` + ) + }; + } + + if (projectRoot === undefined) { + return { + diagnostic: invalid( + `Observation source profile ${profile.id} needs a project root to resolve the path ${declared}.` + ) + }; + } + + // Contained to the project root on purpose, and containment is checked for an + // ABSOLUTE declaration too. A profile is repo config: letting `/etc/hosts` + // through while refusing `../../etc/hosts` would enforce nothing, since a PR + // can write either. `path.resolve` returns an absolute declaration unchanged, + // so both forms reach the same comparison. + const resolved = path.resolve(projectRoot, declared); + const relative = path.relative(projectRoot, resolved); + if (relative.startsWith("..") || path.isAbsolute(relative)) { + return { + diagnostic: invalid( + `Observation source profile ${profile.id} resolves host.options.${option} ${declared} outside the project root.` + ) + }; + } + + return { resolved }; +} + +// The one evidence pointer this transport produces: the report a reviewer +// opens. It is attached to every observation the run produced, because that is +// what it is — one report describing the whole run. Quality links to it and +// stops there; the report is the viewer. +function reportRef( + profile: ObservationSourceProfile, + projectRoot: string | undefined +): { readonly refs: readonly ObservationEvidenceRefInput[]; readonly diagnostics: readonly ScanDiagnostic[] } { + const declared = profile.host?.options.report; + if (declared === undefined || declared.length === 0) { + return { refs: [], diagnostics: [] }; + } + + if (/^https?:\/\//i.test(declared)) { + return { refs: [{ ref: declared, label: REPORT_LABEL }], diagnostics: [] }; + } + + const location = resolveOption(profile, "report", projectRoot); + if (location.resolved === undefined) { + return { refs: [], diagnostics: location.diagnostic === undefined ? [] : [location.diagnostic] }; + } + + // Recorded project-root-relative rather than absolute, so the ref means the + // same thing to every reader of this repo instead of encoding one machine's + // checkout location. `resolveOption` has already refused anything that lands + // outside the root, so this is always a contained path. + const ref = path.relative(projectRoot ?? "", location.resolved); + return { refs: [{ ref: ref.replaceAll("\\", "/"), label: REPORT_LABEL }], diagnostics: [] }; +} + +export function createLocalReportsTransport(): HostObservationTransport { + return async ({ profile, projectRoot }) => { + const format = profile.host?.options.format ?? "playwright-json"; + if (!SUPPORTED_FORMATS.has(format)) { + return { + batches: [], + diagnostics: [ + invalid( + `Observation source profile ${profile.id} asks for report format ${format}; ${LOCAL_REPORTS_PROVIDER} supports ${[...SUPPORTED_FORMATS].join(", ")}.` + ) + ] + }; + } + + const location = resolveOption(profile, "path", projectRoot); + if (location.resolved === undefined) { + return { batches: [], diagnostics: location.diagnostic === undefined ? [] : [location.diagnostic] }; + } + + let reportJson: string; + try { + reportJson = await readFile(location.resolved, "utf8"); + } catch (error) { + // `warning`, not `error`, because "you have not run the suite yet" is an + // ordinary state for a local source and the message should read as + // guidance rather than a fault. + // + // The EXECUTION STATUS is still `invalid`, and deliberately so: a source + // that produced no observations read nothing, and `statusFor` is right to + // say so. Downgrading this to `info` to soften the status would report the + // same empty read as `valid`, which is a worse lie than a red badge. + return { + batches: [], + diagnostics: [ + missingReport( + `Observation source profile ${profile.id} could not read the report at ${location.resolved}: ${error instanceof Error ? error.message : String(error)}` + ) + ] + }; + } + + // A local report records no commit. Rather than stamp the working tree's + // HEAD onto results that may predate it — inventing provenance the report + // never claimed — the commit stays absent unless config pins one. The + // diagnostic says so, because an unpinned observation is skipped by any + // commit-scoped evaluation and that silence would otherwise look like a bug. + const commit = profile.host?.options.commit; + const diagnostics: ScanDiagnostic[] = + commit === undefined + ? [ + createDiagnostic({ + severity: "info", + code: "INVALID_OBSERVATION_SELECTION", + message: `Observation source profile ${profile.id} read a local report that records no commit, so a commit-scoped evaluation will not count it. Set host.options.commit to pin one.` + }) + ] + : []; + + const source = { + id: profile.id, + kind: LOCAL_REPORTS_PROVIDER, + label: profile.name + }; + const revision = commit === undefined ? {} : { revision: { commit } }; + const report = reportRef(profile, projectRoot); + + const built = + format === "shiplight-report" + ? buildShiplightObservationBatch({ + report_json: reportJson, + source, + ...revision, + evidence_refs: report.refs + }) + : buildPlaywrightObservationBatch({ report_json: reportJson, source, ...revision }); + + const batch = + built.batch === undefined || report.refs.length === 0 + ? built.batch + : { + ...built.batch, + observations: (built.batch.observations ?? []).map((observation) => ({ + ...observation, + evidence_refs: report.refs + })) + }; + + return { + batches: batch === undefined ? [] : [batch], + diagnostics: [...diagnostics, ...report.diagnostics, ...built.diagnostics] + }; + }; +} diff --git a/packages/core/src/observation-sources/observation-sources.schema.json b/packages/core/src/observation-sources/observation-sources.schema.json index c87bdb1..6457e37 100644 --- a/packages/core/src/observation-sources/observation-sources.schema.json +++ b/packages/core/src/observation-sources/observation-sources.schema.json @@ -4,7 +4,9 @@ "title": "Quality Observation Source Profiles", "type": "object", "additionalProperties": false, - "required": ["profiles"], + "required": [ + "profiles" + ], "properties": { "profiles": { "type": "array", @@ -24,9 +26,15 @@ "additionalProperties": false, "minProperties": 1, "properties": { - "path": { "$ref": "#/definitions/nonEmptyString" }, - "url": { "$ref": "#/definitions/nonEmptyString" }, - "label": { "$ref": "#/definitions/nonEmptyString" } + "path": { + "$ref": "#/definitions/nonEmptyString" + }, + "url": { + "$ref": "#/definitions/nonEmptyString" + }, + "label": { + "$ref": "#/definitions/nonEmptyString" + } } }, "auth": { @@ -35,72 +43,159 @@ "properties": { "required_env": { "type": "array", - "items": { "$ref": "#/definitions/nonEmptyString" } + "items": { + "$ref": "#/definitions/nonEmptyString" + } } } }, "github": { "type": "object", "additionalProperties": false, - "required": ["repo", "workflow", "artifact_names"], + "required": [ + "repo", + "workflow", + "artifact_names" + ], "properties": { - "repo": { "$ref": "#/definitions/nonEmptyString" }, - "workflow": { "$ref": "#/definitions/nonEmptyString" }, + "repo": { + "$ref": "#/definitions/nonEmptyString" + }, + "workflow": { + "$ref": "#/definitions/nonEmptyString" + }, "artifact_names": { "type": "array", "minItems": 1, - "items": { "$ref": "#/definitions/nonEmptyString" } + "items": { + "$ref": "#/definitions/nonEmptyString" + } }, - "branch": { "$ref": "#/definitions/nonEmptyString" } + "branch": { + "$ref": "#/definitions/nonEmptyString" + } } }, "localFolder": { "type": "object", "additionalProperties": false, - "required": ["path"], + "required": [ + "path" + ], "properties": { - "path": { "$ref": "#/definitions/nonEmptyString" } + "path": { + "$ref": "#/definitions/nonEmptyString" + } + } + }, + "host": { + "type": "object", + "additionalProperties": false, + "required": [ + "provider" + ], + "properties": { + "provider": { + "$ref": "#/definitions/nonEmptyString" + }, + "options": { + "type": "object", + "additionalProperties": { + "$ref": "#/definitions/nonEmptyString" + } + } } }, "profile": { "type": "object", "additionalProperties": false, - "required": ["id", "name", "transport", "observation_path"], + "required": [ + "id", + "name", + "transport" + ], "properties": { - "id": { "$ref": "#/definitions/nonEmptyString" }, - "name": { "$ref": "#/definitions/nonEmptyString" }, - "description": { "$ref": "#/definitions/nonEmptyString" }, + "id": { + "$ref": "#/definitions/nonEmptyString" + }, + "name": { + "$ref": "#/definitions/nonEmptyString" + }, + "description": { + "$ref": "#/definitions/nonEmptyString" + }, "transport": { - "enum": ["github-actions", "local-folder"] + "enum": [ + "github-actions", + "local-folder", + "host" + ] + }, + "observation_path": { + "$ref": "#/definitions/nonEmptyString" }, - "observation_path": { "$ref": "#/definitions/nonEmptyString" }, "source_refs": { "type": "array", - "items": { "$ref": "#/definitions/sourceRef" } + "items": { + "$ref": "#/definitions/sourceRef" + } }, - "auth": { "$ref": "#/definitions/auth" }, - "github": { "$ref": "#/definitions/github" }, - "local_folder": { "$ref": "#/definitions/localFolder" } + "auth": { + "$ref": "#/definitions/auth" + }, + "github": { + "$ref": "#/definitions/github" + }, + "local_folder": { + "$ref": "#/definitions/localFolder" + }, + "host": { + "$ref": "#/definitions/host" + } }, "allOf": [ { "if": { "properties": { - "transport": { "const": "github-actions" } + "transport": { + "const": "github-actions" + } + } + }, + "then": { + "required": [ + "github", + "observation_path" + ] + } + }, + { + "if": { + "properties": { + "transport": { + "const": "local-folder" + } } }, "then": { - "required": ["github"] + "required": [ + "local_folder", + "observation_path" + ] } }, { "if": { "properties": { - "transport": { "const": "local-folder" } + "transport": { + "const": "host" + } } }, "then": { - "required": ["local_folder"] + "required": [ + "host" + ] } } ] diff --git a/packages/core/src/observation-sources/parse.ts b/packages/core/src/observation-sources/parse.ts index 6d24209..6e02bed 100644 --- a/packages/core/src/observation-sources/parse.ts +++ b/packages/core/src/observation-sources/parse.ts @@ -1,9 +1,12 @@ import { readFileSync } from "node:fs"; import { parseDocument } from "yaml"; +import { OBSERVATION_SOURCE_TRANSPORTS } from "./types"; import type { GitHubActionsObservationSourceConfig, + HostObservationSourceConfig, LocalFolderObservationSourceConfig, ObservationSourceProfile, + ObservationSourceTransport, ObservationSourceProfileDiagnostic, ObservationSourceProfileParseBatch, ObservationSourceProfileSource, @@ -21,9 +24,12 @@ const PROFILE_KEYS = new Set([ "source_refs", "auth", "github", - "local_folder" + "local_folder", + "host" ]); +const TRANSPORTS = new Set(OBSERVATION_SOURCE_TRANSPORTS); + function isRecord(value: unknown): value is Record { return typeof value === "object" && value !== null && !Array.isArray(value); } @@ -107,6 +113,41 @@ function localFolderConfig(value: unknown): LocalFolderObservationSourceConfig | return folderPath === undefined ? undefined : { path: folderPath }; } +// `options` is passed to the host handler untouched. Values are restricted to +// strings so config stays a flat, reviewable block in the repo rather than a +// place to smuggle arbitrary structure past a reader. +// +// A non-string value is REPORTED, not quietly dropped. YAML turns an unquoted +// `commit: 1234567` into a number, and dropping it silently leaves a source +// that reads fine and produces observations a commit-scoped evaluation then +// ignores — a wrong answer with nothing on screen to explain it. +function hostConfig( + value: unknown, + onInvalidOption: (key: string) => void +): HostObservationSourceConfig | undefined { + if (!isRecord(value)) { + return undefined; + } + + const provider = stringValue(value.provider); + if (provider === undefined) { + return undefined; + } + + const rawOptions = isRecord(value.options) ? value.options : {}; + const options: Record = {}; + for (const [key, optionValue] of Object.entries(rawOptions)) { + const normalized = stringValue(optionValue); + if (normalized === undefined) { + onInvalidOption(key); + continue; + } + options[key] = normalized; + } + + return { provider, options }; +} + function validateProfile( profile: ObservationSourceProfile, source: ObservationSourceProfileSource, @@ -134,6 +175,30 @@ function validateProfile( }) ); } + + if (profile.transport === "host" && profile.host === undefined) { + diagnostics.push( + diagnostic(source, { + severity: "error", + code: "INVALID_OBSERVATION_SOURCE_PROFILE", + message: `Profile ${profile.id} requires a host block with a provider for transport host.`, + yamlPath: `${yamlPath}.host` + }) + ); + } + + // Only the file-based transports address a file. Requiring observation_path + // of a host transport would force config to name a path that is never read. + if (profile.transport !== "host" && profile.observationPath === undefined) { + diagnostics.push( + diagnostic(source, { + severity: "error", + code: "INVALID_OBSERVATION_SOURCE_PROFILE", + message: `Profile ${profile.id} must define observation_path for its canonical quality-observations JSON file.`, + yamlPath: `${yamlPath}.observation_path` + }) + ); + } } function profileFrom( @@ -171,7 +236,7 @@ function profileFrom( ); } - if (id === undefined || name === undefined || (transport !== "github-actions" && transport !== "local-folder")) { + if (id === undefined || name === undefined || !TRANSPORTS.has(transport as ObservationSourceTransport)) { diagnostics.push( diagnostic(source, { severity: "error", @@ -182,28 +247,27 @@ function profileFrom( ); return undefined; } - if (observationPath === undefined) { - diagnostics.push( - diagnostic(source, { - severity: "error", - code: "INVALID_OBSERVATION_SOURCE_PROFILE", - message: `Profile ${id} must define observation_path for its canonical quality-observations JSON file.`, - yamlPath: `$.profiles[${index}].observation_path` - }) - ); - return undefined; - } const profile: ObservationSourceProfile = { id, name, description: stringValue(value.description), - transport, + transport: transport as ObservationSourceTransport, observationPath, requiredEnv: stringArray(isRecord(value.auth) ? value.auth.required_env : undefined), sourceRefs: sourceRefs(value.source_refs), github: githubConfig(value.github), - localFolder: localFolderConfig(value.local_folder) + localFolder: localFolderConfig(value.local_folder), + host: hostConfig(value.host, (key) => { + diagnostics.push( + diagnostic(source, { + severity: "error", + code: "INVALID_OBSERVATION_SOURCE_PROFILE", + message: `Profile entry ${index} host.options.${key} must be a non-empty string. Quote a value YAML would otherwise read as a number or boolean.`, + yamlPath: `$.profiles[${index}].host.options.${key}` + }) + ); + }) }; validateProfile(profile, source, `$.profiles[${index}]`, diagnostics); diff --git a/packages/core/src/observation-sources/serialize.ts b/packages/core/src/observation-sources/serialize.ts index f292c2e..dbd082c 100644 --- a/packages/core/src/observation-sources/serialize.ts +++ b/packages/core/src/observation-sources/serialize.ts @@ -20,7 +20,7 @@ export function serializeObservationSources(profiles: readonly ObservationSource ? {} : { description: profile.description }), transport: profile.transport, - observation_path: profile.observationPath, + ...(profile.observationPath === undefined ? {} : { observation_path: profile.observationPath }), ...(requiredEnv.length > 0 ? { auth: { required_env: requiredEnv } } : {}), ...(sourceRefs.length > 0 ? { source_refs: sourceRefs } : {}), ...(profile.github === undefined @@ -33,7 +33,17 @@ export function serializeObservationSources(profiles: readonly ObservationSource ...(profile.github.branch === undefined ? {} : { branch: profile.github.branch }) } }), - ...(profile.localFolder === undefined ? {} : { local_folder: { path: profile.localFolder.path } }) + ...(profile.localFolder === undefined ? {} : { local_folder: { path: profile.localFolder.path } }), + ...(profile.host === undefined + ? {} + : { + host: { + provider: profile.host.provider, + ...(Object.keys(profile.host.options).length === 0 + ? {} + : { options: { ...profile.host.options } }) + } + }) }; }) }); diff --git a/packages/core/src/observation-sources/types.ts b/packages/core/src/observation-sources/types.ts index 8040545..3d7f9ee 100644 --- a/packages/core/src/observation-sources/types.ts +++ b/packages/core/src/observation-sources/types.ts @@ -33,18 +33,64 @@ export interface LocalFolderObservationSourceConfig { readonly path: string; } +export interface HostObservationSourceConfig { + readonly provider: string; + readonly options: Readonly>; +} + +// The single list every consumer reads: the parser validates against it and the +// published JSON Schema enumerates it, so a new transport cannot be accepted by +// one and rejected by the other. +export const OBSERVATION_SOURCE_TRANSPORTS = ["github-actions", "local-folder", "host"] as const; + +export type ObservationSourceTransport = (typeof OBSERVATION_SOURCE_TRANSPORTS)[number]; + export interface ObservationSourceProfile { readonly id: string; readonly name: string; readonly description?: string; - readonly transport: "github-actions" | "local-folder"; - readonly observationPath: string; + readonly transport: ObservationSourceTransport; + /** + * Path of the canonical quality-observations JSON inside the fetched artifact + * or folder. Absent for `host` transports, which have no file to address — + * validation requires it for the two file-based transports. + */ + readonly observationPath?: string; readonly requiredEnv: readonly string[]; readonly sourceRefs: readonly ObservationSourceReference[]; readonly github?: GitHubActionsObservationSourceConfig; readonly localFolder?: LocalFolderObservationSourceConfig; + readonly host?: HostObservationSourceConfig; } +/** + * A transport the embedding application supplies, addressed by name from + * `host.provider` in the profile. + * + * This is the seam that lets an integration read results from somewhere the + * engine has no business knowing about — a platform database, a vendor API — + * without that knowledge entering this package. The handler's only job is to + * FETCH and SHAPE: it returns records in the canonical input form and the + * engine keeps everything that follows (normalization, identity, resolution + * against the quality map, every diagnostic). A handler that resolved + * observations onto checks itself would be deciding what proves what outside + * the engine, which is exactly what the independence rule forbids. + */ +export type HostObservationTransport = (input: { + readonly profile: ObservationSourceProfile; + readonly selection?: ObservationSourceExecutionSelection; + readonly projectRoot?: string; + readonly env?: NodeJS.ProcessEnv; +}) => Promise; + +export interface HostObservationTransportResult { + readonly batches: readonly import("../observations/types").ObservationBatchInput[]; + readonly diagnostics?: readonly import("../diagnostics/diagnostic").ScanDiagnostic[]; + readonly selectedRun?: ExecutedObservationSourceRun; +} + +export type HostObservationTransportRegistry = Readonly>; + export interface ParsedObservationSourceProfilesDocument { readonly profiles: readonly ObservationSourceProfile[]; } @@ -99,7 +145,7 @@ export interface ExecutedObservationSourceRun { export interface ObservationSourceExecutionResult { readonly profileId: string; readonly profileName: string; - readonly transport: ObservationSourceProfile["transport"]; + readonly transport: ObservationSourceTransport; readonly status: import("../observations/types").ObservationIngestionStatus; readonly envStatus: ObservationSourceProfileEnvStatus; readonly observations: readonly import("../observations/types").NormalizedObservationRecord[]; diff --git a/packages/core/src/observations/evaluate.ts b/packages/core/src/observations/evaluate.ts index 1912a13..4b96ead 100644 --- a/packages/core/src/observations/evaluate.ts +++ b/packages/core/src/observations/evaluate.ts @@ -218,7 +218,10 @@ export function buildTargetEvaluation( observationId: selected?.observationId, observedAt: selected?.observedAt, commit: selected?.revision.commit, - runUrl: selected?.source.runUrl + runUrl: selected?.source.runUrl, + // Only the selected observation's refs. An unobserved check has none, + // and refs from a run that lost selection describe a different commit. + evidenceRefs: selected?.evidenceRefs ?? [] } satisfies EvaluatedEvidenceObservation; }); diff --git a/packages/core/src/observations/index.ts b/packages/core/src/observations/index.ts index b3bc19d..fff56bd 100644 --- a/packages/core/src/observations/index.ts +++ b/packages/core/src/observations/index.ts @@ -8,4 +8,5 @@ export * from "./recommend"; export * from "./github-actions"; export * from "./junit"; export * from "./playwright-json"; +export * from "./shiplight-report"; export * from "./manifest"; diff --git a/packages/core/src/observations/manifest.ts b/packages/core/src/observations/manifest.ts index 227a2b8..5421f6f 100644 --- a/packages/core/src/observations/manifest.ts +++ b/packages/core/src/observations/manifest.ts @@ -7,6 +7,7 @@ import type { ObservationRecordInput, ObservationRecordStatus, QualityObservationManifest, + QualityObservationManifestArtifact, QualityObservationManifestParseResult, QualityObservationManifestRecord, QualityObservationManifestRevision, @@ -70,7 +71,22 @@ export function buildQualityObservationManifestJsonSchema(): Record(["pass", "fail", "error", "skipped"]); function isRecord(value: unknown): value is Record { @@ -203,6 +220,64 @@ function worstStatus( type ObservationEntryMode = "strict" | "tolerant"; +// A malformed evidence pointer must never cost us the result it points at. The +// pass/fail fact is what scores; the ref is only how a reviewer looks at it. So +// a bad entry is dropped and reported, and the observation survives without it. +// `validate` still fails the document, because entryDiagnostic raises these to +// errors in strict mode — a producer fixing its output wants to hear about it. +function parseObservationArtifacts( + value: unknown, + index: number, + entryDiagnostic: (message: string) => ScanDiagnostic, + diagnostics: ScanDiagnostic[] +): readonly QualityObservationManifestArtifact[] | undefined { + if (value === undefined) { + return undefined; + } + + if (!Array.isArray(value)) { + diagnostics.push(entryDiagnostic(`Quality observations entry ${index} artifacts must be an array.`)); + return undefined; + } + + const artifacts: QualityObservationManifestArtifact[] = []; + value.forEach((entry, artifactIndex) => { + const position = `entry ${index} artifact ${artifactIndex}`; + if (!isRecord(entry)) { + diagnostics.push(entryDiagnostic(`Quality observations ${position} must be an object and was skipped.`)); + return; + } + + const extras = unknownKeys(entry, OBSERVATION_ARTIFACT_KEYS); + if (extras.length > 0) { + diagnostics.push( + entryDiagnostic( + `Quality observations ${position} contains unknown fields and was skipped: ${extras.join(", ")}.` + ) + ); + return; + } + + const ref = stringValue(entry.ref); + if (ref === undefined) { + diagnostics.push(entryDiagnostic(`Quality observations ${position} requires a non-empty ref and was skipped.`)); + return; + } + + const label = stringValue(entry.label); + if (entry.label !== undefined && label === undefined) { + diagnostics.push( + entryDiagnostic(`Quality observations ${position} label must be a non-empty string when provided.`) + ); + return; + } + + artifacts.push({ ref, ...(label === undefined ? {} : { label }) }); + }); + + return artifacts.length === 0 ? undefined : artifacts; +} + function parseObservations( value: unknown, fallbackObservedAt: string | undefined, @@ -271,12 +346,15 @@ function parseObservations( return; } + const artifacts = parseObservationArtifacts(entry.artifacts, index, entryDiagnostic, diagnostics); + const record: QualityObservationManifestRecord = { path, ...(testCase === undefined ? {} : { test_case: testCase }), status, ...(observedAt === undefined ? {} : { observed_at: observedAt }), - ...(note === undefined ? {} : { note }) + ...(note === undefined ? {} : { note }), + ...(artifacts === undefined ? {} : { artifacts }) }; const key = qualityObservationIdentity(record); const seenIndex = seenKeys.get(key); @@ -454,7 +532,14 @@ export function ingestObservationManifest(input: IngestObservationManifestInput) observed_at: entry.observed_at ?? manifest.observed_at, revision: input.revision ?? { ...manifest.revision }, note: entry.note, - artifacts: artifact === undefined ? [] : [artifact] + artifacts: artifact === undefined ? [] : [artifact], + // The manifest's `artifacts` are evidence pointers; the `artifacts` above + // are the manifest's own provenance. Mapped field by field rather than + // spread, so the two never merge by accident. + evidence_refs: (entry.artifacts ?? []).map((entryArtifact) => ({ + ref: entryArtifact.ref, + label: entryArtifact.label + })) })); const normalized = normalizeObservationBatches([ diff --git a/packages/core/src/observations/normalize.ts b/packages/core/src/observations/normalize.ts index 1694700..bdc1a27 100644 --- a/packages/core/src/observations/normalize.ts +++ b/packages/core/src/observations/normalize.ts @@ -1,12 +1,14 @@ import { createDiagnostic } from "../diagnostics/diagnostic"; import type { ScanDiagnostic } from "../diagnostics/diagnostic"; import type { + NormalizedEvidenceRef, NormalizedObservationArtifact, NormalizedObservationRecord, NormalizedObservationRevision, NormalizedObservationSource, ObservationArtifactInput, ObservationBatchInput, + ObservationEvidenceRefInput, ObservationIngestionResult, ObservationRecordInput, ObservationRecordStatus, @@ -76,6 +78,28 @@ function normalizeArtifacts(value: readonly ObservationArtifactInput[] | undefin })); } +// An entry with no usable ref points at nothing, so it is dropped rather than +// carried as an empty link the UI would have to special-case. The ref itself is +// only trimmed — never rewritten, resolved, or validated for shape, because +// only the integration that produced it knows what a valid one looks like. +function normalizeEvidenceRefs( + value: readonly ObservationEvidenceRefInput[] | undefined +): readonly NormalizedEvidenceRef[] { + if (!Array.isArray(value)) { + return []; + } + + return value.flatMap((entry) => { + const ref = stringValue(entry.ref); + if (ref === undefined) { + return []; + } + + const label = stringValue(entry.label); + return [{ ref, ...(label === undefined ? {} : { label }) }]; + }); +} + function normalizeStatus(value: unknown): ObservationRecordStatus | undefined { const normalized = stringValue(value)?.toLowerCase() as ObservationRecordStatus | undefined; return normalized !== undefined && validStatuses.has(normalized) ? normalized : undefined; @@ -212,7 +236,8 @@ export function normalizeObservationBatches( revision, source, note: stringValue(record.note), - artifacts: normalizeArtifacts(record.artifacts) + artifacts: normalizeArtifacts(record.artifacts), + evidenceRefs: normalizeEvidenceRefs(record.evidence_refs) }); }); }); diff --git a/packages/core/src/observations/playwright-json.ts b/packages/core/src/observations/playwright-json.ts index 12779f9..f1eff3e 100644 --- a/packages/core/src/observations/playwright-json.ts +++ b/packages/core/src/observations/playwright-json.ts @@ -3,6 +3,7 @@ import type { ScanDiagnostic } from "../diagnostics/diagnostic"; import { INTERNAL_OBSERVATION_CONTEXT } from "./types"; import type { IngestPlaywrightJsonReportInput, + ObservationBatchInput, ObservationIngestionResult, ObservationRecordInput, ObservationRecordStatus @@ -126,9 +127,14 @@ function observationIdFor( ].join(":"); } -export function ingestPlaywrightJsonReport( - input: IngestPlaywrightJsonReportInput -): ObservationIngestionResult { +// Parses the report into a canonical batch WITHOUT normalizing it. Split out so +// a host transport can read a Playwright report and still hand the engine the +// same un-normalized input a file-based transport would — there is no second, +// softer path into a score. +export function buildPlaywrightObservationBatch(input: IngestPlaywrightJsonReportInput): { + readonly batch?: ObservationBatchInput; + readonly diagnostics: readonly ScanDiagnostic[]; +} { const diagnostics: ScanDiagnostic[] = []; let parsed: PlaywrightJsonReport; @@ -143,11 +149,7 @@ export function ingestPlaywrightJsonReport( }) ); - return { - status: "invalid", - observations: [], - diagnostics - }; + return { diagnostics }; } if (!Array.isArray(parsed.suites)) { @@ -159,11 +161,7 @@ export function ingestPlaywrightJsonReport( }) ); - return { - status: "invalid", - observations: [], - diagnostics - }; + return { diagnostics }; } const reportFallbackObservedAt = @@ -192,11 +190,7 @@ export function ingestPlaywrightJsonReport( }) ); - return { - status: "invalid", - observations: [], - diagnostics - }; + return { diagnostics }; } const artifact = normalizeArtifact(input.artifact, "playwright-json"); @@ -215,14 +209,26 @@ export function ingestPlaywrightJsonReport( }); }); - const normalized = normalizeObservationBatches([ - { + return { + batch: { source: input.source, context: INTERNAL_OBSERVATION_CONTEXT, observations - } - ]); - const mergedDiagnostics = [...normalized.diagnostics, ...diagnostics]; + }, + diagnostics + }; +} + +export function ingestPlaywrightJsonReport( + input: IngestPlaywrightJsonReportInput +): ObservationIngestionResult { + const built = buildPlaywrightObservationBatch(input); + if (built.batch === undefined) { + return { status: "invalid", observations: [], diagnostics: built.diagnostics }; + } + + const normalized = normalizeObservationBatches([built.batch]); + const mergedDiagnostics = [...normalized.diagnostics, ...built.diagnostics]; return { status: statusFor(normalized.observations.length, mergedDiagnostics.length), diff --git a/packages/core/src/observations/quality-observations.schema.json b/packages/core/src/observations/quality-observations.schema.json index 0629fa4..d67951d 100644 --- a/packages/core/src/observations/quality-observations.schema.json +++ b/packages/core/src/observations/quality-observations.schema.json @@ -94,6 +94,27 @@ "note": { "type": "string", "minLength": 1 + }, + "artifacts": { + "type": "array", + "minItems": 1, + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "ref" + ], + "properties": { + "ref": { + "type": "string", + "minLength": 1 + }, + "label": { + "type": "string", + "minLength": 1 + } + } + } } } } diff --git a/packages/core/src/observations/resolve.ts b/packages/core/src/observations/resolve.ts index e9898d3..251d79d 100644 --- a/packages/core/src/observations/resolve.ts +++ b/packages/core/src/observations/resolve.ts @@ -219,7 +219,8 @@ function auditRowBase(record: NormalizedObservationRecord): Omit< sourceKind: record.source.kind, sourceLabel: record.source.label, runId: record.source.runId, - runUrl: record.source.runUrl + runUrl: record.source.runUrl, + evidenceRefs: record.evidenceRefs }; } @@ -383,7 +384,7 @@ export function resolveObservations( }); return { - status: statusFor(resolved, diagnostics.length), + status: statusFor(resolved, diagnostics.filter((entry) => entry.severity !== "info").length), observations: resolved, auditRows, diagnostics diff --git a/packages/core/src/observations/shiplight-report.ts b/packages/core/src/observations/shiplight-report.ts new file mode 100644 index 0000000..d394739 --- /dev/null +++ b/packages/core/src/observations/shiplight-report.ts @@ -0,0 +1,160 @@ +import { createDiagnostic } from "../diagnostics/diagnostic"; +import type { ScanDiagnostic } from "../diagnostics/diagnostic"; +import { INTERNAL_OBSERVATION_CONTEXT } from "./types"; +import type { + ObservationBatchInput, + ObservationEvidenceRefInput, + ObservationRecordInput, + ObservationRecordStatus, + ObservationRevisionInput, + ObservationSourceInput +} from "./types"; +import { isoTimestamp, normalizePath, stringValue } from "./ingest-helpers"; + +/** + * Reads the report a Shiplight YAML run writes next to the suite + * (`shiplight-report/report-data.json`). + * + * It exists because a YAML suite's results are not otherwise readable without + * asking the producer to add a second reporter: the run transpiles to Playwright + * and then writes THIS file, and a repo that has it usually has no Playwright + * JSON at all. + */ + +interface ShiplightReportTest { + readonly file?: string; + readonly title?: string; + readonly baseTitle?: string; + readonly status?: string; + readonly startTime?: string; + readonly endTime?: string; +} + +interface ShiplightReport { + readonly tests?: readonly ShiplightReportTest[]; + readonly timestamp?: string; +} + +// The same vocabulary Playwright uses, because the run is a Playwright run. +function mapStatus(value: string | undefined): ObservationRecordStatus | undefined { + switch (value) { + case "passed": + return "pass"; + case "failed": + return "fail"; + case "timedOut": + case "interrupted": + return "error"; + case "skipped": + return "skipped"; + default: + return undefined; + } +} + +/** + * Maps a reported spec path back to the YAML it was generated from. + * + * The runner reports the TRANSPILED file, but that file is a build artifact — + * gitignored, absent from a fresh checkout, and regenerated on every run. The + * source is the `.test.yaml`, and that is what a quality map can honestly pin, + * so it is what the observation must be keyed on. Pinning the generated spec + * instead would point a check at a file no reviewer has. + * + * This inverts the transpiler's own rule, which names the output by replacing + * the source suffix: `yamlPath.replace(/\.test\.yaml$/, '.yaml.spec.ts')`. + * + * Deliberately confined to this adapter rather than applied to Playwright + * reports generally: a plain Playwright project may contain a hand-written file + * genuinely called `foo.yaml.spec.ts`, and rewriting that would corrupt an + * honest identity. Here the convention is the format's own. + */ +export function shiplightSourcePath(reported: string): string { + return reported.endsWith(".yaml.spec.ts") + ? `${reported.slice(0, -".yaml.spec.ts".length)}.test.yaml` + : reported; +} + +export interface BuildShiplightObservationBatchInput { + readonly report_json: string; + readonly source?: ObservationSourceInput; + readonly revision?: ObservationRevisionInput; + readonly evidence_refs?: readonly ObservationEvidenceRefInput[]; +} + +export function buildShiplightObservationBatch(input: BuildShiplightObservationBatchInput): { + readonly batch?: ObservationBatchInput; + readonly diagnostics: readonly ScanDiagnostic[]; +} { + const diagnostics: ScanDiagnostic[] = []; + const invalid = (message: string): ScanDiagnostic => + createDiagnostic({ severity: "error", code: "INVALID_OBSERVATION_ARTIFACT", message }); + + let parsed: ShiplightReport; + try { + parsed = JSON.parse(input.report_json) as ShiplightReport; + } catch (error) { + return { + diagnostics: [ + invalid( + `Shiplight report could not be parsed: ${error instanceof Error ? error.message : String(error)}` + ) + ] + }; + } + + if (!Array.isArray(parsed.tests)) { + return { diagnostics: [invalid("Shiplight report is missing a tests array.")] }; + } + + const reportObservedAt = isoTimestamp(parsed.timestamp); + const observations: ObservationRecordInput[] = []; + + parsed.tests.forEach((test, index) => { + const reported = normalizePath(stringValue(test.file)); + const status = mapStatus(stringValue(test.status)); + if (reported === undefined || status === undefined) { + diagnostics.push( + createDiagnostic({ + severity: "warning", + code: "INVALID_OBSERVATION_RECORD", + message: `Shiplight report test ${index} has no usable file or status and was skipped.` + }) + ); + return; + } + + // `baseTitle` is the YAML test's own name; `title` is that name with the + // run's tags prefixed. A quality map is authored against the YAML, so the + // bare name is the identity a human would pin — and the tagged form would + // not match it, since tags are prefixed rather than joined by the suite + // separator the resolver folds on. + const testCase = stringValue(test.baseTitle) ?? stringValue(test.title); + const observedAt = isoTimestamp(test.endTime) ?? isoTimestamp(test.startTime) ?? reportObservedAt; + + observations.push({ + observation_id: ["shiplight-report", reported, testCase ?? "test", index].join(":"), + test_file: shiplightSourcePath(reported), + ...(testCase === undefined ? {} : { test_case: testCase }), + status, + ...(observedAt === undefined ? {} : { observed_at: observedAt }), + revision: input.revision, + evidence_refs: input.evidence_refs ?? [] + }); + }); + + if (observations.length === 0) { + return { + diagnostics: [...diagnostics, invalid("Shiplight report contained no usable test results.")] + }; + } + + return { + batch: { + source: input.source, + context: INTERNAL_OBSERVATION_CONTEXT, + observations + }, + diagnostics + }; +} diff --git a/packages/core/src/observations/types.ts b/packages/core/src/observations/types.ts index 121f7e1..6420d15 100644 --- a/packages/core/src/observations/types.ts +++ b/packages/core/src/observations/types.ts @@ -25,6 +25,26 @@ export interface ObservationArtifactInput { readonly [key: string]: unknown; } +// A pointer to the run evidence a producer kept for one observation: the video, +// the screenshot gallery, the trace, the HTML report — whatever lets a reviewer +// see what the test actually did. +// +// `ref` is OPAQUE. Quality records it and hands it back; it never parses it for +// meaning, and no engine behaviour may branch on its contents. Interpretation +// belongs to whoever configured the producer: a Shiplight run URL, a local +// report path, and a CI artifact address are all the same thing here. The one +// exception lives in the presentation layer, which links an absolute http(s) +// ref directly rather than asking a host to resolve it. +// +// Deliberately distinct from ObservationArtifactInput above, which records +// where the observation MANIFEST came from. Conflating the two would show the +// manifest file itself as run evidence. +export interface ObservationEvidenceRefInput { + readonly ref?: string; + readonly label?: string; + readonly [key: string]: unknown; +} + export interface ObservationSourceInput { readonly id?: string; readonly kind?: string; @@ -53,6 +73,7 @@ export interface ObservationRecordInput { readonly revision?: ObservationRevisionInput; readonly note?: string; readonly artifacts?: readonly ObservationArtifactInput[]; + readonly evidence_refs?: readonly ObservationEvidenceRefInput[]; readonly [key: string]: unknown; } @@ -99,12 +120,20 @@ export interface QualityObservationManifestRun { readonly url?: string; } +// The public spelling of ObservationEvidenceRefInput inside the canonical +// manifest. `ref` is required here: an entry with no pointer is not evidence. +export interface QualityObservationManifestArtifact { + readonly ref: string; + readonly label?: string; +} + export interface QualityObservationManifestRecord { readonly path: string; readonly test_case?: string; readonly status: ObservationRecordStatus; readonly observed_at?: string; readonly note?: string; + readonly artifacts?: readonly QualityObservationManifestArtifact[]; } export interface QualityObservationManifest { @@ -128,6 +157,11 @@ export interface NormalizedObservationArtifact { readonly label?: string; } +export interface NormalizedEvidenceRef { + readonly ref: string; + readonly label?: string; +} + export interface NormalizedObservationSource { readonly id?: string; readonly kind?: string; @@ -155,6 +189,7 @@ export interface NormalizedObservationRecord { readonly source: NormalizedObservationSource; readonly note?: string; readonly artifacts: readonly NormalizedObservationArtifact[]; + readonly evidenceRefs: readonly NormalizedEvidenceRef[]; } export interface ObservationIngestionResult { @@ -195,6 +230,7 @@ export interface ObservationResolutionAuditRow { readonly evidenceId?: string; readonly evidenceLocalId?: string; readonly evidencePath?: string; + readonly evidenceRefs: readonly NormalizedEvidenceRef[]; } export interface ObservationResolutionResult { @@ -217,6 +253,7 @@ export interface EvaluatedEvidenceObservation { readonly observedAt?: string; readonly commit?: string; readonly runUrl?: string; + readonly evidenceRefs: readonly NormalizedEvidenceRef[]; } export interface EvaluatedExpectationSnapshot { diff --git a/packages/core/src/operations/index.ts b/packages/core/src/operations/index.ts index 716da2a..6f30155 100644 --- a/packages/core/src/operations/index.ts +++ b/packages/core/src/operations/index.ts @@ -30,6 +30,7 @@ import { serializeHumanSources, serializeObservationSets, serializeObservationSources, + type HostObservationTransportRegistry, type HumanSource, type ObservationContextQualityRollup, type ObservationResolutionAuditRow, @@ -220,6 +221,13 @@ export interface QcExecuteObservationSetInput { * injects the org's GitHub App installation token as GITHUB_TOKEN here; a client can't set it. */ readonly env?: NodeJS.ProcessEnv; + /** + * Transports the embedding application supplies for `transport: host` + * profiles, keyed by the profile's `host.provider`. Absent in the OSS CLI and + * Explorer, which register none — a repo declaring a host provider they do + * not serve gets an explicit diagnostic rather than a silent empty result. + */ + readonly hostTransports?: HostObservationTransportRegistry; } export interface QcExecuteObservationSourceInput { readonly projectPath: string; @@ -227,6 +235,8 @@ export interface QcExecuteObservationSourceInput { readonly selection?: ObservationSetExecutionSelection; /** See QcExecuteObservationSetInput.env — the github-actions token env, injected by the box. */ readonly env?: NodeJS.ProcessEnv; + /** See QcExecuteObservationSetInput.hostTransports. */ + readonly hostTransports?: HostObservationTransportRegistry; } interface QcExecutionResolution { readonly status: ObservationResolutionResult["status"]; @@ -1078,6 +1088,7 @@ export async function executeObservationSetOp( projectRoot: scan.target.resolvedPath, selection: input.selection, env: input.env, + hostTransports: input.hostTransports, }); const resolution = resolveObservations(scan, execution); const scopedScan = applySavedQcView(scan, input.viewId) ?? scan; @@ -1134,6 +1145,7 @@ export async function executeObservationSourceOp( projectRoot: scan.target.resolvedPath, selection: input.selection, env: input.env, + hostTransports: input.hostTransports, }); const resolution = resolveObservations(scan, execution); const usable = hasUsableProof({ diff --git a/packages/core/src/recommendation-export/index.ts b/packages/core/src/recommendation-export/index.ts index d6f4bb0..0ba33d6 100644 --- a/packages/core/src/recommendation-export/index.ts +++ b/packages/core/src/recommendation-export/index.ts @@ -12,6 +12,7 @@ import { type ObservationSetExecutionSelection } from "../observation-sets"; import { findSavedQcView } from "../views/filter"; +import type { HostObservationTransportRegistry } from "../observation-sources"; import { resolveObservations } from "../observations/resolve"; import { scanProject } from "../discovery/scan-project"; import type { ScanDiagnostic } from "../diagnostics/diagnostic"; @@ -169,6 +170,8 @@ export interface BuildRecommendationExportInput { readonly limit?: number; readonly selection?: ObservationSetExecutionSelection; readonly env?: NodeJS.ProcessEnv; + /** Host transports for `transport: host` profiles, keyed by `host.provider`. */ + readonly hostTransports?: HostObservationTransportRegistry; readonly fixPromptRecords?: readonly RecommendationFixPromptRecord[]; readonly generatedAt?: Date; } @@ -552,7 +555,8 @@ export async function buildRecommendationExport( observationSourceProfiles: scan.observationSourceProfiles.primary?.document?.profiles ?? [], projectRoot: scan.target.resolvedPath, env: input.env, - selection: input.selection + selection: input.selection, + hostTransports: input.hostTransports }); const resolution = execution === undefined ? undefined : resolveObservations(scan, execution); const effectiveResult = applySavedQcView(scan, input.viewId) ?? scan; diff --git a/packages/core/src/views/index.ts b/packages/core/src/views/index.ts index 3759e72..0d97275 100644 --- a/packages/core/src/views/index.ts +++ b/packages/core/src/views/index.ts @@ -1,2 +1,3 @@ export * from "./filter"; export * from "./types"; +export * from "./json-schema"; diff --git a/packages/core/src/views/json-schema.ts b/packages/core/src/views/json-schema.ts new file mode 100644 index 0000000..5d468f1 --- /dev/null +++ b/packages/core/src/views/json-schema.ts @@ -0,0 +1,59 @@ +// Emitted from this package's own contract constants, like the other config +// schemas, so the published schema and the parser that enforces it cannot +// disagree. Before this existed the only copy lived in the agent skill, with +// nothing tying it to the parser — and it had already drifted ahead of it. +import { WHOLE_PROJECT_VIEW_ID } from "./types"; + +const nonEmptyString = { $ref: "#/definitions/nonEmptyString" } as const; + +export function buildSavedViewsJsonSchema(): Record { + return { + $schema: "http://json-schema.org/draft-07/schema#", + $id: "https://shiplight.dev/schemas/quality/views.schema.json", + title: "Quality Saved Assessment Scopes", + type: "object", + additionalProperties: false, + required: ["views"], + properties: { + views: { + type: "array", + minItems: 1, + items: { $ref: "#/definitions/view" } + } + }, + definitions: { + nonEmptyString: { type: "string", minLength: 1 }, + viewId: { + type: "string", + minLength: 1, + // The scope shown when no view is selected. A saved view claiming it + // would shadow the default scope everywhere it is used as an id. + // + // No shape rule beyond that: ids only reach filenames through the + // recommendation export's own sanitizer, so a stricter pattern would + // reject working configuration for no gain. + not: { const: WHOLE_PROJECT_VIEW_ID } + }, + view: { + type: "object", + additionalProperties: false, + required: ["id", "name", "feature_ids"], + properties: { + id: { $ref: "#/definitions/viewId" }, + name: nonEmptyString, + description: nonEmptyString, + feature_ids: { + type: "array", + minItems: 1, + uniqueItems: true, + items: nonEmptyString + } + } + } + } + }; +} + +export function serializeSavedViewsJsonSchema(): string { + return `${JSON.stringify(buildSavedViewsJsonSchema(), null, 2)}\n`; +} diff --git a/packages/core/src/views/parse.ts b/packages/core/src/views/parse.ts index 6e37a43..7d08d99 100644 --- a/packages/core/src/views/parse.ts +++ b/packages/core/src/views/parse.ts @@ -1,4 +1,5 @@ import { readFileSync } from "node:fs"; +import { WHOLE_PROJECT_VIEW_ID } from "./types"; import { parseDocument } from "yaml"; import type { ParsedSavedQcViews, @@ -93,6 +94,22 @@ function savedViewFrom( const name = stringValue(value.name); const nextFeatureIds = featureIds(value.feature_ids, source, `$.views[${index}].feature_ids`, diagnostics); + // Enforced here, not only in the published schema: the schema is what an + // author validates against, but this parser is what actually runs during a + // scan, and a rule only one of them applies is a rule the product does not + // really have. + if (id !== undefined && id === WHOLE_PROJECT_VIEW_ID) { + diagnostics.push( + diagnostic(source, { + severity: "error", + code: "INVALID_SAVED_VIEW", + message: `Saved view id ${WHOLE_PROJECT_VIEW_ID} is reserved for the unscoped assessment.`, + yamlPath: `$.views[${index}].id` + }) + ); + return undefined; + } + if (id === undefined || name === undefined) { diagnostics.push( diagnostic(source, { diff --git a/packages/core/src/views/types.ts b/packages/core/src/views/types.ts index a781ac4..37a4cd6 100644 --- a/packages/core/src/views/types.ts +++ b/packages/core/src/views/types.ts @@ -1,3 +1,8 @@ +// The scope the product shows when no saved view is selected. It is used as an +// id in its own right (recommendation export paths, the view picker), so a +// saved view may not claim it. +export const WHOLE_PROJECT_VIEW_ID = "whole-project"; + export type SavedQcViewDiagnosticSeverity = "error" | "warning" | "info"; export type SavedQcViewParseStatus = "parsed" | "invalid"; diff --git a/agent-skills/quality/references/improve/assets/views.schema.json b/packages/core/src/views/views.schema.json similarity index 61% rename from agent-skills/quality/references/improve/assets/views.schema.json rename to packages/core/src/views/views.schema.json index 6ecad50..e4d54f6 100644 --- a/agent-skills/quality/references/improve/assets/views.schema.json +++ b/packages/core/src/views/views.schema.json @@ -4,7 +4,9 @@ "title": "Quality Saved Assessment Scopes", "type": "object", "additionalProperties": false, - "required": ["views"], + "required": [ + "views" + ], "properties": { "views": { "type": "array", @@ -22,22 +24,35 @@ "viewId": { "type": "string", "minLength": 1, - "pattern": "^[a-z0-9]+(?:-[a-z0-9]+)*$", - "not": { "const": "whole-project" } + "not": { + "const": "whole-project" + } }, "view": { "type": "object", "additionalProperties": false, - "required": ["id", "name", "feature_ids"], + "required": [ + "id", + "name", + "feature_ids" + ], "properties": { - "id": { "$ref": "#/definitions/viewId" }, - "name": { "$ref": "#/definitions/nonEmptyString" }, - "description": { "$ref": "#/definitions/nonEmptyString" }, + "id": { + "$ref": "#/definitions/viewId" + }, + "name": { + "$ref": "#/definitions/nonEmptyString" + }, + "description": { + "$ref": "#/definitions/nonEmptyString" + }, "feature_ids": { "type": "array", "minItems": 1, "uniqueItems": true, - "items": { "$ref": "#/definitions/nonEmptyString" } + "items": { + "$ref": "#/definitions/nonEmptyString" + } } } } diff --git a/packages/core/tsup.config.ts b/packages/core/tsup.config.ts index 3adc5fe..ac49df9 100644 --- a/packages/core/tsup.config.ts +++ b/packages/core/tsup.config.ts @@ -31,5 +31,5 @@ export default defineConfig({ splitting: false, sourcemap: false, onSuccess: - "cp src/observation-sets/observation-sets.schema.json dist/observation-sets.schema.json && cp src/observation-sources/observation-sources.schema.json dist/observation-sources.schema.json && cp src/observations/quality-observations.schema.json dist/quality-observations.schema.json" + "cp src/observation-sets/observation-sets.schema.json dist/observation-sets.schema.json && cp src/observation-sources/observation-sources.schema.json dist/observation-sources.schema.json && cp src/observations/quality-observations.schema.json dist/quality-observations.schema.json && cp src/views/views.schema.json dist/views.schema.json" }); diff --git a/packages/quality-tools/src/cli.ts b/packages/quality-tools/src/cli.ts index 67fa63e..d65e730 100644 --- a/packages/quality-tools/src/cli.ts +++ b/packages/quality-tools/src/cli.ts @@ -4,6 +4,7 @@ import { runAnalyzeCommand } from "./commands/analyze"; import { runFixPromptsCommand } from "./commands/fix-prompts"; import { runObservationsCommand } from "./commands/observations"; import { runSchemaCommand } from "./commands/schema"; +import { runSetsCommand, runSourcesCommand, runViewsCommand } from "./commands/sources"; import { runValidateCommand } from "./commands/validate"; function printHelp(): void { @@ -17,7 +18,10 @@ Commands: fix-prompts Generate structural quality-evidence fix prompts. observations Produce and validate canonical workflow observations. schema Print the canonical quality-map JSON Schema. + sets Print the canonical observation-set JSON Schema. + sources Print the canonical observation-source profile JSON Schema. validate Validate a quality-map YAML file against the engine. + views Print the canonical saved-view JSON Schema. Run "quality-tools --help" for command options. `); @@ -43,6 +47,15 @@ async function main(argv: readonly string[]): Promise { if (command === "schema") { return runSchemaCommand(argv.slice(1)); } + if (command === "sets") { + return runSetsCommand(argv.slice(1)); + } + if (command === "sources") { + return runSourcesCommand(argv.slice(1)); + } + if (command === "views") { + return runViewsCommand(argv.slice(1)); + } if (command === "validate") { return runValidateCommand(argv.slice(1)); } diff --git a/packages/quality-tools/src/commands/analyze.ts b/packages/quality-tools/src/commands/analyze.ts index 90e06a1..7e1d5a2 100644 --- a/packages/quality-tools/src/commands/analyze.ts +++ b/packages/quality-tools/src/commands/analyze.ts @@ -2,7 +2,9 @@ import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; import { dirname, join, resolve } from "node:path"; import { buildRecommendationExport, + createLocalReportsTransport, generateFixPrompts, + LOCAL_REPORTS_PROVIDER, type BuildRecommendationExportInput, type RecommendationFixPromptRecord } from "@shiplightai/quality-core"; @@ -230,6 +232,9 @@ export async function runAnalyzeCommand(argv: readonly string[]): Promise { expect(await runObservationsCommand(["validate", source])).toEqual({ exitCode: 0 }); }); + it("records a gate with a pointer to its run evidence", async () => { + const destination = output("quality-observations.json"); + + const result = await runObservationsCommand([ + "record", + "--path", + ".github/workflows/publish.yml", + "--status", + "pass", + "--commit", + "abc123", + "--observed-at", + "2026-07-26T18:00:00Z", + "--artifact-ref", + "https://app.shiplight.ai/runs/8412?test=99231", + "--artifact-label", + "Shiplight run 8412", + "--output", + destination + ]); + + expect(result).toEqual({ exitCode: 0 }); + expect(JSON.parse(readFileSync(destination, "utf8")).observations).toEqual([ + { + path: ".github/workflows/publish.yml", + status: "pass", + artifacts: [ + { ref: "https://app.shiplight.ai/runs/8412?test=99231", label: "Shiplight run 8412" } + ] + } + ]); + }); + + it("keeps run evidence when merging manifests from the same run", async () => { + // Merge is how a sharded CI job assembles one manifest. Losing the refs here + // would silently strip evidence from every sharded suite. + const first = output("first.json"); + const second = output("second.json"); + const merged = output("merged.json"); + const envelope = { + schema_version: 1, + revision: { commit: "abc123" }, + observed_at: "2026-07-26T18:00:00Z" + }; + writeFileSync( + first, + JSON.stringify({ + ...envelope, + observations: [ + { + path: "tests/checkout.yaml", + status: "pass", + artifacts: [{ ref: "https://app.shiplight.ai/runs/8412?test=1" }] + } + ] + }) + ); + writeFileSync( + second, + JSON.stringify({ + ...envelope, + observations: [{ path: "tests/login.yaml", status: "pass" }] + }) + ); + + expect(await runObservationsCommand(["merge", first, second, "--output", merged])).toEqual({ + exitCode: 0 + }); + expect(JSON.parse(readFileSync(merged, "utf8")).observations).toEqual([ + { + path: "tests/checkout.yaml", + status: "pass", + artifacts: [{ ref: "https://app.shiplight.ai/runs/8412?test=1" }] + }, + { path: "tests/login.yaml", status: "pass" } + ]); + }); + + it("prints the checked-in observation-source schema", async () => { + // The engine's JSON file and the emitted schema must stay identical, so the + // skill can fetch the schema instead of vendoring a copy that silently rots. + const write = vi.spyOn(process.stdout, "write").mockReturnValue(true); + const expected = readFileSync( + join(process.cwd(), "packages/core/src/observation-sources/observation-sources.schema.json"), + "utf8" + ); + + expect(runSourcesCommand(["schema"])).toEqual({ exitCode: 0 }); + expect(String(write.mock.calls[0]?.[0])).toEqual(expected); + }); + + it("prints the checked-in observation-set schema", async () => { + const write = vi.spyOn(process.stdout, "write").mockReturnValue(true); + const expected = readFileSync( + join(process.cwd(), "packages/core/src/observation-sets/observation-sets.schema.json"), + "utf8" + ); + + expect(runSetsCommand(["schema"])).toEqual({ exitCode: 0 }); + expect(String(write.mock.calls[0]?.[0])).toEqual(expected); + }); + + it("prints the checked-in saved-view schema", async () => { + const write = vi.spyOn(process.stdout, "write").mockReturnValue(true); + const expected = readFileSync( + join(process.cwd(), "packages/core/src/views/views.schema.json"), + "utf8" + ); + + expect(runViewsCommand(["schema"])).toEqual({ exitCode: 0 }); + expect(String(write.mock.calls[0]?.[0])).toEqual(expected); + }); + + it("rejects an unknown sources subcommand instead of printing something else", async () => { + vi.spyOn(console, "error").mockImplementation(() => {}); + + expect(runSourcesCommand(["nonsense"])).toEqual({ exitCode: 1 }); + }); + it("prints the checked-in canonical schema", async () => { const write = vi.spyOn(process.stdout, "write").mockReturnValue(true); const expected = readFileSync( diff --git a/packages/quality-tools/src/commands/observations.ts b/packages/quality-tools/src/commands/observations.ts index b21b60b..f53b1b6 100644 --- a/packages/quality-tools/src/commands/observations.ts +++ b/packages/quality-tools/src/commands/observations.ts @@ -28,6 +28,8 @@ interface ProducerOptions { readonly testCase?: string; readonly status?: string; readonly note?: string; + readonly artifactRef?: string; + readonly artifactLabel?: string; readonly help: boolean; } @@ -50,6 +52,12 @@ Producer metadata: --run-id Optional run identity. Defaults to GITHUB_RUN_ID. --run-url Optional run URL. Derived from GitHub environment when available. +Run evidence (record only): + --artifact-ref Opaque pointer to this result's run evidence — a report + URL, a run page, a local path. Recorded verbatim and + never interpreted. + --artifact-label Optional human-readable label for that pointer. + Canonical statuses: pass, fail, error, skipped. `); } @@ -67,6 +75,8 @@ function parseOptions(argv: readonly string[]): ProducerOptions { testCase?: string; status?: string; note?: string; + artifactRef?: string; + artifactLabel?: string; dirty: boolean; help: boolean; } = { @@ -95,7 +105,9 @@ function parseOptions(argv: readonly string[]): ProducerOptions { "--path": "path", "--test-case": "testCase", "--status": "status", - "--note": "note" + "--note": "note", + "--artifact-ref": "artifactRef", + "--artifact-label": "artifactLabel" }; const key = keys[arg]; if (key !== undefined) { @@ -201,7 +213,11 @@ function canonicalRecord( ...(testCase === undefined ? {} : { test_case: testCase }), status: observation.status, ...(observation.observedAt === envelopeObservedAt ? {} : { observed_at: observation.observedAt }), - ...(observation.note === undefined ? {} : { note: observation.note }) + ...(observation.note === undefined ? {} : { note: observation.note }), + // Adapters that carry run evidence must not lose it at serialization. The + // bundled junit/playwright adapters emit none today, so this is a no-op for + // them — it is here so adding one later needs no change in this file. + ...(observation.evidenceRefs.length === 0 ? {} : { artifacts: observation.evidenceRefs }) }; } @@ -252,6 +268,9 @@ function convertNativeReport(kind: "junit" | "playwright", sourcePath: string, o } function recordObservation(options: ProducerOptions): void { + if (options.artifactRef === undefined && options.artifactLabel !== undefined) { + throw new Error("--artifact-label requires --artifact-ref."); + } const status = options.status as ObservationRecordStatus | undefined; if (status === undefined || !new Set(["pass", "fail", "error", "skipped"]).has(status)) { throw new Error("--status must be one of: pass, fail, error, skipped."); @@ -264,7 +283,21 @@ function recordObservation(options: ProducerOptions): void { path: nonEmpty(options.path, "--path"), ...(options.testCase === undefined ? {} : { test_case: nonEmpty(options.testCase, "--test-case") }), status, - ...(options.note === undefined ? {} : { note: nonEmpty(options.note, "--note") }) + ...(options.note === undefined ? {} : { note: nonEmpty(options.note, "--note") }), + // A label with no ref points at nothing. Silently dropping it would let a + // CI step believe it published run evidence when it published none. + ...(options.artifactRef === undefined + ? {} + : { + artifacts: [ + { + ref: nonEmpty(options.artifactRef, "--artifact-ref"), + ...(options.artifactLabel === undefined + ? {} + : { label: nonEmpty(options.artifactLabel, "--artifact-label") }) + } + ] + }) } ] }); diff --git a/packages/quality-tools/src/commands/sources.ts b/packages/quality-tools/src/commands/sources.ts new file mode 100644 index 0000000..135104b --- /dev/null +++ b/packages/quality-tools/src/commands/sources.ts @@ -0,0 +1,85 @@ +import { + serializeObservationSetsJsonSchema, + serializeObservationSourceProfilesJsonSchema, + serializeSavedViewsJsonSchema +} from "@shiplightai/quality-core"; +import { printCommandError, type CommandResult } from "./result"; + +function printSourcesHelp(): void { + console.log(`Print the canonical observation-source profile JSON Schema to stdout. + +Usage: + quality-tools sources schema + +The schema is emitted from this quality-tools version's own contract constants, so it always +matches the parser that reads .quality/config/observation-sources.yaml — there is no second +copy to keep in sync. Fetch it rather than vendoring it: + quality-tools sources schema > observation-sources.schema.json +`); +} + +export function runSourcesCommand(argv: readonly string[]): CommandResult { + return runSchemaSubcommand( + argv, + printSourcesHelp, + "sources", + serializeObservationSourceProfilesJsonSchema + ); +} + +function printSetsHelp(): void { + console.log(`Print the canonical observation-set JSON Schema to stdout. + +Usage: + quality-tools sets schema + +Emitted from this quality-tools version's own contract constants, so it always +matches the parser that reads .quality/config/observation-sets.yaml. +`); +} + +export function runSetsCommand(argv: readonly string[]): CommandResult { + return runSchemaSubcommand(argv, printSetsHelp, "sets", serializeObservationSetsJsonSchema); +} + +function printViewsHelp(): void { + console.log(`Print the canonical saved-view JSON Schema to stdout. + +Usage: + quality-tools views schema + +Emitted from this quality-tools version's own contract constants, so it always +matches the parser that reads .quality/config/views.yaml. +`); +} + +export function runViewsCommand(argv: readonly string[]): CommandResult { + return runSchemaSubcommand(argv, printViewsHelp, "views", serializeSavedViewsJsonSchema); +} + +// The three config schemas differ only in which serializer they print, so the +// argument handling — help, unknown subcommand, error shape — lives once. +function runSchemaSubcommand( + argv: readonly string[], + printHelp: () => void, + command: string, + serialize: () => string +): CommandResult { + const subcommand = argv[0]; + + if (argv.includes("--help") || argv.includes("-h") || subcommand === undefined) { + printHelp(); + return { exitCode: 0 }; + } + + try { + if (subcommand !== "schema") { + throw new Error(`Unknown ${command} subcommand: ${subcommand}`); + } + + process.stdout.write(serialize()); + return { exitCode: 0 }; + } catch (error) { + return printCommandError(error); + } +} diff --git a/packages/ui/src/components/FeaturePage.tsx b/packages/ui/src/components/FeaturePage.tsx index 6daa7e9..dba28f4 100644 --- a/packages/ui/src/components/FeaturePage.tsx +++ b/packages/ui/src/components/FeaturePage.tsx @@ -1,14 +1,15 @@ "use client"; -import { useQcApi, useQcRoute } from "../host"; +import { useQcApi, useQcHost, useQcRoute } from "../host"; +import { useQcScanCache } from "./scan-cache"; import { Breadcrumb } from "./Breadcrumb"; import { MarkdownOverlay } from "./MarkdownOverlay"; import Link from "next/link"; import { useCallback, useEffect, useMemo, useRef, useState } from "react"; -import { CheckCircle2, ChevronRight, Copy } from "lucide-react"; +import { CheckCircle2, ChevronRight, Copy, ExternalLink } from "lucide-react"; import { Alert, Anchor, Badge, Button, Collapse, Group, Paper, Select, Stack, Text, TextInput, Title, Tooltip, UnstyledButton } from "@mantine/core"; -import type { ScanResult } from "@shiplightai/quality-core"; +import type { EvaluatedEvidenceObservation, NormalizedEvidenceRef, ScanResult } from "@shiplightai/quality-core"; import { buildProjectIndex } from "@shiplightai/quality-core/project-index"; import { buildGapTriage, type GapRecord } from "@shiplightai/quality-core/gap-triage"; import { canonicalFixPromptForGap } from "../lib/fix-prompt"; @@ -128,6 +129,43 @@ function checkEvidence(graph: QualityGraph, expectation: QualityCheck): QualityG return graph.evidence.filter((entry) => expectation.linkedEvidenceIds.includes(entry.normalizedId)); } +// Same rule the audit panel applies: an absolute url is linked as it stands, a +// project path is served by the host when it can, and anything else stays text +// because resolving it needs a host that can and guessing would invent a +// destination. +function evidenceRefHref( + ref: string, + qcApi: (path: string) => string, + servesEvidenceFiles: boolean +): string | undefined { + if (/^https?:\/\//i.test(ref)) { + return ref; + } + if (!servesEvidenceFiles) { + return undefined; + } + const segments = ref + .split("/") + .filter((segment) => segment.length > 0) + .map((segment) => encodeURIComponent(segment)); + return segments.length === 0 ? undefined : qcApi(`/evidence-file/${segments.join("/")}`); +} + +function observedStateColor(state: string): string { + switch (state) { + case "pass": + return "green"; + case "fail": + return "red"; + case "error": + return "orange"; + case "skipped": + return "gray"; + default: + return "gray"; + } +} + // Only Markdown artifacts can be previewed inline (the artifact/markdown endpoint reads text). function canPreviewMarkdownPath(path: string): boolean { const normalized = path.toLowerCase(); @@ -154,6 +192,8 @@ export function FeaturePage({ }): React.ReactElement { const qcApi = useQcApi(); const qcRoute = useQcRoute(); + const { servesEvidenceFiles = false } = useQcHost(); + const scanCache = useQcScanCache(); const [result, setResult] = useState(); const [isLoading, setIsLoading] = useState(false); // Set true in the effect body (not just useRef init): StrictMode/remount runs cleanup→setup, else a cleanup-only ref stays false and the loader discards its result. @@ -234,6 +274,43 @@ export function FeaturePage({ ); const expectations = graph?.expectations ?? []; + // Runtime proof for the observation set the viewer last ran on the scanner + // page. Inherited rather than re-run: this page has no picker, and running a + // set can mean a network fetch. + const runtime = projectKey === null ? undefined : scanCache?.getRuntime(projectKey); + const observed = useMemo(() => { + const byEvidenceId = new Map(); + const targetId = graph?.target.normalizedId; + if (runtime === undefined || targetId === undefined) { + return { byEvidenceId, coversFeature: false, evaluatedAt: undefined as string | undefined }; + } + + // Whether the run evaluated THIS target at all. A view-scoped run evaluates + // only the targets inside its view, so an empty map here can mean the run + // never looked rather than that it looked and found nothing — two states + // the page must not present the same way. + let coversFeature = false; + let evaluatedAt: string | undefined; + + for (const group of runtime.evaluations) { + for (const target of group.targets) { + if (target.targetId !== targetId) { + continue; + } + coversFeature = true; + evaluatedAt = target.evaluatedAt; + for (const expectation of target.expectations) { + for (const entry of expectation.evidence) { + byEvidenceId.set(entry.evidenceId, entry); + } + } + } + } + + return { byEvidenceId, coversFeature, evaluatedAt }; + }, [graph?.target.normalizedId, runtime]); + const observedEvidence = observed.byEvidenceId; + // Gap records per check (spec 045): the classified gaps — category label ("Weak evidence"), the // residual-risk text, the recommended next proof, and the fix-prompt lookup — the same model the old // Explorer used. Built from the feature's target, keyed by the check's expectation localId. @@ -419,6 +496,23 @@ export function FeaturePage({ ) : qualityMapPath === undefined || graph === undefined ? ( This feature has no quality checks yet. ) : ( + <> + {/* Runtime state on this page is only as good as its attribution: a + check reading "pass" means nothing unless a reviewer can see which + run said so. With nothing loaded the page stays structural rather + than painting every check `unobserved`, which reads like a failure + when it is only a question nobody has asked yet. */} + + {runtime === undefined + ? "No test results loaded. Run an observation set on the dashboard to see runtime proof here." + : observed.coversFeature + ? `Runtime proof from observation set: ${runtime.observationSetName}${ + observed.evaluatedAt === undefined ? "" : ` · evaluated ${observed.evaluatedAt}` + }` + : `Observation set ${runtime.observationSetName} did not cover this feature${ + runtime.viewId === undefined ? "" : ` — it ran scoped to the saved view ${runtime.viewId}` + }.`} + {expectations.length === 0 ? ( No quality checks yet. Copy the add-check instruction below to have your agent add one. @@ -483,12 +577,36 @@ export function FeaturePage({ ) : ( {evidence.length > 0 ? ( - - {evidence.map((entry) => ( - - {evidenceLabel(entry)} - - ))} + + {evidence.map((entry) => { + const observed = observedEvidence.get(entry.normalizedId); + return ( + + + + {evidenceLabel(entry)} + + {observed === undefined ? null : ( + + {observed.state} + + )} + + {observed?.evidenceRefs.map((ref: NormalizedEvidenceRef) => { + const href = evidenceRefHref(ref.ref, qcApi, servesEvidenceFiles); + return href === undefined ? ( + + {ref.label ?? "Run evidence"}: {ref.ref} + + ) : ( + + {ref.label ?? "Run evidence"} + + ); + })} + + ); + })} ) : null} {checkGaps.map((gap) => { @@ -607,6 +725,7 @@ export function FeaturePage({ + )} diff --git a/packages/ui/src/components/ObservationAuditPanel.test.tsx b/packages/ui/src/components/ObservationAuditPanel.test.tsx new file mode 100644 index 0000000..384a9b3 --- /dev/null +++ b/packages/ui/src/components/ObservationAuditPanel.test.tsx @@ -0,0 +1,151 @@ +// @vitest-environment jsdom + +import "@testing-library/jest-dom/vitest"; +import { MantineProvider } from "@mantine/core"; +import { cleanup, screen } from "@testing-library/react"; +import { render } from "../testing"; +import { afterEach, beforeAll, describe, expect, it, vi } from "vitest"; +import type { ObservationResolutionAuditRow } from "@shiplightai/quality-core"; +import { QcUiHostProvider, type QcUiHost } from "../host"; +import { ObservationAuditPanel } from "./ObservationAuditPanel"; + +beforeAll(() => { + Object.defineProperty(window, "matchMedia", { + writable: true, + value: vi.fn().mockImplementation((query: string) => ({ + matches: false, + media: query, + onchange: null, + addListener: vi.fn(), + removeListener: vi.fn(), + addEventListener: vi.fn(), + removeEventListener: vi.fn(), + dispatchEvent: vi.fn() + })) + }); +}); + +afterEach(() => cleanup()); + +function auditRow(overrides: Partial = {}): ObservationResolutionAuditRow { + return { + observationId: "obs-1", + matchStatus: "matched", + testFile: "tests/checkout.yaml", + testCase: "guest can pay", + context: "runtime-review", + status: "pass", + observedAt: "2026-08-27T10:00:00.000Z", + evidenceRefs: [], + ...overrides + }; +} + +function hostWith(servesEvidenceFiles: boolean): QcUiHost { + return { + routeBase: "/quality-explorer", + apiBase: "/api/quality-explorer", + setProject: async () => ({ ok: true }), + servesEvidenceFiles + }; +} + +function renderPanel( + rows: readonly ObservationResolutionAuditRow[], + servesEvidenceFiles = true +): void { + render( + + + {}} /> + + + ); +} + +describe("ObservationAuditPanel run evidence", () => { + it("links an http ref and shows the host it points at", () => { + // Refs are written by evidence producers, so the viewer is told where the + // link goes before clicking rather than trusting a producer-chosen label. + renderPanel([ + auditRow({ + evidenceRefs: [ + { ref: "https://app.shiplight.ai/runs/8412?test=99231", label: "Shiplight run 8412" } + ] + }) + ]); + + const link = screen.getByRole("link", { name: /Shiplight run 8412/ }); + expect(link).toHaveAttribute("href", "https://app.shiplight.ai/runs/8412?test=99231"); + expect(link).toHaveAttribute("rel", expect.stringContaining("noopener")); + expect(link).toHaveAttribute("target", "_blank"); + expect(screen.getByText("app.shiplight.ai")).toBeInTheDocument(); + }); + + it("falls back to a generic label when the producer supplied none", () => { + renderPanel([auditRow({ evidenceRefs: [{ ref: "https://app.shiplight.ai/runs/8412" }] })]); + + expect(screen.getByRole("link", { name: /Run evidence/ })).toBeInTheDocument(); + }); + + it("serves a project-relative ref through the host, as path segments", () => { + // Path segments, not a query parameter: the reports these refs point at + // fetch their own video and trace with relative urls, which only resolve if + // the served page sits at the same shape of address as its folder. + renderPanel([ + auditRow({ evidenceRefs: [{ ref: "playwright-report/index.html", label: "Test report" }] }) + ]); + + const link = screen.getByRole("link", { name: /Test report/ }); + expect(link).toHaveAttribute( + "href", + "/api/quality-explorer/evidence-file/playwright-report/index.html" + ); + expect(screen.getByText("playwright-report/index.html")).toBeInTheDocument(); + }); + + it("encodes each path segment without collapsing the path", () => { + renderPanel([auditRow({ evidenceRefs: [{ ref: "reports/my run/index.html" }] })]); + + expect(screen.getByRole("link", { name: /Run evidence/ })).toHaveAttribute( + "href", + "/api/quality-explorer/evidence-file/reports/my%20run/index.html" + ); + }); + + it("shows a project-relative ref as text when the host cannot serve files", () => { + // A hosted reader with no local checkout has nothing to serve. Linking + // anyway would render a link that 404s on click. + renderPanel( + [auditRow({ evidenceRefs: [{ ref: "playwright-report/index.html", label: "Test report" }] })], + false + ); + + expect(screen.queryByRole("link", { name: /Test report/ })).not.toBeInTheDocument(); + expect(screen.getByText(/playwright-report\/index\.html/)).toBeInTheDocument(); + }); + + it("renders no run evidence section when the observation carried no refs", () => { + renderPanel([auditRow()]); + + expect(screen.queryByLabelText("Run evidence")).not.toBeInTheDocument(); + }); + + it("keeps run evidence separate from the workflow run link", () => { + renderPanel([ + auditRow({ + runUrl: "https://github.com/ShiplightAI/shipyard/actions/runs/42", + evidenceRefs: [{ ref: "https://app.shiplight.ai/runs/8412", label: "Shiplight run 8412" }] + }) + ]); + + expect(screen.getByRole("link", { name: /Open workflow result/ })).toHaveAttribute( + "href", + "https://github.com/ShiplightAI/shipyard/actions/runs/42" + ); + expect(screen.getByRole("link", { name: /Shiplight run 8412/ })).toHaveAttribute( + "href", + "https://app.shiplight.ai/runs/8412" + ); + }); +}); diff --git a/packages/ui/src/components/ObservationAuditPanel.tsx b/packages/ui/src/components/ObservationAuditPanel.tsx index 5ee7b31..72acfaa 100644 --- a/packages/ui/src/components/ObservationAuditPanel.tsx +++ b/packages/ui/src/components/ObservationAuditPanel.tsx @@ -3,7 +3,8 @@ import { ExternalLink, X } from "lucide-react"; import { ActionIcon, Anchor, Badge, Code, Group, Paper, Stack, Text, Title } from "@mantine/core"; import { useMemo, useState } from "react"; -import type { ObservationResolutionAuditRow } from "@shiplightai/quality-core"; +import { useQcApi, useQcHost } from "../host"; +import type { NormalizedEvidenceRef, ObservationResolutionAuditRow } from "@shiplightai/quality-core"; type MatchFilter = "all" | ObservationResolutionAuditRow["matchStatus"]; @@ -12,6 +13,61 @@ interface ObservationAuditPanelProps { onClose(): void; } +function isAbsoluteUrl(ref: string): boolean { + return /^https?:\/\//i.test(ref); +} + +// The single place the UI looks at a ref at all, and it looks only at whether +// the producer already gave us something a browser can open, or whether the +// host can turn a project path into something it can. +// +// A path is passed through as PATH SEGMENTS rather than a query parameter. The +// reports these refs point at fetch their own assets with relative urls, so the +// served page has to sit at the same shape of address as the folder it came +// from or its video and trace links resolve to nothing. +// +// Anything else stays text: resolving it needs a host that can, and guessing a +// URL for it would invent a destination. +function evidenceHref( + ref: string, + qcApi: (path: string) => string, + servesEvidenceFiles: boolean +): string | undefined { + if (isAbsoluteUrl(ref)) { + return ref; + } + + if (!servesEvidenceFiles) { + return undefined; + } + + const segments = ref + .split("/") + .filter((segment) => segment.length > 0) + .map((segment) => encodeURIComponent(segment)); + return segments.length === 0 ? undefined : qcApi(`/evidence-file/${segments.join("/")}`); +} + +// Refs are written by evidence producers, so the destination is shown rather +// than hidden behind a label the producer also chose. A project-relative ref +// shows its path instead: the host it would resolve to is this application, +// which tells the reader nothing. +function evidenceDestination(ref: string): string { + if (!isAbsoluteUrl(ref)) { + return ref; + } + + try { + return new URL(ref).host; + } catch { + return ref; + } +} + +function evidenceLabel(entry: NormalizedEvidenceRef): string { + return entry.label ?? "Run evidence"; +} + function sourceLabel(row: ObservationResolutionAuditRow): string { return row.testFile ?? row.testClass ?? row.observationId; } @@ -53,6 +109,8 @@ export function ObservationAuditPanel({ rows, onClose }: ObservationAuditPanelProps): React.ReactElement { + const qcApi = useQcApi(); + const { servesEvidenceFiles = false } = useQcHost(); const [filter, setFilter] = useState("all"); const filteredRows = useMemo( () => rows.filter((row) => filter === "all" || row.matchStatus === filter), @@ -128,6 +186,35 @@ export function ObservationAuditPanel({ Open workflow result ) : null} + + {row.evidenceRefs.length > 0 ? ( + + Run evidence + {row.evidenceRefs.map((entry) => { + const href = evidenceHref(entry.ref, qcApi, servesEvidenceFiles); + const destination = evidenceDestination(entry.ref); + return href === undefined ? ( + + {evidenceLabel(entry)}: {entry.ref} + + ) : ( + + + {evidenceLabel(entry)} + + + {destination} + + + ); + })} + + ) : null} ))} diff --git a/packages/ui/src/components/ObservationSourcesView.tsx b/packages/ui/src/components/ObservationSourcesView.tsx index d04be0d..499ef51 100644 --- a/packages/ui/src/components/ObservationSourcesView.tsx +++ b/packages/ui/src/components/ObservationSourcesView.tsx @@ -9,8 +9,9 @@ export interface ObservationSourceRow { readonly id: string; readonly name: string; readonly description?: string; - readonly transport: "github-actions" | "local-folder"; - readonly observationPath: string; + readonly transport: "github-actions" | "local-folder" | "host"; + // Absent for a host source, which addresses no file. + readonly observationPath?: string; readonly github?: { readonly repo: string; readonly workflow: string; @@ -18,6 +19,7 @@ export interface ObservationSourceRow { readonly branch?: string; }; readonly localFolder?: { readonly path: string }; + readonly host?: { readonly provider: string; readonly options: Readonly> }; } // Read-only view of the repo's observation sources (spec 045). Authoring moved to the repo — every @@ -97,11 +99,20 @@ export function ObservationSourcesView({ folder: {profile.localFolder.path || "—"} + ) : profile.host !== undefined ? ( + + provider: {profile.host.provider} + {Object.entries(profile.host.options) + .map(([key, value]) => ` · ${key}=${value}`) + .join("")} + ) : null} - - observations: {profile.observationPath} - + {profile.observationPath === undefined ? null : ( + + observations: {profile.observationPath} + + )} ); diff --git a/packages/ui/src/components/ProjectScanner.tsx b/packages/ui/src/components/ProjectScanner.tsx index 871767c..385c074 100644 --- a/packages/ui/src/components/ProjectScanner.tsx +++ b/packages/ui/src/components/ProjectScanner.tsx @@ -541,11 +541,16 @@ export function ProjectScanner({ setCurrentResult(scanResponse.result); setObservationSourceEnv(scanResponse.observationSourceEnv); // Cache this scan under the current project so navigating away and back reuses it. + // Drop the cached runtime with it: an evaluation is only meaningful against the + // structure it was resolved onto, and this scan may have replaced that structure. + // Keeping it would let a feature page paint pass/fail from a run that never saw + // the checks now on screen — a claim about the wrong thing, attributed to a real set. if (projectKey !== null) { scanCache?.set(projectKey, { result: scanResponse.result, observationSourceEnv: scanResponse.observationSourceEnv, }); + scanCache?.clearRuntime(projectKey); } setIsObservationAuditOpen(false); setLastAttemptDiagnostics( @@ -635,6 +640,27 @@ export function ProjectScanner({ const executionResponse = payload as ObservationSetExecutionResponse; setObservationExecution(executionResponse.result); + // Hand the evaluated result to the shared cache so a feature page shows + // proof for the set the viewer just ran, rather than running its own. + // + // Only when the run actually produced evaluations. A run with no usable + // proof — a local report that has not been generated yet, say — still + // answers 200 and returns an empty `evaluations`, and caching that would + // put "Runtime proof from " above a feature whose checks carry no + // badges at all: a claim of proof that is really an absence of it. + if (projectKey !== null && executionResponse.result.evaluations.length > 0) { + const observationSet = observationSets.find( + (candidate) => candidate.id === selectedObservationSetId + ); + scanCache?.setRuntime(projectKey, { + observationSetId: selectedObservationSetId, + observationSetName: observationSet?.name ?? selectedObservationSetId, + ...(selectedView?.id === undefined ? {} : { viewId: selectedView.id }), + evaluations: executionResponse.result.evaluations + }); + } else if (projectKey !== null) { + scanCache?.clearRuntime(projectKey); + } } catch { setObservationExecutionDiagnostics([ fallbackDiagnostic("The observation set could not be executed.") diff --git a/packages/ui/src/components/Settings.tsx b/packages/ui/src/components/Settings.tsx index cf6b7b8..8b46345 100644 --- a/packages/ui/src/components/Settings.tsx +++ b/packages/ui/src/components/Settings.tsx @@ -71,7 +71,8 @@ export function Settings({ artifactNames: p.github.artifactNames, branch: p.github.branch }, - localFolder: p.localFolder === undefined ? undefined : { path: p.localFolder.path } + localFolder: p.localFolder === undefined ? undefined : { path: p.localFolder.path }, + host: p.host === undefined ? undefined : { provider: p.host.provider, options: p.host.options } })), [profiles] ); diff --git a/packages/ui/src/components/scan-cache.tsx b/packages/ui/src/components/scan-cache.tsx index 19c879d..78e6dc1 100644 --- a/packages/ui/src/components/scan-cache.tsx +++ b/packages/ui/src/components/scan-cache.tsx @@ -1,7 +1,11 @@ "use client"; import { createContext, useContext, useRef, type ReactNode } from "react"; -import type { ObservationSourceProfileEnvStatus, ScanResult } from "@shiplightai/quality-core"; +import type { + ObservationSourceProfileEnvStatus, + ScanResult, + TargetEvaluationSnapshot, +} from "@shiplightai/quality-core"; // Per-project scan cache (spec 045). Mounted in the QC layout, which persists across its child // routes (overview / reviews / explorer), so a scan of a given project is reused when the user @@ -13,11 +17,43 @@ export interface CachedScan { readonly observationSourceEnv: readonly ObservationSourceProfileEnvStatus[]; } +/** + * The last observation set a viewer ran for a project, so a feature page shows + * runtime proof for that same set instead of asking again or re-running it. + * + * Cached rather than re-fetched because running a set is not free: a + * github-actions source downloads artifacts over the network, and re-running it + * on every feature page view would turn a navigation into a fetch. It is also + * the only way the selection can be *inherited* — the run happens on the + * scanner page, and the feature page has no picker of its own. + * + * The set id and name ride along because a feature page showing runtime state + * must be able to say which set produced it; "observed" with no attribution is + * a claim a reviewer cannot check. `viewId` rides along for the same reason: + * a view-scoped run evaluates only the targets inside that view, so a feature + * outside it has no snapshot for a reason the page must be able to state + * instead of rendering an empty proof column that reads like a failure. + */ +export interface CachedRuntime { + readonly observationSetId: string; + readonly observationSetName: string; + /** The saved view the run was scoped to, when it was scoped to one. */ + readonly viewId?: string; + readonly evaluations: readonly { readonly targets: readonly TargetEvaluationSnapshot[] }[]; +} + interface QcScanCache { get(projectKey: string): CachedScan | undefined; set(projectKey: string, value: CachedScan): void; + getRuntime(projectKey: string): CachedRuntime | undefined; + setRuntime(projectKey: string, value: CachedRuntime): void; + // Drop only the runtime, keeping the scan — what a re-scan needs, since the fresh + // structure is worth caching but the evaluation resolved onto the old one is not. + clearRuntime(projectKey: string): void; // Drop a project's cached scan (or all, when no key) after a draft mutation (save/publish/discard) // so a subsequent ProjectScanner page re-scans instead of showing the pre-mutation snapshot. + // Runtime goes with it: an evaluation is only meaningful against the structure it was resolved + // onto, so keeping it across a re-scan could show proof against checks that have since changed. invalidate(projectKey?: string): void; } @@ -25,14 +61,27 @@ const QcScanCacheContext = createContext(null); export function QcScanCacheProvider({ children }: { readonly children: ReactNode }): React.ReactElement { const store = useRef>(new Map()); + const runtimeStore = useRef>(new Map()); const cacheRef = useRef({ get: (key) => store.current.get(key), set: (key, value) => { store.current.set(key, value); }, + getRuntime: (key) => runtimeStore.current.get(key), + setRuntime: (key, value) => { + runtimeStore.current.set(key, value); + }, + clearRuntime: (key) => { + runtimeStore.current.delete(key); + }, invalidate: (key) => { - if (key === undefined) store.current.clear(); - else store.current.delete(key); + if (key === undefined) { + store.current.clear(); + runtimeStore.current.clear(); + } else { + store.current.delete(key); + runtimeStore.current.delete(key); + } }, }); return {children}; diff --git a/packages/ui/src/host.tsx b/packages/ui/src/host.tsx index 97cfa4a..518c6f0 100644 --- a/packages/ui/src/host.tsx +++ b/packages/ui/src/host.tsx @@ -41,6 +41,16 @@ export interface QcUiHost { readonly setProject: ( project: QcProjectSelection, ) => Promise<{ readonly ok: true } | { readonly error: string }>; + /** + * Whether this host serves run-evidence files out of the opened project at + * `{apiBase}/evidence-file/`. + * + * A run-evidence ref that is not already a URL is a path into the project, and + * only a host reading that project can turn it into something openable. + * Quality Explorer can; a hosted reader with no local checkout cannot, and + * without this flag it would render a link that 404s. Absent means no. + */ + readonly servesEvidenceFiles?: boolean; } const QcUiHostContext = createContext(null); @@ -56,7 +66,7 @@ export function QcUiHostProvider({ // invalidate every consumer. `setProject` is a stable Server Action reference in both hosts. const value = useMemo( () => host, - [host.routeBase, host.apiBase, host.setProject], + [host.routeBase, host.apiBase, host.setProject, host.servesEvidenceFiles], ); return {children}; } diff --git a/scripts/check-quality-skill.sh b/scripts/check-quality-skill.sh index 5a31118..bbdb833 100755 --- a/scripts/check-quality-skill.sh +++ b/scripts/check-quality-skill.sh @@ -102,6 +102,17 @@ if [[ -e "${skill_root}/references/improve/assets/quality-observations.schema.js fail "use the published observations schema instead of a bundled static copy" fi +# Same rule for every config schema. A vendored copy cannot be checked against +# the engine's, so it drifts the moment the contract moves -- which is exactly +# what happened when the `host` transport landed, and what left the views copy +# enforcing rules the parser did not. Each is emitted from the engine's own +# constants by `quality-tools schema`; fetch that instead. +for vendored in observation-sources observation-sets views; do + if [[ -e "${skill_root}/references/improve/assets/${vendored}.schema.json" ]]; then + fail "use a quality-tools schema command instead of bundling ${vendored}.schema.json" + fi +done + forbid_regex \ 'machine-readable result|acquisition/parser problems|acquired and parsed' \ "${skill_root}" \ diff --git a/tests/contract/observation-run-evidence.contract.test.ts b/tests/contract/observation-run-evidence.contract.test.ts new file mode 100644 index 0000000..8b7bc76 --- /dev/null +++ b/tests/contract/observation-run-evidence.contract.test.ts @@ -0,0 +1,536 @@ +import path from "node:path"; +import { describe, expect, it } from "vitest"; +import { + buildMarkdownFallbackBatch, + buildShiplightObservationBatch, + buildTargetEvaluation, + executeObservationSourceProfile, + ingestObservationManifest, + parseObservationSourceProfiles, + parseQualityObservationManifest, + resolveObservations, + type HostObservationTransportRegistry, + type ObservationSourceProfile +} from "@shiplightai/quality-core"; +import { parseQualityMaps, type QualityMapSource } from "@shiplightai/quality-map"; +import { createFixtureProject } from "../fixtures/quality-projects/build-fixtures"; +import { projectIndexScanResult } from "../fixtures/project-index/build-fixtures"; + +const checksFixtureRoot = path.resolve("tests/fixtures/observations/checks"); + +function checksScanResult() { + const source: QualityMapSource = { + projectRelativePath: "checks/quality-map.yaml", + resolvedLocalPath: path.join(checksFixtureRoot, "quality-map.yaml"), + targetCandidateId: "ci-runner", + sourcePattern: "tests/fixtures/observations/checks/**/quality-map.yaml" + }; + const qualityMaps = parseQualityMaps([source]); + return projectIndexScanResult({ + qualityMaps, + markdownFallback: buildMarkdownFallbackBatch({ sources: [], qualityMaps }) + }); +} + +// Run evidence is the pointer a producer keeps for one result — the video, the +// report, the run page. Quality records it and shows it; it never interprets it. +// These tests pin that boundary: what the contract accepts, that an opaque ref +// survives ingestion unchanged, and that a bad pointer never costs us the +// pass/fail fact it was attached to. + +function manifest(observations: readonly Record[]): string { + return JSON.stringify({ + schema_version: 1, + revision: { commit: "abc123", branch: "main" }, + run: { id: "run-42", url: "https://ci.example.test/runs/42" }, + observed_at: "2026-08-27T10:00:00Z", + observations + }); +} + +const SHIPLIGHT_REF = "https://app.shiplight.ai/runs/8412?test=99231"; + +describe("observation run evidence contract", () => { + it("carries an opaque artifact ref from the manifest onto the ingested observation", () => { + const result = ingestObservationManifest({ + report_json: manifest([ + { + path: "tests/checkout.yaml", + test_case: "guest can pay", + status: "pass", + artifacts: [{ ref: SHIPLIGHT_REF, label: "Shiplight run 8412" }] + } + ]), + source: { id: "shiplight", kind: "host", label: "Shiplight" } + }); + + expect(result.status).toBe("valid"); + expect(result.diagnostics).toEqual([]); + expect(result.observations[0]?.evidenceRefs).toEqual([ + { ref: SHIPLIGHT_REF, label: "Shiplight run 8412" } + ]); + }); + + it("records a ref that is not a URL without rewriting or resolving it", () => { + // A local report path is as valid a ref as a URL. The engine must not try to + // turn it into one — only the integration that wrote it knows what it means. + const localRef = "test-results/checkout/index.html"; + const result = ingestObservationManifest({ + report_json: manifest([ + { path: "tests/checkout.yaml", status: "pass", artifacts: [{ ref: localRef }] } + ]) + }); + + expect(result.observations[0]?.evidenceRefs).toEqual([{ ref: localRef }]); + }); + + it("leaves an observation with no artifacts carrying an empty ref list", () => { + const result = ingestObservationManifest({ + report_json: manifest([{ path: "tests/checkout.yaml", status: "pass" }]) + }); + + expect(result.observations[0]?.evidenceRefs).toEqual([]); + }); + + it("keeps the observed result when an evidence pointer is malformed", () => { + // The status is the measurement; the ref is only how a reviewer looks at it. + // Dropping a real pass because its video pointer was malformed would trade a + // measurement for a convenience. + const result = ingestObservationManifest({ + report_json: manifest([ + { path: "tests/checkout.yaml", test_case: "guest can pay", status: "fail", artifacts: [{ ref: "" }] } + ]) + }); + + expect(result.observations).toHaveLength(1); + expect(result.observations[0]?.status).toBe("fail"); + expect(result.observations[0]?.evidenceRefs).toEqual([]); + expect(result.diagnostics.map((entry) => entry.severity)).toEqual(["warning"]); + expect(result.diagnostics[0]?.message).toContain("requires a non-empty ref"); + }); + + it("rejects an artifact entry carrying unknown fields rather than ignoring them", () => { + // additionalProperties: false all the way down. A producer inventing a field + // must hear about it, not have it silently dropped. + const parsed = parseQualityObservationManifest( + manifest([ + { + path: "tests/checkout.yaml", + status: "pass", + artifacts: [{ ref: SHIPLIGHT_REF, kind: "video" }] + } + ]) + ); + + expect(parsed.status).toBe("invalid"); + expect(parsed.diagnostics[0]?.message).toContain("unknown fields"); + expect(parsed.diagnostics[0]?.message).toContain("kind"); + }); + + it("fails validation when an artifact entry has no ref", () => { + const parsed = parseQualityObservationManifest( + manifest([{ path: "tests/checkout.yaml", status: "pass", artifacts: [{ label: "video" }] }]) + ); + + expect(parsed.status).toBe("invalid"); + expect(parsed.diagnostics[0]?.severity).toBe("error"); + }); + + it("round-trips artifacts through a parsed manifest document", () => { + const parsed = parseQualityObservationManifest( + manifest([ + { + path: "tests/checkout.yaml", + status: "pass", + artifacts: [{ ref: SHIPLIGHT_REF, label: "Shiplight run 8412" }] + } + ]) + ); + + expect(parsed.status).toBe("valid"); + expect(parsed.document?.observations[0]?.artifacts).toEqual([ + { ref: SHIPLIGHT_REF, label: "Shiplight run 8412" } + ]); + }); +}); + +function hostProfile(overrides: Partial = {}): ObservationSourceProfile { + return { + id: "shiplight-e2e", + name: "Shiplight e2e runs", + transport: "host", + requiredEnv: [], + sourceRefs: [], + host: { provider: "shiplight", options: { repo: "ShiplightAI/shipyard" } }, + ...overrides + }; +} + +describe("host observation transport contract", () => { + it("runs a registered host provider and normalizes what it returns", async () => { + const hostTransports: HostObservationTransportRegistry = { + shiplight: async ({ profile, selection }) => ({ + batches: [ + { + source: { id: profile.id, kind: "host", label: profile.name }, + revision: { commit: selection?.commit ?? "abc123" }, + observed_at: "2026-08-27T10:00:00Z", + observations: [ + { + test_file: "tests/checkout.yaml", + test_case: "guest can pay", + status: "pass", + observed_at: "2026-08-27T10:00:00Z", + evidence_refs: [{ ref: SHIPLIGHT_REF, label: "Shiplight run 8412" }] + } + ] + } + ], + selectedRun: { runId: 8412, runUrl: "https://app.shiplight.ai/runs/8412", commit: "abc123" } + }) + }; + + const result = await executeObservationSourceProfile({ + profile: hostProfile(), + selection: { commit: "abc123" }, + hostTransports + }); + + expect(result.status).toBe("valid"); + expect(result.diagnostics).toEqual([]); + expect(result.transport).toBe("host"); + expect(result.selectedRun?.runId).toBe(8412); + expect(result.observations).toHaveLength(1); + expect(result.observations[0]?.evidenceRefs).toEqual([ + { ref: SHIPLIGHT_REF, label: "Shiplight run 8412" } + ]); + }); + + it("applies the same record validation to host records as to a parsed manifest", async () => { + // The seam is fetch-and-shape only. A host cannot get a record past a check + // that a file-based transport must pass, or it would become a second, softer + // way into a score. + const hostTransports: HostObservationTransportRegistry = { + shiplight: async () => ({ + batches: [ + { + source: { id: "shiplight", kind: "host" }, + revision: { commit: "abc123" }, + observed_at: "2026-08-27T10:00:00Z", + observations: [ + { test_file: "tests/checkout.yaml", status: "passed", observed_at: "2026-08-27T10:00:00Z" } + ] + } + ] + }) + }; + + const result = await executeObservationSourceProfile({ + profile: hostProfile(), + hostTransports + }); + + expect(result.observations).toEqual([]); + expect(result.diagnostics.some((entry) => entry.message.includes("status"))).toBe(true); + }); + + it("names the registered providers when the declared one is not available here", async () => { + // The OSS CLI reading a config written for the hosted app hits this. It has + // to be able to tell "this reader cannot serve that provider" from "typo". + const result = await executeObservationSourceProfile({ + profile: hostProfile(), + hostTransports: { "some-other-host": async () => ({ batches: [] }) } + }); + + expect(result.status).toBe("invalid"); + expect(result.observations).toEqual([]); + expect(result.diagnostics[0]?.message).toContain("shiplight"); + expect(result.diagnostics[0]?.message).toContain("some-other-host"); + }); + + it("reports a provider with no registry at all rather than returning an empty pass", async () => { + const result = await executeObservationSourceProfile({ profile: hostProfile() }); + + expect(result.status).toBe("invalid"); + expect(result.diagnostics[0]?.severity).toBe("error"); + expect(result.diagnostics[0]?.message).toContain("(none)"); + }); + + it("turns a throwing host provider into a diagnostic instead of an unhandled rejection", async () => { + const result = await executeObservationSourceProfile({ + profile: hostProfile(), + hostTransports: { + shiplight: async () => { + throw new Error("shiplight API returned 503"); + } + } + }); + + expect(result.status).toBe("invalid"); + expect(result.diagnostics[0]?.message).toContain("shiplight API returned 503"); + }); + + it("parses a host profile from config without requiring an observation path", async () => { + const fixture = await createFixtureProject("observation-source-host-transport", [ + { + relativePath: ".quality/config/observation-sources.yaml", + contents: `profiles: + - id: "shiplight-e2e" + name: "Shiplight e2e runs" + description: "Read YAML e2e results already reported to the platform." + transport: "host" + host: + provider: "shiplight" + options: + repo: "ShiplightAI/shipyard" +` + } + ]); + + try { + const batch = parseObservationSourceProfiles([ + { + projectRelativePath: ".quality/config/observation-sources.yaml", + resolvedLocalPath: path.join(fixture.root, ".quality/config/observation-sources.yaml"), + sourcePattern: ".quality/config/observation-sources.yaml" + } + ]); + + expect(batch.primary?.status).toBe("parsed"); + expect(batch.primary?.document?.profiles[0]).toEqual( + expect.objectContaining({ + id: "shiplight-e2e", + transport: "host", + observationPath: undefined, + host: { provider: "shiplight", options: { repo: "ShiplightAI/shipyard" } } + }) + ); + } finally { + await fixture.cleanup(); + } + }); + + it("rejects a host profile with no provider", async () => { + const fixture = await createFixtureProject("observation-source-host-no-provider", [ + { + relativePath: ".quality/config/observation-sources.yaml", + contents: `profiles: + - id: "shiplight-e2e" + name: "Shiplight e2e runs" + transport: "host" + host: + options: + repo: "ShiplightAI/shipyard" +` + } + ]); + + try { + const batch = parseObservationSourceProfiles([ + { + projectRelativePath: ".quality/config/observation-sources.yaml", + resolvedLocalPath: path.join(fixture.root, ".quality/config/observation-sources.yaml"), + sourcePattern: ".quality/config/observation-sources.yaml" + } + ]); + + expect(batch.primary?.document?.profiles ?? []).toEqual([]); + expect(batch.primary?.diagnostics[0]?.message).toContain("host block with a provider"); + } finally { + await fixture.cleanup(); + } + }); + + it("still requires an observation path for the file-based transports", async () => { + const fixture = await createFixtureProject("observation-source-missing-path", [ + { + relativePath: ".quality/config/observation-sources.yaml", + contents: `profiles: + - id: "local-review" + name: "Local review" + transport: "local-folder" + local_folder: + path: "artifacts/quality" +` + } + ]); + + try { + const batch = parseObservationSourceProfiles([ + { + projectRelativePath: ".quality/config/observation-sources.yaml", + resolvedLocalPath: path.join(fixture.root, ".quality/config/observation-sources.yaml"), + sourcePattern: ".quality/config/observation-sources.yaml" + } + ]); + + expect(batch.primary?.document?.profiles ?? []).toEqual([]); + expect(batch.primary?.diagnostics[0]?.message).toContain("observation_path"); + } finally { + await fixture.cleanup(); + } + }); +}); + +describe("run evidence through resolution and evaluation", () => { + const agentPayloadRef = "https://app.shiplight.ai/runs/8412?test=99231"; + + function resolvedFixture() { + const ingested = ingestObservationManifest({ + report_json: JSON.stringify({ + schema_version: 1, + revision: { commit: "abc123" }, + observed_at: "2026-08-27T10:00:00Z", + observations: [ + { + path: ".github/workflows/release-ci-runner.yml", + test_case: "agent_payload", + status: "pass", + artifacts: [{ ref: agentPayloadRef, label: "Shiplight run 8412" }] + }, + { + path: ".github/workflows/release-ci-runner.yml", + test_case: "image_inspect", + status: "pass" + } + ] + }) + }); + + return { scan: checksScanResult(), resolution: resolveObservations(checksScanResult(), ingested) }; + } + + it("carries refs onto the audit row for the observation that declared them", () => { + const { resolution } = resolvedFixture(); + const matched = resolution.auditRows.filter((row) => row.matchStatus === "matched"); + + expect(matched.length).toBeGreaterThan(0); + const withRefs = matched.filter((row) => row.evidenceRefs.length > 0); + expect(withRefs.map((row) => row.testCase)).toEqual(["agent_payload"]); + expect(withRefs[0]?.evidenceRefs).toEqual([{ ref: agentPayloadRef, label: "Shiplight run 8412" }]); + }); + + it("leaves the audit row of an observation with no refs carrying an empty list", () => { + const { resolution } = resolvedFixture(); + const imageInspect = resolution.auditRows.find((row) => row.testCase === "image_inspect"); + + expect(imageInspect?.evidenceRefs).toEqual([]); + }); + + it("exposes the selected observation's refs on the evaluated check", () => { + const { scan, resolution } = resolvedFixture(); + const evaluation = buildTargetEvaluation({ + result: scan, + targetId: "checks/quality-map.yaml#target:ci-runner", + observations: resolution, + selection: { commit: "abc123" } + }); + + const evidence = evaluation.expectations.flatMap((expectation) => expectation.evidence); + const agentPayload = evidence.find((entry) => entry.evidenceLocalId === "ci-runner-agent-payload"); + const imageInspect = evidence.find((entry) => entry.evidenceLocalId === "ci-runner-image-inspect"); + + expect(agentPayload?.evidenceRefs).toEqual([{ ref: agentPayloadRef, label: "Shiplight run 8412" }]); + expect(imageInspect?.evidenceRefs).toEqual([]); + }); + + it("reports no refs for a check nothing observed", () => { + // An unobserved check must not borrow evidence from a check that did run. + const { scan, resolution } = resolvedFixture(); + const evaluation = buildTargetEvaluation({ + result: scan, + targetId: "checks/quality-map.yaml#target:ci-runner", + observations: resolution, + selection: { commit: "abc123" } + }); + + const unobserved = evaluation.expectations + .flatMap((expectation) => expectation.evidence) + .filter((entry) => entry.state === "unobserved"); + + expect(unobserved.length).toBeGreaterThan(0); + expect(unobserved.every((entry) => entry.evidenceRefs.length === 0)).toBe(true); + }); +}); + +// A Shiplight YAML run transpiles to Playwright and reports the GENERATED spec. +// That file is gitignored and absent from a fresh checkout, so a quality map +// cannot honestly pin it — the adapter keys observations on the `.test.yaml` +// source instead, inverting the transpiler's own naming rule. +describe("shiplight report adapter", () => { + function report(tests: readonly Record[]): string { + return JSON.stringify({ timestamp: "2026-08-28T10:00:00.000Z", tests }); + } + + it("keys an observation on the YAML source, not the transpiled spec", () => { + const built = buildShiplightObservationBatch({ + report_json: report([ + { + file: "tests/authed/home-analytics-hover-nav.yaml.spec.ts", + baseTitle: "Home cards and charts expose analytics hover navigation", + title: "@e2e @home Home cards and charts expose analytics hover navigation", + status: "passed", + endTime: "2026-08-28T09:59:00.000Z" + } + ]) + }); + + expect(built.diagnostics).toEqual([]); + expect(built.batch?.observations?.[0]).toEqual( + expect.objectContaining({ + test_file: "tests/authed/home-analytics-hover-nav.test.yaml", + // The bare YAML test name, not the tag-prefixed title: a map is authored + // against the YAML, and tags are prefixed rather than joined by the + // suite separator the resolver folds on, so the tagged form would miss. + test_case: "Home cards and charts expose analytics hover navigation", + status: "pass" + }) + ); + }); + + it("leaves a path that is not a transpiled spec untouched", () => { + const built = buildShiplightObservationBatch({ + report_json: report([ + { file: "tests/authed/auth.setup.ts", title: "signup new authed user", status: "passed" } + ]) + }); + + expect(built.batch?.observations?.[0]?.test_file).toBe("tests/authed/auth.setup.ts"); + }); + + it("carries the run evidence ref onto every observation", () => { + const built = buildShiplightObservationBatch({ + report_json: report([ + { file: "a.yaml.spec.ts", baseTitle: "a", status: "passed" }, + { file: "b.yaml.spec.ts", baseTitle: "b", status: "failed" } + ]), + evidence_refs: [{ ref: "https://app.shiplight.ai/runs/8412", label: "Shiplight run 8412" }] + }); + + expect(built.batch?.observations?.map((o) => o.status)).toEqual(["pass", "fail"]); + for (const observation of built.batch?.observations ?? []) { + expect(observation.evidence_refs).toEqual([ + { ref: "https://app.shiplight.ai/runs/8412", label: "Shiplight run 8412" } + ]); + } + }); + + it("skips a test with no usable file or status without losing the rest", () => { + const built = buildShiplightObservationBatch({ + report_json: report([ + { file: "a.yaml.spec.ts", baseTitle: "a", status: "nonsense" }, + { file: "b.yaml.spec.ts", baseTitle: "b", status: "passed" } + ]) + }); + + expect(built.batch?.observations).toHaveLength(1); + expect(built.diagnostics.map((entry) => entry.severity)).toEqual(["warning"]); + }); + + it("rejects a report with no tests array", () => { + const built = buildShiplightObservationBatch({ report_json: JSON.stringify({ timestamp: "x" }) }); + + expect(built.batch).toBeUndefined(); + expect(built.diagnostics[0]?.severity).toBe("error"); + }); +}); diff --git a/tests/integration/local-reports-run-evidence.test.ts b/tests/integration/local-reports-run-evidence.test.ts new file mode 100644 index 0000000..1e5ab21 --- /dev/null +++ b/tests/integration/local-reports-run-evidence.test.ts @@ -0,0 +1,448 @@ +import path from "node:path"; +import { describe, expect, it } from "vitest"; +import { + createLocalReportsTransport, + executeObservationSourceProfile, + LOCAL_REPORTS_PROVIDER, + parseObservationSourceProfiles, + resolveObservations, + scanProject, + type ObservationSourceProfile +} from "@shiplightai/quality-core"; +import { createFixtureProject } from "../fixtures/quality-projects/build-fixtures"; + +// End-to-end validation of the host transport seam using the bundled +// local-reports provider: a real Playwright report on disk, read through a +// registered host transport, resolved against a quality map, arriving as run +// evidence on the matched check. Nothing here is platform-specific — this is +// the whole run-evidence path working with no service involved. +// +// The evidence is the report a reviewer opens, not a catalogue of the videos +// and screenshots inside it. Quality indexes checks to evidence; the runner's +// report is already the viewer. + +const hostTransports = { [LOCAL_REPORTS_PROVIDER]: createLocalReportsTransport() }; + +const QUALITY_MAP = `target: + id: "checkout" + name: "Checkout" + scope: "feature" +expectations: + - id: "guest-checkout" + title: "A guest can complete a purchase" + source_type: "SOURCE" + category: "functional" + priority: "P0" + evidence: + - id: "guest-pays" + type: "e2e" + path: "tests/checkout.spec.ts" + test_case: "guest can pay" + command: "npx playwright test" + contexts: + - "ci" +`; + +const OBSERVATION_SOURCES = `profiles: + - id: "local-playwright" + name: "Local Playwright run" + transport: "host" + host: + provider: "local-reports" + options: + path: "playwright-report/report.json" + report: "playwright-report/index.html" +`; + +function playwrightReport(): string { + return JSON.stringify({ + config: { version: "1.60.0" }, + suites: [ + { + title: "checkout.spec.ts", + file: "tests/checkout.spec.ts", + specs: [ + { + title: "guest can pay", + file: "tests/checkout.spec.ts", + tests: [ + { + projectName: "chromium", + results: [ + { + status: "passed", + duration: 4200, + startTime: "2026-08-27T10:00:00.000Z" + } + ] + } + ] + } + ] + } + ], + stats: { startTime: "2026-08-27T10:00:00.000Z", duration: 4200 } + }); +} + +async function fixture(reportJson: string | undefined) { + return createFixtureProject("local-reports-run-evidence", [ + { relativePath: ".quality/evidence/checkout/quality-map.yaml", contents: QUALITY_MAP }, + { relativePath: ".quality/config/observation-sources.yaml", contents: OBSERVATION_SOURCES }, + ...(reportJson === undefined + ? [] + : [{ relativePath: "playwright-report/report.json", contents: reportJson }]) + ]); +} + +function profileFrom(root: string): ObservationSourceProfile { + const batch = parseObservationSourceProfiles([ + { + projectRelativePath: ".quality/config/observation-sources.yaml", + resolvedLocalPath: path.join(root, ".quality/config/observation-sources.yaml"), + sourcePattern: ".quality/config/observation-sources.yaml" + } + ]); + const profile = batch.primary?.document?.profiles[0]; + if (profile === undefined) { + throw new Error(`fixture profile did not parse: ${JSON.stringify(batch.primary?.diagnostics)}`); + } + return profile; +} + +describe("local-reports host transport", () => { + it("turns a Playwright report on disk into run evidence on the matched check", async () => { + const project = await fixture(playwrightReport()); + + try { + const execution = await executeObservationSourceProfile({ + profile: profileFrom(project.root), + projectRoot: project.root, + hostTransports + }); + + expect(execution.observations).toHaveLength(1); + expect(execution.observations[0]?.status).toBe("pass"); + expect(execution.observations[0]?.evidenceRefs).toEqual([ + { ref: "playwright-report/index.html", label: "Test report" } + ]); + + const scan = await scanProject({ projectPath: project.root, mode: "scan" }); + const resolution = resolveObservations(scan, execution); + const matched = resolution.auditRows.filter((row) => row.matchStatus === "matched"); + + expect(matched).toHaveLength(1); + expect(matched[0]?.evidenceLocalId).toBe("guest-pays"); + expect(matched[0]?.evidenceRefs.map((entry) => entry.ref)).toEqual([ + "playwright-report/index.html" + ]); + } finally { + await project.cleanup(); + } + }); + + it("produces observations with no refs when no report is configured", async () => { + // The results still count. Only the link to look at them is missing. + const project = await createFixtureProject("local-reports-no-report", [ + { relativePath: ".quality/evidence/checkout/quality-map.yaml", contents: QUALITY_MAP }, + { + relativePath: ".quality/config/observation-sources.yaml", + contents: `profiles: + - id: "local-playwright" + name: "Local Playwright run" + transport: "host" + host: + provider: "local-reports" + options: + path: "playwright-report/report.json" +` + }, + { relativePath: "playwright-report/report.json", contents: playwrightReport() } + ]); + + try { + const execution = await executeObservationSourceProfile({ + profile: profileFrom(project.root), + projectRoot: project.root, + hostTransports + }); + + expect(execution.observations).toHaveLength(1); + expect(execution.observations[0]?.status).toBe("pass"); + expect(execution.observations[0]?.evidenceRefs).toEqual([]); + } finally { + await project.cleanup(); + } + }); + + it("records an absolute report URL as given", async () => { + const project = await createFixtureProject("local-reports-url", [ + { relativePath: ".quality/evidence/checkout/quality-map.yaml", contents: QUALITY_MAP }, + { + relativePath: ".quality/config/observation-sources.yaml", + contents: `profiles: + - id: "local-playwright" + name: "Local Playwright run" + transport: "host" + host: + provider: "local-reports" + options: + path: "playwright-report/report.json" + report: "https://ci.example.test/runs/42/index.html" +` + }, + { relativePath: "playwright-report/report.json", contents: playwrightReport() } + ]); + + try { + const execution = await executeObservationSourceProfile({ + profile: profileFrom(project.root), + projectRoot: project.root, + hostTransports + }); + + expect(execution.observations[0]?.evidenceRefs).toEqual([ + { ref: "https://ci.example.test/runs/42/index.html", label: "Test report" } + ]); + } finally { + await project.cleanup(); + } + }); + + it("refuses a report pointer that escapes the project root", async () => { + const project = await createFixtureProject("local-reports-report-escape", [ + { relativePath: ".quality/evidence/checkout/quality-map.yaml", contents: QUALITY_MAP }, + { + relativePath: ".quality/config/observation-sources.yaml", + contents: `profiles: + - id: "local-playwright" + name: "Local Playwright run" + transport: "host" + host: + provider: "local-reports" + options: + path: "playwright-report/report.json" + report: "../../etc/passwd" +` + }, + { relativePath: "playwright-report/report.json", contents: playwrightReport() } + ]); + + try { + const execution = await executeObservationSourceProfile({ + profile: profileFrom(project.root), + projectRoot: project.root, + hostTransports + }); + + expect(execution.observations[0]?.evidenceRefs).toEqual([]); + expect( + execution.diagnostics.some( + (entry) => entry.severity === "error" && entry.message.includes("outside the project root") + ) + ).toBe(true); + } finally { + await project.cleanup(); + } + }); + + it("says the report records no commit rather than stamping one onto it", async () => { + // Stamping the working tree's HEAD onto a report that may predate it would + // invent provenance the producer never claimed. + const project = await fixture(playwrightReport()); + + try { + const execution = await executeObservationSourceProfile({ + profile: profileFrom(project.root), + projectRoot: project.root, + hostTransports + }); + + expect(execution.observations[0]?.revision.commit).toBeUndefined(); + const unpinned = execution.diagnostics.find((entry) => entry.message.includes("records no commit")); + expect(unpinned?.severity).toBe("info"); + } finally { + await project.cleanup(); + } + }); + + it("pins the commit when config supplies one", async () => { + const project = await createFixtureProject("local-reports-pinned", [ + { relativePath: ".quality/evidence/checkout/quality-map.yaml", contents: QUALITY_MAP }, + { + relativePath: ".quality/config/observation-sources.yaml", + contents: `${OBSERVATION_SOURCES} commit: "abc123"\n` + }, + { relativePath: "playwright-report/report.json", contents: playwrightReport() } + ]); + + try { + const execution = await executeObservationSourceProfile({ + profile: profileFrom(project.root), + projectRoot: project.root, + hostTransports + }); + + expect(execution.observations[0]?.revision.commit).toBe("abc123"); + expect(execution.diagnostics).toEqual([]); + } finally { + await project.cleanup(); + } + }); + + it("warns rather than errors when the suite has not been run yet", async () => { + // No report on disk is an ordinary state locally. Every check it would have + // proven already reads unobserved; a red error adds nothing. + const project = await fixture(undefined); + + try { + const execution = await executeObservationSourceProfile({ + profile: profileFrom(project.root), + projectRoot: project.root, + hostTransports + }); + + expect(execution.observations).toEqual([]); + expect(execution.diagnostics[0]?.severity).toBe("warning"); + expect(execution.diagnostics[0]?.code).toBe("MISSING_OBSERVATION_ARTIFACT_MATCH"); + } finally { + await project.cleanup(); + } + }); + + it("refuses a report path that escapes the project root", async () => { + const project = await createFixtureProject("local-reports-escape", [ + { relativePath: ".quality/evidence/checkout/quality-map.yaml", contents: QUALITY_MAP }, + { + relativePath: ".quality/config/observation-sources.yaml", + contents: `profiles: + - id: "local-playwright" + name: "Local Playwright run" + transport: "host" + host: + provider: "local-reports" + options: + path: "../../etc/passwd" +` + } + ]); + + try { + const execution = await executeObservationSourceProfile({ + profile: profileFrom(project.root), + projectRoot: project.root, + hostTransports + }); + + expect(execution.observations).toEqual([]); + expect(execution.diagnostics[0]?.severity).toBe("error"); + expect(execution.diagnostics[0]?.message).toContain("outside the project root"); + } finally { + await project.cleanup(); + } + }); + + it("refuses an absolute report path outside the project root", async () => { + // Containment has to cover the absolute form too. Letting `/etc/hosts` + // through while refusing `../../etc/hosts` would enforce nothing: a profile + // is repo config, and a PR can write either spelling. + const project = await createFixtureProject("local-reports-absolute-escape", [ + { relativePath: ".quality/evidence/checkout/quality-map.yaml", contents: QUALITY_MAP }, + { + relativePath: ".quality/config/observation-sources.yaml", + contents: `profiles: + - id: "local-playwright" + name: "Local Playwright run" + transport: "host" + host: + provider: "local-reports" + options: + path: "/etc/hosts" +` + } + ]); + + try { + const execution = await executeObservationSourceProfile({ + profile: profileFrom(project.root), + projectRoot: project.root, + hostTransports + }); + + expect(execution.observations).toEqual([]); + expect(execution.diagnostics[0]?.severity).toBe("error"); + expect(execution.diagnostics[0]?.message).toContain("outside the project root"); + } finally { + await project.cleanup(); + } + }); + + it("refuses an absolute report pointer outside the project root", async () => { + const project = await createFixtureProject("local-reports-absolute-report", [ + { relativePath: ".quality/evidence/checkout/quality-map.yaml", contents: QUALITY_MAP }, + { + relativePath: ".quality/config/observation-sources.yaml", + contents: `profiles: + - id: "local-playwright" + name: "Local Playwright run" + transport: "host" + host: + provider: "local-reports" + options: + path: "playwright-report/report.json" + report: "/etc/hosts" +` + }, + { relativePath: "playwright-report/report.json", contents: playwrightReport() } + ]); + + try { + const execution = await executeObservationSourceProfile({ + profile: profileFrom(project.root), + projectRoot: project.root, + hostTransports + }); + + expect(execution.observations[0]?.evidenceRefs).toEqual([]); + expect( + execution.diagnostics.some( + (entry) => entry.severity === "error" && entry.message.includes("outside the project root") + ) + ).toBe(true); + } finally { + await project.cleanup(); + } + }); + + it("rejects an unsupported report format by name", async () => { + const project = await createFixtureProject("local-reports-format", [ + { relativePath: ".quality/evidence/checkout/quality-map.yaml", contents: QUALITY_MAP }, + { + relativePath: ".quality/config/observation-sources.yaml", + contents: `profiles: + - id: "local-playwright" + name: "Local Playwright run" + transport: "host" + host: + provider: "local-reports" + options: + path: "reports/junit.xml" + format: "junit-xml" +` + } + ]); + + try { + const execution = await executeObservationSourceProfile({ + profile: profileFrom(project.root), + projectRoot: project.root, + hostTransports + }); + + expect(execution.diagnostics[0]?.message).toContain("junit-xml"); + expect(execution.diagnostics[0]?.message).toContain("playwright-json"); + } finally { + await project.cleanup(); + } + }); +}); From cba93a87a1a558c1c0436827faea0d027167dbdc Mon Sep 17 00:00:00 2001 From: Feng Qian Date: Fri, 4 Sep 2026 11:21:12 -0700 Subject: [PATCH 2/7] fix(quality): close symlink escape in local-reports and de-duplicate ref linking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the review on #15. The evidence-file route already compared real paths, but the local-reports transport checked containment lexically. `path.resolve` never follows links, so a symlink committed inside a project — `playwright-report` pointing at a home directory — was textually contained and read straight through. Both checks now run: the lexical one always, including when the target does not exist, because treating a failed `realpathSync` as "nothing to contain" would wave through every escape that merely points at no file. The tests caught exactly that regression on the first attempt. Also from the review: - The ref-to-href rule lived in both the feature page and the audit panel. Two copies of a rule diverge, and these two would have started disagreeing about which refs are links. - The saved-view id sanitizer was not injective: `my view` and `my-view` both reduced to `my-view`, so two views wrote the same recommendation export and one silently overwrote the other. Confirmed by running it. Pre-existing rather than introduced here, but the review asked for confirmation and a confirmed silent-overwrite path is worth closing. A short digest of the original is appended only when sanitizing changed the id, so existing filenames are untouched. - The new schema subcommands now carry the same version note the run evidence field does; the published 0.3.2 answers `Unknown command`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NhxpEed9pbcjT7MLtnDbiH --- .../quality/references/improve/index.md | 4 ++ .../src/observation-sources/local-reports.ts | 47 +++++++++++--- .../core/src/recommendation-export/index.ts | 14 ++++- packages/ui/src/components/FeaturePage.tsx | 23 +------ .../src/components/ObservationAuditPanel.tsx | 56 +---------------- packages/ui/src/lib/evidence-ref.ts | 63 +++++++++++++++++++ .../local-reports-run-evidence.test.ts | 42 +++++++++++++ 7 files changed, 165 insertions(+), 84 deletions(-) create mode 100644 packages/ui/src/lib/evidence-ref.ts diff --git a/agent-skills/quality/references/improve/index.md b/agent-skills/quality/references/improve/index.md index 0ee03ed..28c57da 100644 --- a/agent-skills/quality/references/improve/index.md +++ b/agent-skills/quality/references/improve/index.md @@ -219,6 +219,10 @@ contract and drifts the moment it moves: - observation sets: `quality-tools sets schema` - saved views: `quality-tools views schema` +The three config-schema commands require a `quality-tools` newer than 0.3.2; +the published version answers `Unknown command`. Until it ships, read the +schemas from the engine source rather than vendoring a copy. + Use `quality-observations.template.json` as the canonical output example. ### Sources diff --git a/packages/core/src/observation-sources/local-reports.ts b/packages/core/src/observation-sources/local-reports.ts index 7c80241..2692713 100644 --- a/packages/core/src/observation-sources/local-reports.ts +++ b/packages/core/src/observation-sources/local-reports.ts @@ -1,3 +1,4 @@ +import { realpathSync } from "node:fs"; import { readFile } from "node:fs/promises"; import path from "node:path"; import { createDiagnostic } from "../diagnostics/diagnostic"; @@ -90,17 +91,47 @@ function resolveOption( // through while refusing `../../etc/hosts` would enforce nothing, since a PR // can write either. `path.resolve` returns an absolute declaration unchanged, // so both forms reach the same comparison. + // + // Compared on REAL paths, for the same reason the evidence-file route does: + // `path.resolve` is lexical and never follows links, so a symlink committed + // inside the project — `playwright-report` pointing at a home directory — + // passes a purely textual check and is then read straight through. Both sides + // are realpath'd so a symlinked checkout still resolves against its own real + // location rather than being refused. + const escaped = (root: string, target: string): boolean => { + const relative = path.relative(root, target); + return relative.startsWith("..") || path.isAbsolute(relative); + }; + const outside = (): { readonly diagnostic: ScanDiagnostic } => ({ + diagnostic: invalid( + `Observation source profile ${profile.id} resolves host.options.${option} ${declared} outside the project root.` + ) + }); + + // The lexical check runs ALWAYS, including when the target does not exist — + // `realpathSync` throws on a missing path, and treating that as "nothing to + // contain" would wave through every escape that merely points at no file. const resolved = path.resolve(projectRoot, declared); - const relative = path.relative(projectRoot, resolved); - if (relative.startsWith("..") || path.isAbsolute(relative)) { - return { - diagnostic: invalid( - `Observation source profile ${profile.id} resolves host.options.${option} ${declared} outside the project root.` - ) - }; + if (escaped(projectRoot, resolved)) { + return outside(); } - return { resolved }; + // Then the real-path check, for the case the lexical one structurally cannot + // see: a symlink committed inside the project — `playwright-report` pointing + // at a home directory — is textually contained and still reads through. Both + // sides are realpath'd so a symlinked checkout resolves against its own real + // location instead of being refused. Skipped when the target does not exist, + // which the lexical check above has already vouched for. + try { + const realRoot = realpathSync(projectRoot); + const realResolved = realpathSync(resolved); + if (escaped(realRoot, realResolved)) { + return outside(); + } + return { resolved: realResolved }; + } catch { + return { resolved }; + } } // The one evidence pointer this transport produces: the report a reviewer diff --git a/packages/core/src/recommendation-export/index.ts b/packages/core/src/recommendation-export/index.ts index 0ba33d6..e345131 100644 --- a/packages/core/src/recommendation-export/index.ts +++ b/packages/core/src/recommendation-export/index.ts @@ -1,3 +1,4 @@ +import { createHash } from "node:crypto"; import { existsSync, statSync } from "node:fs"; import path, { resolve } from "node:path"; import { applySavedQcView } from "../views"; @@ -189,8 +190,19 @@ function hasUsableRuntimeProofStatus(input: { return input.executionStatus !== "invalid" && input.resolutionStatus !== "invalid" && input.observationCount > 0; } +// Sanitizing alone is not injective: `my view` and `my-view` both reduce to +// `my-view`, so two distinct ids would write the same export and one would +// silently overwrite the other. When sanitizing actually changed the id, a short +// digest of the ORIGINAL is appended, which keeps the readable name and makes +// the segment unique. Ids that need no sanitizing keep their exact filename, so +// existing exports are unaffected. function sanitizeFileSegment(value: string): string { - return value.replace(/[^a-zA-Z0-9._-]+/g, "-"); + const sanitized = value.replace(/[^a-zA-Z0-9._-]+/g, "-"); + if (sanitized === value) { + return sanitized; + } + + return `${sanitized}-${createHash("sha256").update(value).digest("hex").slice(0, 8)}`; } // A run without an observation set writes under the reserved "static" prefix, so diff --git a/packages/ui/src/components/FeaturePage.tsx b/packages/ui/src/components/FeaturePage.tsx index dba28f4..82926c3 100644 --- a/packages/ui/src/components/FeaturePage.tsx +++ b/packages/ui/src/components/FeaturePage.tsx @@ -2,6 +2,7 @@ import { useQcApi, useQcHost, useQcRoute } from "../host"; import { useQcScanCache } from "./scan-cache"; +import { evidenceRefHref } from "../lib/evidence-ref"; import { Breadcrumb } from "./Breadcrumb"; import { MarkdownOverlay } from "./MarkdownOverlay"; @@ -129,28 +130,6 @@ function checkEvidence(graph: QualityGraph, expectation: QualityCheck): QualityG return graph.evidence.filter((entry) => expectation.linkedEvidenceIds.includes(entry.normalizedId)); } -// Same rule the audit panel applies: an absolute url is linked as it stands, a -// project path is served by the host when it can, and anything else stays text -// because resolving it needs a host that can and guessing would invent a -// destination. -function evidenceRefHref( - ref: string, - qcApi: (path: string) => string, - servesEvidenceFiles: boolean -): string | undefined { - if (/^https?:\/\//i.test(ref)) { - return ref; - } - if (!servesEvidenceFiles) { - return undefined; - } - const segments = ref - .split("/") - .filter((segment) => segment.length > 0) - .map((segment) => encodeURIComponent(segment)); - return segments.length === 0 ? undefined : qcApi(`/evidence-file/${segments.join("/")}`); -} - function observedStateColor(state: string): string { switch (state) { case "pass": diff --git a/packages/ui/src/components/ObservationAuditPanel.tsx b/packages/ui/src/components/ObservationAuditPanel.tsx index 72acfaa..61319c4 100644 --- a/packages/ui/src/components/ObservationAuditPanel.tsx +++ b/packages/ui/src/components/ObservationAuditPanel.tsx @@ -4,6 +4,7 @@ import { ExternalLink, X } from "lucide-react"; import { ActionIcon, Anchor, Badge, Code, Group, Paper, Stack, Text, Title } from "@mantine/core"; import { useMemo, useState } from "react"; import { useQcApi, useQcHost } from "../host"; +import { evidenceRefDestination, evidenceRefHref } from "../lib/evidence-ref"; import type { NormalizedEvidenceRef, ObservationResolutionAuditRow } from "@shiplightai/quality-core"; type MatchFilter = "all" | ObservationResolutionAuditRow["matchStatus"]; @@ -13,57 +14,6 @@ interface ObservationAuditPanelProps { onClose(): void; } -function isAbsoluteUrl(ref: string): boolean { - return /^https?:\/\//i.test(ref); -} - -// The single place the UI looks at a ref at all, and it looks only at whether -// the producer already gave us something a browser can open, or whether the -// host can turn a project path into something it can. -// -// A path is passed through as PATH SEGMENTS rather than a query parameter. The -// reports these refs point at fetch their own assets with relative urls, so the -// served page has to sit at the same shape of address as the folder it came -// from or its video and trace links resolve to nothing. -// -// Anything else stays text: resolving it needs a host that can, and guessing a -// URL for it would invent a destination. -function evidenceHref( - ref: string, - qcApi: (path: string) => string, - servesEvidenceFiles: boolean -): string | undefined { - if (isAbsoluteUrl(ref)) { - return ref; - } - - if (!servesEvidenceFiles) { - return undefined; - } - - const segments = ref - .split("/") - .filter((segment) => segment.length > 0) - .map((segment) => encodeURIComponent(segment)); - return segments.length === 0 ? undefined : qcApi(`/evidence-file/${segments.join("/")}`); -} - -// Refs are written by evidence producers, so the destination is shown rather -// than hidden behind a label the producer also chose. A project-relative ref -// shows its path instead: the host it would resolve to is this application, -// which tells the reader nothing. -function evidenceDestination(ref: string): string { - if (!isAbsoluteUrl(ref)) { - return ref; - } - - try { - return new URL(ref).host; - } catch { - return ref; - } -} - function evidenceLabel(entry: NormalizedEvidenceRef): string { return entry.label ?? "Run evidence"; } @@ -191,8 +141,8 @@ export function ObservationAuditPanel({ Run evidence {row.evidenceRefs.map((entry) => { - const href = evidenceHref(entry.ref, qcApi, servesEvidenceFiles); - const destination = evidenceDestination(entry.ref); + const href = evidenceRefHref(entry.ref, qcApi, servesEvidenceFiles); + const destination = evidenceRefDestination(entry.ref); return href === undefined ? ( string, + servesEvidenceFiles: boolean +): string | undefined { + if (isAbsoluteEvidenceUrl(ref)) { + return ref; + } + + if (!servesEvidenceFiles) { + return undefined; + } + + const segments = ref + .split("/") + .filter((segment) => segment.length > 0) + .map((segment) => encodeURIComponent(segment)); + return segments.length === 0 ? undefined : qcApi(`/evidence-file/${segments.join("/")}`); +} + +/** + * What to show beside the link. Refs are written by evidence producers, so the + * destination is shown rather than hidden behind a label the producer also + * chose. A project-relative ref shows its path instead: the host it resolves to + * is this application, which tells the reader nothing. + */ +export function evidenceRefDestination(ref: string): string { + if (!isAbsoluteEvidenceUrl(ref)) { + return ref; + } + + try { + return new URL(ref).host; + } catch { + return ref; + } +} diff --git a/tests/integration/local-reports-run-evidence.test.ts b/tests/integration/local-reports-run-evidence.test.ts index 1e5ab21..6b17fc5 100644 --- a/tests/integration/local-reports-run-evidence.test.ts +++ b/tests/integration/local-reports-run-evidence.test.ts @@ -1,3 +1,5 @@ +import { mkdtempSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; import path from "node:path"; import { describe, expect, it } from "vitest"; import { @@ -414,6 +416,46 @@ describe("local-reports host transport", () => { } }); + it("refuses a symlink inside the project that resolves outside it", async () => { + // The lexical check structurally cannot see this: `path.resolve` never + // follows links, so a committed symlink is textually contained and would be + // read straight through. Same containment model as the evidence-file route. + const outside = mkdtempSync(path.join(tmpdir(), "local-reports-outside-")); + writeFileSync(path.join(outside, "report.json"), playwrightReport(), "utf8"); + + const project = await createFixtureProject("local-reports-symlink-escape", [ + { relativePath: ".quality/evidence/checkout/quality-map.yaml", contents: QUALITY_MAP }, + { + relativePath: ".quality/config/observation-sources.yaml", + contents: `profiles: + - id: "local-playwright" + name: "Local Playwright run" + transport: "host" + host: + provider: "local-reports" + options: + path: "escape/report.json" +` + } + ]); + symlinkSync(outside, path.join(project.root, "escape")); + + try { + const execution = await executeObservationSourceProfile({ + profile: profileFrom(project.root), + projectRoot: project.root, + hostTransports + }); + + expect(execution.observations).toEqual([]); + expect(execution.diagnostics[0]?.severity).toBe("error"); + expect(execution.diagnostics[0]?.message).toContain("outside the project root"); + } finally { + await project.cleanup(); + rmSync(outside, { force: true, recursive: true }); + } + }); + it("rejects an unsupported report format by name", async () => { const project = await createFixtureProject("local-reports-format", [ { relativePath: ".quality/evidence/checkout/quality-map.yaml", contents: QUALITY_MAP }, From 45d8135ed223f976422aae3060dc48459b466b78 Mon Sep 17 00:00:00 2001 From: Feng Qian Date: Fri, 4 Sep 2026 11:35:26 -0700 Subject: [PATCH 3/7] fix(quality): keep the skill on the published surface and align adapter status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second review round on #15. The skill pins quality-tools @^0.3.0, and this branch documented CLI commands and a flag that no 0.3.x release has. Version bumps here are their own `chore: release` commit, so bumping to claim the surface would be the wrong fix. The skill now points at what exists: the manifest schema from `observations schema`, and configuration validated by the engine itself, whose `INVALID_*` diagnostics name the exact yamlPath. That is better guidance regardless — the parser is what actually runs. `ingestPlaywrightJsonReport` and `ingestObservationManifest` counted every diagnostic when deciding execution status, while the source-execution and resolution paths had been changed to exclude `info` notes. Status has to be a function of the inputs, not of which ingestion path ran, so `countProblems` moves to the shared helper and all of them use it. Reverted the saved-view filename change from the previous round. It closed a real collision — `my view` and `my-view` both reduce to `my-view`, so one view's recommendation export silently overwrites the other's — but it renames the file for every id that needs sanitizing, which breaks anything addressing an existing export by name. The collision is pre-existing and the trade belongs in its own change; the behaviour is now recorded where the sanitizer is. Also: explicit `frame-src`/`worker-src` rather than relying on the default-src fallback; the loopback-only assumption recorded at the evidence-file route, since `requireQcSession` is a no-op there and loopback is the only boundary; the dangling-symlink case explained where it is handled; and the template's `commit` example quoted, because YAML reads an unquoted sha as a number and the parser now rejects it. Not changed: the review asks the shiplight adapter to drop the array index from `observation_id` because the Playwright adapter is stable. It is not — that adapter builds its id the same way. Changing one would make them inconsistent; if the index is wrong it is wrong in both. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NhxpEed9pbcjT7MLtnDbiH --- .../quality/references/improve/index.md | 22 +++++++++---------- .../evidence-file/[...ref]/route.ts | 10 ++++++++- .../src/observation-sources/local-reports.ts | 5 +++++ .../core/src/observations/ingest-helpers.ts | 9 ++++++++ packages/core/src/observations/manifest.ts | 4 ++-- .../core/src/observations/playwright-json.ts | 4 ++-- .../core/src/recommendation-export/index.ts | 19 +++++----------- 7 files changed, 44 insertions(+), 29 deletions(-) diff --git a/agent-skills/quality/references/improve/index.md b/agent-skills/quality/references/improve/index.md index 28c57da..45e4152 100644 --- a/agent-skills/quality/references/improve/index.md +++ b/agent-skills/quality/references/improve/index.md @@ -180,7 +180,7 @@ acquisition and resolution have been ruled out. ``` -- Observation config: compare with the schemas the engine emits (see above), then +- Observation config: assess the project and read its `INVALID_*` diagnostics, then run the relevant assessment. Engine diagnostics verify acquisition and graph joins. - Implementation or verification-method changes: run their owning verification command before @@ -210,18 +210,18 @@ or requires a human decision. - `.quality/config/observation-sets.yaml` - `.quality/config/views.yaml` -Use the configuration templates under `assets/`. Obtain the current schemas -from the engine rather than a bundled copy, which cannot be checked against the -contract and drifts the moment it moves: +Use the configuration templates under `assets/`. Never vendor a copy of a +schema: a copy cannot be checked against the contract and drifts the moment the +contract moves. -- observation manifest: `quality-tools observations schema` -- observation sources: `quality-tools sources schema` -- observation sets: `quality-tools sets schema` -- saved views: `quality-tools views schema` +For the observation manifest, obtain the current schema from +`quality-tools observations schema`. -The three config-schema commands require a `quality-tools` newer than 0.3.2; -the published version answers `Unknown command`. Until it ships, read the -schemas from the engine source rather than vendoring a copy. +Configuration files — sources, sets, views — are validated by the engine itself +when you assess the project: an invalid profile, set, or view reports an +`INVALID_*` diagnostic naming the exact `yamlPath`. That is the authority, since +it is the parser that actually runs. Read the diagnostics rather than +pre-validating against a schema. Use `quality-observations.template.json` as the canonical output example. diff --git a/apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts b/apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts index dd44857..eb91c4b 100644 --- a/apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts +++ b/apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts @@ -19,7 +19,11 @@ export const runtime = "nodejs"; // // Trust boundary: this serves files from the local project the user themselves // opened, on a loopback-bound single-user dev server, and only the extensions -// below. It is not a general static file server — an unlisted extension is +// below. `requireQcSession` is a no-op in Quality Explorer, so LOOPBACK IS THE +// ONLY BOUNDARY: served behind `--hostname 0.0.0.0`, or port-forwarded out of a +// container, this becomes a read-the-project endpoint for anyone who can reach +// it. A host that binds anywhere else must put real authentication in front of +// this route. It is not a general static file server — an unlisted extension is // refused rather than guessed at, which is what keeps a checked-in `.command` // or `.sh` in a scanned repo from ever being handed to a browser. const CONTENT_TYPES: Readonly> = { @@ -86,6 +90,10 @@ const DOCUMENT_CSP = [ "media-src 'self' data: blob:", "font-src 'self' data:", "connect-src 'self'", + // Covered by default-src's fallback already; stated outright so the intent of + // the two directives that matter most here is not left to be inferred. + "frame-src 'none'", + "worker-src 'self'", "form-action 'none'", "frame-ancestors 'none'", "base-uri 'none'", diff --git a/packages/core/src/observation-sources/local-reports.ts b/packages/core/src/observation-sources/local-reports.ts index 2692713..acc3442 100644 --- a/packages/core/src/observation-sources/local-reports.ts +++ b/packages/core/src/observation-sources/local-reports.ts @@ -130,6 +130,11 @@ function resolveOption( } return { resolved: realResolved }; } catch { + // The target does not exist, so there is nothing to resolve and nothing to + // read. A dangling symlink whose text sits inside the root reaches here and + // keeps its lexically-checked path — safe, because the caller's `readFile` + // fails on it too, and reporting an unreadable file is a better message + // than a containment complaint about a path that resolves to nothing. return { resolved }; } } diff --git a/packages/core/src/observations/ingest-helpers.ts b/packages/core/src/observations/ingest-helpers.ts index fd469eb..f1cba2c 100644 --- a/packages/core/src/observations/ingest-helpers.ts +++ b/packages/core/src/observations/ingest-helpers.ts @@ -1,3 +1,4 @@ +import type { ScanDiagnostic } from "../diagnostics/diagnostic"; import type { ObservationArtifactInput, ObservationIngestionResult } from "./types"; // Shared helpers for the observation ingest adapters (junit, playwright-json, @@ -22,6 +23,14 @@ export function isoTimestamp(value: unknown): string | undefined { return new Date(parsed).toISOString(); } +// Counts only diagnostics that report something going wrong. An `info` note is +// context for the reader, not a degraded read, and the source-execution and +// resolution paths already exclude it — an adapter that counted it would make +// the status depend on which ingestion path ran rather than on the inputs. +export function countProblems(diagnostics: readonly ScanDiagnostic[]): number { + return diagnostics.filter((entry) => entry.severity !== "info").length; +} + export function statusFor( observationCount: number, diagnosticsCount: number diff --git a/packages/core/src/observations/manifest.ts b/packages/core/src/observations/manifest.ts index 5421f6f..14e2814 100644 --- a/packages/core/src/observations/manifest.ts +++ b/packages/core/src/observations/manifest.ts @@ -13,7 +13,7 @@ import type { QualityObservationManifestRevision, QualityObservationManifestRun } from "./types"; -import { isoTimestamp, normalizeArtifact, normalizePath, statusFor, stringValue } from "./ingest-helpers"; +import { countProblems, isoTimestamp, normalizeArtifact, normalizePath, statusFor, stringValue } from "./ingest-helpers"; import { normalizeObservationBatches } from "./normalize"; export const QUALITY_OBSERVATION_SCHEMA_VERSION = 1 as const; @@ -552,7 +552,7 @@ export function ingestObservationManifest(input: IngestObservationManifestInput) const diagnostics = [...normalized.diagnostics, ...parsed.diagnostics]; return { - status: statusFor(normalized.observations.length, diagnostics.length), + status: statusFor(normalized.observations.length, countProblems(diagnostics)), observations: normalized.observations, diagnostics }; diff --git a/packages/core/src/observations/playwright-json.ts b/packages/core/src/observations/playwright-json.ts index f1eff3e..d27df66 100644 --- a/packages/core/src/observations/playwright-json.ts +++ b/packages/core/src/observations/playwright-json.ts @@ -8,7 +8,7 @@ import type { ObservationRecordInput, ObservationRecordStatus } from "./types"; -import { isoTimestamp, normalizeArtifact, normalizePath, statusFor, stringValue } from "./ingest-helpers"; +import { countProblems, isoTimestamp, normalizeArtifact, normalizePath, statusFor, stringValue } from "./ingest-helpers"; import { normalizeObservationBatches } from "./normalize"; interface ParsedPlaywrightCase { @@ -231,7 +231,7 @@ export function ingestPlaywrightJsonReport( const mergedDiagnostics = [...normalized.diagnostics, ...built.diagnostics]; return { - status: statusFor(normalized.observations.length, mergedDiagnostics.length), + status: statusFor(normalized.observations.length, countProblems(mergedDiagnostics)), observations: normalized.observations, diagnostics: mergedDiagnostics }; diff --git a/packages/core/src/recommendation-export/index.ts b/packages/core/src/recommendation-export/index.ts index e345131..8a548f5 100644 --- a/packages/core/src/recommendation-export/index.ts +++ b/packages/core/src/recommendation-export/index.ts @@ -1,4 +1,3 @@ -import { createHash } from "node:crypto"; import { existsSync, statSync } from "node:fs"; import path, { resolve } from "node:path"; import { applySavedQcView } from "../views"; @@ -190,19 +189,13 @@ function hasUsableRuntimeProofStatus(input: { return input.executionStatus !== "invalid" && input.resolutionStatus !== "invalid" && input.observationCount > 0; } -// Sanitizing alone is not injective: `my view` and `my-view` both reduce to -// `my-view`, so two distinct ids would write the same export and one would -// silently overwrite the other. When sanitizing actually changed the id, a short -// digest of the ORIGINAL is appended, which keeps the readable name and makes -// the segment unique. Ids that need no sanitizing keep their exact filename, so -// existing exports are unaffected. +// KNOWN, PRE-EXISTING: this is not injective. `my view` and `my-view` both +// reduce to `my-view`, so two saved views with those ids write the same export +// and one silently overwrites the other. Making the segment unique renames the +// file for every id that needs sanitizing, which breaks anything addressing an +// existing export by name — a trade that belongs in its own change, not here. function sanitizeFileSegment(value: string): string { - const sanitized = value.replace(/[^a-zA-Z0-9._-]+/g, "-"); - if (sanitized === value) { - return sanitized; - } - - return `${sanitized}-${createHash("sha256").update(value).digest("hex").slice(0, 8)}`; + return value.replace(/[^a-zA-Z0-9._-]+/g, "-"); } // A run without an observation set writes under the reserved "static" prefix, so From cf0b3c1cf6346e3158ea377e724c6adc6f00efeb Mon Sep 17 00:00:00 2001 From: Feng Qian Date: Fri, 4 Sep 2026 11:45:55 -0700 Subject: [PATCH 4/7] docs(quality): record the accepted risks the review asked to make explicit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third review round on #15. Every remaining finding was about stating a trade rather than changing one, except two small consolidations. `.js` and `.css` stay in the evidence-file allowlist because Playwright's trace viewer ships as `trace/*.js` beside the report, and dropping them leaves the trace unopenable — the most useful evidence a run produces. The cost is now written where the allowlist is: any project `.js` is reachable as same-origin script, the document CSP permits `script-src 'self'`, and serving them as downloads or confining them to a report subdirectory would either break the viewer or need configuration the profile does not carry. The saved-view filename collision is now tracked as issue #16 and referenced from both the sanitizer and the views schema, so the deferral points somewhere instead of only being described. The skill no longer documents the `artifacts` field. It pins quality-tools @^0.3.0, whose validator rejects the field, so an agent following it today would write records that fail validation — the same reason the schema commands came out last round. `docs/` keeps the documentation, since it describes this repo rather than instructing an agent pinned to a published version. Also: the loopback-only boundary now appears in the Explorer how-to, not just at the route, so someone deploying behind a proxy meets it without reading source; `resolveObservations` uses the shared `countProblems` instead of repeating its filter; and the dangling-symlink fallback records that its ref is safe only because the serving route re-checks containment at request time. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NhxpEed9pbcjT7MLtnDbiH --- .../quality/references/improve/index.md | 31 ------------------- .../evidence-file/[...ref]/route.ts | 14 +++++++-- docs/how-to/inspect-in-the-browser.md | 13 ++++++++ .../src/observation-sources/local-reports.ts | 6 ++++ packages/core/src/observations/resolve.ts | 3 +- .../core/src/recommendation-export/index.ts | 2 +- packages/core/src/views/json-schema.ts | 4 ++- 7 files changed, 37 insertions(+), 36 deletions(-) diff --git a/agent-skills/quality/references/improve/index.md b/agent-skills/quality/references/improve/index.md index 45e4152..a1a5593 100644 --- a/agent-skills/quality/references/improve/index.md +++ b/agent-skills/quality/references/improve/index.md @@ -351,37 +351,6 @@ Follow this sequence. Do not ask the user to choose a parser or config shape. `GITHUB_RUN_ID`. Outside GitHub Actions, supply `--commit`; `--branch`, `--run-id`, `--run-url`, and `--observed-at` are optional. - **Run evidence** (requires a `quality-tools` newer than 0.3.2; the published - validator rejects the field until then). A record may carry pointers to what - the run left behind, so a reviewer opening a check can see what the test did: - - ```json - { - "path": "tests/e2e/checkout.test.yaml", - "test_case": "guest can pay", - "status": "pass", - "artifacts": [ - { "ref": "https://app.example.test/runs/8412?test=99231", - "label": "Run 8412" } - ] - } - ``` - - Not to be confused with a workflow's uploaded artifacts, which are how the - canonical file travels. These are pointers INSIDE it. - - - `ref` is opaque. Quality records and displays it; it never parses, - resolves, or validates it. An absolute `http(s)` ref is linked as it - stands; anything else is a path into the project, which only a reader that - has the project can open. - - Point at the report a person already knows how to read — the run page, the - HTML report — not at each video and screenshot. Quality is an index from - checks to evidence, not a viewer. - - Optional and additive. A record without it still counts exactly the same; - only the link to look at the result is missing. - - A malformed entry is reported and dropped without costing the record its - observed status. The status is the measurement; the ref is only how a - reviewer looks at it. 4. **Schema-validate before upload.** ```bash diff --git a/apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts b/apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts index eb91c4b..1f5d747 100644 --- a/apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts +++ b/apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts @@ -55,8 +55,18 @@ const CONTENT_TYPES: Readonly> = { ".ttf": "font/ttf", }; -// `.js` and `.css` are here because a report is a web page and needs its own -// assets; they are served, never executed by this process. +// `.js` and `.css` are here because a report is a web page that needs its own +// assets: Playwright's trace viewer ships as `trace/*.js` and `trace/*.css` +// beside the report, and dropping them would leave the trace — the most useful +// evidence a run produces — unopenable. +// +// The cost, accepted knowingly: any `.js` anywhere in the scanned project is +// reachable through this route as same-origin script, and the document CSP +// allows `script-src 'self'`, so a hostile report could load it. Serving them +// as downloads instead would break the viewer, and confining them to a report +// subdirectory would need configuration the profile does not carry. The +// boundary stays the one above — a project you would run tests from — and this +// is the sharpest edge of it. const MAX_BYTES = 512 * 1024 * 1024; // Served HTML and SVG come from a scanned repo, which is not a trusted author, diff --git a/docs/how-to/inspect-in-the-browser.md b/docs/how-to/inspect-in-the-browser.md index 235f64c..16a707d 100644 --- a/docs/how-to/inspect-in-the-browser.md +++ b/docs/how-to/inspect-in-the-browser.md @@ -36,6 +36,19 @@ Quality Explorer can: - Show matched, unmatched, and ambiguous runtime results. - Generate text instructions that you can give to a coding agent. +## Bind it to loopback only + +Quality Explorer has no sign-in: it is a single-user local tool, and the +loopback address is the whole access boundary. It serves run-evidence files out +of the opened project — reports, videos, traces — to any caller that can reach +it. + +Do not run it with `--hostname 0.0.0.0`, and do not port-forward it out of a +container or put it behind a reverse proxy. Doing either turns it into an +endpoint that hands the opened project's files to anyone on the network. A +deployment that needs to be reachable must put real authentication in front of +it. + ## Read-only boundaries Quality Explorer does not edit the selected repository. Actions such as diff --git a/packages/core/src/observation-sources/local-reports.ts b/packages/core/src/observation-sources/local-reports.ts index acc3442..79ff167 100644 --- a/packages/core/src/observation-sources/local-reports.ts +++ b/packages/core/src/observation-sources/local-reports.ts @@ -135,6 +135,12 @@ function resolveOption( // keeps its lexically-checked path — safe, because the caller's `readFile` // fails on it too, and reporting an unreadable file is a better message // than a containment complaint about a path that resolves to nothing. + // + // For `report` this also becomes an evidence ref. A dangling symlink could + // later be completed to point outside the project, so the ref alone is not + // a containment guarantee — it is safe because the route that serves it + // re-checks containment against real paths at request time, rather than + // trusting a ref recorded earlier. return { resolved }; } } diff --git a/packages/core/src/observations/resolve.ts b/packages/core/src/observations/resolve.ts index 251d79d..6c96ded 100644 --- a/packages/core/src/observations/resolve.ts +++ b/packages/core/src/observations/resolve.ts @@ -4,6 +4,7 @@ import type { NormalizedQualityGraph } from "@shiplightai/quality-map"; import { createDiagnostic } from "../diagnostics/diagnostic"; +import { countProblems } from "./ingest-helpers"; import type { ScanResult } from "../discovery/types"; import { OBSERVATION_SUITE_SEPARATOR } from "./types"; import type { @@ -384,7 +385,7 @@ export function resolveObservations( }); return { - status: statusFor(resolved, diagnostics.filter((entry) => entry.severity !== "info").length), + status: statusFor(resolved, countProblems(diagnostics)), observations: resolved, auditRows, diagnostics diff --git a/packages/core/src/recommendation-export/index.ts b/packages/core/src/recommendation-export/index.ts index 8a548f5..1186ef0 100644 --- a/packages/core/src/recommendation-export/index.ts +++ b/packages/core/src/recommendation-export/index.ts @@ -189,7 +189,7 @@ function hasUsableRuntimeProofStatus(input: { return input.executionStatus !== "invalid" && input.resolutionStatus !== "invalid" && input.observationCount > 0; } -// KNOWN, PRE-EXISTING: this is not injective. `my view` and `my-view` both +// KNOWN, PRE-EXISTING, tracked as issue #16: this is not injective. `my view` and `my-view` both // reduce to `my-view`, so two saved views with those ids write the same export // and one silently overwrites the other. Making the segment unique renames the // file for every id that needs sanitizing, which breaks anything addressing an diff --git a/packages/core/src/views/json-schema.ts b/packages/core/src/views/json-schema.ts index 5d468f1..a37986d 100644 --- a/packages/core/src/views/json-schema.ts +++ b/packages/core/src/views/json-schema.ts @@ -31,7 +31,9 @@ export function buildSavedViewsJsonSchema(): Record { // // No shape rule beyond that: ids only reach filenames through the // recommendation export's own sanitizer, so a stricter pattern would - // reject working configuration for no gain. + // reject working configuration for no gain. That sanitizer can collide + // on ids differing only in a separator — issue #16 — which a pattern + // here would narrow but not fix, and would break existing configs to do. not: { const: WHOLE_PROJECT_VIEW_ID } }, view: { From 7a4d5fbec5c140f5e3944f5cb09c9f9478003237 Mon Sep 17 00:00:00 2001 From: Feng Qian Date: Fri, 4 Sep 2026 12:48:19 -0700 Subject: [PATCH 5/7] fix(quality): repair the evidence ref under a symlinked root and unblock trace frames MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Local high-effort review found two defects the bot did not, both from my own previous rounds, both reproduced before fixing. The evidence ref was computed against the project root as passed in, while `resolveOption` returns the realpath'd target. Wherever the root traverses a symlink — macOS `/var`, a symlinked checkout, a container workdir — the "project-relative" ref became an escaping chain: ../../../../../../private/var/folders/.../playwright-report/index.html The browser normalises those segments away before sending, so the request never reached the route and every link 404'd. `resolveOption` now returns the root it actually resolved against, and the ref is relative to that. The existing test passed only by accident: its fixture never created `playwright-report/index.html`, so `realpathSync` threw, the lexical fallback ran, and a wrong ref looked right against a missing file. The fixture now creates the report, with a case asserting the ref is neither absolute nor escaping. `frame-src 'none'`, added last round on review advice, blocked the Playwright trace viewer — it replays a captured page by framing it, which is the viewer the `.js` allowance exists to serve. Measured directly: the framed file was requested 0 times under `'none'` and 2 under `'self'`. Remote framing, the part that could carry a URL elsewhere, stays blocked. Also: `startsWith("..")` rejected a contained directory literally named `..cache`, so containment compares the path segment; `junit` and `github-actions` still counted `info` diagnostics toward status, which is what `countProblems` exists to prevent; and evidence-ref React keys used `ref` alone, which the schema permits to repeat with different labels. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NhxpEed9pbcjT7MLtnDbiH --- .../evidence-file/[...ref]/route.ts | 17 +++++++++--- .../src/observation-sources/local-reports.ts | 22 ++++++++------- .../core/src/observations/github-actions.ts | 3 ++- packages/core/src/observations/junit.ts | 4 +-- packages/ui/src/components/FeaturePage.tsx | 6 ++--- .../src/components/ObservationAuditPanel.tsx | 6 ++--- .../local-reports-run-evidence.test.ts | 27 +++++++++++++++++++ 7 files changed, 63 insertions(+), 22 deletions(-) diff --git a/apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts b/apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts index 1f5d747..ce8a834 100644 --- a/apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts +++ b/apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts @@ -100,9 +100,11 @@ const DOCUMENT_CSP = [ "media-src 'self' data: blob:", "font-src 'self' data:", "connect-src 'self'", - // Covered by default-src's fallback already; stated outright so the intent of - // the two directives that matter most here is not left to be inferred. - "frame-src 'none'", + // `'self'`, not `'none'`: Playwright's trace viewer replays a captured page + // by framing it, so refusing frames outright would blank every snapshot pane + // in the viewer this route allows `.js` in order to serve. Remote framing + // stays blocked, which is the part that carries a URL somewhere else. + "frame-src 'self'", "worker-src 'self'", "form-action 'none'", "frame-ancestors 'none'", @@ -112,6 +114,13 @@ const DOCUMENT_CSP = [ const SCRIPTABLE_TYPES = new Set([".html", ".htm", ".svg"]); +// `relative === ".."` or a leading `../` segment. A prefix test would also +// reject a contained directory literally named `..cache`, whose relative path +// starts with two dots but never leaves the root. +function escapesRoot(relative: string): boolean { + return relative === ".." || relative.startsWith(`..${path.sep}`) || path.isAbsolute(relative); +} + export async function GET( _request: Request, context: { params: Promise<{ ref: readonly string[] }> }, @@ -152,7 +161,7 @@ export async function GET( } const contained = path.relative(realRoot, realResolved); - if (contained.startsWith("..") || path.isAbsolute(contained)) { + if (escapesRoot(contained)) { return problem(403, "evidence-ref-outside-project", "That path is outside the opened project."); } diff --git a/packages/core/src/observation-sources/local-reports.ts b/packages/core/src/observation-sources/local-reports.ts index 79ff167..8d699c1 100644 --- a/packages/core/src/observation-sources/local-reports.ts +++ b/packages/core/src/observation-sources/local-reports.ts @@ -68,7 +68,7 @@ function resolveOption( profile: ObservationSourceProfile, option: "path" | "report", projectRoot: string | undefined -): { readonly resolved?: string; readonly diagnostic?: ScanDiagnostic } { +): { readonly resolved?: string; readonly root?: string; readonly diagnostic?: ScanDiagnostic } { const declared = profile.host?.options[option]; if (declared === undefined || declared.length === 0) { return { @@ -100,7 +100,10 @@ function resolveOption( // location rather than being refused. const escaped = (root: string, target: string): boolean => { const relative = path.relative(root, target); - return relative.startsWith("..") || path.isAbsolute(relative); + // Segment comparison, not a prefix test: a contained directory literally + // named `..cache` has a relative path starting with two dots and never + // leaves the root. + return relative === ".." || relative.startsWith(`..${path.sep}`) || path.isAbsolute(relative); }; const outside = (): { readonly diagnostic: ScanDiagnostic } => ({ diagnostic: invalid( @@ -128,7 +131,7 @@ function resolveOption( if (escaped(realRoot, realResolved)) { return outside(); } - return { resolved: realResolved }; + return { resolved: realResolved, root: realRoot }; } catch { // The target does not exist, so there is nothing to resolve and nothing to // read. A dangling symlink whose text sits inside the root reaches here and @@ -141,7 +144,7 @@ function resolveOption( // a containment guarantee — it is safe because the route that serves it // re-checks containment against real paths at request time, rather than // trusting a ref recorded earlier. - return { resolved }; + return { resolved, root: projectRoot }; } } @@ -167,11 +170,12 @@ function reportRef( return { refs: [], diagnostics: location.diagnostic === undefined ? [] : [location.diagnostic] }; } - // Recorded project-root-relative rather than absolute, so the ref means the - // same thing to every reader of this repo instead of encoding one machine's - // checkout location. `resolveOption` has already refused anything that lands - // outside the root, so this is always a contained path. - const ref = path.relative(projectRoot ?? "", location.resolved); + // Relative to the ROOT `resolveOption` actually resolved against, not the one + // passed in. It realpaths the target, and relativizing a real path against a + // symlinked root yields an escaping `../../` chain — which then normalises + // away in the browser and 404s. Anywhere the root traverses a link (macOS + // `/var`, a symlinked checkout, a container workdir) this is not hypothetical. + const ref = path.relative(location.root ?? projectRoot ?? "", location.resolved); return { refs: [{ ref: ref.replaceAll("\\", "/"), label: REPORT_LABEL }], diagnostics: [] }; } diff --git a/packages/core/src/observations/github-actions.ts b/packages/core/src/observations/github-actions.ts index 97832e2..920a025 100644 --- a/packages/core/src/observations/github-actions.ts +++ b/packages/core/src/observations/github-actions.ts @@ -1,6 +1,7 @@ import { createDiagnostic } from "../diagnostics/diagnostic"; import { INTERNAL_OBSERVATION_CONTEXT } from "./types"; import type { ObservationIngestionResult, ObservationRecordInput, ObservationRecordStatus } from "./types"; +import { countProblems } from "./ingest-helpers"; import { normalizeObservationBatches } from "./normalize"; export interface GitHubActionsStepInput { @@ -282,7 +283,7 @@ export function ingestGitHubActionsRun( const mergedDiagnostics = [...diagnostics, ...normalized.diagnostics]; return { - status: statusFor(normalized.observations.length, mergedDiagnostics.length), + status: statusFor(normalized.observations.length, countProblems(mergedDiagnostics)), observations: normalized.observations, diagnostics: mergedDiagnostics }; diff --git a/packages/core/src/observations/junit.ts b/packages/core/src/observations/junit.ts index 609ba37..933510e 100644 --- a/packages/core/src/observations/junit.ts +++ b/packages/core/src/observations/junit.ts @@ -8,7 +8,7 @@ import type { ObservationRecordInput, ObservationRecordStatus } from "./types"; -import { isoTimestamp, normalizeArtifact, normalizePath, statusFor, stringValue } from "./ingest-helpers"; +import { countProblems, isoTimestamp, normalizeArtifact, normalizePath, statusFor, stringValue } from "./ingest-helpers"; import { normalizeObservationBatches } from "./normalize"; interface ParsedJunitCase { @@ -246,7 +246,7 @@ export function ingestJunitXmlReport( const mergedDiagnostics = [...normalized.diagnostics, ...diagnostics]; return { - status: statusFor(normalized.observations.length, mergedDiagnostics.length), + status: statusFor(normalized.observations.length, countProblems(mergedDiagnostics)), observations: normalized.observations, diagnostics: mergedDiagnostics }; diff --git a/packages/ui/src/components/FeaturePage.tsx b/packages/ui/src/components/FeaturePage.tsx index 82926c3..828a572 100644 --- a/packages/ui/src/components/FeaturePage.tsx +++ b/packages/ui/src/components/FeaturePage.tsx @@ -571,14 +571,14 @@ export function FeaturePage({ )} - {observed?.evidenceRefs.map((ref: NormalizedEvidenceRef) => { + {observed?.evidenceRefs.map((ref: NormalizedEvidenceRef, refIndex: number) => { const href = evidenceRefHref(ref.ref, qcApi, servesEvidenceFiles); return href === undefined ? ( - + {ref.label ?? "Run evidence"}: {ref.ref} ) : ( - + {ref.label ?? "Run evidence"} ); diff --git a/packages/ui/src/components/ObservationAuditPanel.tsx b/packages/ui/src/components/ObservationAuditPanel.tsx index 61319c4..6e45918 100644 --- a/packages/ui/src/components/ObservationAuditPanel.tsx +++ b/packages/ui/src/components/ObservationAuditPanel.tsx @@ -140,12 +140,12 @@ export function ObservationAuditPanel({ {row.evidenceRefs.length > 0 ? ( Run evidence - {row.evidenceRefs.map((entry) => { + {row.evidenceRefs.map((entry, entryIndex) => { const href = evidenceRefHref(entry.ref, qcApi, servesEvidenceFiles); const destination = evidenceRefDestination(entry.ref); return href === undefined ? ( ) : ( - + {evidenceLabel(entry)} diff --git a/tests/integration/local-reports-run-evidence.test.ts b/tests/integration/local-reports-run-evidence.test.ts index 6b17fc5..d689be9 100644 --- a/tests/integration/local-reports-run-evidence.test.ts +++ b/tests/integration/local-reports-run-evidence.test.ts @@ -91,6 +91,10 @@ async function fixture(reportJson: string | undefined) { return createFixtureProject("local-reports-run-evidence", [ { relativePath: ".quality/evidence/checkout/quality-map.yaml", contents: QUALITY_MAP }, { relativePath: ".quality/config/observation-sources.yaml", contents: OBSERVATION_SOURCES }, + // The HTML report must EXIST. Without it the ref path never resolves, and a + // ref that is wrong for an existing file looks correct against a missing + // one — which is how a broken `../../` ref passed this suite unnoticed. + { relativePath: "playwright-report/index.html", contents: "

report

" }, ...(reportJson === undefined ? [] : [{ relativePath: "playwright-report/report.json", contents: reportJson }]) @@ -143,6 +147,29 @@ describe("local-reports host transport", () => { } }); + it("keeps the ref project-relative when the project root traverses a symlink", async () => { + // A temp dir on macOS is reached through /var -> /private/var, and any + // symlinked checkout behaves the same. Relativizing the realpath'd target + // against the un-realpath'd root produced `../../../private/var/...`, which + // the browser normalises away before the request is even sent. + const project = await fixture(playwrightReport()); + + try { + const execution = await executeObservationSourceProfile({ + profile: profileFrom(project.root), + projectRoot: project.root, + hostTransports + }); + + const ref = execution.observations[0]?.evidenceRefs[0]?.ref ?? ""; + expect(ref).toBe("playwright-report/index.html"); + expect(ref.startsWith("..")).toBe(false); + expect(path.isAbsolute(ref)).toBe(false); + } finally { + await project.cleanup(); + } + }); + it("produces observations with no refs when no report is configured", async () => { // The results still count. Only the link to look at them is missing. const project = await createFixtureProject("local-reports-no-report", [ From 64591760bcd913ac5f33038c0bdbb7d623a992ba Mon Sep 17 00:00:00 2001 From: Feng Qian Date: Fri, 4 Sep 2026 13:07:26 -0700 Subject: [PATCH 6/7] chore(quality): record the approved size increase and drop the version claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Size gates: approved by Feng Qian, reason "new features", recorded at the measured sizes for quality-tools 0.3.2 (packed 46290, unpacked 168548). quality-ui has no approval field — re-baselining is itself the approval there — so its baseline moves to the measured 39089 / 166479. Written at the maintainer's explicit direction, with the name and reason they supplied verbatim. The how-to said run evidence "requires a quality-tools newer than 0.3.2" while package.json is 0.3.2, which cannot both be true in one tree. Version bumps here are their own `chore: release` commit, so the sentence goes rather than this change asserting a version it does not cut. The release that publishes the field can state which version carries it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NhxpEed9pbcjT7MLtnDbiH --- docs/how-to/make-ci-results-count.md | 3 --- packages/quality-tools/package-size.json | 8 +++++++- packages/ui/package-size.json | 4 ++-- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/docs/how-to/make-ci-results-count.md b/docs/how-to/make-ci-results-count.md index d077328..df9d86a 100644 --- a/docs/how-to/make-ci-results-count.md +++ b/docs/how-to/make-ci-results-count.md @@ -38,9 +38,6 @@ command, retry behavior, gate, or status. ## Link the run evidence (optional) -Requires a `quality-tools` newer than 0.3.2. Until that ships, the published -validator rejects the field. - A result can carry a pointer to what the run left behind, so a reviewer opening a check can see what the test actually did rather than only whether it passed: diff --git a/packages/quality-tools/package-size.json b/packages/quality-tools/package-size.json index 5e0ca99..06cc600 100644 --- a/packages/quality-tools/package-size.json +++ b/packages/quality-tools/package-size.json @@ -1,4 +1,10 @@ { "maxIncreasePercent": 1, - "approvedIncrease": null + "approvedIncrease": { + "version": "0.3.2", + "packedBytes": 46290, + "unpackedBytes": 168548, + "approvedBy": "Feng Qian", + "reason": "new features" + } } diff --git a/packages/ui/package-size.json b/packages/ui/package-size.json index c9bdc43..43539b4 100644 --- a/packages/ui/package-size.json +++ b/packages/ui/package-size.json @@ -1,6 +1,6 @@ { "baselineVersion": "0.1.0", - "baselinePackedBytes": 37350, - "baselineUnpackedBytes": 161150, + "baselinePackedBytes": 39089, + "baselineUnpackedBytes": 166479, "maxIncreasePercent": 1 } From 76bb9e741409aa5315bfe3338d0cb3ba0d271790 Mon Sep 17 00:00:00 2001 From: Feng Qian Date: Fri, 4 Sep 2026 13:17:54 -0700 Subject: [PATCH 7/7] fix(quality): make evidence-file containment self-sufficient MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `path.relative(root, root)` is `""`, and `escapesRoot("")` returned false, so a request resolving to the project root passed containment. It was still refused — the extension lookup and the isFile check both reject a directory — but containment that depends on the guards after it opens silently the moment those are reordered. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NhxpEed9pbcjT7MLtnDbiH --- .../quality-explorer/evidence-file/[...ref]/route.ts | 11 ++++++++++- .../api/quality-explorer/evidence-file/route.test.ts | 6 ++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts b/apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts index ce8a834..adcdcf7 100644 --- a/apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts +++ b/apps/explorer/src/app/api/quality-explorer/evidence-file/[...ref]/route.ts @@ -118,7 +118,16 @@ const SCRIPTABLE_TYPES = new Set([".html", ".htm", ".svg"]); // reject a contained directory literally named `..cache`, whose relative path // starts with two dots but never leaves the root. function escapesRoot(relative: string): boolean { - return relative === ".." || relative.startsWith(`..${path.sep}`) || path.isAbsolute(relative); + // `""` means the request resolved to the project root itself. Downstream the + // extension lookup and the isFile() check both refuse it, but containment + // should not depend on guards that follow it — reorder those and the hole + // opens silently. + return ( + relative === "" || + relative === ".." || + relative.startsWith(`..${path.sep}`) || + path.isAbsolute(relative) + ); } export async function GET( diff --git a/apps/explorer/src/app/api/quality-explorer/evidence-file/route.test.ts b/apps/explorer/src/app/api/quality-explorer/evidence-file/route.test.ts index d42de9a..9b70df7 100644 --- a/apps/explorer/src/app/api/quality-explorer/evidence-file/route.test.ts +++ b/apps/explorer/src/app/api/quality-explorer/evidence-file/route.test.ts @@ -125,6 +125,12 @@ describe("evidence-file route", () => { expect((await get("playwright-report", "absent.html")).status).toBe(404); }); + it("refuses a request that resolves to the project root itself", async () => { + // `path.relative(root, root)` is "", which must be treated as an escape by + // the containment check rather than by the type and isFile guards after it. + expect((await get(".")).status).toBe(403); + }); + it("refuses a directory", async () => { mkdirSync(path.join(root, "playwright-report"), { recursive: true });