`StoryTask.from_dict` ingests `state.json`'s `baseline_commit` unvalidated — a symbolic value reaches the orchestrator's `git reset --hard`

[pbean]

Summary

StoryTask.from_dict reads state.json's baseline_commit with no coercion and no validation:

# src/bmad_loop/model.py:466
baseline_commit=d.get("baseline_commit"),

state.json lives at .bmad-loop/runs/<id>/ — inside the project tree a coding-CLI session can write. Whatever string lands in that key becomes task.baseline_commit verbatim, and task.baseline_commit is passed directly as a git revision argument by the orchestrator, including verify.safe_rollback's git reset --hard (recovery_flow.py:991/:1011 → verify.py:1464).

A symbolic value therefore flows unchecked into orchestrator git calls:

• "HEAD" → git reset --hard HEAD — the rollback is a silent no-op, and safe_reset still returns success.
• "main~2" → the rollback resets history the run never touched.

All line numbers below are against f45a0295 (current main).

Why this field, specifically

An earlier framing of this finding said baseline_commit was bare "unlike every sibling field". That is not true and should not be repeated — an AST partition of all 43 keyword arguments in StoryTask.from_dict finds nine bare pass-throughs, not one: story_key, baseline_commit, spec_file, dispatched_spec_file, commit_sha, defer_reason, preserve_ref, restore_patch, bundle_file. (Its two immediate neighbours do coerce — baseline_untracked → [str(p) ...], baseline_ledger_digest → str(...) — which is what made the "every sibling" reading look right.)

The accurate argument is a stronger one: baseline_commit is not unusual in its ingress, it is unusual in its consumers. No other bare field is fed to git reset --hard.

And this ingress is the only way a non-sha can enter the field. Every attribute store for it is git-derived or None — there are exactly four, by AST enumeration over src/:

Site	Assignment
engine.py:1896	verify.rev_parse_head(self.workspace.root)
runs.py:1587	head (= verify.rev_parse_head(repo))
sweep.py:907	None
sweep.py:913	verify.rev_parse_head(self.workspace.root)

model.py:466 is the sole non-git-derived ingress.

Reproduction

A hand-written state.json → journal.load_state → RunState.from_dict → StoryTask.from_dict round-trips the value unchanged. The coerced sibling fed the identical input stringifies it, which isolates the missing str() as a clean control pair:

state.json value	baseline_commit	baseline_ledger_digest (control)
"HEAD", "HEAD~3", "main", "@{u}", ":/fixup"	passes through as str	same
12345	12345 (int)	'12345' (str)
True / 3.5 / ["HEAD"] / {"a": 1}	unchanged type	stringified

The field is annotated str | None; an int reaching it violates the annotation silently. model.py is pyright-strict, but pyright cannot see through d: dict[str, Any] — the module header suppresses reportUnknownArgumentType for exactly this reason.

Captured at the _run_git chokepoint, going the full route (hand-written state.json → load_state → RecoveryFlow.safe_reset → recovery_flow.py:1011 → verify.safe_rollback → verify.py:1464):

['git','-C','.../sandbox','reset','--hard','HEAD']

Disjoint ablation pair, identical setup, only the state.json value differing:

state.json baseline_commit	HEAD after rollback	failed attempt's file survived?
"HEAD"	unmoved (d68faec…)	yes — rollback silently no-opped
real sha (c74b026…)	rewound to baseline	no

safe_reset returns success in both cases. The orchestrator believes it rolled back and proceeds on a tree still holding the failed attempt.

Consequences beyond the no-op rollback

1. The operator-facing pause notice is falsified. recovery_flow.py:991 → pause_for_manual_recovery → commits_above (verify.py:950, git rev-list <B>..HEAD). With "HEAD", rev-list HEAD..HEAD is empty, so commits == [] and the notice falls to the else shape at recovery_flow.py:1476 ("manual rollback needed") instead of the elif commits shape at :1459 — the one whose entire purpose is to tell the operator "They may already be integrated or pushed to a remote — do NOT reset before checking." The notice then instructs the human to run git -C "<root>" reset --hard HEAD (:1488, short = baseline[:12] at :1390), quoting the poisoned value back at them as a manual step that is itself a no-op.

2. The one-commit-per-story invariant breaks. engine.py:2859 → verify.finalize_commit → verify.py:3025 git reset --soft <baseline>. With "HEAD" the soft reset no-ops, so the squash leaves three commits instead of two, and task.commit_sha (engine.py:2860) then records a sha whose history is not what the orchestrator asserts.

3. A non-string escapes the error taxonomy entirely. 12345 reaches subprocess.run's argv intact and dies as TypeError — not GitError, GitSpawnError, OSError, TimeoutExpired, or UnicodeDecodeError. It therefore passes straight through every except GitError guard on the rollback path. That is a direct breach of the "fail loud at boundaries / typed escalation over bare except" doctrine, and it is reachable only through an uncoerced field.

Reachability (honest scoping)

engine.py:1896 re-stamps task.baseline_commit = verify.rev_parse_head(...) at the start of every fresh dev phase, so a poisoned value cannot survive a normal story start. The live window is resume / crash recovery, which acts on the persisted value before any re-stamp: engine.py:1402-1406 → _rollback_or_pause → safe_reset → safe_rollback — precisely the path measured above. sweep.py:822/847/994 reach the same collaborator.

This is unit-level reachability (sandbox fixtures, real _run_git chokepoint spied but not stubbed), not a full end-to-end run.

Ingress→argv trace: src/ holds 55 .baseline_commit load sites; of those, 25 direct + 10 alias sites reach a git revision argument (the rest are truthiness guards). Destructive: verify.py:1464 (reset --hard), verify.py:3025 (reset --soft), verify.py:693-695 (reset --quiet <rev> -- <path>). The rest are observational (diff --quiet, rev-list, merge-base --is-ancestor, cat-file) — but observational failures here choose notice shapes and gate outcomes, as consequence 1 shows.

Proposed fix

The sharpest fact is that the codebase already owns the validator and applies it to only one side of a comparison. In _verify_shared_gates:

# verify.py:2165  — the session's FRONTMATTER claim
claimed_baseline = str(fm.get("baseline_commit", fm.get("baseline_revision", ""))).strip()
...
# verify.py:2170  — canonicalized through the validator
canonical_claimed = _canonical_commit_oid(paths.project, claimed_baseline)
...
# verify.py:2178  — compared against a RAW, unvalidated task.baseline_commit
if canonical_claimed != task.baseline_commit:

The gate canonicalizes the untrusted frontmatter claim through _canonical_commit_oid (verify.py:344, gated by _OBJECT_ID = re.compile(r"\A[0-9a-fA-F]{7,64}\Z") at :341 plus rev-parse --disambiguate), then compares it against a raw task.baseline_commit that has been through nothing. Reusing _OBJECT_ID / _canonical_commit_oid at the from_dict ingress — or at a shared chokepoint both sides pass through — therefore needs no new mechanism. It is the cheapest available fix.

Minimally: coerce with str() and reject anything _OBJECT_ID does not match, at model.py:466 or a shared helper. Rejecting loudly at load time is consistent with dispatched_spec_snapshot's neighbouring precedent (model.py:447-457 raises ValueError on invalid base64) and with the "repair writes must raise" half of the doctrine.

Severity, and the counterargument stated plainly

This is not being reported as a novel attack. state.json is orchestrator-owned state, and a session that can rewrite it can do other damage — that objection is real and should be weighed.

The finding is about a missing validation chokepoint at a trust boundary: the file sits inside the agent-writable project tree, is re-read on every resume, and feeds a destructive git primitive with no gate between. It mirrors exactly what #645's _canonical_commit_oid did for the frontmatter side of the same comparison — the frontmatter claim is now validated, the persisted claim is not. Consequences 1 and 3 also fire on plain corruption (a truncated or partially-written state.json), with no adversary required.

Related but distinct: #520 covers safe_rollback discarding commits when the baseline is valid but stale. This issue is the case where the baseline is not a commit at all.

Related: an inert copy worth noting

devcontract.py:401 writes the raw frontmatter claim into result.json["baseline_commit"]:

# devcontract.py:390 — the frontmatter READ that feeds it
baseline = str(fm.get("baseline_commit", fm.get("baseline_revision", ""))).strip()
...
# devcontract.py:401 — the copy into result.json
"baseline_commit": baseline,

Nothing in production reads it back. Verified by AST classification (not shell grep — grep is ugrep on this box and exits silently on zero matches, so a shell-grep absence claim is worthless here): exactly six "baseline_commit" string literals exist in src/ — devcontract.py:390 (frontmatter read, upstream of :401), devcontract.py:401 (write), model.py:387 (write → state.json), model.py:466 (read ← state.json, this issue), resolve.py:114 (write → resolve/<key>/context.json), verify.py:2165 (frontmatter read). Zero subscript reads (["baseline_commit"]) anywhere in src/.

Note verify.py:2165 reads frontmatter, not result.json — verify.py's own comment above that line says so — so it is not a consumer of the :401 write.

Honest qualifier: the write is dead to production, not to the test suite. tests/test_devcontract.py:161/245 assert on rj["baseline_commit"], and tests/conftest.py synthesizes it into fixture result dicts (5 sites). The suite pins a value nothing consumes. Not a bug today — a latent trap for a future reader who assumes the key is validated or authoritative.

---

Origin: review of PR #647; recorded here rather than on the merged PR per the standing "findings go to issues" agreement. All claims re-verified against f45a0295.