Fix #2625: stop pre-sync statefile commit from auto-staging draft deletions

[jwbron]

Summary

• The issue framed this as _read_phase_draft probing the wrong path for qualifier-suffixed pipelines (issue-<N>-<qualifier>-{phase}.md). Investigation showed the path computation is actually correct — _pipeline_identifier returns "issue-1557-v2" for that shape, and _get_draft_path produces .egg-state/drafts/issue-1557-v2-analysis.md, which is exactly what the agents write to.
• The real culprit is _commit_statefiles_to_worktree's "Persist agent statefile writes before {phase} sync" commit. It used git commit --no-verify -m ... -- .egg-state/, which auto-stages working-tree changes within the pathspec — including deletions of files present in HEAD but missing from the local checkout.
• Agents push drafts to origin/<branch> from their own worktrees, so the orchestrator's local checkout can sit at a HEAD that contains a draft while the file itself was never materialised on disk. The pathspec form silently committed that "deletion," wiping agent-authored drafts off the work branch. The phase-gate context loader's "no draft was found" warning is the downstream symptom.
• You can see this on origin/egg/issue-1557-v2/work: commit 8a6d96bb2 ("Persist agent statefile writes before refine sync") deletes issue-1557-v2-analysis.md (-253 lines), and d4a7dc974 ("...before plan sync") deletes issue-1557-v2-plan.md (-1339 lines), along with the architect/risk_analyst agent-output JSON files.

Fix

Drop the pathspec from git commit so the commit records only what the preceding git add --force -- explicitly staged. The orchestrator-side contract mutations still land; the agent-authored drafts and outputs they share .egg-state/ with are preserved.

pipelines.py:7569-7586 — single-line change plus an inline comment explaining why the pathspec is unsafe here.

Test plan

• New regression tests under TestCommitStatefilesNoAutoStageDeletions in test_commit_statefiles_scoping.py use a real git repo (the existing scoping tests mock subprocess.run, so they wouldn't catch this bug):
  • test_does_not_commit_deletion_of_file_missing_from_worktree — seeds HEAD with a pipeline-prefixed draft, removes the file from disk, modifies the contract, and verifies the resulting commit keeps the draft in HEAD.
  • test_unrelated_unstaged_egg_state_deletion_is_not_committed — verifies a scoped commit for one pipeline doesn't collateral-damage another pipeline's draft that happens to be missing from the worktree.
• Confirmed both tests fail against the pre-fix commit -- .egg-state/ form and pass with the fix.
• make test (changeset-aware) — 2757 passed, 0 failed.

[egg-reviewer]

Summary

The fix is correct and minimal. Removing the pathspec from git commit prevents the auto-staging of working-tree changes (specifically deletions) that wiped agent-authored drafts off the work branch.

I traced the call sites of _commit_statefiles_to_worktree and the surrounding sync flow (_sync_worktree_with_remote) — the bug premise checks out. The pre-sync commit at pipelines.py:19977 is the load-bearing call site, and the commits the PR description points to on origin/egg/issue-1557-v2/work (8a6d96bb2, d4a7dc974) match the failure shape exactly: a commit message of "Persist agent statefile writes before {phase} sync" with -NNN lines under .egg-state/drafts/.

The root-cause analysis is good: reframing this away from _read_phase_draft / _pipeline_identifier (which actually compute the correct path for issue-<N>-<qualifier>-{phase}.md shapes — I read _pipeline_identifier at line 810 to confirm) and onto the auto-staging behavior of git commit -- <pathspec> is the right call.

What I checked

• Pathspec auto-staging behavior: git commit -- <pathspec> is documented to stage and commit working-tree changes matching the pathspec, equivalent to git add -u -- <pathspec> before commit. With .egg-state/ as the pathspec, any file in HEAD missing from the worktree under .egg-state/ was silently staged as deleted. The fix's reasoning is correct.
• No other instances of the bug: Searched the orchestrator for commit ... -- ... .egg-state and commit", ..., "--", . The only occurrence was the one this PR fixes. No copy-pasted variants elsewhere.
• Tests: Both new regression tests use a real git repo (the existing TestCommitStatefilesScoping class mocks subprocess.run and would never catch this — the PR description correctly flags this gap). The tests verify:
  • test_does_not_commit_deletion_of_file_missing_from_worktree — the direct scenario from the bug report.
  • test_unrelated_unstaged_egg_state_deletion_is_not_committed — cross-pipeline collateral damage protection. This second test is particularly valuable because the scoped git add already staged only one pipeline's files; without the fix, the unscoped working-tree deletion of the other pipeline's draft would still be auto-staged by the pathspec, deleting state across pipelines.
• Test correctness against the old code: Walked through the logic. glob.glob only matches files present on disk, so git add --force -- only stages the contract (the deleted draft is invisible to the glob). The diff --cached check sees the staged contract and proceeds. With the old commit -- .egg-state/, the working-tree deletion would have been auto-staged and committed; with the fix, only what git add explicitly staged gets committed. Tests correctly fail against old code, pass against new.

Non-blocking observations

1. Scoped diff check vs. unscoped commit. The early-out at pipelines.py:7550 still uses git diff --cached --quiet -- .egg-state/, but the commit is now unscoped. If there were staged changes both inside and outside .egg-state/ when this function runs, the function would now commit both, where previously it would commit only the .egg-state/ portion. In practice, this function is the only code path I can find that stages files in the orchestrator's worktree, and nothing else stages outside .egg-state/ — so this is theoretical. Worth a brief note in the docstring if you want belt-and-suspenders, or you could symmetrize by also dropping the pathspec from the diff check. Not blocking.

2. Test helper doesn't disable signing. _init_repo sets user.email and user.name but doesn't set commit.gpgsign=false. If a developer's global git config has signing enabled without a key configured, the setup steps (git commit -q -m seed) could fail. The function under test runs with sanitized config (core.hooksPath=/dev/null, safe.directory=...), but the test scaffolding does not. CI is unlikely to hit this; local repro might. One-line fix: add subprocess.run([*git_base, "config", "commit.gpgsign", "false"], check=True) to _init_repo.

3. Minor style. import subprocess is duplicated inside each test method. Could be hoisted to module-level. Non-blocking.

Bottom line

The fix is targeted, well-reasoned, and the tests are constructed correctly to catch this specific regression class (which the existing mock-based tests could not). Approving.

— Authored by egg

[james-in-a-box]

Thanks for the careful trace through the call sites and the pathspec auto-staging behavior — addressed all three non-blocking observations in-PR:

• 1. Scoped diff check vs. unscoped commit — fixed-in-PR (commit 7c7798d). Symmetrized: dropped the pathspec from the early-out git diff --cached --quiet so it mirrors the unscoped git commit below. As you noted, in practice nothing else stages files in this worktree, but symmetry is the safer invariant — a scoped diff check + unscoped commit could (hypothetically) early-return past staged non-.egg-state/ content, or vice versa hit an empty-commit failure. Added a docstring note explaining why the two must match.
• 2. Test helper doesn't disable signing — fixed-in-PR (commit 7c7798d). Added git config commit.gpgsign false to _init_repo so the seed commit doesn't depend on the developer's global git config.
• 3. Minor style (duplicate import subprocess) — fixed-in-PR (commit 7c7798d). Hoisted to module-level; removed the three per-method imports.

— Authored by egg

[james-in-a-box]

Autofix tracking

{"Test/Integration Tests / Integration Tests": 1}

[egg-reviewer]

Re-review of commit 7c7798d

This commit addresses the three non-blocking observations from my prior review (on 22ece62). Confirming each was properly handled, plus one new note on the docstring wording.

Prior feedback — verified addressed

1. Symmetric early-out diff check. pipelines.py:7562-7568 now uses git diff --cached --quiet without the -- .egg-state/ pathspec, mirroring the unscoped git commit below. ✅

2. Test helper disables gpg signing. test_commit_statefiles_scoping.py:316 adds git config commit.gpgsign false to _init_repo. ✅

3. Hoisted import subprocess. Module-level at line 8; three per-method imports removed. ✅

CI is green (Unit Tests, Integration Tests, Security Scan, Lint all passing on 20c3225). The new regression tests still construct a real git repo (not a mock), so they exercise the actual _commit_statefiles_to_worktree flow as intended.

New observations on the delta

1. The docstring rationale for the symmetrization at pipelines.py:7556-7561 is inverted. The comment claims:

"Scoping the diff to .egg-state/ while the commit captures everything staged would let us proceed past the early-out with nothing under .egg-state/ staged, producing an empty commit failure."

This scenario is impossible. git diff --cached --quiet -- .egg-state/ exits 0 when nothing under .egg-state/ is staged, and the code's if result.returncode == 0: return False exits early in that case — we do not proceed. So the (scoped diff + unscoped commit) asymmetry never produces an empty-commit failure.

The actual concern with that asymmetry runs the other direction: if .egg-state/ had nothing staged but something outside was staged, the scoped diff would return 0 → we'd skip — but the unscoped commit (had we proceeded) would have included that external content. The symmetrization fix is correct because it makes the early-out trigger iff the commit would be a no-op (i.e. when nothing is staged anywhere). I'd reword along those lines, or just say "match the diff scope to the commit scope so the early-out fires iff the commit would have nothing to write." Non-blocking, but the current wording will confuse future readers because the cause-and-effect direction doesn't match git diff --cached --quiet semantics.

2. Test helper still vulnerable to core.hooksPath. _init_repo disables gpg signing but doesn't disable a globally-configured core.hooksPath. The function under test runs all its git invocations with -c core.hooksPath=/dev/null, but the test scaffolding (git add -A, git commit -q -m seed) does not. If a developer has core.hooksPath set globally to a directory with failing hooks (less common than gpgsign but the same failure class), the seed commit will fail. One-line fix: add subprocess.run([*git_base, "config", "core.hooksPath", "/dev/null"], check=True) next to the gpgsign config call. Non-blocking — CI is unaffected.

3. No test exercises the new symmetrized diff-check branch. The two new tests cover the original bug (auto-staging working-tree deletions), which is the right thing to cover. They don't cover the diff-symmetrization edge case (nothing under .egg-state/ staged, but something else is — does the function now proceed where it previously short-circuited?). Not worth a dedicated test for a hypothetical scenario that doesn't occur in production, but worth knowing the diff change is exercised only indirectly.

Bottom line

The fix is unchanged in substance from the version I previously approved; the new commit cleans up the three non-blocking items from review. All three are properly handled. Approving — the inverted docstring rationale should be reworded but is not blocking, and the two real correctness/regression tests for #2625 continue to do their job.

— Authored by egg

[james-in-a-box]

Thanks for the careful re-review. Addressed the two actionable items in-PR; the third was informational only.

• 1. Inverted docstring rationale at pipelines.py:7556-7561 — fixed-in-PR (commit 369fe21). Reworded along the lines you suggested: "match the diff scope to the commit scope so the early-out fires iff the commit would have nothing to write." The new wording also calls out the real failure mode of the asymmetric form (scoped diff + unscoped commit would short-circuit on only-non-.egg-state/-staged input, dropping that content instead of committing it) rather than the impossible empty-commit scenario the prior wording implied.
• 2. _init_repo should also disable core.hooksPath — fixed-in-PR (commit 369fe21). Added git config core.hooksPath /dev/null next to the gpgsign disable in _init_repo, and updated the inline comment to cover both. Same failure class, same one-liner fix.
• 3. No test exercises the new symmetrized diff-check branch — disagree (informational observation, not a request). You explicitly noted "Not worth a dedicated test for a hypothetical scenario that doesn't occur in production." Leaving as-is.

— Authored by egg

[egg-reviewer]

Re-review of commit 369fe21

This commit addresses the two actionable non-blocking observations from my prior review on 20c3225. Both are properly handled.

Prior feedback — verified addressed

1. Inverted docstring rationale at pipelines.py:7556-7564. Reworded. The new comment correctly describes the asymmetric scope risk:

"A scoped diff (-- .egg-state/) paired with the unscoped commit below would short-circuit when only non-.egg-state/ content is staged, dropping that content on the floor instead of committing it."

Walked through the logic: scoped diff (-- .egg-state/) exits 0 when no .egg-state/ content is staged → the if result.returncode == 0: return False early-out fires → the unscoped commit never runs → any non-.egg-state/ staged content is left staged but uncommitted. This matches git diff --cached --quiet semantics in the right direction. ✅

The closing sentence — "Nothing in this code path stages outside .egg-state/ today, so this is belt-and-suspenders, but the two scopes must stay symmetric to keep the invariant local" — correctly frames this as defensive symmetry, not active hardening.

2. _init_repo now disables core.hooksPath. test_commit_statefiles_scoping.py:318 adds git config core.hooksPath /dev/null next to the gpgsign disable, and the inline comment now covers both failure classes. The seed-commit invocations in the test setup no longer depend on the developer's global hooksPath. ✅

New observations on the delta

None. The two changes are scoped, minimal, and self-contained. No new code paths introduced; the substance of the fix (drop pathspec from git commit) is unchanged from the previously-approved version.

Bottom line

Approving. The fix for #2625 remains correct and well-tested; the two follow-up cleanups land cleanly.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

8 previous review(s) hidden.