Repository navigation
🤖 feat: DRY gate (make dry-check, Quality / DRY) on unclebob/dryer - #7147
Conversation
Reports near-duplicate functions that a change makes: each changed function against every production function, with dryer fingerprints and Crapper names. Warn-only. Moves the diff code of crap/check.ts into scripts/crap/changes.ts so both gates agree on what changed.
A new copy that shares a name with an old match (two render methods) was read as the old function and hidden.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
ThomasK33
changed the title
🟢 e2e stories (Haiku): 8 passed21 agent steps · 7 replayed from cache · 24 model calls · 196.1k tokens (74% cached) All 8 tests in 8 files
e2e 0.20.0 · 2m 39s · web · run artifacts 🟢 e2e Electron stories (Haiku): 3 passed3 agent steps · 3 model calls · 8.2k tokens (32% cached) All 3 tests in 3 files
e2e 0.20.0 · 24.4s · electron · run artifacts 🟢 e2e agent repros (Haiku): no tests selectedSpend: $0.00 of the $1.00 cap (proxy ledger) e2e 0.20.0 · 7.4s · 2 targets (web, phone) · run artifacts |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5c0e9e3cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review round 1 on #7147: a function moved into another file (or out of a deleted one) now stands for the removed base form it matches, so its old pairs stay quiet. Two head forms that map to one base form are a new pair. Naming parses each reported file once.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee0389e04a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review round 2 on #7147: a pure rename from a test path to a production path has no hunks, so the gate touched none of its functions and missed their duplicates. The whole head file now counts as changed.
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f0ffc3b8f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… skips hunk bodies Review round 4 on #7147: - Two old duplicates moved together (no rename) can each stand for a different removed base form, so their pair stays quiet. - diffHeaders() skips each hunk's body lines by count, so an added line `++ counter;` (rendered `+++ counter;`) is not a file header. Both parseChangedLines (also used by the CRAP gate) and parseHunks use it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55c0ad187d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review round 5 on #7147: two removed duplicates and three moved copies hid the third copy, because each pair picked its own bases. Now, in head order, each moved form claims its best unclaimed removed form, so the copy left without one is new.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62700a57a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…k-context=0 Review round 6 on #7147: with diff.interHunkContext set, git -U0 merges nearby hunks with context lines. diffHeaders counted them twice and could swallow the next file's headers. It now counts body lines per side, and changesSince pins --inter-hunk-context=0.
|
Review loop paused at the round cap (6 of 6 Codex rounds on this PR). Round 6 left two P2 threads:
The maintainer decides how to continue: a waiver for one more round (maximum-cardinality matching plus the fix above), a smaller scope for move detection, or a tracked follow-up. Generated with |
Review rounds 4 to 6 on #7147 each found a new case in matching moved forms to removed ones (ties, multiplicity, maximum matching). The user chose to reduce scope: a form made only of added lines is new, so a function moved into another file reports its old pairs again. The gate warns by default, and the gauntlet skill states the limit.
|
The maintainer chose to reduce scope (Option B) and approved one more review round past the cap for this change. The next push removes cross-file move detection from the DRY gate and carries the diff-parser fix 8692e50. A finding in that round stops the loop. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28ba71f4ec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Review loop stopped. The waived round on 28ba71f found one new P2: a function whose every line is replaced (a names-only rewrite) loses its same-file counterpart, so its old pairs show as new DRY warnings. That is a false positive in a gate that only warns by default. I did not fix it, because the waiver allowed one round and a finding in it ends the loop. The thread stays open for the maintainer's decision. Generated with |
Review round 7 on #7147: when a hunk replaced every line of a function (names and literals only, same shape), no head line mapped to the base, so its old pairs showed as new. Lines in a replacement hunk now map by position when nothing else maps. Pure additions still have no base.
|
The maintainer approved one more review round past the cap (Option A). The next push fixes the open #7147 finding (a function rewritten line for line keeps its DRY base form) and merges it up the stack. A finding in this round stops the loop. |
# Conflicts: # docs/AGENTS.md # src/node/services/agentSkills/builtInSkillContent.generated.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22475b6a73
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ebob/mutator (coder#7148) ## Summary Adds a mutation gate. `make mutate-check` (and a warn-only `Quality / Mutation` CI job) makes small changes ("mutants") in the functions a PR changed, such as `===` to `!==`, `>` to `>=` or `??` to `||`. It then lists the mutants that no unit test catches. The mutation sites come from Uncle Bob's [mutator](https://github.com/unclebob/mutator). Xum runs the mutants itself. Stack 2 of 3, on coder#7147. ## Background A mutant that survives means no test checks that behavior. The first run found a real gap: `parseGoalTurnCapInput` had no direct test, and all 5 of its mutants survived. coder#7151 adds that test. ## Implementation - **Why not the upstream runner:** the `mutator` command fails on this repo with ENAMETOOLONG, because its import scan reads a string in a source file as a path. It also follows only relative imports, so with Xum's `@/` imports a mutant can run against the original file. `scripts/mutate/mutate_report.py` uses mutator's `discover_raw`/`apply_site` with UTF-8 byte offsets and runs them in its own workers. - **Workers:** each worker owns a private copy of the tracked files, with `node_modules` linked and `src/version.ts` generated. The real checkout is never touched. - **Tests per mutant:** the colocated test runs first, then the nearest other unit-lane tests that import the file, at most 10. Each test file runs in its own `bun test` process. Several Xum test files in one process break each other on shared temp config. A failing file or a timeout (10× the file's baseline, at least 2 s) kills the mutant. The runner kills the whole process group. - **Baselines:** each test file must pass alone first. A file that fails is dropped and listed, and the run exits 3. - **Crash handling:** a crash in one mutant becomes one `error` row. If the crash left the worker's copy mutated, the runner retires that worker. - **CI:** the job waits for Test / Unit and reuses its LCOV, so sites on lines no unit test ran are skipped. The job stops at 300 mutants or 15 minutes. The deadline also cuts a running test, and such a mutant reports `not-run`. `Required` does not list the job. ## Validation On the real repo: - I touched `parseGoalTurnCapInput`. Exactly its 5 mutants survived, with exit 1. - With a test added, all 5 were killed, with exit 0. - With a broken test, the report said `BASELINE FAILED`, with exit 3. - I touched `agentMessageEnvelope.ts`, which 10 unit tests import. Its 4 mutants were killed in 54 s at a host load average of about 200. The end-to-end tests in throwaway repos run real `bun` processes, including an `@/` import kill. I confirmed that the crash tests and the survivor test fail when the code under test is broken. ## Risks Low for merges, because the job only warns and `Required` does not wait for it. The job adds runner minutes on every PR that changes `src/` or CI config. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$12.44`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=12.44 -->
Summary
Adds a DRY gate next to the CRAP gate.
make dry-check(and a new warn-onlyQuality / DRYCI job) reports near-duplicate functions that a change creates. It compares each changed function with every production function insrc/, using structural fingerprints from Uncle Bob's dryer. A pair that already matched on main stays quiet.Stack 1 of 3. #7148 adds the mutation gate, and #7149 adds accept markers and an opt-in block mode.
Background
Uncle Bob's "gauntlet" runs three tools: crapper (complexity × coverage), dryer (duplication) and mutator (mutation testing). Xum already runs Crapper (
scripts/crap/). This PR adds dryer, and #7148 adds mutator.Implementation
scripts/dry/dryer_report.py(uv script, pins dryer66ff6d2, plus the Crapper and grammar pins that CRAP uses). Upstreamdryercompares every function with every other one, and that run did not finish in 15 minutes onsrc/(about 14,000 functions). This script compares only the changed functions with all the others. Extraction takes about 19 s of CPU, plus about 0.13 s per changed function.-U0hunks, not by name. A function made only of added lines has no base version. Matching by name hid a new copy that shared a name with an old match, for example tworendermethods. Regression test:a new copy that shares a name with an old match is a finding.scripts/crap/changes.ts: the diff code moves out ofcrap/check.tsso both gates agree on what "changed" means. CRAP behavior does not change: its 61 script tests pass unchanged.Makefile:dry-check,test-dry-scripts(instatic-check-full), and a sharedtree-sitter-grammarsdownload step with retries..github/workflows/pr.yml:Quality / DRY. It needs onlychanges, so it starts at once. Every step is soft.Requireddoes not list it..xum/skills/gauntlet/SKILL.mdand one AGENTS.md line: how to read and repair findings.Validation
On the real repo:
centsBucketasfeeBucketinworkspaceGoalService.ts.make dry-checkreported 2 warnings at score 1.00, with exit 1.centsBucket. The check passed, because that pair is old.I confirmed that the new-pair test and the same-name test fail on the old rule and pass on the new one. A 200-function sample at the 0.82 threshold found that 7.5% of functions have a near-twin.
Risks
Low. The gate only warns and lives in
scripts/. The CRAP refactor is the one shared path, and its tests cover it.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$12.44