Git integration follow-up

[JanJakes]

Tracking follow-up feedback for the Git integration PR series. I scanned the visible stack context around #11, #12, #13, #14, #16-#26, #28, and #29; this issue should collect findings by PR as review continues.

Some items below appear to be addressed by later stacked PRs, but they are still listed under #11 because they are observable in #11 as submitted.

PR #11: Allow Git pushes to create COW branches

Review target: #11 / 9423bb45c6e6c9a8c12ef7d2da69a9b68d4c0388.

Findings

1. Restore Git refs and branch storage on post-receive failures.

   GitEndpoint mutates the persistent Git refs before COW materialization runs, but the general post-receive failure path only returns HTTP 500:

   forkpress/scripts/git_server/cow_server.php

   Lines 80 to 112 in 9423bb4

   	$request_bytes = file_get_contents('php://input');
   	$response = new StreamingResponseWriter();
   	try {
   	$endpoint = new GitEndpoint($repo);
   	$endpoint->handle_request($endpoint_path, $request_bytes, $response);
   	} catch (\Throwable $e) {
   	error_log("COW git server error: " . $e->getMessage() . "\n" . $e->getTraceAsString());
   	throw $e;
   	}
   	if ($is_post_receive) {
   	try {
   	cow_git_apply_all_refs_to_branches(
   	$repo,
   	$git_repo_dir,
   	$branches_dir,
   	$storage_branches_dir,
   	$branch_list_path,
   	$file_view,
   	$debug_log,
   	$pre_receive_refs
   	);
   	} catch (\Throwable $e) {
   	if (ob_get_level() > 0) {
   	ob_end_clean();
   	}
   	error_log("COW push processing error: " . $e->getMessage() . "\n" . $e->getTraceAsString());
   	http_response_code(500);
   	header('Content-Type: text/plain');
   	$short = substr(str_replace(["\n", "\r"], ' ', $e->getMessage()), 0, 500);
   	echo "forkpress cow: push rejected: $short\n";
   	return;
   	}

   The only ref restore path is the unsupported-name branch:

   forkpress/scripts/git_server/cow_server.php

   Lines 265 to 266 in 9423bb4

   	if ($unsupported) {
   	cow_git_restore_head_refs($git_repo_dir, $pre_receive_refs, $current_refs);

   For valid branch names, any failure in source inference, COW clone, apply, publish, config rewrite, or branches.txt write leaves the pushed ref in .forkpress/cow/git. After rename($tmp, $dest_storage), failures can also leave published storage or a public link because the catch block only removes $tmp:

   forkpress/scripts/git_server/cow_server.php

   Lines 367 to 384 in 9423bb4

   	if (!rename($tmp, $dest_storage)) {
   	throw new \RuntimeException("failed to publish git-created branch '$branch'");
   	}
   	if (cow_git_normalize_path($dest_storage) !== cow_git_normalize_path($dest_public)) {
   	if (!symlink($dest_storage, $dest_public)) {
   	cow_git_remove_tree($dest_storage);
   	throw new \RuntimeException("failed to link git-created branch '$branch' into public branch directory");
   	}
   	}
   	cow_git_rewrite_wp_config($dest_public, $debug_log);
   	cow_git_write_branch_list($branches_dir, $branch_list_path);
   	error_log("ForkPress COW git created branch '$branch' from '$source'");
   	return $dest_public;
   	} catch (\Throwable $e) {
   	cow_git_remove_tree($tmp);
   	throw $e;

   Expected follow-up: restore pre_receive_refs for any post-receive apply failure, and roll back any published storage/public branch paths after the staging directory has been renamed. Add tests for a valid branch push that fails after the Git ref is written and after branch storage is published.

2. Resync the Git view before returning a successful push.

   The new e2e coverage edits database.sql during Git-created branch creation, but only verifies that the runtime branch did not materialize database.sql:

   forkpress/tests/test_cow_strategy_e2e.sh

   Lines 97 to 105 in 9423bb4

   	git -C "$TMP/checkout" checkout -B git-created origin/main
   	printf "created through git\n" > "$TMP/checkout/wordpress/wp-content/git-created.txt"
   	printf "\n-- ignored git-created database.sql edit\n" >> "$TMP/checkout/database.sql"
   	"$BIN" commit "$TMP/checkout" --message "create cow branch through git"
   	test -f "$WORK/git-created/wp-load.php"
   	test -f "$WORK/git-created/wp-content/git-created.txt"
   	grep -F "created through git" "$WORK/git-created/wp-content/git-created.txt" >/dev/null
   	test ! -e "$WORK/main/wp-content/git-created.txt"
   	test ! -e "$WORK/git-created/database.sql"

   In [codex] Allow Git pushes to create COW branches #11, post-receive applies the WordPress files and then flushes the buffered receive-pack response without regenerating the COW Git snapshot:

   forkpress/scripts/git_server/cow_server.php

   Lines 90 to 115 in 9423bb4

   	if ($is_post_receive) {
   	try {
   	cow_git_apply_all_refs_to_branches(
   	$repo,
   	$git_repo_dir,
   	$branches_dir,
   	$storage_branches_dir,
   	$branch_list_path,
   	$file_view,
   	$debug_log,
   	$pre_receive_refs
   	);
   	} catch (\Throwable $e) {
   	if (ob_get_level() > 0) {
   	ob_end_clean();
   	}
   	error_log("COW push processing error: " . $e->getMessage() . "\n" . $e->getTraceAsString());
   	http_response_code(500);
   	header('Content-Type: text/plain');
   	$short = substr(str_replace(["\n", "\r"], ' ', $e->getMessage()), 0, 500);
   	echo "forkpress cow: push rejected: $short\n";
   	return;
   	}
   	if (ob_get_level() > 0) {
   	ob_end_flush();
   	}

   That means the remote ref can temporarily point at the client-pushed commit containing ignored database.sql edits, while the materialized preview ignored those edits. A later fetch may normalize the ref, but immediately after forkpress commit the client and server-visible snapshot can disagree.

   Expected follow-up: after a successful apply, resnapshot the affected branches and update refs before returning success. Add coverage that the remote database.sql is regenerated, ignored paths disappear from the Git view immediately, and the checkout is not left dirty/diverged after forkpress commit.

3. Coordinate Git-created branch mutations with the COW operation lock.

   [codex] Allow Git pushes to create COW branches #11 protects .forkpress/cow/git with git.lock, but branch creation also mutates materialized branch storage, public branch links, wp-config.php, and branches.txt:

   forkpress/scripts/git_server/cow_server.php

   Lines 46 to 58 in 9423bb4

   	cow_git_mkdir(dirname($git_repo_dir));
   	$lock_path = $git_repo_dir . '.lock';
   	$lock = fopen($lock_path, 'c');
   	if (!$lock) {
   	http_response_code(500);
   	echo "Cannot open COW git lock\n";
   	return;
   	}
   	if (!flock($lock, LOCK_EX)) {
   	http_response_code(500);
   	echo "Cannot lock COW git store\n";
   	return;
   	}

   and

   forkpress/scripts/git_server/cow_server.php

   Lines 367 to 379 in 9423bb4

   	if (!rename($tmp, $dest_storage)) {
   	throw new \RuntimeException("failed to publish git-created branch '$branch'");
   	}
   	if (cow_git_normalize_path($dest_storage) !== cow_git_normalize_path($dest_public)) {
   	if (!symlink($dest_storage, $dest_public)) {
   	cow_git_remove_tree($dest_storage);
   	throw new \RuntimeException("failed to link git-created branch '$branch' into public branch directory");
   	}
   	}
   	cow_git_rewrite_wp_config($dest_public, $debug_log);
   	cow_git_write_branch_list($branches_dir, $branch_list_path);

   Those mutations need to be serialized with other COW branch lifecycle operations and with request-side reads of materialized branch trees. Otherwise a Git push can race forkpress branch create, agents setup, reset/delete follow-ups, or a preview request and expose half-published state.

   Expected follow-up: put Git apply/create under the same COW branch operation lock used by the CLI/runtime branch lifecycle paths, and cover a blocked request or concurrent branch operation in tests.

4. Treat file apply failures as hard errors before publishing/listing a branch.

   cow_git_apply_wp_files() ignores the return values from file_put_contents() and unlink():

   forkpress/scripts/git_server/cow_server.php

   Lines 635 to 650 in 9423bb4

   	function cow_git_apply_wp_files(GitRepository $repo, string $branch_root, array $wp_files): void {
   	foreach ($wp_files as $rel => $blob_hash) {
   	$target = $branch_root . '/' . $rel;
   	if (is_file($target) && cow_git_file_blob_hash($target) === $blob_hash) {
   	continue;
   	}
   	$parent = dirname($target);
   	cow_git_mkdir($parent);
   	file_put_contents($target, $repo->read_object($blob_hash)->consume_all());
   	}
   	$current = cow_git_collect_branch_files($branch_root);
   	foreach ($current as $rel => $path) {
   	if (!isset($wp_files[$rel]) && is_file($path)) {
   	unlink($path);
   	}

   PHP warnings do not make this path fail reliably. During Git-created branch creation, a failed write into the staging tree can still lead to publishing the branch and rewriting branches.txt, producing an incomplete preview branch that Git reported as pushed successfully.

   Expected follow-up: check write/delete results, throw with path context, and keep the branch unpublished on any apply error. Add tests for unwritable files or path-shape conflicts during apply.

5. Use the same branch-name reservations as the other Git/branch creation paths.

   COW Git-created branches accept any single-label regex match:

   forkpress/scripts/git_server/cow_server.php

   Lines 837 to 839 in 9423bb4

   	function cow_git_valid_branch_name(string $branch): bool {
   	return (bool)preg_match('/^[A-Za-z0-9_-]{1,63}$/', $branch);
   	}

   Other integration paths already reserve names such as www, admin, api, mail, localhost, and wp because they conflict with routing:

   forkpress/scripts/git_server/server.php

   Lines 487 to 492 in 9423bb4

   	// Reject reserved names that would conflict with HTTP routing.
   	if (in_array(strtolower($branch_name), $reserved, true)) {
   	throw new \RuntimeException(
   	"refusing to push to reserved branch name '$branch_name' (conflicts with routing)"
   	);
   	}

   Expected follow-up: centralize or mirror the reservation check for COW Git-created branches, and make sure COW CLI branch creation follows the same rule if it does not already.

Open Question

• Source inference selects an existing ForkPress branch from Git history, then clones that branch's current live storage. If a user fetched origin/main, the source branch's database changed, and then the user pushed a new branch based on the older fetched commit, [codex] Allow Git pushes to create COW branches #11 will apply the older pushed WordPress tree onto a clone of the current live database. Should Git-created branches require the pushed fork point to be the current source tip, or should the stale-base/database behavior be documented explicitly?

Verification Run

On the PR #11 head worktree:

• php -l scripts/git_server/cow_server.php
• php -l runtime/router_cow.php
• bash -n tests/test_cow_strategy_e2e.sh
• git diff --check aa9d633d335a40901f67cac0993af14461e059d1..HEAD
• FORKPRESS_RUNTIME_BUNDLE=/dev/null cargo test -p forkpress (28 passed)

Not run for this review pass: make test-all, musl release build, or the full COW e2e script.

[JanJakes]

PR #12: Add COW branch reset

Review target: #12 / 9fc8a0b5649a16fecfaed8def7f54b40cd8b4aa0.

This PR addresses part of the #11 locking feedback by putting COW CLI branch creation, reset/delete, agents branch creation, forkpress git branch create, and the PHP COW Git adapter under .forkpress/cow/operations.lock.

Findings

1. Rollback can silently fail while reporting that the previous branch was restored.

   The reset publish error path ignores failures while moving the newly-published tree aside, deleting it, and restoring the backup:

   forkpress/forkpress/src/main.rs

   Lines 4106 to 4124 in 9fc8a0b

   	if let Err(err) = publish {
   	let failed = unique_cow_operation_dir(parent, "reset-failed", branch);
   	if staging_published && path_exists_no_follow(&target) {
   	match fs::rename(&target, &failed) {
   	Ok(()) => {}
   	Err(_) => {
   	let _ = fs::remove_dir_all(&target);
   	}
   	}
   	}
   	if target_moved_to_backup
   	&& path_exists_no_follow(&backup)
   	&& !path_exists_no_follow(&target)
   	{
   	let _ = fs::rename(&backup, &target);
   	}
   	let _ = fs::remove_dir_all(&failed);
   	let _ = fs::remove_dir_all(&staging);
   	return Err(err).context("failed to reset COW branch; restored previous branch contents");

   If fs::rename(&target, &failed) fails and remove_dir_all(&target) also fails, path_exists_no_follow(&target) remains true, so the backup is not renamed back. The returned error still says restored previous branch contents, which can be false. Likewise, if fs::rename(&backup, &target) fails, that failure is discarded.

   Expected follow-up: track rollback failures explicitly and return an error that tells the user whether the target was restored, where the backup remains, and where any failed published tree remains. Add failure-injection coverage for publish failure and rollback failure, not just the happy path.

2. Reset does not refresh the COW Git snapshot/ref after replacing the branch.

   reset_cow_branch() replaces the materialized branch and rewrites branches.txt, then returns:

   forkpress/forkpress/src/main.rs

   Lines 4082 to 4135 in 9fc8a0b

   	let publish = (|| -> Result<()> {
   	fs::rename(&target, &backup).with_context(|| {
   	format!(
   	"failed to move current branch {} to {}",
   	target.display(),
   	backup.display()
   	)
   	})?;
   	target_moved_to_backup = true;
   	fs::rename(&staging, &target).with_context(|| {
   	format!(
   	"failed to publish reset branch {} to {}",
   	staging.display(),
   	target.display()
   	)
   	})?;
   	staging_published = true;
   	let public_root = ensure_cow_public_branch_root(layout, branch, &target, file_view)?;
   	run_cow_bootstrap_script(layout, runtime, shared, &public_root, "ForkPress", "admin")?;
   	write_cow_branch_list(layout)?;
   	Ok(())
   	})();
   	if let Err(err) = publish {
   	let failed = unique_cow_operation_dir(parent, "reset-failed", branch);
   	if staging_published && path_exists_no_follow(&target) {
   	match fs::rename(&target, &failed) {
   	Ok(()) => {}
   	Err(_) => {
   	let _ = fs::remove_dir_all(&target);
   	}
   	}
   	}
   	if target_moved_to_backup
   	&& path_exists_no_follow(&backup)
   	&& !path_exists_no_follow(&target)
   	{
   	let _ = fs::rename(&backup, &target);
   	}
   	let _ = fs::remove_dir_all(&failed);
   	let _ = fs::remove_dir_all(&staging);
   	return Err(err).context("failed to reset COW branch; restored previous branch contents");
   	}
   	if let Err(err) = fs::remove_dir_all(&backup) {
   	eprintln!(
   	"forkpress: warning: reset succeeded but failed to remove old branch backup {}: {err}",
   	backup.display()
   	);
   	}
   	println!("forkpress: reset COW branch '{branch}' from '{from}'");
   	Ok(())

   The Git view is regenerated by cow_git_sync_repository() on the next smart-HTTP request, so ordinary fetch/push paths should eventually self-heal. However, immediately after forkpress branch reset, .forkpress/cow/git/refs/heads/<branch> can still point at the pre-reset snapshot. Since this series is tightening Git integration semantics, reset should either resnapshot the affected branch immediately or deliberately remove/mark the stale ref so diagnostics and any direct internal ref inspection do not report old state.

3. The PHP adapter now takes the exclusive operation lock for all Git smart-HTTP requests.

   cow_git_server_handle() acquires .forkpress/cow/operations.lock before it knows whether the request is upload-pack or receive-pack:

   forkpress/scripts/git_server/cow_server.php

   Lines 46 to 60 in 9fc8a0b

   	cow_git_mkdir(dirname($git_repo_dir));
   	$operation_lock = null;
   	$operation_lock_path = dirname($branch_list_path) . '/operations.lock';
   	$operation_lock = fopen($operation_lock_path, 'c');
   	if (!$operation_lock) {
   	http_response_code(500);
   	echo "Cannot open COW branch operation lock\n";
   	return;
   	}
   	if (!flock($operation_lock, LOCK_EX)) {
   	http_response_code(500);
   	echo "Cannot lock COW branch operations\n";
   	fclose($operation_lock);
   	return;
   	}

   That is conservative and correct for mutation safety, but it also serializes clone/fetch traffic with branch reset/create/delete. If clone/fetch gets slow on larger sites, this will block unrelated branch operations. Consider narrowing the exclusive lock to receive-pack/mutation paths, or using a shared lock for snapshot/read-only Git requests once request-side locking lands.

Test Gap

The added e2e coverage verifies reset success, database replacement through WordPress-visible post state, file replacement, and main protection:

forkpress/tests/test_cow_strategy_e2e.sh

Lines 186 to 208 in 9fc8a0b

	"$BIN" branch --work-dir "$WORK_DIR" create reset-source
	"$BIN" branch --work-dir "$WORK_DIR" create reset-target
	echo "source reset file" > "$WORK/reset-source/wp-content/reset-source.txt"
	echo "target reset file" > "$WORK/reset-target/wp-content/reset-target.txt"
	RESET_SOURCE_TITLE="Reset source $(date +%s)"
	RESET_TARGET_TITLE="Reset target $(date +%s)"
	create_branch_post reset-source "$RESET_SOURCE_TITLE"
	create_branch_post reset-target "$RESET_TARGET_TITLE"
	"$BIN" branch --work-dir "$WORK_DIR" reset reset-target --from reset-source > "$TMP/reset.out"
	grep -F "reset COW branch 'reset-target' from 'reset-source'" "$TMP/reset.out" >/dev/null
	test -f "$WORK/reset-target/wp-content/reset-source.txt"
	test ! -e "$WORK/reset-target/wp-content/reset-target.txt"
	curl -sS -H "Host: reset-target.wp.localhost:$PORT" \
	"http://127.0.0.1:$PORT/wp-admin/edit.php" \
	-o "$TMP/reset-target-edit.html"
	grep -F "$RESET_SOURCE_TITLE" "$TMP/reset-target-edit.html" >/dev/null
	grep -F "$RESET_TARGET_TITLE" "$TMP/reset-target-edit.html" && exit 1
	if "$BIN" branch --work-dir "$WORK_DIR" reset main --from reset-source > "$TMP/reset-main.out" 2>&1; then
	echo "reset main without --force unexpectedly succeeded" >&2
	cat "$TMP/reset-main.out" >&2
	exit 1
	fi
	grep -F "refusing to reset main without --force" "$TMP/reset-main.out" >/dev/null

The riskier code is the staged publish and rollback path, so this PR should also have focused tests around failed staging, failed publish, and failed rollback cleanup.

Verification Run

On the PR #12 head worktree:

• php -l scripts/git_server/cow_server.php
• bash -n tests/test_cow_strategy_e2e.sh
• git diff --check origin/codex/cow-git-create-branches..HEAD
• FORKPRESS_RUNTIME_BUNDLE=/dev/null cargo test -p forkpress (28 passed)

Not run for this review pass: make test-all, musl release build, or the full COW e2e script.

[JanJakes]

PR #13: Delete COW branches from Git

Review target: #13 / a10ea7f6e46a4dbaaa42f05ec1448a3a9d7ab0aa.

This PR adds Git-driven COW branch deletion, rejects multi-ref and unsafe deletes before mutation, adds receive-pack parsing coverage, and extends the COW e2e flow for git push origin --delete <branch>.

Findings

1. Validate the pushed old_oid before deleting the Git ref and COW branch.

   The vendored receive-pack path still has the existing TODO to verify that the command's old_oid matches the current ref tip, then immediately deletes the ref when new_oid is null:

   forkpress/vendor/wordpress-php-toolkit/components/Git/class-gitendpoint.php

   Lines 411 to 428 in a10ea7f

   	$old_oid = $header['old_oid'];
   	// @TODO: Verify the old_oid is the ref_name tip.
   	$new_oid = $header['new_oid'];
   	$ref_name = $header['ref_name'];
   	// Validate ref name.
   	if ( ! preg_match( '|^refs/|', $ref_name ) ) {
   	$git_response->append_error_packet_line( "error invalid ref name: $ref_name\n" );
   	$git_response->append_error_packet_line( '0000' );
   	// @TODO: Throw / catch?
   	return false;
   	}
   	// Handle deletion.
   	if ( Commit::is_null_hash( $new_oid ) ) {
   	$git_response->append_sideband_packet_line( "unpack ok\n" );
   	if ( $this->repository->delete_branch( $ref_name ) ) {

   That is especially risky now that ref deletion cascades into materialized COW branch deletion. A client that fetched an old branch tip can later run git push origin --delete branch and remove a branch that another client already advanced. Git servers should reject the command unless old_oid still matches the advertised/current ref, except for an explicit force path if we decide to support one.

   Expected follow-up: check old_oid against the current ref before delete_branch() and before COW tree deletion, and add coverage for stale delete commands that must leave both the Git ref and branch directory intact.

2. Keep branch deletes staged until the entire post-receive apply succeeds.

   cow_git_delete_removed_branches() stages branch paths, rewrites branches.txt, then immediately discards the staged backups:

   forkpress/scripts/git_server/cow_server.php

   Lines 431 to 452 in a10ea7f

   	$staged = [];
   	try {
   	foreach ($pre_receive_refs as $branch => $_old_tip) {
   	if (!cow_git_valid_branch_name($branch) || array_key_exists($branch, $current_refs)) {
   	continue;
   	}
   	$staged[$branch] = cow_git_stage_delete_branch_tree($branches_dir, $storage_branches_dir, $branch);
   	}
   	if (!$staged) {
   	return;
   	}
   	cow_git_write_branch_list($branches_dir, $branch_list_path);
   	} catch (\Throwable $e) {
   	cow_git_restore_staged_branch_deletes($staged);
   	throw $e;
   	}
   	foreach ($staged as $branch => $entries) {
   	cow_git_discard_staged_branch_delete($entries);
   	error_log("ForkPress COW git deleted branch '$branch'");

   After that, cow_git_apply_all_refs_to_branches() still continues into the normal apply loop for every remaining ref:

   forkpress/scripts/git_server/cow_server.php

   Lines 349 to 409 in a10ea7f

   	cow_git_reject_unsupported_head_refs($git_repo_dir, $pre_receive_refs);
   	$current_refs = cow_git_capture_head_refs($git_repo_dir);
   	cow_git_delete_removed_branches(
   	$git_repo_dir,
   	$branches_dir,
   	$storage_branches_dir,
   	$branch_list_path,
   	$pre_receive_refs,
   	$current_refs
   	);
   	$existing_branches = cow_git_branch_names($branches_dir);
   	foreach (scandir($refs_dir) as $name) {
   	if ($name === '.' || $name === '..') {
   	continue;
   	}
   	if (!cow_git_valid_branch_name($name)) {
   	throw new \RuntimeException("refusing to apply invalid branch name '$name'");
   	}
   	$ref_path = $refs_dir . '/' . $name;
   	if (!is_file($ref_path)) {
   	throw new \RuntimeException("refusing to apply nested branch ref '$name'");
   	}
   	$tip = trim((string)file_get_contents($ref_path));
   	if ($tip === '' || Commit::is_null_hash($tip)) {
   	continue;
   	}
   	$all_files = [];
   	cow_git_walk_tree($repo, $repo->read_object($tip)->as_commit()->tree, '', $all_files);
   	$wp_files = [];
   	foreach ($all_files as $path => $blob_hash) {
   	if (strncmp($path, 'wordpress/', 10) !== 0) {
   	continue;
   	}
   	$rel = substr($path, 10);
   	if ($rel === '' || !cow_git_should_export_relative_path($rel)) {
   	continue;
   	}
   	$wp_files[$rel] = $blob_hash;
   	}
   	$branch_root = rtrim($branches_dir, "/\\") . '/' . $name;
   	if (!is_dir($branch_root)) {
   	$branch_root = cow_git_create_branch_for_ref(
   	$repo,
   	$branches_dir,
   	$storage_branches_dir,
   	$branch_list_path,
   	$file_view,
   	$debug_log,
   	$name,
   	$tip,
   	$wp_files,
   	$pre_receive_refs,
   	$existing_branches
   	);
   	$existing_branches[] = $name;
   	continue;
   	}
   	cow_git_apply_wp_files($repo, $branch_root, $wp_files);

   If a later apply step fails, the outer handler restores Git refs only:

   forkpress/scripts/git_server/cow_server.php

   Lines 128 to 142 in a10ea7f

   	} catch (\Throwable $e) {
   	try {
   	cow_git_restore_head_refs($git_repo_dir, $pre_receive_refs, cow_git_capture_all_head_refs($git_repo_dir));
   	} catch (\Throwable $restore_error) {
   	error_log("COW push ref restore failed after rejection: " . $restore_error->getMessage());
   	}
   	if (ob_get_level() > 0) {
   	ob_end_clean();
   	}
   	error_log("COW push processing error: " . $e->getMessage() . "\n" . $e->getTraceAsString());
   	http_response_code(500);
   	header('Content-Type: text/plain');
   	$short = substr(str_replace(["\n", "\r"], ' ', $e->getMessage()), 0, 500);
   	echo "forkpress cow: push rejected: $short\n";
   	return;

   At that point the deleted branch files may already be gone, so the restored Git ref can point to a branch that no longer exists locally. The one-ref push rule reduces the surface area, but this code still processes unrelated refs after the delete.

   Expected follow-up: keep staged delete backups alive until all post-receive work succeeds, then discard them at the end. On any later failure, restore both refs and staged branch paths. A complementary improvement is to skip unchanged refs during receive-pack apply.

3. Make receive-pack command parsing fail closed for policy enforcement.

   cow_git_parse_push_commands() is now a policy gate for one-ref pushes and protected branch deletion, but parse failures simply return whatever was parsed so far, possibly an empty command list:

   forkpress/scripts/git_server/cow_server.php

   Lines 162 to 218 in a10ea7f

   	function cow_git_parse_push_commands(string $request_bytes): array {
   	$commands = [];
   	$offset = 0;
   	$length = strlen($request_bytes);
   	while ($offset + 4 <= $length) {
   	$marker = substr($request_bytes, $offset, 4);
   	if ($marker === 'PACK') {
   	break;
   	}
   	if (!preg_match('/^[0-9a-f]{4}$/', $marker)) {
   	break;
   	}
   	$offset += 4;
   	if ($marker === '0000') {
   	break;
   	}
   	if ($marker === '0001' || $marker === '0002') {
   	continue;
   	}
   	$packet_length = hexdec($marker) - 4;
   	if ($packet_length < 0 || $offset + $packet_length > $length) {
   	break;
   	}
   	$payload = substr($request_bytes, $offset, $packet_length);
   	$offset += $packet_length;
   	foreach (explode("\n", rtrim($payload, "\n")) as $line) {
   	if ($line === '') {
   	continue;
   	}
   	$capabilities = [];
   	$nul = strpos($line, "\0");
   	if ($nul !== false) {
   	$capabilities = preg_split('/\s+/', trim(substr($line, $nul + 1))) ?: [];
   	$line = substr($line, 0, $nul);
   	}
   	if (!preg_match('/^([0-9a-f]{40}) ([0-9a-f]{40}) (.+)$/', $line, $matches)) {
   	continue;
   	}
   	$commands[] = [
   	'old_oid' => $matches[1],
   	'new_oid' => $matches[2],
   	'ref' => $matches[3],
   	'capabilities' => $capabilities,
   	];
   	}
   	}
   	return $commands;
   	}

   and

   forkpress/scripts/git_server/cow_server.php

   Lines 220 to 258 in a10ea7f

   	function cow_git_reject_push_commands_if_needed(array $commands): bool {
   	$rejections = cow_git_push_command_rejections($commands);
   	if (!$rejections) {
   	return false;
   	}
   	cow_git_close_receive_pack_buffer();
   	cow_git_send_receive_pack_rejections($rejections);
   	return true;
   	}
   	function cow_git_push_command_rejections(array $commands): array {
   	if (count($commands) > 1) {
   	$rejections = [];
   	foreach ($commands as $command) {
   	$rejections[$command['ref']] = 'ForkPress accepts one branch update per push';
   	}
   	return $rejections;
   	}
   	foreach ($commands as $command) {
   	$ref = $command['ref'];
   	if (!str_starts_with($ref, 'refs/heads/')) {
   	return [$ref => 'ForkPress only accepts branch refs'];
   	}
   	$branch = substr($ref, strlen('refs/heads/'));
   	if (!cow_git_valid_branch_name($branch)) {
   	return [
   	$ref => 'unsupported COW branch name; use 1-63 ASCII letters, numbers, underscores, or hyphens',
   	];
   	}
   	if ($branch === 'main' && Commit::is_null_hash($command['new_oid'])) {
   	return [$ref => 'refusing to delete the main COW branch'];
   	}
   	}
   	return [];

   If Git changes the request shape, or if a malformed request still reaches the vendored endpoint, the pre-mutation policy can be bypassed. Because this parser protects destructive branch deletion, unknown or partially parsed command lists should reject before mutation rather than falling through.

   Expected follow-up: return an explicit parse status/error, reject unparseable receive-pack command lists, and add tests for malformed command lists and any protocol features we advertise, such as push options.

Operational Note

The PR is currently marked conflicting against trunk because it still carries the pre-squash stack history. Before merging, rebase only the #13 commit onto current trunk, as was done for #12.

Verification Run

On the PR #13 head worktree:

• php -l scripts/git_server/cow_server.php
• php -l vendor/wordpress-php-toolkit/components/Git/class-gitendpoint.php
• php -l vendor/wordpress-php-toolkit/components/Git/class-gitrepository.php
• php tests/test_cow_git_server.php (15 passed)
• bash -n tests/test_cow_strategy_e2e.sh
• git diff --check 9fc8a0b5649a16fecfaed8def7f54b40cd8b4aa0..HEAD
• FORKPRESS_RUNTIME_BUNDLE=/dev/null cargo test -p forkpress (28 passed)

Not run for this review pass: make test-all, musl release build, or the full COW e2e script.

[JanJakes]

PR #14 / Replacement PR #31: Lock COW branch requests during mutations

Original PR #14 was auto-closed when its stacked base branch was deleted after #13 merged. The same change was rebased onto trunk and reopened as #31.

Review target: #31 / ae2a48eeade8692960703df610813aece1eddca2.

Findings

No blocking correctness findings in this review pass. The implementation routes Git smart-HTTP before taking the request-side shared lock, so Git requests continue through the existing exclusive Git adapter path. Normal COW HTTP requests then take a shared flock before branch lookup, static file serving, or PHP execution:

forkpress/runtime/router_cow.php

Lines 52 to 103 in ae2a48e

	if (preg_match('|^/([a-zA-Z0-9_\-]+)\.git(/.*)?$|', $path, $git_match)) {
	$git_path = $git_match[2] ?? '/';
	$cow_dir = getenv('FORKPRESS_COW_DIR') ?: dirname(rtrim($branches_dir, '/'));
	$git_repo_dir = getenv('FORKPRESS_COW_GIT_DIR') ?: rtrim($cow_dir, '/') . '/git';
	$storage_branches_dir = getenv('FORKPRESS_COW_STORAGE_BRANCHES_DIR') ?: $branches_dir;
	$branch_list = getenv('FORKPRESS_BRANCH_LIST') ?: rtrim($cow_dir, '/') . '/branches.txt';
	$file_view = getenv('FORKPRESS_COW_FILE_VIEW') ?: 'file-copy';
	$debug_log = getenv('FORKPRESS_DEBUG_LOG') ?: '';
	require_once __DIR__ . '/../scripts/git_server/cow_server.php';
	cow_git_server_handle($branches_dir, $git_repo_dir, $git_path, $query, $storage_branches_dir, $branch_list, $file_view, $debug_log);
	return true;
	}
	function forkpress_cow_acquire_request_lock(): bool {
	$cow_dir = getenv('FORKPRESS_COW_DIR') ?: '';
	if ($cow_dir === '') {
	return true;
	}
	$lock_path = rtrim($cow_dir, "/\\") . '/operations.lock';
	$lock_dir = dirname($lock_path);
	if (!is_dir($lock_dir) && !@mkdir($lock_dir, 0755, true) && !is_dir($lock_dir)) {
	http_response_code(503);
	echo "ForkPress COW operation lock unavailable\n";
	return false;
	}
	$handle = @fopen($lock_path, 'c');
	if (!$handle) {
	http_response_code(503);
	echo "ForkPress COW operation lock unavailable\n";
	return false;
	}
	if (!flock($handle, LOCK_SH)) {
	fclose($handle);
	http_response_code(503);
	echo "ForkPress COW operation lock unavailable\n";
	return false;
	}
	register_shutdown_function(static function() use ($handle): void {
	flock($handle, LOCK_UN);
	fclose($handle);
	});
	return true;
	}
	if (!forkpress_cow_acquire_request_lock()) {
	return true;
	}

Follow-up Notes

1. Strengthen the router lock test so it proves the child reached the lock gate.

   tests/test_cow_router_lock.php verifies branch PHP does not execute while the parent holds the exclusive lock:

   forkpress/tests/test_cow_router_lock.php

   Lines 71 to 105 in ae2a48e

   	$lock_path = $cow . '/operations.lock';
   	$lock = fopen($lock_path, 'c');
   	assert_true(is_resource($lock), 'test opened operation lock');
   	if (is_resource($lock)) {
   	assert_true(flock($lock, LOCK_EX), 'test holds exclusive operation lock');
   	$descriptor = [
   	0 => ['pipe', 'r'],
   	1 => ['pipe', 'w'],
   	2 => ['pipe', 'w'],
   	];
   	$process = proc_open([PHP_BINARY, $child, $branches, $cow, $router], $descriptor, $pipes);
   	assert_true(is_resource($process), 'spawned router request process');
   	if (is_resource($process)) {
   	fclose($pipes[0]);
   	usleep(350000);
   	assert_true(!file_exists($started), 'router request waits behind exclusive COW operation lock');
   	$released_at = microtime(true);
   	flock($lock, LOCK_UN);
   	fclose($lock);
   	$stdout = stream_get_contents($pipes[1]);
   	$stderr = stream_get_contents($pipes[2]);
   	fclose($pipes[1]);
   	fclose($pipes[2]);
   	$status = proc_close($process);
   	assert_same($status, 0, 'router request exits cleanly after lock release');
   	assert_same($stdout, 'OK', 'router served branch after lock release');
   	assert_same($stderr, '', 'router request produced no stderr');
   	assert_true(file_exists($started), 'branch PHP executed after lock release');
   	$started_at = (float)trim((string)file_get_contents($started));
   	assert_true($started_at >= $released_at - 0.05, 'branch PHP did not run before exclusive lock release');
   	}

   That covers the important behavior, but the first assertion can also pass if the child process simply has not reached the router yet. A small pre-router marker in the child fixture would make the test distinguish “child started and is blocked on the router lock” from “child has not been scheduled yet”.

2. Consider explicit behavior for long-held mutation locks.

   Request-side flock($handle, LOCK_SH) is blocking and has no timeout:

   forkpress/runtime/router_cow.php

   Lines 79 to 91 in ae2a48e

   	$handle = @fopen($lock_path, 'c');
   	if (!$handle) {
   	http_response_code(503);
   	echo "ForkPress COW operation lock unavailable\n";
   	return false;
   	}
   	if (!flock($handle, LOCK_SH)) {
   	fclose($handle);
   	http_response_code(503);
   	echo "ForkPress COW operation lock unavailable\n";
   	return false;
   	}

   That is reasonable for correctness, and process death releases the lock, but a long reset/delete/Git apply will make HTTP requests wait indefinitely. If that becomes user-visible, add progress/status reporting or a bounded wait with a clear 503 response.

3. Static-file lock coverage would be useful.

   The lock is registered via shutdown callback, so it should cover both PHP and static responses. The current test exercises PHP execution only. A focused static-file case would guard against future router refactors accidentally releasing the lock before readfile() completes.

Verification Run

On the rebased PR #31 head:

• php -l runtime/router_cow.php
• php -l tests/test_cow_router_lock.php
• php tests/test_cow_router_lock.php (10 passed)
• git diff --check trunk..HEAD
• FORKPRESS_RUNTIME_BUNDLE=/dev/null cargo test -p forkpress (28 passed)

Not run for this review pass: make test-all, musl release build, or the full COW e2e script. GitHub CI for #31 was still in progress when reviewed.

[JanJakes]

PR #15: Add COW storage lifecycle diagnostics

Reviewed PR #15 after rebasing its single commit onto trunk so the diff only contains this PR's storage lifecycle changes. Reviewed SHA: 4dc4ef3b6cee6facec57622d65e29e6400e7e543.

Findings

1. Static analysis: new lock-file opens need explicit truncate behavior.
   forkpress/src/main.rs:2340 (lock_server_registry) and forkpress/src/main.rs:4623 (lock_cow_lifecycle) open lock sentinel files with OpenOptions::create(true).read(true).write(true) but without .truncate(...). Clippy flags this as suspicious_open_options. These lock files should preserve contents, so add .truncate(false) explicitly. The already-merged lock_cow_operations uses the same pattern; this is a good follow-up to normalize all three lock-file opens.

Follow-up Notes

• Add a race-oriented test for the lifecycle lock path: background forkpress serve startup should block storage detach/storage compact until the server has either registered or failed, and storage compact should stop a registered server before detaching. The current E2E checks the happy path but does not prove the lifecycle lock closes that race.
• Keep the internal FORKPRESS_COW_PARENT_LIFECYCLE_LOCK marker out of child environments that do not need it. The spawned forkpress start process needs the marker, but the PHP server process inherits the environment by default. Removing it from the PHP Command would avoid accidental lifecycle-lock bypass if anything launched another ForkPress command from that environment.
• PR Add COW storage lifecycle diagnostics #15's description still says it is stacked on Lock COW branch requests during mutations #14. Since the PR is now rebased and retargeted to trunk after replacement PR Lock COW branch requests during mutations #31 merged, update that sentence to avoid confusing future reviewers.

Verification

Passed locally on the reviewed SHA:

• FORKPRESS_RUNTIME_BUNDLE=/dev/null cargo test -p forkpress
• cargo fmt --check
• bash -n tests/test_cow_strategy_e2e.sh
• git diff --check trunk..HEAD

Additional checks:

• FORKPRESS_RUNTIME_BUNDLE=/dev/null cargo clippy -p forkpress -- -D warnings first hits a pre-existing forkpress/build.rs single_element_loop warning outside this PR.
• Rerunning as FORKPRESS_RUNTIME_BUNDLE=/dev/null cargo clippy -p forkpress -- -A clippy::single-element-loop -D warnings exposes the Add COW storage lifecycle diagnostics #15-specific suspicious_open_options warnings above, plus existing collapsible-if warnings in surrounding code.
• make test-all did not reach the PHP tests in this local checkout because building ext/branchfs.so fails at link time against the local Homebrew PHP 8.5 install with unresolved PHP symbols.

[JanJakes]

PR #16: Stabilize COW Git snapshots

Reviewed PR #16 after rebasing its single commit onto trunk so the diff only contains this PR's Git snapshot stability change. Reviewed SHA: 128fe3d3ea9e5b3aff91236b53b1e289baffa9ed.

Findings

1. Mode changes can be hidden by the new snapshot-match check.
   cow_git_commit_matches_updates() compares only path count and blob hash (scripts/git_server/cow_server.php:348-356). cow_git_walk_tree() stores only path => hash and drops the tree entry mode (scripts/git_server/cow_server.php:1054-1062). That means a pushed Git tree containing wordpress/foo.php as an executable file or symlink can be treated as already matching the materialized COW branch if the blob hash matches. The branch application path writes ordinary files with file_put_contents(), and GitRepository::commit() would normally export updates as 100644; this early return can preserve the pushed tree mode in the Git ref instead of normalizing it to the branch's actual ordinary-file view. Consider making the match mode-aware, at least requiring exported paths to be regular 100644 blobs before skipping the snapshot commit.

Follow-up Notes

• Add coverage for file deletion stability: deleting an exported file should advance the ref once, and a following sync should keep that new ref stable.
• Add coverage for non-canonical Git tree entries, especially executable and symlink entries under wordpress/, so the adapter either deliberately preserves or deliberately normalizes those modes.
• The PR body's test command still references /home/claude/forkpress/ext/branchfs.so; update it to a repo-relative/local command before merge if you want the instructions to be directly reusable.

Verification

Passed locally on the reviewed SHA:

• php -l scripts/git_server/cow_server.php
• php -l tests/test_cow_git_server.php
• php tests/test_cow_git_server.php (20 passed)
• FORKPRESS_RUNTIME_BUNDLE=/dev/null cargo test -p forkpress
• cargo fmt --check
• git diff --check trunk..HEAD

make test-all still does not reach the PHP tests in this local checkout because building ext/branchfs.so fails at link time against the local Homebrew PHP 8.5 install with unresolved PHP symbols.

[JanJakes]

PR #17: Resync COW Git refs after push apply

Reviewed PR #17 after rebasing its single commit onto trunk so the diff only contains this PR's post-push resync change. Reviewed SHA: 9bef31a084e8d45018aba43b08dc6813bb4d35cc.

Findings

1. Post-apply resync failure can report a rejected push after branch files already changed.
   cow_git_apply_push_to_branches() now applies the pushed refs and then calls cow_git_sync_repository() (scripts/git_server/cow_server.php:363-383). The caller catches any exception from both phases, restores Git refs to pre_receive_refs, clears the receive-pack response, and returns forkpress cow: push rejected (scripts/git_server/cow_server.php:116-142). If the branch apply succeeds but the new resync fails, the materialized branch tree is not rolled back even though the Git ref is restored and the client sees a rejected push. Either make the apply+resync path transactional for branch files too, or separate the error handling so a resync failure after successful apply is reported/recovered as a post-apply consistency problem rather than as a full push rejection.

Follow-up Notes

• Add failure-injection coverage for the new two-phase path: simulate cow_git_sync_repository() failing after cow_git_apply_all_refs_to_branches() has written a WordPress file, and assert the desired final state for both branch files and Git refs.
• Consider resyncing only the pushed branch after apply. cow_git_sync_repository() currently walks every branch, so a push to one branch can also publish unrelated direct filesystem edits from other branches that happen before the post-push resync.
• The PR body's test command still references /home/claude/forkpress/ext/branchfs.so; update it to a repo-relative/local command before merge if you want the instructions to be directly reusable.

Verification

Passed locally on the reviewed SHA:

• php -l scripts/git_server/cow_server.php
• php -l tests/test_cow_git_server.php
• php tests/test_cow_git_server.php (24 passed)
• FORKPRESS_RUNTIME_BUNDLE=/dev/null cargo test -p forkpress
• cargo fmt --check
• git diff --check trunk..HEAD

make test-all still does not reach the PHP tests in this local checkout because building ext/branchfs.so fails at link time against the local Homebrew PHP 8.5 install with unresolved PHP symbols.