feat: streamline Quality onboarding and static assessments - #11
Conversation
There was a problem hiding this comment.
Review summary
This PR delivers solid work: static-only assessments end-to-end, hardened agent-skill instructions around join-key provenance, score-axis discipline, and human attribution, plus a suite of new contract and agent regression tests. The skill changes tighten the independence and human-gate invariants correctly.
One HIGH severity finding must be addressed before merge. Two lower-severity items are also noted.
HIGH — Schema version bump (v5 → v6) without migration or tolerant read path
Files: packages/core/src/recommendation-export/index.ts, packages/core/src/operations/index.ts:302–318, 450–455
isRecommendationExportFile now hard-gates on schema_version === "6". Any pre-existing file written under v5 (by any published 0.3.x build of quality-tools, before this change ships) falls through to:
// packages/core/src/operations/index.ts:450-455
if (!isRecommendationExportFile(parsed)) {
throw new QcOperationError(
500,
"The saved recommendations file is unreadable. Regenerate it for this scope with the quality-tools analyze command.",
{ code: "invalid-ranked-recommendations-output" },
);
}The error message is actionable, but the HTTP status is 500, not a 4xx. Per the project invariant, a version bump must ship with a migration, a tolerant read path or an actionable non-5xx error. This meets none of those three exits:
- No migration path (no v5→v6 converter).
- Not tolerant (
schema_version === "6"is a strict equality gate). - The error is actionable in text but carries status 500.
This matters because .quality/generated/ was not gitignored before this PR (the .gitignore addition is part of this very PR), so committed v5 artifacts in user repos that upgrade the Explorer will immediately serve HTTP 500 from the /recommendations API route. The Explorer UI then shows an opaque load-error banner with no way to distinguish "file too old" from a server crash.
Minimum fix options:
- Change the error status from 500 to 422 with the existing actionable message, so the client can present it as a recoverable user action rather than a server fault. OR
- Accept v5 files by also tolerating
schema_version === "5"and synthesising aquality_score_availabilitystub ({ status: "not_requested" }). OR - Add a v5 migration that rewrites the file in place before validation.
Option 1 is the smallest change: packages/core/src/operations/index.ts:451 — change 500 to 422.
Note: RecommendationsCompatibility.test.tsx uses a fixture with schema_version: "6" (not "5"), so the stale-v5 path is currently untested at the backend level.
MEDIUM — Developer's local filesystem path in published documentation
File: docs/verification/quality-prompt-sweep.md:1813
Target: `/Users/feng/Shiplight/screen-recorder`
A developer's absolute home-directory path is committed to a public documentation file. It should be replaced with a placeholder such as <your-project-path> or the actual repository name without the local mount point.
LOW — Prose run-on in the-four-scores.md
File: docs/concepts/the-four-scores.md, Quality section
The sentence boundary between the Structure confidence description and the next sentence is missing a paragraph break:
"…still reports Coverage, Evidence confidence, and Structure confidence. Quality is most directly tied to release decisions, but a high score…"
The two thoughts belong in separate paragraphs. This is a minor readability issue from the prose refactoring.
What looks good
- The
QualityScoreAvailabilityRecordtype and the three-status enum (available/not_requested/unavailable) are clear and cover all cases correctly. - The
staticreserved-id guard is applied at both the JSON schema level (observation-sets.schema.json) and at runtime inparse.tsandsaveObservationSetsOp, with consistent case-insensitive matching validated by theit.each(["static", "STATIC", "Static"])test. - The
isRecommendationExportFileguard now correctly enforces thehasObservationSet === hasRuntimeReviewinvariant — a file cannot claim an observation set without aruntime_reviewblock, and vice versa. - Agent-skill instructions for canonical join-key provenance, score-axis separation, policy-field evaluation, and human-attribution are well-specified and the four new agent-test fixtures cover the defects found in the prompt sweep.
recommendationsOutputPathduplication betweenoperations/index.tsandrecommendation-export/index.tshas been removed; the canonical helper is nowrecommendationExportOutputPath.- The
specvsagent_generatedprovenance fix — agent YAML authorship no longer downgrades checks that trace to accepted requirements — is correctly specified and tested withquality-spec-driven-bootstrap.md. - No paths were found where an agent writes, adjusts, or infers a score; no paths were found where agent actions promote
structure_provenanceabove what the human supplied; the Explorer write-boundary and project-root confinement invariants are unaffected.
There was a problem hiding this comment.
PR Review: feat: streamline Quality onboarding and static assessments
Summary
This PR ships three cohesive things: (1) a streamlined, try-first README and concept-guide rewrite; (2) hardened quality-skill instructions addressing spec provenance, join-key discipline, reporting integrity, and attribution; and (3) end-to-end support for static-only assessments (schema v6, optional --observation-set, Explorer and UI updates). The coverage is solid: four new agent-regression tests, four new contract tests, and a documented prompt sweep with defects disclosed rather than hidden.
All project invariants were checked. No CRITICAL, HIGH, or MEDIUM issues found. The findings below are LOW.
LOW — observationSetFrom (parse.ts:131-140) falls through after the reserved-id diagnostic
After pushing the RESERVED_OBSERVATION_SET_ID error, the function does not return undefined; it falls through and returns a valid ObservationSet with id: "static". In contrast, missing id/name (lines 119-128) return undefined immediately. The bug is functionally contained — the batch's error diagnostics mark its status invalid, and buildRecommendationExport has an explicit guard that throws before findObservationSet is called — but the code is inconsistent with other error paths and the contract test only checks batch status, not whether the set is absent from the parsed list. Adding return undefined; after line 139 would match the intent and eliminate any future caller that skips the batch-status check.
LOW — Schema version "5" to "6": stale-file UX improved but divergence from published 0.3.x persists until a patch ships
isRecommendationExportFile now requires schema_version === "6" and rejects v5 files with a 422 + actionable "Regenerate it with analyze" message. This satisfies the CLAUDE.md criterion (actionable non-5xx error), and gitignoring .quality/generated/ makes these files ephemeral. The residual gap: users running the currently-published @shiplightai/quality-tools@^0.3.0 (which still requires --observation-set and writes v5) will enter an error loop — Explorer says regenerate, but the installed CLI writes v5 again. The skill documents the check (analyze --help — inspect whether the flag is optional), so informed users can reach the source-checkout CLI. The concern is whether the prompt sweep or README adequately sets this expectation before the new patch is published.
LOW — RecommendationsCompatibility.test.tsx fixture label is misleading
The legacyStaticPayload fixture uses schema_version: "6" but omits quality_score_availability. This is not a v5 legacy file; it is a partially-formed v6 file. The test purpose (verifying optional-chaining resilience in the UI) is sound, but the name implies compatibility with the old schema, which the UI does not attempt. Renaming to something like incompleteV6Payload would avoid confusion for future contributors.
LOW — Agent skill guidance on structure_provenance: spec
The new rule in map-feature/index.md (lines ~188-191) and start.md allows an agent to set structure_provenance: spec when checks trace to an accepted spec, clarifying that YAML authorship does not determine origin. CLAUDE.md flags anything that lets an agent self-promote provenance. In this case the agent is recording source, not approving its own work — the existing checks_reviewed gate and the explicit statement (~line 196: "must not record user_authored/spec without genuine human authorship or an accepted spec") remain. The risk is that an agent could misread a draft spec as "accepted." The spec-driven bootstrap regression test (tests/agent/quality-spec-driven-bootstrap.md) directly targets this defect, and the guidance explicitly says a directory name or a file called spec.md is not acceptance. No change needed, but worth monitoring as the agent test suite is exercised against real repositories.
Positive observations
- Independence preserved.
qualityScoreAvailabilityForis purely deterministic — derived from engine fields, no random or time-dependent input. No new path where an agent or UI computes or adjusts a score. - Human-gated fields tightened, not weakened. The attribution rule ("write the decision without a name or email; do not mine Git configuration") closes a real defect documented in the prompt sweep (finding #7: reviewer email inferred from local context).
- Deterministic naming. The
staticprefix reservation is enforced at three independent layers: JSON Schema, parse-time diagnostic, and thebuildRecommendationExportguard. - Disclosure over concealment. The prompt sweep document records the nondeterministic bootstrap result and the fact that the last sweep scenario needs rerunning after the CLI fix — honest accounting that makes future verification straightforward.
- Dependency direction maintained. No upward imports introduced;
packages/uicontinues to carry its own local type definitions rather than importing fromcore.
Summary
Verification