Follow up the engine fetch's review: a fetch time limit, a testable skip decision, sturdier tests by polewskm · Pull Request #22 · NCodeGroup/code-review

polewskm · 2026-10-31T00:50:36Z

Follows #19, whose last bot review (at 1516681) landed after it merged. Seven findings, all addressed:

F1, a fetch with no time limit. Each fetch now ends after three minutes (fetchTimeoutMs, overridable in the options) with the fetch's own error and the hint to comment /review. A cleanup that fails because a killed git still holds the folder (it happened on Windows) retries and never hides that error.
F2, the by-name fetch never ran. A new test points the pull request's ref away from the head, so no ref reaches it, and checks that the named commit is still checked out. The test's remote allows fetching a commit by name (uploadpack.allowAnySHA1InWant). Whether github.com serves a commit that no ref reaches is not tested here.
F3, a fixed sleep. The header test polls for the server's output up to ten seconds.
F4, untested skip decisions. The decision to skip an automatic review (a fork, a head already reviewed) is skipReason in command.ts, with tests for a fork, a deleted fork, the same head, another head and a missing head.
F5, a real server in a unit test. Kept, with a comment saying it is the one check that the token reaches the wire.
F6, F7. The fork tests moved beside the other command tests; the imports are in order; the README paragraph is re-wrapped and mentions the time limit.

New: a test that a remote that never answers ends the fetch, and the cleanup after it.

43 tests and the typecheck pass.

🤖 Generated with Claude Code

…kip decision, and sturdier tests A fetch that stalls now ends after three minutes with the fetch's own error, and a cleanup that fails because a killed git still holds the folder never hides it. The decision to skip an automatic review (a fork, a head already reviewed) is one tested function. New tests cover a commit no ref reaches any more and a remote that never answers; the header test polls for the server's output; the fork tests moved beside the other command tests; the README is re-wrapped.

ncode-review

🤖 NCode Review at ab5859f: 6 new findings on the changed lines.

Not on a changed line:

🟠 F4 Should fix: Are skipReason and isFromFork still tested? The isFromFork tests were deleted test/fetch-head.test.ts:7. The PR removes the isFromFork tests and imports skipReason from ./command.ts, but the diff contains no changes to command.ts and no skipReason tests. Fix: Confirm that skipReason exists in command.ts and that its tests (fork, missing head repo, head already reviewed) live elsewhere. If not, add them and restore the isFromFork cases.

ncode-review · 2026-10-31T00:52:15Z

🤖 Review complete

Verdict: 💬 Comment — This PR does three things. It puts a time limit on the engine's git fetches, with cleanup that retries and never hides the fetch's own error. It moves the fork and already-reviewed skip rules into skipReason/skipReasonFor in command.ts. And it hardens the fetch tests, adding a fetch-by-SHA case, a timeout case and timeout validation. The change is small, the design is sound, and the risk is low; the findings are about the shape of the API and how hermetic the tests are, not about correctness. The new skipReason/skipReasonFor functions have no tests in this diff, and the isFromFork test was removed, so check that the promised "testable skip decision" is covered somewhere.

7 reviewers run — 6 findings (run details)

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

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

Findings

🟠 F1 test/fetch-head.test.ts:190: New timeout test depends on a live loopback server and the wall clock
🟡 F2 src/command.ts:46: skipReasonFor is a trigger-flag wrapper, and the /review path builds a lookup it never calls
🟡 F3 src/command.ts:48: skipReasonFor takes four data parameters, and pullRequest/repository recur
🟡 F4 src/fetch-head.ts:111: Node's maximum timeout is the bare literal 2 ** 31 - 1
🟡 F5 test/fetch-head.test.ts:205: The check that a timed-out fetch's folder is removed never runs on Windows
🟡 F6 src/main.ts:88: Reviewed-head lookup closure passed on the /review path where it can never run

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

…decision in one place Second review of the follow-up (F1 to F7): the timeout test checks the error came from git's time limit and that it ended between the limit and a few seconds after; a time limit that isn't positive is refused before anything is touched; the cleanup's retries are named; the /review path and the automatic path both decide through skipReason, once each. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

polewskm · 2026-10-31T00:56:23Z

F4 (not on a changed line) is a false positive: this pull request does change src/command.ts (+14, skipReason) and test/engine.test.ts (+20), where the isFromFork tests moved and the skipReason tests are: a fork, a deleted fork, the same head, another head and a missing head. The finding says the diff has no changes to command.ts, so the reviewer that raised it didn't see those two files; worth a look on the engine side.

polewskm · 2026-10-31T00:58:21Z

/review

ncode-review

🤖 NCode Review at 271d154: 4 new findings on the changed lines.
2 more were already posted on this pull request.

Third review of the follow-up (F2, F3, F4, F6): git() takes its environment and time limit in one options object; the command tests (commandFromEvent, isFromFork, skipReason) move to test/command.test.ts with a static import, and skipReason has one test per rule; the timeout test no longer checks the folder's cleanup, which the 'leaves nothing behind' test covers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

polewskm · 2026-10-31T01:02:08Z

/review

ncode-review

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

…leanup Fourth review of the follow-up (F2, F5): skipReason takes a lookup and calls it only for a pull request that isn't from a fork and has a head, so a fork costs no API call and the head rule lives in one place; a cleanup that fails after a failed fetch is logged to stderr, while the fetch's error stays the one thrown. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

polewskm · 2026-10-31T01:06:07Z

/review

ncode-review

🤖 NCode Review at 5b64ebc: 1 new finding on the changed lines.
4 more were already posted on this pull request.

…lues past int32 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

polewskm · 2026-10-31T01:10:39Z

/review

ncode-review

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

… looks up the reviewed head Fifth review of the follow-up (F1 to F4, F6): the cleanup is an injectable removeFolder, so a test makes it fail and checks the fetch's error is kept and the cleanup's is logged; the timeout test waits for the killed fetch's folder to go (on Linux and macOS) and has no upper time bound; skipReasonFor decides per trigger whether the reviewed head is looked up, with a test for each; the skipReason tests are one behavior each. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

polewskm · 2026-10-31T01:15:05Z

/review

ncode-review

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

Not on a changed line:

🟡 F4 Nit: No test covers the time limit on the fetch by name src/fetch-head.ts:121. The never-answering server makes the first fetch hang, so the second fetch (by SHA) never runs under a time limit in any test. Fix: Add a case where the first fetch succeeds and the fetch by name hangs, then assert the ETIMEDOUT error.
🟡 F7 Nit: Stray blank line at the end of the file test/engine.test.ts:233. The diff adds a blank line after the final });. Fix: Delete the trailing blank line.

… line Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

polewskm · 2026-10-31T01:18:02Z

The two findings not on a changed line, from the review at cb5177b:

F4 (no test of the time limit on the fetch by name): not changed. It needs a remote that serves the first fetch and then hangs on the second, which is a small git server of its own. Both fetches go through the same git(where, args, { env, timeout }) call with the same limit, and the timeout test covers that call.
F7 (a trailing blank line in test/engine.test.ts): fixed in 3362a68.

polewskm · 2026-10-31T01:19:14Z

/review

ncode-review

🤖 NCode Review at 3362a68: 1 new finding on the changed lines.
5 more were already posted on this pull request.

ncode-review · 2026-10-31T01:22:11Z

-    const pullRequest = (await response.json()) as { head?: { repo?: { full_name?: string } } };
-    if (isFromFork(pullRequest, repository)) {
-      console.log(`#${request.number} comes from a fork, and forks aren't reviewed.`);
+    const pullRequest = (await response.json()) as { number?: number; head?: { sha?: string; repo?: { full_name?: string } | null } };


🟡 Nit: Reviewed-head lookup closure passed on the /review path where it can never run _{F6 · hygiene}

On the comment path, the arrow () => lastReviewedHead(...) is passed to skipReasonFor('comment', ...), which drops it (skipReason gets undefined for a comment trigger). The closure is dead code that makes the call look like it might fire for /review.

Consequence: No runtime effect; a reader has to trace skipReasonFor to learn that /review never looks up the reviewed head, and the dead argument invites someone to pass a real lookup later.

Fix: Pass the lookup only in the automatic branch, or document at the call site that the comment trigger ignores it.

Evidence

main.ts: const reason = await skipReasonFor('comment', pullRequest, repository, () => lastReviewedHead(repository, request.number, githubToken)); and command.ts skipReasonFor: return skipReason(pullRequest, repository, trigger === 'automatic' ? reviewedHead : undefined);

Confidence 0.30.

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

ncode-review Bot reviewed Oct 31, 2026

View reviewed changes

Comment thread test/fetch-head.test.ts Resolved

Comment thread src/main.ts Resolved

Comment thread src/fetch-head.ts Resolved

Comment thread src/fetch-head.ts Resolved

Comment thread test/fetch-head.test.ts Resolved

Comment thread src/fetch-head.ts Resolved

ncode-review Bot reviewed Oct 31, 2026

View reviewed changes

Comment thread src/fetch-head.ts Resolved

Comment thread test/engine.test.ts Resolved

Comment thread test/engine.test.ts Resolved

Comment thread test/fetch-head.test.ts Resolved

ncode-review Bot reviewed Oct 31, 2026

View reviewed changes

Comment thread src/fetch-head.ts Resolved

Comment thread src/command.ts Resolved

ncode-review Bot reviewed Oct 31, 2026

View reviewed changes

Comment thread src/fetch-head.ts Resolved

Refuse a fetch time limit Node can't take: Infinity, fractions and va…

bf333dd

…lues past int32 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

ncode-review Bot reviewed Oct 31, 2026

View reviewed changes

Comment thread test/fetch-head.test.ts Resolved

Comment thread test/fetch-head.test.ts Resolved

Comment thread src/main.ts Resolved

Comment thread test/command.test.ts Resolved

Comment thread src/fetch-head.ts Resolved

ncode-review Bot reviewed Oct 31, 2026

View reviewed changes

Comment thread src/fetch-head.ts Resolved

Comment thread src/command.ts Resolved

Comment thread src/fetch-head.ts Resolved

Pin the default fetch time limit in a test, and drop a trailing blank…

3362a68

… line Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

polewskm mentioned this pull request Oct 31, 2026

Re-reviews don't converge: each round raises new nits on unchanged code and can reverse the round before #24

Closed

polewskm marked this pull request as ready for review October 31, 2026 01:20

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

polewskm merged commit 3c370e7 into main Oct 31, 2026
5 checks passed

polewskm deleted the review-follow-ups branch October 31, 2026 01:20

ncode-review Bot reviewed Oct 31, 2026

View reviewed changes

polewskm mentioned this pull request Oct 31, 2026

Title the changelog's section for v0.3.0 #26

Merged

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!

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!

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!

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!

polewskm commented Oct 31, 2026

Uh oh!

ncode-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

polewskm commented Oct 31, 2026

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!

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