Reviews: run 1 of the audit validation program (action-translation, 2026-07-28) - #22
Merged
Conversation
…026-07-28) The first execution of /audit:issues as a skill rather than as a hand-run runbook, recorded per #16's protocol. Result: the bundle is good and the plugin has seven defects (#21), one of them in the checkpoint fix that shipped the day before in #17. Eight of the ten review checks held, including the read-only boundary — git status stayed empty across a run that wrote 127 KB into the audited repo — and a coverage table that matches coverage.json on every number. Two findings are worth naming here because they cut against decisions this repo made recently. A [verified] citation on the audit's headline finding pointed at a commit that is not on main, which doctrine §2 permits because it never says to check reachability. And the run took ~23 minutes for 230 items, against docs promising "hours, not minutes" — which undercuts doctrine §4's stated premise that bulk audits outlive sessions, and so the stated justification for the checkpointing machinery. The honest correction is to keep the machinery and fix the premise, not to pretend the measurement did not happen. Checks 9 and 10 — is the tiering right, would you act on this — are the maintainer's and remain open. Resumability is still untested: the run was not interrupted, so the program's headline claim survives run 1 unvalidated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5 tasks
There was a problem hiding this comment.
Pull request overview
Adds the required run record for validation program run #1 of /audit:issues (executed as a skill) against QuantEcon/action-translation, capturing setup, measured cost, claim verdicts, and a defect list to support follow-up work tracked elsewhere.
Changes:
- Add a new validation run document under
reviews/for/audit:issuesrun 1 (2026-07-28). - Record measured phase timings, claim verdicts, and the seven defects found (with links to the tracking issue and related discussions).
Comments suppressed due to low confidence (1)
reviews/audit-run-action-translation-2026-07-28.md:58
- This link references doctrine §2 but points to the top of doctrine.md. Linking to the specific “Evidence classes” section improves checkability for readers following the citation.
1. **A `[verified]` citation that does not verify, on the headline finding.** The #91 wave-escape entry cites `b99b431` as where `docs/user/heading-maps.md`'s history ends. That commit is real and does touch the file — but it is **not on `main`**; it exists only on `origin/fix/heading-map-position-fallback`, unmerged, dated 2026-03-24. The last commit touching that path on `main` is `0ea2539`. The audit's own header says "verified against `main` @ `2c3d624`". The conclusion survives independent checking and is arguably strengthened, but a reviewer who checks the citation finds nothing — worse than no citation. [doctrine §2](../audit/references/doctrine.md) says cite `file:line`, a merged PR, or a tag; it never says *confirm the commit is an ancestor of the ref you named*. One `git merge-base --is-ancestor` closes it.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mmcky
added a commit
that referenced
this pull request
Jul 28, 2026
The relative form failed the links check because the file lives on #22's branch, not this one. Absolute is the repo's convention for cross-boundary references anyway (developing-skills § Conventions), and it resolves once #22 merges — matching how doctrine.md and audit/README.md already cite the same record. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
16 tasks
mmcky
added a commit
that referenced
this pull request
Aug 3, 2026
The relative form failed the links check because the file lives on #22's branch, not this one. Absolute is the repo's convention for cross-boundary references anyway (developing-skills § Conventions), and it resolves once #22 merges — matching how doctrine.md and audit/README.md already cite the same record. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mmcky
added a commit
that referenced
this pull request
Aug 3, 2026
…un as a skill Copilot review on #24. The cost sentence put "roughly 10 seconds per open issue" beside "a 230-item tracker with 56 open took 22 minutes" and left a reader to reconcile them. They do not reconcile: 56 x 10 s is 9 minutes. The run record has the decomposition — phase 2 verified the 56 open issues in ~9 minutes, and phases 1, 3 and 4 account for the rest. Both figures now say which question they answer: budget the total from the whole run, scale it by the open count, because that is what phase 2 walks. Corrected in all three places that state it — SKILL.md, audit/README.md and the tutorial — since a number restated three ways drifts three ways. The tutorial also claimed /audit:issues "has never been run as a skill, only by hand as a runbook". That was true when written and is not now: the run record this PR cites opens by describing itself as "the first execution of /audit:issues as a skill", on 2026-07-28, and it found seven plugin defects. The tutorial now says it has run once, links the record, and frames the reader's run as the next data point in the validation program rather than the first. While there: the tutorial told readers `claude plugin list` should report 0.1.2. That has been wrong since this branch bumped the version and would have gone wrong again at every future release, so it now points at the marketplace entry instead of naming a number — the same failure benchmark/README.md had twice. Not changed: the two run-record links Copilot flagged as 404s. They are correct and deliberate forward references — the record lands in #22, which merges first. The same "never yet as a skill" claim is also live on main, in CATALOG.md and docs/using-skills.md. That is repo-level rather than this PR's business and is being handled separately.
… citations Copilot review on #22, both comments valid, plus one they did not catch. The Cost table's "Completed" column carried bare clock times while Setup records the snapshot as 2026-07-28T06:45:58Z. The two are ten hours apart and look contradictory; they are the same instant in AEST. In a measurement record that ambiguity is not cosmetic, so the column now names its timezone. The total was wrong against the table's own timestamps. 16:45:59 to 17:08:05 is 22 minutes 6 seconds, and the per-phase elapsed figures sum to about the same, but the Total row said ~23 min. It matters beyond arithmetic: audit/README.md and #24 both cite "22 minutes" sourced from this record, so as written the citation disagreed with its evidence — the same defect this record criticises elsewhere, one level down. Corrected here and in the two places downstream that restate it as prose. Both doctrine citations pointed at the top of doctrine.md rather than the section being cited, which is a poor look in a document whose complaint #1 is a citation that does not verify. §2 is now #2-evidence-classes. §4 is anchored to #4-checkpointing, which is deliberately the post-#24 name. #24 renames that section from "Surviving a long run", and merges immediately after this — so the alternative anchor would be correct for minutes and wrong permanently. lychee runs without --include-fragments, so CI checks neither.
mmcky
added a commit
that referenced
this pull request
Aug 3, 2026
…duced (#24) * audit: re-derive doctrine §4 from evidence the first run actually produced Option A of #21 defect 4. The duration claim was wrong and it was load-bearing, so correcting the number is not enough — the justification it carried has to be replaced with one that survives measurement. **What was claimed.** doctrine §4 opened "Bulk audits outlive sessions. Context runs out, rate limits bite, machines sleep" and derived checkpointing from it; SKILL.md said "a hundred-issue repo is a multi-hour run — SO it works from a frozen snapshot"; README said "expect hours, not minutes, on a repo with a hundred items". Note the causal chain: the machinery was derived from the duration. **What was measured.** 230 items in 22 minutes, and none of §4's three named mechanisms occurred — context did not run out, the snapshot is two API calls in 11 seconds, and nothing slept. A second error sat underneath: cost tracks *open issues needing verification* (~10 s each), not total items, so README was measuring the wrong quantity as well as the wrong magnitude. **What replaces it.** Three reasons that hold at any duration, strongest first: the checkpoint is *evidence* — run 1's 56-of-56 coverage reconciliation was done against findings.md rather than the report, and the catalog was assembled from it; interruption is duration-independent; and the cost is asymmetric. The first is better than what it replaces precisely because it can be checked, which is what §1 rule 8 asks of every other claim in this document. §4 is renamed "Checkpointing" (its two inbound anchors updated) since the section is no longer about long runs. It also now states the criterion the new justification implies: a checkpoint superseded minutes later without being read earns nothing — which is the open question in #23, not settled here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Tutorial: absolute link to the run record, which lands in #22 The relative form failed the links check because the file lives on #22's branch, not this one. Absolute is the repo's convention for cross-boundary references anyway (developing-skills § Conventions), and it resolves once #22 merges — matching how doctrine.md and audit/README.md already cite the same record. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * audit 0.1.4: version the doctrine §4 re-derivation so it reaches installs Rebased onto main, which since #28 requires a version bump and a changelog entry for any change to a plugin's shipped files. This branch changes four of them. The rebase itself was clean, and quietly instructive. This branch and main had both moved audit 0.1.2 -> 0.1.3 — the same edit on both sides, so git merged it without a murmur and GitHub reported the PR MERGEABLE/CLEAN. Merged as it stood, it would have shipped four changed files under a version string already published from main, and every install would have been told it was up to date. That is precisely the failure the guard exists to catch, and it did: !! audit — 4 files of shipped content changed, version still '0.1.3' Hence 0.1.4. No content changed in this commit beyond the manifests and the changelog entry describing what the two commits below already do. * audit: separate the two cost measures, and stop saying it has never run as a skill Copilot review on #24. The cost sentence put "roughly 10 seconds per open issue" beside "a 230-item tracker with 56 open took 22 minutes" and left a reader to reconcile them. They do not reconcile: 56 x 10 s is 9 minutes. The run record has the decomposition — phase 2 verified the 56 open issues in ~9 minutes, and phases 1, 3 and 4 account for the rest. Both figures now say which question they answer: budget the total from the whole run, scale it by the open count, because that is what phase 2 walks. Corrected in all three places that state it — SKILL.md, audit/README.md and the tutorial — since a number restated three ways drifts three ways. The tutorial also claimed /audit:issues "has never been run as a skill, only by hand as a runbook". That was true when written and is not now: the run record this PR cites opens by describing itself as "the first execution of /audit:issues as a skill", on 2026-07-28, and it found seven plugin defects. The tutorial now says it has run once, links the record, and frames the reader's run as the next data point in the validation program rather than the first. While there: the tutorial told readers `claude plugin list` should report 0.1.2. That has been wrong since this branch bumped the version and would have gone wrong again at every future release, so it now points at the marketplace entry instead of naming a number — the same failure benchmark/README.md had twice. Not changed: the two run-record links Copilot flagged as 404s. They are correct and deliberate forward references — the record lands in #22, which merges first. The same "never yet as a skill" claim is also live on main, in CATALOG.md and docs/using-skills.md. That is repo-level rather than this PR's business and is being handled separately. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 3, 2026
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.
The run record required by #16 — the first execution of
/audit:issuesas a skill, againstQuantEcon/action-translation(230 items). Defects filed separately as #21; the.dev/audits/convention question as QuantEcon/QuantEcon.manual#140.What held
Eight of the ten review checks. The two that matter most for trusting a long report both held cleanly: the coverage table matches
coverage.jsonon every number, with a six-item residue and an explicit "bounds this audit placed on itself: none by sampling"; and the read-only boundary held —git statusstayed empty across a run that wrote 127 KB into the audited repo, which is the discovery-ordered working directory from #19 doing exactly what it claimed.I also independently re-verified the audit's headline finding rather than taking it on trust: PR #197 provably never touched the file it was supposed to fix, so the wave escape is real.
What broke
A
[verified]citation that does not verify — on that same headline finding. It cites a commit that is real, does touch the right file, and is not onmain; it lives on an unmerged branch, while the audit's header says "verified againstmain". The conclusion survives; the citation does not, and a tag that invites trust while pointing at nothing is worse than no tag. doctrine §2 permits this because it never says to check reachability.Half of phase 2 was never checkpointed.
findings.mdheld the 56 open issues; the 62 closed ones went straight to the catalog. That is a defect in #17 — my fix — and it would have broken the resume rule had the run died in the closed-set pass.The finding that cuts against this repo's own decisions
~23 minutes for 230 items, against docs promising "hours, not minutes, on a repo with a hundred items". doctrine §4 justifies the whole checkpointing apparatus on "bulk audits outlive sessions" — at 23 minutes they do not.
I argued for that machinery, so to be explicit about what the measurement costs: the machinery should stay (a 1000-item tracker scales up, interruption remains possible, and the incremental log proved independently useful as an audit trail), but it can no longer be justified by a premise the first measurement contradicts. Fixing the premise is the honest move; quietly keeping it is not.
The gap run 1 did not close
Resumability is still untested. Tutorial step 4 asks the operator to interrupt phase 2 and restart; this run went straight through. The checkpoint provably materialises — it did not exist at all before #17 — but resuming from it remains unvalidated, which is the program's headline claim. Run 2 should be interrupted deliberately.
Checks 9 and 10 — is the tiering right, would you act on this — are the maintainer's and are marked pending rather than answered.
🤖 Generated with Claude Code