fix(action): add create-only signed replay

[gmeszaros]

Add opt-in create-only signed replay. Existing destinations and concurrent branch-name collisions fail without updating the remote ref; existing callers retain their current behavior. Authenticate signed-commit fetches with a command-scoped credential helper and stop on fetch failure.

Validation: 235 tests passed; yamllint, ShellCheck, Ruff lint/format and git diff --check passed.

Live GitHub App signed-replay integration remains unverified.

[gmeszaros]

@dobbyphus review

[dobbyphus]

Code Review

Summary

The opt-in replay_new_branch_only policy is built on the right primitive — atomic POST /git/refs — and defaults to false, so existing callers keep their behavior. The second commit's fetch-authentication change is inert under this repository's own documented actions/checkout setup, which is the one area the PR body flags as unverified.

Validation I ran

On origin/fix/replay-new-branch-only in a clean worktree:

Check	Result
pytest tests/ -q	235 passed (main baseline: 227)
ruff check scripts/ tests/	All checks passed
ruff format scripts/ tests/ --check	23 files already formatted
yamllint .	clean
shellcheck scripts/*.sh	clean
git diff --check origin/main origin/fix/replay-new-branch-only	clean

The PR body's validation claims reproduce exactly.

---

1. BLOCKER — scripts/replay_commits.py:259-267: the credential helper never authenticates the fetch when actions/checkout persists credentials

-c credential.helper= resets the helper list. It does not clear http.<url>.extraheader, which actions/checkout writes by default. I confirmed http.https://github.com/.extraheader is present in this repository's checked-out workspace, and examples/agent.yaml:61 uses actions/checkout@v6 with default persist-credentials. No file in the repo sets persist-credentials: false.

I reproduced the precedence against a local HTTP server that logs Authorization and answers 401 WWW-Authenticate: Basic:

Case	git config	Header actually sent
A — this PR	extraheader + -c credential.helper= + -c credential.helper=!gh auth git-credential	basic EXTRAHEADER_TOKEN on the initial request and on the post-401 retry
B	no extraheader + helper	Basic x-access-token:GH_CLI_TOKEN — helper used
C — pre-PR	extraheader only	basic EXTRAHEADER_TOKEN
D	extraheader + -c http.<url>.extraheader= + helper	Basic x-access-token:GH_CLI_TOKEN — helper used

Case A is exactly what this PR ships. The helper's credentials never leave the client; git re-sends the checkout extraheader on the retry. Fetch authentication is byte-identical to case C, the pre-PR behavior. The commit fix(scripts): authenticate signed commit fetch therefore changes nothing about authentication on the documented path.

Fix — clear the URL-scoped extraheader in the same command, which is case D and the only variant I observed reaching the server with the gh token:

server = os.environ.get("GITHUB_SERVER_URL", "https://github.com").rstrip("/") + "/"
git(
    "-c", f"http.{server}.extraheader=",
    "-c", "credential.helper=",
    "-c", "credential.helper=!gh auth git-credential",
    "fetch", "origin", commit_sha,
)

The alternative is to drop the helper flags entirely and document that signed-commit fetches authenticate through checkout's persisted token. Either resolves the finding; shipping the current form advertises a guarantee the code does not provide.

The check=True half of this hunk is a genuine improvement and is not affected: a failed fetch previously died one line later at git reset --hard, so surfacing it at the fetch is strictly clearer.

2. NON-BLOCKER — tests/test_replay_commits.py:82-104: test_fetch_auth_and_failure asserts argv, not authentication

The assertion restates the argument tuple from scripts/replay_commits.py:259-267. It passes in case A above, where zero authentication occurs, so it certifies the guarantee described in finding 1 without testing it. The failure half of the test — the pytest.raises at line 99 and the reset --hard absence check at lines 101-103 — is sound and is worth keeping as-is.

3. NON-BLOCKER — scripts/replay_commits.py:518-522: main() raises instead of returning 1 on a replay-window collision

With new_branch_only=true and a branch claimed during replay, create_ref (scripts/replay_commits.py:222-227) propagates subprocess.CalledProcessError out of main(). The module contract is sys.exit(main()) at scripts/replay_commits.py:519-520, and every other failure path returns 1 with a message (:445-446, :487-488). tests/test_replay_commits.py:55 encodes the traceback as the expected outcome.

The safety property holds — the remote ref is left untouched, which is the point of the POST. Only the exit path needs work: wrap the create_ref call in try/except subprocess.CalledProcessError, print the branch-collision message, and return 1. The test then asserts main() == 1 rather than a traceback.

4. NON-BLOCKER — scripts/replay_commits.py:14-16: fetch failures surface without git's stderr

run() sets capture_output=True, and an uncaught CalledProcessError traceback prints only the exit status. Now that the fetch at :259-267 propagates, an operator reading the Actions log sees returned non-zero exit status 128 and no git message — which is precisely the diagnostic needed to distinguish an auth failure from an unadvertised-object failure. Catch the error at the fetch site and print exc.stderr before re-raising.

5. NON-BLOCKER — tests/test_replay_commits.py:68-73: test_default_preserves_existing_branch_mode asserts on an unreachable state

The test sets branch_exists_on_remote to True while the fixture leaves get_remote_branch_sha at None (tests/test_replay_commits.py:23-24). Both functions read the same endpoint, repos/{repo}/git/refs/heads/{branch} (scripts/replay_commits.py:85-96 and :99-109), so they do not disagree in a real run.

As written, the test named "preserves existing branch mode" skips the entire existing-branch path it is meant to guard: the fetch, filter_new_commits, and replay_base selection at scripts/replay_commits.py:471-481. Set get_remote_branch_sha to a 40-character sha in this test so the PATCH assertion runs against the real default flow.

6. NON-BLOCKER — action.yaml:102-106 and action.yaml:710 have no wiring test

tests/test_install_inputs.py:52-58 (test_action_exposes_opencode_print_logs_input) establishes the repo pattern of asserting both the input declaration and the env passthrough for each new input. Add the equivalent assertions for replay_new_branch_only: and REPLAY_NEW_BRANCH_ONLY: ${{ inputs.replay_new_branch_only }}. Without it, a rename or a dropped env: line in action.yaml leaves all eight new tests green while the input silently stops reaching the script.

7. NON-BLOCKER — README.md:161 contradicts README.md:98

README.md:161 states | PR comment | agent | Commit to PR branch | Push to PR branch |. With replay_new_branch_only: true, that trigger exits 1 at scripts/replay_commits.py:486-488, the "Replay commits as signed" step fails, and the agent's local commits are discarded unpushed. The new input row at README.md:98 documents the ref-level behavior but does not state that PR-comment and existing-branch runs stop working when the flag is on.

---

What is correct

• Default "false" at action.yaml:105 preserves the existing flow end to end; tests/test_replay_commits.py:68-73 guards the PATCH branch.
• Policy parsing fails closed on any unrecognized value (scripts/replay_commits.py:443-447), covered for "", "tru", and "FALSE".
• is_new_branch = new_branch_only or not branch_exists_on_remote(...) at scripts/replay_commits.py:518 short-circuits the pre-check and delegates collision detection to the 422 from POST /git/refs. That is the correct primitive for the atomicity claim, and tests/test_replay_commits.py:52-59 verifies no PATCH is ever issued in that window.
• The early return at scripts/replay_commits.py:486-488 runs before any git or replay_commit call, so a pre-existing branch costs no local mutation and no API writes. tests/test_replay_commits.py:45-50 asserts exactly that.

Verdict

REQUEST_CHANGES — finding 1 must be resolved before merge, since the PR ships a stated authentication fix that does not take effect under the setup this repository documents. The create-only replay policy itself is sound and, once findings 3-7 are addressed, ready.

[jkoelker]

Idea wise LGTM, when dobbyphus is happy I'm happy

[gmeszaros]

@dobbyphus review

[dobbyphus]

Code Review

Summary

d6e01eb resolves the BLOCKER from my previous review and adds a real HTTP-level regression test for it. It also rewrites the signed-replay tree builder to read from the git index instead of the working tree, which fixes seven pre-existing correctness and data-exposure defects that were not part of the original scope. No BLOCKER remains. Findings 3-7 from my previous review are still open and are all NON-BLOCKER.

Validation I ran

Clean worktree at origin/fix/replay-new-branch-only, env -i to isolate from ambient CI variables:

Check	Result
pytest tests/ -q	239 passed (main baseline: 227)
ruff check scripts/ tests/	All checks passed
ruff format scripts/ tests/ --check	24 files already formatted
yamllint .	clean
shellcheck scripts/*.sh	clean
git diff --check origin/main...HEAD	clean

Under my runner's ambient environment tests/test_run_script.py::test_print_logs_dumps_omo_log_file and ::test_runner_debug_enables_print_logs fail because a PROMPT-shaped variable leaks into scripts/run.sh. Both fail identically on origin/main and both pass under env -i on both refs. Pre-existing, environment-induced, unrelated to this PR.

---

Previous BLOCKER — resolved and verified

scripts/replay_commits.py:264-275 now clears the URL-scoped extraheader alongside the helper reset. I verified this is a real fix rather than a cosmetic one by deleting only the two http.{server}.extraheader= argv elements and re-running tests/test_replay_transport.py:

Source state	test_replay_fetch_authentication[False]	[True] (checkout persisted extraheader)
Mutated (extraheader clearing removed — reproduces 6615c0c)	passed	failed with CalledProcessError
PR head as shipped	passed	passed

tests/test_replay_transport.py:16 stands up a real ThreadingHTTPServer, answers 401 WWW-Authenticate: Basic on any non-matching Authorization, and asserts the gh helper credential is the only value that reaches the server. That answers my previous finding 2 as well: the guarantee is now tested at the transport layer, not restated as an argv tuple. The [True] parametrization also asserts the persisted http.<url>.extraheader config value survives the fetch, so the workaround does not corrupt the checkout state for later steps.

The tree-builder rewrite is a strict improvement

scripts/replay_commits.py:162-166, :169-178, and :181-215 moved blob content and file modes from the working tree to the index. I ran the main implementation and the PR implementation side by side against real git repositories with gh_api stubbed to capture exactly what each would publish:

Scenario	main publishes	PR head publishes
Staged symlink	target file's contents as mode 100644	link target string as mode 120000
Unstaged edit after git add	working-tree bytes	staged bytes
git mv old new	new only — old survives in base_tree	new plus a null-sha delete for old
café.txt	"caf\303\251.txt" as a delete; real file dropped	café.txt with contents
Newline in filename	quoted path as a delete	correct path with contents
Exec bit staged, stripped in worktree	100644	100755
git rm --cached	file republished	delete
Staged delete	delete	delete (unchanged)
Submodule gitlink	IsADirectoryError	ValueError: Unsupported index entry: sub

The symlink row is the one worth calling out: on main, staging any symlink published the contents of whatever it pointed at into a signed commit on the remote branch, with the symlink flattened to a regular file. scripts/replay_commits.py:195 now rejects any mode outside 100644/100755/120000 and :197 reads the blob by object sha, so target contents are never read. tests/test_replay_commits.py:112 locks this with a DO_NOT_PUBLISH marker file and asserts it never appears in any blob.

Three details in that rewrite are correct and non-obvious: --no-renames at :164 is required because diff.renames defaults to on and --name-only would otherwise emit only the destination path; -z at :164 and :187 is required because core.quotePath mangles non-ASCII paths and str.split("\n") breaks on newlines in filenames; and the path != file_path guard at :192-193 is required because a pathspec naming a directory matches everything beneath it. tests/test_replay_commits.py:112 covers the last one via the "dir" entry and covers the second via the " link\n\r" parametrization.

---

1. NON-BLOCKER — scripts/replay_commits.py:526-530: main() raises instead of returning 1 on a replay-window collision

Unchanged from my previous review. With new_branch_only=true and the branch claimed during replay, create_ref (:227-232) propagates subprocess.CalledProcessError out of main(). The module contract is sys.exit(main()) at :545-546, and every other failure path returns 1 with a message (:452-454, :494-496). I confirmed the current behavior directly: calling main() with gh_api raising on the POST exits through the exception, not through a return value. tests/test_replay_commits.py:57 encodes the traceback as the expected outcome.

The raise ValueError at :196 reaches the same exit path for a submodule change. Wrap both in try/except, print the reason, and return 1.

The safety property is intact either way — the remote ref is untouched, which is the point of the POST.

2. NON-BLOCKER — scripts/replay_commits.py:14-16: fetch failures surface without git's stderr

Unchanged from my previous review, and now more relevant because :265 propagates. run() sets capture_output=True. I ran the exact fetch argv from :265-275 against an unreachable remote; the operator-visible output ends at:

subprocess.CalledProcessError: Command '['git', '-c', 'http.https://github.com/.extraheader=', ...]' returned non-zero exit status 128.

git's own message is captured and discarded, which is precisely the text needed to separate an auth failure from an unadvertised-object failure. Catch at the fetch site and print exc.stderr before re-raising.

3. NON-BLOCKER — tests/test_replay_commits.py:70: test_default_preserves_existing_branch_mode asserts on an unreachable state

Unchanged from my previous review. The test sets branch_exists_on_remote to True while the fixture leaves get_remote_branch_sha at None (tests/test_replay_commits.py:25). Both read repos/{repo}/git/refs/heads/{branch} (scripts/replay_commits.py:85-96 and :99-109), so they do not disagree in a real run.

I ran the reachable variant — get_remote_branch_sha returning a 40-char sha, filter_new_commits stubbed. It passes, returns 0, still asserts PATCH, and additionally exercises git fetch origin feature, filter_new_commits, and the rev-parse {commits[0]}^ base selection at scripts/replay_commits.py:497-506. As currently written the test reaches none of those, so the branch it is named for is unguarded. Setting a real sha is a one-line change that costs nothing and covers the default path.

4. NON-BLOCKER — action.yaml:102-106 and action.yaml:710 have no wiring test

Unchanged from my previous review. grep over tests/ finds REPLAY_NEW_BRANCH_ONLY only at tests/test_replay_commits.py:19, :65, and :71, all of which are monkeypatch.setenv calls against the script. Nothing asserts action.yaml declares the input or passes it through. tests/test_install_inputs.py:54 (test_action_exposes_opencode_print_logs_input) is the established pattern. Without the equivalent assertions for replay_new_branch_only: and REPLAY_NEW_BRANCH_ONLY: ${{ inputs.replay_new_branch_only }}, a dropped env: line leaves every new test green while the input silently stops reaching the script.

5. NON-BLOCKER — README.md:161 contradicts README.md:98

Unchanged from my previous review. README.md:161 states | PR comment | agent | Commit to PR branch | Push to PR branch |. With replay_new_branch_only: true that trigger returns 1 at scripts/replay_commits.py:494-496, the replay step fails, and the agent's local commits are discarded unpushed. README.md:98 documents the ref-level behavior without stating that PR-comment and existing-branch runs stop working when the flag is on.

6. NON-BLOCKER — the PR body omits the security-relevant half of d6e01eb

The body describes only the create-only policy and the fetch authentication. The symlink-content, file-mode, rename, and path-encoding fixes documented in the table above are the larger behavioral change in this PR and are invisible to anyone reading the PR description or the merge commit. Add them to the body so the fix is discoverable later.

7. NON-BLOCKER — scripts/replay_commits.py:186-188 inherits main's working-directory assumption

git diff --cached --name-only at :164 emits repository-root-relative paths; the :(literal) pathspec at :187 resolves relative to the process working directory. I ran both implementations with the working directory set to a subdirectory of the repository: main and PR head both emit a null-sha delete for a file that was actually added, so this is not a regression. grep -n "working-directory" action.yaml returns nothing, so the shipped step runs at GITHUB_WORKSPACE and this is not reachable today. Recording it because the rewrite touched exactly this function; --full-name on the ls-files call closes it.

---

Verdict

APPROVE. The BLOCKER is fixed and is now covered by a test I confirmed fails without the fix, and the index-based tree builder corrects real data-exposure and data-loss defects that existed on main. Findings 1-7 above are all NON-BLOCKER and suit a follow-up; findings 3 and 4 are one-line changes that would close the two coverage holes in this PR's own tests.