diff --git a/.changeset/autofix-docs-scope.md b/.changeset/autofix-docs-scope.md new file mode 100644 index 00000000..4dfe6e9a --- /dev/null +++ b/.changeset/autofix-docs-scope.md @@ -0,0 +1,13 @@ +--- +"autofix": minor +--- + +Add `docs` to the scope axis: `autofix: docs` / `/autofix docs` fixes only the `documentation` reviewer's threads, selected by the label that reviewer mints (`suggestion (non-blocking, documentation)`). + +It is a **subset of `nits`, not a peer of it**. Documentation findings are non-blocking, so `nits` already covers them and arming both is the same as arming `nits`; the containment runs one way only, and `docs` exists because arming `nits` to clear three stale comments also invites the fixer into every other cosmetic thread on the PR. The flat token namespace cannot express that, so `scope.ts` and the README both say it. `findingLabelsForScope` becomes an exhaustive switch rather than a blocking/non-blocking ternary, and the tests pin the containment and the absence of any blocking label from docs scope. + +`docs` is **not loop-eligible**, which is worth stating because it looks like the exception to "nits never loop" and is only half one: its deletion half converges (a comment that restates the code is either gone or not), while the documentation reviewer's *missing explanation* findings are answered with prose, and prose can always be wanted better. Ineligible until something measures which half dominates. + +One prompt rule comes with it: a documentation item changes text, never code. Deleting a redundant comment is the expected fix (and such findings often carry no suggestion block, since a deletion cannot be expressed as one), but if the honest fix would touch an executable line the item is left unfixed and reported rather than becoming a code change wearing a documentation label. That property — edits that cannot alter behavior — makes `docs` the safest scope to trial first in a repo new to autofix. + +Note the version coupling: autofix selects threads by parsing the label off each posted comment, so this scope finds threads only in repos whose **installed** `review` release mints the documentation label. Against an older reviewer it is not broken, just always empty. diff --git a/.github/workflows/autofix.lock.yml b/.github/workflows/autofix.lock.yml index c6157a16..cdb06e00 100644 --- a/.github/workflows/autofix.lock.yml +++ b/.github/workflows/autofix.lock.yml @@ -1,4 +1,4 @@ -# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"20eb43d1e2146e94b193e6a6fdf585ee6653e72335ebb8f78ad8a881d1d9b036","body_hash":"4bfba4fa8baaf015b86831cbb1ae4502d5f60a2e032429fcc00c809bf902638d","compiler_version":"v0.83.4","strict":true,"agent_id":"claude","agent_model":"claude-opus-4-8","engine_versions":{"claude":"2.1.220"}} +# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"2deaf40d575bb267e56fb604ecc727f5115c24257acdff58dfa2e238ae86f7cd","body_hash":"6edccc5b226cc23eaf036f59719c08cced517c5d54322cbf66cf35719c3c45e9","compiler_version":"v0.83.4","strict":true,"agent_id":"claude","agent_model":"claude-opus-4-8","engine_versions":{"claude":"2.1.220"}} # gh-aw-manifest: {"version":1,"secrets":["ANTHROPIC_API_KEY","COPILOT_GITHUB_TOKEN","GH_AW_CI_TRIGGER_TOKEN","GH_AW_GITHUB_MCP_SERVER_TOKEN","GH_AW_GITHUB_TOKEN","GITHUB_TOKEN","KHAN_ACTIONS_BOT_TOKEN"],"actions":[{"repo":"actions/cache/restore","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/cache/save","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/checkout","sha":"3d3c42e5aac5ba805825da76410c181273ba90b1","version":"v7.0.1"},{"repo":"actions/checkout","sha":"93cb6efe18208431cddfb8368fd83d5badbf9bfd","version":"93cb6efe18208431cddfb8368fd83d5badbf9bfd"},{"repo":"actions/download-artifact","sha":"3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c","version":"v8.0.1"},{"repo":"actions/github-script","sha":"3a2844b7e9c422d3c10d287c895573f7108da1b3","version":"v9.0.0"},{"repo":"actions/setup-node","sha":"820762786026740c76f36085b0efc47a31fe5020","version":"v7.0.0"},{"repo":"actions/upload-artifact","sha":"043fb46d1a93c77aae656e7c1c64a875d1fc6a0a","version":"v7.0.1"},{"repo":"github/gh-aw-actions/setup","sha":"e89c65e17eb281bbd5ff2ff9e9199a03e96654c7","version":"v0.83.4"}],"containers":[{"image":"ghcr.io/github/gh-aw-firewall/agent:0.27.42","digest":"sha256:26a8af4e5566485b02f52af59ee03803ae798271a9619d4767e94d07806deb9b","pinned_image":"ghcr.io/github/gh-aw-firewall/agent:0.27.42@sha256:26a8af4e5566485b02f52af59ee03803ae798271a9619d4767e94d07806deb9b"},{"image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.42","digest":"sha256:944f2686c9ab9bec338fd14b662461662f77cd12cd0ea8a3e7cb8c0987cd1607","pinned_image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.42@sha256:944f2686c9ab9bec338fd14b662461662f77cd12cd0ea8a3e7cb8c0987cd1607"},{"image":"ghcr.io/github/gh-aw-firewall/squid:0.27.42","digest":"sha256:42dfeb649c680a8558cd5423dbc530b653a69413e35ffbe5e71da5d48c94bdf0","pinned_image":"ghcr.io/github/gh-aw-firewall/squid:0.27.42@sha256:42dfeb649c680a8558cd5423dbc530b653a69413e35ffbe5e71da5d48c94bdf0"},{"image":"ghcr.io/github/gh-aw-mcpg:v0.4.6","digest":"sha256:fecabec51bbc41f2ad61076d6bcd9a36ef23b142e672a444e054d37fc29de93c","pinned_image":"ghcr.io/github/gh-aw-mcpg:v0.4.6@sha256:fecabec51bbc41f2ad61076d6bcd9a36ef23b142e672a444e054d37fc29de93c"},{"image":"ghcr.io/github/gh-aw-node","digest":"sha256:a8082161d7dceda14b68f32eb39d0eaa96b825d07f5895b096afab9d9e0c7748","pinned_image":"ghcr.io/github/gh-aw-node@sha256:a8082161d7dceda14b68f32eb39d0eaa96b825d07f5895b096afab9d9e0c7748"},{"image":"ghcr.io/github/github-mcp-server:v1.7.0","digest":"sha256:c491ffdf6f4c85cb5397021bc655edb8ab825c6f5f568e7597d77a1bd7c4d308","pinned_image":"ghcr.io/github/github-mcp-server:v1.7.0@sha256:c491ffdf6f4c85cb5397021bc655edb8ab825c6f5f568e7597d77a1bd7c4d308"}],"has_pull_request":true} # This file was automatically generated by gh-aw (v0.83.4). DO NOT EDIT. To debug this workflow, load the skill at https://github.com/github/gh-aw/blob/main/debug.md # @@ -23,7 +23,7 @@ # # For more information: https://github.github.com/gh-aw/introduction/overview/ # -# Addresses the PR reviewer's own feedback on demand, one run per arming. Arm it with an `/autofix [blocking|nits]` comment, or with an `autofix: blocking` / `autofix: nits` label; the two are peers. The run fixes the reviewer's open threads in that scope, pushes one commit, replies in each thread, and clears the label if one armed it. +# Addresses the PR reviewer's own feedback on demand, one run per arming. Arm it with an `/autofix [blocking|nits|docs]` comment, or with an `autofix: blocking` / `autofix: nits` / `autofix: docs` label; the two are peers. The run fixes the reviewer's open threads in that scope, pushes one commit, replies in each thread, and clears the label if one armed it. # # Source: Khan/actions/workflows/autofix/autofix.md@autofix-v0.0.0 # @@ -614,9 +614,9 @@ jobs: mkdir -p /tmp/gh-aw/safeoutputs mkdir -p /tmp/gh-aw/mcp-logs/safeoutputs mkdir -p "${RUNNER_TEMP}/gh-aw/safeoutputs/upload-artifacts" - cat > "${RUNNER_TEMP}/gh-aw/safeoutputs/config.json" << 'GH_AW_SAFE_OUTPUTS_CONFIG_5c591a0b3081d14f_EOF' - {"add_comment":{"discussions":false,"footer":false,"hide_older_comments":true,"max":1,"target":"triggering"},"create_report_incomplete_issue":{},"missing_data":{},"missing_tool":{},"noop":{"max":1,"report-as-issue":"true"},"push_to_pull_request_branch":{"github-token":"${GH_AW_SECRET_KHAN_ACTIONS_BOT_TOKEN}","if_no_changes":"ignore","max":1,"max_patch_size":4096,"protect_top_level_dot_folders":true,"protected_files":["package.json","bun.lockb","bunfig.toml","deno.json","deno.jsonc","deno.lock","global.json","NuGet.Config","Directory.Packages.props","mix.exs","mix.lock","go.mod","go.sum","stack.yaml","stack.yaml.lock","pom.xml","build.gradle","build.gradle.kts","settings.gradle","settings.gradle.kts","gradle.properties","package-lock.json","yarn.lock","pnpm-lock.yaml","npm-shrinkwrap.json","requirements.txt","Pipfile","Pipfile.lock","pyproject.toml","setup.py","setup.cfg","Gemfile","Gemfile.lock","uv.lock","CODEOWNERS","DESIGN.md","README.md","CONTRIBUTING.md","CHANGELOG.md","SECURITY.md","CODE_OF_CONDUCT.md","CLAUDE.md","AGENTS.md"],"target":"triggering"},"remove_labels":{"allowed":["autofix: blocking","autofix: nits","autofix: loop","autofix: human","autofix: author"]},"reply_to_pull_request_review_comment":{"footer":false,"max":20,"target":"triggering"},"report_incomplete":{},"upload_artifact":{"allowed-paths":["out/**","/tmp/gh-aw/autofix/out/**"],"max-size-bytes":104857600,"max-uploads":1,"retention-days":30}} - GH_AW_SAFE_OUTPUTS_CONFIG_5c591a0b3081d14f_EOF + cat > "${RUNNER_TEMP}/gh-aw/safeoutputs/config.json" << 'GH_AW_SAFE_OUTPUTS_CONFIG_98e85688ab759ad6_EOF' + {"add_comment":{"discussions":false,"footer":false,"hide_older_comments":true,"max":1,"target":"triggering"},"create_report_incomplete_issue":{},"missing_data":{},"missing_tool":{},"noop":{"max":1,"report-as-issue":"true"},"push_to_pull_request_branch":{"github-token":"${GH_AW_SECRET_KHAN_ACTIONS_BOT_TOKEN}","if_no_changes":"ignore","max":1,"max_patch_size":4096,"protect_top_level_dot_folders":true,"protected_files":["package.json","bun.lockb","bunfig.toml","deno.json","deno.jsonc","deno.lock","global.json","NuGet.Config","Directory.Packages.props","mix.exs","mix.lock","go.mod","go.sum","stack.yaml","stack.yaml.lock","pom.xml","build.gradle","build.gradle.kts","settings.gradle","settings.gradle.kts","gradle.properties","package-lock.json","yarn.lock","pnpm-lock.yaml","npm-shrinkwrap.json","requirements.txt","Pipfile","Pipfile.lock","pyproject.toml","setup.py","setup.cfg","Gemfile","Gemfile.lock","uv.lock","CODEOWNERS","DESIGN.md","README.md","CONTRIBUTING.md","CHANGELOG.md","SECURITY.md","CODE_OF_CONDUCT.md","CLAUDE.md","AGENTS.md"],"target":"triggering"},"remove_labels":{"allowed":["autofix: blocking","autofix: nits","autofix: docs","autofix: loop","autofix: human","autofix: author"]},"reply_to_pull_request_review_comment":{"footer":false,"max":20,"target":"triggering"},"report_incomplete":{},"upload_artifact":{"allowed-paths":["out/**","/tmp/gh-aw/autofix/out/**"],"max-size-bytes":104857600,"max-uploads":1,"retention-days":30}} + GH_AW_SAFE_OUTPUTS_CONFIG_98e85688ab759ad6_EOF - name: Generate Safe Outputs Tools env: GH_AW_TOOLS_META_JSON: | @@ -624,7 +624,7 @@ jobs: "description_suffixes": { "add_comment": " CONSTRAINTS: Maximum 1 comment(s) can be added. Target: triggering. Supports reply_to_id for discussion threading.", "push_to_pull_request_branch": " CONSTRAINTS: Maximum 1 push(es) can be made.", - "remove_labels": " CONSTRAINTS: Only these labels can be removed: [autofix: blocking autofix: nits autofix: loop autofix: human autofix: author].", + "remove_labels": " CONSTRAINTS: Only these labels can be removed: [autofix: blocking autofix: nits autofix: docs autofix: loop autofix: human autofix: author].", "reply_to_pull_request_review_comment": " CONSTRAINTS: Maximum 20 reply/replies can be created." }, "repo_params": {}, @@ -1585,7 +1585,7 @@ jobs: uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 env: WORKFLOW_NAME: "PR Autofixer" - WORKFLOW_DESCRIPTION: "Addresses the PR reviewer's own feedback on demand, one run per arming. Arm it with an `/autofix [blocking|nits]` comment, or with an `autofix: blocking` / `autofix: nits` label; the two are peers. The run fixes the reviewer's open threads in that scope, pushes one commit, replies in each thread, and clears the label if one armed it." + WORKFLOW_DESCRIPTION: "Addresses the PR reviewer's own feedback on demand, one run per arming. Arm it with an `/autofix [blocking|nits|docs]` comment, or with an `autofix: blocking` / `autofix: nits` / `autofix: docs` label; the two are peers. The run fixes the reviewer's open threads in that scope, pushes one commit, replies in each thread, and clears the label if one armed it." HAS_PATCH: ${{ needs.agent.outputs.has_patch }} with: script: | @@ -1887,7 +1887,7 @@ jobs: GH_AW_ALLOWED_DOMAINS: "*.githubusercontent.com,anthropic.com,api.anthropic.com,api.github.com,api.snapcraft.io,archive.ubuntu.com,azure.archive.ubuntu.com,cdn.playwright.dev,codeload.github.com,crl.geotrust.com,crl.globalsign.com,crl.identrust.com,crl.sectigo.com,crl.thawte.com,crl.usertrust.com,crl.verisign.com,crl3.digicert.com,crl4.digicert.com,crls.ssl.com,docs.github.com,files.pythonhosted.org,ghcr.io,github-cloud.githubusercontent.com,github-cloud.s3.amazonaws.com,github.blog,github.com,github.githubassets.com,host.docker.internal,json-schema.org,json.schemastore.org,keyserver.ubuntu.com,khanacademy.atlassian.net,khanacademy.dev,khanacademy.org,lfs.github.com,localhost,objects.githubusercontent.com,ocsp.digicert.com,ocsp.geotrust.com,ocsp.globalsign.com,ocsp.identrust.com,ocsp.sectigo.com,ocsp.ssl.com,ocsp.thawte.com,ocsp.usertrust.com,ocsp.verisign.com,packagecloud.io,packages.cloud.google.com,packages.microsoft.com,patch-diff.githubusercontent.com,patchdiff.githubusercontent.com,playwright.download.prss.microsoft.com,ppa.launchpad.net,pypi.org,raw.githubusercontent.com,registry.npmjs.org,s.symcb.com,s.symcd.com,security.ubuntu.com,sentry.io,statsig.anthropic.com,ts-crl.ws.symantec.com,ts-ocsp.ws.symantec.com,www.googleapis.com" GITHUB_SERVER_URL: ${{ github.server_url }} GITHUB_API_URL: ${{ github.api_url }} - GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG: "{\"add_comment\":{\"discussions\":false,\"footer\":false,\"hide_older_comments\":true,\"max\":1,\"target\":\"triggering\"},\"create_report_incomplete_issue\":{},\"missing_data\":{},\"missing_tool\":{},\"noop\":{\"max\":1,\"report-as-issue\":\"true\"},\"push_to_pull_request_branch\":{\"github-token\":\"${{ secrets.KHAN_ACTIONS_BOT_TOKEN }}\",\"if_no_changes\":\"ignore\",\"max\":1,\"max_patch_size\":4096,\"protect_top_level_dot_folders\":true,\"protected_files\":[\"package.json\",\"bun.lockb\",\"bunfig.toml\",\"deno.json\",\"deno.jsonc\",\"deno.lock\",\"global.json\",\"NuGet.Config\",\"Directory.Packages.props\",\"mix.exs\",\"mix.lock\",\"go.mod\",\"go.sum\",\"stack.yaml\",\"stack.yaml.lock\",\"pom.xml\",\"build.gradle\",\"build.gradle.kts\",\"settings.gradle\",\"settings.gradle.kts\",\"gradle.properties\",\"package-lock.json\",\"yarn.lock\",\"pnpm-lock.yaml\",\"npm-shrinkwrap.json\",\"requirements.txt\",\"Pipfile\",\"Pipfile.lock\",\"pyproject.toml\",\"setup.py\",\"setup.cfg\",\"Gemfile\",\"Gemfile.lock\",\"uv.lock\",\"CODEOWNERS\",\"DESIGN.md\",\"README.md\",\"CONTRIBUTING.md\",\"CHANGELOG.md\",\"SECURITY.md\",\"CODE_OF_CONDUCT.md\",\"CLAUDE.md\",\"AGENTS.md\"],\"target\":\"triggering\"},\"remove_labels\":{\"allowed\":[\"autofix: blocking\",\"autofix: nits\",\"autofix: loop\",\"autofix: human\",\"autofix: author\"]},\"reply_to_pull_request_review_comment\":{\"footer\":false,\"max\":20,\"target\":\"triggering\"},\"report_incomplete\":{},\"upload_artifact\":{\"allowed-paths\":[\"out/**\",\"/tmp/gh-aw/autofix/out/**\"],\"max-size-bytes\":104857600,\"max-uploads\":1,\"retention-days\":30}}" + GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG: "{\"add_comment\":{\"discussions\":false,\"footer\":false,\"hide_older_comments\":true,\"max\":1,\"target\":\"triggering\"},\"create_report_incomplete_issue\":{},\"missing_data\":{},\"missing_tool\":{},\"noop\":{\"max\":1,\"report-as-issue\":\"true\"},\"push_to_pull_request_branch\":{\"github-token\":\"${{ secrets.KHAN_ACTIONS_BOT_TOKEN }}\",\"if_no_changes\":\"ignore\",\"max\":1,\"max_patch_size\":4096,\"protect_top_level_dot_folders\":true,\"protected_files\":[\"package.json\",\"bun.lockb\",\"bunfig.toml\",\"deno.json\",\"deno.jsonc\",\"deno.lock\",\"global.json\",\"NuGet.Config\",\"Directory.Packages.props\",\"mix.exs\",\"mix.lock\",\"go.mod\",\"go.sum\",\"stack.yaml\",\"stack.yaml.lock\",\"pom.xml\",\"build.gradle\",\"build.gradle.kts\",\"settings.gradle\",\"settings.gradle.kts\",\"gradle.properties\",\"package-lock.json\",\"yarn.lock\",\"pnpm-lock.yaml\",\"npm-shrinkwrap.json\",\"requirements.txt\",\"Pipfile\",\"Pipfile.lock\",\"pyproject.toml\",\"setup.py\",\"setup.cfg\",\"Gemfile\",\"Gemfile.lock\",\"uv.lock\",\"CODEOWNERS\",\"DESIGN.md\",\"README.md\",\"CONTRIBUTING.md\",\"CHANGELOG.md\",\"SECURITY.md\",\"CODE_OF_CONDUCT.md\",\"CLAUDE.md\",\"AGENTS.md\"],\"target\":\"triggering\"},\"remove_labels\":{\"allowed\":[\"autofix: blocking\",\"autofix: nits\",\"autofix: docs\",\"autofix: loop\",\"autofix: human\",\"autofix: author\"]},\"reply_to_pull_request_review_comment\":{\"footer\":false,\"max\":20,\"target\":\"triggering\"},\"report_incomplete\":{},\"upload_artifact\":{\"allowed-paths\":[\"out/**\",\"/tmp/gh-aw/autofix/out/**\"],\"max-size-bytes\":104857600,\"max-uploads\":1,\"retention-days\":30}}" GH_AW_CI_TRIGGER_TOKEN: ${{ secrets.GH_AW_CI_TRIGGER_TOKEN }} GITHUB_TOKEN: ${{ secrets.KHAN_ACTIONS_BOT_TOKEN }} with: diff --git a/.github/workflows/autofix.md b/.github/workflows/autofix.md index 68f10fe3..0f435c13 100644 --- a/.github/workflows/autofix.md +++ b/.github/workflows/autofix.md @@ -1,10 +1,10 @@ --- description: > Addresses the PR reviewer's own feedback on demand, one run per arming. Arm it - with an `/autofix [blocking|nits]` comment, or with an `autofix: blocking` / - `autofix: nits` label; the two are peers. The run fixes the reviewer's open - threads in that scope, pushes one commit, replies in each thread, and clears - the label if one armed it. + with an `/autofix [blocking|nits|docs]` comment, or with an `autofix: blocking` + / `autofix: nits` / `autofix: docs` label; the two are peers. The run fixes the + reviewer's open threads in that scope, pushes one commit, replies in each + thread, and clears the label if one armed it. on: # Two arming surfaces, and they are PEERS — neither is a shorthand for the @@ -167,6 +167,7 @@ safe-outputs: allowed: - "autofix: blocking" - "autofix: nits" + - "autofix: docs" - "autofix: loop" - "autofix: human" - "autofix: author" @@ -472,6 +473,16 @@ Edit the files directly in the workspace. Rules, all hard: the finding unfixed and explain why in Step 7. Deleting an assertion, loosening a matcher, adding a skip, or widening an expected range to make something pass is never an acceptable outcome of this workflow. +- **A documentation item changes text, never code.** An item labelled + `suggestion (non-blocking, documentation)` is a finding about a comment or a + prose doc, and the whole reason its scope exists is that its edits cannot + alter behaviour. Deleting a comment the finding calls redundant is the + expected fix, not an overreach, and such a finding often carries no + suggestion block precisely because a deletion cannot be expressed as one. But + if the honest fix would touch an executable line — renaming the symbol the + comment misdescribes, changing the constant the comment contradicts — that is + a code change wearing a documentation label: leave the item unfixed and say + why in Step 7. Fix the sentence, or fix nothing. - **Do not touch files no item points at.** The one exception is a change that is mechanically forced by a fix (a caller that must be updated for a changed signature); note any such file in Step 7. diff --git a/workflows/autofix/README.md b/workflows/autofix/README.md index 0f3a0d29..3369fbdf 100644 --- a/workflows/autofix/README.md +++ b/workflows/autofix/README.md @@ -17,12 +17,14 @@ Two ways to arm it, and they are peers. Neither is a shorthand for the other. | ------------------ | ---------------------------------------------------------- | | `autofix: blocking` | The reviewer's open blocking threads (`issue (blocking)`, `issue (blocking, best-practice)`, `todo (blocking)`) | | `autofix: nits` | The reviewer's open non-blocking threads (suggestions, nitpicks, questions, thoughts, notes) | +| `autofix: docs` | Only the `documentation` reviewer's threads (`suggestion (non-blocking, documentation)`) — a subset of `nits`, see below | **Or comment on the PR:** ``` /autofix # same as /autofix blocking /autofix nits +/autofix docs /autofix blocking nits ``` @@ -122,10 +124,18 @@ names a value on exactly one of them: | Axis | Tokens | Combination rule | Implemented | | ----------- | ----------------------------- | ---------------------- | ----------- | -| **scope** | `blocking`, `nits` | union | yes | +| **scope** | `blocking`, `nits`, `docs` | union | yes | | **cadence** | `loop` (absent = once) | flag | no | | **source** | `human`, `author` (absent = the reviewer bot) | union | no | +The three scope values are not three disjoint classes. `blocking` and `nits` +partition the reviewer's threads between them; **`docs` is a subset of `nits`**, +selecting only the `documentation` reviewer's label. Arming both is the same as +arming `nits`, and the containment runs one way only: `docs` exists because +arming `nits` to clear three stale comments also invites the fixer into every +other cosmetic thread on the PR. A flat namespace cannot show that, so it is +written down here and in `scope.ts`. + Read this before adding a token to the vocabulary. `nits` and `loop` look like peers and are not, and the day both are requested the rule that resolves them has to already exist. @@ -146,11 +156,67 @@ two forms would be indistinguishable at a glance. ### One constraint that outlives v1: nits never loop -`isLoopEligible` is enforced in code rather than left to convention. -Non-blocking findings have no fixed point — the reviewer will always find -something cosmetic in the autofixer's own output — so a nits-scoped loop cannot -converge. Blocking scope terminates naturally at the merge gate, which is why it -is the scope a cadence axis would be built on. +What enforces this in v1 is the **token table**: `loop` sits in +`UNIMPLEMENTED_TOKENS`, so `autofix: loop` is rejected and no cadence can be +armed at all. `isLoopEligible` in `scope.ts` states the rule itself and has no +production caller, because there is no cadence axis to call it; it exists so the +rule does not have to be rediscovered. **Whoever builds the cadence axis must +call it**; until then it is a documented intent with a test, not a gate. + +The rule: non-blocking findings have no fixed point, so a nits-scoped loop +cannot converge. Blocking scope terminates at the merge gate, which is why it is +the scope a cadence axis would be built on. But note what that rests on: the +merge gate is a claim about a human eventually merging, not something the +pipeline enforces, and the one mechanism that does bound re-flagging exempts +blocking from itself: the reviewer's newly-changed-code scope filter +(`applyScopeFilter` in `review/lib/dispatch-contracts.ts`) keeps plain +`issue (blocking)` / `todo (blocking)` findings regardless of scope, so blocking +is the one class that can be re-raised on previously-reviewed, untouched lines on +every cycle. The filter bounds nits; it does not bound blocking. + +Khan/webapp#41194 is the first live evidence, and it corrects what this section +used to claim the generator was. One blocking-scoped cycle fixed its finding, and +the re-review resolved that thread and approved, then filed two fresh +non-blocking findings against code autofix never wrote (`counts.go:7` is +`func MergeCounts`, twelve lines above its first added line; `counts.go:16` is a +context line in its own hunk). That run planned `no-prior-fingerprint`, so the +scope filter was a no-op and the whole diff was re-derived with no memory of what +the previous review had already said. Open non-blocking threads went 3 → 5 in one +cycle with no nits-scoped work done. The generator is a **memoryless +re-derivation over the whole diff**, not the fixer's own prose; it does not need +the fixer to have written anything. + +`docs` is the value worth pausing on, because it looks like the exception and is +only half one. Its deletion half genuinely converges: a comment that restates +the code is either gone or it is not. Its other half does not, because the +documentation reviewer also flags a *missing* explanation, the fixer answers +with prose, and prose is the thing a reviewer can always want written better. +So `docs` is ineligible too. Khan/webapp#41194 gives that its first data point, +and it lands on the half that does not converge: `counts.go:16` is a *missing* +explanation (`TopKey`'s doc comment "covers tie resolution but not the +empty/nil-map case"), raised unprompted against the fixer's own PR. Read it as +one observation of the shape rather than a measurement of the domain, and note +it did not come from the documentation reviewer: that repo runs a `review` +release too old to mint the `documentation` label, so the finding was a plain +`note (non-blocking)` and `autofix: docs` would not have selected it. If the +cadence axis is ever built, `docs` is still the first candidate to re-examine, +and which half dominates in practice is still the thing to measure first. + +### Why `docs` is the safest scope + +Its edits cannot change program behaviour, which no other scope can say. That +makes it the natural first scope to trial in a repo that has not run autofix +before, and the prompt holds the line in Step 4: if the honest fix for a +documentation finding would touch an executable line, the item is left unfixed +and reported rather than quietly becoming a code change wearing a documentation +label. + +Note the version coupling. Autofix selects threads by parsing the +Conventional-Comment label off each posted comment, so `autofix: docs` finds +threads only when the reviewer that posted them minted the documentation label. +A repo needs a `review` release carrying that label **installed** before this +scope does anything; against an older reviewer it is not broken, it is simply +always empty. ## What it refuses to do @@ -266,8 +332,9 @@ autofix reads its threads, its label taxonomy, and its fingerprint stamp. ### Repository setup -Create the two labels (`autofix: blocking`, `autofix: nits`) if you want the -label surface; the `/autofix` command needs no setup. Nothing else is +Create the labels you want (`autofix: blocking`, `autofix: nits`, +`autofix: docs`) if you want the label surface; the `/autofix` command needs no +setup, and an uncreated label simply cannot be applied. Nothing else is configured per repo: scope is chosen per PR by label or argument. ## Design notes diff --git a/workflows/autofix/autofix.md b/workflows/autofix/autofix.md index 68f10fe3..0f435c13 100644 --- a/workflows/autofix/autofix.md +++ b/workflows/autofix/autofix.md @@ -1,10 +1,10 @@ --- description: > Addresses the PR reviewer's own feedback on demand, one run per arming. Arm it - with an `/autofix [blocking|nits]` comment, or with an `autofix: blocking` / - `autofix: nits` label; the two are peers. The run fixes the reviewer's open - threads in that scope, pushes one commit, replies in each thread, and clears - the label if one armed it. + with an `/autofix [blocking|nits|docs]` comment, or with an `autofix: blocking` + / `autofix: nits` / `autofix: docs` label; the two are peers. The run fixes the + reviewer's open threads in that scope, pushes one commit, replies in each + thread, and clears the label if one armed it. on: # Two arming surfaces, and they are PEERS — neither is a shorthand for the @@ -167,6 +167,7 @@ safe-outputs: allowed: - "autofix: blocking" - "autofix: nits" + - "autofix: docs" - "autofix: loop" - "autofix: human" - "autofix: author" @@ -472,6 +473,16 @@ Edit the files directly in the workspace. Rules, all hard: the finding unfixed and explain why in Step 7. Deleting an assertion, loosening a matcher, adding a skip, or widening an expected range to make something pass is never an acceptable outcome of this workflow. +- **A documentation item changes text, never code.** An item labelled + `suggestion (non-blocking, documentation)` is a finding about a comment or a + prose doc, and the whole reason its scope exists is that its edits cannot + alter behaviour. Deleting a comment the finding calls redundant is the + expected fix, not an overreach, and such a finding often carries no + suggestion block precisely because a deletion cannot be expressed as one. But + if the honest fix would touch an executable line — renaming the symbol the + comment misdescribes, changing the constant the comment contradicts — that is + a code change wearing a documentation label: leave the item unfixed and say + why in Step 7. Fix the sentence, or fix nothing. - **Do not touch files no item points at.** The one exception is a change that is mechanically forced by a fix (a caller that must be updated for a changed signature); note any such file in Step 7. diff --git a/workflows/autofix/lib/scope.test.ts b/workflows/autofix/lib/scope.test.ts index 8c576c2a..91f28b26 100644 --- a/workflows/autofix/lib/scope.test.ts +++ b/workflows/autofix/lib/scope.test.ts @@ -14,6 +14,7 @@ import { } from "./scope.ts"; import { BLOCKING_LABELS, + DOCUMENTATION_LABEL, NON_BLOCKING_LABELS, } from "../../review/lib/render-comment.ts"; @@ -121,12 +122,20 @@ describe("isLoopEligible", () => { it("forbids nits from ever looping", () => { expect(isLoopEligible("nits")).toBe(false); }); + + // Docs looks convergent (a restating comment is either deleted or not) and + // is only half so: the same reviewer flags MISSING explanations, and prose + // can always be wanted better. Ineligible until measured. + it("forbids docs from looping, despite its deletion half converging", () => { + expect(isLoopEligible("docs")).toBe(false); + }); }); describe("findingLabelsForScope", () => { it("maps each scope onto the reviewer's own taxonomy", () => { expect(findingLabelsForScope("blocking")).toEqual(BLOCKING_LABELS); expect(findingLabelsForScope("nits")).toEqual(NON_BLOCKING_LABELS); + expect(findingLabelsForScope("docs")).toEqual([DOCUMENTATION_LABEL]); }); it("keeps the two classes disjoint", () => { @@ -135,6 +144,27 @@ describe("findingLabelsForScope", () => { expect(blocking.has(label)).toBe(false); } }); + + // The containment the flat label namespace cannot show: arming `nits` + // already includes every docs thread, so `nits + docs` is just `nits`. + // `docs` earns its token by being the narrowing, not by adding anything. + it("makes docs a strict subset of nits", () => { + const nits = new Set(findingLabelsForScope("nits")); + for (const label of findingLabelsForScope("docs")) { + expect(nits.has(label)).toBe(true); + } + expect(findingLabelsForScope("docs").length).toBeLessThan( + findingLabelsForScope("nits").length, + ); + }); + + // A docs run must never be able to touch a thread that blocks the merge. + it("never puts a blocking label in docs scope", () => { + const blocking = new Set(BLOCKING_LABELS); + for (const label of findingLabelsForScope("docs")) { + expect(blocking.has(label)).toBe(false); + } + }); }); describe("resolveCommand", () => { diff --git a/workflows/autofix/lib/scope.ts b/workflows/autofix/lib/scope.ts index 78955813..64d19170 100644 --- a/workflows/autofix/lib/scope.ts +++ b/workflows/autofix/lib/scope.ts @@ -29,16 +29,41 @@ * working loop that stopped after one cycle, which is the worst of both * behaviours. * - * One constraint outlives this version and is enforced here rather than left to - * convention: **`nits` is never loop-eligible**. Non-blocking findings have no - * fixed point (the reviewer will always find something cosmetic in the - * autofixer's own output), so a nits-scoped loop cannot converge and must not be - * offered. See {@link isLoopEligible}; when the cadence axis lands, the loop - * token has to consult it. + * One constraint outlives this version: **`nits` is never loop-eligible**. + * Non-blocking findings have no fixed point, so a nits-scoped loop cannot + * converge and must not be offered. What enforces that *here* is the token + * table: `loop` is in {@link UNIMPLEMENTED_TOKENS}, so no cadence can be armed + * at all. {@link isLoopEligible} states the rule for whoever adds the cadence + * axis and has no caller until they write one; calling it is their job. + * + * The generator observed was a **memoryless re-derivation over the whole + * diff**, not the fixer's own prose, which is what this comment used to guess. + * Khan/webapp#41194 measured one blocking-scoped cycle: the fix cleared its + * finding, the re-review resolved that thread and approved, and then filed two + * fresh non-blocking findings against code the fixer never wrote (`counts.go:7` + * is `func MergeCounts`, twelve lines above its first added line; + * `counts.go:16` is a context line in its own hunk). That run planned + * `no-prior-fingerprint`, so the reviewer's newly-changed-code scope filter was + * a no-op and the whole diff was re-derived with no memory of the previous + * review. Open non-blocking threads went 3 → 5 in one cycle with no nits-scoped + * work done. The fixer does not have to have written anything for this to + * happen. + * + * Read memorylessness as a **condition, not a property**: it was a consumer + * version artifact with a named remedy. That repo pins `review-v1.7.0` + * (2026-07-21), which carries no `stage-pr.ts` and no `stampSource` at all, so + * its only fingerprint carrier was the review-body stamp that gh-aw's ingest + * sanitizer strips; `review-v1.8.0` (2026-07-30) has both, so a consumer on it + * can anchor a re-review and scope one. What a cadence axis must not assume is + * that any given consumer is on it. (Distinct from autofix's own currency + * check, which is degraded for an unrelated and non-version reason: cache + * memory is scoped per workflow, so the reviewer's carrier is unreachable from + * here. See `staleness.ts`.) */ import { BLOCKING_LABELS, + DOCUMENTATION_LABEL, NON_BLOCKING_LABELS, } from "../../review/lib/render-comment.ts"; @@ -51,9 +76,17 @@ export const AUTOFIX_COMMAND = "/autofix"; /** * The scope axis: which class of review finding a token puts in scope. * `blocking` is the default a repo reaches for (it terminates naturally at the - * merge gate); `nits` is the deliberate one-shot tidy-up. + * merge gate); `nits` is the deliberate one-shot tidy-up; `docs` is the + * narrowest, and the only one whose edits cannot change program behaviour. + * + * **`docs` is a subset of `nits`, not a peer of it.** Documentation findings + * are non-blocking, so `nits` already covers them and arming both is the same + * as arming `nits`. The reason `docs` exists as its own token is that the + * reverse is not true: arming `nits` to clear three stale comments also invites + * the fixer into every other cosmetic thread on the PR. The flat namespace + * cannot show this containment, so it is stated here and in the README. */ -export const AUTOFIX_SCOPES = ["blocking", "nits"] as const; +export const AUTOFIX_SCOPES = ["blocking", "nits", "docs"] as const; export type AutofixScope = typeof AUTOFIX_SCOPES[number]; @@ -63,6 +96,7 @@ export type AutofixScope = typeof AUTOFIX_SCOPES[number]; export const SCOPE_TOKENS: Readonly> = { blocking: "blocking", nits: "nits", + docs: "docs", }; /** @@ -112,13 +146,55 @@ export const DEFAULT_COMMAND_SCOPE: AutofixScope = "blocking"; * terminate at the merge gate; non-blocking ones have no fixed point. This * version has no loop, but the rule is encoded now because it is the constraint * most likely to be violated by whoever adds one later. + * + * Note what `blocking`'s eligibility actually rests on: the merge gate is a + * claim about a human eventually merging, not a termination proof, and nothing + * in the reviewer backs it. The one mechanism that bounds re-flagging exempts + * blocking from itself: `applyScopeFilter` in + * `review/lib/dispatch-contracts.ts` keeps plain `issue (blocking)` / + * `todo (blocking)` findings whether or not they land on newly-changed code, so + * blocking is the one class that can be re-raised on previously-reviewed, + * untouched lines every cycle. The scope filter bounds nits; it does not bound + * blocking. A cadence axis needs its own stop condition and cannot inherit one + * from this predicate. + * + * `docs` is the case worth pausing on, because it looks convergent and is only + * half so. Its deletion half has a fixed point (a comment that restates the + * code is either gone or not), which is exactly the argument someone will make + * for looping it. Its other half does not: the documentation reviewer also + * flags a *missing* explanation, the fixer answers with prose, and prose is + * the thing a reviewer can always want written better. So `docs` stays + * ineligible until something measures which half dominates in practice. + * Khan/webapp#41194 is the first data point and it lands on the non-convergent + * half: `counts.go:16` flags a doc comment that covers tie resolution but not + * the empty/nil-map case, raised unprompted against the fixer's own PR. That is + * one observation of the shape, from a reviewer too old to mint the + * `documentation` label (so `autofix: docs` would not have selected it), and it + * says nothing yet about which half dominates. */ export const isLoopEligible = (scope: AutofixScope): boolean => scope === "blocking"; -/** The Conventional-Comment labels a given autofix scope covers. */ -export const findingLabelsForScope = (scope: AutofixScope): readonly string[] => - scope === "blocking" ? BLOCKING_LABELS : NON_BLOCKING_LABELS; +/** + * The Conventional-Comment labels a given autofix scope covers. + * + * `docs` resolves to the single label the documentation reviewer mints. That + * label is the only thing distinguishing a documentation thread from any other + * nit by the time autofix sees it: the worklist reads threads off the PR and + * parses their leading label, so nothing else about the finding survives. + */ +export const findingLabelsForScope = ( + scope: AutofixScope, +): readonly string[] => { + switch (scope) { + case "blocking": + return BLOCKING_LABELS; + case "docs": + return [DOCUMENTATION_LABEL]; + case "nits": + return NON_BLOCKING_LABELS; + } +}; /** How this run was armed. Recorded so the summary can say which surface. */ export type RequestSurface = "label" | "command";