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
7 changes: 4 additions & 3 deletions .agents/skills/comment-and-doc-style/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -189,9 +189,10 @@ gate. It reports a prose comment that opens its own line, in the diff's scope. A
modified line as an added one, so rewording one and re-indenting one each report it. That is the
rule's cost and the label is its answer, since a comment worth keeping takes the same label as a
comment worth writing. A trailing comment is out of scope. That is because which mid-line marker
opens a comment differs by language in ways a gate cannot settle from the marker alone. A docstring is not
a comment line, an instruction to a tool is not a comment the rule reads, and Markdown is out of
scope.
opens a comment differs by language in ways a gate cannot settle from the marker alone. A line
holding code after a block comment's closer is out of scope too, since deleting it deletes that
code. A docstring is not a comment line, an instruction to a tool is not a comment the rule reads,
and Markdown is out of scope.

Deleting the comment is the ordinary answer, since the bullets above already say what one has to
earn. Where a comment is genuinely owed, and a rule requiring one is the clearest case of that, the
Expand Down
Original file line number Diff line number Diff line change
@@ -1 +1 @@
846e20f174fbe3a7
921c0c80d236c9a8
Original file line number Diff line number Diff line change
Expand Up @@ -189,9 +189,10 @@ gate. It reports a prose comment that opens its own line, in the diff's scope. A
modified line as an added one, so rewording one and re-indenting one each report it. That is the
rule's cost and the label is its answer, since a comment worth keeping takes the same label as a
comment worth writing. A trailing comment is out of scope. That is because which mid-line marker
opens a comment differs by language in ways a gate cannot settle from the marker alone. A docstring is not
a comment line, an instruction to a tool is not a comment the rule reads, and Markdown is out of
scope.
opens a comment differs by language in ways a gate cannot settle from the marker alone. A line
holding code after a block comment's closer is out of scope too, since deleting it deletes that
code. A docstring is not a comment line, an instruction to a tool is not a comment the rule reads,
and Markdown is out of scope.

Deleting the comment is the ordinary answer, since the bullets above already say what one has to
earn. Where a comment is genuinely owed, and a rule requiring one is the clearest case of that, the
Expand Down
32 changes: 27 additions & 5 deletions .github/actions/prose-gate/prose_lint.py
Original file line number Diff line number Diff line change
Expand Up @@ -476,13 +476,17 @@ def discover(


class Line(NamedTuple):
"""One source line of a unit, a comment's text without its marker and `raw` with it."""
"""One source line of a unit, a comment's text without its marker and `raw` with it.

`trailed` marks a line where code follows the comment's closer, so deleting it deletes code.
"""

number: int
text: str
leading: bool = True
raw: str = ""
help: bool = False
trailed: bool = False


class Unit(NamedTuple):
Expand Down Expand Up @@ -615,6 +619,17 @@ def comment_tokens(text: str, lexer: str) -> list[tuple[int, str]]:
return out


def code_follows(text: str, found: list[tuple[int, str]], k: int) -> bool:
"""Whether code follows comment `k` of `found` on the line it ends on, a later comment aside."""
at = found[k][0] + len(found[k][1])
stop = len(text) if (eol := text.find("\n", at)) < 0 else eol
for offset, value in found[k + 1 :]:
if offset >= stop or text[at:offset].strip():
break
at = offset + len(value)
return bool(text[at:stop].strip())


def comment_lines(
text: str, lexer: str, spec: dict[str, Any], shift: int = 0
) -> list[tuple[Line, bool]]:
Expand All @@ -627,7 +642,8 @@ def comment_lines(
out: list[tuple[Line, bool]] = []
blocks = sorted(spec.get("block", ()), key=lambda b: -len(b[0]))
markers = sorted(spec.get("line", ()), key=len, reverse=True)
for offset, value in comment_tokens(text, lexer):
found = comment_tokens(text, lexer)
for k, (offset, value) in enumerate(found):
value = value.rstrip("\r\n")
number, col = starts.at(offset)
leading = not starts.line(number)[:col].strip()
Expand All @@ -643,15 +659,20 @@ def comment_lines(
out.append((Line(number + shift, body.strip(), leading, value, help_form), doc))
continue
opener, closer, *cont = block or ("", "")
for i, part in enumerate(value[len(opener) :].removesuffix(closer).split("\n")):
parts = value[len(opener) :].removesuffix(closer).split("\n")
trailed = code_follows(text, found, k)
for i, part in enumerate(parts):
body = part.strip()
if i and cont and body.startswith(cont[0]) and body[1:2].isspace():
body = body[1:].strip()
elif i and cont and body == cont[0]:
body = ""
row = starts.line(number + i)
raw = row[min(col, len(row) - len(row.lstrip())) :] if i else value.split("\n")[0]
out.append((Line(number + i + shift, body, leading or i > 0, raw, help_form), doc))
last = trailed and i == len(parts) - 1
out.append(
(Line(number + i + shift, body, leading or i > 0, raw, help_form, last), doc)
)
return out


Expand Down Expand Up @@ -1453,12 +1474,13 @@ def rule_comment_added(unit: Unit, ctx: Context) -> list[Finding]:
"""Every prose comment line opening its own line, which the diff scope narrows to a change.

A modified line counts as added, the label being the answer. A help block, a documentation
comment, and a trailing comment are out of scope.
comment, and a comment sharing its line with code, before or after it, are out of scope.
"""
lines = {
line.number
for line in unit.lines
if line.leading
and not line.trailed
and not line.help
and is_comment_prose(without_trailing_directive(ctx, line.text), ctx.config)
}
Expand Down
7 changes: 4 additions & 3 deletions .github/skills/comment-and-doc-style/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -189,9 +189,10 @@ gate. It reports a prose comment that opens its own line, in the diff's scope. A
modified line as an added one, so rewording one and re-indenting one each report it. That is the
rule's cost and the label is its answer, since a comment worth keeping takes the same label as a
comment worth writing. A trailing comment is out of scope. That is because which mid-line marker
opens a comment differs by language in ways a gate cannot settle from the marker alone. A docstring is not
a comment line, an instruction to a tool is not a comment the rule reads, and Markdown is out of
scope.
opens a comment differs by language in ways a gate cannot settle from the marker alone. A line
holding code after a block comment's closer is out of scope too, since deleting it deletes that
code. A docstring is not a comment line, an instruction to a tool is not a comment the rule reads,
and Markdown is out of scope.

Deleting the comment is the ordinary answer, since the bullets above already say what one has to
earn. Where a comment is genuinely owed, and a rule requiring one is the clearest case of that, the
Expand Down
2 changes: 1 addition & 1 deletion scripts/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ Whole-tree (`python3 scripts/prose_lint.py .`) reports zero, so a finding is a l

The default rule set covers comment shape (`comment-wrap` and `comment-case`) alongside the prose rules. That means a run nobody parameterized reports a wrapped comment, so the rule is enforced on every run. Comment shape is the most frequently regressed rule in agent-authored work. The tree carries no backlog of either, so both comment rules gate whole-tree, and `--diff` scopes a run rather than letting it survive a backlog.

The default set also carries `comment-added`, which reports the prose comment lines a change writes into code or config. Two weeks of fleet review rounds put 8% of every bot finding on agent-written comment text. Removing the surface was measured as the largest reduction available short of telling the reviewers not to read prose at all ([ptr727/ProjectTemplate#1631][review-cost-issue]). About half of them are classed as real defects rather than taste, so this removes a surface rather than a category of noise. The rule reports a prose comment that opens its own line, in the diff's scope, and infers nothing beyond that. A trailing comment is out of scope, because which mid-line marker opens a comment is a parser fact that differs by language. TOML and HCL carry a `#` inside a multi-line string, and a git pattern file holds no trailing comment at all. PowerShell opens one after almost anything, and shell after its metacharacters. Six attempts at deciding that from the marker's neighbourhood each got a language wrong. They did so by reporting a line holding no comment or silencing one that held several. A marker opening its line is unambiguous in all of them. `comment-wrap` and `comment-case` have judged only leading comments for years, which is why neither ever met those defects. `git diff --unified=0` counts a modified line as an added one, so re-indenting a commented block and rewording a comment each report it. That is the rule's cost rather than a defect in it. The rule does not read the file at the diff's base to tell an authored comment from a touched one. That would add rename, duplicate and prose-filter cases for precision the rule does not need. A wanted comment takes the same label either way. It is the one default rule a whole-tree run does not check, since with no diff every comment in the tree is in scope. The run states that omission on stderr rather than standing down quietly. A docstring is out of scope, as is Markdown, and a file whose Python does not parse is still lexed, its docstrings staying string text. Where the comment is wanted, `--allow-comments` stands the rule down and names the act on stderr. Likewise where it is wanted, `PROSE_ALLOW_COMMENTS`, set to anything but a false spelling, does the same for a caller with nowhere to pass a flag. `false` and `0` read as off, since the composite action's own input spells its off value `false`. That is also because a value mirroring that input turned the rule off at every commit. The composite action clears that variable and passes the flag from its own `allow-comments` input. What decides a pull request is therefore whatever the calling workflow puts there. The fleet's own caller passes it for either of two reasons, the `comments` label declared in [`repo-config/labels.json`][labels], and the pull request being a release-model promotion. That is this repository's own `develop` branch into its default branch, and its diff was reviewed where it landed. On the operational model a promotion runs the rule. That is because any of its lines may have landed by direct commit and the promotion is then their first review. The caller names its model in `validate-task.yml`'s `workflow-model` input, and one passing none is read as `release`. A repository calling the action from a workflow of its own passes whatever that workflow passes. The label reaches a repository through `repo-config/configure.sh apply` rather than through this change. Where the lint log shows "Read the pull request's labels live", a label added after a run fails applies when that job reruns. Otherwise, as in a private repository, it applies to the next push, since the event's snapshot misses it ([ptr727/ProjectTemplate#2354][private-labels-issue]).
The default set also carries `comment-added`, which reports the prose comment lines a change writes into code or config. Two weeks of fleet review rounds put 8% of every bot finding on agent-written comment text. Removing the surface was measured as the largest reduction available short of telling the reviewers not to read prose at all ([ptr727/ProjectTemplate#1631][review-cost-issue]). About half of them are classed as real defects rather than taste, so this removes a surface rather than a category of noise. The rule reports a prose comment that opens its own line, in the diff's scope, and infers nothing beyond that. A trailing comment is out of scope, because which mid-line marker opens a comment is a parser fact that differs by language. TOML and HCL carry a `#` inside a multi-line string, and a git pattern file holds no trailing comment at all. PowerShell opens one after almost anything, and shell after its metacharacters. Six attempts at deciding that from the marker's neighbourhood each got a language wrong. They did so by reporting a line holding no comment or silencing one that held several. A marker opening its line is unambiguous in all of them. `comment-wrap` and `comment-case` have judged only leading comments for years, which is why neither ever met those defects. A line holding code after a block comment's closer is out of scope too, since deleting it deletes that code. `git diff --unified=0` counts a modified line as an added one, so re-indenting a commented block and rewording a comment each report it. That is the rule's cost rather than a defect in it. The rule does not read the file at the diff's base to tell an authored comment from a touched one. That would add rename, duplicate and prose-filter cases for precision the rule does not need. A wanted comment takes the same label either way. It is the one default rule a whole-tree run does not check, since with no diff every comment in the tree is in scope. The run states that omission on stderr rather than standing down quietly. A docstring is out of scope, as is Markdown, and a file whose Python does not parse is still lexed, its docstrings staying string text. Where the comment is wanted, `--allow-comments` stands the rule down and names the act on stderr. Likewise where it is wanted, `PROSE_ALLOW_COMMENTS`, set to anything but a false spelling, does the same for a caller with nowhere to pass a flag. `false` and `0` read as off, since the composite action's own input spells its off value `false`. That is also because a value mirroring that input turned the rule off at every commit. The composite action clears that variable and passes the flag from its own `allow-comments` input. What decides a pull request is therefore whatever the calling workflow puts there. The fleet's own caller passes it for either of two reasons, the `comments` label declared in [`repo-config/labels.json`][labels], and the pull request being a release-model promotion. That is this repository's own `develop` branch into its default branch, and its diff was reviewed where it landed. On the operational model a promotion runs the rule. That is because any of its lines may have landed by direct commit and the promotion is then their first review. The caller names its model in `validate-task.yml`'s `workflow-model` input, and one passing none is read as `release`. A repository calling the action from a workflow of its own passes whatever that workflow passes. The label reaches a repository through `repo-config/configure.sh apply` rather than through this change. Where the lint log shows "Read the pull request's labels live", a label added after a run fails applies when that job reruns. Otherwise, as in a private repository, it applies to the next push, since the event's snapshot misses it ([ptr727/ProjectTemplate#2354][private-labels-issue]).

The default set also carries `issue-ref`. It reports an issue or pull request reference in a comment, in a Python docstring, and in the instruction text an agent reads as law. That text means the Skills trees, the fleet's own rule documents, and `.github/copilot-instructions.md`. In instruction text it reports the URL spelling as well, an inline link destination, a reference definition, and a bare URL alike. It reads them off the line as written rather than off the blanked copy the bare-form scan reads. It does so because that blanking exists to hide the very destinations this half looks for. A URL is reported nowhere else, because in a code or workflow comment the rule carves out one kind of URL. That kind names an issue or a pull request on a public repository other than this one. The bare spellings, by contrast, stay banned on every surface the rule reaches. A docstring and a documentation comment lie outside that carve-out's surface as well, so a URL is banned there too. On every surface but instruction text the gate reads the bare form alone and a banned URL goes unreported. That is the same unreported half a commit reference already sits in. One forge's URL paths are read, and on its bare host and that host's `www` alias rather than any subdomain of it. This is because the tracker is served from the bare host and a subdomain is a different service on the same domain. No subdomain was measured to serve a tracker-shaped path, so the bound is the one that was measured rather than a hazard that was found. A wildcard host is the wider claim. A URL naming a tracker the pattern does not know is banned by the rule text and goes unreported. That is the relationship the rule already holds with a commit SHA. One line naming one reference reports once however many spellings of it that line carries. The key is the number both patterns capture rather than the text that spelled it. That key costs one case, stated rather than left to be found. A line naming two different projects' references under one number reports once. The second reports on the run after the first is fixed, so it is a round rather than a lost detection. A documentation comment is read like any other comment here, unlike in `comment-added`, since a reference in one is the same defect. A Markdown comment is read only where the document itself is instruction text. How far a comment read reaches is a parser fact that differs by language. `rule_issue_ref` in `.github/actions/prose-gate/prose_lint.py` is the one statement of it, since every restatement of it here was wrong in its own way. The ban the rule serves is wider than the rule, covering a commit reference as well. A commit is not a shape a gate can read. That is because a short SHA carries the same shape as a blob id, a version fragment, and a fixture hash. So the rule text in the `comment-and-doc-style` Skill covers that half and this rule reads the pattern-detectable one. A string literal is not read, because a test builds the numbers it asserts against. That is also because reading those would report a fixture rather than a claim about the repository. A tracker, a history, a plan, and a README outside those trees are the repository's own narrative and keep their references, this file among them. The tree holds no reference the rule reports. A file whose rationale blocks all ended in an issue number would teach the house style the rule forbids. No rule text competes with the surrounding file until that file changes.

Expand Down
26 changes: 26 additions & 0 deletions tests/test_prose_lint.py
Original file line number Diff line number Diff line change
Expand Up @@ -2087,6 +2087,32 @@ def test_a_comment_opening_its_line_is_read_in_every_syntax(self) -> None:
with self.subTest(name=name):
self.assertEqual(["comment-added"], self.kinds(text, {"comment-added"}, name=name))

def test_a_line_closing_a_comment_before_code_is_not_reported(self) -> None:
"""Deleting it deletes the code after the closer and leaves the comment unterminated.

A later comment on that line is not code, and a comment on a line of its own is still reported.
"""
for name, code, opener, closer in (
("a.css", "a { color: red; }", "/*", "*/"),
("a.ts", "const y = 2;", "/*", "*/"),
("a.cs", "int y = 2;", "/*", "*/"),
("a.jsonc", '"b": 2', "/*", "*/"),
("a.html", "<p>y</p>", "<!--", "-->"),
("a.xml", "<b/>", "<!--", "-->"),
("a.csproj", "<b/>", "<!--", "-->"),
):
with self.subTest(name=name):
text = (
f"{code} {opener} A comment here.\nThe rest of it. {closer} {code}\n"
f"{opener} A real comment. {closer}\n"
f"{opener} A later one. {closer} {opener} And one more. {closer}\n"
f"{opener} A closer then code. {closer} {code}\n"
f"{opener} A. {closer} {opener} B. {closer} {code}\n"
)
path = self._write(text, name)
found = prose_lint.check_file(path, {"comment-added"})
self.assertEqual([3, 4], [n for n, _, _ in found])

def test_config_carries_the_rule_as_code_does(self) -> None:
for name, text in (
(
Expand Down
Loading