fix: address review suggestions from #1 (single-branch clone, HooksManager pinning) - #2
Conversation
- scripts/upstream: clone upstream --single-branch (ephemeral CI clone, full ref namespace doubles clone time), with a fetch fallback so a --ref outside the default branch still resolves. - verify.sh / contract: the HooksManager check grepped for the exact TS spelling while its comment claimed the name was loose — a future Go-idiomatic rename would falsely fail the gate. The 0.8.0 port chose the upstream spelling verbatim, so pin it explicitly: the contract now names HooksManager as the required Go type and the verify.sh comment says a rename is a contract change, not a drift to tolerate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review nit on #2: separate the ref from options in the fallback fetch (-- terminator) and rev-parse (--end-of-options) so a manually supplied --ref value starting with '-' cannot be parsed as an option. CI refs always start with '@'; this hardens the manual path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@devin please review this PR — head is |
There was a problem hiding this comment.
Note: Perry's verdict is LGTM, but this review is posted as a comment because the maintainer GitHub App (used for APPROVE verdicts) is not installed on the OpenRouterTeam org. The review content below is an approval.
Perry's Review
Verdict: ✅ LGTM
Risk: 🟢 Low
Review details
Re-review of ade75b0 (previous review was on 69e3b8d). The new commit directly addresses the single nit from the previous review, and the original changes (single-branch clone, HooksManager pinning) remain correct. CI is green (check + verify-port, both pass). The PR is already merged.
scripts/upstream — option injection guards (the new delta)
The new commit adds --end-of-options to the rev-parse verify call and -- to the fetch origin call, exactly as suggested in the previous review. Verified empirically:
--end-of-optionsis supported since git 2.24; the CI environment runs git 2.43. A dash-prefixed ref correctly fails with exit 1 (triggering the fallback) instead of being parsed as an option.--on the fetch command is the standard separator to prevent refspec-as-option injection.- The second rev-parse (FETCH_HEAD) correctly omits the guard, since FETCH_HEAD is a git-controlled string that never starts with a dash.
- At-prefixed CI refs (the npm-tag format used by the workflow) resolve correctly both with and without the guard.
scripts/upstream — single-branch clone (from the first commit)
The single-branch clone is the right optimization for an ephemeral CI checkout. Verified that tags reachable from the default branch are fetched (git clones all tags with single-branch unless no-tags is also passed), so the CI ref format resolves locally without needing the fallback. The fallback fetch covers the manual-run edge case of a branch ref not present in the single-branch clone. The existing-clone path (local development) is untouched.
verify.sh and upstreamer.md — HooksManager pinning
The grep already checked the exact HooksManager spelling; this aligns the comment and contract with that reality. Correct and well-documented.
|
@devin review — second request. Slack pings at ~10:52 and ~10:56 CDT and a GitHub mention at ~11:19 have all gone unanswered on this PR, and Perry has since reviewed the current head. Flagging explicitly rather than letting it sit silent. |
|
Correction to my earlier ping — withdrawing the escalation. Devin is not unresponsive here; it is not installed on this repo. Checked the last 20 PRs in each: zero Devin reviews on That makes Perry-at-head the complete automated coverage available here, not a partial pass waiting on a second reviewer. Apologies for the noise from the repeat If Devin coverage on these repos is wanted, that is a one-time GitHub App install rather than anything to do on this PR. |
Follow-up to the two review suggestions on #1, which merged before the fixes landed on the branch.
scripts/upstream: clone the upstream reference--single-branch(ephemeral CI clone; full ref namespace doubled clone time), with a fetch fallback so a--refoutside the default branch still resolves.verify.sh+ contract: theHooksManagercheck grepped for the exact TS spelling while its comment claimed the name was loose — a future Go-idiomatic rename would have falsely failed the gate and blocked state advancement. The merged 0.8.0 port chose the upstream spelling verbatim, so this pins it explicitly: the contract now namesHooksManageras the required Go type, and the verify.sh comment states a rename is a contract change (update naming map + check together), not drift to tolerate. Verifier re-run:PASS: 0 failures.🤖 Generated with Claude Code