Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ Every rule below is a level-two section of [`GOVERNANCE.md`](./GOVERNANCE.md). R
| A live config repo rather than a code repo | `Operational Repositories`, packaged as the `operational-vs-release-workflow` Skill |
| Onboarding a repo or running a conformance sweep | `Repository Onboarding and Conformance` (hub only, not carried). Standing up a new repo from a hub checkout is packaged as the `standup-a-repo` Skill, resyncing one already stood up the same way is `resync-a-repo`, and measuring a named repo against the fleet ground truth per `AUDIT.md` is `audit-a-repo`, all hub-context only |
| Running a fleet gate, the review digest, or the config script | `Hub-Hosted Tooling` |
| Running a lint or format check locally, or a lint tool missing from `command -v` | `Running the Linters Locally (Known-Working Invocations)` |
| Writing a commit message or pull request title | `Pull Request Title and Commit Message Conventions`, packaged as the `comment-and-doc-style` Skill |
| Any prose, comment, doc, or line-ending change | `Documentation Style Conventions`, packaged as the `comment-and-doc-style` Skill |
| Proving work actually happened | `Verification Discipline`, surfaced at its decision moment by the `agent-conduct` Skill, and the section keeps the full rules |
Expand Down
2 changes: 1 addition & 1 deletion CODESTYLE.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ This is packaged as the `python-codestyle` Skill at `.agents/skills/python-codes

## Shell

Bash, and only where a program cannot be Python: a bootstrap that installs the interpreter cannot be written in it, and a host tool that must run before a development toolchain exists cannot depend on one. Everything else is Python, with a test under the scripts tree's `tests/` directory. The mandatory `set -Eeuo pipefail` header, the pipefail-versus-early-reader pitfall, self-locating scripts, the `shellcheck`-plus-`shfmt` clean-compile, and the why-not-what comment rule are packaged as the `shell-codestyle` Skill at `.agents/skills/shell-codestyle/SKILL.md` in the hub, not a repo-relative link since that path is hub-local and not carried into every fleet repo. Read the skill for the full rules.
Bash, and only where a program cannot be Python: a bootstrap that installs the interpreter cannot be written in it, and a host tool that must run before a development toolchain exists cannot depend on one. Everything else is Python, with a test under the scripts tree's `tests/` directory. The mandatory `set -Eeuo pipefail` header, the pipefail-versus-early-reader pitfall, self-locating scripts, the `shellcheck`-plus-`shfmt` clean-compile, and the why-not-what comment rule are packaged as the `shell-codestyle` Skill at `.agents/skills/shell-codestyle/SKILL.md` in the hub, not a repo-relative link since that path is hub-local and not carried into every fleet repo. Read the skill for the full rules. Run the clean-compile check itself per [GOVERNANCE.md "Running the Linters Locally"][governance-running-the-linters-locally], not by probing `command -v shellcheck`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

1. codestyle.md duplicates lint rule 📘 Rule violation ⚙ Maintainability

The added sentence restates the canonical GOVERNANCE.md rule that a missing native shellcheck
binary does not make the check unavailable. The non-canonical file should only point readers to the
governing section.
Agent Prompt
## Issue description
`CODESTYLE.md` partially restates the canonical rule about handling a missing native lint binary.

## Issue Context
Cross-cutting conditions and obligations must remain in `AGENTS.md` or `GOVERNANCE.md`. Other files may reference the canonical section without repeating its substance.

## Fix Focus Areas
- CODESTYLE.md[54-54]
- GOVERNANCE.md[215-217]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


<!-- Repo -->

Expand Down
4 changes: 2 additions & 2 deletions GOVERNANCE.md
Original file line number Diff line number Diff line change
Expand Up @@ -212,9 +212,9 @@ This section and [`WORKFLOW.md`](./WORKFLOW.md) keep the full rules, this sectio
- **Docker layer cache**: cache to/from a registry tag (`type=registry`, e.g. `buildcache-<branch>` on Docker Hub), not the GitHub Actions cache (`type=gha`), to keep large image layers off the 10 GB Actions cache. A **multi-image** repo uses a **per-image** buildcache tag (`<repo>:buildcache-<branch>` for each image, plus the base image's own tag and inline cache). It does not fall back to `type=gha` for the extra images.
- **Tag pinning on releases**: when using `softprops/action-gh-release` (or any tag-creating action), pass `target_commitish` explicitly, because without it GitHub's REST API defaults the new tag to the repository's default branch instead of the commit that built the artifact. Pin it to the **exact built commit's SHA** (the publisher uses NBGV's `GitCommitId` output), not `github.sha` (which may differ from the exact commit NBGV versioned) and not a branch name (a moving ref that a mid-run commit could advance past the built tree).

### Running the Linters Locally (Known-Working Invocations)
## Running the Linters Locally (Known-Working Invocations)

CI runs the full lint set, but run the linters locally before pushing to catch issues early, so an agent must know how to invoke them. Their non-Docker install paths (curl-pipe installers, global npm) are frequently blocked in sandboxes or fail on WSL, so **prefer the Docker invocations below, the known-working path that needs no local toolchain.** These tools auto-discover their targets from the working directory.
CI runs the full lint set, but run the linters locally before pushing to catch issues early, so an agent must know how to invoke them. Their non-Docker install paths (curl-pipe installers, global npm) are frequently blocked in sandboxes or fail on WSL, so **prefer the Docker invocations below, the known-working path that needs no local toolchain.** These tools auto-discover their targets from the working directory. None of markdownlint, cspell, actionlint, editorconfig-checker, shellcheck, shfmt, or PSScriptAnalyzer is installed natively on a fleet host, by decision, so a `command -v <tool>` miss means exactly that and not that the check is unavailable here: read this section for the working invocation before reporting a check as skipped or unavailable.

**Each surface runs the lint with the tool that fits it, all from the same config files** (`.markdownlint-cli2.jsonc`, `cspell.json`, `.editorconfig`):

Expand Down
4 changes: 2 additions & 2 deletions catalog/snippets/pre-commit/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ the commit rather than silently skipping the gate.

Install and enable with `uv tool install pre-commit` once, then `pre-commit install`.
`pre-commit` itself is never added as a project dependency: the lint-only profile has no
project environment to add it to, and `uv tool install` gives a persistent, PATH-available
command independent of any project, the same footing `uvx` gives the tools the hooks run.
project environment to add it to, and `uv tool install` gives a persistent command independent
of any project. The hooks use `uvx` to run tools independently of the project.
If `pre-commit install` reports the command not found right after installing it, `uv tool
install`'s own bin directory is not yet on `PATH`: run `uv tool update-shell` and restart or
re-source the shell, or add the directory `uv tool dir --bin` prints directly.
Expand Down
33 changes: 22 additions & 11 deletions scripts/pr_review.py
Original file line number Diff line number Diff line change
Expand Up @@ -522,19 +522,30 @@ def copilot_history(owner: str, repo: str) -> list[tuple[int, dict]]:
"""The reviewer's own reviews and comments across the repository's most recently updated
pull requests, newest activity first regardless of which connection it came from.

Read at HISTORY_PRS first and, only where that comes back with nothing at all, read again at
the wider HISTORY_PRS_WIDE. A narrow window emptying out is the ordinary case, a repository
whose most recent activity genuinely carries none of the reviewer's, and costs nothing beyond
the one call either caller below was always going to make. It stops being ordinary once an
outage outlasts HISTORY_PRS pull requests: every one of them then carries the same silence,
the narrow window empties out too, and both callers would otherwise fall back to blind
polling for the rest of the outage with no way to tell that outage apart from a repository
that has simply never seen a Copilot review (#985, reproduced on ptr727/ProjectTemplate
PRs #981-984). The wider read is what tells the two apart, and it is tried only once the
narrow one is empty, so the ordinary case still costs one call rather than two.
Read at HISTORY_PRS first and, only where that comes back carrying no usable bot id, read
again at the wider HISTORY_PRS_WIDE. A narrow window emptying out is the ordinary case, a
repository whose most recent activity genuinely carries none of the reviewer's, and costs
nothing beyond the one call either caller below was always going to make. It stops being
ordinary once an outage outlasts HISTORY_PRS pull requests: every one of them then carries
the same silence, the narrow window empties out too, and both callers would otherwise fall
back to blind polling for the rest of the outage with no way to tell that outage apart from a
repository that has simply never seen a Copilot review (#985, reproduced on
ptr727/ProjectTemplate PRs #981-984). The wider read is what tells the two apart.

Emptying out is not the only way the narrow window fails a bot-id lookup, though: it can
carry real activity and still have none, when every entry within it is a plain comment. A
formal review, `copilot_bot_id`'s only source for the id, can sit just outside the narrow
window while a newer comment sits inside it, and returning the narrow read as soon as it has
anything at all left that review permanently unread. Widening is keyed on
`copilot_bot_id(entries)` rather than on emptiness for exactly that case, so a comment-only
narrow window still triggers the wider read the same way an empty one does. The ordinary case
-- a narrow window already carrying a review -- still costs one call rather than two, since
that is the common shape a usable bot id already satisfies.
"""
entries = _copilot_history_window(owner, repo, HISTORY_PRS)
return entries if entries else _copilot_history_window(owner, repo, HISTORY_PRS_WIDE)
if entries and copilot_bot_id(entries) is not None:
return entries
return _copilot_history_window(owner, repo, HISTORY_PRS_WIDE)


def _copilot_history_window(owner: str, repo: str, prs: int) -> list[tuple[int, dict]]:
Expand Down
23 changes: 23 additions & 0 deletions scripts/tests/test_pr_review.py
Original file line number Diff line number Diff line change
Expand Up @@ -2790,6 +2790,29 @@ def fake(_query: str, **variables: object) -> dict:
pr_review.copilot_history("o", "r")
self.assertEqual([pr_review.HISTORY_PRS], seen)

def test_a_comment_only_narrow_window_still_widens(self) -> None:
"""A narrow window carrying activity is not itself a usable bot id: a comment carries
none, per `copilot_bot_id`, so this must widen the same as an empty window would rather
than returning a history no caller can read a bot id from."""
self.answer(payload([]))
seen: list[object] = []
narrow_node = {
"number": 970,
"reviews": {"nodes": []},
"comments": {"nodes": [comment(at=LATE)]},
}

def fake(_query: str, **variables: object) -> dict:
seen.append(variables["prs"])
if variables["prs"] == pr_review.HISTORY_PRS:
return {"repository": {"pullRequests": {"nodes": [narrow_node]}}}
return {"repository": {"pullRequests": {"nodes": [hist_review(900, QUOTA_REFUSED)]}}}

self.enterContext(mock.patch.object(pr_review, "gh_graphql", side_effect=fake))
history = pr_review.copilot_history("o", "r")
self.assertEqual([pr_review.HISTORY_PRS, pr_review.HISTORY_PRS_WIDE], seen)
self.assertEqual("BOT_1", pr_review.copilot_bot_id(history))

def test_both_windows_empty_still_carries_no_signal_and_no_bot_id(self) -> None:
"""An outage wide enough to empty HISTORY_PRS_WIDE too is a real, if rarer, case: still
no id to request with and no fabricated one, rather than a crash on the second call."""
Expand Down
42 changes: 42 additions & 0 deletions scripts/tests/test_spec_validate.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import sys
import tempfile
import time
import unittest
from pathlib import Path

Expand Down Expand Up @@ -188,6 +189,40 @@ def test_a_reference_style_markdown_link_is_rejected(self) -> None:
["Fixture: description carries Markdown links - keep it link-free plain text"],
)

def test_a_nested_bracket_link_label_is_rejected(self) -> None:
# Regresses a gap where `[^\]]*` stopped at the first `]` and missed a label with its own brackets.
self.assertEqual(
validate.description_errors(
"Fixture", "See [API [docs]](https://example.test) for more."
),
["Fixture: description carries Markdown links - keep it link-free plain text"],
)

def test_a_destination_with_two_parenthesized_groups_is_rejected(self) -> None:
# Regresses the matching gap on the destination side: more than one balanced `()` run after the link.
self.assertEqual(
validate.description_errors(
"Fixture", "See [docs](https://example.test/a_(b)_(c)) for more."
),
["Fixture: description carries Markdown links - keep it link-free plain text"],
)

def test_a_link_nested_inside_a_non_link_bracket_run_is_still_rejected(self) -> None:
# A failed outer span used to jump past the whole run instead of retrying one character in.
# That skipped the valid inner link in `[[docs](url)]` (#1011, qodo).
self.assertEqual(
validate.description_errors("Fixture", "See [[docs](url)] for more."),
["Fixture: description carries Markdown links - keep it link-free plain text"],
)

def test_an_escaped_bracket_inside_a_label_does_not_corrupt_the_match(self) -> None:
# A backslash-escaped `\[` used to count as real nesting, corrupting the label match.
# It reads as a literal character instead (#1011, qodo).
self.assertEqual(
validate.description_errors("Fixture", r"See [API \[docs](url) for more."),
["Fixture: description carries Markdown links - keep it link-free plain text"],
)

def test_leading_or_trailing_whitespace_is_rejected(self) -> None:
# Not silently trimmed here, even though spec/audit.py and configure.sh both strip it defensively.
# Rejecting it at the source keeps the registry's own text the exact canonical form every mirror carries.
Expand All @@ -206,6 +241,13 @@ def test_an_embedded_newline_is_rejected(self) -> None:
],
)

def test_a_long_run_of_unmatched_brackets_stays_linear(self) -> None:
# A run of unmatched '[' used to re-scan the remaining text from every position.
# That was O(N^2) (#1011, CodeRabbit), and a slow run here means a regression back to it.
start = time.monotonic()
validate.contains_description_markdown_link("[" * 20000)
self.assertLess(time.monotonic() - start, 1.0)
Comment on lines +247 to +249

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

6. Wall-clock checks can flake 🐞 Bug ☼ Reliability

The new regression test fails solely when one invocation takes over one wall-clock second, so a
paused, contended, or coverage-instrumented CI process can fail even though _bracket_matches()
remains linear. The same timing gate is duplicated in spec/audit.py --selftest, making the
validation workflow vulnerable in two places.
Agent Prompt
## Issue description
The new complexity regression checks use a fixed one-second wall-clock threshold, which can fail nondeterministically when CI is paused or contended even if the implementation remains linear.

## Issue Context
Both the unit suite and `spec/audit.py --selftest` run under coverage in the validation workflow, adding variable instrumentation and host overhead. Keep regression coverage deterministic; move performance measurement to a non-gating benchmark or verify the algorithm without a wall-clock deadline.

## Fix Focus Areas
- scripts/tests/test_spec_validate.py[244-249]
- spec/audit.py[3492-3505]
- .github/actions/validate/action.yml[28-35]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


def test_exactly_the_cap_is_clean(self) -> None:
self.assertEqual(validate.description_errors("Fixture", "a" * 100), [])

Expand Down
Loading
Loading