Skip to content

feat(runtime): export one Session as a portable bundle - #5113

Merged
M4n5ter merged 5 commits into
apache:mainfrom
Joob1n:feat/session-export-bundle-0a
Sep 10, 2026
Merged

feat(runtime): export one Session as a portable bundle#5113
M4n5ter merged 5 commits into
apache:mainfrom
Joob1n:feat/session-export-bundle-0a

Conversation

@Joob1n

@Joob1n Joob1n commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Export one Session — and the subagent subtree under it — as a single portable .maka-session file, through exportSessionBundleState() rather than beside it.

The first version of this PR built its own export: an eight-table allow-list serialised to JSONL. @likun666661 and @MicroGery pointed out that the repository already had a Session export boundary, that four things were wrong with mine, and that three of the four were already solved over there. They were right, and this is the rebuild. The conversation below has the details; what follows describes what is actually being merged.

What was already there, and is now used

exportSessionBundleState() prepares a Session for a bundle:

  • A consistent snapshot. lease.backup() freezes the database. Separate reads under readOnly: true — what the first version did — do not.
  • Deny by default. The filter deletes what the Session does not own instead of selecting what it does, so a table added later is emptied rather than silently omitted, and PRAGMA foreign_key_check proves the result.
  • A writer boundary. withOfflineContextSnapshot() and withArtifactWriterLock().
  • The context-offload closure. planContextSnapshotFiles() / copyContextSnapshot(). A Session holding a read-image snapshot exported clean from the first version and would have arrived without its bytes.
  • Filesystem safety. Artifact metadata is decoded through a strict codec whose relativePath must equal <sessionId>/<id>-<name>, and assertRegularFile() rejects symlinks and anything that is not a regular file. The containment guarantee is the metadata contract: a record naming a path outside its own Session does not decode at all.

What this PR adds to it

Four options, each defaulting to the function's previous behaviour so production-session-snapshot is unchanged.

includeSubtree. Migration starts wherever it is pointed and takes the subtree hanging below, so the named Session is often not a top-level one — a mid-tree child, a leaf, or a branch Session all export as roots of their own bundle, leaving their ancestors behind. A subagent child holds the result of a tool call its parent made, so the descendants have to travel with it. The walk tracks membership, because subagent_parent_session_id is an ordinary column and nothing stops a row from naming itself or an ancestor.

requireQuiescent. A partial stream snapshot or an invocation with no terminal event refuses the export, and one active child refuses the tree. The check runs on the backed-up copy, not the live database: the locks held here do not stop a turn from starting, and lease.backup() is what actually freezes the content. That is the only place where what was checked and what ships are the same bytes.

omitDiagnostics. core_agent_run_events rows describing a request rather than the conversation are dropped — 98% of that table in a real Session, and none of them reach the model. Not on that list: the two kinds that decide what the model reads (history_compact_checkpoint_recorded, model_projection_transition_recorded), and any type this build has never seen.

Schema validation. The filter runs DELETE over whatever tables the database has, so a schema this build cannot read yields a bundle whose shape will not match its manifest. The registry the database keeps about itself is checked before the plan is made, and its versions are what the manifest reports — not this build's constants.

Then the thin part in @maka/runtime: seal the prepared directory with the bundle codec, carry a manifest as the state identity, and expose maka session-export.

Two corrections to the filter, both pre-existing

Ownership and lineage were conflated. session_id says whose row it is; session_metadata.parent_session_id names a different Session the row descends from. Reading the second as ownership deletes the very Session being exported whenever its source lies outside the subtree — a branch Session failed its own export for naming something the bundle is supposed to leave behind. Ownership is session_id when the table has one; the other columns are Session columns only on tables that have none, which is what a link table is.

A link table was emptied wholesale. That is why those columns are read at all: subagent_spawns spells its pair parent_session_id / child_session_id, so under the original three-name list it looked Session-less and was deleted entirely. A subtree bundle carried the child Sessions and none of the records saying which tool call spawned each one. Probed before fixing: source 1 row, bundle 0 rows. Nullable columns count only when set.

missing_entry. A record naming bytes the workspace does not have now reports separately from a state root that is not there. They are different mistakes and a caller should be able to tell them apart.

No credentials

The bundle names the connection by slug and model. The importing side resolves the slug against its own catalog; where it cannot, the existing stale-connection state already says so. Configuration is kept out by the policy's state allow-list, not by root separation — Maka's desktop layout keeps state and configuration in one directory. There is a test that plants a vault beside the state being read, hydrates the bundle, and greps every file in it.

To be precise about the claim: this means application configuration credentials. History is carried byte for byte, so a user message or tool output may itself contain a secret, and no export can promise otherwise.

Tests

12 in @maka/runtime, plus the 20 existing tests across session-bundle-policy, context-offload-snapshot and artifact-writer-lock, which all still pass — that is the check that the extension did not change the snapshot and backup callers.

Notable coverage: export from four kinds of starting node — a top-level parent, a mid-tree child whose parent stays behind, a leaf, and a branch Session whose source stays behind — each carrying exactly its own subtree; the subagent link survives (subagent_spawns rows for all three descendants); JSON columns come back as the same bytes, using an integer past Number.MAX_SAFE_INTEGER and non-canonical spacing that a re-encoding implementation cannot preserve; an unknown event type is carried while diagnostics are dropped; a cycle in the parent link terminates; a mid-turn child refuses the tree.

Assertions were checked by deleting the implementation they cover: the diagnostics filter, the subtree walk, the ownership/lineage separation, the link-table column list, the cycle guard (which hangs without it, exit 124 under timeout 15), the workspace-existence check, and re-encoding the JSON columns. Each turns the corresponding test red.

Gates: @maka/core, @maka/storage, @maka/runtime build and typecheck clean; biome check on every changed file; check:asf-headers passes. No schema change, no protocol epoch change.

Self-review

Reviewing the first rebuild against the path it extends turned up four defects, all mine, all fixed here:

The delete predicate was inverted. The original joined <> ? with OR, so a row survived only when every session column named the Session. My subtree version joined NOT IN with AND, which keeps a row when any column matches — a link with one end outside the bundle would have arrived pointing at a Session that is not there. The comment claimed to preserve the original shape while doing the opposite.

Two options changed a second caller. Subtree and diagnostics went into the shared filter unconditionally, so production-session-snapshot silently started pulling in child Sessions and dropping diagnostics. Both are options now.

Quiescence was checked against the wrong bytes — the live database, before the backup. Moved onto the copy.

The ownership-column gap described above.

Two things I found in my own fixtures while chasing an artifact that would not appear in the bundle, worth stating because they mean the first version's artifact tests proved less than they looked like they did: the metadata codec takes an exact key set and requires relativePath === '<sessionId>/<id>-<name>', so my hand-rolled records decoded to nothing; and artifact_records has no status column in the current schema, so the "deleted artifact" case I had written a test for does not exist.

The open question from the first round — whether exporting a subagent child on its own should be allowed — is settled as yes: migration starts from any node and takes the subtree below it. Chasing that decision through the filter is what surfaced the ownership/lineage conflation above, since a bundle rooted at a non-top-level Session is exactly the case where a lineage pointer leaves the set.

What this PR does not do

Import, id-conflict handling, and the copy path for a Session that already exists locally are the next PR. Extracting this archive is not importing a Session, so nothing here claims model-history equivalence — that acceptance test belongs there. Desktop menus, IPC and the protocol epoch come after. Flags for usage tables and profile-name resolution come later still.

@github-actions github-actions Bot added the effort/XL Under 2500 readable lines label Sep 9, 2026
Comment thread packages/runtime/src/session-export.ts Outdated
let files = 0;
let bytes = 0;
for (const record of records) {
const relativePath = String(record.relative_path);

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.

Could we validate relative_path and record_json through the normal artifact-metadata boundary before using this path? Raw artifact_records rows have no database-level containment guarantee, while join(workspaceRoot, "artifacts", relativePath) resolves ../… outside the artifact root and copyFile follows symlinks. A malformed row could therefore put a neighboring workspace file into a shareable bundle despite the no-credentials contract. Please fail closed on unsafe or mismatched metadata and non-regular source files, with a regression case such as ../credential-vault.json.

Comment thread packages/runtime/src/session-export.ts Outdated
rootSessionId: input.sessionId,
sessionIds,
schema: {
runtime: SQLITE_RUNTIME_SCHEMA_VERSION,

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.

Could this validate the source database schema, or serialize versions read and verified from that source, before emitting this manifest? This exporter opens runtime.sqlite directly and bypasses the operational-store current-schema check. An older or unsupported database can therefore produce a bundle whose manifest advertises the exporter build’s current versions, giving a future importer a false compatibility signal. Failing closed, or recording the actual supported source versions, would keep the bundle contract truthful.

@MicroGery MicroGery 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.

How should this format relate to the Session Root direction in #4666? That discussion treats a portable/rehydratable Session as a manifest of an event-log head, workspace snapshot, artifact references, lineage, and an owner-controlled control-plane cut. This PR intentionally copies selected SQLite rows and artifact bytes while omitting the workspace snapshot. That is coherent if 0a is explicitly a model-visible conversation-history export, but it is not yet a general portable Session, fork, or cross-worker rehydration contract. Could we state that boundary here and avoid letting the physical table list become the long-term Session identity contract?

@likun666661

Copy link
Copy Markdown
Member

Reviewed head ae8d314d4. I would address the following before merging:

Findings

  1. [P1] The export omits context-offload data needed to restore image history. The selected tables and artifacts/ are not the complete dependency closure. Read-image snapshots produce session_context references backed by context-offload.sqlite and context-offload-values/, neither of which this exporter carries. A Session containing these references can export successfully while its image bytes are absent. The existing planSessionBundleExport() already includes context snapshot files. Please carry the required context references/blobs for the exported subtree and add a test that resolves an exported image reference without access to the source workspace.

    References: export selection, image snapshot reference, existing context export.

  2. [P1] Artifact copying follows symlinks and can include credentials outside the artifact root. copyExportArtifacts() directly joins relative_path and calls copyFile() without validating path containment or the source file identity/type. I reproduced this using the extracted, otherwise unchanged helper: a live artifact path symlinked to a temporary credential-vault.json copied the test secret into the staging directory successfully. At that point the codec sees an ordinary file, so its own symlink rejection cannot protect the source boundary. Please enforce safe paths and source containment, reject unsafe links, and cover this case in the credential test.

    Reference: artifact copy.

  3. [P1] Quiescence checks and state reads do not form a consistent snapshot. Tree discovery, activity checks, and the table reads execute without a shared read transaction or writer-exclusion boundary. Another process can start an invocation after the check or commit between table reads, yielding an active Session, a missing newly spawned child, or mismatched events and ordinals. readOnly: true does not freeze other connections. Please establish a consistent database snapshot encompassing discovery, checks and reads, and protect the corresponding artifact/context bytes for snapshot preparation. The existing SessionSnapshotQuiescenceAuthority documents the required writer boundary. Add concurrent-writer coverage, not just fixtures that are already active before export.

    References: check/read sequence, existing quiescence contract.

  4. [P2] The manifest reports build-time schema versions without validating the source database. The CLI opens an arbitrary supplied workspace directly, but manifest.schema uses current constants. An older/newer database whose tables still satisfy these SELECTs can therefore be exported under an incorrect schema label. Please inspect and validate the actual source schema before export, or record its actual supported versions. The repository already has inspectOperationalStateSchema() for this boundary.

    Reference: manifest versions.

Problem definition / Occam review

A smaller outcome-oriented contract would be: Export a self-contained, consistent snapshot of a quiescent Session and its descendants, sufficient to reconstruct existing model-visible history, without copying application configuration credentials.

The eight-table list and JSONL representation are implementation choices, not the problem definition. In particular:

  • The repository already has session-bundle-policy.ts, production-session-snapshot.ts, context snapshot handling, and filesystem safety mechanisms. Please explain why extending/reusing that path is insufficient before establishing another export representation and preparation path. This PR adds subtree selection and selective ledger export; it is not the first definition of a Session export boundary.
  • Keeping JSON column strings unchanged is useful, but does not prove model-history equivalence when referenced bytes are missing. A restore-and-materialize test is the relevant acceptance criterion; extracting the archive is not importing a Session.
  • The no-credentials claim should mean application configuration credentials are not copied. Arbitrary user messages/tool outputs may themselves contain secrets, so byte-preserving history cannot also promise that no credential exists anywhere in the bundle.
  • Deferring import conflict handling, desktop UI, and optional diagnostics is reasonable. Snapshot consistency, reference closure, filesystem safety, and schema validation cannot be deferred because they determine whether the exported artifact is valid.

Verification scope: reviewed the full diff and related storage code, and ran the isolated symlink-copy reproduction. I did not run the full build or the PR test suites. Findings 1, 3 and 4 are based on code-path analysis, not an end-to-end import reproduction.

@Joob1n

Joob1n commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

You were both right — I missed planSessionBundleExport() on my first pass and built a weaker version of it next door. Rebuilt on exportSessionBundleState().

That path already had the consistent snapshot, the writer locks, and the context closure, so findings 1–3 are its, not a second version of them. Filesystem safety turned out to be the artifact metadata contract itself: a record whose relativePath isn't <sessionId>/<id>-<name> doesn't decode. Finding 4 is new code — the manifest now reports the versions the source registers, and refuses a schema this build can't read.

What I added there is four options — subtree, quiescence, diagnostics, schema — each defaulting to the old behaviour, so production-session-snapshot is unchanged and its tests still pass.

Two filter bugs found on the way, both pre-existing:

  • subagent_spawns spells its columns parent_session_id / child_session_id, so it looked Session-less and was emptied wholesale. A subtree bundle carried the child Sessions and nothing saying which tool call spawned them.
  • Ownership and lineage were conflated once I started reading those columns. session_id says whose row it is; session_metadata.parent_session_id names a different Session. Reading the second as ownership deleted the Session being exported whenever its source lay outside the subtree.

On finding 3 specifically: quiescence now runs on the backed-up copy. The locks don't stop a turn from starting, and lease.backup() is what freezes the content, so the copy is the only place where what was checked and what ships are the same bytes.

Also took the wording points — it says application configuration credentials now, and doesn't claim model-history equivalence, since extracting an archive isn't importing a Session.

The open question is settled: migration starts from any node and takes the subtree below it, so a mid-tree child or a branch Session exports as its own root. That's what surfaced the ownership bug above.

History is rewritten to describe what's merging rather than the abandoned approach.

@Joob1n
Joob1n marked this pull request as draft September 9, 2026 15:11
…scope

`exportSessionBundleState()` already prepares a Session for a bundle: a
consistent database snapshot through `lease.backup()`, the context and
artifact writer locks around it, a filter that deletes what the Session does
not own rather than selecting what it does, the context-offload closure, and
`PRAGMA foreign_key_check` to prove the result. Nothing calls it for a
portable bundle yet, and four things are missing before something can.

**Subtree** (`includeSubtree`). A subagent child holds the result of a tool
call its parent made, so a bundle of the parent alone is a conversation with a
hole in it. The walk tracks membership: `subagent_parent_session_id` is an
ordinary column and nothing stops a row from naming itself or an ancestor.

**Quiescence** (`requireQuiescent`). A partial stream snapshot or an
invocation with no terminal event refuses the export, and one active child
refuses the tree. The check runs on the backed-up copy rather than the live
database, because the locks held here do not stop a turn from starting and
`lease.backup()` is what actually freezes the content — this is the only place
where what was checked and what ships are the same bytes.

**Diagnostics** (`omitDiagnostics`). `core_agent_run_events` rows that describe
a request rather than the conversation are dropped. In a real Session they are
98% of that table and none of them reach the model. The two kinds that decide
what the model reads — `history_compact_checkpoint_recorded` and
`model_projection_transition_recorded` — are deliberately not on that list, and
neither is any type this build has never seen: an export moves rows, it does
not interpret them.

**Schema validation.** The filter runs `DELETE` over whatever tables the
database happens to have, so a schema this build cannot read produces a bundle
whose shape will not match what its manifest claims. The registry the database
keeps about itself is now checked before the plan is made, and its versions are
what the plan reports.

Two corrections to the filter itself, both of which applied before this change:

- `SESSION_OWNING_COLUMNS` names each spelling the schema uses. The filter
  recognised three, and `subagent_spawns` uses `parent_session_id` and
  `child_session_id` — so it looked Session-less and was emptied wholesale,
  taking with it the record of which tool call spawned each child. Nullable
  columns count only when set.
- A missing planned entry reports `missing_entry` rather than `invalid_root`.
  A record naming bytes the workspace does not have and a state root that is
  not there are different mistakes.

Every option defaults to this function's previous behaviour, so
`production-session-snapshot` is unchanged.

Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
Seal a prepared Session into a single `.maka-session` file, and expose it as
`maka session-export` so the round trip can be exercised before any surface
commits to it.

The preparation is `exportSessionBundleState()`; this is the thin part on top
of it. What it adds is the manifest the codec carries as the bundle's state
identity: the Sessions inside, the schema versions the SOURCE database
registers, the state entries the policy included and the ones it left behind,
and the connection by slug and model.

No credential is carried. The importing side resolves the slug against its own
catalog, and where it cannot, the existing stale-connection state already says
so. This means application configuration, not that no secret can appear
anywhere in a bundle: history is carried byte for byte, and a user message or
tool output may contain one.

Failures are distinct because a script should be able to tell them apart: a
directory that was never a workspace reports `workspace_not_found` rather than
`session_not_found`, since a workspace holding no Sessions still has a database
and a mistyped path must not read as an empty catalog.

Extracting this archive is not importing a Session, so nothing here claims
model-history equivalence. That acceptance test belongs with the import PR.

Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
@Joob1n
Joob1n force-pushed the feat/session-export-bundle-0a branch from 29b4187 to 0a32c74 Compare September 9, 2026 16:11
@Joob1n
Joob1n marked this pull request as ready for review September 9, 2026 16:12
@Joob1n
Joob1n requested a review from MicroGery September 9, 2026 16:20
Migration starts wherever it is pointed and takes the subtree hanging below,
so the named Session is often not a top-level one. That exposed a conflation in
the filter: `session_id` says whose row this is, while `parent_session_id` on
`session_metadata` names a DIFFERENT Session it descends from.

Reading the second as ownership deleted the very Session being exported. A
branch Session whose source lay outside the subtree failed its own export --
its row was dropped for naming a Session the bundle does not contain, which is
exactly what a bundle rooted here is supposed to leave behind.

Ownership is `session_id` when the table has one. The other columns are Session
columns only on tables that have no `session_id`: link tables, whose whole
content is the pair they join and which mean nothing when one end is missing.
`subagent_spawns` is the one that matters, and it is why those columns are read
at all.

Covered by exporting from four kinds of node -- a top-level parent, a mid-tree
child whose parent stays behind, a leaf, and a branch Session whose source
stays behind -- each carrying exactly its own subtree.

Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
`./artifact-metadata-codec` was exported for a throwaway probe while chasing an
artifact that would not appear in a bundle. The probe is gone; the promise it
needed should not outlive it.

Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J

@M4n5ter M4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

English

Disclosure: this is an AI-assisted review prepared by Codex and verified against commit f576eade4299c8d9b9d616087aa0d3d44fea701f.

Verdict

I do not think this revision is mergeable yet. I found five blocking correctness/security issues and one important error-contract issue.

  1. [BLOCKING][privacy] The filtered SQLite file still contains data from excluded Sessions, and ownerless rows remain query-visible. The filter deletes rows and then ships the same database file, so deleted content can remain in SQLite freelist pages. In an end-to-end export/hydrate repro, SQL reported zero excluded rows, but freelist_count was 250 and the excluded marker was still present in the raw runtime.sqlite bytes. Separately, the owner predicate preserves rows whose owning session_id is NULL; I reproduced an unattributed usage row in the bundle. Please distinguish an owner column from nullable link endpoints, delete ownerless rows, and rebuild/VACUUM the private copy after filtering. A regression should inspect the raw bundled database, not only query results.

  2. [BLOCKING][consistency] Planning and export use different database snapshots. The Session tree, artifacts, schema, and route are read from the live database, while the database that actually ships is backed up later. The artifact/context locks do not fence general Session or runtime writers. A child Session or artifact can therefore be created between those points, producing a database, copied files, and manifest that describe different moments. Take the backup first, then derive subtree membership, schema, route, artifacts, quiescence, filtering, and manifest from that single copy.

  3. [BLOCKING][source mutation/schema truth] A read-only export can migrate the source and emit a self-contradictory manifest. The local schema authority checks only five scopes, omitting workflow and usage, although the central authority defines seven. The later default database acquisition may migrate the live source. With the usage registry downgraded from 7 to 6, export succeeded, upgraded the source back to 7, shipped registry version 7, and reported usage: 6 in the manifest. Reuse the central exact-schema inspector, require a current/non-migrating source, and derive the manifest from the backed-up copy. Please add a real schema_unsupported regression.

  4. [BLOCKING][path safety] A symlink in an artifact's ancestor path escapes the state root. Planning checks only the final path, and that check is a final-component lstat; copyFile then follows ancestor symlinks. I reproduced a valid artifact record with artifacts/<session-id> symlinked outside the workspace: export succeeded and copied the external secret. Artifact reads need a race-safe “beneath this root, no symlinks” boundary and should copy from a stably opened file descriptor; add an ancestor-symlink regression.

  5. [BLOCKING][quiescence] Unsettled tool operations are accepted as a complete Session. The quiescence check only tests partial snapshots and invocations without a terminal runtime event. A prepared tool operation can remain unsettled even when its invocation has a terminal failed event, and the cleanup retains it whenever that invocation exists. I reproduced listUnsettledToolOperations() === 1 while export returned success and shipped the prepared operation. Reject every unsettled tool operation in the exported subtree, using the backed-up database from finding 2.

  6. [IMPORTANT][error contract] A destination publication race reports the wrong failure. There is an early existence precheck, but the packer's atomic no-replace publication can still lose a race. Its SessionBundleFileError('destination_exists') is mapped to io_failed, so the CLI returns 1 instead of its declared exit code 5. Preserve the precheck for fast failure, but map the packer error code to destination_exists too.

Non-blocking simplification

  • Replace the duplicated portable-schema map/validator with the existing operational-state schema authority; this also fixes finding 3.
  • The three independent booleans on SessionBundleExportInput expose eight combinations, while production has only two meaningful profiles. A single snapshot | portable mode would make invalid combinations unrepresentable and preserve the current snapshot default.
  • manifest.connection duplicates SQLite metadata and describes only the root even though descendants may use different routes. Unless V1 has an explicit pre-hydration preview requirement, remove it and keep SQLite as the sole authority; otherwise define and represent per-Session routes.

Verification: core and storage builds passed; 12 runtime Session-export tests and 20 storage bundle-policy/context/lock tests passed. Those tests do not exercise the adversarial cases above.

中文

说明:这份审查由 Codex 辅助完成,并已针对提交 f576eade4299c8d9b9d616087aa0d3d44fea701f 核验。

结论

这版暂时不宜合并:有 5 个阻断性的正确性/安全问题,以及 1 个重要的错误契约问题。

  1. [阻断][隐私] 被排除 Session 的内容仍残留在 SQLite 文件里,而且无归属行仍可查询。 当前逻辑删完记录后直接打包同一个数据库文件,SQLite 空闲页里可能继续留着已删除的内容。端到端导出再解包后,SQL 查询看不到排除行,但 freelist_count 为 250,直接扫描 runtime.sqlite 仍能找到排除 Session 的标记。另一个问题是,归属判断条件 会保留 session_id IS NULL 的行;复现中导出包里留下了 1 条无法归属 Session 的 usage 记录。需要把“行归属列”和“可空的关联端点”分开处理,删除无归属行,并在过滤后重建或 VACUUM 私有副本。回归测试应检查包内数据库的原始字节,而不只是 SQL 查询结果。

  2. [阻断][一致性] 规划和实际导出取自两个不同的数据库时刻。 Session 子树、artifact、schema 和路由先从在线数据库读取,真正装进包里的数据库却在稍后才备份。artifact/context 锁挡不住普通的 Session 和 runtime 写入;两次读取之间若新建了子 Session 或 artifact,数据库、复制出的文件和 manifest 就会各自描述不同的状态。应先备份,再从这一个副本完成子树枚举、schema/路由读取、artifact 规划、静止性检查、过滤和 manifest 生成。

  3. [阻断][源数据/schema 真实性] 本应只读的导出会迁移源数据库,还可能生成自相矛盾的 manifest。 新增的 schema 判断只检查 5 个 scope,漏掉了 workflowusage;而现有中央定义一共有 7 个。随后按默认方式打开数据库还可能就地迁移源文件。把 usage registry 从 7 降到 6 后,导出仍然成功:源库被升回 7,包内 registry 也是 7,但 manifest 却写着 usage: 6。这里应复用现有的完整 schema 检查,要求源库已是当前版本且禁止导出流程迁移它,并从同一个备份副本生成 manifest;同时补上真正覆盖 schema_unsupported 的回归测试。

  4. [阻断][路径安全] artifact 路径的上级目录可以通过符号链接逃出状态目录。 规划阶段只检查最终路径,而且只是对末级做 lstat;后面的 copyFile 会跟随上级符号链接。复现方式是让一条合法 artifact 记录对应的 artifacts/<session-id> 指向工作区外部;导出成功,并把外部 secret 装进了包。读取 artifact 时需要用抗竞态的方式保证路径始终位于根目录之下、全程不跟随符号链接,并从稳定打开的文件描述符复制;还应增加上级目录为符号链接的回归测试。

  5. [阻断][静止性] 尚未结算的工具操作会被当成完整 Session 导出。 目前的检查只看 partial snapshot,以及没有终止事件的 invocation。即使 invocation 已记录终止性的 failed 事件,prepared 工具操作仍可能处于未结算状态;而清理逻辑只要 invocation 存在就会保留该操作。复现中 listUnsettledToolOperations() === 1,导出却成功并携带了这条 prepared 操作。应在第 2 点所述的备份副本上,拒绝子树内任何未结算的工具操作。

  6. [重要][错误契约] 目标文件发布竞态会返回错误的失败类型。 虽然前面做了存在性预检,但 packer 在最终原子发布时仍可能输掉竞态。它抛出的 SessionBundleFileError('destination_exists') 会被统一映射成 io_failed,导致 CLI 返回 1,而不是契约约定的 5。可以保留预检来尽早失败,但最终发布阶段也必须把该错误映射为 destination_exists

非阻断性的简化建议

  • 删除重复的 portable schema 表和校验器,直接复用 operational-state 的中央 schema 权威;这也会同时解决第 3 点。
  • SessionBundleExportInput 的 3 个独立布尔值产生 8 种组合,但生产代码实际只有两种有意义的配置。收敛成 snapshot | portable 单一模式,可以直接排除无效组合,同时保持现有 snapshot 默认行为。
  • manifest.connection 与 SQLite 元数据重复,而且只描述根 Session,子 Session 却可能使用不同路由。如果 V1 没有明确的“解包前预览”需求,建议删除该字段,让 SQLite 成为唯一权威;如果确实有这个需求,则应先定义并表达每个 Session 的路由。

验证情况:core 和 storage 构建通过;12 个 runtime Session 导出测试、20 个 storage bundle-policy/context/lock 测试通过。现有测试尚未覆盖上述对抗性场景。

Six findings from review, five of them blocking. All but one share a root:
the plan was read from the live database while a different, later copy was
what actually shipped.

**One moment, not two.** The backup is taken first, and the subtree, schema,
artifact list, quiescence check and manifest are all derived from that copy.
The artifact and context locks do not fence ordinary Session and runtime
writers, so a child Session created between the plan and the backup produced a
database, a set of files and a manifest describing different instants.

**A read-only export no longer migrates its source.** The default open upgrades
the database in place; the backup now demands a current schema instead. And the
schema check is the operational store's own inspector rather than a private
list, which was missing two of the seven scopes and could report versions the
source did not have.

**Deleted bytes leave the file.** `DELETE` frees pages without erasing them, so
the bundle shipped a database whose freelist still held excluded Sessions'
content -- invisible to SQL, plain in the bytes. The filtered copy is vacuumed.

**A row whose owner is NULL owns nothing.** The nullable guard was meant for
link endpoints, where naming no counterpart is not the same as naming one
outside the bundle. Applied to the owner column it kept unattributable rows.

**Symlinks are refused along the whole path.** `artifacts/<sessionId>` can
itself be a link, and `copyFile` follows ancestors, so an artifact record that
decodes perfectly could pull in a file from outside the workspace. Every
segment is checked and the bytes are read from a descriptor opened with
`O_NOFOLLOW`. Node has no `openat`, so a segment swapped between its check and
the open is narrowed, not closed; the comment says so.

**An unsettled tool operation is not a complete Session.** An invocation can
carry a terminal `failed` event while its operation is still prepared, so an
invocation check does not see it. Quiescence now uses the same predicate the
runtime store uses, so the word means one thing.

The remaining finding is in the runtime layer: the packer publishes atomically
and can lose the race the precheck cannot cover, and its `destination_exists`
was being flattened into a generic IO failure, giving the CLI a different exit
code for the same outcome.

Five adversarial tests, one per blocking finding, each verified to fail when
its fix is removed. The freelist one inspects the bundled file's bytes rather
than querying it, because that is the difference the finding is about.
@Joob1n

Joob1n commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

All six addressed. Thanks — the reproductions made these fast to confirm.

Five of them shared a root I had half-fixed and not finished: the plan was read from the live database while a later, different copy was what shipped. The backup is now taken first, and the subtree, schema, artifact list, quiescence check and manifest all come from that one copy (finding 2). With that in place the rest land where you said:

  • Freelist (1). The filtered copy is vacuumed. The regression checks the bundled file's bytes rather than querying it, since that is exactly the difference.
  • Ownerless rows (1). The nullable guard was meant for link endpoints — naming no counterpart is not the same as naming one outside the bundle — and applying it to the owner column kept rows that belong to nobody. Owner NULL is now deleted.
  • Schema (3). Replaced my private five-scope map with inspectOperationalStateSchema, and the backup opens with require_current so a read-only export cannot migrate its source. Manifest versions are read from the source registry.
  • Ancestor symlink (4). Every segment is checked and the bytes are read from a descriptor opened with O_NOFOLLOW. Node has no openat, so a segment swapped between its check and the open is narrowed rather than closed — the comment says that rather than implying otherwise.
  • Unsettled tool operations (5). Now uses the same predicate listUnsettledToolOperations uses, so "unsettled" means one thing. Your point that a terminal failed invocation can still hold a prepared operation is what my check missed.
  • destination_exists (6). Mapped from the packer error too; the precheck stays for fast failure.

Five adversarial tests, one per blocking finding, each verified to fail with its fix removed.

On the simplifications: the schema one is done as part of finding 3. The other two I would rather do in a follow-up than fold into this diff —

  • snapshot | portable instead of three booleans is right, and I would like the import PR to be the thing that proves the second profile is the only other one worth having.
  • manifest.connection — agreed it duplicates SQLite and describes only the root. I will drop it unless the import side turns out to need a pre-hydration preview, which is the question that should decide it.

Say the word if you would rather see either of those here.

@M4n5ter M4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

English

Disclosure: this is an AI-assisted re-review prepared by Codex and verified against commit dd30dc313d3680b4cea672d73036ae947ced6801.

Approved. The six findings from the previous review are addressed: the export is now derived from one private snapshot, filtered pages are vacuumed and ownerless rows removed, schema validation uses the central authority without migrating the source, artifact ancestor symlinks are rejected, unsettled tool operations fail quiescence, and the publication race preserves destination_exists.

One non-blocking observation remains. backupOperationalState() currently maps every database-acquisition failure to schema_unsupported, including environmental failures that the operational store deliberately preserves. With an unreadable runtime.sqlite (chmod 000), I reproduced schema_unsupported instead of io_failed. The narrow fix would be to translate only OperationalStateMigrationBlockedError and rethrow permission, busy, and I/O errors.

This fails closed and seems unlikely on the expected path, where the workspace has already been initialized and is readable, so I do not consider it merge-blocking. Please use your judgment on fixing it here or following up separately.

Verification: 17 runtime Session-export tests and 20 storage bundle/context/lock tests passed; core and storage builds, targeted runtime compilation, Biome, git diff --check, and the current GitHub checks passed.

中文

说明:这次复审由 Codex 辅助完成,并已针对提交 dd30dc313d3680b4cea672d73036ae947ced6801 核验。

同意合并。上一轮的 6 个问题都已处理:整个导出现在基于同一个私有快照;过滤后的数据库会执行 VACUUM,并清除无归属记录;schema 校验复用了中央权威,且不会迁移源库;artifact 上级符号链接会被拒绝;未结算工具操作无法通过静止性检查;最终发布竞态也能保留 destination_exists 语义。

还有一个非阻断问题:backupOperationalState() 会把打开数据库时的所有异常都映射成 schema_unsupported,包括 operational store 原本有意保留的环境错误。把 runtime.sqlite 权限设为不可读(chmod 000)后,我复现到返回 schema_unsupported,而不是 io_failed。最小修复是只转换 OperationalStateMigrationBlockedError,权限、busy 和 I/O 错误则原样抛出。

这个路径会安全失败,而且在正常流程中工作区已经初始化并可读,实际触发概率应该很低,因此我不把它视为合并阻断。是否在本 PR 顺手修复,或另行跟进,由作者判断即可。

验证情况:17 个 runtime Session 导出测试和 20 个 storage bundle/context/lock 测试通过;core、storage 构建、相关 runtime 文件定向编译、Biome、git diff --check 以及当前 GitHub checks 均通过。

@M4n5ter
M4n5ter merged commit 638fa48 into apache:main Sep 10, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XL Under 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants