OKF: the stall can mask a real failure; a local visual red needs a master run - #515
Merged
Conversation
…ster run Two gate rules sharpened after a six-PR merge pass. Both are increments on rules the bundle already had, added to the existing concepts rather than as new ones. ci-gates already said "re-run, don't raise the cap" and "if the job got past checkout it is a real failure". Necessary, not sufficient. On #511 `Unit Tests` read `fail 10m2s` - the stall signature exactly - and "known flake, merge it" was the tempting call. The fresh run got past checkout and failed again in 1m48s on a genuine defect: PavedPathGuardTest caught friday-report.css wired into bin/qtest but missing from css-bundle-ownership-map.md. Same check, same PR, two unrelated causes. Rule: a slow failure and a fast failure are different failures, and the first red's explanation must never carry over to the second. test-gates gains the local-vs-master rule. Verifying #511, `bin/qtest --changed` went red pointing at services/fractional-cto - a page it never touched - and the available story was that its postcss.config.js edit had shifted CSS site-wide. It had not: that edit only ADDS purgecss safelist entries, and safelisting more can only preserve more CSS, never remove any. The check that settled it was the same system test on clean origin/master on this macOS host: 34 runs, 6 failures, 8 of 77 screenshots mismatched. The suite is red on master here, so the local leg could not answer the question; CI's native-Linux Screenshot Tests passed #511 in 16m15s and was right. Validated: okf_validate --strict conformant, 0 errors (the 70 warnings are the pre-existing house log-heading convention). bin/hugo-build green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Session wrap-up. Two gate rules sharpened after a six-PR merge pass, both added to existing concepts rather than as new ones.
ci-gates — the stall can hide a real failure behind it
The concept already said re-run, don't raise the cap and if the job got past checkout it's a real failure. Necessary, not sufficient.
On #511
Unit Testsreadfail 10m2s— the stall signature exactly — and "known flake, merge it" was the tempting call. The fresh run got past checkout and failed again in 1m48s on a genuine defect:PavedPathGuardTestcaughtfriday-report.csswired intobin/qtestbut missing fromcss-bundle-ownership-map.md. Same check, same PR, two unrelated causes.Rule added: a slow failure and a fast failure are different failures. Multi-minute with no assertion output is infrastructure; fast with an assertion is the code. The first red's explanation must never carry over to the second.
test-gates — a local visual red can't condemn a branch until master runs the same way
Verifying #511,
bin/qtest --changedwent red pointing atservices/fractional-cto— a page the PR never touched. The available story was that itspostcss.config.jsedit had shifted CSS site-wide. It hadn't: that edit only adds purgecss safelist entries, and safelisting more can only preserve more CSS, never remove any.The check that settled it: the same system test on clean
origin/masteron this macOS host — 34 runs, 6 failures, 8 of 77 screenshots mismatched. The suite is red on master here, so the local leg couldn't answer the question. CI's native-Linux Screenshot Tests passed #511 in 16m15s and was right.Also noted: the run rewrites two of those baselines mid-run then fails its own dirty-check, so
git checkout -- test/fixtures/screenshots/never converges.Validation
okf_validate --strictconformant, 0 errors (the 70 warnings are the pre-existing house log-heading convention, unrelated to these edits).bin/hugo-buildgreen. Docs-only.🤖 Generated with Claude Code