Repository navigation
Conversation
… branch gather() reads config.yml (sensitive, skip, policies, tiers), guidance.md and reviewers/*.md from the base checkout only, never the pull request's head. config.yml is a small YAML subset checked by a hand-written schema; anything unusable is a warning in the check run and the defaults, not a failed run. Policies join the conventions within their own budget and must resolve inside the checkout; skipped files are listed in the check run; project reviewers join the roster as project:<name> and their paths: frontmatter adds them to the floor.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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 a589e4c: 16 new findings on the changed lines.
🤖 Review completeVerdict: 💬 Comment — This PR reads per-repository review settings (config.yml, guidance.md, reviewers/*.md) from the default branch's checkout under .github/code-review/. They feed skip, sensitive paths, policies, model tiers and project reviewers into gather and review. Reading only from the base checkout, confining reads to that checkout after resolving symlinks, and degrading to a warning when a setting fails are all sound choices, and the tests cover them well. The main design cost is a hand-written YAML subset parser that sits beside the existing lenient frontmatter reader. Everything else raised is cleanup: duplicated checks, unnamed numbers and test structure. 7 reviewers run — 7 findings (run details)
Skipped: api-compat, data-model, infrastructure, intent, performance, tests Findings
Trial: findings are posted as review threads and in the check run. $0.81 on Amazon Bedrock, 88s, at 45c6c29. (how to interact) |
Settings reads go through one open file and turn any fs error into a warning, so an unreadable file can't fail the review or change between its checks and its read. A project reviewer's frontmatter is validated (paths: only, a list of globs or one comma-separated string), shares the frontmatter split and glob list with the instruction files, and its warning loses a stray colon. parseYaml refuses a __proto__ key. A tiers: name matching no reviewer is a check-run warning, and a pull request whose every changed file is skipped by config runs no model. Drop the ?? fallbacks on fields gather always sets, cite issue #42 as the convention asks, and test the size limits, truncation, unreadable files, a reviewers path that isn't a folder, projectRoster and fileList.
|
/review |
There was a problem hiding this comment.
🤖 NCode Review at dc5a450: 9 new findings on the changed lines.
5 more were already posted on this pull request.
Not on a changed line:
- 🟡 F4 Nit: Is haiku the intended default tier for project reviewers?
src/review.ts:392. Project reviewers aren't in defaultReviewerTiers, sotiers[reviewer] ?? 'haiku'runs everyproject:<name>on the cheapest model. Every engine reviewer defaults to sonnet, and neither the README nor the CHANGELOG says this. Fix: Default project reviewers to sonnet, or document the haiku default in the README's tiers: bullet.
…ment the project reviewers' tier
|
Autopilot: answering F4 in #69 (review) (also listed in #69 (comment)): fixed in 45c6c29. Haiku is the intended default. It is the engine's default for any reviewer not in |
|
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 45c6c29: 4 new findings on the changed lines.
2 more were already posted on this pull request.
Not on a changed line:
- 🟡 F4 Nit: Reviewed-file filter repeated where gather already computes it
src/review.ts:186. projectFloor recomputes the non-generated paths withfiles.filter((f) => !f.generated).map(...), the same list gather.ts already computes asreviewed. Fix: Expose the reviewed paths on Gathered, or share one helper for the filter.
# Conflicts: # CHANGELOG.md # src/main.ts # src/review.ts
# Conflicts: # CHANGELOG.md # src/review.ts
|
Autopilot: answering F4 in #69 (review) (also listed in #69 (comment)): declined: nit; the review-round limit (3) is reached. |
|
Autopilot: Round 3 answered. The review-round limit (3) is reached; no blocker or should-fix is left open, and the remaining threads are nits or repeats, declined above with pointers to the earlier answers. The branch now has |
…-only pull requests A change to a file config.yml's sensitive: matches runs correctness and security on Opus for the parts of the diff with it, and verification on Opus for its findings. A documentation-only pull request, or a template update of template-owned files, runs hygiene alone on Haiku with no context, selection or synthesis. CODE_REVIEW_TIERS still overrides any reviewer, and the check run's reviewer table shows each reviewer's models. Closes #42
A file with a code or script extension, or a git hook, never counts as documentation or as a template-owned file a template update can change, so a pull request touching one gets the standard review even in docs/ or alongside .template-version. Also tests a deep review whose floor already chose security, and fixes a stray colon in the partTiers comment.
Deep reviews for sensitive paths and a light review for documentation-only pull requests
# Conflicts: # CHANGELOG.md # src/main.ts # src/results.ts # src/review.ts # src/verify.ts # test/verify.test.ts
The depth tests' fake agent returns records, and the fallback test reviews a non-documentation file so its review isn't a light one. A reviewer's models leave out a call refused for credits and name a model once across providers.
|
Autopilot: merged
|
# Conflicts: # CHANGELOG.md
# Conflicts: # README.md # src/context.ts # src/gather.ts # src/main.ts # src/results.ts # src/review.ts # test/verify.test.ts
… claims guard added, and test the light review's guard and a re-review's tiers
|
Autopilot: merged
|
# Conflicts: # CHANGELOG.md # src/results.ts # src/review.ts
…les, so the acknowledgement and finish scripts load no npm package This branch's results.ts imported depthLabel from review.ts and creditsRanOutReason from provider.ts as values, which since main's acknowledgement (#74) made acknowledge.ts load the agent SDK through comments.ts and results.ts.
# Conflicts: # CHANGELOG.md # src/config.ts # src/gather.ts # src/main.ts # src/results.ts # src/review.ts # test/rereview.test.ts
|
Autopilot: merged
|
# Conflicts: # src/main.ts # src/review.ts
gather()now reads.github/code-review/from the base checkout only. It readsconfig.yml(sensitive:,skip:,policies:,tiers:),guidance.mdandreviewers/*.md, so a pull request that edits them is reviewed with the default branch's copy.Policies are added to the conventions with their path, within a 40,000-character budget of their own. Changed files that
skip:leaves out are listed in the check run as "skipped by config". Project reviewers join the roster asproject:<name>and never replace an engine reviewer; apaths:frontmatter adds one to the floor. Settings the engine can't use become check-run warnings, and the review runs with the defaults instead of failing.sensitive:is stored on the gathered context for #42; it doesn't change the review yet. The trace gets asettingsblock. README has a new "Repository settings" section with an Identity example. Tests are intest/settings.test.ts.Decisions
*must be quoted, as real YAML also requires.config.ymlfor the defaults, with one warning. Unsafe or missing policy paths are dropped one entry at a time, with a warning for each..., backslash or control characters. They must resolve inside the real path of the base checkout and be regular files. The same check applies toconfig.yml,guidance.mdand the reviewers folder, so a symlinked.github/code-reviewisn't followed.tiers:: the engine defaults, then the repository'stiers:, thenCODE_REVIEW_TIERS.Closes #43
This branch now also carries #78 (deep reviews for sensitive paths and a light path for documentation-only pull requests), which was merged into it.
Closes #42