obs: quiet quality-delta PR comment + repo-health metric history on main - #1477
obs: quiet quality-delta PR comment + repo-health metric history on main#1477devin-ai-integration[bot] wants to merge 9 commits into
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
1bbdb92 to
fedccbd
Compare
|
Quality delta vs no main baseline
|
|
Reviewed exact head
Completed checks are green; remaining CI is still in progress. Please add regression coverage for the artifact race policy and fallback-baseline labeling. |
|
Both fixed in P1 — the artifact race. Rendering moved out of the Coverage job into Fork PRs are skipped there on purpose and it is now stated in the workflow and README: Regression coverage in P2 — fallback baseline provenance. The exactness flag is no longer discarded: it flows through One extra, from a maintainer note about comment fatigue: the marker is now Local gates green: |
|
Re-reviewed exact head |
Fixed in The renderer no longer trusts "any open PR associated with
Concurrency stays keyed by head sha but is no longer load-bearing for correctness, and the workflow comment now says so explicitly, since a head-sha group provably cannot cancel across heads. Regressions: Agreed on the evidence caveat: |
2bc9e7d to
90b5008
Compare
|
[P1] Revalidate the PR head in the posting path. The producer gate checks that workflow_run.head_sha is current, but setup, artifact downloads, history fetch, and rendering all occur before the sticky-comment write. A push during that window leaves the old-head run uncancelled because concurrency is SHA-scoped, so it can still overwrite newer metrics after passing the early gate. Recheck the PR current head immediately before posting, fail closed if it moved, and add a regression that advances the head after readiness but before the write. The artifact-freshness integration itself is sound; completed exact-head checks are green. The new workflow_run route still lacks live execution evidence until present on default main. |
Done in
Regression ( Also in this push, since #1471 merged: the branch is rebased directly onto Agreed the |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…nance Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…of the CLI shell Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…lines, generalize the comment marker Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…w refactor Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
410352d to
b33911b
Compare
|
P1: serialize sticky-comment writers by PR, not only by head SHA. The new |
…d sha Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Done exactly that in concurrency:
group: quality-delta-${{ github.event.workflow_run.pull_requests[0].number || github.event.workflow_run.head_sha }}
cancel-in-progress: trueSo the old-head renderer is now cancelled by PR identity the moment the new head's renderer starts — including while it sits between its head GET and the comment POST/PATCH, which is the window
(This push is also rebased onto current |
|
Closing without merge — not a verdict on the implementation, which is careful work (the pwn-request refusal, the CAS'd orphan-ref history, and the schema-version refusal are all right). The 2026-07-28 descope on #1412 cut Track C: the observatory is deferred until a concrete consumer exists, #1424 is closed not-planned, and the #1423 repo-health snapshot this PR builds its history on is being removed in the same cleanup. Landing a If Track C is revived, this PR is the reference design to start from. Generated by Claude Code |
Summary
Closes #1424 (Track C of #1412). Turns the size-report sticky comment into one quiet quality-delta
comment, and gives it something to compare against: a JSONL history of the #1423 repo-health
snapshot, appended on every push to
main.Quiet is the whole point. Nothing crossed a threshold ⇒ the comment is literally:
Something crossed ⇒ only those rows, each naming the gate that owns the number and the command that
reproduces it:
(real output from this branch, baseline hand-perturbed)
Shape
scripts/quality-delta/thresholds.ts— the ONE config block: every candidate metric, its noisethreshold, and its owning gate/fix. Two kinds:
deltametrics scored against the main baseline(size bytes, R6/R9 ratchets, fallow suppressions, redundant edges;
worseOnlyfor ratchets soremoving a suppression stays silent), and
absolutemetrics that have no baseline byconstruction (changed-line/branch coverage and excluded changed lines from ci: changed-line coverage gate on PRs #1418, slow-test
breaches) scored against a bound. Coverage's bound is
CHANGED_LINE_COVERAGE_THRESHOLD + 10, soapproaching the gate costs one quiet line instead of a surprise failure next commit.
scripts/quality-delta/model.ts— pure compare + render.MAX_COMMENT_LINES = 20is a hard cap;overflow drops the least-severe rows (severity = multiples of its own threshold crossed) and
replaces them with
+N more, see the job summary. The job summary always carries every row.scripts/quality-delta/history.ts+append-history.ts— the store:history/YYYY-MM.jsonlon anorphan ref, one compact snapshot per line.
appendEntryis keyed by commit sha and a no-op for acommit already stored, which is what makes the CAS retry below safe.
scripts/quality-delta/run.ts— all I/O, and the only place that knows the sticky marker.Refusals (both required by the issue, both tested)
schemaVersionbetween baseline and head is refused, not diffed: everybaseline-derived row is dropped and the comment says
Refusing to diff snapshot schemaVersion 1 against 2: write a migration note in scripts/quality-delta/README.md, then rerun.PR-local rows still show.stale(
provenance.inputs.*.status) renders asJS gzip (stale artifact)rather than silently claimingto describe the head commit. An
unknownstatus (today's honest answer for every artifact, sinceno producer stamps a commit) gets one job-summary note instead of a caveat on every size and
coverage row.
Missing inputs cost rows, never runs: no baseline / no size artifact / no coverage JSON each drop
their rows and add a job-summary note. A baseline that is not the PR's own base commit (the
history has no entry for it yet) is used but labelled —
### Quality delta vs nearest main@<sha> (PR base not in history yet)plus a summary note that deltas may include main-side changes the PR did notmake — so post-base movement on
mainis never attributed to the PR.Fork PRs degrade to the producers' job summaries. The rendering workflow skips them deliberately:
workflow_runcarries a write token in the base repo, so checking out and installing fork code therewould be a pwn-request.
One comment, not two — and it renders after BOTH producers finish
run.tsprepends the markerscripts/size-report.mjsowns and the comment is posted through thatsame
--post-commentmachinery: same update-in-place, no new bot.size.ymlno longer posts (itsfull base-vs-head table stays in its job summary; it publishes
size-report-jsoninstead) and itdropped
pull-requests: write.run.test.tsfails if the marker constants drift apart. Since size isnow one row family inside a general comment, the marker is
<!-- agent-device-quality-delta -->, withthe old size-specific one kept in
LEGACY_COMMENT_MARKERSso PRs that already carry that comment getit updated rather than duplicated.
The rows come from two workflows that run concurrently on the same head sha (CI: changed-line
coverage + slow tests; Size: the size report), so rendering inside either one is a race whose loser is
permanent — the comment is written once, so a size report landing a minute later would never reach it.
Rendering therefore lives in
.github/workflows/quality-delta.yml, triggered byworkflow_runonboth, with
scripts/quality-delta/producers.tsas the readiness decision:The last producer to complete renders; earlier firings exit at the gate, because the later
completion fires the trigger again. No polling, no waiting minutes. A re-run of one producer
supersedes its earlier run.
Writes are serialized per pull request —
concurrency: quality-delta-${{ workflow_run.pull_requests[0].number || workflow_run.head_sha }},cancel-in-progress: true— so anewer head cancels an older renderer even mid-write; the sha fallback covers fork triggers, which are
skipped anyway. Behind that, the gate refuses a superseded head (
selectRenderTarget: open PRwhose current
head.shais still the rendered sha) and hands the posting step itspr_number/pr_base_sha, and the write path re-checks the head itself and fails closed:so a push landing between the gate and the write — a window sha-scoped concurrency cannot cancel —
leaves the newer comment intact instead of overwriting it with older metrics.
History storage: dedicated orphan ref
metrics/repo-healthNot an artifact chain (artifacts expire and are keyed by run, not commit, so "what was metric X at
sha Y" becomes an API crawl), not gh-pages (that branch is the docs deploy target). An orphan ref is
permanent, keeps data out of
main's history, fetches shallowly, and is queryable by sha with plaingit. Monthly shards keep a lookup bounded (~11 kB/snapshot ⇒ ~1 MB for a busy month).
Serialization of concurrent main writes — both mechanisms: workflow
concurrency: { group: repo-health-history, cancel-in-progress: false }(queue, never cancel — acancelled run is a lost entry), plus compare-and-swap with one retry on the ref: clone tip →
append → push; a rejected push re-runs the whole append against the new tip. Idempotent per commit,
so a retry cannot double-write.
CI-scope changes and measured cost
.github/workflows/repo-health-history.yml(push tomainonly)pnpm build+pnpm size:markdown, which the snapshot reads. Not on the PR gate..github/workflows/quality-delta.yml(workflow_runon CI + Size, same-repo PRs only)pnpm repo-healthmeasured 0.81s locally (no network), the rest is a few API calls and one shallow fetch. Fires twice per head sha but the not-yet-ready firing exits in seconds.ci.ymlCoverage job: onequality-delta-inputsartifact uploadcontinue-on-error. No added permissions (the earlier revision'spull-requests: writeon this job is gone).size.yml: comment step →size-report-jsonartifact (7-day retention, ~11 kB)pull-requests: write.check:coverage-changed --json, andAGENT_DEVICE_SLOW_TEST_REPORTon the coverage runNo new gate:
CHECK_CATALOGis untouched, because nothing here can fail a build. The only workflowthat may post a comment is
quality-delta.yml, and a test asserts no other workflow contains--post-comment.Dependency on #1471 (#1423) — resolved
#1471 has merged, so this branch is now rebased directly onto
mainand contains only this issue'scommits. It consumes that snapshot's documented schema (
schemaVersion,provenance.commit,provenance.inputs.*.{sha256,producerCommit,status}) and does not modify it.Testing
scripts/quality-delta/{model,history,thresholds,producers}.test.tsinunit-core(pure, plus therace-policy wiring assertions over the workflow YAML); the CLI tests that spawn the commands are in
subprocess-stubper #1412 rule 4.thresholds.test.tsenumerates the config and fails if a row'sowner path doesn't exist or its fix isn't a real package script — a dead link in a PR comment is worse
than no comment.
Gates run green:
pnpm format:check && pnpm lint && pnpm typecheck,pnpm test:unit(532 files, 4717 tests),
pnpm check:layering,pnpm test:output-economy, andpnpm check:fallow --base origin/main.Link to Devin session: https://app.devin.ai/sessions/012e0d8a00984135a7b1c0bfcd2af2d5
Requested by: @thymikee