Skip to content

[AI-7065] Refresh flow resume state in the Togo TUI without restarting the command - #24701

Open
luisorofino wants to merge 3 commits into
loa/openmetrics-ai-genfrom
loa/refresh-flow-resume
Open

[AI-7065] Refresh flow resume state in the Togo TUI without restarting the command#24701
luisorofino wants to merge 3 commits into
loa/openmetrics-ai-genfrom
loa/refresh-flow-resume

Conversation

@luisorofino

@luisorofino luisorofino commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

This is PR 1 of 2 in a stacked series:

  1. Add composable deterministic task validation #24777 <- you are here
  2. Add CHECKPOINTED status to phase nodes to separate it from PENDING #24702

What does this PR do?

Recomputes a flow's resume state whenever a TUI screen becomes active again, and collapses the resume-state API onto a single entry point.

Refresh on ScreenResume

  • FlowScreen reads and applies resume state from on_screen_resume, the event Textual posts to a screen that becomes active. Textual posts it on push_screen as well as pop_screen, so one handler covers both the first open and every return — there is no on_mount counterpart to keep in sync, and no duplicate read per open.
  • FlowCard holds the resume state in a single resume_state reactive rather than parallel display flags, with resumable derived from it.
  • Both the tile label and the Resume button now follow the state in both directions. The old code could only ever set display = True, so a stale button survived a completed run.

One way to read resume state

  • CheckpointManager.resume_state(resolved_flow) reads the checkpoint file once and returns a ResumeState of completed and frontier phases. is_resumable is simply a non-empty frontier: a flow that never ran has nothing to start from, so resuming it is just a launch.
  • Replaces has_resumable_run, resume_completed_phases, and the module-level resolve_resume_state. runs.py keeps only its real job — locating run directories — behind one flow_resume_state(flow, runs_dir).
  • A flow screen previously cost three checkpoint reads (emptiness check, closure, then the graph); it now costs one per open. The "is this resumable?" policy moves out of the TUI, which had been re-deriving orchestration semantics.
  • ConfigError translation moves to PhaseOrchestrator, where the "delete it and restart" advice is actionable. The runtime module no longer imports the config error type.

A corrupt checkpoint is reported, not hidden

  • ResumeState carries an error, so "nothing was recorded" and "the recording could not be read" stop collapsing onto the same value. resume_state() still raises CheckpointReadError and the orchestrator keeps its precise message; flow_resume_state converts it instead of discarding it, so one corrupt file still cannot crash a grid of flows.
  • FlowCard renders a third footer state, ✕ checkpoint unreadable. Previously a corrupt file was indistinguishable from a flow that had never run, which left the remedy undiscoverable from the TUI.

Behaviour changes

  • Resuming with nothing left to resume is now refused rather than silently doing something else. Before, no checkpoints ran every phase exactly as a plain launch would, and an already-finished run skipped every phase, registered no processors, and reported instant success without doing any work. PhaseOrchestrator validates is_resumable — the same property the TUI uses — and raises with a reason distinguishing the two cases.
  • The TUI refuses first, so that case is an inline message rather than a failed run. Screen.dismiss invokes the result callback before popping, so the launch modal's dismiss commits a tick ahead of the ScreenResume refresh — the app would compute the correct answer immediately after acting on the stale one. _do_resume re-checks both before opening the modal and in the dismiss callback, resyncs the button, and reports why. The orchestrator check remains the CLI backstop.
  • CheckpointManager.read() raised an unhandled AttributeError on a checkpoint file holding valid YAML that is not a mapping. The TUI's bare except Exception had been masking it; narrowing that handler to CheckpointReadError surfaced it. read() now raises CheckpointReadError like it does for every other unreadable file, so a corrupt file can no longer crash the flow grid on startup.

Motivation

Whether a flow can be resumed is derived from its checkpoint file, so the data on disk is always correct. The TUI only read it while first building a screen, which left the display stale for the rest of the session:

  • A flow that failed mid-session kept showing no resumable label on its tile until the command was quit and rerun, because cards are built once at compose time and MainScreen is never popped.
  • Returning to the flow screen after a failed run did not reveal the Resume button, because FlowScreen is resumed rather than re-mounted. The user had to navigate back to the grid and re-enter the flow so a fresh screen was built.

The result was confusing rather than broken — the resume genuinely worked, nothing in the UI said so.

Follow-up in AI-7066: interrupting a run before any phase completes writes no checkpoint at all, so an interrupted run cannot be told apart from one that never started. Telling those apart needs a run marker written at start; it is not recoverable from what is on disk today.

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Add qa/required if this PR needs QA validation, or qa/skip-qa if it does not. Exactly one of the two is required.
  • If you need to backport this PR to another branch, you can add the backport/<branch-name> label to the PR and it will automatically open a backport PR once this one is merged

@luisorofino luisorofino added the qa/skip-qa Automatically skip this PR for the next QA label Jul 28, 2026
@dd-octo-sts dd-octo-sts Bot added the ddev label Jul 28, 2026
@luisorofino luisorofino changed the title Refresh flow resume state in the Togo TUI without restarting the command [AI-7065] Refresh flow resume state in the Togo TUI without restarting the command Jul 28, 2026
@luisorofino

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: 6dca3f56b6

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@datadog-prod-us1-5

datadog-prod-us1-5 Bot commented Jul 28, 2026

Copy link
Copy Markdown

Tests  Code Coverage

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
Patch Coverage: 99.55%
Overall Coverage: 89.10%

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 6c226de | Docs | Datadog PR Page | Give us feedback!

@luisorofino
luisorofino force-pushed the loa/refresh-flow-resume branch from 6dca3f5 to 5b2a117 Compare July 28, 2026 11:58
@luisorofino
luisorofino marked this pull request as ready for review July 28, 2026 12:01
@luisorofino
luisorofino requested a review from a team as a code owner July 28, 2026 12:01

Copy link
Copy Markdown
Contributor Author

Resume availability is derived from the run's checkpoint file, so the data on
disk is always accurate. The TUI only read it while first building a screen,
which left the display stale for the rest of the session: a flow that failed
mid-session kept showing no resumable label on its tile, and returning to the
flow screen did not reveal the Resume button until the screen was rebuilt from
the grid.

Recompute on ScreenResume, which Textual posts to a screen that becomes active
again. FlowCard.resumable is now reactive so MainScreen can reassign it in place
without recomposing the grid, preserving focus and scroll. Both the tile label
and the Resume button now follow the state in both directions, so they also
disappear once a run completes.

Collapse the resume-state API onto one entry point while here. Reading it took
three checkpoint reads per flow screen and left the "is this resumable?" policy
in the TUI, duplicating orchestration semantics. CheckpointManager.resume_state
now reads once and returns a ResumeState of completed and frontier phases;
is_resumable is simply a non-empty frontier, since a flow that never ran has
nothing to start from. That replaces has_resumable_run, resume_completed_phases,
and resolve_resume_state, and drops the runtime module's import of the config
error type.

Two consequences worth noting:

- Resuming with nothing left to resume is now refused instead of silently doing
  something else. Previously, no checkpoints ran every phase as a plain launch
  would, and an already-finished run skipped every phase and reported instant
  success without doing any work.
- read() raised an unhandled AttributeError on a checkpoint file holding valid
  YAML that is not a mapping. The TUI's bare except had been masking it; it now
  raises CheckpointReadError like every other unreadable file.

Follow-up in AI-7066: interrupting a run before any phase completes writes no
checkpoint at all, so the interrupted run cannot be told apart from one that
never started.
@luisorofino
luisorofino force-pushed the loa/refresh-flow-resume branch from 5b2a117 to d758fa7 Compare July 30, 2026 13:19
Comment thread ddev/src/ddev/cli/meta/ai/tui/screens/flow.py
Comment thread ddev/src/ddev/ai/runtime/checkpoints.py
Comment thread ddev/src/ddev/cli/meta/ai/tui/screens/flow.py Outdated
Comment thread ddev/src/ddev/cli/meta/ai/tui/runs.py
Addresses review feedback on the resume-state refactor.

Distinguish "nothing recorded" from "could not be read". `ResumeState` gains an
`error` field. `CheckpointManager.resume_state` still raises, so the
orchestrator keeps its actionable message; `flow_resume_state` converts the
`CheckpointReadError` into `error` rather than discarding it, so one corrupt
file still cannot crash a grid of flows but no longer reads as a clean slate.
`FlowCard` renders a third footer state for it, making the remedy discoverable
from the grid instead of unreachable.

Re-check before committing to a resume. `_do_resume` validated nothing beyond
`values is not None`, and `Screen.dismiss` invokes the result callback before
popping, so the push happened a tick ahead of the `ScreenResume` refresh: the
app computed the correct answer immediately after acting on the stale one. The
user landed on an execution screen of phases that would never run, with the
orchestrator's `ConfigError` wrapped as an `on_initialize` hook failure. Both
`_do_resume` and its dismiss callback now confirm resumability, resync the
button, and report inline. The orchestrator check stays as the CLI backstop.

Drop the dead `on_mount`. Textual posts `ScreenResume` on push as well as pop,
so both handlers fired on every open and `checkpoints.yaml` was read twice per
screen. `on_screen_resume` alone covers "screen becomes active", which also
removes the two-handlers-must-agree hazard and matches `MainScreen`.

`FlowCard` carries one `resume_state` reactive rather than parallel resumable
and error flags that would have to be kept in sync.
Comment thread ddev/src/ddev/cli/meta/ai/tui/screens/flow.py Outdated
Comment thread ddev/src/ddev/cli/meta/ai/tui/screens/flow.py
…kpoint

`Screen.notify` defaults to `markup=True` and `Toast.render` passes the message
through `Content.from_markup`, so embedding raw exception text made the toast
content attacker-shaped by accident. A checkpoint file that is valid YAML but an
invalid checkpoint entry — schema drift between ddev versions, or a partial
write — yields pydantic text containing `[type=missing, input_value={...},
input_type=dict]`. A bare word before `=` inside brackets is not valid markup, so
`MarkupError` escapes the compositor reflow and takes the app down. In the worst
case the commit that added a graceful refusal turned a survivable error into a
hard crash. Passing `markup=False` holds whatever the exception happens to
contain; compacting the text does not, since it only works while the offending
brackets stay off the first line.

The composed sentence was also malformed. The join assumed trailing punctuation,
but only one of the three `CheckpointReadError` shapes has it and the other two
are multi-line, so the remedy landed after a YAML caret or a pydantic URL —
eight and thirteen lines respectively.

`ExecutionScreen._compact_error_detail` already reduced multi-line error text to
one truncated line, so it moves to `tui/errors.py` as `compact_error_detail` over
`str` rather than growing a second copy. The execution screen keeps its
phase-prefix trimming and strips those prefixes before compacting, as before, so
a long message loses the same characters it used to.

`App.run_test` takes `notifications=False`, which is why no existing test caught
this: toasts are never mounted, so an unrenderable message raises nothing. The
two regression tests pass `notifications=True` and render for real.
@luisorofino
luisorofino force-pushed the loa/refresh-flow-resume branch from a8ef01d to 6c226de Compare August 7, 2026 09:51
@dd-octo-sts

dd-octo-sts Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Validation Report

All 21 validations passed.

Show details
Validation Description Status
agent-reqs Verify check versions match the Agent requirements file
ci Validate CI configuration and code coverage settings
codeowners Validate every integration has a CODEOWNERS entry
config Validate default configuration files against spec.yaml
dep Verify dependency pins are consistent and Agent-compatible
http Validate integrations use the HTTP wrapper correctly
imports Validate check imports do not use deprecated modules
integration-style Validate check code style conventions
jmx-metrics Validate JMX metrics definition files and config
labeler Validate PR labeler config matches integration directories
legacy-signature Validate no integration uses the legacy Agent check signature
license-headers Validate Python files have proper license headers
licenses Validate third-party license attribution list
metadata Validate metadata.csv metric definitions
models Validate configuration data models match spec.yaml
openmetrics Validate OpenMetrics integrations disable the metric limit
package Validate Python package metadata and naming
qa-label Validate the pull request declares whether it needs QA for the next Agent release
readmes Validate README files have required sections
saved-views Validate saved view JSON file structure and fields
version Validate version consistency between package and changelog

View full run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ddev qa/skip-qa Automatically skip this PR for the next QA team/agent-integrations

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants