SD-10194: Change push to PR open

[inesp]

Failed:

Succeded:

the pr:

https://github.com/sleuth-io/skills-repository/pull/2/changes

[linear-code]

SD-10194

[claude]

Claude finished @inesp's task in 2m 11s —— View job

---

The PR cleanly delivers the RBAC→PR fallback and the latest commit resolved all prior MEDIUM findings (non-forcing push, random branch suffix, deferred AbortPRBranch cleanup, origin/HEAD-derived base); only a few LOW polish items remain.

[LOW] docs/rbac.md still documents the old branch name without the new random suffix

Location: docs/rbac.md:38

Issue: The doc says the asset is committed to a branch sx/add-<name>-<version>, but prBranchName (internal/commands/add.go:757) now appends a random hex suffix (sx/add-<name>-<version>-<hex>). The very change that fixed the force-push/collision risk left this line stale, so the docs understate why the branch is collision-safe.

Suggestion: Update the example to sx/add-<name>-<version>-<suffix> and note the suffix keeps retries / concurrent adds from colliding (so the push never has to force).

Fix this →

[LOW] --yes silently opens a PR on a permission denial, still undocumented

Location: internal/commands/add.go:685-687 (promptOpenPR), docs/rbac.md:36-38

Issue: Under assumeYes, promptOpenPR returns true without prompting, so a non-interactive sx add --yes that hits the edit gate now opens a PR instead of hard-failing. The behavior is reasonable, but docs/rbac.md doesn't mention that --yes auto-accepts the PR fallback — automation that previously relied on a hard permission error changes behavior with no documented signal.

Suggestion: Add one line to the "Blocked? Open a pull request" section noting that --yes proceeds to open the PR automatically.

[LOW] Compare-URL fallback prints a meaningless link for non-GitHub / file:// remotes

Location: internal/vault/gitrepo_pr.go:135-160 (openPullRequest / compareURL), :201-218 (webBaseURL)

Issue: When gh is absent or fails, compareURL unconditionally builds a …/compare/…?expand=1 URL. For a non-GitHub remote (the file:// URL used in tests, or a GitLab/Bitbucket vault) this resolves to nothing useful, yet addViaPullRequest prints it as "Open it manually here:". The feature is documented github-vault-only, so the impact is small, but the printed link is misleading.

Suggestion: Detect a GitHub host (in webBaseURL/compareURL) before emitting the compare URL; for non-GitHub remotes, print the pushed branch + base and tell the user to open the PR via their host's UI instead of a dead link.

[LOW] Commit/push aren't re-asserted to be on the PR branch under the lock (downgraded from prior MEDIUM)

Location: internal/vault/gitrepo_pr.go:100-129 (FinishPRBranch), internal/vault/gitrepo.go:661-671 (commitAndPush)

Issue: PR mode still spans three independent lock scopes — StartPRBranch (lock→checkout→unlock), AddAsset (lock→commit→unlock), FinishPRBranch (lock→push→unlock) — and the "clone stays on prBranch" invariant is held only in memory (g.prBranch). Within one process hasSynced makes this safe, but a second concurrent sx sharing the cached clone could checkout/pull a different branch between phases, so the commit (gitrepo.go:663) could land on the wrong branch. The push now targets the named ref so it's lower risk than before, but the commit isn't guarded.

Suggestion: Re-assert GetCurrentBranch == g.prBranch under the lock at the start of FinishPRBranch (and ideally just before the AddAsset commit), failing loudly if it drifted.

[claude]

Claude finished @inesp's task in 2m 53s —— View job

---

The PR is in solid shape: the prior review's findings are either fixed (docs/rbac.md now documents the random branch suffix and the --yes auto-PR behavior) or consciously deferred and recorded in docs/decisions/pr-fallback-push.md — only those two documented edges remain, and both are reasonable to defer.

[LOW] Compare-URL fallback still prints a non-actionable link for non-GitHub remotes (documented as deferred)

Location: internal/vault/gitrepo_pr.go:202-218 (compareURL / webBaseURL); noted in docs/decisions/pr-fallback-push.md:31

Issue: When gh is absent or fails, compareURL unconditionally emits a …/compare/…?expand=1 URL, which is meaningless for a file:// (test) or GitLab/Bitbucket remote, yet addViaPullRequest (internal/commands/add.go:744) prints it under "Open it manually here:". This is correctly flagged as a known unfixed edge in the decision doc, and the feature is documented github-vault-only, so impact is small — calling it out only to confirm the deferral is acceptable for this PR.

Suggestion: Fine to ship as-is given the decision-doc note. If/when revisited: detect a GitHub host in webBaseURL/compareURL and, for non-GitHub remotes, print the pushed branch + base with "open the PR via your host's UI" instead of a dead link.

[LOW] Commit/push not re-asserted to be on the PR branch under the lock (documented as deferred)

Location: internal/vault/gitrepo_pr.go:100-129 (FinishPRBranch), internal/vault/gitrepo.go:661-671 (commitAndPush); noted in docs/decisions/pr-fallback-push.md:31

Issue: PR mode spans three independent lock scopes — StartPRBranch, AddAsset, FinishPRBranch — and the "clone stays on prBranch" invariant is held only in memory (g.prBranch). Within one process hasSynced keeps this safe; a second concurrent sx sharing the cached clone could in theory checkout a different branch between phases. The non-forcing named-ref push (PushSetUpstream, internal/git/client.go:276) plus the unique branch suffix make this low-risk, and the decision doc explicitly defers it with the cheap-guard suggestion already written down.

Suggestion: Acceptable to defer. The recorded cheap guard — re-check GetCurrentBranch == g.prBranch under the lock at the start of FinishPRBranch (and before the AddAsset commit), failing loudly on drift — remains the right fix if this is ever revisited.

[LOW] `handleIdenticalAsset` path skips the edit gate (likely intentional — flagging for confirmation)

Location: internal/commands/add.go:314-318, 499-549

Issue: The RBAC edit gate (enforceAssetEditPermission) is only consulted in addNewAsset; the contentsIdentical branch routes to handleIdenticalAsset, which never checks edit permission. This looks correct — identical content republishes nothing, and scope changes still flow through resolveAddScope's own gating — so a non-member can only re-configure scope/install, not publish new content. Noting it only to confirm the asymmetry is deliberate.

Suggestion: No change needed if intended; a one-line comment on handleIdenticalAsset stating "no edit-gate check: identical content publishes nothing; scope is gated separately" would make the omission obviously intentional to future readers.

---

Beyond the above, the implementation reads well: the deferred AbortPRBranch cleanup with the finished flag correctly avoids a double-restore (the clone is never stranded on the PR branch), ghPRCreateArgs pinning --repo is well-guarded by a regression test with a clear "do not remove" rationale, and the e2e tests cover both the yes (lands on a unique branch, default branch untouched) and no (hard-fails, pushes nothing) paths. No HIGH/MEDIUM issues found.

[claude]

Claude finished @inesp's task in 2m 6s —— View job

---

Review of PR #167 — RBAC → PR fallback

The implementation is solid and thoroughly documented; all prior-review findings are either fixed or consciously deferred in docs/decisions/pr-fallback-push.md, and I found no new HIGH/MEDIUM issues — only previously-noted LOW deferrals remain.

[LOW] Compare-URL fallback builds a non-actionable link for non-GitHub remotes (documented as deferred)

Location: internal/vault/gitrepo_pr.go:135-160, 196-218 (openPullRequest / compareURL / webBaseURL / repoSlug)

Issue: When gh is absent or fails, compareURL unconditionally emits a …/compare/…?expand=1 URL, and repoSlug happily reduces a file:// (test) or GitLab/Bitbucket remote to a bogus host/path. For those remotes addViaPullRequest prints "Open it manually here:" followed by a dead link. This is correctly flagged as a known edge in docs/decisions/pr-fallback-push.md, and the feature is documented github-vault-only, so impact is small — calling it out only to confirm the deferral holds.

Suggestion: Acceptable to ship. If revisited: detect a GitHub host in webBaseURL/compareURL and, for non-GitHub remotes, print the pushed branch + base with "open the PR via your host's UI" instead of a link.

Fix this →

[LOW] Commit/push not re-asserted to be on the PR branch under the lock (documented as deferred)

Location: internal/vault/gitrepo_pr.go:100-129 (FinishPRBranch), internal/vault/gitrepo.go:667-672 (commitAndPush)

Issue: PR mode spans three independent lock scopes — StartPRBranch, AddAsset, FinishPRBranch — and the "clone stays on prBranch" invariant is held only in memory (g.prBranch). Within one process hasSynced keeps this safe; a second concurrent sx sharing the cached clone could in theory checkout a different branch between phases, so the commit could land on the wrong branch. The non-forcing named-ref push (PushSetUpstream) plus the unique <hex> suffix make this low-risk, and docs/decisions/pr-fallback-push.md explicitly defers it with the cheap-guard write-up.

Suggestion: Acceptable to defer. The recorded cheap guard — re-check GetCurrentBranch == g.prBranch under the lock at the start of FinishPRBranch (and before the AddAsset commit), failing loudly on drift — remains the right fix if revisited.

[LOW] handleIdenticalAsset path skips the edit gate (likely intentional — flagging for confirmation)

Location: internal/commands/add.go:314-318, 508-549

Issue: The RBAC edit gate (enforceAssetEditPermission) is only consulted in addNewAsset; the contentsIdentical branch routes to handleIdenticalAsset, which never checks edit permission. This looks correct — identical content republishes nothing, and scope changes still flow through resolveAddScope's own gating — so a non-member can only re-configure scope/install, not publish new content. Noting it only to confirm the asymmetry is deliberate.

Suggestion: No change needed if intended; a one-line comment on handleIdenticalAsset ("no edit-gate check: identical content publishes nothing; scope is gated separately") would make the omission obviously intentional to future readers.

---

Beyond the above, the implementation reads well: the deferred AbortPRBranch cleanup with the finished flag correctly avoids a double-restore; ghPRCreateArgs pinning --repo is well-guarded by a clear "do not remove" regression test; the non-forcing PushSetUpstream + unique branch suffix are documented and load-bearing; and the e2e tests cover both the yes path (lands on a unique branch, default branch untouched) and no path (hard-fails, pushes nothing). No HIGH/MEDIUM issues found.
• SD-10194-offer-pr

[inesp]

Still works: