feat: Codex alignment — all 10 phases (persistence, protocol, resume, compaction) - #36
Merged
Conversation
…ory seq counter and version-tolerant reader
…, Codex-style sessions layout, one-time legacy run migration
The Phase 3 one-time legacy run migration joined `.rove` onto the workspace root and then wrote its "already migrated" marker there, which created a state directory in workspaces that had no legacy state to migrate. That leaked into `rove-cli`'s state-directory rebase test, which failed on clean HEAD as well as on this branch. Return early when the legacy state directory does not exist: a scan that finds nothing must leave the workspace exactly as it was.
Mirror codex's `ResponseItem` vs `EventMsg` split so resume rebuilds
model context from explicitly recorded facts instead of reclassifying UI
events heuristically.
- core: new `HistoryItem` (serde `kind` tag) defining what enters the
model context, plus `history_to_messages` projection.
- runtime: new `TraceEntry{History, Ui}` trace payload (untagged serde —
`kind` vs `type` makes both generations self-describing on disk), a
`history_projection` module deriving history items from the event
stream with the same rules `artifacts.rs` already persists, and a
single facade choke point that writes them.
- trace reader: parses three generations (new envelope, Phase 1
envelope, bare legacy events) and collects the explicit history
stream; the pre-existing `TraceEntry` struct becomes `TraceRecord`.
- reconcile: rebuild history from the trace's explicit stream when one
exists, merged into the snapshot by suffix alignment so the
crash-between-writes gap closes without ever double-counting. A stream
that shares no suffix with the snapshot is treated as divergence: keep
the durable snapshot and warn rather than guess. Canonical session and
its compatibility tail are kept in agreement.
`StreamEvent` and its wire representation are deliberately unchanged,
so CLI/API/Web consumers and the cross-language event-name contract are
untouched; legacy traces keep resolving through the snapshot path.
Tests: 6 new (1591 total, all green). `tests/history_resume.rs` is the
plan's soul test — it empties the snapshot entirely, reconciles from the
trace alone, and asserts the resumed run's provider receives the first
run's conversation. Verified non-vacuous by mutation: short-circuiting
the rebuild makes it fail.
Check off the Phase 2 acceptance items with the tests backing each one, and record three divergences under the plan's §0.3 rule: - D1: `StreamEvent` was not split into a separate `UiEvent` enum; it is wrapped as `TraceEntry::Ui` instead. Splitting it would touch three consumers and the cross-language event-name contract while the actual goal — heuristic-free resume — is met by the explicit history stream alone. Consequence: step 6's `message_adapter.rs` synthesis layer is unnecessary, since apps/api never reads the trace and the SSE wire representation never changed. - D2: `HistoryItem` reuses the normalized `Message` rather than splitting Message/ToolCall/ToolResult, which rove's protocol already carries inside a message. - D3: the trace reader's existing `TraceEntry` struct was renamed `TraceRecord` to free the name for the new enum.
Extract the patch/edit text kernels out of runtime/tools into a new
leaf crate `rove-tools-text`, mirroring codex-rs/apply-patch's shape:
- patch.rs heredoc patch parser (Add/Delete/Update + Move to, @@ hunks)
- matching.rs graded context matching (Exact / TrailingWhitespace /
Whitespace) with ambiguity detection at the strongest
confidence, char-based so multi-byte content never splits
- apply.rs pure kernel (input_files, patch) -> ApplyOutcome, whole
patch validated before anything is returned (no half-apply),
CRLF preserved only when consistent, retryable errors limited
to ContextNotFound / AmbiguousContext
- diff.rs localized_diff / render_unified_diff, byte-budgeted
The crate has no tokio, no std::fs and no local dependencies. runtime
keeps the Tool impls (async + approval + workspace boundary are product
semantics) and calls the kernel at two type-level sites.
workspace_architecture.rs now pins the direction: rove-tools-text is a
leaf, and runtime's local dependency set is exactly
{rove-core, rove-models, rove-tools-text}.
Also skip project_trust's junction-retargeting test when Windows refuses
to traverse the junction (os error 448) and therefore no capability
digest — and so no grant — can exist. That test was already red on main
here; the refusal is the safe outcome, so the scenario is skipped rather
than the assertion loosened.
48 new tests; full workspace suite green.
rove has two entry points that can migrate the same database at the same moment: the long-lived desktop API and a transient CLI invocation. The runtime state index ran its migration loop with no transaction at all, on every single connect() — a plain TOCTOU race on the database Phase 5 is about to bump to v15. Wrap both stores' migration sequences in an fs2 advisory file lock, with double-checked locking so the common already-current path stays lock-free. Each individual step runs in an Immediate transaction, so an interrupted run either records a version or rolls the step back whole, letting the next start resume from the prefix. The lock is a sibling of each database rather than one global path: rove has two independent databases, and per-workspace and per-test databases must not block each other. Also fixes two pre-existing races the new concurrent test exposed: - PRAGMA journal_mode=WAL needs an exclusive lock, and SQLite refuses it with a bare SQLITE_BUSY without consulting the busy handler, so the connection's busy_timeout never covered it. Concurrent first starts could fail to open the database at all. Retried within the same budget. - The legacy state prune classified the new .migrate.lock as Unknown and left it behind, degrading legacy_disposition to partially_pruned. It is transient coordination state, so it is skipped like -wal/-shm.
Codex alignment Phase 6. Resume rebuilt its model context from the
task_state snapshot alone, so a run killed before its checkpoint landed
came back with no conversation at all -- the trace held every history
item, and nothing read them.
Three pieces close that:
- ReverseJsonlScanner: chunked backwards scan of a JSONL file. Reading a
bounded tail costs a bounded number of bytes regardless of file size
(65 KB for a 3-item tail of a 4.4 MB trace, asserted). A torn final
record from a crash is reported and skipped rather than ending the scan.
- InitialHistory{New, Resumed, Forked}: a run must name how it begins
instead of leaving it to be inferred from whether an optional field
happened to be populated. get_initial_history resolves it; a compaction
marker ends the read as complete, since the summary already stands in
for everything older.
- TraceEntry::Link(TraceLink::ResumedFrom): rove owns a directory per run,
so a resumed run writes its own trace. Without an explicit marker the
two files look like unrelated runs. read_history_chain walks the links
backwards and returns one continuously replayable history, with a
chain-wide item budget so a long chain costs no more to open than a
single long run.
The facade prefers the snapshot when it has content, so every existing
resume path stays byte-identical, and falls back to the trace only when
the snapshot is empty. Trace-derived history closes any interrupted tool
round with an explicit unknown-effect result, mirroring what
Session::close_unresolved_tool_calls does for canonical checkpoints:
replay is refused rather than assumed, and the call identity survives.
Verified: rove-runtime lib 601/601, e2e 108/108, clippy clean
workspace-wide. The acceptance test was checked against a mutation --
with the fallback disabled it fails, showing the prompt falls back to the
lossy session summary and loses both original turns.
Codex alignment Phase 5. The filesystem is the record and SQLite is a rebuildable cache -- but neither database could actually be rebuilt. Runtime index: a run's owning session lived only in SQLite, so a run whose process died before its first task_state.json was unrecoverable, and its missing `runs` row made every later event insert violate a foreign key -- one crashed run failed the entire repair. Traces now open with an identity header carrying session/job/run and a start time, and repair reads it to recreate the run row. `backfill_missing_runs` runs this at startup, but only after a directory listing and one id query show something is actually missing. `event_offsets` is dropped in migration v4: it held exactly what `runs.last_event_seq` holds, written in the same transaction from the same seq, so two copies only invited a divergence no reader could arbitrate. The header takes RUN_META_SEQ = 0 rather than drawing from the event counter. `?after=N` and SSE Last-Event-ID are a wire contract anchored on the first event being seq 1; spending a sequence here shifted run_started to 2 and replayed it to clients that had already acknowledged event 1. Product catalog: which product session owns a run existed nowhere but the catalog, so deleting it lost every session. Each bound run now records its owner beside itself in product_owner.json, and startup rebuilds sessions from those records. Recovery is per-session because the reader validates product_session_runs as a chain: it renumbers ordinals from 1 and relinks resumed_from_run_id as it inserts, so a lost record shifts later ordinals instead of leaving a gap that makes the whole session unreadable. A session the catalog still holds is left alone -- on-disk records are a snapshot from run start and know nothing about renames or archiving. A runtime identity already owned by another session is skipped, not stolen. Diverges from the plan in four places, recorded in the plan document: the two databases stay separate (one is per-workspace, the other global), the `rollouts` table became a sidecar (the ownership fact does not exist in codex's model, and a table would again be its only copy), the schema change is runtime v3->v4 rather than a merged v15, and "zero redundancy" was already true after Phase 2 -- model history never enters the database.
The listing had no paging: it read a whole workspace and relied on MAX_PRODUCT_SESSIONS = 2048 as an implicit LIMIT, so a workspace past that count had its tail silently truncated and unreachable. Sessions sort archived-last, so the leading sort term is a CASE expression. A keyset predicate that lets that term vary has to be a three-way disjunction, and SQLite cannot prove such a scan is ordered -- it materialises and sorts, at a cost that grows with the workspace. The page is therefore assembled one rank group at a time, with the rank bound as an equality and absent from ORDER BY, which keeps the index scan itself ordered. Rank has two values, so a page costs at most two seeks. Migration 015 indexes the CASE expression so one index covers the whole sort key, preserving the grouping clients already see. Cursors are opaque three-part tokens (rank, updated_at, id), following the listWorkspaceFiles precedent rather than the plain next_after_seq idiom, so the sort order stays out of the public contract. Malformed cursors are rejected with 400 rather than falling back to page one. include_archived defaults to true for wire compatibility; the web opts out explicitly instead of filtering after transfer. 10k-session fixture: p95 9.27ms over 60 pages, flat with depth. Every mechanism was mutation-tested; the depth-ratio assertion was measured not to discriminate and was removed rather than left as decoration.
…turn summaries
Completes the compaction phase: an operator-triggered path, and three
correctness fixes the acceptance tests turned up.
Manual compaction
- `CompactionTrigger{Automatic,Manual}` separates consent from health. The
`enabled` switch means "do not compact behind my back", so `Manual` bypasses
it while `Automatic` respects it; both still honour the circuit breaker, which
is about the model failing rather than about permission. Splitting
`breaker_tripped()` out of `circuit_open()` is what makes that possible —
the latter reports `false` while compaction is switched off, which is right
for the UI and wrong as a gate for the manual path.
- `Engine::compact_resume_state()` compacts a caller-owned snapshot without
starting a run: no RunId, no trace, no `PromptCompacted` event, since there is
no run for one to belong to. Any prior summary is folded into the new one.
- CLI `/compact` edits only the in-memory resume snapshot, so the next prompt's
own run persists it through the normal checkpoint path and quitting without
another prompt leaves the stored session untouched.
Correctness
- React sent the compacting turn with the history already dropped and the
summary not yet in place, so the summary only landed one turn later. The
context is now rebuilt after compacting, as PlanReact already did.
- `continue_from_summary` wrote only `TaskState::summary`, which every completed
run also fills with a truncated final output — so it could not carry a
compaction forward without making ordinary resumes look compacted. The summary
now lands in `checkpoint.summary`, the field the resume path actually reads,
creating a minimal checkpoint when the session had none.
- Phase 6's trace fallback read "empty history" as a lost snapshot and refilled
it, undoing the compaction and leaving the prompt larger than before.
`history_was_compacted_away()` distinguishes deliberate emptiness from a run
that died before checkpointing; only the former is exempt.
Acceptance
- `a_compacted_session_resumes_with_the_summary_instead_of_its_history` asserts
both directions: summary present, replaced turns gone.
- `a_compaction_leaves_the_full_history_exportable_from_the_trace` pins the
audit guarantee at its source — the trace bytes are unchanged and still export
every original message.
- Both were mutation-checked: neutering the Phase 6 carve-out fails the first.
On a machine with a configured active profile, `--model fake` resolved to that profile and sent the literal model name "fake" to it. The request was live and billable, and could only ever fail — SiliconFlow answers it with HTTP 400 "Model does not exist". `fake` is never a model a real provider serves: it only arrives as an explicit request for the offline client, so it now outranks the configured active profile and selects a fake-typed profile, which `assemble_run` already short-circuits to `FakeModelClient`. The carve-out is narrow — every other model still follows the active profile. This is why five `cli_repl` tests failed on any developer machine with a real `~/.rove/config.toml`, on this branch and on main alike: they spawn the real binary with `--model fake` and asserted on fake output. They now also pin `ROVE_CONFIG_ROOT` to a temp dir, so ambient config cannot reach them either way; `USER_CONFIG_ROOT_ENV` is re-exported from the bootstrap crate root for it. Both fixes are kept deliberately: isolation stops the tests depending on the machine, and the precedence fix is what makes the product correct for users who have a provider configured. A unit test pins the precedence directly, since isolation would otherwise hide the bug.
Phase 4 of the codex alignment program. The plan called for collecting
DTOs out of apps/api; reading the code ruled that out and pointed at a
better target, so the shape here differs from the plan on four counts
(recorded as divergences D1-D4 in the plan doc).
What moved: the wire vocabulary that appears in persisted artifacts,
HTTP paths, and SSE payloads -- SessionId/JobId/RunId/CallId, and the
RunStatus/ApprovalPolicy/RunMode/ApprovalDecision enums. rove-runtime
and rove-core re-export them, so all 1718 call sites are untouched.
That re-export is the whole reason a genuine zero-dependency crate is
affordable here rather than a 2800-line rewrite.
The crate depends on serde and ulid and nothing else -- no tokio, no
axum, no utoipa, no other rove crate. OpenAPI is unaffected because
apps/api already attaches schemas at the point of use via
#[schema(value_type = String, format = "ulid")].
Also adds PROTOCOL_VERSION and the Versioned<T> envelope, so every SSE
frame leads with "v". The payload is flattened rather than nested, which
keeps the wire backward compatible in both directions: an older client
still finds `type` and every event field at the top level, and a frame
recorded before the field existed still deserializes.
The isolation is now a test rather than a manual check --
workspace_architecture.rs asserts rove-protocol has no local
dependencies and that nine forbidden packages stay out of its tree.
Both that guard and the SSE assertion were mutation-verified.
Side fix: the OpenAPI description for /jobs/{job_id}/events misstated
the frame shape (it named JobStreamEvent, whose `seq` actually travels
in the SSE `id:` line, not in `data:`). Adding `v` widened the gap, so
the description now spells out the real layout.
AllureCurtain
changed the title
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
1 participant
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.
Implements all 10 phases of the Codex alignment plan (
docs/plans/2026-08-25-codex-alignment-implementation-plan.md): trace envelope, model-history / UI-event separation, global home, protocol crate, store rebuildability, resume hardening, session pagination, context compaction, migration locking, and tool crate isolation.76 files, +12210 / −570. One PR by request — the phases share a spine (P2 upgrades the type P1 introduces; P6 reads what P1/P2 write; P8's correctness depends on P6's fallback), so a stack would not have been independently reviewable anyway.
The plan doc is the per-phase source of truth: every acceptance box carries a commit hash, and every deliberate deviation from the written design is recorded inline as a
分歧记录（§0.3 规则）blockquote. Please read those before reviewing — five phases intentionally differ from what the plan prescribed, because the plan was written before the code existed in its current shape.Phases
69be671TraceLine{ts, seq, event}; seq from an in-memory counter seeded once from the state index, so the append path no longer hits SQLite per event. Version-tolerant reader.e91ff95~/.rovewithROVE_HOMEresolution, Codex-style sessions layout, one-time legacy run migration.01e69fbResponseItemvsEventMsgsplit:HistoryItemin core,TraceEntry{History, Ui}on disk, so resume rebuilds model context from explicitly recorded facts instead of reclassifying UI events heuristically.6c05187rove-tools-text: pure text kernels (patch, diff, matching), no IO, no async, no local dependencies.3ef3cb6584fe54InitialHistory, empty snapshots refilled fromtrace.jsonl.46f15b5product_owner.jsonsidecar.21eb08e2ff2266/compact) compaction, with consent and circuit-breaker health kept as orthogonal gates.aefa1e3rove-protocolas the workspace leaf: serde + ulid only. Owns the identifiers, lifecycle enums,PROTOCOL_VERSION, and theVersioned<T>SSE envelope.Three incidental fixes are included, each explained in its own commit message:
1d5f7c0(Phase 3 was materializing.rovein pristine workspaces — also broke a test on cleanmain),77f1787(--model fakewas pairing the literal name with a configured real provider, issuing a billable request to it; the CLI REPL suite is now isolated from ambient machine config viaROVE_CONFIG_ROOT), and the OpenAPI description for/jobs/{job_id}/events, which misstated the frame shape.Compatibility
Nothing about the existing wire format breaks.
StreamEventand its serde representation are unchanged; CLI, API, and Web consumers are untouched, as is the cross-language event-name contract.rove-protocolby re-export, so all ~1718 call sites acrossapps/,runtime/,core/, andtests/are unmodified. The re-export is the intended long-term form, not a migration shim."v"field on SSE frames uses#[serde(flatten)]rather than nesting, so a client written before versioning still findstypeand every event field at the top level. In the other direction,vdefaults toPROTOCOL_VERSION, so frames recorded before the field existed still deserialize.apps/apialready attached them at the point of use via#[schema(value_type = String, format = "ulid")].Test plan
cargo test --workspace --no-fail-fast: 1724 passed, 0 failed.cargo fmt --all --checkandcargo clippy --workspace --all-targetsboth clean (zero diagnostics). Run on Windows, per the plan's platform requirement.New assertions were mutation-verified rather than assumed load-bearing — each was watched to fail against a deliberately broken implementation:
tests/history_resume.rs— the plan's "soul test": empties the snapshot entirely, reconciles from the trace alone, asserts the resumed run's provider receives the first run's conversation. Short-circuitingrebuild_history_from_tracemakes it fail.tests/e2e.rs— a compacted session resumes with the summary and not its original history; and a compaction leaves the full history exportable from a byte-identicaltrace.jsonl.tests/api.rs— SSE frames lead with{"v":1,, keeptypeat the top level, and nest nothing. Revertingsse_eventto serialize the bare event makes it fail.tests/workspace_architecture.rs—rove-protocolhas an empty local-dependency set and nine forbidden packages stay out of its tree. Addingtokiotoprotocol/Cargo.tomlmakes it fail.runtime/src/foundation/types.rs— a checkpointless session still carries its summary forward; only a genuinely compacted state reports its history as compacted away.apps/cli/src/cli/runtime.rs— an explicitfakemodel outranks a configured real profile, and a real model still follows the configured active profile.tests/event_contract.rs,tests/artifact_compatibility.rs— unchanged and passing, which is the evidence for "wire semantics unchanged" and "old artifacts still read".Two things to know before reviewing
A known-flaky test, pre-existing and not from this work.
paging_deep_into_a_ten_thousand_session_workspace_stays_flat(from P7) asserts wall-clock p95 < 50ms. It measures ~13.8ms in isolation but sometimes exceeds the threshold under full-workspace parallel load — it fails and passes across runs of the same commit, and it fails onmaintoo. Rerun before believing a failure. Replacing the wall-clock assertion with a work-proportional one is a decision I did not make unilaterally.Web unit tests were not executed here.
apps/webhas nonode_modulesin this worktree: a machine-wide NTFS junction traversal failure breaks everypnpm installwith misleading "Cannot find module" errors. Unrelated to this branch. What I verified instead, by reading: the web SSE path isJSON.parse(...) as StreamEventdispatched ontype, with no zod or exact-key validation, so the addedvfield is inert there. Flagging it rather than claiming a pass I did not observe.Notable divergences
Full reasoning is in the plan doc; the ones most worth a reviewer's attention:
StreamEventwas not split into a separateUiEventenum. Splitting it would touch three consumers plus the cross-language contract, while the actual goal (heuristic-free resume) is achieved by the explicit history stream alone. Consequence: the plan'smessage_adapter.rssynthesis layer is unnecessary, so "no apps/api protocol change" holds structurally rather than via a shim.state.db. Merging would break rove's product semantics: the runtime index is per-workspace, the product catalog is global. The real goal ("files are the record, SQLite is a rebuildable cache") is orthogonal to file count, so each database was instead given independent rebuild capability.apps/api/src/lib.rsshrinks ≥30% via DTO extraction" is unachievable: that file has 62 handlers and exactly onepub struct. There are no DTOs in it to move. Shrinking it means splitting handlers or migrating the store, which is Phase 5 territory, not a side effect of a protocol split.apps/api. Those DTOs are allutoipa::ToSchemaderives, which contradicts a zero-utoipa crate. What genuinely needed to be dependency-free was the vocabulary appearing in persisted artifacts, HTTP paths, and SSE payloads — the identifiers and lifecycle enums.Follow-ups deliberately left out
docs/architecture-walkthrough/(untracked onmain) is unrelated to this branch and not included.