Nothing checks that a gate left the worktree unmodified, and Exit-0 → pass would report a repairing gate as a clean run

[djabi]

Plan

Plan: Worktree modification check around gate execution (#41)

Normative check

Consistent. docs/gates-and-commands.md § "The non-modification rule is checked, not assumed" (line 160) requires exactly this:

The runner records tracked state before spawning and compares after; paths the project ignores are outside the subject. A difference means broke the contract, never measured.

OutcomeBrokeContract is declared at gate.go:52. The outcome table (line 118) assigns this cause: "modified the subject it measured." No document contradicts. No document update needed — the rule is already stated; this implements it.

---

Change 1 — pkg/backend/github/worktree.go: wrap RunGate with before/after comparison

Current method (lines 182–188) validates the name, builds argv, and delegates to runGate. The new version:

1. Before the gate: call w.b.git.StatusPorcelain(ctx) and store the result as before. If this fails, return an error (no gate ran, no outcome exists — the runner cannot fulfil its contract without a starting snapshot).

2. Run the gate: call runGate exactly as today.

3. Short-circuit on error or non-measured outcome: if runGate returned an error, or the outcome is anything other than OutcomeMeasured, return immediately. The four non-measured outcomes are already failures; overriding them would lose attribution (e.g., timeout → broke-contract sends the wrong person to investigate).

4. After the gate: call w.b.git.StatusPorcelain(ctx) again, store as after. If this fails, set run.Outcome = OutcomeBrokeContract with a Detail naming the git error — the gate did run and claimed to measure, but the runner cannot verify integrity, so the safe direction is refusal.

5. Compare: if before != after, set run.Outcome = OutcomeBrokeContract and run.Detail = "the gate modified the worktree:\n" + after.

6. Return run.

Why StatusPorcelain and not IsDirty

IsDirty (git.go:111) uses --untracked-files=no — it misses new files a gate drops into the tree. The issue and normative doc both require untracked non-ignored files to count. StatusPorcelain (git.go:119) uses --untracked-files=normal, which is the correct boundary. Git-ignored files are excluded by git itself, matching the doc: "paths the project ignores are outside the subject."

Why only override measured

A timed-out gate that also dirtied the tree is still a timeout for retry purposes. A died gate is still died for attribution. The worktree is still "spent" in both cases, but that enforcement (refusing subsequent gates) is out of scope — see below.

---

Change 2 — pkg/backend/github/gate_test.go: update gateWorktree dummy runner

The existing dummy runner (line 49) calls t.Errorf for ANY command through the runner, which will now fire for StatusPorcelain's git calls. Update it to allow git status while still catching unintended calls:

runner: func(_ context.Context, _, name string, args ...string) ([]byte, []byte, error) {
    for _, a := range args {
        if a == "status" {
            return nil, nil, nil // StatusPorcelain: appears clean
        }
    }
    t.Errorf("a gate was spawned through the command runner (%s)", name)
    return nil, nil, nil
},

This returns empty output (= clean tree) for status calls, so before == after == "" and all existing outcome tests pass unchanged. The guard still catches any other git command being called from the gate-spawning path.

---

Change 3 — pkg/backend/github/gate_test.go: new test helper and test cases

New helper: gateWorktreeGit(t, script, timeout) (*worktree, string)

Like gateWorktree, but initializes a real git repo:

• Creates temp dir with a space in the name (same convention).
• git init, git add -A, git commit -m "init" so the tree starts clean.
• Writes bin/gate with the script.
• Commits the script so it's tracked (otherwise it would show up as an untracked file in the before snapshot, but the gate doesn't modify it, so before == after still holds). Actually — the script must be committed or else before will list it as untracked, after will too (unchanged), and before == after. Either way works, but committing is cleaner.
• Constructs a Backend with a real gitOps (via newGitOps(dir)).

Test cases

All in a single TestRunGate_DetectsWorktreeModification table-driven test:

Case	Script	Expected outcome	Why
clean gate stays measured	echo '{"g":1}'	OutcomeMeasured	no false positives
modifies tracked file	echo x >> tracked; echo '{"g":1}'	OutcomeBrokeContract	the core case
creates untracked file	touch stray; echo '{"g":1}'	OutcomeBrokeContract	untracked counts
modifies and restores	echo x >> tracked; git checkout -- tracked; echo '{"g":1}'	OutcomeMeasured	normative doc line 172
timed out + modified	touch stray; exec sleep 60	OutcomeTimedOut	non-measured not overridden
died + modified	touch stray; exit 0	OutcomeDied	non-measured not overridden

The helper creates a committed file named (e.g.) tracked so the "modifies tracked file" and "modifies and restores" cases have something to work with.

---

What this does NOT do

• Spent-worktree enforcement. The normative doc says remaining gates must not run in a modified worktree. There is no multi-gate loop today — runIntegrationGate runs one gate, and any non-measured outcome already abandons the resolution. Enforcement belongs in the loop that doesn't yet exist, and pre-building it would be speculative structure.

• No change to runGate(). It is a pure process runner with no git knowledge. The modification check is the worktree layer's concern.

• No change to the flow.Worktree interface or GateRun struct. The existing OutcomeBrokeContract and Detail field are sufficient.

• No change to the fake backend (pkg/backend/fake/). It doesn't spawn processes and has no worktree to check.

• No change to downstream consumers (runIntegrationGate, CheckFit). Both already handle OutcomeBrokeContract correctly — it's a non-measured outcome, and all non-measured outcomes are treated as gate failure.

---

Verification

1. go test ./pkg/backend/github/ -run TestRunGate — all existing tests pass with the dummy runner update; new modification-detection tests pass.
2. go test ./... — full suite, no regressions.
3. bin/verify — the project's own gate, which is what the flow runs before opening a PR.

Review

bin/verify passes clean.

---

Review

Correctness

The implementation does what the issue asked:

1. Captures tracked state before spawning — StatusPorcelain (which runs git status --porcelain --untracked-files=normal) at worktree.go:190.
2. Compares after the process ends — worktree.go:208-218.
3. Returns OutcomeBrokeContract on difference, never OutcomeMeasured — worktree.go:215-218.
4. Only overrides OutcomeMeasured — worktree.go:203-206. Non-measured outcomes (timed out, died, could not start, broke contract from bad envelope) are already failures and keep their attribution. This is correct: overriding timed_out with broke_contract would send the wrong person to investigate.

Untracked, non-ignored files count as a change — confirmed: StatusPorcelain uses --untracked-files=normal, which reports ?? entries for untracked non-ignored files. The test at line 458-461 ("creates untracked file") verifies this.

"Modified and restored passes" — confirmed by the test at line 463-466, which matches the document's stated limit (line 172).

The mock in gateWorktree (the non-git helper) is updated correctly: it intercepts status args to return clean, so existing tests that don't use a real git repo continue to work without the new check interfering.

Edge path I checked: if StatusPorcelain fails after the gate (e.g., .git corrupted by the gate), the implementation returns OutcomeBrokeContract with a diagnostic rather than propagating an error — correct, since a gate did run and did produce something.

Scope

The diff touches exactly two files, both in pkg/backend/github/. No unrelated changes.

Duplication

StatusPorcelain already existed in git.go:122 and is reused here. No new source of truth.

Tests

Six test cases covering:

• Clean gate → measured (positive control)
• Tracked file modified → broke_contract
• Untracked file created → broke_contract
• Modified and restored → measured (the stated limit)
• Timed out gate that also modified → timed_out (attribution preserved)
• Died gate that also modified → died (attribution preserved)

Each would fail if the check were reverted: the modify/untracked cases would return measured instead of broke_contract. The gateWorktreeGit helper correctly initializes a real git repo with a tracked file and committed state so StatusPorcelain produces meaningful output.

Workarounds

None. The implementation is direct.

Against the normative document

docs/gates-and-commands.md lines 160-172: the implementation matches. "The subject is the tracked tree" — StatusPorcelain with --untracked-files=normal captures exactly this (tracked changes + untracked non-ignored; ignored paths excluded). "A difference means broke the contract, never measured" — enforced at line 215-218. The two stated limits (modify-and-restore passes; detection is after the fact) are both accepted by the implementation.

What I deliberately left

Step 4 of the issue — "Refuse the remaining gates for that transition in that worktree" — is not implemented in RunGate and does not need to be: RunGate is the per-gate boundary. The caller (runIntegrationGate in issue/steps.go:721-743) already treats any non-measured outcome as an error that stops the step, so no further gates run in the same worktree after a broke_contract. When integration becomes a composition of multiple gates, the loop that iterates them will need to check for this — but that composition does not exist yet, so there is nothing to wire.

Nothing to fix. The change is correct, scoped, tested, and aligned with the normative document.

Coverage

bin/verify passes cleanly.

---

Already covered by the existing TestRunGate_DetectsWorktreeModification (shipped with the implementation):

• Clean gate stays measured
• Tracked file modification → broke_contract
• Untracked file creation → broke_contract
• Modify-and-restore → measured (the documented limit)
• Non-measured outcomes are not overridden (timeout + modification stays timeout; died + modification stays died)

Added:

1. TestRunGate_DetectsTrackedFileDeletion — a deletion is a modification. Without this, someone could remove the before != after comparison for deletions specifically (e.g. by comparing only added lines) and no test would fail.

2. TestRunGate_ModificationDetailNamesTheChangedFiles — asserts that the Detail field carries the porcelain output naming each changed file. Without this, the detail could be empty or generic and the outcome would still pass the existing table-driven test, leaving a human with no way to know what was changed.

3. TestRunGate_PreSnapshotFailureIsAnErrorAndDoesNotSpawn — when StatusPorcelain fails before the gate runs, RunGate must return an error (not an outcome) and must not spawn the gate. Asserts all three: error message wraps the cause, outcome is empty, bin/gate was not executed. Without this, the pre-snapshot failure could silently fall through to spawn.

4. TestRunGate_PostSnapshotFailureIsBrokeContract — when StatusPorcelain fails after a measured gate, the outcome must be broke_contract (not an error, and not measured). Uses a call counter to fail only the second StatusPorcelain call. Without this, a post-snapshot failure could be returned as an error (which implies the gate never ran) or could fall through as measured.

Not tested, and why:

• could_not_start + modification bypass: could_not_start returns through the err != nil path from runGate, which exits before the post-check. This is already tested by TestRunGate_CouldNotStartIsNotDied and adding a modification variant would test runGate's error return, not this change's logic.
• broke_contract (bad envelope) + modification bypass: same structure as timeout/died — the run.Outcome != OutcomeMeasured guard covers all non-measured outcomes uniformly. Two subcases already exercise that guard; a third adds no distinguishing power.

Gate

• gate: integration
• outcome: measured
• acceptable: true
• verdict: every judged metric is within its cap

measurement:

{"gate":"integration","metrics":[{"name":"unformatted_files","type":"int","value":0},{"name":"unbuildable_packages","type":"int","value":0},{"name":"vet_findings","type":"int","value":0},{"name":"failed_tests","type":"int","value":0},{"name":"failed_packages","type":"int","value":0}]}

thresholds:

{"failed_packages":0,"failed_tests":0,"unbuildable_packages":0,"unformatted_files":0,"vet_findings":0}

Closes #41