Skip to content

fix(cli): guard bulk/overwrite CLI ops against partial state and clobbering (#778) - #886

Merged
frankbria merged 2 commits into
mainfrom
fix/778-cli-bulk-overwrite-guards
Jul 23, 2026
Merged

fix(cli): guard bulk/overwrite CLI ops against partial state and clobbering (#778)#886
frankbria merged 2 commits into
mainfrom
fix/778-cli-bulk-overwrite-guards

Conversation

@frankbria

@frankbria frankbria commented Jul 23, 2026

Copy link
Copy Markdown
Owner

Closes #778.

What changed

Three of the issue's four sub-problems needed fixes; the fourth was already shipped.

  1. tasks set --all no longer aborts mid-loop on an invalid transition. Failures are collected per task and reported (Updated N / Skipped M / K failed with task ids). Valid transitions are always applied; total failure (nothing updated, failures present) exits 1 so scripts don't read it as success. Single-task mode keeps the loud error (exit 1).
  2. init --generate-config no longer clobbers an existing CODEFRAME.md. It warns and preserves the file; the new --force flag makes the overwrite explicit.
  3. batch follow no longer double-counts completed tasks. It resumed from the oldest recent batch event (list_recent is newest-first but the code indexed [-1]), replaying history into progress counters already seeded from batch.results. It now resumes from the newest event, and seeds in-flight tasks (STARTED event, no terminal result) so the running count and ETA are correct when attaching mid-batch.
  4. Ambiguous batch id (stop/status/resume/follow) was already fixed by [P2.26] CLI batch resolution/listing truncates at 100 batches (same class as #743) #825 — all four subcommands error with a candidate list; regression-tested in tests/core/test_batch_truncation.py. No change needed; verified in the demo below.

Review

  • CodeRabbit (committed diff vs main): 0 findings.
  • Cross-family review (opencode / GLM): 0 critical/major, 3 minor, 3 nit. Accepted and fixed: in-flight tasks invisible after follow attach (real display regression of the first version of this fix); all-fail bulk update exiting 0. Declined with reasoning: catching ValueError for concurrently-deleted tasks mid-loop (needs a concurrent writer; pre-existing), the microsecond attach race (self-healing — final summary re-reads from disk), erroring on a lone --force (matches repo modifier-flag convention).

Demo evidence (per acceptance criterion)

Criterion Action Outcome evidence Status
Bulk update reports partial progress cf tasks set status DONE --all over READY/IN_PROGRESS/BACKLOG tasks Output Updated 1 … 2 failed (invalid transition) with task ids; tasks list afterwards shows only IN_PROGRESS→DONE applied, others untouched VERIFIED
Total bulk failure is not silent cf tasks set status BACKLOG --all (unreachable from all states) Updated 0 … 2 failed, exit code 1 VERIFIED
Config write guarded by --force Re-run cf init . --generate-config over a hand-edited file Warning printed; cat shows content unchanged; --force re-run regenerates it (front matter shown) VERIFIED
Ambiguous batch id errors with candidates cf work batch status aa with two aa… batches Multiple batches match 'aa' + both candidates, exit 1 VERIFIED
follow resumes from newest event Attach to a RUNNING batch with 3 historical events; emit terminal event 3 s later Only the new COMPLETED 2/2 event printed — no historical replay, count correct VERIFIED

Full narrated demo (Showboat, real command outputs): session scratchpad demo-778.md.

Tests

  • 10 new tests (TDD — written first, confirmed red): bulk partial progress ×4, overwrite guard ×2, follow since_id + in-flight seeding ×2, plus mutation-check verified all fail against main's app.py.
  • ruff clean, strict mypy clean on changed files.

Known limitations

  • follow reconstructs in-flight tasks from the last 100 events; a task started earlier than that window shows as pending until its terminal event (commented at the site).
  • Bulk update catches only InvalidTransitionError; a task deleted concurrently mid-loop would still abort (pre-existing, needs a concurrent writer).
  • Partial bulk failure (some updated, some failed) exits 0 by design — only total failure is script-visible via exit code; the per-task failure report is the signal for partial breakage.

…bering (#778)

- tasks set --all: an invalid transition no longer aborts the loop
  mid-way leaving partial state; failures are collected and reported
  (Updated N / Skipped M / K failed). Single-task mode still errors.
- init --generate-config: existing CODEFRAME.md is preserved with a
  warning; new --force flag overwrites explicitly.
- batch follow: resume tailing from the newest event instead of
  replaying history into progress counters already seeded from
  batch.results (completed tasks were counted twice).

Ambiguous batch-id handling (stop/status/follow) was already fixed by
#825 and is regression-tested in tests/core/test_batch_truncation.py.
- batch follow: seed in-flight tasks (BATCH_TASK_STARTED without a
  terminal result) so the running count and ETA are correct when
  attaching mid-batch — batch.results only records terminal states.
- tasks set --all: exit 1 when every transition failed (total failure
  must not look like success to scripts); partial success stays 0.
@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Claude finished @frankbria's task in 5m 18s —— View job


GLM precision review — bug hunt

Reviewed only the production change in codeframe/cli/app.py (3 hunks) against its callers/callees. Style/coverage/test scope is out of scope (covered by the general review).

  • gh pr diff + existing comments (none with findings to repeat)
  • init clobber guard — verified
  • tasks_set bulk-failure handling — verified against state_machine + tasks.update_status
  • batch follow since_id + in-flight seeding — verified against events.tail/list_recent + conductor ordering

No defects found. Summary posted via gh pr comment.

@frankbria

Copy link
Copy Markdown
Owner Author

Cross-family review (opencode / GLM) — final pre-merge pass

Reviewer: zai-coding-plan/glm via opencode, run against the full PR diff.

I'll verify both fixes against the actual codebase to confirm correctness.Let me verify the BatchProgress class to confirm seeding semantics, and check how record_task_start interacts with the seeded state during the subsequent tail loop.Let me verify InvalidTransitionError is raised by tasks.update_status and its message format, plus confirm the test assertions hold.Let me confirm the conductor emits the full batch.id in BATCH_TASK_STARTED payloads (the seeding equality check depends on it) and that events.list_recent ordering is newest-first.## Verdict

Fix #1 — in-flight task seeding (batch follow): CORRECT

  • since_id = recent_events[0].id correctly resumes from the newest event (any type), so nothing historical is replayed into counters already seeded from batch.results. Verified list_recent is ORDER BY id DESC (newest-first), so [0] is the newest.
  • The seeding guard is sound: task_id not in batch.results excludes terminal tasks (COMPLETED/FAILED/BLOCKED all live in results), so they can't be double-counted as running.
  • setdefault over a newest-first iteration correctly retains the latest STARTED timestamp for retried tasks (conductor.py emits full batch.id, so the equality check holds in prod, not just the test).
  • running_tasks derives from len(task_start_times), and pending = remaining - running, so seeded tasks display consistently across both display modes.
  • Duration math benefits: record_task_complete uses the real evt.created_at as the start, giving accurate elapsed time.

Fix #2 — all-fail bulk update exits 1: CORRECT

  • InvalidTransitionError is the exact type raised by tasks.update_statusvalidate_transition (state_machine.py:87), and its message begins "Invalid transition:", matching the single-task assertion.
  • Single-task mode re-raises (if not all_tasks_flag: raise) → propagates to the except Exception handler → Exit(1). Preserved.
  • Bulk mode collects failures; if not updated_count: raise typer.Exit(1) ensures total failure can't masquerade as success. The skipped-only case (0 updated, 0 failed) correctly stays exit 0.
  • failed stores the pre-update Task, so the reported {old} -> {new} is accurate.

New findings

  • Minorapp.py:2265: partial failure (some updated, some failed) still exits 0. Defensible design (only total failure is treated as script-visible failure), but a CI/script consuming exit code won't notice partial breakage. Worth a one-line note in --help or docs; not blocking.
  • No critical/major regressions introduced. The 100-event "ponytail" for very old in-flight tasks is already self-documented in-code, so I'm not counting it as a finding.

Recommend merge.

@github-actions

Copy link
Copy Markdown
Contributor

GLM review: no defects found.

Scope: the three production hunks in codeframe/cli/app.py (init --generate-config clobber guard, tasks_set bulk invalid-transition handling, batch follow since_id + in-flight seeding). Verified against core/state_machine.py, core/tasks.py:update_status, core/events.py (tail/list_recent), core/progress.py, and core/conductor.py emit-vs-results ordering.

Notes on the highest-risk spots (all clean):

  • InvalidTransitionError subclasses Exception (not ValueError), so in single-task mode it correctly propagates to the outer except Exception → exit 1; in bulk mode it is caught locally and the loop continues. Exit-code logic (failed and not updated_count → exit 1) matches the stated contract.
  • tail is exclusive (event.id > last_id), so since_id = recent_events[0].id replays zero historical events — no double-count path exists. Every conductor task-completion site persists batch.results (_save_batch) before emitting BATCH_TASK_COMPLETED, so the single dropped newest event's outcome is always already in the seeded counters.
  • In-flight seeding (task_start_times.setdefault) is consistent with record_task_start (both set task_start_times), and the task_id not in batch.results guard prevents completed tasks from being double-counted as running.

View job run

@frankbria
frankbria merged commit 8c520d3 into main Jul 23, 2026
10 of 11 checks passed
@frankbria
frankbria deleted the fix/778-cli-bulk-overwrite-guards branch July 23, 2026 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[P3.7] Bulk/overwrite CLI ops are non-atomic or overwrite without confirmation — batch fix

1 participant