Repository navigation
Answer late review findings on #62, #78 and #79 - #85
Conversation
A fence closes only on its own marker, at least as long; an unclosed fence is named in the error. Answers late review findings F1 and F2 on #62.
Any file that isn't Markdown, text or reStructuredText, or an image or PDF in docs/, gets the standard review, template updates included. A light review whose hygiene call failed on a part of the diff says so and doesn't approve. Answers late review findings F1-F5 on #78.
…backs Answers late review findings F1, F2, F4 and F5 on #79.
![ncode-review[bot]](https://avatars.githubusercontent.com/u/12114478?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 NCode Review at f296477: 3 new findings on the changed lines.
Not on a changed line:
- 🟡 F2 Nit: No test for a closing-fence candidate with trailing text
tools/release-notes.ts:31. A fence closes only when nothing follows the marker run (!rest.trim()), and no test checks that a line like```markdowninside an openfence leaves it open. **Fix:** In the 'closes a fence only with its own marker' test, add ````markdown ```` inside a backtick fence, follow it with a heading, and assert that the heading stays in the section.
🤖 Review completeVerdict: 💬 Comment — This PR replaces the loose documentation regex in src/gather.ts with an allowlist: Markdown, reStructuredText and text files anywhere, plus images and PDFs under doc/ or docs/. It also leaves out public API listings and build or dependency 7 reviewers run — 1 finding (run details)
Skipped: api-compat, data-model, infrastructure, intent, performance, security Findings
Trial: findings are posted as review threads and in the check run. $0.46 on Amazon Bedrock, 76s, at d08efe8. (how to interact) |
…fence edge cases The check run's light-review line now says documentation or a template version file. Tests pin that a backtick line whose info string holds a backtick is inline code, and that a marker run followed by text doesn't close a fence.
|
Autopilot: answering #85 (review)
|
|
Autopilot: Round 1 answered. This PR continues threads that already went three rounds on #62, #78 and #79, so it took only small, clearly correct changes: F1, F2 and F4 are fixed in 6bff291, and F3 is declined (SVGs in |
|
/review |
There was a problem hiding this comment.
🤖 NCode Review at 6bff291: 3 new findings on the changed lines.
…rder gather.ts's isDocumentation is now the allowlist the light review uses, and reviewOrder ranks by it, so a script or config file in docs/, a build or dependency list and a public API listing no longer go last in an oversized diff.
|
Autopilot: Round 2 answered. No blocker or should-fix is left open; the remaining threads are nits, answered above. Recommend merging once you've resolved them. |
There was a problem hiding this comment.
🤖 NCode Review at d08efe8: 1 new finding on the changed lines.
|
|
||
| export function isDocumentation(file: string): boolean { | ||
| return docPattern.test(file); | ||
| if (publicApiPath.test(file) || listingFile.test(file)) return false; |
There was a problem hiding this comment.
🟡 Nit: The documentation allowlist depends on a growing list of .txt names to exclude F1 · architecture
Every .txt file counts as documentation unless its name matches publicApiPath or listingFile, so other machine-read .txt files still get classified as docs, such as license or dependency lists, test fixtures and data inputs.
Consequence: Each new kind of non-prose .txt needs another entry here. Until someone adds it, a pull request that changes such a file gets only the light hygiene review and is ordered last when the diff is over budget. How much this matters depends on how common these files are in the target repositories.
Fix: Count .txt as documentation only by location or name, such as the repository root or doc/ and docs/. Or keep the exclusions in one shared list that the README and the tests also read.
Evidence
src/gather.ts: documentationFile = /\.(md|rst|txt)$/i, then exceptions for listingFile (CMakeLists.txt, requirements/constraints) and publicApiPath.
Confidence 0.30.
There was a problem hiding this comment.
Autopilot: declined for now: a nit on code that has already merged, after three review rounds on this chain. The point stands: .txt counts as documentation everywhere except the named exclusions. Narrowing .txt to doc/, docs/ and the repository root (or one shared exclusion list that the README and tests read) is now an item in the follow-up checklist #84.
Answers the review threads left open on #62, #78 and #79 after they merged. The light review now runs only when every changed file is on an allowlist of documentation types. Release notes read code fences as CommonMark does. Each fix comes with tests.
Fixed findings:
1.,1)) count as changes. Release: require the version's changelog section, and explain the missing Dependabot bumps #62 (comment).md/.txt/.rstfiles, plus images and PDFs indoc(s)/, and a template update's version files. Every other file gets the standard review, including one with no extension, a file indocs/, a non-Markdown template-owned file,CMakeLists.txtand pip requirements files. Deep reviews for sensitive paths and a light review for documentation-only pull requests #78 (comment)planTeachingtest showing that an org rule from a private repository leaves the finding out. /review teach: turn a correction into a review-guidance issue #79 (comment)postTeachingtest showing that a 422 or 500 from the issue create fails the step and posts no reply. /review teach: turn a correction into a review-guidance issue #79 (comment)Decisions
.txtstays documentation exceptPublicAPI.*.txt,CMakeLists.txtandrequirements*.txt/constraints*.txt. Images and PDFs count only inside adoc/ordocs/folder.Refs #62
Refs #78
Refs #79