Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/gittensory-engine/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,7 @@ export {
export {
LOCAL_WRITE_BOUNDARY,
buildApplyLabelsSpec,
buildClosePrSpec,
buildCreateBranchSpec,
buildDeleteBranchSpec,
buildFileIssueSpec,
Expand Down
17 changes: 17 additions & 0 deletions packages/gittensory-engine/src/miner/local-write-tools.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,23 @@ export function buildOpenPrSpec(input: { repoFullName: string; base: string; hea
return spec("open_pr", "Open a pull request from your local branch.", { repoFullName: input.repoFullName, base: input.base, head: input.head, title: input.title, body: input.body, draft }, command);
}

/** Close a pull request the miner itself opened (e.g. it lost a claim-conflict adjudication to an earlier
* claimant, #4848) -- never used against a PR the miner does not own. `comment`, when supplied, is posted
* before the close via a separate `gh pr comment` so the reason survives on the PR even though `gh pr close`
* itself has no comment-body flag. */
export function buildClosePrSpec(input: { repoFullName: string; number: number; comment?: string | undefined }): LocalWriteActionSpec {
const closeCommand = `gh pr close ${input.number} --repo ${sq(input.repoFullName)}`;
const command = input.comment
? `gh pr comment ${input.number} --repo ${sq(input.repoFullName)} --body ${sq(input.comment)} && ${closeCommand}`
: closeCommand;
return spec(
"close_pr",
"Close a pull request you opened.",
{ repoFullName: input.repoFullName, number: input.number, ...(input.comment ? { comment: input.comment } : {}) },
command,
);
}

/** File an issue (e.g. an issue-discovery proposal). */
export function buildFileIssueSpec(input: { repoFullName: string; title: string; body: string; labels?: string[] | undefined }): LocalWriteActionSpec {
const labels = input.labels ?? [];
Expand Down
3 changes: 3 additions & 0 deletions packages/gittensory-miner/lib/attempt-cli.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import type { buildCodingTaskSpec } from "./coding-task-spec.js";
import type { resolveAmsPolicy } from "./ams-policy.js";
import type { checkMinerKillSwitch } from "./governor-kill-switch.js";
import type { resolveMinerGoalSpec } from "./miner-goal-spec.js";
import type { ClaimConflictResult, resolveClaimConflict } from "./claim-conflict-resolver.js";

type CommonAttemptResultFields = {
repoFullName: string;
Expand Down Expand Up @@ -45,6 +46,7 @@ export type AttemptCliResult =
decision?: unknown;
spec?: LocalWriteActionSpec;
execResult?: unknown;
claimConflict?: ClaimConflictResult;
});

export type ParsedAttemptArgs =
Expand Down Expand Up @@ -79,6 +81,7 @@ export type RunAttemptOptions = {
checkMinerKillSwitch?: typeof checkMinerKillSwitch;
resolveMinerGoalSpec?: typeof resolveMinerGoalSpec;
runMinerAttempt?: typeof runMinerAttempt;
resolveClaimConflict?: typeof resolveClaimConflict;
/** Invoked with the real structured result at every return point, in addition to (never instead of) the
* plain exit-code return -- the loop orchestrator's real hook into what actually happened. */
onResult?: (result: AttemptCliResult) => void;
Expand Down
41 changes: 37 additions & 4 deletions packages/gittensory-miner/lib/attempt-cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
// (worktree-allocator.js + attempt-worktree.js), the four ledgers (claim/event/attempt-log/governor), the
// real coding-agent driver (#5131) and slop assessor (#5133), a live SelfReviewContext fetch (#5145), a real
// coding-task spec (#5239), the operator's AmsPolicySpec execution policy (#5249), rejectionSignaled (#5241),
// and finally a real runMinerAttempt call -- the first point in this epic where a real coding agent actually
// runs, not just checks-and-reports-blocked.
// a real runMinerAttempt call -- the first point in this epic where a real coding agent actually runs, not
// just checks-and-reports-blocked -- and, only on a real "submitted" outcome, a real post-submission
// claim-conflict resolution (#4848, claim-conflict-resolver.js) for the narrow race window
// checkSubmissionFreshness cannot see (two miners submitting almost simultaneously).
//
// KNOWN, DOCUMENTED GAPS (not fabricated -- see attempt-input-builder.js's own header for the full list):
// governor.convergenceInput is an honest first-attempt-shaped literal, not a real per-issue attempt-history
Expand All @@ -17,6 +19,8 @@ import { fetchLiveIssueSnapshot } from "./live-issue-snapshot.js";
import { executeLocalWrite } from "./execute-local-write.js";
import { openClaimLedger } from "./claim-ledger.js";
import { resolveMinerGoalSpec } from "./miner-goal-spec.js";
import { resolveClaimConflict } from "./claim-conflict-resolver.js";
import { parsePrNumberFromExecResult } from "./pr-number-parse.js";
import { initEventLedger } from "./event-ledger.js";
import { initAttemptLog } from "./attempt-log.js";
import { initGovernorLedger } from "./governor-ledger.js";
Expand Down Expand Up @@ -362,8 +366,9 @@ export async function runAttempt(args, options = {}) {
// Real soft-claim (#5393): recorded once we've committed to a real attempt (past feasibility), so a
// sibling miner process on this machine sees it via claimLedger.listClaims/listActiveClaims while this
// attempt is in flight. Released in `finally` on every terminal outcome -- mirrors the worktree
// allocation slot's own acquire-then-always-release pattern below.
claimLedger.claimIssue(parsed.repoFullName, parsed.issueNumber, `attempt:${attemptId}`);
// allocation slot's own acquire-then-always-release pattern below. The real claimedAt this returns is
// ALSO this miner's own claim-time for the post-submission conflict check further down (#4848).
const claimRecord = claimLedger.claimIssue(parsed.repoFullName, parsed.issueNumber, `attempt:${attemptId}`);
claimedIssue = true;

const runAttemptPipeline = options.runMinerAttempt ?? runMinerAttempt;
Expand All @@ -382,6 +387,30 @@ export async function runAttempt(args, options = {}) {
);

worktreeResult.attemptOk = result.outcome === "submitted";

// Real claim-conflict resolution (#4848): only meaningful once a real PR exists, so this only ever runs
// on a real "submitted" outcome. checkSubmissionFreshness (inside runMinerAttempt) already caught the
// common pre-submission case; this closes the narrower TOCTOU window where two miners raced past that
// check almost simultaneously -- see claim-conflict-resolver.js's own header for why the adjudicator
// can only run POST-submission (it needs a real PR number on both sides of the election).
let claimConflict;
if (result.outcome === "submitted") {
const selfPrNumber = parsePrNumberFromExecResult(result.execResult, parsed.repoFullName);
if (selfPrNumber !== null) {
const resolveConflict = options.resolveClaimConflict ?? resolveClaimConflict;
claimConflict = await resolveConflict(
{
repoFullName: parsed.repoFullName,
issueNumber: parsed.issueNumber,
selfPrNumber,
selfClaimedAt: claimRecord.claimedAt,
minerLogin: parsed.minerLogin,
},
{ fetchLiveIssueSnapshot: deps.fetchLiveIssueSnapshot, executeLocalWrite: deps.executeLocalWrite },
);
}
}

const finalResult = {
outcome: `attempt_${result.outcome}`,
repoFullName: parsed.repoFullName,
Expand All @@ -404,6 +433,10 @@ export async function runAttempt(args, options = {}) {
...("decision" in result ? { decision: result.decision } : {}),
...("spec" in result ? { spec: result.spec } : {}),
...("execResult" in result ? { execResult: result.execResult } : {}),
// Present only on a real "submitted" outcome whose PR number was recoverable from execResult -- omitted
// (not fabricated as "checked: false") on every other outcome, and on a submitted outcome where the new
// PR's number genuinely couldn't be parsed (an honest gap, not silently swallowed).
...(claimConflict !== undefined ? { claimConflict } : {}),
};

if (parsed.json) {
Expand Down
29 changes: 29 additions & 0 deletions packages/gittensory-miner/lib/claim-conflict-resolver.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import type { LiveIssueSnapshot } from "./submission-freshness-check.js";
import type { ObservedClaim } from "./claim-adjudication.js";
import type { LocalWriteActionSpec } from "@jsonbored/gittensory-engine";

export function assembleCompetingClaims(
snapshot: LiveIssueSnapshot | null | undefined,
selfPrNumber: number,
minerLogin: string,
): ObservedClaim[];

export type ClaimConflictInput = {
repoFullName: string;
issueNumber: number;
selfPrNumber: number;
selfClaimedAt: string | null;
minerLogin: string;
};

export type ClaimConflictDeps = {
fetchLiveIssueSnapshot: (repoFullName: string, issueNumber: number) => Promise<LiveIssueSnapshot | null>;
executeLocalWrite: (spec: LocalWriteActionSpec) => Promise<unknown>;
};

export type ClaimConflictResult =
| { checked: false; reason: "live_state_unavailable" }
| { checked: true; isWinner: true; winnerNumber: number | null; competingCount: number }
| { checked: true; isWinner: false; winnerNumber: number | null; competingCount: number; closeResult: unknown };

export function resolveClaimConflict(input: ClaimConflictInput, deps: ClaimConflictDeps): Promise<ClaimConflictResult>;
97 changes: 97 additions & 0 deletions packages/gittensory-miner/lib/claim-conflict-resolver.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
// Real claim-conflict resolution (#4848): the missing piece over claim-adjudication.js's own adjudicator,
// which is correct and well-tested in isolation but has no caller that assembles a REAL competing-claims set.
// checkSubmissionFreshness (submission-freshness-check.js) already catches the common case pre-submission --
// aborting before open_pr if another author's PR already references the issue -- but that check can only see
// what's PUBLIC at the moment it runs. Two miners racing closely enough that BOTH pass their own freshness
// check before either's PR exists yet is a genuine TOCTOU window freshness cannot close. This module is the
// POST-submission reconciliation for exactly that window: once THIS miner's PR is real and public, check
// whether ANOTHER open PR also claims the same issue and, if this miner's claim loses the election, close its
// own just-opened PR (never anyone else's) -- the write action the contributor-vs-maintainer safety framework
// keeps maintainer-only (#4833's own scope note), since it means the autonomous loop acts on a race-resolution
// decision with no human review.
//
// CLAIM-TIME ASYMMETRY (documented, not accidental): `self`'s claimedAt is the miner's OWN real local
// claim-ledger timestamp (claim-ledger.js, recorded before work even started). A competing PR's claimedAt uses
// its real GitHub `createdAt` instead -- the maintainer gate's own duplicate-winner election uses gittensory
// server's "first observed this PR's linked-issue set" timestamp, but that requires a continuous, persistent
// observation history this stateless client-side tool does not have for a PR it doesn't own. `createdAt` is
// the best real, publicly-observable proxy available for someone else's PR -- live-issue-snapshot.js's own
// comment on `createdAt` explains this in more detail.
//
// EVENTUAL CONSISTENCY: this checks GitHub's live state immediately after submission. A competing PR that
// exists but hasn't yet propagated through GitHub's own search/GraphQL indexing in that instant would be
// invisible to this one-shot check -- there is no retry/backoff here, which would be its own separate scope.

import { adjudicateSoftClaim } from "./claim-adjudication.js";
import { buildClosePrSpec } from "@jsonbored/gittensory-engine";

/**
* Assemble the real competing-claims set from a fetched LiveIssueSnapshot: every OTHER open PR referencing
* the issue, excluding `selfPrNumber` and any PR authored by `minerLogin` itself (case-insensitive, mirrors
* checkSubmissionFreshness's own author comparison -- a login can be echoed back with different casing).
* Pure given its inputs.
*
* @param {import("./submission-freshness-check.js").LiveIssueSnapshot | null | undefined} snapshot
* @param {number} selfPrNumber
* @param {string} minerLogin
* @returns {import("./claim-adjudication.js").ObservedClaim[]}
*/
export function assembleCompetingClaims(snapshot, selfPrNumber, minerLogin) {
const minerLoginKey = minerLogin.trim().toLowerCase();
const referencingPrs = Array.isArray(snapshot?.referencingPrs) ? snapshot.referencingPrs : [];
return referencingPrs
.filter((pr) => pr.state === "open" && pr.number !== selfPrNumber)
.filter((pr) => typeof pr.authorLogin !== "string" || pr.authorLogin.trim().toLowerCase() !== minerLoginKey)
.map((pr) => ({ number: pr.number, claimedAt: pr.createdAt ?? null }));
}

/**
* Resolve a real claim conflict for an already-submitted PR. Fails OPEN (never closes anything) when the live
* snapshot can't be fetched -- an unavailable check is not evidence of a lost claim.
*
* @param {{ repoFullName: string, issueNumber: number, selfPrNumber: number, selfClaimedAt: string | null, minerLogin: string }} input
* @param {{
* fetchLiveIssueSnapshot: (repoFullName: string, issueNumber: number) => Promise<import("./submission-freshness-check.js").LiveIssueSnapshot | null>,
* executeLocalWrite: (spec: import("@jsonbored/gittensory-engine").LocalWriteActionSpec) => Promise<unknown>,
* }} deps
* @returns {Promise<{
* checked: boolean,
* reason?: "live_state_unavailable",
* isWinner?: boolean,
* winnerNumber?: number | null,
* competingCount?: number,
* closeResult?: unknown,
* }>}
*/
export async function resolveClaimConflict(input, deps) {
let snapshot;
try {
snapshot = await deps.fetchLiveIssueSnapshot(input.repoFullName, input.issueNumber);
} catch {
snapshot = null;
}
if (!snapshot || typeof snapshot !== "object") {
return { checked: false, reason: "live_state_unavailable" };
}

const competing = assembleCompetingClaims(snapshot, input.selfPrNumber, input.minerLogin);
const adjudication = adjudicateSoftClaim({ number: input.selfPrNumber, claimedAt: input.selfClaimedAt }, competing);

if (adjudication.isWinner) {
return { checked: true, isWinner: true, winnerNumber: adjudication.winnerNumber, competingCount: competing.length };
}

const comment = adjudication.winnerNumber
? `Closing this PR: pull request #${adjudication.winnerNumber} claimed this issue first. This is an automated soft-claim conflict resolution -- no action needed from you.`
: `Closing this PR: another open pull request already claims this issue. This is an automated soft-claim conflict resolution -- no action needed from you.`;
const spec = buildClosePrSpec({ repoFullName: input.repoFullName, number: input.selfPrNumber, comment });
const closeResult = await deps.executeLocalWrite(spec);

return {
checked: true,
isWinner: false,
winnerNumber: adjudication.winnerNumber,
competingCount: competing.length,
closeResult,
};
}
10 changes: 9 additions & 1 deletion packages/gittensory-miner/lib/live-issue-snapshot.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const LIVE_ISSUE_SNAPSHOT_QUERY = `
number
state
author { login }
createdAt
}
}
}
Expand Down Expand Up @@ -50,7 +51,14 @@ function normalizeReferencingPr(node) {
const state = normalizeIssueOrPrState(node.state);
if (state !== "open" && state !== "closed" && state !== "merged") return null;
const authorLogin = typeof node.author?.login === "string" ? node.author.login : "";
return { number: node.number, state, authorLogin };
// GitHub's real PR creation timestamp (ISO 8601), when present -- null otherwise (never fabricated). Not
// an ordering signal for the maintainer gate's own duplicate-cluster election (duplicate-winner.ts's own
// doc explains why: a PR can be backdated by editing an old placeholder to add the linked issue later), but
// it's the only real, publicly-observable claim-time proxy claim-conflict-resolver.js's own client-side
// caller has for a THIRD-PARTY PR -- unlike gittensory's own server, the miner has no continuous observation
// history to derive a true "first linked" timestamp from.
const createdAt = typeof node.createdAt === "string" ? node.createdAt : null;
return { number: node.number, state, authorLogin, createdAt };
}

function parseRepoFullName(repoFullName) {
Expand Down
15 changes: 1 addition & 14 deletions packages/gittensory-miner/lib/loop-cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import { pollCheckRuns } from "./ci-poller.js";
import { recordPrOutcomeSnapshot } from "./pr-outcome.js";
import { buildLoopClosureSummary } from "./loop-closure.js";
import { attemptLoopReentry } from "./loop-reentry.js";
import { parsePrNumberFromExecResult } from "./pr-number-parse.js";
import { DEFAULT_AMS_POLICY_SPEC } from "@jsonbored/gittensory-engine";

const LOOP_USAGE =
Expand Down Expand Up @@ -150,20 +151,6 @@ function parseIssueNumberFromIdentifier(identifier) {
return match ? Number(match[1]) : null;
}

/** `gh pr create` (local-write-tools.ts's `buildOpenPrSpec` -- no `--json` flag) prints the created PR's own
* URL to stdout on success; this is `gh`'s real, documented, stable CLI behavior, not an invented contract.
* Scoped to the exact target repo so an unrelated URL elsewhere in stdout/stderr noise can never match. */
function parsePrNumberFromExecResult(execResult, repoFullName) {
if (!execResult || execResult.timedOut || execResult.code !== 0 || typeof execResult.stdout !== "string") {
return null;
}
const escapedRepo = repoFullName.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
const match = execResult.stdout.match(new RegExp(`github\\.com/${escapedRepo}/pull/(\\d+)`));
if (!match) return null;
const prNumber = Number(match[1]);
return Number.isInteger(prNumber) && prNumber > 0 ? prNumber : null;
}

function convergenceKey(repoFullName, identifier) {
return `${repoFullName}:${identifier}`;
}
Expand Down
4 changes: 4 additions & 0 deletions packages/gittensory-miner/lib/pr-number-parse.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export function parsePrNumberFromExecResult(
execResult: { stdout?: string | undefined; code?: number | null | undefined; timedOut?: boolean | undefined } | null | undefined,
repoFullName: string,
): number | null;
23 changes: 23 additions & 0 deletions packages/gittensory-miner/lib/pr-number-parse.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// Shared PR-number extraction from a real `gh pr create` executeLocalWrite result (#4848). `gh pr create`
// prints the new PR's URL to stdout on success -- this is the one place that URL is authoritatively parsed,
// so loop-cli.js's CI/gate-status polling and attempt-cli.js's post-submission claim-conflict check agree on
// exactly how a PR number is recovered from a real command's raw output.

/** `gh pr create` (local-write-tools.ts's `buildOpenPrSpec` -- no `--json` flag) prints the created PR's own
* URL to stdout on success; this is `gh`'s real, documented, stable CLI behavior, not an invented contract.
* Scoped to the exact target repo so an unrelated URL elsewhere in stdout/stderr noise can never match.
*
* @param {{ stdout?: string, code?: number | null, timedOut?: boolean } | null | undefined} execResult
* @param {string} repoFullName
* @returns {number | null}
*/
export function parsePrNumberFromExecResult(execResult, repoFullName) {
if (!execResult || execResult.timedOut || execResult.code !== 0 || typeof execResult.stdout !== "string") {
return null;
}
const escapedRepo = repoFullName.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
const match = execResult.stdout.match(new RegExp(`github\\.com/${escapedRepo}/pull/(\\d+)`));
if (!match) return null;
const prNumber = Number(match[1]);
return Number.isInteger(prNumber) && prNumber > 0 ? prNumber : null;
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ export type SubmissionFreshnessCandidate = {

export type LiveIssueSnapshot = {
state: "open" | "closed";
referencingPrs: Array<{ number: number; state: "open" | "closed" | "merged"; authorLogin: string }>;
referencingPrs: Array<{ number: number; state: "open" | "closed" | "merged"; authorLogin: string; createdAt: string | null }>;
};

export type SubmissionFreshnessClaimLedger = {
Expand Down
Loading