Skip to content

refactor(core): type journaled phases so the plan and the journal share one vocabulary - #401

Open
PiquelChips wants to merge 2 commits into
mainfrom
feat/timeline-1-phases
Open

PiquelChips wants to merge 2 commits into
mainfrom
feat/timeline-1-phases

Conversation

@PiquelChips

@PiquelChips PiquelChips commented Nov 10, 2026 •

Copy link
Copy Markdown
Member

The deployment timeline (live steps, progress and time estimates in the dashboard and CLI) will be built entirely from the event journal, so the journal has to say precisely what each step is. Today a journaled action's phase is a free string, typed out at every call site, and the same work goes by two names: preparation logs resolving_image where the plan says resolve_image, and progress said running_job where the action said run_job.

This is the first PR of the timeline stack. It makes phase a typed piqueld_core::Phase everywhere: the journal, operations, events, analytics and telemetry. Plan actions are journaled under ActionKind::phase, so the plan and the journal share one vocabulary, and the compiler now catches a misspelled phase.

  • Stored names don't change. History and analytics keep grouping old and new actions together.
  • Edit events stop borrowing phase. Typed configuration edits recorded the edited field ("jobs", "service"…) in phase. That moves into a new typed event detail ({"type": "edited", "field": "jobs"}), which the timeline will also use for recorded plans.
  • Migration 0034 renames the progress-only names (running_job → run_job, preparing_sources → preparing), moves edit fields into detail_json, and clears any other unknown phase so reads can't fail on old rows.

Clients see two API changes: phase is now an enum in the OpenAPI document, and application_edited events carry the field in detail instead of phase.

Proved with a migration test on a schema-31 database (edit fields move to detail, legacy progress names map, unknown phases clear) and the full workspace suite (1032 tests).

Reviewed with review-loop: 3 rounds with 3× Claude Opus 5.5 (GPT 6.1 Sol was out of credits; a Sol pass is still owed). Verdict: approved

Note

Written by Claude Opus 5.5 in Claude Code and reviewed by Claude Opus 5.5 (via T3 Code)

Summary

This change replaces free-form journal phase strings with the shared piqueld_core::Phase enum. It also separates typed configuration-edit details from event phases.

Core types and planning

  • Added Phase as the shared type for operation phases and action-duration analytics.
  • Changed Operation.phase, Event.phase, and ActionDuration.phase to use Phase instead of strings.
  • Added ActionKind::phase() so plan actions use the same phase vocabulary as journaled actions.
  • Added EditedField and ApplicationEdit::field() to identify the configuration area changed by an edit.
  • Added EventDetail, including the typed Edited detail with its EditedField.

Journal and runtime behavior

  • Updated journal creation, operation progress, reconciliation, ingress, certificate, and service call sites to pass Phase variants.
  • The journal converts phases to their stored names when it writes database rows and lifecycle events.
  • Operation reads now decode stored phase names into Phase; invalid names return decoding errors.
  • Updated telemetry and user-interface rendering to format typed phases through their string representation.

Event storage and migration

  • Configuration-edit events now store the edited field in structured detail_json, for example detail: "edited" with the corresponding field.
  • Added migration 0034_typed_phases.sql. It adds events.detail_json, moves legacy edit fields from phase into event details, and maps running_job to run_job and preparing_sources to preparing.
  • The migration clears other phase values that are not in its known-phase list.

API and client contract

  • Updated the OpenAPI schema to define Phase, EditedField, and EventDetail.
  • Event, operation, and action-duration phase fields now use the Phase schema.
  • Event responses now include the typed detail field.
  • Re-exported Phase from piqueld-client.
  • Updated API documentation to describe event details for configuration edits.

Tests and user-facing output

  • Updated test fixtures and assertions to use typed phases and typed edit details.
  • Added a migration regression test for legacy phase values and edit details.
  • Updated CLI and UI phase formatting to render typed values.

@coderabbitai

coderabbitai Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

Walkthrough

The change introduces typed phases for operation progress, journal actions, events, and analytics. Edited-configuration events now carry structured field details. Storage migration, runtime call sites, API schemas, CLI output, telemetry, and browser views are updated to use these types.

Changes: Typed observability data

Layer / File(s) Summary
Phase and event-detail contracts
crates/piqueld-core/src/*, docs/openapi-v1.json, docs/dev/architecture/observability.md, docs/user/reference/api.md
Core models add Phase, EditedField, and EventDetail. Operation, event, and action-duration phase fields use Phase. OpenAPI schemas and documentation describe the typed fields.
Storage and migration
apps/piqueld/migrations/*, apps/piqueld/src/store/*, apps/piqueld/tests/api_contract/field_edits.rs
Journal and operation storage accept typed phases and write their stored names. Event reads decode phases and structured details. The migration converts recognized legacy phases and moves edited-field data into event details.
Runtime phase usage
apps/piqueld/src/api/service/*, apps/piqueld/src/application/runtime.rs, apps/piqueld/src/ingress/*, apps/piqueld/src/reconcile/*
Service, application, ingress, and reconciliation code passes Phase variants to progress and journal APIs. Action kinds expose a phase mapping used by reconciliation.
Telemetry, display, and tests
apps/piqueld/src/telemetry/journal.rs, apps/piquelctl/*, apps/piqueld-ui/src/browser/*, apps/piqueld/tests/*
Telemetry and user-facing displays convert typed phases to strings. Tests use typed phases and check edited-field details. The CLI analytics fixture uses resolving_image in place of pull.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Refactor

Merge Risk: Minimal · up to 8b905

No concrete merge-blocking failure remains established. The question about event pages containing unknown phases remains unverified, so normal checks can proceed.

Pre-merge checks: Passed 3 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Description check Warning The description states the purpose clearly: journaled phases become a typed piqueld_core::Phase, plan and journal share one vocabulary, edit fields move from phase to a typed detail, and clients see t… 1. Add a conventional-commit title, for example 'refactor(core): type journaled phases as Phase'. 2. Add the template's 'What and why' heading and link the issue it closes (for example 'Closes #...'); the PR references #401 in the objective…
Passed Passed checks: 3 passed

Check name Status Explanation
Title check Passed The title clearly identifies the main change: using typed phases to align the plan and journal. It is concise and specific.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description states the purpose clearly: journaled phases become a typed piqueld_core::Phase, plan and journal share one vocabulary, edit fields move from phase to a typed detail, and clients see two API changes. It also gives verification (a schema-31 migration test and a 1,032-test workspace run). It does not follow the repository template exactly. It has no title line in the template's conventional-commit form, and the 'What and why' heading is absent. The checklist is missing, and neither item is addressed: the description does not confirm that the feature works through the manifest, piquelctl, and the dashboard, and it does not say whether the documentation and NixOS module were updated. The docs were updated (observability.md, api.md, openapi-v1.json), but the NixOS module is not mentioned. It also does not link an issue with 'Closes #...'. The migration number is inconsistent: the description says Migration 0034, while the summary and objectives say 0032. Whether the review-loop note needs to appear is a minor point.

Resolution

  1. Add a conventional-commit title, for example 'refactor(core): type journaled phases as Phase'. 2. Add the template's 'What and why' heading and link the issue it closes (for example 'Closes #...'); the PR references #401 in the objectives, but the description does not. 3. Complete the checklist: state whether the feature works through the manifest, piquelctl, and the dashboard, and state whether the NixOS module was updated. The docs were updated, so confirm that as well. 4. Correct the migration number: the description says 0034, but the migration file is 0032_typed_phases.sql.
Finishing Touches

Generate unit tests (beta)

  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

I’m a rabbit with a phase in hand,
Typed in code across the land.
Event details hop in view,
Old names find their mapping too,
I nibble logs and outputs new.
Then thump my paws and scamper through!

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional comments: 1

apps/piqueld/src/store/observability/event.rs: 1

632-632: Trivial · Stability & Availability

Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.

One unknown stored phase now fails the whole event page.

decode_name returns StoreError::corrupt for a name outside Phase. The migration clears old unknown names. A row written later by a rollback to an older binary, or by a future version, then breaks filtered_events for every page that includes it. Consider a tolerant decode that maps unknown names to None, or document that downgrades are unsupported.

Review info

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 13d6a667-2a33-4c72-8397-2f1b0c2af0b6
Commits

Reviewing files that changed from the base of the PR and between 67ede67 and 8b90584.

Files selected for processing: 52

  • apps/piquelctl/src/commands.rs
  • apps/piquelctl/src/output/reports/operations.rs
  • apps/piquelctl/tests/cli.rs
  • apps/piqueld-ui/src/browser/dashboard.rs
  • apps/piqueld-ui/src/browser/management/deployments.rs
  • apps/piqueld-ui/src/browser/observability.rs
  • apps/piqueld-ui/src/browser/runtime.rs
  • apps/piqueld/migrations/0032_typed_phases.sql
  • apps/piqueld/src/api/service/mod.rs
  • apps/piqueld/src/api/service/startup.rs
  • apps/piqueld/src/application/runtime.rs
  • apps/piqueld/src/ingress/certificates.rs
  • apps/piqueld/src/ingress/gateway.rs
  • apps/piqueld/src/ingress/node.rs
  • apps/piqueld/src/ingress/records/mod.rs
  • apps/piqueld/src/ingress/records/tests.rs
  • apps/piqueld/src/ingress/tests/dns01.rs
  • apps/piqueld/src/ingress/tests/mod.rs
  • apps/piqueld/src/ingress/tunnel.rs
  • apps/piqueld/src/ingress/wire.rs
  • apps/piqueld/src/reconcile/actions.rs
  • apps/piqueld/src/reconcile/controller.rs
  • apps/piqueld/src/reconcile/coordinator.rs
  • apps/piqueld/src/reconcile/deployment.rs
  • apps/piqueld/src/reconcile/images.rs
  • apps/piqueld/src/reconcile/jobs.rs
  • apps/piqueld/src/reconcile/links.rs
  • apps/piqueld/src/store/acceptance.rs
  • apps/piqueld/src/store/auth/access.rs
  • apps/piqueld/src/store/journal.rs
  • apps/piqueld/src/store/observability/event.rs
  • apps/piqueld/src/store/observability/mod.rs
  • apps/piqueld/src/store/observability/tests.rs
  • apps/piqueld/src/store/operation.rs
  • apps/piqueld/src/store/secret/tests.rs
  • apps/piqueld/src/telemetry/journal.rs
  • apps/piqueld/tests/api_contract/field_edits.rs
  • apps/piqueld/tests/api_contract/secrets.rs
  • apps/piqueld/tests/fake_docker/image_retention.rs
  • apps/piqueld/tests/fake_docker/jobs.rs
  • apps/piqueld/tests/fake_docker/rollouts.rs
  • apps/piqueld/tests/fake_docker/secrets.rs
  • crates/piqueld-client/src/lib.rs
  • crates/piqueld-core/src/edit.rs
  • crates/piqueld-core/src/event.rs
  • crates/piqueld-core/src/lib.rs
  • crates/piqueld-core/src/observability/mod.rs
  • crates/piqueld-core/src/operation.rs
  • crates/piqueld-core/src/planner.rs
  • docs/dev/architecture/observability.md
  • docs/openapi-v1.json
  • docs/user/reference/api.md

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

@PiquelChips

Copy link
Copy Markdown
Member Author

Note

🤖 Claude Opus 5.5 responding on behalf of PiquelChips

Declining the tolerant decode for events.phase (event.rs:632). The store decodes every stored enum through decode_name and reports an unknown value as StoreError::Corrupt: event scope, operation kind and state already work this way, so phases now match them. Migration 0032 clears the unknown names written before this change, and every name the daemon writes comes from Phase.

The rollback case would need a downgrade to run against a schema it doesn't know, which piqueld doesn't support for any of these columns. Mapping unknown phases to None would also hide real corruption on the one column where it is now detectable.

@PiquelChips
PiquelChips force-pushed the feat/timeline-1-phases branch from 8b90584 to aad3cbe Compare November 10, 2026 21:29
@PiquelChips
PiquelChips added this pull request to stack #449 November 12, 2026 13:48
@PiquelChips PiquelChips self-assigned this Nov 12, 2026
@PiquelChips PiquelChips added this to the v0.2 milestone Nov 12, 2026
@PiquelChips PiquelChips moved this from Needs Triage to In Progress in origin-src.dev Nov 12, 2026
@PiquelChips
PiquelChips force-pushed the feat/timeline-1-phases branch from aad3cbe to 4e39acb Compare November 13, 2026 14:39
The phase a journaled action records was a free string, written at each call
site and compared as text by readers. It is now `piqueld_core::Phase`, used
by the journal, operations, events and analytics, and plan actions are
journaled under `ActionKind::phase`, so the plan and the journal share one
vocabulary. Stored names are unchanged.

Typed configuration edits recorded the edited field in `phase`; it moves into
a new typed event `detail`. A migration renames the progress-only names
(`running_job`, `preparing_sources`) to the actions they stood for and clears
any other unknown value.
@PiquelChips
PiquelChips force-pushed the feat/timeline-1-phases branch from 4e39acb to 0e8d200 Compare November 13, 2026 15:00
An event's `detail` object was tagged by its own `detail` field, so clients
read `detail.detail`. It is tagged by `type` instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

1 participant