Repository navigation
refactor(storage): split cleanup.ts into cohesive leaf modules - #4491
Conversation
Pure-move split of src/storage/cleanup.ts (3141 lines) into src/storage/cleanup/ leaves: types, paths, db, satellite, pending, reconcile, staging, preview, execute, restore. The original file remains a facade re-exporting every public name. No behavior change; no consumer edits.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
github-actions
Bot
added
the
intake: hygiene-blocked
|
github-actions
Bot
added
the
chore
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (12)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe PR adds an archived-session cleanup subsystem. It previews and validates candidates, stages files, reconciles SQLite rows with backups, supports quarantine or permanent deletion, and restores staged files through resumable trash entries. ChangesArchived cleanup and restore
Priority: ➖ Normal Estimated code review effort: 5 (Critical) | ~90 minutes Change: Refactor Sequence Diagram(s)sequenceDiagram
participant CleanupClient
participant executeArchivedCleanup
participant Preview
participant Reconcile
participant TrashRestore
CleanupClient->>executeArchivedCleanup: request cleanup with preview digest
executeArchivedCleanup->>Preview: select and validate candidates
executeArchivedCleanup->>Reconcile: stage files and reconcile database rows
Reconcile-->>executeArchivedCleanup: cleanup result
executeArchivedCleanup-->>CleanupClient: quarantine or permanent result
CleanupClient->>TrashRestore: restore trash entry
TrashRestore->>Reconcile: restore thread and satellite rows
TrashRestore-->>CleanupClient: restore result
Merge Risk: ⚪ Minimal · up to No concrete behavior regression or production exposure is established, so the refactor is ready for normal CI validation. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 48.48% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 99 functions across 10 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
✅ READY
Hygiene✅ Deterministic PR hygiene checks passed. |
|
This PR is a pure-move refactor with zero behavior change: the original file stays as a facade re-exporting every name it exported before, so no consumer and no test is edited. The hygiene gate counts any non-comment change under Adding a new focused test here would be the wrong artifact. The oracle for a pure move is the existing suite for the moved subsystem, unchanged, which is exactly what proves the relocation preserved behavior. Those suites run on the stack tip (#4493). Fidelity was verified statically instead, and the checks found real defects rather than rubber-stamping: every baseline top-level declaration resolves to exactly one leaf with a comment-normalized identical body, the facade's export set matches the baseline exactly, every module-level mutable binding and test hook has a single declaration site, and every relative import specifier resolves. Local product suite, typecheck, and build were NOT RUN by operator instruction. |
lidge-jun
added
the
test-exception-approved
github-actions
Bot
removed
the
intake: hygiene-blocked
리뷰 · 우선순위 64 / 80설명 중요한 점은 "동작 변경 없는 순수 이동"이라는 계약입니다. 예전에 내보내던 이름 35개를 철자까지 같게 다시보내고, 소비자나 테스트를 고치지 않습니다. 모듈 수준에서 바꾸던 유일한 값인 왜 지금 cleanup부터냐면, 계획 문서( 정적 검토에서 잡힌 실수도 고쳤습니다. 우선순위 64는 "지금 당장 사용자 버그를 고치는 건 아니지만", 유지보수와 이후 분할 프로그램의 안전한 첫 발이라서입니다. package는 여전히 2.53.0이고, types/config 분할과는 직접 충돌하지 않습니다. 다만 hygiene이
심볼 hygiene 심볼 스택 #4492 / #4493 - 바닥만 먼저 머지하고 tip CI를 볼지, tip이 초록일 때까지 셋을 묶을지. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a0709790d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * Implementation lives in the `./cleanup/` leaf modules; this file is the | ||
| * stable public surface and re-exports every original name. |
There was a problem hiding this comment.
This split changes the architecture of the owned src/storage/ area, which structure/manifest.json:92 assigns to structure/runtime.md, but the commit does not update that document. Record the new cleanup facade/leaf-module boundary in runtime.md in the same change so the source-ownership documentation remains synchronized.
AGENTS.md reference: AGENTS.md:L37-L41
Useful? React with 👍 / 👎.
1 participant
Summary
src/storage/cleanup.tswas 3141 lines carrying quarantine preview, execution, satellite-database snapshot/restore, trash staging, and restore-pending bookkeeping in one file. This splits it into ten cohesive leaves undersrc/storage/cleanup/and leaves the original path as a 63-line facade that re-exports all 35 previously exported names, spelled identically, so no consumer and no test is edited.The leaves are
types,paths,db,satellite,pending,reconcile,staging,preview,execute, andrestore, layered acyclically (types/paths→db→satellite→reconcile, with the rest on top). The only module-level mutable binding in the original, the_restorePendingSeqfilename sequencer, lives in exactly one module (pending.ts) next to its only writer.This is a pure move: zero behavior change, no renamed symbol, no changed signature. It also opens
devlog/_plan/260913_godfile_round1/, which records how the round's three targets were chosen — the ranking is by live open-PR contention rather than size, becauseresponses/core.tsandconfig.tseach have 19 open PRs against them and splitting either today would rebase all of them.First of a three-PR stack: this →
src/service.ts→src/adapters/kiro.ts.Verification
export typevsexportpreserved.src/lab/import, no relative specifier left unresolvable.randomUUIDwas used inpaths.tsandrestore.tswithout being imported, which would have thrownReferenceErroron every quarantine execute and mid-commit on every restore.Checklist
Summary by CodeRabbit