Over-broad pattern silently hijacks non-partial-clone upload-pack errors: /remote error: upload-pack/i matches any remote upload-pack error — auth failures, rate-limiting, server-side packing errors — not only the not our ref partial-clone case this function is meant to detect. A transient auth error would incorrectly trigger the backfill path, waste time on a git fetch --no-filter, and then surface the misleading message \u201cthe required commit objects could not be backfilled\u201d instead of the real error.

- /remote error: upload-pack/i.test(output) ||

object alternation in pattern 4 is too broad — misclassifies genuine merge conflicts: The regex /(unable to read|object).*(missing|not found)/i uses object as a bare alternation. Any git error mentioning the word "object" followed anywhere by "missing" or "not found" matches — including messages from a normal conflict (e.g. error: Your local changes to the following files would be overwritten by merge: ... object or similar). This silently promotes a genuine merge conflict into the partial-clone recovery path, wasting an expensive fetch-and-retry before ultimately failing with a misleading error.

Similarly, /fatal: bad object/i fires for wrong ref names, detached HEAD anomalies, and corrupt local objects — none of which are recoverable via promisor backfill.

- /(unable to read|object).*(missing|not found)/i.test(output) ||
- /fatal: bad object/i.test(output)
+ /unable to read [0-9a-f]{40}/i.test(output) ||
+ /fatal: object [0-9a-f]{40} is unavailable/i.test(output)

headOid may be a rebase-internal SHA if --abort failed silently, poisoning the backfill fetch: rev-parse HEAD is called after git rebase --abort whose failure is swallowed. If abort failed (e.g. the rebase-state lock is held), HEAD remains pointing at the cherry-pick-in-progress commit — a local-only SHA that does not exist on origin. Passing that SHA to git fetch --no-filter origin <sha1> <sha2> <bad-sha> causes the entire fetch to fail (git fetch is all-or-nothing for multi-ref fetches), so recovered = false and the caller throws the misleading "could not be backfilled" error rather than attempting recovery with just the two known-good anchor SHAs.

Additionally, the filter at line 449 (sha.length > 0) is weaker than backfillCommitObjects's own guard (/^[0-9a-f]{40}$/i), so the core.warning message may report e.g. "3 anchor commit(s)" while only 2 valid SHAs are actually fetched.

- const fetchTargets = [firstGraphqlParentOid, firstReplayParentOid, headOid].filter(sha => typeof sha === "string" && sha.length > 0);
+ const headOidValid = typeof headOid === "string" && /^[0-9a-f]{40}$/i.test(headOid);
+ const fetchTargets = [firstGraphqlParentOid, firstReplayParentOid, ...(headOidValid ? [headOid] : [])];

Fetching only 3 anchor commits does not guarantee all intermediate objects are present — retry will fail with the same promisor error for long ranges: backfillCommitObjects fetches exactly firstGraphqlParentOid, firstReplayParentOid, and HEAD. In a partial clone, git rebase --onto needs the full tree+blob content for every commit in the range firstReplayParentOid..HEAD. For a branch that is tens or hundreds of commits behind, git must lazily fetch intermediate objects for each cherry-picked commit in turn. Fetching only the three endpoints does not pre-materialise those blobs — the retry rebase hits the same could not fetch ... from promisor remote failure on the first intermediate commit.

The error at line 467 then says "failed to rebase ... even after backfilling the required commit objects", implying the objects were backfilled but the rebase still failed — which is misleading; the objects were never fully backfilled.

git fetch --no-filter origin firstReplayParentOid..firstGraphqlParentOid

Dead outer catch around backfillCommitObjects is unreachable and misleading: backfillCommitObjects catches all errors internally and always returns a boolean — it never throws. The catch (recoveryError) block at this line can therefore never execute, and the core.warning inside it will never fire. This makes the code look like a defensive guard when it is actually dead code, and any future reader will assume backfillCommitObjects can throw (it can't without changing its internal contract).

- let recovered = false;
- try {
-   recovered = await backfillCommitObjects(exec, fetchTargets, { cwd, env: { ...process.env, ...(gitAuthEnv || {}) } });
- } catch (recoveryError) {
-   core.warning(`pushSignedCommits: targeted object backfill failed: ...`);
- }
+ const recovered = await backfillCommitObjects(exec, fetchTargets, { cwd, env: { ...process.env, ...(gitAuthEnv || {}) } });

[/diagnose] This regex is too broad and risks false-positives that would suppress real errors.

(unable to read|object).*(missing|not found) can match git output well beyond promisor failures — e.g. a locally corrupted object or working-tree message. A false positive causes the code to abort a healthy rebase failure, run a useless targeted fetch, and surface a confusing "even after backfilling" error instead of the real root cause.

// before
/(unable to read|object).*(missing|not found)/i.test(output)

// after — require a 40-char SHA adjacent to the keyword
/(?:unable to read|object) [0-9a-f]{40}|[0-9a-f]{40}.*(?:missing|not found)/i.test(output)

[/diagnose] fatal: bad object is too generic — it fires for invalid/missing revision arguments unrelated to promisor fetches (e.g. a bad branch name, a deleted ref).

A false positive would incorrectly trigger the backfill path for an unrecoverable error, masking the real cause.

// before
/fatal: bad object/i.test(output)

// after — only match when a 40-char OID follows the phrase
/fatal: bad object [0-9a-f]{40}/i.test(output)

[/tdd] isPartialCloneObjectFailure is the load-bearing classifier gate for this entire recovery path, yet it has no isolated unit tests — only the integration tests exercise it (implicitly, via two specific strings).

If git ever changes its error message format, or if a new pattern is added, there is no fast-failing unit test to catch the regression.

describe('isPartialCloneObjectFailure', () => {
  // Positive cases — should return true
  it.each([
    ['could not fetch OID from promisor remote',
     'fatal: could not fetch 4f0af081abc from promisor remote'],
    ['upload-pack: not our ref',
     'fatal: remote error: upload-pack: not our ref 0035eb55'],
    ['remote error: upload-pack',
     'error: remote error: upload-pack: filter not supported'],
    ['unable to read SHA',
     'fatal: unable to read tree 1234567890abcdef1234567890abcdef12345678'],
    ['fatal: bad object SHA',
     'fatal: bad object 1234567890abcdef1234567890abcdef12345678'],
  ])('returns true for: %s', (_, output) => {
    expect(isPartialCloneObjectFailure(output)).toBe(true);
  });

  // Negative cases — should return false
  it.each([
    ['merge conflict', 'CONFLICT (content): Merge conflict in file.txt'],
    ['empty string', ''],
    ['null', null],
  ])('returns false for: %s', (_, output) => {
    expect(isPartialCloneObjectFailure(output)).toBe(false);
  });
});

[/tdd] The new tests cover two of the three branches in the recovery flow, but the third branch — recovered === true yet the retry rebase still fails (lines 460-471 of push_signed_commits.cjs) — is untested. This path produces a distinct error message ("even after backfilling") and has its own rebase --abort call.

it('should throw "even after backfilling" when backfill succeeds but retry rebase also fails', async () => {
  // ... set up workDir with a conflicting change ...
  let rebaseAttempts = 0;
  global.exec = {
    getExecOutput: async (program, args, opts = {}) => {
      if (program === 'git' && args[0] === 'rebase' && args[1] === '--onto') {
        rebaseAttempts++;
        // First attempt: simulate promisor failure
        if (rebaseAttempts === 1) {
          return { exitCode: 128, stdout: '', stderr: 'fatal: could not fetch 0035eb55... from promisor remote' };
        }
        // Second attempt: simulate a conflict (or another failure)
        return { exitCode: 1, stdout: 'CONFLICT (content): Merge conflict in file.txt', stderr: '' };
      }
      if (program === 'git' && args[0] === 'fetch' && args.includes('--no-filter')) {
        return { exitCode: 0, stdout: '', stderr: '' };
      }
      return realExec.getExecOutput(program, args, opts);
    },
    exec: realExec.exec,
  };

  await expect(pushSignedCommits({ ... }))
    .rejects.toThrow('even after backfilling the required commit objects');
  expect(rebaseAttempts).toBe(2);
});

[/diagnose] The fetchTargets filter uses sha.length > 0 while backfillCommitObjects re-validates to /^[0-9a-f]{40}$/i — two different gates for the same invariant.

If headOid ever resolves to an abbreviated SHA (or any non-40-char string), the outer filter passes it through but the inner one discards it silently. If ALL targets end up discarded, backfillCommitObjects returns false with no log and the caller throws a misleading "could not be backfilled" error.

// before
const fetchTargets = [firstGraphqlParentOid, firstReplayParentOid, headOid]
  .filter(sha => typeof sha === "string" && sha.length > 0);

// after
const fetchTargets = [firstGraphqlParentOid, firstReplayParentOid, headOid]
  .filter(sha => typeof sha === "string" && OID_PATTERN.test(sha));

[/diagnose] When targets.length === 0, the function returns false silently with no log entry. The caller in push_signed_commits.cjs then throws a "could not be backfilled" error with no indication that the real problem was an empty target list (e.g. all SHAs failed the 40-char hex validation).

  if (targets.length === 0) {
    core.warning('backfillCommitObjects: no valid 40-char SHA targets provided; targeted fetch skipped');
    return false;
  }

[/diagnose] There is no log entry on the success path — when the targeted fetch completes with exitCode === 0 the function returns true silently. This makes it hard to confirm in run logs that the backfill actually happened vs. that the recovery path was never triggered.

    if (exitCode !== 0) {
      core.warning(`backfillCommitObjects: targeted fetch exited with code ${exitCode}: ${String(stderr || '').trim()}`);
    } else {
      core.info(`backfillCommitObjects: successfully fetched object content for ${targets.length} anchor commit(s) from origin`);
    }
    return exitCode === 0;

[/zoom-out] Threading gitAuthEnv into the rebase exec looks like a secondary but independent correctness fix — the original code passed no env override, so in private repos where checkout credentials are scrubbed before this point, the rebase's own promisor lazy-fetches would have failed with auth errors that none of the five classifier patterns would match. Worth calling out explicitly in the PR description (and perhaps with a separate commit) so reviewers understand the two orthogonal improvements: (1) recovery from promisor object failures, and (2) ensuring the rebase step itself has auth context for promisor fetches.

No code change needed — just a documentation/narrative suggestion.

[/tdd] global.exec is overwritten directly and never restored. If an earlier assertion in this test throws (e.g. the pushSignedCommits call rejects unexpectedly), the global remains polluted for subsequent tests in the same suite.

const originalExec = global.exec;
try {
  global.exec = { getExecOutput: ..., exec: realExec.exec };
  // ... test body ...
} finally {
  global.exec = originalExec;
}

[/tdd] The conflict test verifies backfillAttempted === false for --no-filter, --unshallow, and --deepen fetch variants, which is solid. But the test doesn't directly verify that isPartialCloneObjectFailure returns false for real conflict output — it only proves the integrated behaviour works for this one scenario.

If a future pattern addition to isPartialCloneObjectFailure inadvertently matches merge-conflict text, the integration test would catch it, but only for this specific conflict message. A dedicated unit test for the classifier (see the comment on line 37) would catch regressions more precisely and without needing a full integration run.