fix(ce-code-review): replace resolve-base.sh with prose-driven base detection#815
fix(ce-code-review): replace resolve-base.sh with prose-driven base detection#815
Conversation
…etection Branch mode and standalone mode invoked `bash scripts/resolve-base.sh` via a bare relative path. The runtime Bash tool's CWD is the user's project, not the skill directory, so the helper was never found and the agent improvised its way through base detection on every run -- silently, since the spec-mandated "stop on script error" guard wasn't enforced in practice. Rather than fix the call site (PR #812), remove the helper. PR mode already has its own inline fork-safe base resolution (the `PR_BASE_REMOTE` block); branch and standalone modes now share that block via prose plus a default- branch fallback chain (origin/HEAD -> gh repo view -> main/master/develop/ trunk) and the hard "do not fall back to `git diff HEAD`" guard. Net result: -300 lines of script tests, -100 lines of script, +13 lines of prose. The agent does what it was already doing empirically, but now the spec admits it. Fixes #811. Supersedes #812 -- thanks Frank Stallone for the original report and PR.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3027c58447
ℹ️ 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".
| ``` | ||
| **If a PR exists for `<branch>`** (check with `gh pr view <branch> --json baseRefName,url`): reuse PR mode's `PR_BASE_REMOTE` block above. Use `baseRefName` as `<base>` and derive `<base-repo>` from the PR URL (e.g., `EveryInc/foo` from `https://github.com/EveryInc/foo/pull/123`). The block already sets `$BASE` to the merge-base SHA — `origin` may point at the user's fork, which is why naive `origin/<base>` is unsafe and the fork-safe block is required. | ||
|
|
||
| **If no PR exists**: derive the default branch. Primary source is `git symbolic-ref --quiet --short refs/remotes/origin/HEAD | sed 's#^origin/##'`; fall back to `gh repo view --json defaultBranchRef --jq '.defaultBranchRef.name'`, then to the first of `main`/`master`/`develop`/`trunk` that exists as `origin/<name>` locally. Compute `BASE=$(git merge-base HEAD origin/<base-branch>)`. If `BASE` is empty and the clone is shallow (`git rev-parse --is-shallow-repository`), run `git fetch --unshallow origin` and retry. |
There was a problem hiding this comment.
The new no-PR base resolution path only computes BASE from origin/<base-branch> and retries with git fetch --unshallow origin, but it no longer falls back to a local base ref (for example main) when the remote-tracking ref is missing. In single-branch clones, repos without an origin remote, or checkouts where origin/<default> has not been fetched, this now forces a hard stop even though a valid local base branch exists. The deleted resolve-base.sh logic previously handled this case by trying local refs after remote lookup, so this is a functional regression in branch and standalone mode scope detection.
Useful? React with 👍 / 👎.
1 participant
Summary
Fixes #811 by removing the failing helper rather than fixing how it's called. Supersedes #812.
ce-code-reviewbranch mode and standalone mode invokedbash scripts/resolve-base.shvia a bare relative path. At runtime the Bash tool's CWD is the user's project, not the skill directory, so the helper has been silently failing on every invocation — the agent improvised its way through base detection on its own judgment, which is why nobody noticed.What changed
scripts/resolve-base.sh(and its 300-line test file). The helper's most distinctive feature (fork-safe PR-base resolution) wasn't realized at the call sites that actually used it — PR mode has its own inline fork-safe block, and branch/standalone modes never had a real PR-URL to resolve against.PR_BASE_REMOTEblock for fork safety) →origin/HEAD→gh repo view→ common branch names. Compute the merge-base, unshallow on shallow clones, stop hard if nothing resolves.git diff HEAD" — a review without a base would only show uncommitted changes and silently miss all committed work. The contract test in `tests/review-skill-contract.test.ts` now pins this guard phrase per-mode.${CLAUDE_SKILL_DIR}workaround andallowed-toolsgrant that fix(ce-code-review): resolve base helper from skill dir #812 was going to add — both become unnecessary when there's no bundled script to invoke.Net: -429 lines (script + script tests), +13 lines (prose + new guard test).
Why this shape instead of #812
#812 was a correct minimal fix that kept the helper and corrected the call. Discussion in #812 and our review session converged on a different question: does the helper earn its complexity?
Empirically no — users have been hitting "couldn't find resolve-base.sh" errors, the spec said to stop, but the agent improvised and the review still completed with reasonable scope. That's a signal that the deterministic helper wasn't load-bearing in branch/standalone modes. PR mode (which DOES need fork-safe resolution) has its own inline block and is untouched.
The change trades:
allowed-tools+\${CLAUDE_SKILL_DIR}workaround → nothingTest plan
Credit
Thanks to @frankstallone for the original report (#811) and PR (#812). The closing comment on #812 explains the structural choice.