feat(improve-pr): the improvement radar measures first, and states its cost - #145
Merged
Conversation
…s cost The third of the maintenance loop's three use cases (org process/content/maintenance-loop.md §6), and a sibling of `spec-drift-pr`: same `completeStructured` engine, same CONFIG_KV-resolved backend, same container-only-for-git posture, same draft-PR output. One run with a `dimension` input rather than three runs. The dimensions differ in their measurement step and their rubric and share everything else — the estate, the backend, the floor, the PR shape, the suppression key. Three runs would be three copies of everything except the part that differs. **Measure first; the model only interprets.** A model asked "how could this CI be faster?" invents plausible advice. A model handed "0 steps use a cache action across 3 workflows" writes a specific PR. Each dimension runs a deterministic script in the container and only the measurement reaches the model, which is also what makes the finding checkable: a proposal whose numbers a reviewer cannot re-derive is an opinion. **Every finding states its cost.** `cost` is a required schema field beside `saving`. The schema cannot make a reviewer weigh them, but it can refuse to let a proposal omit the half that argues against itself — and a model asked only "how could this be better?" never volunteers it. Below the materiality floor (default 10%) a finding is a report line, never a PR. Three things I deliberately did NOT do, because the honest bound matters more than the impressive one: The measurement scripts do not install, build, or reach the network. So `size` reads the dependency surface and the largest tracked files rather than `wrangler deploy --dry-run` bundle bytes, and `ci-speed` reads the workflow CONFIGURATION that predicts slowness — no cache action, no concurrency group — rather than real per-step durations, which live in the GitHub Actions API and in our own `executions` rows and are wired into neither. The header says so. A dimension that cannot measure reports nothing rather than guessing, and the deterministic exit makes that free. `finops` is not a fourth member. `finops-audit` exists, runs Mondays, and reads Cloudflare account usage rather than a checkout; a member here shelling to the same data would be a second implementation of a run we have. And there is no `schedules` entry and no cron. §6 staggers one dimension per weekday across T1 and arming that is a product decision — a storm of agent runs is both a cost spike and an unreviewable pile of PRs. It ships dispatchable by hand, exactly as `triage-prs` and `triage-issues` do, and a test asserts the absence so the two halves of cron parity cannot drift apart silently. The suppression key names the finding's LOCATION, not the model's prose for it. §10 gap 11 records why: a model-authored key drifts between runs, so a `declined.jsonl` entry suppresses one spelling rather than one finding. A path is copied out of the measurement, so the same finding about the same file keys the same way next week whatever the model calls it. Tests: 7 covering the floor in both directions, an operator-lowered floor, the deterministic exit reaching no model, the unset-estate no-op, the unconfigured backend failing loudly, and the absent schedule. Full runs suite green (358).
![flaredispatch-fractalboxdev[bot]](https://avatars.githubusercontent.com/in/4273665?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI code review — 💬 Comment
Risk tier: full · 0 critical · 1 warnings · 1 suggestions
Reviewers: security
1. ⚠️ Warning — Documented suppression key is not implemented
The comments describe a stable suppression key and the PR body emits 'maintenance-key' values, but this run never reads or checks declined/suppression state before opening a PR. Either implement the documented suppression behavior or revise these comments and generated text so operators are not told that findings can be suppressed when they cannot.
2. 💡 Suggestion — Add operator-facing documentation for the new run
The new dispatchable run exposes several CONFIG_KV settings and three input dimensions, but those instructions exist only in source comments. Update the repository's run catalog/maintenance documentation with how to invoke 'improve-pr', valid dimensions, required backend/model configuration, and the intentional absence of a schedule so operators can discover and use it without reading TypeScript.
… run CI caught two things my typecheck did not, and both are the registry doing its job. Adding a run to `runs/index.ts` and `registry.ts` is three of the five places a run exists. `grant-catalog.test.ts` asserts an entry for EVERY registered run, so a run that ships without a declared egress grant fails the build rather than defaulting to one. That is the correct direction and it caught me. The entry is `["public-repo-read"]` — the narrow one, the same as `org-spec-audit` and deliberately not `spec-drift-pr`'s `JS_BUILD`. Every measurement script in this run is read-only: no install, no build, no network. A container that could reach a registry would be a grant wider than any use. That is the same decision as the honest measurement bound in the header — it is why `size` reports the dependency surface rather than built bundle bytes, and the grant and the measurement are two faces of one choice rather than a limitation I worked around. `GET /health` publishes the registered run names and its test pins the list, so the run is now in it, alphabetically. Worth recording why I did not catch these: I ran `tsc` against `apps/dispatcher/tsconfig.json` and the `runs` project's vitest suite, and reported "358 passed" from that. The repo's own gate is `pnpm typecheck && pnpm test` across every workspace, which is what `offload-test` runs, and it is where both failures live. A green subset is not a green suite, and I described one as the other. Full suite now: 174 files, 2292 passed, 1 skipped.
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
1 participant
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.
What
improve-pr— the third of the maintenance loop's three use cases (§6), and half of that doc's Gap 3. A sibling ofspec-drift-pr: samecompleteStructuredengine, same CONFIG_KV-resolved backend, same container-only-for-gitposture, same draft-PR output.One run with a
dimensioninput (ci-speed|security|size) rather than three runs — they differ in their measurement step and rubric and share everything else.Measure first; the model only interprets
A model asked "how could this CI be faster?" invents plausible advice. A model handed "0 steps use a cache action across 3 workflows" writes a specific PR. Each dimension runs a deterministic script in the container, and only the measurement reaches the model. That is also what makes a finding checkable — a proposal whose numbers a reviewer cannot re-derive is an opinion.
Every finding states its cost
costis a required schema field besidesaving. The schema can't make a reviewer weigh them, but it can refuse to let a proposal omit the half that argues against itself — and a model asked only "how could this be better?" never volunteers it. Below the materiality floor (default 10%) a finding is a report line, never a PR.What it deliberately does not do
The honest bound matters more than the impressive one, so the header says all of this:
sizereads the dependency surface and largest tracked files, notwrangler deploy --dry-runbundle bytes;ci-speedreads the workflow configuration that predicts slowness (no cache action, no concurrency group), not real per-step durations — those live in the Actions API and our ownexecutionsrows, and neither is wired in.finopsis not a fourth member.finops-auditexists, runs Mondays, and reads account usage rather than a checkout.No cron, on purpose
No
schedulesentry and nothing inwrangler.jsonc. §6 staggers one dimension per weekday across T1, and arming that is a product decision — a storm of agent runs is both a cost spike and an unreviewable pile of PRs. It ships dispatchable by hand, exactly astriage-prsandtriage-issuesdo, and a test asserts the absent schedule so cron parity cannot drift silently.Suppression key names the location, not the prose
§10 gap 11 records that a model-authored key drifts between runs, so a
declined.jsonlentry suppresses one spelling. A path is copied out of the measurement, so the same finding about the same file keys the same way next week whatever the model calls it.The other half of Gap 3 is blocked, and not on effort
upstream-upgrade-pris not in this PR and cannot be built as specified. §4 has it "query the brain for HIGH/MEDIUM items since the last run", and no run capability can read the store —packages/core/src/services/has no memory/context service. That is ADR-0002, which is unimplemented, and which the loop itself just filed as org#157. The gap row sizes both runs as "siblings ofspec-drift-pr"; that is true of this one and not of that one.Test
7 tests: the floor in both directions, an operator-lowered floor, the deterministic exit reaching no model, the unset-estate no-op, the unconfigured backend failing loudly, and the absent schedule. Full runs suite green — 358 passed. Typecheck clean.