refactor: render Spirit progress from structured fields, not a parsed string - #1314
Draft
aparajon wants to merge 1 commit into
Draft
refactor: render Spirit progress from structured fields, not a parsed string#1314aparajon wants to merge 1 commit into
aparajon wants to merge 1 commit into
Conversation
… string Spirit's status.Progress already carries the row counts, the checksum counters, the throttle status, and the ETA as typed fields, and the engine wrapper consumes them that way. The CLI still kept a regex that parsed a Spirit-shaped "rows/total pct% phase" line back into numbers, and the engine synthesized exactly such a line so the CLI could parse it again. Nothing on the wire ever populated that field, so the parser only ran against preview fixtures. Delete the parser and both call sites, stop synthesizing the line, and rewrite the parts of docs/spirit_progress.md that still described the ETA as regex-parsed out of Summary. ProgressDetail stays a free-text note for a human reader. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The CLI now drops and never renders progress_detail, which removes operator-facing notes still produced by the engine (e.g., direct execution markers and early Summary fallback).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Refactors Spirit progress rendering to be fully structured end-to-end (rows/percent/ETA/throttle/checksum), removing the legacy regex-based parsing of Spirit-shaped progress strings in the CLI and updating documentation to reflect the structured data flow.
Changes:
- Removed
ParseSpiritProgress+ regex parsing and switched CLI progress rendering to rely solely on structured fields. - Stopped the Spirit engine wrapper from synthesizing a
"X/Y Z% copyRows"ProgressDetailstring. - Updated
docs/spirit_progress.mdto documentstatus.ETA/ checksum progress as structured fields and to describeSummaryas display-only.
File summaries
| File | Description |
|---|---|
| pkg/engine/spirit/spirit.go | Stops synthesizing copy progress detail strings; keeps structured fields as source of truth. |
| pkg/engine/spirit/spirit_test.go | Updates tests to stop asserting on the synthesized ProgressDetail string. |
| pkg/engine/spirit/drain_outcome_test.go | Updates drained-outcome tests to remove expectations around dropped detail lines. |
| pkg/engine/engine.go | Clarifies TableProgress.ProgressDetail semantics as a non-parsed human note. |
| pkg/cmd/internal/templates/progress.go | Removes string-parsing branch; renders in-progress output from structured fields only. |
| pkg/cmd/internal/templates/progress_states_test.go | Updates CLI rendering tests to no longer rely on ProgressDetail fixtures. |
| pkg/cmd/internal/templates/progress_parse.go | Removes parsing/override logic and drops ProgressDetail from internal CLI types. |
| pkg/cmd/internal/templates/progress_parse_test.go | Adjusts parsing tests to validate structured field pass-through + status normalization. |
| pkg/cmd/commands/preview_log.go | Updates preview fixtures to set ETASeconds directly instead of embedding ETA in ProgressDetail. |
| pkg/cmd/commands/apply_log_test.go | Updates log emitter test naming and removes reliance on a Spirit detail string. |
| docs/spirit_progress.md | Updates documentation to describe ETA/checksum as structured and Summary as display-only. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why this matters
SchemaBot carried a regex that parsed a Spirit-shaped progress line (
71436/221193 32.30% copyRows …) back into row counts and a percentage, and the engine wrapper went out of its way to produce such a line from Spirit's structured per-table counters so the CLI could parse it again. Every value that round trip recovered is already a typed field onstatus.Progress: the per-table rows, the checksum counters, the throttle status, and the ETA (status.ETA{State, Duration}), which the engine has consumed structurally for some time. The parser had also gone dead on the live path, since no server layer populates the wire field it read, so it only ever ran against preview fixtures — but a parser that mimics an engine's log format is exactly the kind of workaround AGENTS.md's Upstream First rule names, anddocs/spirit_progress.mdstill told readers the ETA was regex-parsed out ofSummary.What it does
ParseSpiritProgress, its regex, and theSpiritProgressInfotype from the CLI templates, along with both call sites: the override inParseProgressResponseand the string-first branch of the in-progress table renderer. The renderer's structured branches (estimate-exceeded, bar, rows/ETA line, phase status) already covered every case the string branch did.buildSpiritTableProgressfrom synthesizing a"%d/%d %d%% copyRows"detail string.engine.TableProgress.ProgressDetailremains a free-text note for a human — Spirit'sSummarybefore per-table progress exists, or the direct-execution marker — and its comment now says so.ProgressDetailfrom the CLI's internaltemplates.TableProgress; the publicapitypesJSON field and the proto field are untouched.docs/spirit_progress.md: thestatus.Progresstable now listsETAandChecksum,Summaryis documented as display-only, and the CLI section and key-behaviors entry describe the ETA as structured end to end (status.ETA→ETASeconds→ task row → API →ui.FormatETA).Two properties worth calling out:
TEMPLATES.mdregenerates byte-identical, and the log previews render the same heartbeat lines fromETASecondsthat they previously carried in the fixture string.status.ETAearlier; this PR retires the consumer-side parser that outlived it. The one remaining string-only value inSummary(the runner-wide aggregateCopyProgress) is never read here; feat(status): expose row-copy progress as a structured field on Progress spirit#1220 adds it as aCopyfield onstatus.Progressso that every value inSummaryhas a typed counterpart, and the doc here now says that a value found only inSummaryis fixed upstream, not parsed.How it moves us toward the northstar
The progress path is now structured from the engine to the terminal with no text in between, which is the shape Upstream First asks for: when Spirit adds a signal, SchemaBot picks it up as a field, and when SchemaBot needs one Spirit lacks, the fix lands in Spirit for every consumer rather than as a parser here.
Opened by Claude (Fable 5).