Description
Two allowlists in this org disagree about whether git merge-base is a permitted read-only subcommand, and the disagreement now has a workaround shipped in production code.
| allowlist |
merge-base |
@bounded-systems/git → ALLOWED_SUBCOMMANDS (the execGit seam) |
absent |
packages/prx/src/tools/git_read.ts → READ_ONLY_GIT_SUBCOMMANDS |
present |
git_read.ts is the stricter-sounding of the two — it describes itself as "a deny-by-default contract that admits only purely-read git subcommands" and explicitly excludes flag-dependent-mutating verbs like branch and config. execGit's list, by contrast, admits push, commit, checkout and merge. So the more conservative allowlist permits merge-base and the less conservative one does not, which is backwards from what either file's rationale would predict.
How it surfaced
#119 / #1037 needed the merge base of HEAD and origin/main. submit/stage.ts reads git through execGit, so merge-base was unavailable, and the fix reads it with log instead:
mergeBase: (a, b, cwd) =>
parseMergeBase(gitOrNull("log", ["--boundary", "--format=%m %H", `${a}...${b}`], cwd)),This is sound — the boundary of a symmetric difference is the merge base, it is the classic rev-list --boundary idiom, and log sits in exactly the same read-only policy tier as the diff/rev-parse that module already used, so nothing was escalated. It is covered by three real-git tests. But it is a workaround for an allowlist gap, not a preferred spelling, and it costs a parser plus ~10 lines of explanatory comment where merge-base would have been one call.
Why merge-base is a safe addition
It is purely read-only under every flag: it computes and prints commit SHAs and never writes a ref, an object, or the index. --is-ancestor only sets an exit code. It is strictly less powerful than log, which is already allowed.
Note on where the fix goes
ALLOWED_SUBCOMMANDS lives in @bounded-systems/git (consumed here as npm:@jsr/bounded-systems__git), not in this repo, so the change is a bump there plus a dependency update here. Filed in prx because this is where the workaround lives and where the follow-up (deleting it) lands. Transfer if the seam's repo is the better home.
Acceptance Criteria
- The two allowlists agree about
merge-base, in whichever direction is decided — if it is deliberately excluded from execGit, that rationale is written down, since its absence currently reads as an oversight.
- If added:
submit/stage.ts drops parseMergeBase and the log --boundary comment in favour of the direct call, with the existing real-git tests in test/submit/stage.test.ts unchanged and still passing — they assert the resolved SHA, not the mechanism, so they should survive the swap untouched. That is the check on whether this was a true refactor.
Description
Two allowlists in this org disagree about whether
git merge-baseis a permitted read-only subcommand, and the disagreement now has a workaround shipped in production code.merge-base@bounded-systems/git→ALLOWED_SUBCOMMANDS(theexecGitseam)packages/prx/src/tools/git_read.ts→READ_ONLY_GIT_SUBCOMMANDSgit_read.tsis the stricter-sounding of the two — it describes itself as "a deny-by-default contract that admits only purely-read git subcommands" and explicitly excludes flag-dependent-mutating verbs likebranchandconfig.execGit's list, by contrast, admitspush,commit,checkoutandmerge. So the more conservative allowlist permitsmerge-baseand the less conservative one does not, which is backwards from what either file's rationale would predict.How it surfaced
#119 / #1037 needed the merge base of
HEADandorigin/main.submit/stage.tsreads git throughexecGit, somerge-basewas unavailable, and the fix reads it withloginstead:This is sound — the boundary of a symmetric difference is the merge base, it is the classic
rev-list --boundaryidiom, andlogsits in exactly the same read-only policy tier as thediff/rev-parsethat module already used, so nothing was escalated. It is covered by three real-git tests. But it is a workaround for an allowlist gap, not a preferred spelling, and it costs a parser plus ~10 lines of explanatory comment wheremerge-basewould have been one call.Why
merge-baseis a safe additionIt is purely read-only under every flag: it computes and prints commit SHAs and never writes a ref, an object, or the index.
--is-ancestoronly sets an exit code. It is strictly less powerful thanlog, which is already allowed.Note on where the fix goes
ALLOWED_SUBCOMMANDSlives in@bounded-systems/git(consumed here asnpm:@jsr/bounded-systems__git), not in this repo, so the change is a bump there plus a dependency update here. Filed inprxbecause this is where the workaround lives and where the follow-up (deleting it) lands. Transfer if the seam's repo is the better home.Acceptance Criteria
merge-base, in whichever direction is decided — if it is deliberately excluded fromexecGit, that rationale is written down, since its absence currently reads as an oversight.submit/stage.tsdropsparseMergeBaseand thelog --boundarycomment in favour of the direct call, with the existing real-git tests intest/submit/stage.test.tsunchanged and still passing — they assert the resolved SHA, not the mechanism, so they should survive the swap untouched. That is the check on whether this was a true refactor.