Skip to content

refactor(core): separate persisted decode boundaries - #3562

Open
Sun-GLiang wants to merge 8 commits into
apache:mainfrom
Sun-GLiang:refactor/3413-persisted-decode-boundary
Open

refactor(core): separate persisted decode boundaries#3562
Sun-GLiang wants to merge 8 commits into
apache:mainfrom
Sun-GLiang:refactor/3413-persisted-decode-boundary

Conversation

@Sun-GLiang

Copy link
Copy Markdown
Contributor

Summary

Separate strict decoding for new input and wire data from compatibility decoding at durable-storage boundaries.

  • introduce invariant PersistedValue<T> branding for raw durable values
  • add persisted decoders for AgentRun, SessionHeader, StoredMessage/ToolResult, and ScheduledTask records
  • fold retired AgentRun states/fields and execute permission modes only on persisted read paths
  • cover RuntimeEvent replay, conversation copy/archive, continuation claims, pending scheduled fires, metadata, catalogs, transcripts, and backup restore
  • keep create, update, import, append, live tool output, and protocol paths strict
  • perform no database rewrite, schema migration, or compatibility epoch bump

Fixes #3413

Verification

  • npm --workspace @maka/core test — 629 passed
  • npm --workspace @maka/storage test — 902 passed, 14 skipped
  • focused RuntimeEvent read-model and conversation-copy tests — 68 passed
  • npm --workspace @maka/runtime test — 3072 passed, 13 skipped; one pre-existing environment-dependent macOS executable-root assertion fails when Node is installed at /usr/local/bin/node
  • npm run lint
  • npm run format:check
  • npm run build
  • npm run typecheck
  • git diff --check

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Codex implemented the persisted decoding boundary refactor and regression tests under human review. Each substantive commit includes a Generated-by: Codex trailer.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@Astro-Han Astro-Han 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.

Reviewed at exact head 35344e23bda0bd2615c36677cd31299e2767d074.

Coverage of this review: behavioural equivalence of the persisted-decode split (old data read through the new path), error-propagation paths, and reachability of the removed code. Not covered: naming/file organisation, UI.

Result: no P0–P2 findings in the covered scope.

What was checked

  • AgentRun header. The old path validated shape, then folded status / permissionMode, then validated. The new path splits this into a persisted fold (waiting_permissionwaiting_for_user map, decodePersistedPermissionMode) followed by a strict canonical validation (isPermissionMode). Accept/reject outcomes were compared field by field for retired spellings, unknown junk, and missing fields — they match.
  • Tool-result content. Subagent permissionMode folding moved out of the canonical decoder into the persisted variant. The folding scope is unchanged (subagent only).
  • Call sites. Every persisted read now uses the persisted variant with markPersisted (conversation-copy ×3, read-model, session-store/sqlite metadata read+write, Desktop/CLI loadTranscript). Write paths and protocol frames stay canonical, which is correct for in-process domain data. subagent-tools / tool-runtime construct fresh in-process content, so canonical is right there too.
  • Removed code. foldRetiredPermissionMode has no remaining callers. The continuation-claim persisted read at packages/storage/src/sqlite-runtime-store.ts:3723 correctly gained decodePersistedAgentRunHeader, so the strict decode inside claim receives an already-folded header — the easiest one to miss in this refactor, and it is not missed.

Two deliberate tightenings, both fail-closed

Unknown effect.kind, agent_run missing execution, and an undecodable permissionMode in scheduled tasks now throw where the old code passed them through to the execution layer. The effect union only has notify / session_resume / agent_run, so valid historical data is unaffected. Closing the "accept a corrupt record as valid" door is the right direction.

[P3] Batch decode failure has no bad-row skip

packages/storage/src/scheduled-task-store.ts:598-621 — in the list/claim batch read, one row failing to decode throws for the whole batch (previously an unknown kind passed silently). This is consistent with the existing semantics, since JSON-layer corruption already fails the whole batch, and it is not a regression introduced here. Recording it only as an operational-resilience observation: if a single bad row ever appears, the entire task list becomes unreadable with no degraded path that skips it. No change requested.

Verification

Targeted suites run locally against the exact head after building: core 48/48 (including the new persisted-value-contract tests), storage 121/121, runtime conversation-copy 11/11 and read-model 57/57, runtime-host 32/32 — 269 green.

Limits, stated plainly:

  • Hosted CI has not run on this head — check-runs total_count is 0 and the combined status is pending with zero statuses. That is neither green nor red, so this review is a comment only and does not clear the CI gate.
  • Equivalence is established by source-level comparison plus targeted tests. No real legacy database file was constructed for an end-to-end migration test; retired-spelling coverage rests on the old and new unit-test assertions.

@Astro-Han Astro-Han 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.

Follow-up to my earlier comment: hosted CI had never run on this head, so I triggered it. It has now run and test fails — for a reason unrelated to anything in my previous review.

Protocol epoch guard: Protocol files changed but RUNTIME_HOST_COMPATIBILITY_EPOCH is still 39,
the current base parent's value.
Changed files without a compatible-change declaration:
  packages/runtime-host/src/protocol/session-turns.ts

The guard's own message states the remedy: same-number bumps on sibling branches merge without a git conflict (#3313), so a protocol change has to land with an epoch the current base has not seen. Rebase onto current main and set RUNTIME_HOST_COMPATIBILITY_EPOCH past 39 — or, if you consider the session-turns.ts change compatible, add the compatible-change declaration the guard is looking for.

My review findings are unchanged: still no P0–P2 in the covered scope, with the one [P3] on batch decode failure. This is purely the CI gate that was previously invisible because no run existed.

@Sun-GLiang

Copy link
Copy Markdown
Contributor Author

Conflicts have been resolved. Please rerun the CI checks. Thanks!

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.

refactor(core): retired-value folding is re-implemented per decoder with nothing enforcing it

2 participants