Deterministic contract commit in local orchestrator

[james-in-a-box]

Deterministic contract commit in local orchestrator

The local orchestrator creates contract files (.egg-state/contracts/{issue}.json)
in per-pipeline worktrees but never commits them to git. The old GitHub Actions
workflow had an explicit git add + git commit step. The local orchestrator
relied on the agent to notice the untracked file and include it — which is
non-deterministic. Issues 635 and 545 lost their contracts; issue 645 got lucky
because the agent happened to bundle it.

This adds a _commit_contract_to_worktree() helper that runs git add +
git commit in the worktree immediately after the contract file is created,
mirroring the old GHA behavior. The commit is idempotent (skips if nothing
staged) and non-fatal (logs a warning on failure rather than failing the
pipeline).

Issue: none

Test plan:

• ruff check passes
• Existing orchestrator tests pass (319/319, 1 pre-existing failure in test_state_store unrelated)
• Next pipeline run should produce an "Initialize SDLC contract" commit on the feature branch

Authored-by: egg

[egg-reviewer]

Review: Deterministic contract commit

The approach is sound — committing the contract file deterministically rather than relying on the agent to notice it fixes a real problem (issues #635, #545). The implementation is mostly correct but has two issues worth addressing.

Issue 1: git diff --cached --quiet checks ALL staged changes, not just the contract file

_commit_contract_to_worktree runs git add <contract_file> then git diff --cached --quiet to decide whether to commit. --cached --quiet checks the entire staging area, not just the file that was just added. If any other file happens to be staged in the worktree at this point (e.g., from a previous failed operation, or from create_local_contract/create_contract leaving something staged), the commit will silently sweep those changes in alongside the contract.

This is unlikely given the current flow (freshly created worktree), but it's fragile. Scope the diff check to the contract file:

result = subprocess.run(
    [*git_base, "diff", "--cached", "--quiet", "--", contract_rel_path],
    capture_output=True, text=True, check=False,
)

This ensures only the contract file's staged status triggers the commit, and the commit itself only includes the contract file (since only it was git add-ed). The -- separator also makes this safe against filenames that look like flags.

Issue 2: Contract path constructed inline instead of reusing loader.py constants

Lines 1920-1923 construct the contract relative path with an inline f-string:

contract_rel = f".egg-state/contracts/{pipeline.issue_number}.json"

But loader.py already defines DEFAULT_CONTRACTS_DIR = ".egg-state/contracts" and has get_contract_path(). If the directory ever changes, the caller will diverge. Consider reusing the constant:

from egg_contracts.loader import DEFAULT_CONTRACTS_DIR

if pipeline.issue_number is not None:
    contract_rel = f"{DEFAULT_CONTRACTS_DIR}/{pipeline.issue_number}.json"
else:
    contract_rel = f"{DEFAULT_CONTRACTS_DIR}/{pipeline_id}.json"

Or even cleaner, use the existing get_contract_path() and make it relative:

from egg_contracts.loader import get_contract_path

contract_abs = get_contract_path(
    pipeline.issue_number if pipeline.issue_number is not None else pipeline_id,
    repo_root=worktree_repo_path,
)
contract_rel = str(contract_abs.relative_to(worktree_repo_path))

Minor observations (non-blocking)

• No timeout on subprocess calls. The three subprocess.run calls have no timeout. If git hangs (index lock contention, NFS stall), the pipeline thread blocks indefinitely. The existing state_store.py pattern has the same gap, so this isn't a regression, but adding timeout=30 would be cheap insurance. Not blocking since this matches existing patterns.

• contract_synced = True set before commit. If the commit fails (caught by the inner try/except), the pipeline continues with contract_synced = True. On re-run, the contract won't be re-committed. This is fine for the current use case — the contract file exists on disk and the agent will find it — but worth noting that "synced" now means "file created" not "file committed".

Summary

Issue 1 (scoping the diff check) is the only change I'd consider blocking. The rest is advisory. The overall design — idempotent commit, non-fatal on failure, hooks disabled — is well-considered.

— Authored by egg

[james-in-a-box]

Thanks for the thorough review. Both issues addressed in f358751:

Issue 1 — Agreed. Scoped git diff --cached --quiet to the contract file with -- contract_rel_path. This prevents accidentally sweeping in unrelated staged changes.

Issue 2 — Agreed. Replaced the inline f-string with get_contract_path() from egg_contracts.loader, keeping the path construction in sync with the canonical constant.

The non-blocking observations (subprocess timeouts, contract_synced semantics) are noted — they match existing patterns so I've left them as-is.

— Authored by egg

[egg-reviewer]

Re-review: Both issues addressed correctly

Verified the delta from 128b09b to f358751.

Issue 1 (diff check scoping): Fixed

git diff --cached --quiet now includes -- contract_rel_path, scoping the check to the contract file only. This matches the suggestion exactly and prevents accidentally sweeping unrelated staged changes into the commit.

Issue 2 (reuse loader constants): Fixed

The inline f-string is replaced with get_contract_path(identifier, repo_root=worktree_repo_path).relative_to(worktree_repo_path). The identifier selection logic (issue_number when present, pipeline_id otherwise) is consistent with how contract_key works in the Contract model and how save_contract resolves paths — so the path produced here will always match the file that was just written by create_contract / create_local_contract.

New code inspection

No new issues introduced. The from egg_contracts.loader import get_contract_path is a lazy import inside the function body, which matches the existing pattern a few lines above (from egg_contracts.loader import create_local_contract and from egg_contracts.loader import create_contract). The .relative_to() call is safe here because get_contract_path is called with repo_root=worktree_repo_path, guaranteeing the returned path starts with worktree_repo_path.

No blocking issues remain.

Minor note (non-blocking)

The lazy import of get_contract_path could be consolidated with the existing create_local_contract / create_contract imports a few lines above, since all three come from egg_contracts.loader and are in the same code path. Not blocking since the current pattern is consistent with how the existing imports are structured (each imported at point of use).

— Authored by egg

[jwbron]

@james-in-a-box let's ensure all pertinent statefiles are persisted the same way. See https://github.com/jwbron/egg/tree/main/.egg-state

[james-in-a-box]

Generalized the contract-only commit to cover all .egg-state/ statefiles.

What changed:

• Renamed _commit_contract_to_worktree() → _commit_statefiles_to_worktree() — now stages and commits the entire .egg-state/ directory (contracts, drafts, reviews, check results) instead of just the contract file.
• Added a second call site after each phase completes so that agent-written statefiles (drafts, review verdicts, check results) and orchestrator-written updates (_populate_contract_from_plan) are deterministically committed even if the agent did not commit them itself.

This mirrors the GHA workflow's git add .egg-state/ at phase boundaries (sdlc-pipeline.yml:440). The commit remains idempotent (skips when nothing is staged) and non-fatal (logs a warning on failure).

State files now covered:

Directory	Files	Previously committed?
.egg-state/contracts/	Contract JSON	✅ (this PR's original change)
.egg-state/drafts/	Analysis + plan markdown	❌ relied on agent
.egg-state/reviews/	Review verdict JSON	❌ relied on agent
.egg-state/checks/	Check results JSON	❌ relied on agent

Verification: ruff check passes, 342/342 orchestrator tests pass (1 pre-existing failure in test_state_store unrelated).

— Authored by egg

[egg-reviewer]

Re-review: Generalized statefile commit

The generalization from contract-only to all .egg-state/ files is a reasonable response to the owner's request. The overall structure is good — idempotent, non-fatal, hooks disabled. One issue from the original review has resurfaced in a new form.

Issue: git commit is unscoped — can sweep in unrelated staged files

_commit_statefiles_to_worktree (line 1098-1100) runs:

subprocess.run(
    [*git_base, "commit", "--no-verify", "-m", message],
    ...
)

The git add and git diff --cached are correctly scoped to .egg-state/, but the git commit has no pathspec — it commits everything in the staging area, not just .egg-state/ files.

This is the same class of bug as the original Issue 1 (unscoped git diff --cached), now at the commit step. The original fix scoped the diff check, but the generalization introduced an unscoped commit.

When this matters: The phase-boundary call site (line 2425) runs after the agent container exits. If the agent staged files but didn't commit them (partial staging on error exit, interrupted operation, or the agent intentionally staging before a planned commit), those files get swept into a commit with message "Persist statefiles after {phase} phase". The agent's work ends up attributed to an orchestrator housekeeping commit with an unrelated message, and the agent's next phase sees those changes already committed — potentially breaking its workflow.

The GHA workflow (sdlc-pipeline.yml:440-441) has the same git add .egg-state/ && git commit pattern, but GHA runners start with a clean checkout and no prior staged state. The local orchestrator's shared worktree does not have that guarantee.

Fix — scope the commit to .egg-state/:

subprocess.run(
    [*git_base, "commit", "--no-verify", "-m", message, "--", ".egg-state/"],
    capture_output=True, text=True, check=True,
)

git commit -- .egg-state/ creates a partial commit containing only staged changes within .egg-state/. Any other staged files remain staged and undisturbed.

Docstring is misleading (non-blocking)

The docstring (line 1077) says "raise on git add failure only", but git commit (line 1098) also uses check=True and will raise CalledProcessError. Both call sites correctly catch this, so the behavior is fine — the docstring just doesn't match. Consider:

non-fatal to callers — a CalledProcessError is raised on failure
but both call sites catch and log it.

Or simply: "Raises subprocess.CalledProcessError on git failure."

Previous review issues

• Issue 1 (scoped diff check): The diff check remains scoped to .egg-state/ (line 1092), which is correct for the generalized function. But the unscoped commit is the same category of problem at a different step.
• Issue 2 (reuse loader constants): No longer applicable — the function operates on .egg-state/ as a directory, so there's no path to construct. The get_contract_path import was correctly removed.

Summary

The unscoped git commit is the only blocking issue. Everything else — the directory existence guard, idempotency check, hook disabling, error handling at call sites, the second call site at phase boundaries — is well-implemented.

— Authored by egg

[james-in-a-box]

Both issues addressed in 1d55cea:

Unscoped git commit — Agreed. Added -- .egg-state/ to the git commit command so it creates a partial commit scoped to statefiles only. Any other staged files remain undisturbed. All three git operations (add, diff --cached, commit) are now consistently scoped to .egg-state/.

Docstring mismatch — Agreed. Updated the docstring to accurately state that CalledProcessError is raised on any git failure, and that both call sites catch and log it.

ruff check passes, 3226/3226 tests pass.

— Authored by egg

[egg-reviewer]

Re-review: Both issues fixed

Verified the delta from df0aeff to 1d55cea.

Issue 1 (unscoped git commit): Fixed

git commit now includes -- .egg-state/, creating a partial commit scoped to statefiles only. All three git operations in _commit_statefiles_to_worktree are now consistently scoped:

• git add .egg-state/
• git diff --cached --quiet -- .egg-state/
• git commit --no-verify -m <message> -- .egg-state/

Any other staged files remain undisturbed, which was the original concern.

Issue 2 (docstring mismatch): Fixed

The docstring now accurately states that CalledProcessError is raised on any git failure, and that both call sites catch and log rather than aborting the pipeline. This matches the actual check=True on both git add and git commit.

New code inspection

No new issues introduced. The delta is minimal and precisely addresses the two raised concerns with no other changes.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

9 previous review(s) hidden.