/review teach: turn a correction into a review-guidance issue by polewskm · Pull Request #79 · NCodeGroup/code-review

polewskm · 2026-10-31T08:33:55Z

Stacked on #75; GitHub retargets this to main when #75 merges.

Adds /review teach <rule>, /review teach F<n> <rule> (or a finding comment's link) and /review teach org <rule>: writer-only, top-level comments only, no forks, no model call. The review step words the issue (rule fenced as data with mentions defused, link to the comment, and for F<n> the finding's file, line, text and link); the publish step, after everything else, creates the review-guidance label if missing, opens the issue in this repository (or NCodeGroup/code-review for org) and replies with a short comment linking it. Everything posted is scanned with assertNoSecrets. npm run train now copies the rules of teach org issues closed as completed into a "Taught with /review teach" section of training/feedback.md (injectable fetch, hermetic tests).

Decisions

Issues write comes from a second app token step (permission-issues: write, scoped to the one target repository, continue-on-error: true), not the existing token: asking for a permission the installation lacks fails the mint, which would break every review until the app setting changes. When that mint fails, or GitHub answers 403/404, the reply says the app lacks the permission and quotes the rule to add by hand; 410 says issues are off.
A teach org issue lands in the public NCodeGroup/code-review, so from a private repository it names neither the repository nor its pull request. teach org takes no finding, for the same reason.
npm run train reads only bot-opened, scope=org issues in NCodeGroup/code-review closed as completed (not planned drops them); project rules belong in that repository's .github/code-review/guidance.md (Read per-repository settings from .github/code-review/ on the default branch #43). --no-taught-rules skips it.
An empty rule, a rule over 2,000 characters, or an unknown finding gets a reply instead of an issue.
The workflow's concurrency group gives each teach comment its own group, as for explain, so a teach never cancels a running review.

Left for the maintainer

In the NCode Review app's settings, add Repository permissions → Issues: Read and write, and accept the new permission on the NCodeGroup installation; for teach org, make sure the installation includes NCodeGroup/code-review. Until then, /review teach replies that the app lacks the permission.
Not run end to end against GitHub; npm run typecheck and npm test (207 tests) pass.

Closes #45

…ction Parse /review teach [F<n>|<link>] <rule> and /review teach org <rule>. The review step words the issue with no model call; the publish step opens it, labelled review-guidance, with a second app token that can write only issues, and replies with a link, or says the app lacks the permission. npm run train copies the rules of teach org issues closed as completed into training/feedback.md.

ncode-review

🤖 NCode Review at 3d44647: 11 new findings on the changed lines.

ncode-review · 2026-10-31T08:35:47Z

🤖 Review complete

Verdict: 💬 Comment — This PR adds /review teach. The review step turns a writer's rule, which can come from a finding, into the wording of a review-guidance issue without calling the model. The publish step then opens that issue in this repository, or in NCodeGroup/code-review for an org-wide rule, using an issues token, and replies on the pull request. The design is sound: rule text and finding text are quoted as data, @mentions are neutralized, and a private repository's details are kept out of org issues. The real risk is how the privacy and failure behavior are tested: the private-finding suppression is never exercised through planTeaching, and postTeaching has no test for issue-create errors other than permission errors. The remaining items are small test gaps and style nits.

8 reviewers run — 8 findings (run details)

Reviewer	Model	Findings	Top
synthesis	opus-5-5	8	🟠
architecture	sonnet-5-5	0	🟢
completeness	sonnet-5-5	0	🟢
conventions	sonnet-5-5	1	🟡
correctness	sonnet-5-5	0	🟢
hygiene	haiku-5-5	0	🟢
rule-check	sonnet-5-5	3	🟡
security	sonnet-5-5	0	🟢
tests	haiku-5-5	5	🟠

Skipped: api-compat, data-model, infrastructure, intent, performance

Findings

🟠 F1 src/teach.ts:160: No end-to-end test that planTeaching hides a private repository's finding
🟠 F2 src/teach.ts:193: No test for what postTeaching does when issue creation fails with a non-permission error
🟡 F3 src/main.ts:104: Teach routing and the isPrivate default in main.ts are untested
🟡 F4 src/command.ts:60: No test for a first word that is an invalid link, which falls back to rule text
🟡 F5 src/teach.ts:128: The unknown-link branch of teachUnknownRefBody is untested
🟡 F6 src/teach.ts:170: Literal HTTP status codes and label colour
🟡 F7 src/teach.ts:205: Several functions take four or more data parameters
🟡 F8 src/main.ts:83: writeForPublish now takes five parameters

_{Trial: findings are posted as review threads and in the check run. $0.69 on Amazon Bedrock, 94s, at c44bb79. (how to interact)}

…rker and issue source type ensureLabel takes a 422 from the label create as success, so two teach commands racing on a repository without the label both open their issue. ruleFromIssue compares against guidanceMarker('org'), IssueSource derives from TeachContext, and the guidance-issue page size is one constant. Tests cover the race, a private repository's finding in an org issue, a link ref through planTeaching, and an issue body with no comment URL or finding line.

polewskm · 2026-10-31T15:57:15Z

/review

ncode-review

🤖 NCode Review at 8aa5a0b: 9 new findings on the changed lines.
1 more was already posted on this pull request.

# Conflicts: # src/main.ts

polewskm · 2026-10-31T16:14:32Z

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. (The branch also has issue-44, this PR's base, merged in: 7a2ed53. F8 of #79 (comment) is the writeForPublish finding answered in round 1.)

ncode-review

🤖 NCode Review at c44bb79: 5 new findings on the changed lines.
3 more were already posted on this pull request.

ncode-review · 2026-10-31T16:16:53Z

+      rule: command.rule,
+      title: issueTitle(command.rule),
+      body: issueBody(command.scope, command.rule, source, finding),
+    },


🟠 Should fix: No end-to-end test that planTeaching hides a private repository's finding _{F1 · tests}

The org-scope privacy rule is only tested by calling issueBody directly. No test runs planTeaching with a finding ref and isPrivate: true, which is the path main.ts actually takes.

Consequence: A regression that drops isPrivate or passes the finding differently would leak a private repository's finding text, path and name into a public NCodeGroup/code-review issue without any test failing.

Fix: Add a planTeaching test for teach org F<n> <rule> with isPrivate: true and a reader that returns the finding. Assert that the body has no ### From finding, no finding path or text, and no private repository name.

Evidence

In test/teach.test.ts, the 'planTeaching for teach org ... hidden' test uses no ref, and 'issueBody keeps a private repository's finding out' calls issueBody directly.

Confidence 0.55.

Autopilot: fixed in #85 (f296477): a new end-to-end planTeaching test reads the finding through the reader for an org rule from a private repository. It asserts that the body has no ### From finding section, none of the finding's path, title, text or link, and no repository name. As the parser stands, teach org F2 ... keeps F2 in the rule and names no finding, so the test builds the command directly. It also checks the parsed form, which reads nothing.

ncode-review · 2026-10-31T16:16:53Z

+    if (error instanceof GitHubError && error.status === 410) return { refused: 'disabled' };
+    throw error;
+  }
+}


🟠 Should fix: No test for what postTeaching does when issue creation fails with a non-permission error _{F2 · tests}

openGuidanceIssue rethrows any refusal other than 403/404/410, for example a 422 or a 500. No test calls postTeaching with such a failure, so the behavior the user sees (no reply, the step fails) is not pinned down.

Consequence: A later change could swallow the error and post a misleading 'can't write issues' reply or a success reply, and no test would catch it.

Fix: Add a postTeaching test where the issue POST returns a 500 (or a 422). Assert that it rejects and that no reply comment was posted.

Evidence

The only postTeaching failure tests cover the 403 reply and the secret scan. The rethrow in openGuidanceIssue is only tested by calling it directly.

Confidence 0.50.

Autopilot: fixed in #85 (f296477): a postTeaching test makes the issue create return a 422 and then a 500, and asserts that it rejects and posts no reply.

ncode-review · 2026-10-31T16:16:53Z

+  if (ref && ref.kind !== 'invalid') return { kind: 'teach', scope: 'repository', ref, rule: (first![2] ?? '').trim() };
+  return { kind: 'teach', scope: 'repository', rule: rest };
+}
+


🟡 Nit: No test for a first word that is an invalid link, which falls back to rule text _{F4 · tests}

When the first word is an invalid finding link, such as an http URL or a URL that isn't a comment link, parseTeach keeps the whole text as the rule. No test covers this fallback.

Consequence: A change in this behavior, for example a link silently becoming part of the rule or becoming an error, would go unnoticed.

Fix: Add a parseCommand('/review teach https://example.com/x a rule') assertion that the whole text is kept as the rule.

Evidence

The parseCommand tests cover F-ids, valid links and 'organizations', but not an invalid link as the first word.

Confidence 0.30.

Autopilot: fixed in #85 (f296477): parseCommand tests now cover an https://example.com link, an http: finding link and a pull request link with no comment anchor. Each keeps the whole text as the rule.

ncode-review · 2026-10-31T16:16:53Z

+  return `That rule is ${length.toLocaleString('en-US')} characters long; teach one of at most ${maxRuleChars.toLocaleString('en-US')} characters.`;
+}
+
+export function teachUnknownRefBody(ref: FindingRef, validIds: string[]): string {


🟡 Nit: The unknown-link branch of teachUnknownRefBody is untested _{F5 · tests}

The reply for an unknown link ("That link isn't to one of NCode Review's finding comments") is never tested; the only unknown-ref test uses an id (F9).

Consequence: A wrong message for a bad link would go unnoticed.

Fix: Add a planTeaching case with a link to a missing comment and assert the link wording.

Evidence

The only unknown-ref test uses parseCommand('/review teach F9 a rule').

Confidence 0.30.

Autopilot: fixed in #85 (f296477): a planTeaching test teaches from a link to a comment that isn't a finding, and checks the link wording and the newest review's ids.

ncode-review · 2026-10-31T16:16:53Z

+      ? `issues are turned off in ${teaching.target}`
+      : `the NCode Review app can't write issues in ${teaching.target}. A maintainer needs to give the app the Issues: Read and write permission${teaching.scope === 'org' ? ` and install it on ${orgGuidanceRepository}` : ''}`;
+  return `I couldn't open a \`${guidanceLabel}\` issue: ${why}. The rule, to add by hand meanwhile:\n\n${fenced(teaching.rule)}`;
+}


🟡 Nit: Several functions take four or more data parameters _{F7 · conventions, rule-check}

postTeaching, publishTeaching and issueBody take four data parameters, and writeForPublish in src/main.ts (line 83) now takes five. The same api/issuesApi/secrets group appears in several of these signatures. Does the C#-oriented 2–3 parameter rule apply to this TypeScript code?

Consequence: The signatures are harder to read, and it is easy to swap the two API callbacks without the compiler noticing.

Fix: If the rule applies, group api, issuesApi and secrets into a PublishContext, pass issueBody a single input object, and use an options object for writeForPublish (or split file writing from output writing).

Evidence

The rule says 'at most 2–3 data parameters ... Turn recurring parameter groups into a type' (csharp-production.instructions.md).

Confidence 0.30.

Autopilot: declined: the 2-3 parameter rule is C# guidance. In this TypeScript the parameters are typed, and the tests record calls on api and issuesApi separately, so swapping them would fail a test. Grouping them into a PublishContext is a refactor with no behavior change, so it stays out of this fix.

Answer late review findings on #62, #78 and #79

polewskm deployed to code-review October 31, 2026 08:34 — with GitHub Actions Active

ncode-review Bot reviewed Oct 31, 2026

View reviewed changes

polewskm added 4 commits October 31, 2026 10:10

Merge remote-tracking branch 'origin/issue-44' into issue-45

7a2ed53

# Conflicts: # src/main.ts

Test that a rule exactly at the length limit is taught

701dd4f

Warn when npm run train reads the guidance issues without a GitHub token

ffdc5e9

Test that a label read failing with anything but 404 creates no label

c44bb79

polewskm marked this pull request as ready for review October 31, 2026 16:14

polewskm deployed to code-review October 31, 2026 16:14 — with GitHub Actions Active

polewskm merged commit de22624 into issue-44 Oct 31, 2026
3 checks passed

polewskm deleted the issue-45 branch October 31, 2026 16:15

ncode-review Bot reviewed Oct 31, 2026

View reviewed changes

This was referenced Oct 31, 2026

/review explain <finding>: answer whether a finding still holds #75

Merged

Answer late review findings on #62, #78 and #79 #85

Merged

polewskm added a commit that referenced this pull request Oct 31, 2026

Merge pull request #85 from NCodeGroup/late-feedback

0398d5a

Answer late review findings on #62, #78 and #79

polewskm mentioned this pull request Oct 31, 2026

Roll out the 2026-10-31 changes: release, consumer pins, app permission, CodeQL #97

Closed

Conversation

polewskm commented Oct 31, 2026

Uh oh!

ncode-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ncode-review Bot commented Oct 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 Review complete

Uh oh!

polewskm commented Oct 31, 2026

Uh oh!

ncode-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

polewskm commented Oct 31, 2026

Uh oh!

Uh oh!

ncode-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ncode-review Bot Oct 31, 2026

Choose a reason for hiding this comment

Uh oh!

polewskm Oct 31, 2026

Choose a reason for hiding this comment

Uh oh!

ncode-review Bot Oct 31, 2026

Choose a reason for hiding this comment

Uh oh!

polewskm Oct 31, 2026

Choose a reason for hiding this comment

Uh oh!

ncode-review Bot Oct 31, 2026

Choose a reason for hiding this comment

Uh oh!

polewskm Oct 31, 2026

Choose a reason for hiding this comment

Uh oh!

ncode-review Bot Oct 31, 2026

Choose a reason for hiding this comment

Uh oh!

polewskm Oct 31, 2026

Choose a reason for hiding this comment

Uh oh!

ncode-review Bot Oct 31, 2026

Choose a reason for hiding this comment

Uh oh!

polewskm Oct 31, 2026

Choose a reason for hiding this comment

Uh oh!

This branch was successfully deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

ncode-review Bot commented Oct 31, 2026 •

edited

Loading