Skip to content

obs: quiet quality-delta PR comment + repo-health metric history on main - #1477

Closed
devin-ai-integration[bot] wants to merge 9 commits into
mainfrom
devin/1785254908-quality-delta-comment
Closed

obs: quiet quality-delta PR comment + repo-health metric history on main#1477
devin-ai-integration[bot] wants to merge 9 commits into
mainfrom
devin/1785254908-quality-delta-comment

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

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:

No notable quality deltas.

Something crossed ⇒ only those rows, each naming the gate that owns the number and the command that
reproduces it:

### Quality delta vs main@`0000000`

| Metric | Base | Head | Δ / bound | Gate · fix |
| --- | --- | --- | --- | --- |
| R6 type inversions | 12 | 7 | -5 | [CI / Layering Guard](…/scripts/layering/check.ts) · `pnpm check:layering` |
| Fallow suppressions | 16 | 19 | +3 | [CI / Fallow Code Quality](…/.fallowrc.json) · `pnpm check:fallow` |

(real output from this branch, baseline hand-perturbed)

Shape

  • scripts/quality-delta/thresholds.ts — the ONE config block: every candidate metric, its noise
    threshold, and its owning gate/fix. Two kinds: delta metrics scored against the main baseline
    (size bytes, R6/R9 ratchets, fallow suppressions, redundant edges; worseOnly for ratchets so
    removing a suppression stays silent), and absolute metrics that have no baseline by
    construction (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, so
    approaching the gate costs one quiet line instead of a surprise failure next commit.
  • scripts/quality-delta/model.ts — pure compare + render. MAX_COMMENT_LINES = 20 is 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.jsonl on an
    orphan ref, one compact snapshot per line. appendEntry is keyed by commit sha and a no-op for a
    commit 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)

  • Differing schemaVersion between baseline and head is refused, not diffed: every
    baseline-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.
  • A row whose head value came from an artifact repo-health proved stale
    (provenance.inputs.*.status) renders as JS gzip (stale artifact) rather than silently claiming
    to describe the head commit. An unknown status (today's honest answer for every artifact, since
    no 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 not
make — so post-base movement on main is never attributed to the PR.

Fork PRs degrade to the producers' job summaries. The rendering workflow skips them deliberately:
workflow_run carries a write token in the base repo, so checking out and installing fork code there
would be a pwn-request.

One comment, not two — and it renders after BOTH producers finish

run.ts prepends the marker scripts/size-report.mjs owns and the comment is posted through that
same --post-comment machinery: same update-in-place, no new bot. size.yml no longer posts (its
full base-vs-head table stays in its job summary; it publishes size-report-json instead) and it
dropped pull-requests: write. run.test.ts fails if the marker constants drift apart. Since size is
now one row family inside a general comment, the marker is <!-- agent-device-quality-delta -->, with
the old size-specific one kept in LEGACY_COMMENT_MARKERS so PRs that already carry that comment get
it 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 by workflow_run on
both, with scripts/quality-delta/producers.ts as the readiness decision:

producerReadiness(runs, headSha) // newest run per producer, ready only when all are `completed`

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 requestconcurrency: quality-delta-${{ workflow_run.pull_requests[0].number || workflow_run.head_sha }}, cancel-in-progress: true — so a
newer 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 PR
whose current head.sha is still the rendered sha) and hands the posting step its
pr_number/pr_base_sha, and the write path re-checks the head itself and fails closed:

pnpm size --post-comment .tmp/quality-delta.md --expect-head <rendered sha>

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-health

Not 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 plain
git. 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 — a
cancelled 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

Change Cost
New .github/workflows/repo-health-history.yml (push to main only) ~5 min/push, almost entirely pnpm build + pnpm size:markdown, which the snapshot reads. Not on the PR gate.
New .github/workflows/quality-delta.yml (workflow_run on CI + Size, same-repo PRs only) ~2 min/PR update: toolchain setup dominates; pnpm repo-health measured 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.yml Coverage job: one quality-delta-inputs artifact upload Seconds; continue-on-error. No added permissions (the earlier revision's pull-requests: write on this job is gone).
size.yml: comment step → size-report-json artifact (7-day retention, ~11 kB) Net −1 API call; loses pull-requests: write.
check:coverage-changed --json, and AGENT_DEVICE_SLOW_TEST_REPORT on the coverage run Writes one small JSON each; no extra work. Both feed rows instead of being scraped from log text.

No new gate: CHECK_CATALOG is untouched, because nothing here can fail a build. The only workflow
that 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 main and contains only this issue's
commits. 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.ts in unit-core (pure, plus the
race-policy wiring assertions over the workflow YAML); the CLI tests that spawn the commands are in
subprocess-stub per #1412 rule 4. thresholds.test.ts enumerates the config and fails if a row's
owner 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, and
pnpm check:fallow --base origin/main.

Link to Devin session: https://app.devin.ai/sessions/012e0d8a00984135a7b1c0bfcd2af2d5
Requested by: @thymikee

@thymikee thymikee self-assigned this Jul 28, 2026
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@devin-ai-integration
devin-ai-integration Bot force-pushed the devin/1785254908-quality-delta-comment branch from 1bbdb92 to fedccbd Compare July 28, 2026 16:28
@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-07-28 19:56 UTC

@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown

Quality delta vs no main baseline

Metric Base Head Δ / bound Gate · fix
Slow-test budget breaches 2 > 0 CI / Coverage · pnpm test:unit

@thymikee

Copy link
Copy Markdown
Member

Reviewed exact head 92ed3f5; two readiness issues remain:

  • P1 — .github/workflows/ci.yml:289-305: Coverage does a one-shot lookup for the concurrently produced Size artifact. If Coverage reaches this step first, it permanently omits size-delta rows for that head; Size finishing later never rerenders the sticky comment. Poll/wait for the matching artifact, or render from a workflow that runs after Size completes.
  • P2 — scripts/quality-delta/run.ts:60-72,108-120: findBaseline returns whether the lookup was exact, but the caller discards that flag and renders a fallback snapshot as main@<sha>. If main advances before the PR base snapshot is stored, unrelated post-base changes can be attributed to the PR. Preserve/render exact-vs-fallback provenance (as the README promises), or omit baseline-derived rows without an exact base.

Completed checks are green; remaining CI is still in progress. Please add regression coverage for the artifact race policy and fallback-baseline labeling.

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

Both fixed in ec1cae3.

P1 — the artifact race. Rendering moved out of the Coverage job into .github/workflows/quality-delta.yml, triggered by workflow_run on both producers (CI and Size, types: [completed]). scripts/quality-delta/producers.ts decides readiness from the runs GitHub reports for that head sha: the last producer to complete renders, and any earlier firing exits at the gate because the later completion fires the trigger again (no polling, no CI minutes spent waiting). A re-run of one producer supersedes its earlier run. CI's Coverage job now only uploads quality-delta-inputs (changed-coverage + slow-tests JSON); Size keeps uploading size-report-json.

Fork PRs are skipped there on purpose and it is now stated in the workflow and README: workflow_run carries a write token in the base repo, so checking out and installing fork code in it would be a pwn-request. Fork PRs keep the producers' job summaries — the documented degradation.

Regression coverage in scripts/quality-delta/producers.test.ts: readiness/deferral/reasons, other-sha runs never counting, re-run supersession, plus wiring assertions over the YAML (triggers on exactly the two producer workflows, every post-gate step is gated on ready, the fork guard is present, and only quality-delta.yml contains --post-comment). The $GITHUB_OUTPUT plumbing has a subprocess-project test in run.test.ts.

P2 — fallback baseline provenance. The exactness flag is no longer discarded: it flows through DeltaSources.baseExact into report.base.exact, and an inexact baseline renders as ### Quality delta vs nearest main@<sha> (PR base not in history yet) with a job-summary note that "Deltas may include main-side changes this PR did not make." An exact hit still renders the plain vs main@<sha>. Tested in model.test.ts (both headings + the note) and in run.test.ts end-to-end via a base sha absent from the history.

One extra, from a maintainer note about comment fatigue: the marker is now <!-- agent-device-quality-delta --> instead of the size-specific one it grew out of, with the old marker kept in LEGACY_COMMENT_MARKERS so a PR that already carries the size comment gets it updated in place rather than duplicated. Still exactly one comment per PR, and the test above pins that no other workflow can post one.

Local gates green: format:check, lint, typecheck, test:unit (532 files), check:layering, test:output-economy, and check:fallow --base origin/main.

@thymikee

Copy link
Copy Markdown
Member

Re-reviewed exact head ec1cae3f: the producer race and inexact-baseline attribution findings are genuinely fixed with effective decision/wiring regressions. One P1 blocker remains in .github/workflows/quality-delta.yml:26-30,105-140: rendering concurrency is keyed by head SHA, so old- and new-head runs do not cancel each other. The PR lookup accepts any open PR associated with HEAD_SHA but never verifies that PR’s current head.sha still equals it. A cancelled/completed old-head producer can therefore trigger a late renderer and overwrite the current sticky comment with obsolete metrics. Before posting, require the PR’s current head to equal workflow_run.head_sha (or serialize/cancel by PR identity), and add a superseded-head regression. All exact-head checks are green; the new workflow_run workflow has only static/unit evidence until it exists on default branch.

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

require the PR's current head to equal workflow_run.head_sha (or serialize/cancel by PR identity), and add a superseded-head regression

Fixed in 2bc9e7d43.

The renderer no longer trusts "any open PR associated with HEAD_SHA". The gate now answers both halves of "may this run post" in one decision (scripts/quality-delta/producers.ts):

  • producerReadiness(runs, headSha) — every producer completed (unchanged), and
  • selectRenderTarget(pulls, headSha) — returns the PR only if it is open and its current head.sha === headSha, otherwise null.

producers-run.ts emits ready, pr_number, and pr_base_sha from that single verified lookup; ready=false for a superseded head, so a late old-head renderer exits before pnpm quality-delta and cannot overwrite the sticky comment. The separate ungated Resolve the pull request and its base step — the unverified lookup you flagged — is gone; the posting step takes its PR number and base sha from the gate, so there is no path that writes the comment for a PR whose head has moved on.

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: producers.test.ts covers superseded head → null, closed PR → null, no PR → null, exact head → {number, baseSha}, plus an assertion that the posting step is gated and consumes steps.producers.outputs.pr_number. run.test.ts covers the CLI end to end: ready=true with pr_number/pr_base_sha, producer-still-running, and superseded head → ready=false with an empty pr_number.

Agreed on the evidence caveat: workflow_run only exists once it is on the default branch, which is why the policy lives in a pure module with the workflow YAML asserted against it rather than in shell.

@devin-ai-integration
devin-ai-integration Bot force-pushed the devin/1785254908-quality-delta-comment branch from 2bc9e7d to 90b5008 Compare July 28, 2026 18:14
@thymikee

Copy link
Copy Markdown
Member

[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.

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

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.

Done in 410352d15. The recheck now lives inside the write path, not in a step before it:

node scripts/size-report.mjs --post-comment .tmp/quality-delta.md --expect-head "${{ github.event.workflow_run.head_sha }}"

postGitHubComment refuses before listing or writing any comment unless GET /repos/:owner/:repo/pulls/:number still reports head.sha === expectedHead. It fails closed on any uncertainty: a moved head, a missing head.sha, or a lookup that returned non-OK all refuse and log Refusing to comment: PR #N head is <x>, not the <y> this report describes. — the existing comment keeps its newer metrics.

Regression (scripts/quality-delta/run.test.ts, subprocess-stub): the real size-report.mjs is spawned against a stub GitHub API (via GITHUB_API_URL, the same env var Actions sets) that reports a different head, i.e. exactly the "head advanced after readiness, before the write" case, and the test asserts the recorded calls contain no POST/PATCH. The paired case — same head — asserts POST /repos/callstack/agent-device/issues/7/comments still happens, so the guard cannot silently disable posting. producers.test.ts additionally asserts the workflow's posting step passes --expect-head, so the wiring cannot be dropped.

Also in this push, since #1471 merged: the branch is rebased directly onto main (no dependency commits left) and the reader consumes the merged provenance shape — provenance.inputs.*.status instead of the earlier stale/producerInputs. A stale artifact still marks its row; unknown (today's answer for every artifact, since no producer stamps a commit) gets one job-summary note rather than a caveat on every size and coverage row.

Agreed the workflow_run route has no live execution evidence until it is on main — the fix for that is merging it, which is why the decisions it depends on are pure and unit-owned rather than shell inline.

thymikee and others added 8 commits July 28, 2026 18:58
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>
@devin-ai-integration
devin-ai-integration Bot force-pushed the devin/1785254908-quality-delta-comment branch from 410352d to b33911b Compare July 28, 2026 19:00
@thymikee

Copy link
Copy Markdown
Member

P1: serialize sticky-comment writers by PR, not only by head SHA. The new --expect-head guard fetches the PR head, then lists comments before POST/PATCH; a push in that remaining window can still let the old renderer overwrite the newer sticky comment. The regression covers only a mismatch before the first GET. Key quality-delta concurrency by workflow_run.pull_requests[0].number with a safe SHA fallback, retain the late head check as defense in depth, and assert the concurrency/race policy in the wiring regression. Exact-head checks are green; fork/token handling otherwise looks sound.

…d sha

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

Key quality-delta concurrency by workflow_run.pull_requests[0].number with a safe SHA fallback, retain the late head check as defense in depth, and assert the concurrency/race policy in the wiring regression.

Done exactly that in 74e41a37a:

concurrency:
  group: quality-delta-${{ github.event.workflow_run.pull_requests[0].number || github.event.workflow_run.head_sha }}
  cancel-in-progress: true

So 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 --expect-head alone cannot close. The sha fallback only ever applies to fork triggers (pull_requests is empty there), which the job's head_repository guard skips anyway, so it can never merge two same-PR runs into one group.

--expect-head stays as defense in depth for the cancellation-propagation window, and the wiring regression in producers.test.ts now asserts the policy itself: the group expression contains workflow_run.pull_requests[0].number and the head_sha fallback, and cancel-in-progress: true, alongside the existing assertions that every post-gate step is gated, that posting consumes steps.producers.outputs.pr_number and --expect-head, and that no other workflow contains --post-comment.

(This push is also rebased onto current main after #1465 folded jobs into named scripts — the posting step now calls pnpm size --post-comment rather than node scripts/size-report.mjs.)

Copy link
Copy Markdown
Member

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 workflow_run renderer, a new orphan ref, and two workflows to serve a deferred track would add exactly the kind of standing surface the descope is removing.

If Track C is revived, this PR is the reference design to start from.


Generated by Claude Code

@thymikee thymikee closed this Jul 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

obs: evolve the size-report PR comment into a quiet quality-delta comment + metric history on main

1 participant