feat: unify session resources and files - #190
Merged
arthur-zhang merged 35 commits intoAug 5, 2026
Merged
Conversation
…talog # Conflicts: # internal/db/filestore_cleanup.go
…talog # Conflicts: # internal/db/filestore_cleanup.go # internal/db/filestore_filesystems.go # internal/db/sessions.go
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR replaces ChangesSession resource and persistence model
Estimated code review effort: 5 (Critical) | ~90+ minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
pxguan
marked this pull request as ready for review
July 29, 2026 12:35
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/db/session_namespace_mutations.go (1)
430-445: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftOwned
filesrow is left active when a single file is removed.
RemoveFilestoreDirectorysoft-deletes the backingfilesrows for owned file resources in the subtree (Lines 519-537), butRemoveFilestoreFileonly retires the resource and enqueues object cleanup. Thefilesrow staysdeleted_at is nullwhile its object is deleted by the cleanup worker. Consequences:
- a dangling
filesrow pointing at a deleted object;activeFileReferenceQueryno longer blocks it, so a laterfiles.deletesucceeds and subtractsfiles_byteseven thoughfilestore_byteswas already released here — the ledger double-releases.The overwrite branch of
MoveFilestoreFile(Lines 212-224) has the same gap. Both should soft-delete the ownedfilesrow in the same transaction, using the directory path as the reference implementation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/db/session_namespace_mutations.go` around lines 430 - 445, Update RemoveFilestoreFile and the overwrite branch of MoveFilestoreFile to soft-delete the owned files row within the same transaction that retires the session namespace node, matching the existing RemoveFilestoreDirectory implementation. Ensure the files row is retired before cleanup can delete its object, while preserving the existing storage-delta and cleanup-job behavior.
🧹 Nitpick comments (2)
tests/sessions_file_resources_api_test.go (1)
798-813: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGlobal DDL on the shared
filestable can leak into other tests.
alter table files add constraint ... not validtakes an ACCESS EXCLUSIVE lock and applies database-wide for the duration, so any concurrently running test that inserts a scopedfilesrow fails spuriously; a failure in the deferredt.Fatalfalso leaves the constraint installed for the rest of the run. If this package can share a database with other test packages, consider forcing the write failure through a session-local mechanism instead (e.g. abefore inserttrigger on a savepoint, or asserting atomicity via a deliberately invalid blob) or at minimum guarantee the drop always executes withoutt.Fatalf.(The ast-grep SQL-injection hint on line 808 is a false positive:
constraintis a localconstand DDL identifiers cannot be parameterized.)♻️ Make cleanup non-fatal so the constraint never leaks
defer func() { if _, err := app.db.Pool.Exec( context.Background(), "alter table files drop constraint if exists "+constraint, ); err != nil { - t.Fatalf("drop catalog failure constraint: %v", err) + t.Errorf("drop catalog failure constraint: %v", err) } }()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/sessions_file_resources_api_test.go` around lines 798 - 813, Replace the global files-table constraint setup with a session-local failure mechanism, avoiding database-wide DDL and interference with concurrent tests. Update the test around the current constraint setup and cleanup so the intended insert failure and atomicity assertion remain covered; if DDL must remain, make deferred cleanup non-fatal and ensure it always executes without t.Fatalf.Source: Linters/SAST tools
internal/db/session_file_mounts_sqlx.go (1)
258-269: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
unbindSessionFileResourceTxis now only a lock acquisition — make that explicit.The body no longer unbinds anything (the mount lives on the
session_resourcesrow that the caller soft-deletes), so the name now misdescribes the behavior and future readers may assume path/file cleanup still happens here. Consider renaming to something likelockSessionFileResourceUnbindTxand adding a one-line comment stating that removing the resource row removes the/uploadsnode, and that no quota/object cleanup is required because Input Resources own no bytes.♻️ Suggested clarification
-func unbindSessionFileResourceTx( +// lockSessionFileResourceUnbindTx 只负责获取文件系统变更锁：Input Resource 的 +// 命名空间路径与 Resource 同行，调用方软删除该行即解除挂载；引用型资源不拥有 +// 字节，无需配额调整或对象清理。 +func lockSessionFileResourceUnbindTx( ctx context.Context, tx *sqlx.Tx, session Session, resource SessionResource, ) error { if resource.ResourceType != SessionResourceTypeFile { return nil } _, err := lockSessionFilestoreMutationTx(ctx, tx, session) return err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/db/session_file_mounts_sqlx.go` around lines 258 - 269, Rename unbindSessionFileResourceTx to reflect that it only acquires the mutation lock, such as lockSessionFileResourceUnbindTx, and update all call sites. Add a concise comment explaining that removing the session_resources row removes the /uploads node and no quota or object cleanup is needed because Input Resources own no bytes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/research/pr-182-session-file-catalog-summary.md`:
- Around line 1-13: Fix Markdown lint issues in the PR `#182` summary: keep the
introductory blockquote continuous by removing the blank line between its quoted
lines, and adjust the standalone lines beginning with “#182” (including the one
near the summary and the later occurrence) so they no longer parse as malformed
ATX headings while preserving their intended text.
In `@internal/db/files_sqlx.go`:
- Around line 46-63: The not-exists subquery in visibleFileSQLXPredicate cannot
use the existing partial file_uuid index because its predicates do not match the
index condition. Add a matching partial index on session_resources covering
workspace_id and file_uuid with file_uuid is not null and payload is null, or
restructure both subqueries to share an index-eligible predicate while
preserving the current visibility behavior.
In `@internal/db/filestore_cleanup.go`:
- Around line 199-215: Update both cleanup loops around
enqueueSessionNamespaceNodeCleanupJobTx to detect malformed file namespace nodes
with nil S3Bucket or S3Key before enqueueing. Skip those entries and log the
condition, mirroring the existing ReferencesSourceFile guard, so
ErrPreconditionFailed from malformed rows does not abort the transaction while
valid cleanup jobs continue.
In `@internal/db/filestore_scan.go`:
- Around line 92-112: Update the filestore_filesystems join in the query to
require filesystem.deleted_at is null alongside the existing session_uuid and
workspace_uuid predicates. Keep the remaining joins and filters unchanged so
retired filesystem rows cannot duplicate or restore namespace nodes.
- Around line 13-15: Update the filesystem row query returned by the visible
scan-query function so the correlated session lookup cannot produce a NULL for
the non-pointer filestoreFilesystemRow.SessionID field. Preserve the
missing-session case as an explicit domain-level ErrNotFound (using the existing
filesystem read path and error symbols), rather than allowing SQL scanning to
fail implicitly; ensure valid session rows continue returning their session ID.
In `@internal/db/migrations/00036_unify_session_resources_and_files.sql`:
- Around line 146-176: Update the migration’s file materialization flow around
the INSERT into files to reconcile workspace_storage_usage for every affected
workspace after the new rows are created. Ensure the reconciliation recalculates
files_bytes plus filestore_bytes, either inline per workspace or through the
established ReconcileWorkspaceStorageUsage step, so usage remains consistent.
In `@internal/db/session_namespace_helpers.go`:
- Around line 250-271: Map sql.ErrNoRows to an appropriate domain error, such as
ErrNotFound or ErrInvalidState, immediately after the directory insert’s
namedGetContext call and before its unique-violation handling. Apply the same
mapping to the inserted_file CTE insert at
internal/db/session_namespace_helpers.go lines 394-432; both sites require this
change so missing or archived sessions never expose the raw sql.ErrNoRows
sentinel.
In `@internal/db/session_namespace_mutations.go`:
- Around line 243-245: Update the moved-node re-fetch in the session namespace
mutation to include the same workspace_id and session_id tenant scope used by
the preceding update, in addition to id and deleted_at filtering. Adjust the
bound arguments passed to getSessionNamespaceNodeSQLX so the select cannot
return a node outside the current tenant.
In `@internal/db/workspace_storage.go`:
- Around line 61-77: The files_bytes query currently multiplies a file’s size by
each matching session_resources attachment. Update the aggregation to sum each
file once, using EXISTS/NOT EXISTS checks for active resource_type = 'file'
resources instead of joining session_resources, while preserving the workspace,
file deletion, payload, and resource activity filters.
In `@tests/sessions_api_test.go`:
- Around line 3611-3613: Update the ErrNotFound retry branch in the session API
test to wait or back off before continuing while the deadline remains active.
Preserve the existing deadline check and retry behavior, but ensure repeated
stale-work lookups cannot hot-spin.
---
Outside diff comments:
In `@internal/db/session_namespace_mutations.go`:
- Around line 430-445: Update RemoveFilestoreFile and the overwrite branch of
MoveFilestoreFile to soft-delete the owned files row within the same transaction
that retires the session namespace node, matching the existing
RemoveFilestoreDirectory implementation. Ensure the files row is retired before
cleanup can delete its object, while preserving the existing storage-delta and
cleanup-job behavior.
---
Nitpick comments:
In `@internal/db/session_file_mounts_sqlx.go`:
- Around line 258-269: Rename unbindSessionFileResourceTx to reflect that it
only acquires the mutation lock, such as lockSessionFileResourceUnbindTx, and
update all call sites. Add a concise comment explaining that removing the
session_resources row removes the /uploads node and no quota or object cleanup
is needed because Input Resources own no bytes.
In `@tests/sessions_file_resources_api_test.go`:
- Around line 798-813: Replace the global files-table constraint setup with a
session-local failure mechanism, avoiding database-wide DDL and interference
with concurrent tests. Update the test around the current constraint setup and
cleanup so the intended insert failure and atomicity assertion remain covered;
if DDL must remain, make deferred cleanup non-fatal and ensure it always
executes without t.Fatalf.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a3e41ae9-1f96-42ea-b4f8-525da356cdc2
📒 Files selected for processing (49)
docs/design/be/filestore.mddocs/design/be/managed-agent-skills-runtime.mddocs/research/anthropic-session-file-resource-contract.mddocs/research/pr-182-session-file-catalog-summary.mdinternal/db/AGENTS.mdinternal/db/files.gointernal/db/files_sqlx.gointernal/db/files_sqlx_test.gointernal/db/filestore.gointernal/db/filestore_archive_entries.gointernal/db/filestore_cleanup.gointernal/db/filestore_cleanup_sqlx_test.gointernal/db/filestore_entry_methods.gointernal/db/filestore_scan.gointernal/db/filestore_sqlx.gointernal/db/filestore_test.gointernal/db/migrations/00036_unify_session_resources_and_files.sqlinternal/db/migrations_postgres_test.gointernal/db/session_file_mounts_sqlx.gointernal/db/session_file_mounts_sqlx_test.gointernal/db/session_namespace_helpers.gointernal/db/session_namespace_mutations.gointernal/db/session_namespace_node_methods.gointernal/db/session_namespace_nodes.gointernal/db/session_skill_archive_resources.gointernal/db/session_skill_archive_resources_test.gointernal/db/sessions.gointernal/db/sessions_sqlx.gointernal/db/workspace_storage.gointernal/environments/runner.gointernal/filestore/cleanup.gointernal/filestore/cleanup_test.gointernal/filestore/persistent_backend.gointernal/filestore/protocol_test.gointernal/filestore/service.gointernal/filestore/service_test.gointernal/filestore/service_test_support_test.gointernal/filestore/skill_archives.gointernal/filestore/skill_archives_test.gointernal/skills/handler.gotests/deployments_api_test.gotests/environments_runner_cloud_test.gotests/files_api_test.gotests/filestore_db_test.gotests/filestore_fixed_roots_test.gotests/filestore_provision_roots_test.gotests/sessions_api_test.gotests/sessions_file_resources_api_test.gotests/workspace_storage_usage_test.go
💤 Files with no reviewable changes (2)
- internal/db/filestore_entry_methods.go
- internal/db/filestore_archive_entries.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/filestore/cleanup_test.go (1)
255-282: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePlace this success case after the failure case.
TestCleanupWorkerRunTTLSweepOnceLogsMetadataAnomaliessucceeds, but it precedesTestCleanupWorkerRunTTLSweepOnceReturnsDatabaseError. Move it below the failure test. As per coding guidelines, “**/*_test.go: 测试组织顺序先写失败场景，再写成功场景”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/filestore/cleanup_test.go` around lines 255 - 282, Reorder the tests so TestCleanupWorkerRunTTLSweepOnceReturnsDatabaseError appears before the successful TestCleanupWorkerRunTTLSweepOnceLogsMetadataAnomalies case. Do not change either test’s implementation or behavior.Source: Coding guidelines
internal/db/filestore.go (1)
273-280: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse unified Resource terminology in the cleanup contract.
This PR removes
filestore_entries, but the new exported field still usesEntryExternalIDand produces anentry_external_idoperational key. Rename it toResourceExternalIDorSessionResourceExternalIDand update the worker/tests/logging together.Proposed rename
- EntryExternalID string + ResourceExternalID string🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/db/filestore.go` around lines 273 - 280, Rename the exported FilestoreCleanupAnomaly field EntryExternalID to ResourceExternalID (or SessionResourceExternalID), then update every worker, test, and logging reference to use the corresponding unified resource operational key instead of entry_external_id.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/db/migrations/00036_unify_session_resources_and_files.sql`:
- Around line 277-279: Update the session_resources_owned_file_uuid_v1_idx
migration to build the index with CREATE INDEX CONCURRENTLY and mark the Goose
migration as NO TRANSACTION, preserving the existing columns and partial-index
predicate.
---
Nitpick comments:
In `@internal/db/filestore.go`:
- Around line 273-280: Rename the exported FilestoreCleanupAnomaly field
EntryExternalID to ResourceExternalID (or SessionResourceExternalID), then
update every worker, test, and logging reference to use the corresponding
unified resource operational key instead of entry_external_id.
In `@internal/filestore/cleanup_test.go`:
- Around line 255-282: Reorder the tests so
TestCleanupWorkerRunTTLSweepOnceReturnsDatabaseError appears before the
successful TestCleanupWorkerRunTTLSweepOnceLogsMetadataAnomalies case. Do not
change either test’s implementation or behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2aaa21b6-a307-4169-b821-ab509891e1ed
📒 Files selected for processing (17)
docs/design/be/filestore.mdinternal/db/files_sqlx_test.gointernal/db/filestore.gointernal/db/filestore_cleanup.gointernal/db/filestore_scan.gointernal/db/filestore_sqlx.gointernal/db/migrations/00036_unify_session_resources_and_files.sqlinternal/db/session_namespace_helpers.gointernal/db/session_namespace_mutations.gointernal/db/session_namespace_review_test.gointernal/db/workspace_storage.gointernal/filestore/cleanup.gointernal/filestore/cleanup_test.gotests/filestore_db_test.gotests/sessions_api_test.gotests/sessions_file_resources_api_test.gotests/workspace_storage_usage_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/sessions_api_test.go
- internal/db/workspace_storage.go
- internal/db/filestore_sqlx.go
- internal/db/session_namespace_mutations.go
- internal/db/session_namespace_helpers.go
- internal/db/filestore_cleanup.go
- tests/filestore_db_test.go
- tests/sessions_file_resources_api_test.go
Collaborator
Author
Session Resource 与 File 统一持久化
建议评审顺序
变化概览flowchart LR
subgraph Before["统一前：三份事实"]
A["session_resources\nResource 声明"]
B["filestore_entries\npath + object"]
C["files\n真实 File + projection"]
A <--> B
B <--> C
end
subgraph After["统一后：两份事实"]
D["session_resources\nSession + path + reference"]
E["files\nidentity + metadata + object"]
F["filestore_filesystems\nfilesystem -> Session"]
F --> D
D --> E
end
Before -->|"00036 一次切换"| After
最终裁决
数据模型
flowchart LR
WS["Workspace"] --> SES["Session"]
SES --> FS["Filestore filesystem"]
SES --> RES["Session Resource"]
WS --> FILE["File"]
RES -. "file_uuid" .-> FILE
RES -. "skill_version_uuid" .-> SKILL["Skill Version"]
必须保持的不变量
三个 adapter，共用一份事实flowchart TB
FACT["Resource + File"]
FACT --> S["Sessions adapter\npayload 非空"]
FACT --> F["Files adapter\n/uploads + /outputs"]
FACT --> FS["Filestore adapter\n完整 namespace"]
写入与生命周期flowchart TD
I["Attach Input"] --> IR["只写 Resource\n引用 Source File"]
IR --> IN["不创建 File\n不计费\n不清理对象"]
O["Put / Copy Output"] --> OBJ["唯一 S3 key + orphan guard"]
OBJ --> TX["同一事务：Resource + Owned File\nquota delta + cleanup job"]
M["Move"] --> MP["只改 Resource path\nFile ID / object key 不变"]
D["Remove / TTL"] --> RT["同一事务退休 Resource + File\n释放容量 + cleanup job"]
锁序
配额与对象所有权
Migration 00036flowchart LR
A["扩展列/约束"] --> B{"Preflight"}
B -->|"失败"| R["整体回滚\n保留旧表"]
B -->|"通过"| C["合并 Input"]
C --> D["Materialize 非 Input File"]
D --> E["创建内部 Resource"]
E --> F["删除 Input projection"]
F --> G["建索引/验证 CHECK"]
G --> H["删除 filestore_entries"]
Preflight
Identity 矩阵
Reviewer checklist 与证据
非目标与已知代价
|
Rename the SessionNamespaceNode type and all related identifiers across the codebase to SessionResourceFile, aligning the naming with the unified session resource and file model introduced in this PR. - Rename type, methods, constants and helper functions in internal/db - Rename five session_namespace*.go source files to session_resource* - Update SQL derived-table alias from namespace_nodes to session_resource_files - Update error messages and comments to drop the obsolete node wording - Sync design docs (filestore.md, session-resource-file-unification.md)
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/db/filestore_cleanup.go (1)
204-224: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRetire expired source references rather than skipping them.
continueleaves an invalid historical source reference active. Because the query is bounded and ordered, enough such rows can be selected on every sweep and permanently block later owned files from expiring. Resolve its scope and soft-delete only the Resource; do not enqueue object cleanup or decrement quota.Proposed fix
for _, entry := range entries { + scope, found := cleanupScopeByFilesystemUUID[entry.FilesystemUUID] + if !found { + return nil, nil, ErrNotFound + } if entry.ReferencesSourceFile() { + if err := retireSessionResourceFileTx(ctx, tx, scope.WorkspaceID, entry.ID, now); err != nil { + return nil, nil, err + } continue } - scope, found := cleanupScopeByFilesystemUUID[entry.FilesystemUUID] - if !found { - return nil, nil, ErrNotFound - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/db/filestore_cleanup.go` around lines 204 - 224, Update the entry handling in the cleanup loop around ReferencesSourceFile so source-file references still resolve their cleanup scope and are passed through retirement via retireSessionResourceFileTx. For these entries, skip sessionResourceFileCleanupAnomaly and enqueueSessionResourceFileCleanupJobTx, ensuring only the Resource is soft-deleted without object cleanup or quota decrement; continue normal cleanup processing for owned entries.
🧹 Nitpick comments (3)
tests/sessions_file_resources_api_test.go (3)
774-842: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCheck for orphan
filesrows after a failed catalog write.The current assertions prove that no active session resource or scoped catalog entry remains, but they do not detect an unscoped
filesrow left behind. Because the injected constraint allowsscope_id IS NULL, a partial write could insert an orphan File and still pass these checks. Assert that the failed output’s File/S3 identity is absent from both scoped and unscoped catalog views.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/sessions_file_resources_api_test.go` around lines 774 - 842, Extend TestSessionOutputCatalogWriteIsAtomic to capture the failed output’s File/S3 identity and assert it is absent from both scoped and unscoped catalog views after PutFilestoreFile fails. Reuse the existing file-listing or catalog lookup helpers, ensuring the assertions detect orphan files with scope_id IS NULL as well as scoped entries, while preserving the current resource and storage-byte checks.
1157-1194: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winVerify expiry quota and cleanup side effects.
ExpireSessionResourceFilesreturns cleanup jobs and releases workspace bytes, but this test discards those results and only checks catalog visibility. A regression that hides expired rows without releasing quota or enqueueing cleanup would pass. Assert the expected ledger reduction for both expired owned files and validate the returned cleanup work.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/sessions_file_resources_api_test.go` around lines 1157 - 1194, Extend the expiry scenario around ExpireSessionResourceFiles to retain its returned cleanup jobs and released-byte result instead of discarding them. Assert that the workspace byte ledger decreases by the expired owned file’s size, and verify the returned cleanup work contains the expected expired resource. Keep the existing catalog-visibility assertions.
1205-1267: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winVerify namespace and object state after every rejected mutation.
The matrix checks only
ErrPreconditionFailed, storage bytes, and source File metadata. A partial move, remove, or overwrite could still alter the session resource path or backing object while leaving those checks unchanged. After each rejection, assert that the original resource still references the same source and that destination/parent paths and object contents remain unchanged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/sessions_file_resources_api_test.go` around lines 1205 - 1267, Extend the per-mutation assertions in the test loop around mutate to verify namespace and object state after every ErrPreconditionFailed result: confirm the session resource still references the original file and /locked/input.txt path, destination and parent paths remain unchanged, and the backing file object/content is unchanged. Keep the existing storage-byte and source-metadata checks, and use the database/resource helpers already used by this test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/be/session-resource-file-unification.md`:
- Around line 18-20: Update the review-map references in the design document:
replace the removed session_namespace_* file links with their current
session-resource equivalents, change the line 20 test-evidence link to the
existing heading anchor, and rename the test file reference at the later
review-test section from session_namespace_review_test.go to
session_resource_file_review_test.go. Preserve the surrounding review-map
structure and wording.
---
Outside diff comments:
In `@internal/db/filestore_cleanup.go`:
- Around line 204-224: Update the entry handling in the cleanup loop around
ReferencesSourceFile so source-file references still resolve their cleanup scope
and are passed through retirement via retireSessionResourceFileTx. For these
entries, skip sessionResourceFileCleanupAnomaly and
enqueueSessionResourceFileCleanupJobTx, ensuring only the Resource is
soft-deleted without object cleanup or quota decrement; continue normal cleanup
processing for owned entries.
---
Nitpick comments:
In `@tests/sessions_file_resources_api_test.go`:
- Around line 774-842: Extend TestSessionOutputCatalogWriteIsAtomic to capture
the failed output’s File/S3 identity and assert it is absent from both scoped
and unscoped catalog views after PutFilestoreFile fails. Reuse the existing
file-listing or catalog lookup helpers, ensuring the assertions detect orphan
files with scope_id IS NULL as well as scoped entries, while preserving the
current resource and storage-byte checks.
- Around line 1157-1194: Extend the expiry scenario around
ExpireSessionResourceFiles to retain its returned cleanup jobs and released-byte
result instead of discarding them. Assert that the workspace byte ledger
decreases by the expired owned file’s size, and verify the returned cleanup work
contains the expected expired resource. Keep the existing catalog-visibility
assertions.
- Around line 1205-1267: Extend the per-mutation assertions in the test loop
around mutate to verify namespace and object state after every
ErrPreconditionFailed result: confirm the session resource still references the
original file and /locked/input.txt path, destination and parent paths remain
unchanged, and the backing file object/content is unchanged. Keep the existing
storage-byte and source-metadata checks, and use the database/resource helpers
already used by this test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3df1db5f-e789-4085-b1d8-f80fbcf9f219
📒 Files selected for processing (29)
docs/design/be/filestore.mddocs/design/be/session-resource-file-unification.mdinternal/db/filestore.gointernal/db/filestore_cleanup.gointernal/db/filestore_cleanup_sqlx_test.gointernal/db/filestore_scan.gointernal/db/filestore_sqlx.gointernal/db/filestore_test.gointernal/db/session_resource_file_helpers.gointernal/db/session_resource_file_methods.gointernal/db/session_resource_file_mutations.gointernal/db/session_resource_file_review_test.gointernal/db/session_resource_files.gointernal/db/session_skill_archive_resources.gointernal/db/session_skill_archive_resources_test.gointernal/db/sessions.gointernal/filestore/cleanup.gointernal/filestore/cleanup_test.gointernal/filestore/persistent_backend.gointernal/filestore/service.gointernal/filestore/service_test.gointernal/filestore/service_test_support_test.gointernal/filestore/skill_archives.gointernal/filestore/skill_archives_test.gotests/environments_runner_cloud_test.gotests/filestore_db_test.gotests/filestore_provision_roots_test.gotests/sessions_file_resources_api_test.gotests/workspace_storage_usage_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/db/sessions.go
- docs/design/be/filestore.md
…n layer 将运行时代码中 4 处在 SQL 层用 gen_random_uuid()+concat 生成 uuid 与 external_id 的逻辑，改为在 Go 代码层用 ids.New()+uuid.NewString() 统一生成， 使 external_id 采用与项目其他位置一致的 24 位 Base62 格式，而非去横线 UUID。 涉及 directory、owned file（File + Resource 两层）与 skill archive 创建路径。 migration 00036 属一次性数据迁移，保留 SQL 层生成。
…-file-unification' into codex/issue-184-session-resource-file-unification
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/db/session_resource_file_helpers.go (1)
405-422: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: collapse the two identity helpers into one prefixed generator.
Both functions are structurally identical; a single
newIdentity(prefix string) (string, string, error)keeps the duplicate-code budget headroom and the call sites just as readable.♻️ Suggested consolidation
-func newSessionResourceIdentity() (resourceUUID, resourceExternalID string, err error) { - resourceExternalID, err = ids.New("sesrsc_") - if err != nil { - return "", "", err - } - return uuid.NewString(), resourceExternalID, nil -} - -// newFileIdentity 在应用层生成真实 File 的 uuid 与 file_ external ID。 -func newFileIdentity() (fileUUID, fileExternalID string, err error) { - fileExternalID, err = ids.New("file_") - if err != nil { - return "", "", err - } - return uuid.NewString(), fileExternalID, nil -} +// newIdentity 在应用层生成 uuid 与带前缀的 external ID。 +func newIdentity(prefix string) (identityUUID, externalID string, err error) { + externalID, err = ids.New(prefix) + if err != nil { + return "", "", err + } + return uuid.NewString(), externalID, nil +}As per coding guidelines: “修改 Go 或 TypeScript/TSX 生产代码后运行重复代码检查；遵守 Go 3.75% … 的生产代码预算”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/db/session_resource_file_helpers.go` around lines 405 - 422, Optionally consolidate newSessionResourceIdentity and newFileIdentity into a shared newIdentity(prefix string) helper that generates the prefixed external ID and UUID, preserving the existing error propagation and return ordering. Update both call sites to pass their respective prefixes while keeping their behavior unchanged.Source: Coding guidelines
internal/db/filestore_cleanup.go (1)
242-244: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPrefer
entry.OwnedBytes()overfilestoreInt64(entry.SizeBytes)for released-byte accounting.
OwnedBytes()already encodes the ownership rule (0 for non-file kinds and source-file references), which is what the mutation paths ininternal/db/session_resource_file_mutations.gouse. Using rawSizeByteshere releases bytes for any expired/cleaned resource kind that carries a size but is not charged (e.g. archive snapshots), which can drive usage negative until reconciliation.♻️ Suggested change
releasedBytes, err := addWorkspaceStorageDelta( - releasedBytesByWorkspace[scope.WorkspaceID], filestoreInt64(entry.SizeBytes), + releasedBytesByWorkspace[scope.WorkspaceID], entry.OwnedBytes(), )releasedBytes, err = addWorkspaceStorageDelta( releasedBytes, - filestoreInt64(entry.SizeBytes), + entry.OwnedBytes(), )Also applies to: 370-373
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/db/filestore_cleanup.go` around lines 242 - 244, The released-byte accounting in the cleanup flow should use the ownership-aware value from entry.OwnedBytes() instead of filestoreInt64(entry.SizeBytes). Update both occurrences around addWorkspaceStorageDelta so non-file kinds and source-file references contribute zero, matching the mutation paths and preventing uncharged resources from reducing workspace usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/db/migrations/00037_snapshot_session_skills.sql`:
- Around line 57-62: Update the `insert into files` query in migration
`00037_snapshot_session_skills.sql` to select only `skill_archive` resources
whose `file_uuid` is non-null and have no corresponding row in `files`. Preserve
the existing filters and ensure reruns or pre-existing snapshot rows cannot
reinsert an already-existing file UUID.
In `@tests/workspace_storage_usage_test.go`:
- Line 279: Move the “failure expired file path remains reserved until ttl
cleanup” subtest before the “success files and filestore maintain one
transactional total”, “success overwrite move ...”, and “success expired bytes
are released by the ttl transaction” subtests, keeping the failure scenarios
grouped before success scenarios without changing test behavior.
---
Nitpick comments:
In `@internal/db/filestore_cleanup.go`:
- Around line 242-244: The released-byte accounting in the cleanup flow should
use the ownership-aware value from entry.OwnedBytes() instead of
filestoreInt64(entry.SizeBytes). Update both occurrences around
addWorkspaceStorageDelta so non-file kinds and source-file references contribute
zero, matching the mutation paths and preventing uncharged resources from
reducing workspace usage.
In `@internal/db/session_resource_file_helpers.go`:
- Around line 405-422: Optionally consolidate newSessionResourceIdentity and
newFileIdentity into a shared newIdentity(prefix string) helper that generates
the prefixed external ID and UUID, preserving the existing error propagation and
return ordering. Update both call sites to pass their respective prefixes while
keeping their behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a905a68-3501-4a94-a607-f8b5093d4020
📒 Files selected for processing (22)
docs/design/be/filestore.mddocs/design/be/managed-agent-skills-runtime.mddocs/design/be/session-resource-file-unification.mdinternal/db/AGENTS.mdinternal/db/filestore.gointernal/db/filestore_cleanup.gointernal/db/filestore_scan.gointernal/db/filestore_sqlx.gointernal/db/migrations/00037_snapshot_session_skills.sqlinternal/db/migrations_postgres_test.gointernal/db/session_resource_file_helpers.gointernal/db/session_resource_file_mutations.gointernal/db/session_skill_archive_resources.gointernal/db/session_skill_archive_resources_test.gointernal/db/sessions.gointernal/db/sessions_sqlx.gointernal/environments/runner.gointernal/filestore/skill_archives.gointernal/filestore/skill_archives_test.gotests/filestore_db_test.gotests/sessions_file_resources_api_test.gotests/workspace_storage_usage_test.go
💤 Files with no reviewable changes (4)
- internal/filestore/skill_archives_test.go
- internal/db/sessions.go
- internal/filestore/skill_archives.go
- tests/sessions_file_resources_api_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/design/be/managed-agent-skills-runtime.md
- internal/environments/runner.go
- docs/design/be/filestore.md
将 origin/main 合并到当前分支，冲突按语义处理： 核心矛盾：HEAD 的 session_resource 重构（filestore_entries → session_resources） 与 main 的 bigint→UUID 标识符迁移在同一代码区域正交碰撞。 合并策略：保留 HEAD 的 session_resource 业务模型，完整 UUID 化以适配 main 的 schema。 主要变更： - Migration：HEAD 00036-00038 保留在前，main 00036-00046 顺延为 00039-00049 - SQL 语义修复：移除所有 select uuid from workspaces where id = :workspace_uuid 旧模式，改为 CAST(:workspace_uuid AS uuid) 直接比较 - 方法签名 UUID 化：filestore cleanup job ID 从 int64 改为 string (UUID) - filestore filesystem 分页游标从 (path, id) 改为 (path, uuid) - 测试全面 UUID 化：所有 int64 ID 引用更新为 UUID 字符串
…on-resource-file-unification # Conflicts: # internal/db/filestore_archive_entries.go # internal/db/filestore_cleanup.go # internal/db/filestore_entry_helpers.go # internal/db/filestore_scan.go # internal/db/filestore_sqlx.go # internal/db/filestore_test.go # internal/db/session_file_mounts_sqlx.go # internal/db/session_file_mounts_sqlx_test.go # internal/db/session_resource_file_mutations.go # internal/db/session_resource_files.go # internal/db/sessions.go # internal/db/workspace_storage.go
恢复 Session File Catalog 的可见性与删除边界。 将清理任务、Skill Archive 和测试夹具适配 UUID schema，避免继续引用已移除的 identity 列或 filestore_entries。 Refs: #184 Co-authored-by: Codex <noreply@openai.com>
|
Run failed. View the logs →
|
Co-authored-by: Codex <noreply@openai.com>
![duckpr[bot]](https://avatars.githubusercontent.com/in/4166808?s=60&v=4){kind=small}
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
✅ 未发现新问题。
Reviewed changes — 本次复评审仅覆盖增量提交 f24fcb6（"fix(db): 对齐 Session 与清理任务 UUID 边界"），该提交将清理任务和 Session 分页查询的 UUID 绑定对齐到原生 uuid 类型。
- 对齐 Session 分页游标的 UUID 绑定 —
ListSessionsPage、ListSessionThreadsPage、ListSessionEventsPage将params.Cursor.UUID包入dbUUID()，使游标参数以原生uuid类型绑定到s.uuid/uuid列，而非文本。 - 清理任务查询改用原生 JSONB UUID 比较 —
leasedFilestoreCleanupJobQuery、leaseFilestoreCleanupJobs等 join 条件从cast(fs.uuid as text) = j.payload->>'filesystem_uuid'切换为j.payload->'filesystem_uuid' = to_jsonb(fs.uuid)，enqueueFilestoreFilesystemCleanupJobQuery与insertFilestoreObjectCleanupJobSQLX改用fs.uuid = :filesystem_uuidbind 参数 join。 - Payload 构造迁移到应用层 —
enqueueFilestoreFilesystemCleanupJobTx与insertFilestoreObjectCleanupJobSQLX将 payload 从 SQLjsonb_build_object改为 Gojson.Marshal+cast(:payload as jsonb)bind，列投影不再cast(... as text)。 - 引入 typed cleanup job row structs — 新增
filestoreObjectCleanupJobRow/filestoreFilesystemCleanupJobRow直接扫描uuid.UUID字段，Lease 入口从直接扫描 public job struct 改为先扫描 row 再.job()转换。
anthropic/glm-5.2 ｜ 𝕏
…on-resource-file-unification
…on-resource-file-unification
…on-resource-file-unification
…-file-unification' into codex/issue-184-session-resource-file-unification # Conflicts: # internal/db/filestore_cleanup.go # internal/db/filestore_sqlx.go
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
✅ 未发现新问题。
Reviewed changes — 本次复评审覆盖自上一次 Pullfrog 评审（f24fcb6）以来的增量提交，核心是 commit b5f0b91 将整个 DB 层从手写 sqlx 查询迁移到 Yourbatis mapper（XML SQL + sqlmapgen 生成代码），并合并了 origin/main 的 Yourbatis 基础设施与 admin API 拆分。
- 将 DB 层迁移到 Yourbatis mapper —
session_resource_file_mutations.go、session_resource_file_helpers.go、filestore_cleanup.go、files.go、workspace_storage.go、filestore_filesystems.go等全部从BeginTxx/namedExecContext切换为yourbatis.DB.Transaction+ typed mapper interface（FileMapper、SessionResourceMapper、FilestoreCleanupMapper等），SQL 移入对应*_mapper.xml，事务边界和配额扣减逻辑保持不变。 - 迁移文件重编号 — 因
origin/main合入新迁移，统一迁移从00036顺延为00047_unify_session_resources_and_files.sql，skill 快照迁移从00037顺延为00048_snapshot_session_skills.sql；SQL 内容不变。 - 停止跟踪生成代码 —
*.gen.go加入.gitignore，新增go-generatepre-commit hook，CI lint/complexity/dead-code 工作流在检查前增加go generate ./...步骤。 - 合并 admin API 拆分 — 从
internal/db/admin.go（1162 行）拆分为admin_api_keys.go、admin_users.go、admin_workspaces.go等聚焦文件，引入对应的 Yourbatis mapper 与 PostgreSQL mapper 测试。
本次增量评审通过正确性与数据完整性两个 lens 并行检查，重点验证了 Yourbatis 迁移后的事务原子性、错误传播、sql.ErrNoRows 到领域错误的映射、多租户隔离以及存储账本一致性。所有业务逻辑与 f24fcb6 版本保持 1:1 对应，未发现迁移引入的新问题。先前 CodeRabbit 评审已标记的迁移账本对齐和清理字节核算问题仍为 open 状态，此处不再重复。
｜ View workflow run ｜ Using anthropic/glm-5.2 ｜ 𝕏
anthropic/glm-5.2 ｜ 𝕏
Co-authored-by: Codex <noreply@openai.com>
This was referenced Aug 5, 2026
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
✅ 未发现新问题。
Reviewed changes — 本次复评审仅覆盖增量提交 d895da6（"feat(sessions): 支持相对挂载路径输入"），该提交将 Session 文件资源的挂载路径输入从绝对路径（/uploads/reports/input.csv）改为相对路径（reports/input.csv），提升输入体验。
- 挂载路径校验改为相对路径语义 —
isValidSessionFileMountPath不再要求以/uploads/开头，改为拒绝空串、前导/、尾随/、//以及./..路径段；sessionFileRuntimePath与sessionFileAPIMountPath的前缀拼接逻辑同步调整。 - API 契约保持不变 —
sessionFileAPIMountPath仍为相对路径补/后发送给后端（/reports/input.csv），后端接收的mount_path格式未变。 - 前端组件、i18n 与设计文档同步更新 — placeholder、帮助文案（
mountHelp）、中英文 messages 与session-file-resources-form.md均改为相对路径表述；单元测试和 suite 测试用例已同步覆盖新的合法/非法输入。
改动范围小且自洽，校验逻辑、运行时路径预览与 API 边界三者一致。后端仍是路径规则、Workspace 隔离与冲突检测的权威来源，无安全性问题。
anthropic/glm-5.2 ｜ 𝕏
删除 ExpireSessionResourceFiles 数据层路径、filestore cleanup worker 的 TTL 扫描循环、Yourbatis mapper 中到期查询与相关测试，并同步更新 filestore 设计文档：expires_at 只影响 Resource/Catalog 查询可见性，不再触发自动软删除、配额释放或对象清理任务。
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
✅ 未发现新问题。
Reviewed changes — 本次复评审覆盖自上一次 Pullfrog 评审（b5f0b91）以来的两个增量提交：d895da6（前端相对挂载路径输入）和 54472b1（移除 session resource 文件的 TTL 到期清理逻辑）。
- 移除 TTL 到期自动清理 — 删除了
ExpireSessionResourceFiles数据层路径、cleanup worker 的 TTL sweep 循环与定时器、Yourbatis mapper 中的ListExpiredScopes/ListExpiredFiles查询，以及对应的 mapper 测试和 worker 测试。设计文档同步更新：expires_at现在只影响 Resource/Catalog 查询的可见性，不再触发自动软删除、配额释放或对象清理。 - 保留目录覆盖时的过期子树回收 —
retireExpiredFilestoreSubtreeTx及其ListExpiredSubtreeFiles查询被正确保留，确保目录 move/overwrite 时仍能回收已过期的目标文件。 - 清理无残留引用 —
filestoreTTLSweepBatchSize、filestoreTTLSweepInterval、runTTLSweepAndLog等 TTL 相关常量与方法已全部移除，filestoreCleanupDatabaseinterface 已同步删除ExpireSessionResourceFiles方法；全仓库无残留调用。FilestoreCleanupAnomaly和logCleanupAnomalies仍被 filesystem cleanup 路径使用，不属于死代码。 - 前端改用相对挂载路径 —
file-resource-path.ts将校验逻辑从要求/uploads/前缀改为要求非空相对路径（不以/开头或结尾），sessionFileAPIMountPath在提交时补/前缀以保持 API 合同不变。i18n 文案与测试同步更新。
anthropic/glm-5.2 ｜ 𝕏
…on-resource-file-unification # Conflicts: # internal/environments/runner.go
arthur-zhang
deleted the
codex/issue-184-session-resource-file-unification
branch
August 5, 2026 07:08
This was referenced Aug 5, 2026
Merged
7 tasks
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
2 participants
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.
概要
filestore_entries与fse_identity，以session_resources作为 Session namespace 唯一事实源filesfile_idsesrsc_区分，Session Catalog 按真实 File 去重背景
PR #182 引入 Session File projection 后，Resource、Filestore Entry 和 File projection 形成多重事实源，导致 create、overwrite、move、delete、TTL 与 cleanup 都需要同步维护。
本 PR 按 #184 的最终设计进行一次性切换，不引入双写、双读、兼容 view、trigger 或 feature flag。Input 不再生成 Session File Alias；一次 Attach 的 identity 由
sesrsc_表达，files.delete(file_...)始终面向真实 File，活动 Resource 引用存在时返回冲突。这与 Anthropic 当前 attach 返回新 session-scoped
file_id的行为存在已确认的兼容性偏差，设计取舍和迁移行为已记录在文档及 #184 评论中。Main 合并
已合入
origin/main的最新代码，并保留双方功能：统一迁移因此顺延为
00036_unify_session_resources_and_files.sql。验证
go test ./... -count=1：所有 package 通过；tests package 中TestEnvironmentRunnerLaunchesManagedAgentCloudSession在全套并行负载下出现队列时序超时仓库级
golangci-lint ./...仍会扫描web/node_modules/flatted并命中两条既有 govet 告警；本次改动包 lint 与提交 hook 均通过。Closes #184
Summary by CodeRabbit
/skillscontents.