P0: main went red
Workflow: CI concluded failure on main.
Failing run: https://github.com/edobry/minsky/actions/runs/32422455824
HEAD SHA: c7662f2b6e6446514f01f3d570d3580b1fccf950
Head commit: fix(mt#4307): Roll back a conflicted stash pop instead of corrupting the tree
Summary
A failed git stash pop left conflict markers in the working tree, and the next gate to run
failed for a reason that named the downstream symptom instead of the cause. In the originating
incident session_commit landed a clean commit, popped the update-parked stash, hit a conflict,
and then pushed — so the pre-push gated suite ran against a corrupted tree. Twenty
src/cockpit/** tests failed identically on JSON Parse error: Unrecognized token '<', none of
them related to the change, because src/generated/interceptor-catalog.json now began with a
marker. pushError carried that test output; the actual cause sat in stashRestore.error on the
same payload.
Three arms: stop the corruption at its source, catch it if it ever happens anyway, and stop a
sibling defect in the same family from mislabelling its own errors.
Key Changes
A — a conflicted stash pop is rolled back (packages/domain/src/session/session-stash-restore.ts)
A failed pop has two shapes that throw identically, and they need opposite handling:
- Refused — "your local changes would be overwritten". git compared, disliked what it saw, and
touched nothing. Retrying after clearing the blocker is correct; that is the existing
generated-file path.
- Conflicted — git attempted the merge, wrote markers into every overlapping file, and recorded
them unmerged. The tree is now corrupt and retrying cannot help.
The code classifies on git diff --diff-filter=U and undoes a conflicted pop with
git reset --hard HEAD. That is safe here for two stated reasons, not by assumption: every caller
runs immediately after a merge commit, when the tree is clean at HEAD — so HEAD is the pre-pop
state — and the work is still parked in the stash, which is asserted before the reset rather
than trusted to git's documented behaviour. If the stash entry did not survive, the markers are
the only copy of that work, so the reset is refused and the outcome says so plainly.
B — a pre-commit conflict-marker check (src/hooks/conflict-marker-detector.ts)
A NUL byte and a conflict marker are the same class of "this file was never meant to be committed
in this state", and only one of them was checked.
The discrimination was measured rather than assumed: zero files repo-wide carry any of git's
three markers at line start, so an open or close marker fires on sight. A bare ======= does not —
a 7-character Markdown setext underline is indistinguishable from a conflict separator by the line
alone, so it fires only with a corroborating marker in the same file. Markdown-family files exempt
an isolated marker inside a fenced block so docs can quote one, but a complete block fires
even fenced, because a conflict can unbalance the fences around it and a corrupted .mdc rule file
is exactly what started this.
Deliberately does not skip src/generated/**. Several siblings do; that is where the corruption
was invisible.
C — the error that was manufactured and then destroyed (packages/domain/src/session/commands/pr-command.ts)
sessionPr wrapped its whole body in one try whose catch turned any ValidationError into
ResourceNotFoundError("No session detected…"). A bodyPath relative to the session workspace
could not be read, the code raised the exact, correct, actionable message naming the file, and
forty-nine lines later the same function replaced it with a claim that was false — and misleading in
the way that costs most, since it names as missing the very thing the caller supplied.
session_pr_create failed four times in a row this way, including when passed an explicit
--sessionId, while session_get --task resolved that same session throughout.
Session resolution now owns that guidance and nothing else translates. A relative bodyPath
resolves against the session workspace instead of the process cwd (the main repo, for the MCP
server), and the read error names both the given and the resolved path.
Judgment calls
- SC3 shipped a third mechanism. The criterion offered two implementations — move the update
before the commit, or abort before the gate runs. Neither shipped. Rolling the pop back discharges
the criterion's main clause directly, and the ordering is unchanged because the post-commit moment
is the only one where a pop is legal at all. Aborting the push would have traded a corrupted tree
for a stranded commit. SC3's own text is amended with the reasoning.
- SC4 and SC5 contradict each other, and SC4's bare-
======= arm is now conditional. SC4 asks
for any line matching ^(<{7}|={7}|>{7})( |$) to block; SC5 asks for a Markdown setext =======
underline not to. A setext underline of exactly seven = matches that regex exactly, so no
line-level rule satisfies both. The check therefore evaluates the file: an open or close marker
fires alone — measured at zero occurrences repo-wide, so the fail-closed floor is intact — and a
bare separator fires only with a corroborating marker, which a real conflict block always has. The
originating corruption carried open and close markers, so the unconditional arm catches it. SC4's
own text is amended with this.
- The catch class is two sites, not three. The spec's count came from grepping the message
string, which is not the catch shape. session-conflicts-operations.ts:99 throws that sentence
directly in a branch reached only when the context genuinely is missing — where it is true. Left
alone; corrected in the spec. Same family as this task, one level up.
Testing
bun test over the six affected files: 68 pass, 0 fail. Typecheck clean across 8 projects;
ESLint 0 errors / 0 warnings over 3822 files.
Execution evidence:
$ bun test --preload ./tests/setup.ts packages/domain/src/session/session-stash-restore-real-git.test.ts \
packages/domain/src/session/session-commit-stash-restore.test.ts \
packages/domain/src/session/session-update-stash-restore.test.ts \
packages/domain/src/session/session-update-conflict-markers.test.ts \
src/hooks/conflict-marker-detector.test.ts \
packages/domain/src/session/commands/pr-command-error-preservation.test.ts
68 pass
0 fail
Ran 68 tests across 6 files. [6.63s]
AT1 (conflicted session_update pop leaves no marker, work still recoverable) —
session-stash-restore-real-git.test.ts, real git in throwaway repos, not a fake:
(pass) restoreSessionStash — a CONFLICTED pop is rolled back (mt#4307) > leaves NO conflict markers in any tracked file, and the work stays recoverable
(pass) restoreSessionStash — a CONFLICTED pop is rolled back (mt#4307) > the report names the conflict rather than only 'could not be restored'
(pass) restoreSessionStash — a CONFLICTED pop is rolled back (mt#4307) > refuses to roll back when the stash entry did not survive — markers are then the only copy
The test asserts the marker regex against every tracked file, that the tree equals its pre-pop
content, that git diff --diff-filter=U is empty, and that git stash show -p still contains the
parked work.
AT2 (session_commit reports the pop failure as its primary outcome) and SC3 (the tree the
pre-push gate tests is clean) — session-commit-stash-restore.test.ts, driving the real
sessionCommit against a real repo with a real bare remote:
(pass) sessionCommit — a conflicted post-commit pop (mt#4307) > the commit lands and pushes, and the tree the push saw carries NO markers
(pass) sessionCommit — a conflicted post-commit pop (mt#4307) > the result LEADS with the pop failure rather than burying it in a side field
AT3 (staged marker blocks the commit and names the file) — exercised live against the wired
hook, not only the pure function:
$ printf ... > live-conflict-check.txt && git add live-conflict-check.txt
$ bun run src/hooks/pre-commit.ts
Checking staged files for conflict markers...
Conflict marker(s) detected in staged files. Commit blocked.
live-conflict-check.txt: line(s) 2, 4, 6
EXIT=1
AT4 (setext underline + fenced marker quote does not fire) and SC5 (regex pinned against
both a real conflict and false-positive fixtures) — conflict-marker-detector.test.ts:
(pass) AT4 / SC5 — legitimate content does NOT fire > a setext underline plus a fenced marker quote is clean
(pass) AT4 / SC5 — legitimate content does NOT fire > a setext underline on its own is clean
(pass) AT4 / SC5 — legitimate content does NOT fire > prose quoting a marker inline is clean
(pass) AT4 / SC5 — legitimate content does NOT fire > DISCRIMINATION: the same content OUTSIDE a fence does fire
(pass) AT4 / SC5 — legitimate content does NOT fire > a fence exemption does NOT extend to a complete conflict block
AT5 (the generated-JSON replay) — conflict-marker-detector.test.ts:
(pass) AT5 — the generated file the originating incident hid in > a conflict in src/generated/** is caught at commit time
(pass) AT5 — the generated file the originating incident hid in > generated paths are NOT allowlisted — that is the whole point
Negative control — AT1's scenario really does corrupt the tree:
(pass) restoreSessionStash — a CONFLICTED pop is rolled back (mt#4307) > NEGATIVE CONTROL: a raw pop in this scenario really does write markers
Without this, "no markers afterwards" would be satisfied by a fixture that never produced any. The
control asserts the raw git stash pop fails, that the marker regex matches, and that
git diff --diff-filter=U names the file — so the assertions downstream are the fix talking, not
the setup.
Negative control — arm C, the tests run against the un-fixed file:
$ git restore --source=HEAD -- packages/domain/src/session/commands/pr-command.ts
$ bun test ... pr-command-error-preservation.test.ts
Expected to contain: "PR title must not be empty"
Received: "No session detected. Please provide a session ID (--sessionId), task ID (--task), or run this command from within a session workspace."
0 pass
4 fail
The defect observed verbatim. The fixed file was restored immediately afterwards and the same four
tests pass.
Negative control — AT3, the commit proceeds when the check is absent:
$ MINSKY_SKIP_CONFLICT_MARKER_CHECK=1 bun run src/hooks/pre-commit.ts
[pre-commit:conflict-marker-check] override MINSKY_SKIP_CONFLICT_MARKER_CHECK=1 at 2026-08-20T21:18:18.982Z — conflict-marker check skipped
...
✅ All checks passed! Commit proceeding...
---exit of hook was: 0
Same staged marker file, check disabled, hook runs to completion and exits 0 — i.e. exactly the
behaviour that shipped the corruption. This doubles as the override branch's live exercise, so both
production branches of the new step have been run.
Live verification
Both branches of the new pre-commit step were exercised against the real wired hook (above): the
blocking path names the file and the exact marker lines and exits 1, and the override path
audit-logs the skip and proceeds. The check also ran clean over this PR's own 14 staged files
(No conflict markers detected in 14 staged file(s)), including the detector source and its test —
which contain marker strings deliberately assembled so none sits at column 0.
Registration is complete across all five surfaces: the pre-commit step list, the override in
HOOK_ONLY_ENV_VAR_CATEGORIES plus its known-override-env-vars.ts mirror, known-guard-names.ts,
interceptor-descriptions.ts and interceptor-coordinates.ts. The generated interceptor catalog's
declaredButNotDescribed is empty again at population 139, and the four compile outputs
(CLAUDE.md, AGENTS.md, .cursor/rules/hook-files.mdc, .claude/hooks/*) were regenerated and
each verified to carry the new entry.
Deploy verification: packages/domain/** is bundled into the minsky-mcp image, so after merge I
will run deployment_wait-for-latest with notBefore pinned to the merge timestamp and assert the
health body's service field rather than a bare 200.
Related
- mt#3660 shipped the stash-naming half of this path; its tests all park work on a file disjoint
from the conflict, which is why their pop always succeeds and the conflicted case was never
reached.
- mt#1367 deliberately leaves merge markers in the tree for the agent to resolve. That is a
different path and is untouched — this changes only what a failed stash pop leaves behind.
FakeGitService now answers --diff-filter=U with "". Its catch-all returned "mock git output", which read as one unmerged file of that name and made every failed-pop test look
conflicted — the standing hazard of a fake that answers any command.
Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
What this means
A push to main triggered CI and the workflow above did not conclude success. Per
CLAUDE.md user preference ("main must never be broken"), this is severity-1.
Diagnostic checklist
- Open the failing run URL above; identify which job/step failed.
- Check whether the offending PR was merged with a known-failing required check
(operator-API bypass via gh api PUT /merge despite enforce_admins).
- Confirm
enforce_admins is currently enabled:
gh api repos/edobry/minsky/branches/main/protection --jq .enforce_admins.enabled
true post-mt#1938. If false, that is itself a separate finding.
Recovery
- Open a hotfix branch off current
main.
- Apply the smallest fix that turns CI green (often a formatter pass or a config
flip).
- Land via the standard Minsky session flow:
tasks_create → session_start → session_commit → session_pr_create → /review-pr → session_pr_merge.
- Verify the post-merge
main build is green within ~5 minutes.
- Close this issue with a link to the hotfix PR.
Cross-references
P0: main went red
Workflow:
CIconcluded failure onmain.Failing run: https://github.com/edobry/minsky/actions/runs/32422455824
HEAD SHA:
c7662f2b6e6446514f01f3d570d3580b1fccf950Head commit: fix(mt#4307): Roll back a conflicted stash pop instead of corrupting the tree
Summary
A failed
git stash popleft conflict markers in the working tree, and the next gate to runfailed for a reason that named the downstream symptom instead of the cause. In the originating
incident
session_commitlanded a clean commit, popped the update-parked stash, hit a conflict,and then pushed — so the pre-push gated suite ran against a corrupted tree. Twenty
src/cockpit/**tests failed identically onJSON Parse error: Unrecognized token '<', none ofthem related to the change, because
src/generated/interceptor-catalog.jsonnow began with amarker.
pushErrorcarried that test output; the actual cause sat instashRestore.erroron thesame payload.
Three arms: stop the corruption at its source, catch it if it ever happens anyway, and stop a
sibling defect in the same family from mislabelling its own errors.
Key Changes
A — a conflicted stash pop is rolled back (
packages/domain/src/session/session-stash-restore.ts)A failed pop has two shapes that throw identically, and they need opposite handling:
touched nothing. Retrying after clearing the blocker is correct; that is the existing
generated-file path.
them unmerged. The tree is now corrupt and retrying cannot help.
The code classifies on
git diff --diff-filter=Uand undoes a conflicted pop withgit reset --hard HEAD. That is safe here for two stated reasons, not by assumption: every callerruns immediately after a merge commit, when the tree is clean at HEAD — so HEAD is the pre-pop
state — and the work is still parked in the stash, which is asserted before the reset rather
than trusted to git's documented behaviour. If the stash entry did not survive, the markers are
the only copy of that work, so the reset is refused and the outcome says so plainly.
B — a pre-commit conflict-marker check (
src/hooks/conflict-marker-detector.ts)A NUL byte and a conflict marker are the same class of "this file was never meant to be committed
in this state", and only one of them was checked.
The discrimination was measured rather than assumed: zero files repo-wide carry any of git's
three markers at line start, so an open or close marker fires on sight. A bare
=======does not —a 7-character Markdown setext underline is indistinguishable from a conflict separator by the line
alone, so it fires only with a corroborating marker in the same file. Markdown-family files exempt
an isolated marker inside a fenced block so docs can quote one, but a complete block fires
even fenced, because a conflict can unbalance the fences around it and a corrupted
.mdcrule fileis exactly what started this.
Deliberately does not skip
src/generated/**. Several siblings do; that is where the corruptionwas invisible.
C — the error that was manufactured and then destroyed (
packages/domain/src/session/commands/pr-command.ts)sessionPrwrapped its whole body in onetrywhose catch turned anyValidationErrorintoResourceNotFoundError("No session detected…"). AbodyPathrelative to the session workspacecould not be read, the code raised the exact, correct, actionable message naming the file, and
forty-nine lines later the same function replaced it with a claim that was false — and misleading in
the way that costs most, since it names as missing the very thing the caller supplied.
session_pr_createfailed four times in a row this way, including when passed an explicit--sessionId, whilesession_get --taskresolved that same session throughout.Session resolution now owns that guidance and nothing else translates. A relative
bodyPathresolves against the session workspace instead of the process cwd (the main repo, for the MCP
server), and the read error names both the given and the resolved path.
Judgment calls
before the commit, or abort before the gate runs. Neither shipped. Rolling the pop back discharges
the criterion's main clause directly, and the ordering is unchanged because the post-commit moment
is the only one where a pop is legal at all. Aborting the push would have traded a corrupted tree
for a stranded commit. SC3's own text is amended with the reasoning.
=======arm is now conditional. SC4 asksfor any line matching
^(<{7}|={7}|>{7})( |$)to block; SC5 asks for a Markdown setext=======underline not to. A setext underline of exactly seven
=matches that regex exactly, so noline-level rule satisfies both. The check therefore evaluates the file: an open or close marker
fires alone — measured at zero occurrences repo-wide, so the fail-closed floor is intact — and a
bare separator fires only with a corroborating marker, which a real conflict block always has. The
originating corruption carried open and close markers, so the unconditional arm catches it. SC4's
own text is amended with this.
string, which is not the catch shape.
session-conflicts-operations.ts:99throws that sentencedirectly in a branch reached only when the context genuinely is missing — where it is true. Left
alone; corrected in the spec. Same family as this task, one level up.
Testing
bun testover the six affected files: 68 pass, 0 fail. Typecheck clean across 8 projects;ESLint 0 errors / 0 warnings over 3822 files.
Execution evidence:
AT1 (conflicted
session_updatepop leaves no marker, work still recoverable) —session-stash-restore-real-git.test.ts, real git in throwaway repos, not a fake:The test asserts the marker regex against every tracked file, that the tree equals its pre-pop
content, that
git diff --diff-filter=Uis empty, and thatgit stash show -pstill contains theparked work.
AT2 (
session_commitreports the pop failure as its primary outcome) and SC3 (the tree thepre-push gate tests is clean) —
session-commit-stash-restore.test.ts, driving the realsessionCommitagainst a real repo with a real bare remote:AT3 (staged marker blocks the commit and names the file) — exercised live against the wired
hook, not only the pure function:
AT4 (setext underline + fenced marker quote does not fire) and SC5 (regex pinned against
both a real conflict and false-positive fixtures) —
conflict-marker-detector.test.ts:AT5 (the generated-JSON replay) —
conflict-marker-detector.test.ts:Negative control — AT1's scenario really does corrupt the tree:
Without this, "no markers afterwards" would be satisfied by a fixture that never produced any. The
control asserts the raw
git stash popfails, that the marker regex matches, and thatgit diff --diff-filter=Unames the file — so the assertions downstream are the fix talking, notthe setup.
Negative control — arm C, the tests run against the un-fixed file:
The defect observed verbatim. The fixed file was restored immediately afterwards and the same four
tests pass.
Negative control — AT3, the commit proceeds when the check is absent:
Same staged marker file, check disabled, hook runs to completion and exits 0 — i.e. exactly the
behaviour that shipped the corruption. This doubles as the override branch's live exercise, so both
production branches of the new step have been run.
Live verification
Both branches of the new pre-commit step were exercised against the real wired hook (above): the
blocking path names the file and the exact marker lines and exits 1, and the override path
audit-logs the skip and proceeds. The check also ran clean over this PR's own 14 staged files
(
No conflict markers detected in 14 staged file(s)), including the detector source and its test —which contain marker strings deliberately assembled so none sits at column 0.
Registration is complete across all five surfaces: the pre-commit step list, the override in
HOOK_ONLY_ENV_VAR_CATEGORIESplus itsknown-override-env-vars.tsmirror,known-guard-names.ts,interceptor-descriptions.tsandinterceptor-coordinates.ts. The generated interceptor catalog'sdeclaredButNotDescribedis empty again at population 139, and the four compile outputs(
CLAUDE.md,AGENTS.md,.cursor/rules/hook-files.mdc,.claude/hooks/*) were regenerated andeach verified to carry the new entry.
Deploy verification:
packages/domain/**is bundled into theminsky-mcpimage, so after merge Iwill run
deployment_wait-for-latestwithnotBeforepinned to the merge timestamp and assert thehealth body's
servicefield rather than a bare 200.Related
from the conflict, which is why their pop always succeeds and the conflicted case was never
reached.
different path and is untouched — this changes only what a failed stash pop leaves behind.
FakeGitServicenow answers--diff-filter=Uwith"". Its catch-all returned"mock git output", which read as one unmerged file of that name and made every failed-pop test lookconflicted — the standing hazard of a fake that answers any command.
Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
What this means
A push to
maintriggered CI and the workflow above did not conclude success. PerCLAUDE.md user preference ("main must never be broken"), this is severity-1.
Diagnostic checklist
(operator-API bypass via
gh api PUT /mergedespiteenforce_admins).enforce_adminsis currently enabled:truepost-mt#1938. Iffalse, that is itself a separate finding.Recovery
main.flip).
tasks_create → session_start → session_commit → session_pr_create → /review-pr → session_pr_merge.mainbuild is green within ~5 minutes.Cross-references
.claude/hooks/SPEC.md§Layered enforcement model — three-layer model.github/workflows/main-watch.yml.