Repository navigation
docs(skill): complete the setup skill for agent runs - #687
Conversation
TheGreatAxios
left a comment
TheGreatAxios
left a comment
There was a problem hiding this comment.
Independent review, final pass at 2752a1b.
Checked: every step 1 block runs on its own in a fresh shell and prints what the worked examples claim on this repository; the issue-link count gives 0 for "GPT-4 support" and "ES-2022 and SHA-1 and COVID-19", 1 for "Closes CL-12", "CL-12: x" and a cl-NNN branch, and 1 GitHub for "fixes #3"; the setup-skill test passes; the skill's step 5 check prints valid for the starter file and for a file built from the suggestions; the catalog covers every built-in rule and step kind; the user manual and the action README match the job id and pin.
No findings.
|
Triage: In progress Triage is waiting for CI to finish before it decides. This comment shows the verdict once it has. |
![corbits-triage[bot]](https://avatars.githubusercontent.com/u/275831056?s=60&v=4){kind=small}
There was a problem hiding this comment.
Rewrites the corbits-triage-setup skill so it analyzes the repository, suggests policy rules with measured numbers, and leaves the choice to the maintainer, and expands reference.md into a full catalog of policy options. The main defects are an AI-attribution regex that counts human co-authors, and an Extras block and Actions example that can add unrequested model rules or close pull requests without a supporting rule.
|
|
||
| ## 3. Propose the rules | ||
| ```sh | ||
| gh pr list --state merged --limit 50 --json title,body --jq 'map(select((.title + " " + .body) | test("Co-Authored-By:|Claude-Session:|claude\\.ai/code/session|noreply@anthropic\\.com|Generated with \\[?Claude"; "i"))) | length' |
There was a problem hiding this comment.
P2 · AI-attribution count includes human Co-Authored-By lines
The regex matches any Co-Authored-By: line, including human co-authors. Step 1 suggests no-ai-attribution whenever the count is above 0, and when nobody answers the skill writes that rule anyway.
Trigger: merged PR bodies with Co-Authored-By: Jane <jane@example.com> from human pairing or squash bodies that list co-authors.
Fix: count only the AI-specific markers (Claude-Session:, claude.ai/code/session, noreply@anthropic.com, Generated with Claude) and drop the bare Co-Authored-By: alternative, or require the co-author email to match an AI pattern.
There was a problem hiding this comment.
Fixed: the count now matches only lines that name an AI tool (Co-Authored-By with noreply@anthropic.com or a Claude, Copilot, ChatGPT, Cursor, Codex or Gemini name, Generated with Claude, Claude-Session:, claude.ai/code/session); a plain Co-Authored-By no longer counts.
| - id: title-says-what-changed | ||
| check: model | ||
| text: The title says what changed. | ||
| reason: Reviewers pick pull requests from their titles. | ||
| evidence: [maintainer-stated] |
There was a problem hiding this comment.
P2 · Extras block adds a model rule with no measured signal
title-says-what-changed is a check: model rule. It has no row under "When to add" and no step 1 signal. Step 2 says not to add model rules unless step 1 found a signal, but step 4 tells the agent to copy rule blocks from "Extras, written out", so copying this block adds a model rule nobody chose.
Trigger: an agent with no maintainer answer, or a maintainer without a decision model, copies the Extras block into .corbits-triage.yml.
Fix: remove the block from Extras, or add a "When to add" row and a step 1 signal for it and mark it as requiring a decision model. Make step 4 copy only rules the maintainer confirmed.
There was a problem hiding this comment.
Fixed: removed title-says-what-changed from Extras, and step 4 now copies only the blocks of rules written in step 3.
| when: { linkedIssue: false } | ||
| - do: request-changes |
There was a problem hiding this comment.
P2 · Actions example closes pull requests on Fail without a linked-issue rule
The Actions example is presented as general, but its Fail step close fires on linkedIssue: false. The Starter file does not enable linked-issue, and the note about the linkedIssue step is not placed next to the example.
Trigger: a repository without a linked-issue rule copies the example. Any Fail verdict on a pull request that names no issue then triggers close.
Fix: state directly above the example that the linkedIssue step requires the linked-issue rule, or move that step into a separate, clearly marked optional block. Avoid presenting close as a default action.
There was a problem hiding this comment.
Fixed: the general Actions example's Fail is request-changes alone; the linkedIssue close step is shown apart, only for when linked-issue is on and the maintainer wants it.
| - Is the repository on GitHub or Bitbucket Data Center? Only when there is no remote. | ||
| - On GitHub, add a pull request check that checks this file and runs its rules that need no model on every pull request? Suggest yes. | ||
| ```sh | ||
| gh pr list --state merged --limit 50 --json title,body,headRefName --jq 'map({h: .title, t: (.title + " " + .body), b: .headRefName}) | "github \(map(select(.t | test("(?i)\\b(close[sd]?|fix(es|ed)?|resolve[sd]?)\\s+#[0-9]+|github\\.com/[^/ ]+/[^/ ]+/issues/[0-9]+"))) | length), linear \(map(select((.h | test("^([A-Z]{2,6}-[0-9]+:|\\[[A-Z]{2,6}-[0-9]+\\])")) or (.t | test("(?i)\\b(close[sd]?|fix(es|ed)?|resolve[sd]?|refs?)\\s+[A-Z]{2,6}-[0-9]+|linear\\.app/[^/ ]+/issue/")) or (.b | test("^[a-z]{2,6}-[0-9]+-[a-z0-9]+-[a-z0-9]+")))) | length) of \(length)"' |
There was a problem hiding this comment.
P3 · Linear branch regex counts ordinary GitHub-style branches
The pattern ^[a-z]{2,6}-[0-9]+-[a-z0-9]+-[a-z0-9]+ matches branches like fix-123-login-bug or feat-42-add-auth, which are normal GitHub issue branches. Those inflate the Linear count and can produce a wrong tracker: linear suggestion.
Trigger: a repository with branches named fix-<issue#>-short-desc and no Linear use.
Fix: require the team key to be uppercase in the branch name or to be a key that appears in the PR title, for example ^[a-z]{2,6}-[0-9]+- combined with a check that the key also appears in the title, or drop the branch signal when a GitHub-style closing keyword is present.
There was a problem hiding this comment.
Fixed: a Linear-shaped branch (2 to 6 letter key, number, slug of two or more words) is printed apart as a hint, not counted, and only when its key also appears in a title or linear.app link.
| git fetch origin "$branch" | ||
| git log --first-parent --format=%s -n 50 "origin/$branch" | grep -cE '\(#[0-9]+\)$' |
There was a problem hiding this comment.
P3 · Squash and commit-style counts are wrong in shallow clones
git fetch origin "$branch" does not deepen a shallow clone, so git log -n 50 and -n 200 only see the commits that are present. Agent and CI checkouts are often depth 1, which makes the squash count 0 or 1 and the commit ratios meaningless.
Trigger: the skill runs in a checkout made by actions/checkout with the default fetch-depth of 1.
Fix: before reading history, run git rev-parse --is-shallow-repository and, if true, run git fetch --deepen=200 origin "$branch" or git fetch --unshallow origin. If the history is still too short, report that the signal could not be measured instead of using the partial count.
There was a problem hiding this comment.
Fixed: both history blocks run git fetch --unshallow origin first (falling back to fetching the branch) and print how many commits they read, and the step says a short count is reported, not used for a suggestion.
There was a problem hiding this comment.
Completes the corbits-triage-setup skill so it analyzes the last 50 merged pull requests, suggests policy rules with measured evidence, and leaves the choice to the maintainer. It also expands reference.md into a full catalog, renames the policy-check job to corbits-triage-policy with a pinned action commit, and updates the user manual and action README to match. The most important defect is that the no-ai-attribution rule as written would flag human co-authors, which makes the rule unreliable on unattended runs.
| text: The title says what changed. | ||
| reason: Reviewers pick pull requests from their titles. | ||
| phrases: | ||
| - "Co-Authored-By:" |
There was a problem hiding this comment.
P2 · no-ai-attribution matches every bare Co-Authored-By line, including human co-authors
The rule in reference.md uses the bare phrase Co-Authored-By:, which matches any co-author trailer, not just AI ones. Step 1 counts only AI co-author lines, so the suggested rule and the measured count disagree. The previous guidance forbade this exact phrase for this reason.
Trigger: a pull request whose commit contains Co-Authored-By: Jane <jane@x.com>. The rule flags it, and every human co-authored pull request becomes Unsure after an unattended run.
Fix: use only the AI-specific trailer patterns that step 1 counts, and drop the bare Co-Authored-By: entry.
There was a problem hiding this comment.
Fixed: the rule's phrases are now only the AI-specific patterns step 1 counts (noreply@anthropic.com, Co-Authored-By naming Claude, Copilot, ChatGPT, Cursor, Codex or Gemini, Claude-Session:, claude.ai/code/session, Generated with Claude); the bare Co-Authored-By: is gone.
| - Is the repository on GitHub or Bitbucket Data Center? Only when there is no remote. | ||
| - On GitHub, add a pull request check that checks this file and runs its rules that need no model on every pull request? Suggest yes. | ||
| ```sh | ||
| gh pr list --state merged --limit 50 --json title,body,headRefName --jq 'map({h: .title, t: (.title + " " + .body), b: .headRefName}) | ([.[].h | scan("\\b([A-Z]{2,6})-[0-9]+\\b") | .[0]] + [.[].t | scan("linear\\.app/[^/ ]+/issue/([A-Za-z]{2,6})-[0-9]+") | .[0] | ascii_upcase] | unique) as $keys | map(select(.t | test("(?i)\\b(close[sd]?|fix(es|ed)?|resolve[sd]?)\\s+#[0-9]+|github\\.com/[^/ ]+/[^/ ]+/issues/[0-9]+"))) as $github | map(select((.h | test("^([A-Z]{2,6}-[0-9]+:|\\[[A-Z]{2,6}-[0-9]+\\])")) or (.t | test("(?i)\\b(close[sd]?|fix(es|ed)?|resolve[sd]?|refs?)\\s+[A-Z]{2,6}-[0-9]+|linear\\.app/[^/ ]+/issue/")))) as $linear | map(select(.b | test("^[a-z]{2,6}-[0-9]+-[a-z0-9]+(-[a-z0-9]+)+$"; "i")) | select((.b | capture("^(?<k>[A-Za-z]+)-").k | ascii_upcase) as $k | any($keys[]; . == $k))) as $branch | "github \($github | length), linear \($linear | length) of \(length); hint: \($branch | length) branches named like a Linear issue with a key seen in a title or link"' |
There was a problem hiding this comment.
P2 · Case-insensitive keyword regex counts ordinary titles as Linear links
The keyword branch of the Linear test runs under (?i), so [A-Z]{2,6}-[0-9]+ also matches lowercase tokens. Ordinary phrases then count as Linear issue links and inflate the number used to suggest linked-issue with tracker: linear.
Trigger: a merged pull request titled fix utf-8 handling or Fixes ES-2022 parsing is counted under linear, even though neither is an issue key.
Fix: keep the key pattern case-sensitive by applying (?i) only to the keyword part, or match the keyword with a separate case-insensitive test and the key with an uppercase-only test.
There was a problem hiding this comment.
Fixed: only the keyword is case-insensitive (?i:...); the key stays uppercase-only, so fixes utf-8 or refs abc-12 no longer count.
| ## 6. Finish | ||
|
|
||
| If the user said yes to the pull request check, write the workflow from [reference.md](reference.md) to `.github/workflows/corbits-triage-policy.yml`. It needs no secrets. There is none for Bitbucket Data Center. Then tell the user, in two lines, which rules are on and that the policy applies once the file is merged to the default branch. | ||
| If the user said yes to the pull request check, add the job from reference.md's "Pull request check (GitHub)", pinned to the commit it gives. When a workflow already runs on `pull_request` with no `paths`, `paths-ignore` or `branches` filter that could skip a pull request changing only `.corbits-triage.yml`, add the job to it; otherwise write the workflow to `.github/workflows/corbits-triage-policy.yml`. It needs no secrets. There is none for Bitbucket Data Center. |
There was a problem hiding this comment.
P2 · Reuse rule for an existing workflow ignores types filters and other skipping conditions
Only paths, paths-ignore and branches are treated as reasons to write a separate workflow. A pull_request trigger with types: [opened], or a workflow-level or job-level if, can also skip the run when .corbits-triage.yml changes.
Trigger: the CI workflow has on: pull_request: types: [opened] and the skill adds the job to it. Later edits to the policy file do not rerun the check.
Fix: also check for types, if conditions at workflow or job level, and any other filter that could skip a pull request changing only the policy file. When in doubt, write the separate file.
There was a problem hiding this comment.
Fixed: the job joins an existing workflow only when nothing could skip a policy-only change (no paths, paths-ignore, branches or types filter, no workflow- or job-level if, no other condition); otherwise or in doubt, the separate file is written.
| - id: invented-fallbacks | ||
| check: code-patterns | ||
| text: Added code handles a missing value instead of inventing an empty one. | ||
| patterns: | ||
| - id: no-console | ||
| - id: more-invented-fallbacks |
There was a problem hiding this comment.
P2 · invented-fallbacks overlaps the convention preset, so both can flag the same lines
reference.md says the conventions preset already flags invented ?? 0 and ?? [] fallbacks outside tests. Step 3 offers both as separate options, so a repository that picks both gets duplicate findings on the same lines.
Trigger: a maintainer picks both conventions and invented-fallbacks for a TypeScript repository, and a diff adds x ?? []. Both report it.
Fix: state the overlap in the option list, and either drop invented-fallbacks when conventions is chosen or make the skill offer it only as an alternative.
There was a problem hiding this comment.
Fixed: step 3 and the When to add row state the overlap; invented-fallbacks is offered only when conventions is not picked, never both.
| ## Pull request check (GitHub) | ||
|
|
||
| `.github/workflows/corbits-triage-policy.yml`: | ||
| Pin the policy-check action to the commit below. When a workflow already runs on `pull_request` and its trigger has no `paths`, `paths-ignore` or `branches` filter that could skip a pull request changing only `.corbits-triage.yml`, add the `corbits-triage-policy` job to its `jobs`; otherwise write the whole file as `.github/workflows/corbits-triage-policy.yml`. The check appears on each pull request as `<workflow name> / corbits-triage-policy`: |
There was a problem hiding this comment.
P2 · Renamed check is not flagged for branch protection updates
The job id change renames the published check from "Corbits Triage policy / check" to " / corbits-triage-policy". Any branch protection rule that requires the old name will wait forever on a check that no longer runs, and neither the skill nor the manual tells the maintainer to update required checks.
Trigger: a repository requires the "Corbits Triage policy / check" status before merge. After the rename, no run produces that name and merges are blocked.
Fix: add a note in reference.md, the manual and the action README telling maintainers to update required status checks to the new name, and mention this in the pull request description.
There was a problem hiding this comment.
Fixed: reference.md, the manual and the action README tell maintainers to change a required Corbits Triage policy / check status to the new name, and the PR description says so.
| - **Commit style**: whether `git log --format=%s -n 50` mostly reads `feat: …`, `fix(scope): …`, and which types. | ||
| - **Vendored or generated code** checked in: `vendor/`, `third_party/`, `dist/`, `generated/`. | ||
| - **Existing file**: if `.corbits-triage.yml` or `.corbits-triage.yaml` exists, read it and edit it instead of starting over. If both exist, Triage reads only `.yml`; tell the user and suggest deleting the other. | ||
| - **Size**: files and lines at the 90th percentile by nearest rank, each with the pull request at that rank. Suggest `max-files-changed` and `max-lines-changed` at those numbers rounded up, files to a multiple of 5 and lines to a multiple of 100, never above 30 files and 3,000 lines: "p90 is 23 files (#412), suggest 25". |
There was a problem hiding this comment.
P3 · Size suggestion can round to an invalid max of 0
The 90th-percentile value is rounded up to a multiple of 5 or 100 with only an upper cap. A p90 of 0 rounds to 0, but max must be at least 1, so a file written without a person answering would be refused.
Trigger: a repository whose recent merged pull requests change 0 files or 0 lines at the 90th percentile, such as empty or metadata-only merges.
Fix: enforce a floor of 1 after rounding, for example max(1, round_up(p90)), and state the floor in SKILL.md.
There was a problem hiding this comment.
Fixed: SKILL.md now says the rounded size suggestions are never below 1.
|
Independent review of 1985b85 against main. I ran every step 1 command from the repository root, and they print the same numbers as the description (issue links now give linear 49 of 50). The step 5 check prints valid for this repository's file, for a file built from those suggestions with measured evidence, for the sensitive-paths groups example and for the close-on-Fail actions example, and refuses Not ready to merge: the description fails this repository's own policy, and some instructions can be followed wrongly. Findings:
|
|
Addressing the independent review (comment 6458572053), all eleven fixed:
|
|
Second independent review of f5ec2bc against main. I ran every step 1 block fresh from the repository root. Size gives 35 files (#646) and 1,573 lines (#639) with lockfiles left out, using the same lockfile list as the size rules. Issue links give github 0 and linear 49 of 50. Merge style gives 0 of 50. Conventional titles give 50 of 50, the type list gives chore 4, docs 2, feat 12, fix 26, refactor 4, style 1 and test 1, and commits give 200 of 200. Authors give TheGreatAxios 50. The checked-in folders list The title and commit regexes now accept only the rule's default types, in any case. All eleven earlier findings are fixed. One finding is left:
|
|
Addressing comment 6458661538: SKILL.md and reference.md now suggest types only when the history uses a type outside the default list, and the suggested list is the whole default list plus those extra types. |
|
No findings: d52d7ff makes the types suggestion the default list plus each extra type in both SKILL.md and reference.md, which closes the last finding. |
There was a problem hiding this comment.
Rewrites the corbits-triage setup skill so it measures the repository and suggests policy rules for the maintainer to confirm, expands reference.md into a full catalog of policy options, and updates the manual, action README and setup-skill test. The main defect is that the GitHub issue-link count ignores bare #N references that the github tracker actually accepts, so the measured numbers can suggest the wrong tracker.
| ``` | ||
|
|
||
| Ask in one message together with the proposal from step 3, giving your suggested answer for each. Ask: | ||
| - **Issue references**: how many link an issue. GitHub counts a closing keyword (`Closes #12`, `fixes #12`, `resolved #12`) or an issue link, not a bare `#12`. Linear counts a title that starts with a key followed by a colon or in brackets (`ABC-123: …`, `[ABC-123] …`), a closing or `Refs` keyword, in any case, before an uppercase key (`Closes ABC-123`, `refs ABC-123`, not `fixes utf-8`), or a `linear.app` issue link. A branch named like a Linear issue (a 2 to 6 letter key, a number, then a slug of at least two words, such as `abc-123-fix-login`) is a hint, not a count: it is printed apart, and only when its key also appears in a title or a `linear.app` link among these pull requests, since `fix-123-login-bug` has the same shape. When most link one kind, suggest `linked-issue` with that tracker, `either` when both are common, and none when few do; the branch hint alone suggests nothing: "50 of 50 name a Linear key, suggest linked-issue with tracker linear". |
There was a problem hiding this comment.
P2 · GitHub issue-link count ignores bare #N, which the github rule counts
The skill's GitHub measurement counts only closing keywords and issue links, and says a bare #12 is not counted. The github rule accepts any #N preceded by whitespace or (, so the measured counts undercount what linked-issue with tracker github accepts.
Trigger: merged pull requests whose text says see #123 with no closing keyword. The skill reports github 0, but the rule passes them, so it may suggest the wrong tracker or none.
Fix: make the description and the jq filter match the rule. Count bare #N preceded by whitespace or ( as a GitHub reference, or state clearly in the skill that the measurement is narrower than the rule and why.
There was a problem hiding this comment.
Fixed: the GitHub count now matches the github rule (#N at the start or after a space or (, issue links, 123-title branches); the prose says commit messages are not read, so it can only undercount.
| Only when `linked-issue` is on, and the maintainer wants it, Fail can close a pull request that names no issue and ask for changes on the rest: | ||
|
|
||
| ```yaml | ||
| fail: |
There was a problem hiding this comment.
P2 · Close example is a fragment with a second fail key that collides when pasted under the earlier Actions example
The close example is a top-level fail: block, not nested under actions:. If it is pasted under the Actions example, which already has fail:, the file gets a duplicate key. The test only validates the first yaml block under each heading, so this snippet is never checked.
Trigger: a maintainer merges the close snippet into the existing fail: section, or writes it as a second fail: in one file.
Fix: show the close rule as a complete actions: example, or merge it into the existing fail: block, and extend the test so every yaml block under the actions headings is validated.
There was a problem hiding this comment.
Fixed: the close example is a complete actions: block, and the skill test now validates every yaml block under each heading (a broken second Actions block fails it).
| - Is the repository on GitHub or Bitbucket Data Center? Only when there is no remote. | ||
| - On GitHub, add a pull request check that checks this file and runs its rules that need no model on every pull request? Suggest yes. | ||
| ```sh | ||
| gh pr list --state merged --limit 50 --json number,title,body,headRefName --jq 'map({n: .number, h: .title, t: (.title + " " + .body), b: .headRefName}) | ([.[].h | scan("\\b([A-Z]{2,6})-[0-9]+\\b") | .[0]] + [.[].t | scan("linear\\.app/[^/ ]+/issue/([A-Za-z]{2,6})-[0-9]+") | .[0] | ascii_upcase] | unique) as $keys | map(select(.t | test("(?i)\\b(close[sd]?|fix(es|ed)?|resolve[sd]?)\\s+#[0-9]+|github\\.com/[^/ ]+/[^/ ]+/issues/[0-9]+"))) as $github | map(select((.h | test("^([A-Z]{2,6}-[0-9]+:|\\[[A-Z]{2,6}-[0-9]+\\])")) or (.t | test("\\b(?i:close[sd]?|fix(?:es|ed)?|resolve[sd]?|refs?)\\s+[A-Z]{2,6}-[0-9]+\\b|linear\\.app/[^/ ]+/issue/")))) as $linear | map(select(.b | test("^[a-z]{2,6}-[0-9]+-[a-z0-9]+(-[a-z0-9]+)+$"; "i")) | select((.b | capture("^(?<k>[A-Za-z]+)-").k | ascii_upcase) as $k | any($keys[]; . == $k))) as $branch | "of \(length): github \($github | length) (\($github | map("#\(.n)") | join(" "))), linear \($linear | length) (\($linear | map("#\(.n)") | join(" "))); hint: \($branch | length) branches named like a Linear issue with a key seen in a title or link"' |
There was a problem hiding this comment.
P3 · Branch-hint key set includes non-issue tokens like GPT-4 or SHA-1
$keys collects every [A-Z]{2,6}-[0-9]+ match from titles, not only Linear keys. A title that mentions GPT-4 therefore makes a branch named gpt-4-add-support count as a Linear hint, which breaks the stated rule that the hint needs a real issue key seen in a title or link.
Trigger: a title feat: GPT-4 support together with a branch gpt-4-add-support.
Fix: build $keys only from title keys that match the Linear title pattern (a key at the start followed by a colon or in brackets) or from linear.app links, the same sources the Linear count uses.
There was a problem hiding this comment.
Fixed: branch-hint keys come only from Linear-shaped titles (ABC-1: or [ABC-1]) and linear.app links, so GPT-4 no longer makes gpt-4-add-support a hint.
| ## 6. Finish | ||
|
|
||
| If the user said yes to the pull request check, write the workflow from [reference.md](reference.md) to `.github/workflows/corbits-triage-policy.yml`. It needs no secrets. There is none for Bitbucket Data Center. Then tell the user, in two lines, which rules are on and that the policy applies once the file is merged to the default branch. | ||
| If a workflow already uses `corbitsdev/corbits-triage/packages/policy-check-action`, under any job id, leave it as it is and tell the user it already runs; a branch protection rule may require its name. Otherwise, if the user said yes to the pull request check, add the job from reference.md's "Pull request check (GitHub)", pinned to the commit it gives. When a workflow already runs on `pull_request` and nothing could skip it for a pull request changing only `.corbits-triage.yml` (no `paths`, `paths-ignore`, `branches` or `types` filter, no workflow-level or job-level `if`, no other condition), add the job to it; otherwise, or when in doubt, write the workflow to `.github/workflows/corbits-triage-policy.yml`. It needs no secrets. There is none for Bitbucket Data Center. |
There was a problem hiding this comment.
P3 · Job may be added to a workflow that also triggers on non-PR events
The criteria check only that the workflow runs on pull_request and has no skip filters. A workflow that also runs on push or schedule would run the policy-check job on those events too, where there is no pull request.
Trigger: an existing workflow with on: [push, pull_request] and no filters.
Fix: require that the on: block lists pull_request as its only event, or that the job guards itself with an event check, before adding it to an existing workflow. Otherwise write the separate file.
There was a problem hiding this comment.
Fixed: the job joins an existing workflow only when pull_request is its only event and nothing could skip it; otherwise the separate file is written.
c2c2ca6 to
2609b48
Compare
|
No findings on 2609b48. The GitHub count matches the rule's own pattern in checks.ts on sample strings under gh's jq. Branch-hint keys come only from Linear-form titles and linear.app links. The setup-skill test passes and fails when the close example is broken, and lint passes. |
There was a problem hiding this comment.
Completes the corbits-triage-setup skill so it analyzes the repository, suggests policy rules with measured numbers, and leaves the choice to the maintainer. It also expands reference.md into a full catalog of policy options and adds a pull request check job to the docs. One defect to fix: the vendored-folder grep in SKILL.md can miss top-level folders because its greedy prefix reports only the deepest match.
| - **Vendored or generated code**: which `vendor/`, `third_party/`, `dist/` or `generated/` folders are checked in, and which pull requests changed them. Checked in and changed in some: suggest a `sensitive-paths` group, since the team changes it on purpose. Checked in and changed in none: list `no-forbidden-paths` as an option only, for a person to confirm; it fails maintainers too, so it is never written without one. | ||
|
|
||
| ```sh | ||
| git ls-files | grep -oE '^(.*/)?(vendor|third_party|dist|generated)/' | sort -u |
There was a problem hiding this comment.
P3 · Greedy prefix in vendored-folder grep reports the deepest match instead of the checked-in folder
Why it is wrong: ^(.*/)? is greedy, so on a path with several matching folder names, grep reports only the last (deepest) one. The top-level vendored folder can then disappear from the output.
Triggering input: a repository whose only matching file is vendor/pkg/dist/index.js. The command prints vendor/pkg/dist/ and never lists vendor/, so a suggested no-forbidden-paths or sensitive-paths glob would miss the real checked-in folder.
Suggested fix: make the prefix non-greedy or anchor it to each path segment, for example:
git ls-files | grep -oE '(^|/)(vendor|third_party|dist|generated)/' | sed 's#^/##' | sort -u
Then rerun it against a repository with nested matches to confirm the top-level folder is printed.
2609b48 to
4fac8f5
Compare
TheGreatAxios
left a comment
TheGreatAxios
left a comment
There was a problem hiding this comment.
Independent review of 4fac8f5 against CL-11608.
Verified: setup-skill test passes, bun run lint exits 0, every step 1 and step 5 shell block runs from this repo's root (size p90 33 files #678 / 1425 lines #702; linear 49 of 50; squash 14 of 50; 200 of 200 commits conventional; the BBDC fallback block reads 50 commits). The action pin 8446967 is the latest commit touching packages/policy-check-action on origin/main. The job id corbits-triage-policy is the same in the skill, README and manual. The manual's label paragraph keeps main's text (labels come off once the rule passes). No attribution lines. Every key and check the skill names exists in policy-file.ts and check-pack.ts, including no-duplicate, duplicates-open-pull-request and the reserved duplicates-open-pull-request- prefix. Outcome items 1 to 7 are met.
Findings:
- Outcome item 8 is only partly met. The test reads only the
Starter file,Extras, written out,Actions,LabelsandBlocks and decidesections. Thesensitive-pathsgroupsexample under## Rulesis never validated, yet step 4 tells the agent to write from it. I checked it by hand by swapping it in for the starter's sensitive-paths rule, and it is valid. Still, the PR body's claim that the test "validates every YAML block the skill tells an agent to write" is not true. Fix: cover that block in the test, swapping it in for the starter rule since a built-in may appear only once. - The vendored-folder command throws away where the folder is.
git ls-files | grep -oE '(^|/)(vendor|...)/' | sed 's#^/##'printsdist/forpackages/a/dist/x.js. Globs match the full path from the root (globMatchin packages/triage-workflows/src/logic/glob.ts usesmatches), so theno-forbidden-pathsorsensitive-pathsglob an agent writes from this output, in the shape of the reference'svendor/**, never matches a nested folder. Fix: print each matching folder with its full path, or tell the agent to write**/<folder>/**for a nested one. - On Bitbucket Data Center, the merge-style block and the 200-commit conventional-subject block find the default branch with
gh repo view, which fails there. The fallback block only prints 50 raw messages (1,285 lines here) and leaves the agent to count by hand. Fix: find the branch withgit symbolic-ref refs/remotes/origin/HEAD, as the fallback block already does, so the commit-count block runs on both hosts. - The squash left something behind. The commit message body is a second subject line:
docs(skill): complete the setup skill for agent runs. - The PR description's last two bullets, "The vendored-folder command now prints a top-level folder..." and "Rebased onto main...", describe how the branch got here rather than what changed against main. Drop them.
4fac8f5 to
f845ad5
Compare
TheGreatAxios
left a comment
TheGreatAxios
left a comment
There was a problem hiding this comment.
No findings.
There was a problem hiding this comment.
Rewrites the corbits-triage setup skill so the agent analyzes the repository and suggests policy rules with measured numbers, leaving the choice to the maintainer, and turns reference.md into a catalog of accepted policy fields. It also adds a policy-check job to existing workflows under a new id and updates the user manual and action README to match. The main defect is that the workflow join rule ignores concurrency cancellation, which could cause a newer push to cancel the policy check.
| echo "$(git log --no-merges --format=%s -n 200 "origin/$branch" | grep -ciE '^(feat|fix|refactor|perf|docs|test|chore|ci|build|style|revert)(\([^)]*\))?!?: ') of $(git rev-list --no-merges --count --max-count=200 "origin/$branch")" | ||
| ``` | ||
|
|
||
| - **Authors**: who opened them. The people who open most are the candidate maintainers for actions, alongside CODEOWNERS. |
There was a problem hiding this comment.
P2 · Commit count treats Revert commits as non-conventional, though the rule skips them
The rule skips Revert "…" commits, but this grep counts them in the total and never as conventional. The conventional share comes out lower than what the rule sees, which can push the skill toward suggesting fields: [title].
Trigger: a repository whose history has several Revert "…" commits from git revert.
Fix: exclude commits whose subject starts with Revert " from both the numerator and the denominator, so the measurement matches the rule's behavior.
| ## 6. Finish | ||
|
|
||
| If the user said yes to the pull request check, write the workflow from [reference.md](reference.md) to `.github/workflows/corbits-triage-policy.yml`. It needs no secrets. There is none for Bitbucket Data Center. Then tell the user, in two lines, which rules are on and that the policy applies once the file is merged to the default branch. | ||
| If a workflow already uses `corbitsdev/corbits-triage/packages/policy-check-action`, under any job id, leave it as it is and tell the user it already runs; a branch protection rule may require its name. Otherwise, if the user said yes to the pull request check, add the job from reference.md's "Pull request check (GitHub)", pinned to the commit it gives. When a workflow's only event is `pull_request` and nothing could skip it for a pull request changing only `.corbits-triage.yml` (no `paths`, `paths-ignore`, `branches` or `types` filter, no workflow-level or job-level `if`, no other condition), add the job to it; otherwise, or when in doubt, write the workflow to `.github/workflows/corbits-triage-policy.yml`. It needs no secrets. There is none for Bitbucket Data Center. |
There was a problem hiding this comment.
P2 · Join rule ignores concurrency cancellation, so the policy job can be cancelled by a newer push
The join rule only excludes paths, branches, types and if-conditions. A workflow-level concurrency block with cancel-in-progress: true, like the repository's own ci.yml (lines 14-16), cancels the in-flight policy job when a new commit is pushed. The check then ends cancelled, which the rules treat as unconfirmed, and it can stall a branch protection rule.
Trigger: a repository whose pull_request-only workflow has concurrency: {group: ..., cancel-in-progress: true} gets the corbits-triage-policy job appended, and a second push cancels the first run's check.
Fix: add "no workflow-level concurrency with cancel-in-progress" to the list of disqualifiers in the join rule, and write a separate workflow file in that case.
| ## Pull request check (GitHub) | ||
|
|
||
| `.github/workflows/corbits-triage-policy.yml`: | ||
| Pin the policy-check action to the commit below. When a workflow's only event is `pull_request` and nothing could skip it for a pull request changing only `.corbits-triage.yml` (no `paths`, `paths-ignore`, `branches` or `types` filter, no workflow-level or job-level `if`, no other condition), add the `corbits-triage-policy` job to its `jobs`; otherwise, or when in doubt, write the whole file as `.github/workflows/corbits-triage-policy.yml`. When a workflow already uses this action, under any job id, leave it as it is. The check appears on each pull request as `<workflow name> / corbits-triage-policy`. A workflow written with the earlier job id, `check`, keeps reporting `Corbits Triage policy / check` until someone edits it; when that job is replaced with this one, change a branch protection rule or ruleset that requires the old name to the new one: |
There was a problem hiding this comment.
P2 · Join criteria in the reference omit concurrency, the same gap as SKILL.md
The reference repeats the 'nothing could skip it' test without listing concurrency with cancel-in-progress, so an agent following this block can add the job to a workflow that cancels runs and gives the check a cancelled result.
Trigger: an agent follows the reference's join rule for a workflow with a concurrency group that cancels in-progress runs.
Fix: add the concurrency condition to the disqualifier list here, matching the fix in SKILL.md.
| git remote set-head origin --auto >/dev/null | ||
| branch="$(git symbolic-ref --short refs/remotes/origin/HEAD | sed 's|^origin/||')" | ||
| git fetch --unshallow origin 2>/dev/null || git fetch origin "$branch" |
There was a problem hiding this comment.
P3 · Default-branch lookup leaves branch empty when set-head fails
If git remote set-head origin --auto fails (no network or auth, or the remote is not called origin), symbolic-ref fails and branch is empty. The later commands then run against origin/, and their errors or zero counts can be read as real measurements.
Trigger: an agent run without credentials for the remote, or a clone whose remote has a different name.
Fix: stop and report when the branch is empty, for example by testing [ -n "$branch" ] || { echo 'default branch not found'; exit 1; } before running the measurements.
|
|
||
| ## 2. Ask only what you can't tell | ||
| ```sh | ||
| gh pr list --state merged --limit 50 --json number,changedFiles,additions,deletions,files --jq 'if length == 0 then "no merged pull requests" else map((.files | map(select(.path | test("(^|/)(bun\\.lockb?|package-lock\\.json|npm-shrinkwrap\\.json|yarn\\.lock|pnpm-lock\\.yaml|Cargo\\.lock|go\\.sum|poetry\\.lock|uv\\.lock|Gemfile\\.lock|composer\\.lock)$")))) as $locks | {number, files: (.changedFiles - ($locks | length)), lines: (.additions + .deletions - ($locks | map(.additions + .deletions) | add // 0))}) | (length * 0.9 | ceil - 1) as $i | sort_by(.files)[$i] as $f | sort_by(.lines)[$i] as $l | "files \($f.files) (#\($f.number)), lines \($l.lines) (#\($l.number)); read \(map("#\(.number)") | join(" "))" end' |
There was a problem hiding this comment.
P3 · Lockfile subtraction misses lockfiles when gh truncates the files list
gh returns at most 100 entries in files, while changedFiles counts every file. A lockfile past the first 100 is not subtracted, so file and line counts for large pull requests are too high and push the p90 up.
Trigger: a merged pull request that changes more than 100 files, including a lockfile.
Fix: fetch the complete file list for each pull request (for example via the REST files endpoint with pagination), or skip pull requests where changedFiles exceeds the number of entries returned and say so in the output.
The corbits-triage-setup skill no longer picks settings. It analyzes the repository, suggests rules with the numbers it measured, and leaves the choice to the maintainer. When nobody can answer, it writes the suggested values and lists each one under "Waiting on you" in the pull request body. reference.md is now a full catalog of what the policy file accepts and prescribes nothing.
corbits-triage-policyin the skill, the user manual and the action README, and the pinned action commit is the current one on main.Verified: the setup-skill test passes and validates every YAML block the skill tells an agent to write, including the sensitive-paths groups example.
bun run linthas no errors. I ran the step 1 commands from the repository root and checked the counting patterns on sample titles and branches. The step 5 check prints valid for the starter file and for a file built from the suggestions.Closes CL-11608