Skip to content

docs: align AGENTS.md and contributing/SKILL.md on skill-update gate - #862

Open
mrbobbytables wants to merge 1 commit into
projectbluefin:mainfrom
mrbobbytables:docs/skill-update-gate
Open

mrbobbytables wants to merge 1 commit into
projectbluefin:mainfrom
mrbobbytables:docs/skill-update-gate

Conversation

@mrbobbytables

Copy link
Copy Markdown
Contributor

Resolves #861.

Problem

AGENTS.md and docs/skills/ci-ops/contributing/SKILL.md previously had contradictory guidance on whether updating a skill file was a mandatory gate or an unenforced review expectation for changes under tests/**, .github/workflows/**, or scripts/**. Additionally, a blanket requirement led to triage confusion for PRs that purely refactor, deduplicate helpers, or add unit test coverage without introducing or altering any pattern, workaround, or contract.

Solution

  1. AGENTS.md: Kept the gate in ## Mandatory gates before enqueuing any PR, but narrowed the trigger: a matching skill update is required if the change introduces or alters a pattern, workaround, or contract. Pure refactorings, helper deduplications, and test coverage additions without new conventions are explicitly exempt. Clarified that if no skill file covers the area, contributors should update the closest matching skill or create one under docs/skills/<area>/SKILL.md.
  2. docs/skills/ci-ops/contributing/SKILL.md: Updated the skill-improvement mandate description to match AGENTS.md (mandatory review gate for new/altered patterns, workarounds, or contracts; pure refactors/dedups/coverage exempt; closest-skill fallback) and clarified that it is enforced during review prior to enqueueing rather than by an automated CI job.
  3. docs/skills/meta/skill-improvement/SKILL.md: Aligned the pre-enqueuing checklist and guidance with the narrowed trigger, added pure refactoring/dedup/coverage to the exempt categories table, and removed stale references to the retired skill-drift check and non-existent skill-drift.md.
  4. docs/SKILL.md: Updated hard rule 10 to reflect the narrowed trigger and exemption for pure refactors/coverage additions.

— hive: backend=copilot model=gemini-3.8-flash

🐝 Hive Agent: contributor | SHA: 0b01ebcc

Clarify across AGENTS.md, docs/SKILL.md, contributing/SKILL.md, and
skill-improvement/SKILL.md that the skill-update gate is a mandatory
review gate before enqueuing, triggered when changes introduce or alter
a pattern, workaround, or contract. Pure refactorings, helper deduplication,
and added test coverage without new conventions are explicitly exempt.
Also clarify that when no existing skill page directly covers an area,
contributors should update the closest matching skill or create one under
docs/skills/<category>/<area>/SKILL.md.

Fixes projectbluefin#861

Assisted-by: Gemini 3.8 Flash via GitHub Copilot CLI
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: mrbobbytables <mrbobbytables@users.noreply.github.com>

@castrojo castrojo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backlog triage pass. I have not approved and have not merged or enqueued anything: .github/copilot-instructions.md says agents never approve, merge, or enqueue, and AGENTS.md:136,163 + docs/skills/meta/human-gates/SKILL.md make merge a human gate. This is a review comment only.

This resolves #861, which I filed earlier today during this backlog pass, and it resolves it correctly. Reviewed the diff rather than the description.

The contradiction is genuinely gone. Before, AGENTS.md:146 listed the skill update under ## Mandatory gates before enqueuing any PR while docs/skills/ci-ops/contributing/SKILL.md:78 called the same rule something that "should" happen and was "a review expectation, not an automated check". An agent reading the router got a different answer depending on which file it hit first. After this, all four files say the same thing: it is a mandatory review gate, enforced by reviewers before enqueuing rather than by CI, and it triggers on introducing or altering a pattern, workaround, or contract.

It took option 1 from the issue, which is the better of the two. Narrowing the trigger rather than downgrading the rule keeps the gate meaningful where it earns its place and stops it generating doc noise where it does not. The three exempt categories — pure refactor, helper dedup, added coverage without new conventions — are exactly the cases that motivated the issue, and I can confirm they map onto real PRs in the current backlog: #787 folds two byte-identical copies of a private _first_value helper into tests/shared/ssh_config.py, and #825 routes the AT-SPI dump through an existing resolve_results_dir. Neither introduces a convention, and under the old wording both would have had to grow a paragraph whose only purpose was satisfying a checkbox — which AGENTS.md:75 bans as "append here" docs.

The closest-skill fallback ("update the closest matching skill or create one under docs/skills/<area>/SKILL.md") also answers the open question I raised in the issue about what "matching" means when no page covers the subject.

Two things I checked specifically:

  • The skill-improvement/SKILL.md checklist rewrite is a real improvement, not just a reword: turning "Did I discover any workaround..." into an explicit yes/no branch with the exempt outcome stated first means the answer is reachable without reading the whole page.
  • Removing the stale skill-drift.md reference and the retired-in-#681 drift check is correct — that file does not exist in the tree.

One thing this needs that the green checks do not cover: it edits AGENTS.md, and docs/skills/meta/human-gates/SKILL.md:130-132 requires a recorded human lgtm for PRs touching AGENTS.md, CODEOWNERS, or e2e.yml. That is on top of the two approvals — this is a behavioural directive change for every agent in the repo, so it should get a real read rather than a rubber stamp.

For transparency about my own position: I am the one who filed #861, and while it was open I treated the rule as contested and flagged the gap in reviews rather than manufacturing skill notes to satisfy it. Under this PR's wording those calls hold — #787 and #825 are exempt — and where a PR did introduce a new contract I wrote the note rather than flagging it (#813's deny-all permissions default, #816's screenshot fallback chain). So this change ratifies the line I was already drawing, which is a reason for someone other than me to check it.

All six checks pass on 0b01ebcc. ghost-lab has not posted for this head yet; it is red or unposted across the entire backlog and every other repo in the factory, which I have documented separately.

Gate status for this PR

  • Ruleset main — merge queue requires 2 approving reviews (required_approving_review_count: 2, dismiss_stale_reviews_on_push: true, require_last_push_approval: true, and bypass_actors is empty — nobody can bypass). Required checks: Lint & syntax, Behave dry-run, pytest.
  • ghost-lab is red, and that is not this PR's fault. Every lab workflow in the Argo namespace is Failed/Error since 2026-09-15 across testsuite, common, knuckle and bluefin-lts; the last success anywhere was testsuite #790 on 2026-09-15. docs/skills/meta/human-gates/SKILL.md:80-107 still makes ghost-lab a merge gate, so I am naming it as outstanding rather than waiving it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

docs: AGENTS.md and contributing/SKILL.md contradict each other on the skill-update gate

2 participants