which-fixer skill#19
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new which-fixer skill to standardize “restrictive which → that” fixes in source-code comments and repository documentation, with guidance for bulk vs incremental operation and an agent entrypoint configuration.
Changes:
- Introduces
skills/which-fixer/SKILL.mddefining scope, workflow, and reporting format for the fixer. - Adds
skills/which-fixer/agents/openai.yamlto register the skill with a default prompt and UI metadata.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| skills/which-fixer/SKILL.md | Documents the skill’s grammar rule, scanning constraints, and bulk/incremental workflow including completion recording. |
| skills/which-fixer/agents/openai.yaml | Registers the skill’s display name, short description, and default prompt for the OpenAI agent interface. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - Present → **incremental mode**: scan only files modified on the current | ||
| branch (`git diff --name-only master...HEAD` — three-dot: changes since the | ||
| branch diverged from `master`, not a plain tip-to-tip diff; fall back to | ||
| `origin/master...HEAD` if `master` does not resolve locally). |
![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: 00200e74b2
ℹ️ 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".
| - **Comma check (mandatory):** the word "which" is *not* immediately preceded | ||
| by a comma (allowing for optional whitespace between the comma and "which"). | ||
| If a comma precedes it, this is non-restrictive — leave it untouched. |
There was a problem hiding this comment.
This comma-only guard will still rewrite non-restrictive clauses that are set off with parentheses or dashes, e.g. the report (which is generated by Kover) or the report — which is generated —, because those occurrences are not preceded by a comma and pass the later checks. When the skill runs over docs/comments containing those common parenthetical forms, it will introduce incorrect that rewrites instead of leaving non-restrictive clauses untouched.
Useful? React with 👍 / 👎.
| - `**/*.kt`, `**/*.kts`, `**/*.java` — Kotlin and Java sources | ||
| - `**/*.md` — Markdown documentation | ||
| - `**/*.adoc` — AsciiDoc (if present) |
There was a problem hiding this comment.
This is the target-file filter used in both bulk and incremental modes, but it omits **/*.proto. Spine repositories keep substantial API documentation in Protobuf comments, so a branch that changes only .proto documentation will be skipped entirely by this advertised comments/docs grammar sweep.
Useful? React with 👍 / 👎.
| Exclude always: paths under `build/`, `.gradle/`, generated sources, and | ||
| anything matched by `.gitignore`. |
There was a problem hiding this comment.
In consumer repositories this repo's assets are mounted under .agents/shared, but bulk mode scans the whole repository and this exclude list only removes build/generated/ignored paths. A filesystem-based sweep can therefore edit .agents/shared/**/*.md or skill files, dirtying the shared submodule instead of just fixing the consumer project; add the shared-agent paths to the excludes or require a tracked-file scan that does not recurse into submodules.
Useful? React with 👍 / 👎.
| branch only). Records completion in `.agents/memory/` so the next invocation | ||
| switches to incremental mode automatically. Use once per repo for the initial | ||
| sweep; `pre-pr` can then invoke it on each branch. |
There was a problem hiding this comment.
This description tells users that pre-pr can invoke the incremental check, but I checked skills/pre-pr/SKILL.md and its reviewer dispatch never includes which-fixer. Repositories that rely on this advertised path after the bulk marker is written will not get branch-level which fixes during pre-PR, so either add the skill to the pre-pr workflow or remove this promise from the trigger text.
Useful? React with 👍 / 👎.
| 4. **Record completion (bulk mode only).** | ||
| After the sweep finishes, write `.agents/memory/which-fixer-applied.md`: | ||
|
|
There was a problem hiding this comment.
.agents
This writes the mode marker under .agents/memory, but .agents is part of the repository layout and this path is not gitignored; the existing repo guidance uses built-in auto-memory for per-developer memory instead. Running the bulk sweep in a consumer repo will therefore create real repo files just to remember local state, which can dirty PRs or get committed alongside grammar fixes.
Useful? React with 👍 / 👎.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5fe189058a
ℹ️ 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".
| Within the scanned text, locate every occurrence of the word "which" | ||
| (case-insensitive). Before replacing, verify **all** of the following: |
There was a problem hiding this comment.
Because Markdown headings are scanned and the replacement checks do not exclude hyphenated identifiers, a bulk run over this repo can treat the newly added # which-fixer heading as a lowercase occurrence of which and rewrite the skill name to that-fixer. The same applies to other kebab-case names in docs, so add a guard that skips hyphenated identifiers/file names before applying the grammar rewrite.
Useful? React with 👍 / 👎.
| *For `.kt`, `.kts`, `.java` files:* confine scanning to comment text only — | ||
| KDoc (`/** … */`), block comments (`/* … */`), and line comments (`//`). | ||
| Do not alter string literals, identifiers, or any executable code token. |
There was a problem hiding this comment.
For source files with KDoc/Javadoc examples, this scans all comment text while only Markdown/AsciiDoc code fences are excluded later. A documented snippet such as a shell example using the which command can therefore be rewritten inside the comment, corrupting the sample even though the skill promises not to alter code; source comments need the same fenced/inline-code exclusion before replacements run.
Useful? React with 👍 / 👎.
| - Present → **incremental mode**: scan only files modified on the current | ||
| branch (`git diff --name-only master...HEAD` — three-dot: changes since the | ||
| branch diverged from `master`, not a plain tip-to-tip diff; fall back to | ||
| `origin/master...HEAD` if `master` does not resolve locally). |
There was a problem hiding this comment.
main when computing incremental diffs
In repositories whose default branch is main rather than master (this checkout currently has main and no master/origin/master), incremental mode has no valid base after the first marker is written, so the skill cannot produce its changed-file list. Please resolve the configured default branch or also fall back to main/origin/main before giving up.
Useful? React with 👍 / 👎.
| - Read `.agents/memory/which-fixer-applied.md` if it exists. | ||
| - Absent → **bulk mode**: scan the entire repository. | ||
| - Present → **incremental mode**: scan only files modified on the current | ||
| branch (`git diff --name-only master...HEAD` — three-dot: changes since the |
There was a problem hiding this comment.
On branches that delete a matching .md, .kt, or .java file, this command still emits the deleted path; I checked git diff -h, where --name-only is described as showing only names of changed files, not filtering by status. The next step tells the agent to scan each path, so a deleted file can make the fixer try to read a file that no longer exists; add a diff filter such as --diff-filter=ACMR or skip non-existent paths.
Useful? React with 👍 / 👎.
| - Present → **incremental mode**: scan only files modified on the current | ||
| branch (`git diff --name-only master...HEAD` — three-dot: changes since the | ||
| branch diverged from `master`, not a plain tip-to-tip diff; fall back to | ||
| `origin/master...HEAD` if `master` does not resolve locally). |
There was a problem hiding this comment.
When incremental mode is run before edits are committed, git diff --name-only master...HEAD lists only committed branch changes and omits staged/unstaged working-tree files; I verified this locally with an uncommitted .md edit. In this repo agents often work uncommitted until explicit commit authorization, so the fixer can report no files while the user's current comments still need fixes; include working-tree and cached diffs or scan existing modified files too.
Useful? React with 👍 / 👎.
| *For `.md` and `.adoc` files:* scan prose and headings only. Skip fenced code | ||
| blocks and inline code spans, applying the same "do not touch code" discipline | ||
| used for source files, so that documented code samples are never altered. |
There was a problem hiding this comment.
This only excludes fenced blocks and inline code, but Markdown can still contain indented code blocks and AsciiDoc commonly uses delimited listing/source blocks such as ----; in those files a command like which java remains in the scan area and can be rewritten even though it is a code sample. Please explicitly skip those block forms before applying replacements.
Useful? React with 👍 / 👎.
What
Adds a new shared skill,
which-fixer, that fixes the restrictive"which/that" grammar error in source-code comments and documentation. It
rewrites a restrictive "which" (no preceding comma) to "that" and leaves
non-restrictive ", which" clauses alone. The mistake recurs across the Spine
codebase because the Russian relative pronoun который covers both English
senses indiscriminately, so it warrants a dedicated, repeatable fixer rather
than ad-hoc cleanup.
How it works
.agents/memory/which-fixer-applied.mdmarker) it sweeps the whole codebase,then records completion. Every later run reads that marker and switches to
incremental mode — only the files changed on the current branch
(
git diff master...HEAD).rather than printing a list of complaints.
.kt/.kts/.javait touches comment text only(never code tokens or string literals); in
.md/.adocit skips fenced andinline code so documented samples are never altered.
(non-restrictive), any preposition + "which", interrogative/determiner
(including embedded), sentence-initial "Which", and the fixed phrases
"that which" / "which is which". An ambiguous case is left unchanged and
reported under
Skipped[]— a missed fix is preferred over a wrong one.Rollout
Run
which-fixeronce per repo for the initial sweep; the recorded marker thenkeeps later invocations in incremental mode.
pre-prcan call it afterwards soeach branch is checked on its modified files only.
Files
skills/which-fixer/SKILL.md— the skill definition.skills/which-fixer/agents/openai.yaml— Codex/OpenAI interface metadata.Verification
correctness, grammar, US-spelling consistency, false-positive coverage):
HAS FINDINGS → HAS FINDINGS → CLEAN; all findings fixed in the
Address round 1/2 reviewcommits.author-skillchecklist passes: directory name matchesname, description540 chars (< 1024), 141 lines (< 500),
$which-fixerprompt present, notask-plan references.
"which".