Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,41 @@ this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

### Fixed

- **`REPORT_PUBLICATION_FAILED` now names its actual failure domain.** The
Action's terminal trust step treated any non-`success` publish outcome —
including a merely skipped step — as a publication failure, and conversely a
failed PR-comment or commit-status delivery after a successful publish
reported the diff verdict as if the reviewer had seen it. The trust state now
distinguishes publish `failure` (→ `REPORT_PUBLICATION_FAILED`), publish
skipped/cancelled (→ the diff verdict, `CERTIFICATION_FAILED` when absent),
and delivery failure after a successful publish (→
`REPORT_PUBLICATION_FAILED`). The verdict's `DEGRADED_BASELINE` check also
accepts `base-capture-failed` case-insensitively, matching the downstream
GitHub-expression gate.
- **`--tolerate-surface-failures` no longer promotes unledgered failures.** A
nonzero Playwright exit with maps on disk but NOTHING in the surface-failure
ledger (a self-check, nondeterminism, or harness failure) used to publish as
a "partial baseline"; promotion now requires at least one ledgered surface
failure, and anything else keeps the failing exit. Ledger filenames also
gained a content digest so two surface keys that sanitize to the same
filename can no longer clobber each other's entries.
- **`styleproof-ci` discards base-capture debris instead of keeping it as a
baseline.** With ledgered failures now exiting 0, a nonzero base capture
exit means an UNtolerated failure — any maps left on disk have no
publishable manifest, and keeping them reported real regressions as
approvable "new surfaces". The base dir is cleared and the run proceeds as a
bare baseline with `base-capture-failed=true` (`DEGRADED_BASELINE`).
- **`styleproof-init --check`/`--upgrade` now treat both generated workflows as
marker-owned.** The CI and approval workflow templates carry an ownership
marker (like the pre-push hook already did), so a consumer-authored
workflow at the same path reports `unmanaged` and is never overwritten by
`--upgrade`, instead of being flagged stale and clobbered.
- **The generated pre-push hook no longer bakes the default spec path.** With
the default spec, the hook execs bare `styleproof-prepush`, which resolves
the spec at run time (flag > env > `styleproof.config.json` > built-in) — so
moving the spec via config alone can't strand a stale baked `--spec`. A
non-default `--dir` is still baked explicitly.

- **A derived-only change now renders as reviewable evidence instead of failing
closed with none.** When a surface's only differences are size/position
longhands (`width`, `inline-size`, `transform-origin`, offsets…) — typically
Expand Down
24 changes: 21 additions & 3 deletions action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,10 @@ runs:
else if (inventoryFailures > 0) state = 'INVENTORY_REMOVAL_UNACKNOWLEDGED';
else if (certificationFailed || rawOnlyNoReviewable) state = 'CERTIFICATION_FAILED';
else if (partialBaseline) state = 'PARTIAL_BASELINE';
else if ('${{ inputs.base-capture-failed }}' === 'true') state = 'DEGRADED_BASELINE';
// Case-tolerant like the GitHub-expression `==` the block step uses, so a
// consumer passing 'True' can't get DEGRADED skipped here (a green status)
// while the block step still fails the job.
else if ('${{ inputs.base-capture-failed }}'.toLowerCase() === 'true') state = 'DEGRADED_BASELINE';
else if ('${{ steps.diff.outputs.changed }}' === 'true') state = 'VISUAL_APPROVAL_REQUIRED';
core.setOutput('state', state);
core.setOutput('data-residue-keys', JSON.stringify(residueKeys));
Expand Down Expand Up @@ -319,6 +322,7 @@ runs:
echo "raw-base=https://raw.githubusercontent.com/${REPO}/${REPORT_COMMIT}/${REPORT_PATH}" >> "$GITHUB_OUTPUT"

- name: Upsert PR comment
id: comment
if: steps.context.outputs.pr-number != ''
uses: actions/github-script@v9
with:
Expand Down Expand Up @@ -392,6 +396,7 @@ runs:
# instead of failing the job. Green when nothing changed; red ("needs
# sign-off") until a write-access reviewer ticks the box. PR events only.
- name: Set review status
id: status
if: steps.context.outputs.pr-number != '' && inputs.require-approval == 'true'
uses: actions/github-script@v9
with:
Expand Down Expand Up @@ -540,8 +545,21 @@ runs:
uses: actions/github-script@v9
with:
script: |
const published = '${{ steps.publish.outcome }}' === 'success';
// Name the FAILURE DOMAIN precisely, so automation retries the right thing:
// publish 'failure' → the publish step itself broke → REPORT_PUBLICATION_FAILED;
// publish 'skipped'/'cancelled' → an EARLIER step (capture/diff/config) failed —
// not a publication fault; report the verdict if one exists, else the
// capture/certification domain;
// comment/status delivery failed after a successful publish → the consumer-
// visible half of publication never landed → REPORT_PUBLICATION_FAILED,
// never a green classification from a run that delivered nothing.
const publishOutcome = '${{ steps.publish.outcome }}';
const diffState = '${{ steps.verdict.outputs.state }}';
const state = published ? (diffState || 'CERTIFICATION_FAILED') : 'REPORT_PUBLICATION_FAILED';
const deliveryFailed =
'${{ steps.comment.outcome }}' === 'failure' || '${{ steps.status.outcome }}' === 'failure';
let state;
if (publishOutcome === 'failure') state = 'REPORT_PUBLICATION_FAILED';
else if (publishOutcome !== 'success') state = diffState || 'CERTIFICATION_FAILED';
else state = deliveryFailed ? 'REPORT_PUBLICATION_FAILED' : (diffState || 'CERTIFICATION_FAILED');
core.setOutput('state', state);
core.info(`StyleProof terminal trust state: ${state}`);
19 changes: 11 additions & 8 deletions bin/styleproof-ci.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -333,18 +333,21 @@ try {
}
}
if (baseStatus !== 0) {
// Tolerated surface failures already exit 0 with a partial baseline
// (styleproof-map promotes only ledgered failures), so a nonzero exit
// here is an UNtolerated failure — any maps on disk are debris from a
// run that never produced a publishable manifest. Keeping them would
// report real regressions as approvable "new surfaces".
const baseDirPath = path.join(root, 'base');
const mapCount = countMaps(baseDirPath);
if (mapCount > 0) {
if (mapCount > 0)
log(
`base capture exited ${baseStatus} but ${mapCount} surface map(s) remain — keeping partial baseline (tolerated surface failures)`,
`base capture exited ${baseStatus} with ${mapCount} surface map(s) on disk but no publishable manifest — discarding the debris`,
);
} else {
log(`base capture failed (exit ${baseStatus}) — continuing with a bare baseline`);
fs.rmSync(baseDirPath, { recursive: true, force: true });
fs.mkdirSync(baseDirPath, { recursive: true });
baseCaptureFailed = true;
}
log(`base capture failed (exit ${baseStatus}) — continuing with a bare baseline`);
fs.rmSync(baseDirPath, { recursive: true, force: true });
fs.mkdirSync(baseDirPath, { recursive: true });
baseCaptureFailed = true;
}
} else {
// The base commit predates the spec (first adoption): an empty base dir means
Expand Down
21 changes: 16 additions & 5 deletions bin/styleproof-init.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ To certify a refactor:
`;

const argv = process.argv.slice(2);
let specPath = 'e2e/styleproof.spec.ts';
const DEFAULT_SPEC_PATH = 'e2e/styleproof.spec.ts';
let specPath = DEFAULT_SPEC_PATH;
let baseUrl = 'http://localhost:3000';
let force = false;
let hookOnly = false;
Expand Down Expand Up @@ -381,8 +382,10 @@ export default defineConfig({
`;

const CI_PATH = '.github/workflows/styleproof.yml';
const CI_OWNERSHIP_MARKER = '# StyleProof CI workflow';
const CI_WORKFLOW = `name: StyleProof

# StyleProof CI workflow (generated by styleproof-init; refreshed by styleproof-init --upgrade).
# Cache-first v4 flow:
# - run \`styleproof-map\` locally after committing to build/upload this commit's map
# outside CI when possible;
Expand Down Expand Up @@ -533,6 +536,10 @@ function ensureGitignoreLine(line) {
// The hook file is a thin shim: the refspec/docs-only/capture rules live in the
// packaged styleproof-prepush command, so behavior updates with each styleproof
// release instead of drifting in a copied bash file per consumer.
// Only a NON-default spec path gets baked into the hook. With the default,
// styleproof-prepush resolves the spec itself (flag > env > styleproof.config.json
// > built-in), so a later config-only spec move doesn't strand a stale baked path.
const hookSpecArgs = specPath === DEFAULT_SPEC_PATH ? '' : ` --spec ${specPath}`;
const HOOK = `#!/bin/sh
# StyleProof pre-push (generated by styleproof-init; refresh with: styleproof-init --hook).
# Capture the pushed commit's map and publish it to the styleproof-maps branch, so CI
Expand All @@ -543,7 +550,7 @@ const HOOK = `#!/bin/sh
# A skipped capture is always safe — CI just recaptures on a cache miss:
# STYLEPROOF_SKIP_CAPTURE=1 git push
[ "\${STYLEPROOF_SKIP_CAPTURE:-}" = "1" ] && exit 0
exec ./node_modules/.bin/styleproof-prepush --spec ${specPath}
exec ./node_modules/.bin/styleproof-prepush${hookSpecArgs}
`;
const HOOK_OWNERSHIP_MARKER = '# StyleProof pre-push';

Expand Down Expand Up @@ -572,6 +579,8 @@ if (hookOnly) {
}

const APPROVE_PATH = '.github/workflows/styleproof-approve.yml';
// First line of example/styleproof-approve.yml — the packaged template carries it.
const APPROVE_OWNERSHIP_MARKER = '# StyleProof approval gate';
function readApproveTemplate() {
const approveSource = path.join(
path.dirname(fileURLToPath(import.meta.url)),
Expand Down Expand Up @@ -604,8 +613,10 @@ function machineOwnedFiles() {
executable: true,
ownershipMarker: HOOK_OWNERSHIP_MARKER,
},
{ file: CI_PATH, contents: CI_WORKFLOW },
...(approve === undefined ? [] : [{ file: APPROVE_PATH, contents: approve }]),
{ file: CI_PATH, contents: CI_WORKFLOW, ownershipMarker: CI_OWNERSHIP_MARKER },
...(approve === undefined
? []
: [{ file: APPROVE_PATH, contents: approve, ownershipMarker: APPROVE_OWNERSHIP_MARKER }]),
];
}

Expand Down Expand Up @@ -644,7 +655,7 @@ if (upgrade) {
for (const { file, contents, executable, ownershipMarker } of machineOwnedFiles()) {
const existing = fs.existsSync(file) ? fs.readFileSync(file, 'utf8') : undefined;
if (existing !== undefined && ownershipMarker && !existing.includes(ownershipMarker)) {
console.log(`unmanaged ${file} (left unchanged; use --hook to replace it explicitly)`);
console.log(`unmanaged ${file} (left unchanged; delete it and rerun --upgrade to adopt the packaged template)`);
continue;
}
if (existing === contents) {
Expand Down
12 changes: 11 additions & 1 deletion bin/styleproof-map.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -382,11 +382,21 @@ if (result.error) {
let status = result.status ?? 1;
const captured = fs.existsSync(targetDir) ? fs.readdirSync(targetDir).filter(isMapFile).length : 0;
const toleratedFailures = readSurfaceCaptureFailures(targetDir);
if (status !== 0 && tolerateSurfaceFailures && captured > 0) {
// Promote to a publishable partial baseline ONLY when the failures are actually
// LEDGERED. Self-check/nondeterminism failures are deliberately never recorded —
// promoting a run that failed for an unrecorded reason would publish a "partial
// baseline (0 tolerated failures)" whose missing surfaces later read as approvable
// greenfield-new: exactly the laundering the ledger exists to prevent.
if (status !== 0 && tolerateSurfaceFailures && captured > 0 && toleratedFailures.length > 0) {
console.error(
`styleproof-map: Playwright exited ${status} but ${captured} surface map(s) were captured — publishing partial baseline (${toleratedFailures.length} tolerated failure(s))`,
);
status = 0;
} else if (status !== 0 && tolerateSurfaceFailures && captured > 0) {
console.error(
`styleproof-map: Playwright exited ${status} with ${captured} surface map(s) but NO ledgered surface failure — ` +
'an unrecorded failure class (e.g. a self-check/nondeterminism failure) is not tolerable; failing the capture.',
);
}
if (status === 0) {
if (!keepHar) removeHarFiles(targetDir);
Expand Down
6 changes: 5 additions & 1 deletion src/map-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,11 @@ export function remoteExists(remote = DEFAULT_REMOTE, cwd = process.cwd()): bool
* git fields. Shared by {@link writeMapManifest} (spec capture) and
* {@link writeCaptureManifest} (one-shot capture) so the object shape lives in one place. */
function failureFileName(key: string): string {
return `${key.replace(/[^a-zA-Z0-9@._-]+/g, '_')}.json`;
// Sanitization can collide distinct keys (`a/b@1280` vs `a_b@1280`); suffix a
// short hash of the RAW key so a later write can never erase another surface's
// ledger entry (which would resurface its missing surface as greenfield-new).
const digest = createHash('sha256').update(key).digest('hex').slice(0, 8);
return `${key.replace(/[^a-zA-Z0-9@._-]+/g, '_')}-${digest}.json`;
}

/** Record one tolerated surface failure (safe under parallel Playwright workers). */
Expand Down
13 changes: 13 additions & 0 deletions test/action.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -126,10 +126,23 @@ test('composite action exposes one precedence-ordered machine-readable trust ver
degraded > partial &&
visual > degraded,
);
// The verdict's degraded-baseline check must accept the same values the
// GitHub-expression gate downstream accepts (case-insensitive 'true').
assert.match(verdict[0], /base-capture-failed[^\n]*\.toLowerCase\(\) === 'true'/);
const terminal = actionYml.match(/- id: trust[\s\S]*$/);
assert.ok(terminal, 'action.yml should always expose a terminal trust state');
assert.match(terminal[0], /if: always\(\)/);
assert.match(terminal[0], /REPORT_PUBLICATION_FAILED/);
// The trust step names failure DOMAINS, not just "publish wasn't success":
// publish failure and delivery (comment/status) failure both mean the reviewer
// may be looking at a stale or absent report; a merely-skipped publish must NOT
// masquerade as a publication failure.
assert.match(terminal[0], /publishOutcome === 'failure'/);
assert.match(terminal[0], /publishOutcome !== 'success'/);
assert.match(terminal[0], /steps\.comment\.outcome/);
assert.match(terminal[0], /steps\.status\.outcome/);
assert.match(actionYml, /- name: Upsert PR comment\n\s+id: comment/);
assert.match(actionYml, /- name: Set review status\n\s+id: status/);
});

test('composite action hard-gates partial baseline repair debt', () => {
Expand Down
9 changes: 8 additions & 1 deletion test/ci-cli.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -536,7 +536,13 @@ test(
path.join(bin, 'playwright'),
`#!/bin/sh
if [ "$1" = "install" ]; then exit 0; fi
if [ "$(git rev-parse HEAD)" = "$BASE_FAIL_SHA" ]; then exit 1; fi
if [ "$(git rev-parse HEAD)" = "$BASE_FAIL_SHA" ]; then
# Leave map debris behind the failure: an untolerated failure with maps on
# disk must be discarded, never kept as a lying "partial baseline".
mkdir -p "$STYLEPROOF_BASEDIR/$STYLEMAP_DIR"
printf '{}' > "$STYLEPROOF_BASEDIR/$STYLEMAP_DIR/home@900.json"
exit 1
fi
if [ -n "$HEAD_FAIL_SHA" ] && [ "$(git rev-parse HEAD)" = "$HEAD_FAIL_SHA" ]; then exit 1; fi
mkdir -p "$STYLEPROOF_BASEDIR/$STYLEMAP_DIR"
printf '{}' > "$STYLEPROOF_BASEDIR/$STYLEMAP_DIR/home@900.json"
Expand Down Expand Up @@ -571,6 +577,7 @@ printf '{}' > "$STYLEPROOF_BASEDIR/$STYLEMAP_DIR/home@900.json"
assert.match(outputs, /head-hit=false/);
assert.match(outputs, /capture-needed=true/);
assert.match(outputs, /base-capture-failed=true/);
assert.match(result.stderr, /surface map\(s\) on disk but no publishable manifest — discarding the debris/);
assert.match(result.stderr, /base capture failed .* continuing with a bare baseline/);
const installedAt = fs.readFileSync(pmLog, 'utf8').trim().split('\n');
assert.ok(installedAt.includes(base), 'the base commit uses its pnpm lockfile');
Expand Down
4 changes: 3 additions & 1 deletion test/cli.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -940,9 +940,11 @@ test('init scaffolds the out-of-the-box gate: cache-first maps + report workflow
assert.equal(r.status, 0, r.stderr);

const hook = fs.readFileSync(path.join(dir, '.githooks', 'pre-push'), 'utf8');
// Default spec path → no baked --spec: styleproof-prepush resolves the spec
// at run time (flag > env > styleproof.config.json > built-in).
assert.match(
hook,
/exec \.\/node_modules\/\.bin\/styleproof-prepush --spec/,
/exec \.\/node_modules\/\.bin\/styleproof-prepush$/m,
'hook delegates to the installed packaged pre-push command',
);
assert.doesNotMatch(hook, /styleproof-map --/, 'no inlined capture invocation to drift');
Expand Down
Loading
Loading