Skip to content

feat(local): review PR refs from any branch#7

Merged
frankekn merged 5 commits into
frankekn:mainfrom
samzong:feat/local-pr-review
Jun 29, 2026
Merged

feat(local): review PR refs from any branch#7
frankekn merged 5 commits into
frankekn:mainfrom
samzong:feat/local-pr-review

Conversation

@samzong

@samzong samzong commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

What's changed?

  • Review local --pr by resolving PR base/head refs and running against the PR head from any branch.
  • Share PR diff construction with GitHub Action mode.
  • Add repo helper coverage for PR diffs and detached worktree review paths.
  • Update README/CLI wording for the new local --pr behavior.

Why

  • Local PR reviews should match the PR base..head range instead of depending on the current checkout branch.

Verification

  • ./node_modules/.bin/tsc --noEmit
  • node scripts/test.mjs (39 passed)
  • git diff --check && git diff --cached --check
  • /pre-ship --committed (0 MUST-FIX)

Signed-off-by: samzong <samzong.lu@gmail.com>
@samzong samzong force-pushed the feat/local-pr-review branch from 8fa2a43 to e614b53 Compare June 29, 2026 06:36
@frankekn

Copy link
Copy Markdown
Owner

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: de24fe7025

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

return {
repoPath: sandboxPath,
prompt: options.prompt.split(options.repoPath).join(sandboxPath),
prompt: options.prompt.split(sourceRepoPath).join(sandboxPath),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Rewrite relative repo paths in sandbox prompts

When needlefish pr 123 --repo ../app or any relative --repo is used, the bundle still contains that relative repoPath because runLocal/runLocalPr pass cwd through unchanged, but this replacement only looks for path.resolve(options.repoPath). The runner is then started in the temp sandbox, so any model command that follows the bundle's repoPath will fail or inspect a non-sandbox checkout instead of the PR head, defeating the new any-branch PR review path; normalize the bundle path or replace both the original and resolved forms.

Useful? React with 👍 / 👎.

Comment thread src/shared/repo.ts Outdated
hasCommit(cwd, pr.baseSha) &&
hasCommit(cwd, pr.headSha) &&
hasMergeBase(cwd, pr.baseSha, pr.headSha);
if (ready()) return;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid returning before deepening shallow PR refs

In a shallow detached checkout where the PR base, head, and merge-base are already present, this returns before the unshallow fallback runs. The later runner sandbox has to clone/fetch from that shallow source; if the PR head only exists through FETCH_HEAD rather than a local ref, Git can reject the shallow-root update or leave no usable FETCH_HEAD, so needlefish pr aborts even though ensurePrCommits declared the repo ready. Keep deepening shallow repos or materialize the PR head under a local ref before returning.

Useful? React with 👍 / 👎.

@frankekn

Copy link
Copy Markdown
Owner

Addressed both Codex findings in a2e793b:

  • Relative --repo paths: runLocal/runLocalPr now resolve the repo path before building the review bundle, so runner prompts point at the sandbox checkout instead of a relative caller path. Covered by src/adapters/local.test.ts.
  • Shallow PR refs: ensurePrCommits no longer returns while the source repo is still shallow; it deepens PR refs before the runner sandbox clone/fetch path. Covered by src/shared/repo.test.ts.

Verified locally:

  • pnpm exec tsc --noEmit
  • pnpm test
  • git diff --check

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a2e793ba04

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

return {
repoPath: sandboxPath,
prompt: options.prompt.split(options.repoPath).join(sandboxPath),
prompt: options.prompt.split(sourceRepoPath).join(sandboxPath),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Replace relative repo paths in sandbox prompts

When GitHub mode is invoked with a relative --repo (the parser still accepts --repo for github commands, and runGithub passes cwd through to makeBundle as repoPath at src/adapters/github.ts:122-124), the bundle contains that relative path. This replacement only searches for the resolved absolute path, so the prompt sent to the runner keeps repoPath: "../checkout" instead of the sandbox path; any runner command that follows the prompt path can read from the caller-relative location or fail outside the clone. Resolve the repo before bundling in runGithub, or replace both the raw and resolved paths here.

Useful? React with 👍 / 👎.

@frankekn

Copy link
Copy Markdown
Owner

Addressed the latest Codex finding in 894d9fe:

  • GitHub mode now resolves the repo path before git operations and bundle creation, so relative --repo values cannot leak into runner prompts as caller-relative sandbox paths. Covered by src/adapters/github.test.ts.

Verified locally:

  • node --test --import tsx src/adapters/github.test.ts
  • node --test --import tsx src/adapters/local.test.ts src/shared/repo.test.ts src/adapters/github.test.ts
  • pnpm exec tsc --noEmit
  • pnpm test
  • git diff --check

@codex review

@frankekn

Copy link
Copy Markdown
Owner

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🚀

Reviewed commit: 894d9fe6ac

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@frankekn

Copy link
Copy Markdown
Owner

Prepared changelog for PR #7 in 7b1dc1d:

Verified locally:

  • pnpm exec tsc --noEmit
  • pnpm test
  • git diff --check

@codex review

@frankekn frankekn merged commit d81e775 into frankekn:main Jun 29, 2026
@frankekn

Copy link
Copy Markdown
Owner

Merged as d81e775.

Thanks @samzong.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7b1dc1d708

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/cli/args.ts
Comment on lines +46 to +47
--focus <text> narrow the review lens
--deep wider context (call sites, history, adjacent tests)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 Badge Move local-only flags out of shared help

When users run needlefish --help, --focus and --deep are advertised under “Shared options,” but GitHub mode still rejects both options in this parser. A user following the help text with needlefish --github --pr 1 --focus security gets an error instead, so these flags should either be moved to a local/PR-only section or accepted in GitHub mode.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants