feat: complete the quality graph and publish canonical observations from CI - #6
Conversation
`quality-map-edit` labelled proof-policy edits "Gate 5", but the independence contract defines exactly four ratification gates and `policy_override` is not among them. A third numbering existed too: GateProgress.tsx used "Gate 1/2/6" for setup progression, where its "Gate 1" covered ratification gates 2 and 3 and its "Gate 2" covered gates 1 and 4. Same word, three schemes, colliding numbers. Proof policy is not a ratification gate. It feeds coverage and quality through the gap reasons in quality-structure/assessment.ts, never structure confidence, and it is the mirror of accepted risk: a human RAISING the bar rather than lowering it. `apply-edits.ts` already labelled accepted-risk edits without a gate number, so the file contradicted itself. - Drop the number; describe proof policy as the mirror of accepted risk. - Document both in independence.md as the two human decisions that are not gates, with the rule the repo was missing: an agent may propose either and may tighten freely, because a harder bar cannot manufacture trust, but must never loosen -- writing `accepted_gaps`, or removing or weakening an owner's `policy_override`, discards a human decision exactly as self-advancing a gate would. `requireGate: false` makes that reachable today. - Rename GateProgress to SetupProgress and renumber its comments to steps. It is internal to packages/ui, imported once, and absent from the public index, so the published surface is unchanged. A repo-wide audit now finds only gates 1 through 4. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reporters qualify a test with its suite chain but do not agree on the delimiter. `OBSERVATION_SUITE_SEPARATOR` is " > " (U+203A), which is what Playwright emits and what the junit adapter joins with; vitest's JUnit reporter writes a plain ASCII ">". The resolver recognised only the canonical form, so every bare-name pin in a vitest project failed to match: measured against this repo's own suite, 22 observations ingested and 22 resolved to nothing. The checks read as unobserved while every test passed -- the exact silent unmatch this logic exists to prevent. Three defects, one root cause, so one redesign rather than three patches: - Fold both sides onto a single sentinel before comparing, so a pin authored in either form matches an observation reported in either form. Normalising only the observed side left a canonically pinned check unmatchable by a vitest run. - Use a suffix test, not last-segment extraction. A title such as "errors when count > 0" contains the separator itself, so taking the final segment yields "0" and unmatches the check. - Prefer exact and file-level matches over suite-leaf matches. Treating them as equal candidates made an observation naming one check in full ambiguous against another pinned to its trailing words, dropping the observation and taking BOTH checks to unobserved. Folding happens at resolution, not ingestion: the stored observation keeps exactly the identity the producer reported, so a test genuinely named "a > b" is never rewritten. Verified end to end against the real suite: 0 of 22 resolved before, 309 of 354 after, with the remainder being tests not mapped to any check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The missing-artifact guard fired only when a run matched no artifact at all. A run that published some of its configured artifacts still yielded matches, so acquisition looked successful while every check proven only by the missing artifact read unobserved -- indistinguishable from having no proof. The failure is ordinary: one job uploads test observations, another a release gate. If the test job dies before its upload, the profile still selects that run, the gate artifact matches, and an entire test suite disappears from the assessment with nothing said. Adds INCOMPLETE_OBSERVATION_ARTIFACT_MATCH, mirroring the per-selector shape already used for ambiguity: any configured selector that matched no artifact is named, while the observations that did arrive stay usable. Reviewer guidance points at the job that failed to publish rather than at the checks that look unproven. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI ran the suite and reported pass or fail to GitHub, but serialized nothing, so this project's own quality score was unavailable and no estimate could stand in for it. The test job now converts its own JUnit output and the build job records the size-gate outcome; a transport-only source locates both. Within the producer edit boundary: the `Test` step keeps calling `pnpm test`, so the script, the tests and the exit code are unchanged and only reporter flags are added. Nothing here decides a result. Details that are load-bearing: - Emission is NOT gated on success. Publishing only green runs would feed the engine a diet of passes and bias every score upward. - It is gated on the artifact existing rather than `always()`: if the build failed or the job was cancelled, vitest never wrote junit.xml, and unconditional steps would turn one real failure into several red ones that publish nothing. - The size gate uses `!cancelled()`, not `always()`. A cancelled run has no verdict, and `always()` would have recorded the gate as FAILED on a commit whose gate never finished -- a fabricated failure fed to the engine. - `artifact_names` are exact, never a glob. One selector matching several artifacts is rejected as ambiguous and yields zero observations. - Uploads run only on `push`, because the profile pins `branch: main` and a pull_request run's artifacts can never be selected. - The workflow runs the built CLI from dist, not the published package: 0.3.1 predates the suite-separator handling these observations need in order to resolve at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Completes the quality graph: all six features now carry a quality map. 79 checks and 234 evidence pins, every pin verified to resolve to a real test name -- the engine validator checks the map's shape but not its join keys, so a renamed test would otherwise unmatch a check silently. Every check carries `priority: UNKNOWN` because no artifact in this repository declares a level, and every map stays `inferred_brownfield` with `checks_reviewed: false`. Coverage rose 45 -> 94 and evidence confidence 46 -> 95; structure confidence has not moved from 40 across any of this work, which is correct -- mapping proof cannot make a check list more trustworthy, only a human review can. Gaps recorded rather than papered over, the sharpest first: - Nothing proves the NEGATIVE direction of the ratification gate. Seven checks prove that a recorded human decision raises structure confidence; none proves that ingesting observations, adding evidence, or a passing run leaves it unchanged. The claim an agent could most easily violate is the one with no proof behind it. - `.quality/config/sources.yaml` is proven to parse, but nothing proves map-project ever reads it. An author could fill it in, see no effect, and every test would still be green. - Several performance bounds assert wall-clock time, so they report the runner's speed as much as the code's. Also updates the project map to match reality now that CI publishes observations: the runtime score is uncomputed, awaiting a real workflow run, rather than unavailable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Two MEDIUM issues and two LOW issues. The separator fix and partial-artifact detection are sound, but the refactoring commit is incomplete and the sentinel comment in resolve.ts is factually wrong in a way that matters for correctness.
Findings
MEDIUM — Incomplete refactor: apply-edits.ts:50 still names gate-5
File: packages/core/src/quality-map-edit/apply-edits.ts, line 50
The PR's stated goal is "stop the word 'gate' meaning two things." The field-level JSDoc on policyEdits was updated (from "Gate 5: per-check proof-policy edits" to the new language), and the function-level test name was updated, and independence.md now explicitly states that policy_override is not a ratification gate and "neither carries a gate number." But the function JSDoc at line 50 still reads:
Apply gate-4 (map-level checks_reviewed, add/remove checks) and gate-5
(per-check policy_override) edits to a raw quality-map YAML string...
A reader following this JSDoc would correctly give a gate number to something the PR explicitly says should have no gate number. The refactor is half-done.
Fix: Replace "and gate-5 (per-check policy_override)" with "and per-check policy_override" to match the now-documented understanding.
MEDIUM — Misleading sentinel comment and latent false-positive leaf match in resolve.ts
File: packages/core/src/observations/resolve.ts, lines 65-66 and 102
// Not a legal character in a test title, so it cannot collide with real text.
const SUITE_SEPARATOR_SENTINEL = " ";
The comment claims a space is "not a legal character in a test title," but spaces are the most common character in test titles (every word boundary). The sentinel IS just a space (U+0020). The logic works correctly for the intended cases — the suffix check at line 102:
foldedObserved.endsWith(`${SUITE_SEPARATOR_SENTINEL}${foldedPin}`)
…means a pin of "test" would false-positive leaf-match an observation of "unit test" that was produced with no suite separator at all, because "unit test".endsWith(" test") is true. The tests cover the specific separator-folding scenarios but do not cover this case. Since the exact-match branch fires first, this becomes a problem only when the false-positive leaf match is the only candidate for a given observation — unlikely for descriptive test names but not prevented structurally.
The comment needs to accurately describe the sentinel choice. The code should either use a genuinely non-text-character sentinel or document that the space is deliberately chosen and explain why the exact-beats-leaf priority limits the blast radius.
LOW — Binary diff for a scoring-path file
File: packages/core/src/observations/resolve.ts
The diff shows this file as binary (Binary files a/... and b/... differ), so the textual changes are invisible in the standard PR diff view. The file has no CRLF, no BOM, and no null bytes — the binary flag is likely a git heuristic artifact. The PR author flags this file as on the scoring path and "deserving review on its own merits"; reviewers relying on the diff alone cannot do that.
Adding a .gitattributes entry (*.ts text eol=lf) would force git to always diff TypeScript as text. The current file logic looks correct when read directly, but the missing .gitattributes is a maintenance gap.
LOW — INCOMPLETE_OBSERVATION_ARTIFACT_MATCH emitted as warning, not error
File: packages/core/src/observation-sources/execute.ts, line 629
The PR description explicitly calls the partial-artifact case "the dangerous case, because it looks like a successful acquisition while every check backed by the missing artifact silently reads unobserved." The new diagnostic is emitted as severity: "warning". The choice may be intentional (the observations that did arrive are valid and a hard error would discard them), but stating the reasoning explicitly in a comment alongside the severity assignment would prevent a later editor from silently upgrading it to error (breaking partial-acquisition) or downgrading it without understanding the consequence.
What is correct and well-done
- Separator-folding logic is sound. The suffix check replaces the last-segment extraction that broke pins whose titles contain the separator. The four new regression tests (vitest plain >, title-containing-separator, canonical-separator pin, exact-vs-leaf priority) each confirm a previously failing behaviour.
- Partial-artifact detection is correct. unmatchedSelectors in execute.ts filters for selectors with no match in the download set, emits the new diagnostic, and still forwards the partial results. The contract test exercises the exact one-artifact-missing, one-artifact-present scenario.
- CI emission design is correct. Emitting on failure, gating on !cancelled() rather than always(), and using exact artifact names (not globs) are all the right calls and are explained in the workflow comments.
- Human-gate invariants are unbroken. All six new quality maps set structure_provenance: "inferred_brownfield" and checks_reviewed: false. No agent-skill guidance tells an agent to write accepted_gaps or flip checks_reviewed. The new section in independence.md correctly characterises policy_override and accepted_gaps as non-gate human decisions and correctly constrains what agents may do with each.
- Proof gaps are disclosed honestly. The maps call out the missing negative-direction check for structure confidence, the wall-clock nature of performance assertions, and the unproven sources.yaml consumption path.
- Observation set config is correct. Exact artifact names, one transport profile, branch: main so PR runs are unreachable.
- Terminology refactor is almost complete. GateProgress -> SetupProgress, "Gate 6" -> "Step 3" in the UI, policyEdits field comment updated, test name updated — only the function JSDoc at apply-edits.ts:50 was missed.
The sentinel was correct in value but embedded as a literal 0x00 byte in the source. One raw NUL is enough for git, grep, and file(1) to classify the module as binary, so every change to it diffed as "Binary files differ" — hiding a scoring-path change from code review entirely. The value is unchanged; only its spelling is. Also from review: - The sentinel comment claimed only that a NUL "cannot collide with real text", which read as arbitrary. It now states what the choice buys: folding every reporter delimiter onto a character no title can contain makes the suffix test mean "the pin is a whole trailing suite segment" rather than "any trailing substring". A printable sentinel really would leaf-match a pin of "test" against "unit test". - apply-edits.ts still named "gate-5" in the function JSDoc, so the terminology change was half-applied: the field comment and the test had been updated, the function comment had not. - Record why INCOMPLETE_OBSERVATION_ARTIFACT_MATCH is a warning rather than an error, so a later editor does not raise it and discard a whole run's real observations because one job failed to publish. No .gitattributes entry: forcing `*.ts text` would have made git diff the file despite the NUL, masking the defect instead of removing it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Review: Complete the quality graph and publish canonical observations from CI
Decision: Approve. No CRITICAL, HIGH, or MEDIUM issues found. Four LOW findings below. The two scoring-path fixes (resolve.ts, execute.ts) are well-reasoned, correctly bounded, and fully test-covered. All project invariants respected.
INVARIANT CHECK
Independence of verification: PASS. CI converts JUnit to canonical facts; observations record transcribes a step outcome set by GitHub Actions, not the engine.
Human-gated fields: PASS. All five new quality-map.yaml files carry structure_provenance: inferred_brownfield and checks_reviewed: false.
Determinism: PASS. foldSeparators is a pure reduce over a fixed array; unmatchedSelectors is deterministic; no Date.now/Math.random on the scoring path.
Dependency direction: PASS. No new cross-layer imports introduced.
Explorer read-only: PASS. Only UI rename in packages/ui; no new data-access changes.
Project-root confinement: PASS. No new API routes accepting projectPath.
Saved-artifact compatibility: PASS. No schema_version changes; INCOMPLETE_OBSERVATION_ARTIFACT_MATCH is additive.
Published surface: PASS. CaseMatchKind export is additive; no renamed commands or flags.
Agent-skills safety: PASS. independence.md explicitly prohibits agents from writing accepted_gaps or weakening policy_override.
Docs match reality: PASS. project-map.yaml updated to resolved; no stale gate-5 references remain anywhere in agent-skills, docs, packages, or tests.
SCORING-PATH CHANGES
packages/core/src/observations/resolve.ts - The separator-folding fix is sound. Using NUL (U+0000) as the sentinel is correct: test names cannot contain NUL, so foldedObserved.endsWith(SENTINEL + foldedPin) means the pin is an entire trailing suite segment, not an arbitrary substring (a printable sentinel like space would leaf-match pin 'test' against unqualified observation 'unit test'). The suffix approach correctly handles test titles that themselves contain ' > ' (e.g., 'errors when count > 0') where the old last-segment extraction would yield '0' and silently unmatch. The folding order in REPORTER_SUITE_SEPARATORS is immaterial because the U+203A and ASCII forms do not overlap. Four regression tests cover: vitest separator, title containing separator, canonical-pin vs vitest-observed, and exact-beats-leaf tie-breaking.
packages/core/src/observation-sources/execute.ts - The unmatchedSelectors placement is correct: it fires only in the non-empty, non-ambiguous branch, catching partial uploads that previously read as successful full acquisition. Emitting severity warning (not error) is the right call - valid observations from present artifacts must keep counting. The contract test covers the exact scenario (one artifact uploaded, one missing). The new INCOMPLETE_OBSERVATION_ARTIFACT_MATCH code is registered in diagnostic.ts and has guidance text in diagnostic-guidance.ts.
packages/core/src/quality-map-edit/apply-edits.ts - The gate-5 to non-gate refactoring is complete. No stale 'gate 5' strings remain in agent-skills, docs, packages, or tests.
LOW FINDINGS
LOW-1 - Expression inline in run: block (.github/workflows/ci.yml, build job 'Emit canonical observations')
The expression steps.package-size.outcome is system-controlled (success/failure/cancelled/skipped) so the argument to --status evaluates to the literal string 'pass' or 'fail' and shell injection is impossible. However, using dollar-brace expressions inside run: is the pattern security linters flag because it is easy to accidentally substitute a user-controlled context value in a future step. The hygienic alternative is an env: mapping plus a shell variable. No change required for this PR.
LOW-2 - No mkdir -p reports before the test step (.github/workflows/ci.yml, test job)
The workflow relies on vitest creating reports/ before writing reports/junit.xml. Current vitest versions do create parent directories, and the hashFiles guard on the next step means a silently missing file causes a no-op rather than a broken pipeline. An explicit mkdir -p reports before pnpm test would remove the tooling assumption.
LOW-3 - CaseMatchKind exported but not visibly consumed (packages/core/src/observations/resolve.ts:83)
The type is exported with 'export type CaseMatchKind' but no importer appears in the diff. If it is purely internal, unexported would be cleaner; if intended for a future downstream consumer, a brief comment would explain why it is public.
LOW-4 - Ambiguity diagnostic occludes missing-selector diagnostic (packages/core/src/observation-sources/execute.ts)
The 'else if (ambiguityDetails.length > 0)' branch skips the unmatchedSelectors check. If a run has both an ambiguous match AND a missing selector, only the ambiguity surfaces until the ambiguity is fixed. No correctness risk, but minor UX friction in a debugging scenario.
ACKNOWLEDGED PROOF GAPS (self-documented, no action needed)
- Negative direction for structure confidence: no test asserts that ingesting observations or stronger evidence leaves structure_confidence unchanged. Features 003 and 004 document this shared gap with a concrete next step.
- sources.yaml consumed by map-project: parsing is proven; no integration test asserts values reach the project map.
- Reproducibility: export compared against a shape in one run; no two-run deep-equality check. Documented.
- CLI flag contract: no pinned-flag test for quality-tools commands. Documented.
- Wall-clock performance bounds: hardware-dependent; documented throughout.
1 participant
Summary
Five commits. The graph is now complete and CI publishes the results that feed it.
feat: map the five remaining features. All six features carry a quality map — 79 checks, 234 evidence pins, every pin verified to resolve to a real test name. The engine validator checks a map's shape but not its join keys, so a renamed test would otherwise unmatch a check silently. Coverage 45 → 94, evidence confidence 46 → 95. Structure confidence has not moved from 40 through any of this work, which is correct: mapping proof cannot make a check list more trustworthy, only human review can.feat(ci): publish canonical observations. The test job converts its own JUnit output; the build job records the size-gate outcome. Within the producer edit boundary — theTeststep still callspnpm test, so tests, script and exit code are unchanged and only reporter flags are added.fix(core): join vitest-reported test cases to their pinned checks. The wiring did not work out of the box, and the failure was silent.OBSERVATION_SUITE_SEPARATORis" › "(U+203A, what Playwright emits); vitest's JUnit reporter writes plain ASCII">". Measured against this repo's own suite: 22 observations ingested, 22 resolved to nothing — every check reading unobserved while every test passed.fix(core): report a partially published run. The missing-artifact guard fired only when a run matched no artifact. A run publishing some of them looked like successful acquisition while every check backed by the missing artifact silently read unobserved.refactor: stop the word "gate" meaning two things. Three numbering schemes shared the word, with colliding numbers. Proof policy is not a ratification gate — it feeds coverage and quality, never structure confidence, and is the mirror of accepted risk.Load-bearing details
!cancelled(), notalways(). A cancelled run has no verdict;always()would record the gate as failed on a commit whose gate never finished — a fabricated failure.artifact_namesare exact, never a glob. One selector matching several artifacts is rejected as ambiguous and yields zero observations."a > b"keeps the identity its producer reported.dist, not the published package: 0.3.1 predates the separator handling these observations need to resolve at all.Recorded, not papered over
.quality/config/sources.yamlis proven to parse, but nothing provesmap-projectever reads it.Test plan
pnpm test— 64 files, 357 tests (4 new regression tests, each confirmed failing before its fix)pnpm typecheck— clean across all five packagespnpm buildscripts/check-quality-skill.shquality-tools validatepnpm test→ JUnit →observations from-junit→ manifest valid → 309 of 354 resolve onto mapped evidence, 0 ambiguous (the 45 unmatched are tests deliberately not mapped)analyzewithout credentials degrades as designed:MISSING_OBSERVATION_SOURCE_ENV, structural scores retainedanalyzecomputes a runtime quality scoreReviewer note
resolve.tsandexecute.tsare on the scoring path and outsideimprove's formal edit boundary. Both changes came from working wiring that failed silently, and both are test-covered, but they deserve review on their own merits rather than as incidental to the CI work.