Skip to content

feat: resolve a task's change set and add 'bernstein undo --dry-run' (#2919) - #5160

Merged
chernistry merged 8 commits into
mainfrom
run-20260901T2145Z-issue2919
Sep 2, 2026
Merged

feat: resolve a task's change set and add 'bernstein undo --dry-run' (#2919)#5160
chernistry merged 8 commits into
mainfrom
run-20260901T2145Z-issue2919

Conversation

@chernistry

@chernistry chernistry commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

What

Resolves, read-only, the exact set of paths one task changed, and surfaces it as bernstein undo <task_id> --dry-run.

New module src/bernstein/core/worktrees/change_set.py:

  • resolve_task_change_set(repo_root, task_id) -> TaskChangeSet
  • TaskChangePath — one path with its change_type and its pre-/post-change blob hash
  • TaskChangeSetUnresolved — raised when the set cannot be determined

New flag on bernstein undo: --dry-run prints that set and returns before any git write.

This PR does not revert anything. _find_commits_to_revert and _execute_reverts are untouched, and bernstein undo without --dry-run behaves exactly as before. No fresh-worktree restore, no later-task conflict detection, no signed reversal receipt, no bernstein task revert command.

Why

Reverting an agent's work needs a precise, reproducible answer to what did this task change. An agent task is one logical change spread over several files, so reverting it by picking files or reverting a merge routinely misses part of the change or reverts unrelated work that landed nearby.

Neither existing source can supply that answer:

Source Why it cannot answer
Commit subjects _find_commits_to_revert (src/bernstein/cli/commands/undo_cmd.py) matches task:<task_id> across the last 50 subjects. Nothing in the tree writes that string — the agent commit prompts emit [WIP] <title> (core/agents/spawner_core.py:589) and feat: <summary> (:1110). The scan finds nothing for a real task.
Lineage spine A LineageSpine entry (core/lineage/spine.py) carries artifact_path, content_hash, actor, step_id, model, timestamp — and no task id. record_artifact_write's own docstring notes that CLI-adapter subprocess file writes never cross that boundary.

The per-task worktree does know. Each task runs on its own agent/<session_id> branch, and the session-to-task binding is already recorded in .sdd/runtime/pids/<session_id>.json and surfaced by classify_worktrees.

How

  1. classify_worktrees(repo_root) maps task_id to session_id; the branch is agent/<session_id>.
  2. git merge-base main agent/<session_id> fixes where the task forked.
  3. git diff --raw --no-renames -z --abbrev=64 <merge_base>..<branch> yields the changed paths in git path order, each with its pre- and post-change blob hash. --no-renames because rename detection reports only a rename's destination and a reversal has to restore the source path too. --abbrev=64 asks for more hexdigits than any hash git uses, which git clamps to the full object name, so the hashes are never abbreviated prefixes.
  4. Git's all-zero blob sentinel becomes None, so an added path's missing pre-image cannot be mistaken for a content hash.
  5. --dry-run renders that set and returns before _find_commits_to_revert is reached.

Three dots, not two. main..agent/<sid> reports every path that landed on main after the task forked as a deletion the task never made. Diffing from the merge base excludes it. Test 3 pins exactly this.

Refusals, not empty answers. A task with no worktree, a task claimed by two worktrees, a missing branch, and a failed or timed-out git call all raise TaskChangeSetUnresolved. An empty set is a real answer — a task that touched no files — so "we could not look" must not be returned in its shape. This mirrors IncomingChangeUnreadable in core/agents/spawner_merge.py.

undo_cmd is imported eagerly by cli/main.py, so the new import is kept cheap: change_set pulls only classifier (stdlib only) and git_basic, not the git_ops facade, which would drag git_pr and the GitHub module onto every CLI startup. Importing change_set costs 0.05s and loads neither.

Open decision, decided here. The issue left --dry-run's relationship to --all open. --dry-run now requires a task id, and --all --dry-run is a click.UsageError. A change set is a property of one task; --all names none, so the alternative would be an empty report that reads as "this session changed nothing" — the failure mode the refusals above exist to prevent. An unresolvable task id likewise exits non-zero with the reason rather than printing an empty panel.

Tests

tests/unit/test_task_change_set.py — a real repo with a real linked worktree: the task branch adds added.txt, modifies mod.txt, deletes del.txt; main separately gains integ.txt. The task's commit subject is feat: add feature, carrying no task:<id> marker, so a subject-based resolver would return nothing here.

All six failed before the change (ModuleNotFoundError: No module named 'bernstein.core.worktrees.change_set' at import; the two CLI tests additionally had no --dry-run flag to invoke).

  1. test_change_set_names_exactly_the_paths_the_task_changed — the set is the task's three paths, in path order, with the right change kinds; asserts first that the fixture hands the resolver no subject marker.
  2. test_change_set_records_pre_and_post_blob_hashes_for_each_path — hashes match git rev-parse <rev>:<path>; an added path's pre_hash and a deleted path's post_hash are None.
  3. Load-bearing: test_integration_only_path_is_absent_from_the_change_set — asserts the two-dot trap reproduces (integ.txt appears in main..agent/<sid>), then that the resolved set excludes it. Every later conflict check compares this set against what other work touched, so an integration-only path leaking in would make a reversal restore a file the task never removed and still report a clean run.
  4. test_unknown_task_id_is_refused_not_answered_with_an_empty_set — raises, naming the task id.
  5. test_dry_run_prints_the_change_set_and_leaves_the_tree_byte_identicalgit status --porcelain byte-identical before and after, HEAD unmoved, the three paths printed and integ.txt not.
  6. test_dry_run_without_a_task_id_is_refused--all --dry-run exits non-zero.

Also run green: tests/unit/cli/test_undo_cmd.py (existing undo behaviour unchanged) and tests/unit/test_worktree_classifier.py.

Checklist

  • uv run ruff check src/ — clean
  • uv run ruff format --check src/ — clean
  • uv run pyright src/bernstein/core/worktrees/change_set.py src/bernstein/cli/commands/undo_cmd.py — no new errors; mypy clean on both
  • uv run python scripts/run_tests.py -x tests/unit/test_task_change_set.py tests/unit/cli/test_undo_cmd.py tests/unit/test_worktree_classifier.py — 20 passed
  • uv run python scripts/run_tests.py tests/unit/cli/ tests/unit/test_task_change_set.py tests/unit/test_worktree_classifier.py tests/unit/test_worktrees_cmd.py tests/unit/test_tui_worktree_status.py tests/unit/test_cli_command_registration.py tests/unit/scripts/test_run_tests_affected_gate.py tests/unit/test_sonar_s3358_nested_ternary.py — 61 of 62 files pass. tests/unit/cli/test_run_banner.py fails with a subprocess TimeoutError on this machine, and fails identically on an unmodified origin/main checkout — environmental, not from this change. The full --affected origin/main set is 1533 files (cli/main.py imports undo_cmd eagerly, so the reverse-dependency closure is nearly the whole suite); it did not finish locally, so CI runs it.
  • Type hints on every new function and dataclass field
  • User-visible behaviour: docs/reference/cli-reference.md row for bernstein undo updated (its source path was stale too)
  • Operator workflow: new docs/operations/task-revert.md, wired into mkdocs.yml nav
  • N/A — docs/api/: no public API schema changed
  • Architecture / new module: uv run bernstein agents-md sync run, produced no changes
  • N/A — no new test layer; this lands in tests/unit/

Part of #2919

Remaining

  • Reversal in a fresh worktree: restore each path to its recorded pre-task blob and verify the resulting diff is the inverse of the change set.
  • Later-task conflict detection: a path changed since by a different task blocks that path and names the blocking task, the rest revert — no silent partial revert.
  • Signed reversal receipt binding {reverted_task_id, reverted_change_set_hash, pre_task_content_hashes, revert_commit}, anchored in the audit chain and verifiable offline, plus its tamper test.
  • The bernstein task revert <task_id> command surface.

chernistry and others added 5 commits September 2, 2026 02:42
Six tests over a real repo with a real linked worktree: the resolved set
is exactly the task branch's paths with their pre/post blob hashes, a
path changed only on the integration branch is absent, an unresolvable
task is refused rather than answered with an empty set, and the dry run
leaves the tree byte-identical.

The integration-only case is load-bearing. A two-dot diff reports that
path as a deletion the task never made, so any later revert built on
such a set would restore a file the task never removed.
Neither existing source can say what one task changed. The commit-subject
scan in undo matches 'task:<id>', which nothing in the tree writes - agent
commits read '[WIP] <title>' and 'feat: <summary>'. A lineage spine entry
carries artifact_path, content_hash, actor and step_id and no task id, and
CLI-adapter subprocess writes never reach that boundary.

The per-task worktree does know: the session-to-task binding is already in
.sdd/runtime/pids/<session>.json and surfaced by classify_worktrees, and
the task's commits live on agent/<session>. resolve_task_change_set maps
task to session, then diffs the branch three-dot against the integration
branch so the set holds only what the task itself changed, each path with
its pre- and post-change blob hash.

Unresolvable cases raise instead of returning an empty set: an empty set
is a real answer, so 'we could not look' must not look like 'nothing is
there'.
Prints one line per changed path - kind, path, abbreviated pre -> post
blob - and returns before any git write, so status, index and HEAD are
untouched.

--dry-run requires a task id: a change set belongs to one task and --all
names none, so the combination is a usage error rather than an empty
report that would read as 'this session changed nothing'. An
unresolvable task id exits non-zero with the reason.
Covers why commit subjects and the lineage spine cannot name a task's
files, what the resolver returns, why the diff is three-dot, and the
refusal cases. States plainly that the reversal itself is not implemented
yet and that undo without --dry-run still uses the subject scan.
The git_ops facade re-exports it but also pulls in git_pr, and undo_cmd is
imported eagerly by cli/main - so the facade would put the GitHub PR module
on every CLI startup for a read-only diff. git_pr itself imports run_git
from git_basic for the same reason.
@bernstein-orchestrator

Copy link
Copy Markdown
Contributor

VERDICT: request-changes

Missing release-notes fragment for a user-visible CLI change.

# Finding Where Smallest fix
1 No docs/release-notes/fragments/ entry for bernstein undo --dry-run src/bernstein/cli/commands/undo_cmd.py (new --dry-run flag) Add fragment per convention (see docs/release-notes/fragments/4978-compliance-coverage.md for format); issue number from PR #2919

The new --dry-run flag on bernstein undo is a user-visible CLI surface. G7 and the release-notes rule require a per-change fragment under docs/release-notes/fragments/<issue>-<slug>.md for every user-visible change — new CLI commands, changed output, or security properties. None exists.

Nits (non-blocking):

verify_cli formatting churn. verify_cli/bernstein_verify_receipt/__main__.py and verify.py contain multi-line string concatenations collapsed to single lines (e.g. verify.py:291, verify.py:418, verify.py:465, verify.py:510). These are harmless but outside the PR's scope. Either land them in a separate commit with a clear message, or drop them from this PR. G6 says pre-existing defects in unrelated files should not be attributed to this diff.

test_orchestrator.py blank line. A single trailing blank line was inserted before # --- Reverse task-to-session index ---. Benign; no action needed.

Scope review — all hunks trace to the PR's stated intent:

  • src/bernstein/core/worktrees/change_set.py: new module, core logic.
  • src/bernstein/cli/commands/undo_cmd.py: --dry-run flag wiring.
  • tests/unit/test_task_change_set.py: 6 tests covering the load-bearing three-dot vs two-dot distinction, refusal semantics, and CLI dry-run behavior. All 6 pass.
  • docs/operations/task-revert.md: new docs for the feature.
  • docs/reference/cli-reference.md: updated undo entry.
  • mkdocs.yml: nav entry for the new doc.

Logic review — the three-dot diff is correct and load-bearing:

  • main...agent/<sid> (three-dot) diffed against the merge base excludes paths that landed on main after the task forked.
  • test_integration_only_path_is_absent_from_the_change_set verifies this: integ.txt appears in a two-dot diff but not in the resolved set. Without it, a reversal would restore files the task never touched.
  • TaskChangeSetUnresolved is raised (not an empty set) for unknown tasks, ambiguous bindings, and git failures — empty set is a real answer for a task that touched nothing.
  • --abbrev=64 ensures full object names, never abbreviated prefixes.
  • --no-renames avoids rename-detection ambiguity for reversals.
  • _blob_or_none maps git's all-zero sentinel to None.

Tests as evidence — all 6 tests pass. The fixture deliberately avoids writing task:<id> into commit subjects (the real shape agents produce), proving the resolver does not depend on that broken path. test_dry_run_prints_the_change_set_and_leaves_the_tree_byte_identical verifies git status --porcelain is unchanged and HEAD is untouched.

Security — read-only git operations only. No new network calls, no credential access, no permission widening. _GIT_TIMEOUT_S = 30 bounds git hangs.

Hygiene — clean. run_git imported from git_basic where it lives. Module is small, focused, and well-documented.

Release notes — blocking finding #1. The fragment must use the PR's closing keyword to an open issue. Per the fragment convention (see 4978-compliance-coverage.md), the body should end with the issue number in parentheses.


bernstein v3.19.0 - unattended review run run-20260902T032050p1170758Z - no operator in the loop

Signed review receipt - verify with bernstein review-receipt verify

field value
diff_hash sha256:395e29d17079afccbed20f0f35e0ac326ca7dae29cdd6725e759c1607e17043e
journal_entry_hash sha256:de000cce109e2da4c5e97f76ca2f9f92ff017b90146d0c83cb4f1c0079a803c0

@bernstein-orchestrator

Copy link
Copy Markdown
Contributor

VERDICT: approve

Well-scoped feature: reads a task's worktree branch and prints its changed paths as --dry-run output, touching nothing.


Review dimensions examined

  • Scope: all hunks trace to the stated intent (change-set resolution + undo --dry-run). No drive-by edits; verify_cli/ and test_orchestrator.py are touched only by the auto-formatting commit e24921f, confirmed by git log origin/main..HEAD -- <path>.
  • Logic: change_set.py handles all three refusal cases (no worktree, multiple worktrees, git failure/timeout). _blob_or_none correctly converts git's all-zero sentinel to None. Three-dot diff ({merge_base}..{branch}) is load-bearing and documented — two-dot would leak integration-only deletions into the set. _diff_raw splits on NUL bytes and validates record structure before emitting a TaskChangePath.
  • Breaking changes: None. --dry-run is additive; without it, undo_cmd behaves byte-for-byte as before. No public API or config surface altered.
  • Tests as evidence: 6 tests pass. All assertions are behavioral, not implementation-coupled: paths in path-order, blob hashes verified against real git objects, tree left byte-identical after dry-run, unknown task id raises rather than returning empty set, --all --dry-run is refused. Test 3 (test_integration_only_path_is_absent_from_the_change_set) is the load-bearing assertion: it first proves the fixture reproduces the two-dot trap (integ.txt in main..agent/<sid>), then confirms three-dot excludes it.
  • Security: read-only git calls with a 30 s timeout. No network calls, no credential handling, no unsanitized input at trust boundaries.
  • Hygiene: follows existing repo patterns (run_git, classify_worktrees). Naming is consistent. subprocess is imported in change_set.py but unused — harmless, can be cleaned in a follow-up.
  • Release notes: fragment docs/release-notes/fragments/2919-undo-dry-run.md exists with a heading and a closing paragraph ending in (#2919), matching the issue number in the PR title.

Verification run

uv run pytest tests/unit/test_task_change_set.py -v
# 6 passed in 2.92s

uv run ruff check src/bernstein/core/worktrees/change_set.py src/bernstein/cli/commands/undo_cmd.py tests/unit/test_task_change_set.py
# All checks passed

uv run mypy src/bernstein/core/worktrees/change_set.py src/bernstein/cli/commands/undo_cmd.py
# clean (only pyproject.toml unused-section note)

Nits (non-blocking)

  • change_set.py imports subprocess but never uses it. Safe to remove in a follow-up.
  • Release-notes fragment lacks a trailing newline. Cosmetic but worth fixing.
  • verify_cli/ and tests/unit/test_orchestrator.py diffs are purely formatting noise from commit e24921f, unrelated to the feature.

bernstein v3.19.0 - unattended review run run-20260902T041046p1330468Z - no operator in the loop

Signed review receipt - verify with bernstein review-receipt verify

field value
diff_hash sha256:83f88edc1c8fb8d8c9998e6265927c312474ed592e78c9fbf66fd3e32fe751da
journal_entry_hash sha256:e1a0c46c0716f77fde3be0c9a246aca7a830f50424df34f96f39bc9e983c4f2d

@bernstein-orchestrator bernstein-orchestrator Bot added the fleet-approved Reviewed, fixed and verified by the unattended contour label Sep 2, 2026
@bernstein-orchestrator
bernstein-orchestrator Bot marked this pull request as ready for review September 2, 2026 04:30
@chernistry
chernistry added this pull request to the merge queue Sep 2, 2026
@github-actions
github-actions Bot requested a review from Chirag6722 September 2, 2026 04:31
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 2, 2026
@chernistry
chernistry added this pull request to the merge queue Sep 2, 2026
@chernistry chernistry added the fleet-blocked Unattended fix budget exhausted; needs an operator label Sep 2, 2026
Merged via the queue into main with commit 5139e4f Sep 2, 2026
69 of 70 checks passed
@chernistry
chernistry deleted the run-20260901T2145Z-issue2919 branch September 2, 2026 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cli core docs fleet-approved Reviewed, fixed and verified by the unattended contour fleet-blocked Unattended fix budget exhausted; needs an operator size/l tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant