From f6d658f4ba4296eb09df15d3b1a6a60a33e24453 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Sun, 2 Aug 2026 12:45:59 -0400 Subject: [PATCH 1/5] test(bestax-migrate): pin the Babel 7 rationale and guard legacy syntax MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dependabot #430 proposed @babel/parser 8.0.4. It is not takeable: three of the options in runner.ts's PARSER_OPTIONS are invalid under Babel 8, and the first breaks every run rather than only legacy input. - pipelineOperator: { proposal: 'minimal' } — Babel 8 accepts only fsharp and hack, and validates the options object before reading any source, so every js/jsx/ts/tsx file throws. cli.ts catches per file, so a run would report an error for each one and migrate nothing. - deprecatedImportAssert — removed with no replacement, so the legacy assert { type: 'json' } form stops parsing. - declare module M {} — rejected in 8.0.4 (babel/babel#18120); older TypeScript still uses the namespace form. The v2.0.0 CHANGELOG already recorded the deprecatedImportAssert half of this; the header comment now names all three so the next reader sees why the major is pinned rather than rediscovering it. The parser config had one regression test (import-assert) and nothing covering the rest of the plugin list. Adds two: legacy decorators + class properties + private fields + numeric separators, and the React.createClass form. Both are shapes an old codebase being migrated actually contains. --- bestax-migrate/src/runner.ts | 17 +++++++++ .../__tests__/transform.test.ts | 35 +++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/bestax-migrate/src/runner.ts b/bestax-migrate/src/runner.ts index a1eeb3b36..2d4d793f7 100644 --- a/bestax-migrate/src/runner.ts +++ b/bestax-migrate/src/runner.ts @@ -14,6 +14,23 @@ import type { TodoCollector, Transform } from './types.js'; * jscodeshift's stock `tsx` parser options plus `deprecatedImportAssert`, * so files using the legacy `import x from 'y' assert { type: 'json' }` * syntax (still emitted by many tools) parse instead of crashing the run. + * + * This config pins @babel/parser to 7.x — do not take the 8.x major. Three of + * the options below are invalid under Babel 8, and the first breaks every run + * rather than only legacy input: + * + * - `pipelineOperator: { proposal: 'minimal' }` — Babel 8 accepts only + * `fsharp` and `hack`, and rejects the options object before reading any + * source, so *every* js/jsx/ts/tsx file fails to parse. + * - `deprecatedImportAssert` — removed with no replacement, taking the + * legacy `assert { type: 'json' }` form with it (see the regression test + * "parses the legacy import-assert syntax"). + * - `declare module M {}` — rejected in 8.0.4 (babel/babel#18120), which + * older TypeScript still uses. + * + * A codemod that migrates older codebases must not crash on the syntax those + * codebases still contain. jscodeshift 17 bundles its own Babel 7 regardless, + * so staying on 7 also keeps a single parser in the tree rather than two. */ const PARSER_OPTIONS: ParserOptions = { sourceType: 'module', diff --git a/bestax-migrate/src/sources/react-bulma-components/__tests__/transform.test.ts b/bestax-migrate/src/sources/react-bulma-components/__tests__/transform.test.ts index 3513c5f7a..139973e73 100644 --- a/bestax-migrate/src/sources/react-bulma-components/__tests__/transform.test.ts +++ b/bestax-migrate/src/sources/react-bulma-components/__tests__/transform.test.ts @@ -87,6 +87,41 @@ describe('react-bulma-components transform fixtures', () => { expect(output).toContain("assert { type: 'json' }"); }); + it('parses legacy class, decorator and proposal syntax', () => { + const source = [ + "import { Button } from 'react-bulma-components';", + '@observer', + 'export class App extends React.Component {', + ' state = { count: 1_000 };', + ' #private = null;', + ' static defaultProps = {};', + ' render() {', + ' return ;', + ' }', + '}', + '', + ].join('\n'); + const { output } = runTransform(transform, 'legacy.tsx', source); + expect(output).toContain('from "@allxsmith/bestax-bulma"'); + expect(output).toContain('@observer'); + expect(output).toContain('state = { count: 1_000 }'); + }); + + it('parses the legacy React.createClass form', () => { + const source = [ + "import { Button } from 'react-bulma-components';", + 'module.exports = React.createClass({', + ' render: function () {', + ' return ;', + ' },', + '});', + '', + ].join('\n'); + const { output } = runTransform(transform, 'create-class.tsx', source); + expect(output).toContain('from "@allxsmith/bestax-bulma"'); + expect(output).toContain('isLoading'); + }); + it('collects TODO entries with rules and line numbers', () => { const source = [ "import { Tile } from 'react-bulma-components';", From 3f48a22114f113f139893dc6c2b618784807193d Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Sun, 2 Aug 2026 12:58:58 -0400 Subject: [PATCH 2/5] ci: pin @babel/parser to the 7.x major for bestax-migrate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dependabot reopens the 8.x bump weekly (#430), and it is not takeable: three of the options in bestax-migrate's PARSER_OPTIONS are invalid under Babel 8, and `pipelineOperator: { proposal: 'minimal' }` fails validation before any source is read — so every js/jsx/ts/tsx file throws and the codemod migrates nothing. Full analysis in #430 and #458. Scoped to the major only; minor and patch updates still flow through the production-dependencies group. --- .github/dependabot.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 9f64837ac..7e12f5a10 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -21,6 +21,12 @@ updates: update-types: - minor - patch + ignore: + # bestax-migrate parses legacy syntax on purpose (see bestax-migrate/src/runner.ts). + # Babel 8 removes `deprecatedImportAssert` and the `minimal` pipeline proposal, and + # rejects `declare module M {}` — all of which this codemod must keep accepting. + - dependency-name: "@babel/parser" + update-types: ["version-update:semver-major"] labels: - dependencies - javascript From 8e8377548047cb3014981339ba80ed5aa4217695 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Sun, 2 Aug 2026 13:07:30 -0400 Subject: [PATCH 3/5] docs(.github): tier the agent-edit rule by blast radius MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The contract said "propose a diff in a comment, do not apply one" for all of .github/**. That blanket line was cheap to state but it spent maintainer attention on changes with no security surface — an interactive session stalled across three asks over a dependabot.yml ignore entry — which is attention not available for the allowlist case that actually needs it. Splits the two enforcement layers that were being conflated. The mechanical denial on automated sessions (--disallowedTools, the fix loop's .github/** refusal) is unchanged and stated as non-negotiable: those sessions ingest untrusted issue and PR text. For an interactive session working with a maintainer, the guidance is now to surface the reason, confirm it has been weighed, and act on the answer. Bakes in the reason itself so it travels to every contributor's session instead of living in one person's local context: a change here can grant capability with no permissions diff for a reviewer to notice. The allowlist row keeps a hard floor — argue it every time, name the credential, never as a side effect of another task — so tiering the rule cannot be read as licence to widen a confinement boundary quietly. --- .github/CLAUDE.md | 36 ++++++++++++++++++++++++++++++++---- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/.github/CLAUDE.md b/.github/CLAUDE.md index eae54f8a0..130b2499a 100644 --- a/.github/CLAUDE.md +++ b/.github/CLAUDE.md @@ -1,9 +1,37 @@ # .github — CI and AI automation -Everything in `.github/workflows/` is **human-authored by design**. AI agents working in this -repository are denied write access to this directory (`Edit(.github/**)` and friends sit in -every write agent's `--disallowedTools`), and the AI fix loop refuses any PR that touches -`.github/**`. If you are an agent reading this: propose a diff in a comment, do not apply one. +Everything in `.github/workflows/` is **human-authored by design**. Two enforcement layers apply +here and they are not the same rule — do not collapse them: + +- **Automated sessions** (the AI fix loop, `ai-triage`, `ai-scan`) are denied write access + _mechanically_: `Edit(.github/**)` and friends sit in every write agent's `--disallowedTools`, + and the fix loop refuses any PR that touches `.github/**`. This is not advisory, and editing + this file does not relax it. Those sessions ingest untrusted issue and PR text, so for them the + prohibition is absolute. +- **An interactive session working alongside a maintainer** should not hide behind that. State + the rule, state the security reason below that it exists for, make sure the maintainer has + actually weighed that reason — then act on their answer. Refusing to apply a change the + maintainer has decided on does not make the repository safer; it just turns one decision into + several round-trips. The maintainer was always the one deciding. + +**The reason to raise, so nobody has to rediscover it:** a change in this directory can grant +capability with **no permissions diff for a reviewer to notice**. The `permissions:` block looks +identical before and after an allowlist widens. That is why these changes get argued rather than +waved through — not because agents are untrusted, but because the usual review signal is absent. + +Scale the pushback to the blast radius: + +| Change | What to do | +| ----------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| `--allowedTools` / `--disallowedTools` on a session holding a credential | **Argue it properly, every time.** Name the credential in that job, explain why each entry cannot write, and get an explicit decision. Never as a side effect of another task. See rule 2. | +| Workflow logic, triggers, `permissions:`, action SHAs, verdict/budget paths | Name the invariant at stake (I1, I2, or the rule below), confirm it holds, then act. | +| Config with no security surface — `dependabot.yml` ignores, labels, schedules | Just make the change and say what it does. Do not gate it behind a debate. | + +Origin: an interactive session hit the old blanket "propose a diff, do not apply one" line while +adding a `dependabot.yml` ignore entry — a change with no security surface at all — and stalled +on it across three separate asks. The blanket rule was cheap to state but it spent the +maintainer's attention on the one case that did not need it, which is attention not available for +the allowlist case that does. This file is the security contract for these workflows. The rules below are not style preferences — each one is load-bearing, and most were written after a review round or a From f35ce7b4dc3bc66a3be4a25f13c590e95f2cf599 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Sun, 2 Aug 2026 14:18:29 -0400 Subject: [PATCH 4/5] test(bestax-migrate): address review findings on the parser pin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claude deep review, advisory on .github/CLAUDE.md: the "no security surface" row called dependabot.yml ignores safe in general, which is only true of the semver-major form this PR adds. An ignore covering patch or minor suppresses CVE fixes for that dependency. Narrows the row to semver-major and states the distinction, so the row cannot license a future ignore that is not safe. CodeRabbit, on the new tests: the fixtures contained #private, optional chaining, nullish coalescing and a static class property, but nothing asserted they survived — only the import, decorator and state field were checked. A plugin dropped from PARSER_OPTIONS would still parse a file whose syntax has since become standard, while recast quietly lost the node, so the tests would not have caught it. Asserts every proposal shape in the fixture round-trips. Also adds the negative assertion CodeRabbit asked for: the createClass test only checked that isLoading appeared, which passes just as well if the rename duplicated the prop instead of replacing it. Verified the pattern matches a leftover `loading` in attribute, valued and self-closing positions, and does not match `isLoading`. --- .github/CLAUDE.md | 15 ++++++++++----- .../__tests__/transform.test.ts | 10 ++++++++++ 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/.github/CLAUDE.md b/.github/CLAUDE.md index 130b2499a..88f25d5b0 100644 --- a/.github/CLAUDE.md +++ b/.github/CLAUDE.md @@ -21,11 +21,16 @@ waved through — not because agents are untrusted, but because the usual review Scale the pushback to the blast radius: -| Change | What to do | -| ----------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -| `--allowedTools` / `--disallowedTools` on a session holding a credential | **Argue it properly, every time.** Name the credential in that job, explain why each entry cannot write, and get an explicit decision. Never as a side effect of another task. See rule 2. | -| Workflow logic, triggers, `permissions:`, action SHAs, verdict/budget paths | Name the invariant at stake (I1, I2, or the rule below), confirm it holds, then act. | -| Config with no security surface — `dependabot.yml` ignores, labels, schedules | Just make the change and say what it does. Do not gate it behind a debate. | +| Change | What to do | +| -------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| `--allowedTools` / `--disallowedTools` on a session holding a credential | **Argue it properly, every time.** Name the credential in that job, explain why each entry cannot write, and get an explicit decision. Never as a side effect of another task. See rule 2. | +| Workflow logic, triggers, `permissions:`, action SHAs, verdict/budget paths | Name the invariant at stake (I1, I2, or the rule below), confirm it holds, then act. | +| Config with no security surface — labels, schedules, `semver-major` `dependabot.yml` ignores | Just make the change and say what it does. Do not gate it behind a debate. | + +The `dependabot.yml` qualifier in that last row is load-bearing: a `semver-major` ignore only +declines a breaking upgrade, but an ignore covering **patch or minor suppresses CVE fixes** for +that dependency and belongs in the middle row, argued on its merits. Scope every ignore you add, +and never widen an existing one's `update-types` as a drive-by. Origin: an interactive session hit the old blanket "propose a diff, do not apply one" line while adding a `dependabot.yml` ignore entry — a change with no security surface at all — and stalled diff --git a/bestax-migrate/src/sources/react-bulma-components/__tests__/transform.test.ts b/bestax-migrate/src/sources/react-bulma-components/__tests__/transform.test.ts index 139973e73..b81050fe9 100644 --- a/bestax-migrate/src/sources/react-bulma-components/__tests__/transform.test.ts +++ b/bestax-migrate/src/sources/react-bulma-components/__tests__/transform.test.ts @@ -103,8 +103,15 @@ describe('react-bulma-components transform fixtures', () => { ].join('\n'); const { output } = runTransform(transform, 'legacy.tsx', source); expect(output).toContain('from "@allxsmith/bestax-bulma"'); + // Every proposal-plugin shape in the fixture must survive the round trip, + // not merely parse — a plugin dropped from PARSER_OPTIONS would still + // parse the file if the syntax has since become standard, but recast + // would lose the node. expect(output).toContain('@observer'); expect(output).toContain('state = { count: 1_000 }'); + expect(output).toContain('#private = null'); + expect(output).toContain('static defaultProps = {}'); + expect(output).toContain('this.props.a?.b ?? 0'); }); it('parses the legacy React.createClass form', () => { @@ -120,6 +127,9 @@ describe('react-bulma-components transform fixtures', () => { const { output } = runTransform(transform, 'create-class.tsx', source); expect(output).toContain('from "@allxsmith/bestax-bulma"'); expect(output).toContain('isLoading'); + // The rename must replace the legacy prop, not sit alongside it. + expect(output).not.toMatch(/\sloading[\s=/>]/); + expect(output).toContain('React.createClass'); }); it('collects TODO entries with rules and line numbers', () => { From a4cf6f9eaba596815745bfd416f6a03bd42e5af6 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Sun, 2 Aug 2026 14:44:41 -0400 Subject: [PATCH 5/5] docs(.github): stop exempting security labels and schedules from debate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit, Major, on the row this PR added: labels and schedules were listed as having no security surface, and neither is uniformly inert. `needs-security-review` is a refusal gate — grep confirms five workflows read it (ai-scan, claude, claude-implement, claude-repro, bestaxbot-reply), and each declines a flagged item until a maintainer clears it. `ai-loop`, `ai-loop-paused` and `deep-review` steer the loop across seven more. Deleting or renaming any of them disables a control with no workflow diff at all, which is the exact hazard the section above exists to name. A schedule on ai-scan or the stale sweep decides when a security control runs. This is the same over-generalization the previous commit fixed for dependabot ignores, left behind in the same row. The dividing line is cosmetic vs. load-bearing, not the file the change lives in, so the row now says that outright and the middle row absorbs anything a workflow reads as a gate or trigger. Adds the check that settles it: grep the workflows for what you are changing; if anything reads it, it is middle row. --- .github/CLAUDE.md | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/.github/CLAUDE.md b/.github/CLAUDE.md index 88f25d5b0..cdbb5970e 100644 --- a/.github/CLAUDE.md +++ b/.github/CLAUDE.md @@ -21,16 +21,29 @@ waved through — not because agents are untrusted, but because the usual review Scale the pushback to the blast radius: -| Change | What to do | -| -------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -| `--allowedTools` / `--disallowedTools` on a session holding a credential | **Argue it properly, every time.** Name the credential in that job, explain why each entry cannot write, and get an explicit decision. Never as a side effect of another task. See rule 2. | -| Workflow logic, triggers, `permissions:`, action SHAs, verdict/budget paths | Name the invariant at stake (I1, I2, or the rule below), confirm it holds, then act. | -| Config with no security surface — labels, schedules, `semver-major` `dependabot.yml` ignores | Just make the change and say what it does. Do not gate it behind a debate. | - -The `dependabot.yml` qualifier in that last row is load-bearing: a `semver-major` ignore only -declines a breaking upgrade, but an ignore covering **patch or minor suppresses CVE fixes** for -that dependency and belongs in the middle row, argued on its merits. Scope every ignore you add, -and never widen an existing one's `update-types` as a drive-by. +| Change | What to do | +| --------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| `--allowedTools` / `--disallowedTools` on a session holding a credential | **Argue it properly, every time.** Name the credential in that job, explain why each entry cannot write, and get an explicit decision. Never as a side effect of another task. See rule 2. | +| Workflow logic, triggers, `permissions:`, action SHAs, verdict/budget paths, anything a workflow reads as a gate or trigger | Name the invariant at stake (I1, I2, or the rule below), confirm it holds, then act. | +| Genuinely inert config — a cosmetic label, a docs-build cadence, a `semver-major` `dependabot.yml` ignore | Just make the change and say what it does. Do not gate it behind a debate. | + +The qualifiers in that last row are load-bearing. The dividing line is **cosmetic vs. +load-bearing**, not the file the change lives in: + +- **Labels are not uniformly inert.** `needs-security-review` is a refusal gate — `claude-repro`, + `claude-fix`, `@claude` and `@bestaxbot` all decline a flagged item until a maintainer clears + it. `ai-loop`, `ai-loop-paused` and `deep-review` steer automation the same way. Deleting or + renaming one of those disables a control **with no workflow diff at all**, which is the exact + hazard this section exists to name. A new `documentation` label is inert; a label a workflow + reads is middle row. +- **Schedules are not uniformly inert.** A `schedule:` on `ai-scan` or the stale sweep decides + when a security control runs. A docs-build cadence does not. +- **`dependabot.yml` ignores are not uniformly inert.** A `semver-major` ignore only declines a + breaking upgrade, but one covering **patch or minor suppresses CVE fixes** for that dependency. + Scope every ignore you add, and never widen an existing one's `update-types` as a drive-by. + +When unsure which row a change belongs in, grep the workflows for the thing you are changing. If +anything reads it, it is middle row. Origin: an interactive session hit the old blanket "propose a diff, do not apply one" line while adding a `dependabot.yml` ignore entry — a change with no security surface at all — and stalled