Skip to content

Commit 68ce986

Browse files
authored
fix(miner): close_pr runs unconditionally before its best-effort comment (#5494)
Per gittensory's own review on this PR: buildClosePrSpec chained gh pr comment && gh pr close, so a transient comment failure would silently leave a known-losing PR open -- the exact outcome claim-conflict resolution exists to prevent. The close now runs first and unconditionally; the explanatory comment is a best-effort follow-up that only runs once the close itself succeeds, and never blocks or masks it. Also documents why assembleCompetingClaims deliberately excludes same-author PRs (a miner never competes against its own retry attempts).
1 parent 733d0a9 commit 68ce986

3 files changed

Lines changed: 12 additions & 6 deletions

File tree

packages/gittensory-engine/src/miner/local-write-tools.ts

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -45,13 +45,15 @@ export function buildOpenPrSpec(input: { repoFullName: string; base: string; hea
4545
}
4646

4747
/** Close a pull request the miner itself opened (e.g. it lost a claim-conflict adjudication to an earlier
48-
* claimant, #4848) -- never used against a PR the miner does not own. `comment`, when supplied, is posted
49-
* before the close via a separate `gh pr comment` so the reason survives on the PR even though `gh pr close`
50-
* itself has no comment-body flag. */
48+
* claimant, #4848) -- never used against a PR the miner does not own. The close runs FIRST and
49+
* unconditionally -- it's the safety-critical action (never leave a known-losing PR open); `comment`, when
50+
* supplied, is a best-effort follow-up posted only once the close itself succeeds (`gh pr close` has no
51+
* comment-body flag of its own, and a transient `gh pr comment` failure must never mask or block the close
52+
* it's explaining). */
5153
export function buildClosePrSpec(input: { repoFullName: string; number: number; comment?: string | undefined }): LocalWriteActionSpec {
5254
const closeCommand = `gh pr close ${input.number} --repo ${sq(input.repoFullName)}`;
5355
const command = input.comment
54-
? `gh pr comment ${input.number} --repo ${sq(input.repoFullName)} --body ${sq(input.comment)} && ${closeCommand}`
56+
? `${closeCommand} && gh pr comment ${input.number} --repo ${sq(input.repoFullName)} --body ${sq(input.comment)}`
5557
: closeCommand;
5658
return spec(
5759
"close_pr",

packages/gittensory-miner/lib/claim-conflict-resolver.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,10 @@ import { buildClosePrSpec } from "@jsonbored/gittensory-engine";
2929
* Assemble the real competing-claims set from a fetched LiveIssueSnapshot: every OTHER open PR referencing
3030
* the issue, excluding `selfPrNumber` and any PR authored by `minerLogin` itself (case-insensitive, mirrors
3131
* checkSubmissionFreshness's own author comparison -- a login can be echoed back with different casing).
32+
* Excluding same-author PRs is deliberate, not an edge case slipping through: a miner never competes against
33+
* its own work, so if this login somehow has ANOTHER open PR on the same issue (e.g. a retry after a crash
34+
* left a stale one behind), that PR is never treated as a competing claim to lose against -- only a genuinely
35+
* different claimant's PR can trigger a real close.
3236
* Pure given its inputs.
3337
*
3438
* @param {import("./submission-freshness-check.js").LiveIssueSnapshot | null | undefined} snapshot

test/unit/local-write-tools.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,10 +37,10 @@ describe("local write-tool specs (#780)", () => {
3737
expect(s.command.endsWith("--draft")).toBe(true);
3838
});
3939

40-
it("close_pr closes with a preceding comment when one is supplied", () => {
40+
it("close_pr closes FIRST (unconditionally), then best-effort comments when one is supplied", () => {
4141
const s = buildClosePrSpec({ repoFullName: "o/r", number: 7, comment: "Closing: lost the claim to #5" });
4242
expect(s.action).toBe("close_pr");
43-
expect(s.command).toBe("gh pr comment 7 --repo 'o/r' --body 'Closing: lost the claim to #5' && gh pr close 7 --repo 'o/r'");
43+
expect(s.command).toBe("gh pr close 7 --repo 'o/r' && gh pr comment 7 --repo 'o/r' --body 'Closing: lost the claim to #5'");
4444
expect(s.boundary).toBe(LOCAL_WRITE_BOUNDARY);
4545
expect(s.inputs).toEqual({ repoFullName: "o/r", number: 7, comment: "Closing: lost the claim to #5" });
4646
});

0 commit comments

Comments
 (0)