feat(auth): add backup browser#85
feat(auth): add backup browser#85ndycode wants to merge 12 commits intogit-plan/11-sync-history-logfrom
Conversation
📝 WalkthroughWalkthroughintroduces an interactive backup browser replacing direct-restore flows, adds rotating backup support, tightens backup name/path validation, extends backup metadata, and routes oauth/login recovery through the new browser manager. see Changes
Sequence Diagram(s)sequenceDiagram
participant user as User
participant manager as CodexManager
participant storage as Storage
participant logger as Logger
user->>manager: open backup browser / initiate recovery
manager->>storage: listNamedBackups()
storage-->>manager: named backups
manager->>storage: listRotatingBackups()
storage-->>manager: rotating backups
manager->>manager: assess backups (assessment / errors)
manager->>logger: log assessment & format strings
manager-->>user: display browser list
user->>manager: inspect backup
manager->>manager: buildBackupStatusSummary()
manager-->>user: show backup details
user->>manager: confirm restore
manager->>storage: restoreNamedBackup(name)
storage-->>manager: restore result / error
manager-->>user: show result or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes review notes
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/codex-manager.ts">
<violation number="1" location="lib/codex-manager.ts:4254">
P2: The restore action menu clears the just-rendered backup details, so valid named backups lose their inspect view.</violation>
</file>
<file name="test/codex-manager-cli.test.ts">
<violation number="1" location="test/codex-manager-cli.test.ts:1430">
P2: This test no longer verifies that selecting "restore" actually calls `restoreNamedBackup`, so it can pass while the restore path is broken.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/storage.ts`:
- Around line 464-482: normalizeBackupLookupName currently routes lookup through
normalizeBackupName which collapses on-disk basenames (e.g., "my backup.json" →
"my-backup.json"); change normalizeBackupLookupName to only perform
traversal/path-drive checks (trim, strip .json/.bak, reject separators/../drive
prefixes) and return the exact trimmed basename (preserving spaces/case) without
calling normalizeBackupName; ensure backup creation paths still call
normalizeBackupName when creating filenames, and update restore/lookup code in
codex-manager to use the lookup basename as-is rather than normalizing again;
add a vitest in test/storage.test.ts that writes a file with spaces (e.g., "my
backup.json") and asserts lookup/restore uses the exact on-disk basename, and
verify new queue/restore logic properly retries or handles EBUSY/429 per
concurrency/auth rotation guidelines.
In `@test/codex-manager-cli.test.ts`:
- Line 1448: This test sets an unusually long Jest timeout via the "}, 20_000);"
tail; locate the containing test/it block (the test function that ends with "},
20_000);") and either reduce the timeout to a smaller sensible value (e.g.,
5_000 or remove the explicit timeout to use Jest's default) or, if the scenario
legitimately requires lengthy execution, split the heavy test into smaller
focused tests so each can complete quickly; update the numeric literal "20_000"
accordingly or refactor the test into multiple test/it blocks (keeping the same
assertions split across them).
- Around line 1519-1540: This test uses an ad-hoc try/finally to restore the spy
(logSpy) which is inconsistent with other tests; make cleanup consistent by
removing the try/finally here and instead restore spies via a shared afterEach
(e.g., add an afterEach that calls logSpy.mockRestore() or
jest.restoreAllMocks()), or conversely update the other tests to use the same
try/finally pattern—ensure you target the logSpy created for
runCodexMultiAuthCli and any other spies used in the file so all tests
consistently restore mocks.
- Line 1752: The test uses new Date(0).toLocaleString() (assigned to
epochDisplay) which is locale-dependent and causes flakiness; replace it with a
deterministic representation (e.g. new Date(0).toISOString()) or mock
Date.prototype.toLocaleString via vitest's vi.spyOn before the assertion and
restore the spy in a finally block so the global is always restored; update the
code that references epochDisplay to use the new fixed string or the mocked
return value accordingly.
- Around line 1824-1833: Add an assertion after the existing check that
restoreNamedBackupMock wasn't called which verifies the rotated backup's detail
panel doesn't offer a "restore" action: inspect the second call to selectMock
(selectMock.mock.calls[1]), extract the passed options array (the detail panel
argument for the rotatingBackup inspection) and assert that none of the option
objects have value === "restore" (e.g., expect(detailOptions.some(opt =>
opt.value === "restore")).toBe(false)); keep references to selectMock,
rotatingBackup and restoreNamedBackupMock to locate the test.
In `@test/storage.test.ts`:
- Around line 34-56: The teardown code still calls fs.rm directly causing
intermittent Windows flakiness; replace those raw fs.rm calls inside the various
afterEach/afterAll cleanup hooks with the new removeWithRetry helper (call await
removeWithRetry(targetPath, { recursive: true, force: true }) or preserve the
original options used) and remove direct fs.rm usage in test/storage.test.ts;
ensure removeWithRetry is in scope for those hooks and keep the same await
semantics so cleanup remains asynchronous and deterministic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 799da05c-1bda-466d-9847-b9a465510b5e
📒 Files selected for processing (4)
lib/codex-manager.tslib/storage.tstest/codex-manager-cli.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/storage.tslib/codex-manager.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/storage.test.tstest/codex-manager-cli.test.ts
🔇 Additional comments (5)
lib/codex-manager.ts (1)
4294-4302: regression test for rotating backup selection already existsthe test at test/codex-manager-cli.test.ts:1749 ("shows epoch timestamps in backup browser details instead of unknown") already drives the backup browser through a rotating backup selection and verifies
expect(restoreNamedBackupMock).not.toHaveBeenCalled(). the guards at lib/codex-manager.ts:4294-4302 and 4445-4448 are properly covered by this vitest.> Likely an incorrect or invalid review comment.test/codex-manager-cli.test.ts (4)
43-49: logger mock factory looks good.the
createLoggerMockfactory with trackedloggerWarnMockis a clean pattern for verifying logger calls in backup browser flows.
1450-1541: restore failure test properly validates error surfacing.the test at
test/codex-manager-cli.test.ts:1450-1541correctly verifies that restore failures are logged vialoggerWarnMockand displayed to the user, addressing the pr objective about surfacing restore failures.
1616-1747: degraded entry visibility test is thorough.the test properly verifies that broken backups appear with
color: "yellow"andhint: "restore assessment unavailable", matching the pr objective to show degraded entries instead of dropping them.
1543-1614: limit-exceeded coloring test validates red styling.correctly asserts that backups with
wouldExceedLimit: truerender withcolor: "red"per the pr objectives for blocked-backup coloring.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-manager.ts`:
- Around line 4378-4385: getNamedBackupsDirectoryPath() and getStoragePath() are
being resolved on the render path and can throw (causing the browser to exit)
before degraded-entry handling; wrap calls to getNamedBackupsDirectoryPath() and
getStoragePath() in a local try/catch in the render loop that falls back to
undefined or a safe message and logs the error (without leaking secrets) so
loadBackupBrowserEntries()/the "no backups found" branch still runs; update the
loop around loadBackupBrowserEntries() and the console message construction to
use the guarded values (referencing getNamedBackupsDirectoryPath,
getStoragePath, and loadBackupBrowserEntries) and add a Vitest regression that
stubs those functions to throw (assert the UI degrades and process does not
exit), and apply the same guard pattern to the similar block around lines
4436-4444.
- Around line 4337-4363: The current implementation fans out
assessNamedBackupRestore across all namedBackups with Promise.all, causing
unbounded parallel filesystem reads; change the logic that populates
namedEntries to use a concurrency-limited worker (or simple serial for now) that
iterates namedBackups and calls assessNamedBackupRestore one at a time (or with
a small cap) and preserves the same return shape (kind, label, backup,
assessment or assessmentError) using normalizeBackupAssessmentError on failures;
add a Vitest regression that creates many mock named backups and injects
transient EBUSY/EPERM read failures to assert the capped path avoids mass
failures (and verify logging doesn’t leak sensitive tokens/emails and that
EBUSY/429 scenarios are retried/handled according to guidelines).
In `@lib/storage.ts`:
- Around line 464-466: normalizeBackupLookupName currently strips ".bak" which
makes listNamedBackups/assess/restore resolve "name.bak.json" to "name.json";
change normalizeBackupLookupName to only strip a trailing ".json"
(case-insensitive) and whitespace — do not remove ".bak" — so the lookup
preserves on-disk basenames; update call sites (listNamedBackups, the assess
function at the earlier assess/restore call sites) to rely on that normalized
name for exact filename matching; add a vitest regression that creates a file
named "name.bak.json", verifies listNamedBackups returns the exact basename
including ".bak.json", and that assess and restore operate on that exact on-disk
basename; ensure the new tests follow lib/** guidelines and mention affected
tests to catch Windows/EBUSY/429 IO concurrency patterns.
- Around line 505-507: The current traversal check rejects basenames that merely
start with ".." (e.g., "..snapshot.json"); change the validation to only fail
when the relative path actually walks up out of backupDir by splitting on
path.sep and checking the first path segment equals ".." (e.g., const
firstSegment = relativePath.split(path.sep)[0]; if (firstSegment === ".." ||
isAbsolute(relativePath)) throw new StorageError(...)); reference the variables
relativePath, backupDir, backupPath and the StorageError constructor; add a
vitest that asserts a filename like "..snapshot.json" inside the named-backups
directory is accepted and that a path with a real parent traversal
(".."/"../file") is rejected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8d8f426c-91ce-4deb-a75c-e132b7d8bff5
📒 Files selected for processing (4)
lib/codex-manager.tslib/storage.tstest/codex-manager-cli.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/storage.test.tstest/codex-manager-cli.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/storage.tslib/codex-manager.ts
🔇 Additional comments (5)
test/storage.test.ts (1)
543-617: good windows regression coverage here.
test/storage.test.ts:543-706covers the two failure modes most likely to regress in this feature: a rotating snapshot disappearing mid-read and a non-enoentfailure whose path happens to containenoent. that makes the storage-side filtering much safer to evolve.As per coding guidelines,
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.Also applies to: 619-706
test/codex-manager-cli.test.ts (3)
1454-1545: good resilience coverage for restore failures in the backup browser.
test/codex-manager-cli.test.ts:1454-1545correctly asserts restore failure surfacing, warning emission, and continued login flow without forcing oauth. this is exactly the failure mode we need pinned.As per coding guidelines, "test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior."
1851-1959: deterministic epoch rendering test is solid.
test/codex-manager-cli.test.ts:1851-1959avoids locale/timezone flake by mockingDate.prototype.toLocaleStringand restoring it infinally. good deterministic test hygiene.As per coding guidelines, "test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior."
2661-2905: nice regression matrix for windows/concurrency/429 save paths.
test/codex-manager-cli.test.ts:2661-2905covers windowsEBUSY, concurrent-save ordering, token-refresh races, and 429-like retries in one place. this materially improves safety around settings persistence under contention.As per coding guidelines, "test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior."
lib/storage.ts (1)
1403-1437: good hardening on rotating-backup enumeration failure paths.
lib/storage.ts:1403-1437now keepsgetStoragePath()and enumeration inside a single try/catch and degrades to[]with logging. this closes the uncaught windows io crash path cleanly.As per coding guidelines, "lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios."
| const namedEntries: NamedBackupBrowserEntry[] = await Promise.all( | ||
| namedBackups.map(async (backup) => { | ||
| try { | ||
| const assessment = await assessNamedBackupRestore(backup.name, { | ||
| currentStorage, | ||
| }); | ||
| return { | ||
| kind: "named", | ||
| label: assessment.backup.name, | ||
| backup: assessment.backup, | ||
| assessment, | ||
| }; | ||
| } catch (error) { | ||
| const assessmentError = normalizeBackupAssessmentError(error); | ||
| log.warn("Failed to assess named backup for backup browser", { | ||
| name: backup.name, | ||
| error: assessmentError, | ||
| }); | ||
| return { | ||
| kind: "named", | ||
| label: backup.name, | ||
| backup, | ||
| assessment: null, | ||
| assessmentError, | ||
| }; | ||
| } | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
cap named-backup assessments instead of fanning them all out with promise.all.
lib/codex-manager.ts:4337-4363 launches one assessNamedBackupRestore per backup. on a large backup directory that turns browser open into unbounded parallel filesystem reads, which is exactly where windows home-dir storage tends to start surfacing transient ebusy/eperm noise and false "assessment unavailable" rows. this path is interactive, so a small concurrency cap or a simple serial loop is the safer default. please cover the capped path with a vitest regression that loads many named backups under a transient read failure.
possible fix
- const namedEntries: NamedBackupBrowserEntry[] = await Promise.all(
- namedBackups.map(async (backup) => {
- try {
- const assessment = await assessNamedBackupRestore(backup.name, {
- currentStorage,
- });
- return {
- kind: "named",
- label: assessment.backup.name,
- backup: assessment.backup,
- assessment,
- };
- } catch (error) {
- const assessmentError = normalizeBackupAssessmentError(error);
- log.warn("Failed to assess named backup for backup browser", {
- name: backup.name,
- error: assessmentError,
- });
- return {
- kind: "named",
- label: backup.name,
- backup,
- assessment: null,
- assessmentError,
- };
- }
- }),
- );
+ const namedEntries: NamedBackupBrowserEntry[] = [];
+ for (const backup of namedBackups) {
+ try {
+ const assessment = await assessNamedBackupRestore(backup.name, {
+ currentStorage,
+ });
+ namedEntries.push({
+ kind: "named",
+ label: assessment.backup.name,
+ backup: assessment.backup,
+ assessment,
+ });
+ } catch (error) {
+ const assessmentError = normalizeBackupAssessmentError(error);
+ log.warn("Failed to assess named backup for backup browser", {
+ name: backup.name,
+ error: assessmentError,
+ });
+ namedEntries.push({
+ kind: "named",
+ label: backup.name,
+ backup,
+ assessment: null,
+ assessmentError,
+ });
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager.ts` around lines 4337 - 4363, The current implementation
fans out assessNamedBackupRestore across all namedBackups with Promise.all,
causing unbounded parallel filesystem reads; change the logic that populates
namedEntries to use a concurrency-limited worker (or simple serial for now) that
iterates namedBackups and calls assessNamedBackupRestore one at a time (or with
a small cap) and preserves the same return shape (kind, label, backup,
assessment or assessmentError) using normalizeBackupAssessmentError on failures;
add a Vitest regression that creates many mock named backups and injects
transient EBUSY/EPERM read failures to assert the capped path avoids mass
failures (and verify logging doesn’t leak sensitive tokens/emails and that
EBUSY/429 scenarios are retried/handled according to guidelines).
| const backupDir = getNamedBackupsDirectoryPath(); | ||
| const ui = getUiRuntimeOptions(); | ||
|
|
||
| while (true) { | ||
| const { namedEntries, rotatingEntries } = await loadBackupBrowserEntries(); | ||
| if (namedEntries.length === 0 && rotatingEntries.length === 0) { | ||
| console.log( | ||
| `No backups found. Named backups live in ${backupDir}. Rotating backups live next to ${getStoragePath()}.`, |
There was a problem hiding this comment.
guard backup-browser path resolution before the first render.
lib/codex-manager.ts:4378-4444 still resolves getNamedBackupsDirectoryPath() and getStoragePath() on the render path without a fallback. if either lookup throws on windows, the browser exits before the degraded-entry handling or the "no backups found" message can render. please keep those lookups behind a local try/catch and add a vitest regression for a thrown storage-path resolution here.
possible fix
async function runBackupBrowserManager(
displaySettings: DashboardDisplaySettings,
): Promise<void> {
- const backupDir = getNamedBackupsDirectoryPath();
const ui = getUiRuntimeOptions();
+ let backupDir = "unknown";
+ let rotatingDir = "unknown";
+
+ try {
+ backupDir = getNamedBackupsDirectoryPath();
+ rotatingDir = dirname(getStoragePath());
+ } catch (error) {
+ log.warn("failed to resolve backup browser paths", {
+ error: normalizeBackupAssessmentError(error),
+ });
+ }
while (true) {
const { namedEntries, rotatingEntries } = await loadBackupBrowserEntries();
if (namedEntries.length === 0 && rotatingEntries.length === 0) {
console.log(
- `No backups found. Named backups live in ${backupDir}. Rotating backups live next to ${getStoragePath()}.`,
+ `No backups found. Named backups live in ${backupDir}. Rotating backups live in ${rotatingDir}.`,
);
return;
}
@@
const selection = await select(items, {
message: "Backup Browser",
- subtitle: `Named: ${backupDir} | Rotating: ${dirname(getStoragePath())}`,
+ subtitle: `Named: ${backupDir} | Rotating: ${rotatingDir}`,
help: "Enter Inspect | Q Back",Also applies to: 4436-4444
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager.ts` around lines 4378 - 4385,
getNamedBackupsDirectoryPath() and getStoragePath() are being resolved on the
render path and can throw (causing the browser to exit) before degraded-entry
handling; wrap calls to getNamedBackupsDirectoryPath() and getStoragePath() in a
local try/catch in the render loop that falls back to undefined or a safe
message and logs the error (without leaking secrets) so
loadBackupBrowserEntries()/the "no backups found" branch still runs; update the
loop around loadBackupBrowserEntries() and the console message construction to
use the guarded values (referencing getNamedBackupsDirectoryPath,
getStoragePath, and loadBackupBrowserEntries) and add a Vitest regression that
stubs those functions to throw (assert the UI degrades and process does not
exit), and apply the same guard pattern to the similar block around lines
4436-4444.
| function normalizeBackupLookupName(rawName: string): string { | ||
| const trimmed = rawName.trim().replace(/\.(json|bak)$/i, ""); | ||
| if (!trimmed) { |
There was a problem hiding this comment.
stop stripping .bak from named-backup lookup names.
lib/storage.ts:465 removes .bak, but listNamedBackups can legitimately emit names ending in .bak from files like name.bak.json (lib/storage.ts:1384). then assess/restore (lib/storage.ts:1492, lib/storage.ts:1546) target name.json instead of name.bak.json, which can restore the wrong file or fail unexpectedly.
proposed fix
function normalizeBackupLookupName(rawName: string): string {
- const trimmed = rawName.trim().replace(/\.(json|bak)$/i, "");
+ const trimmed = rawName.trim().replace(/\.json$/i, "");
if (!trimmed) {
throw new StorageError(
`Invalid backup name: ${rawName}`,please also add a vitest regression that writes *.bak.json, lists it, and proves assess/restore resolve the exact on-disk basename.
As per coding guidelines, "lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios."
Also applies to: 1384-1384, 1492-1493, 1546-1548
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/storage.ts` around lines 464 - 466, normalizeBackupLookupName currently
strips ".bak" which makes listNamedBackups/assess/restore resolve
"name.bak.json" to "name.json"; change normalizeBackupLookupName to only strip a
trailing ".json" (case-insensitive) and whitespace — do not remove ".bak" — so
the lookup preserves on-disk basenames; update call sites (listNamedBackups, the
assess function at the earlier assess/restore call sites) to rely on that
normalized name for exact filename matching; add a vitest regression that
creates a file named "name.bak.json", verifies listNamedBackups returns the
exact basename including ".bak.json", and that assess and restore operate on
that exact on-disk basename; ensure the new tests follow lib/** guidelines and
mention affected tests to catch Windows/EBUSY/429 IO concurrency patterns.
| const relativePath = relative(backupDir, backupPath); | ||
| if (relativePath.startsWith("..") || isAbsolute(relativePath)) { | ||
| throw new StorageError( |
There was a problem hiding this comment.
tighten traversal detection to avoid false positives on valid ..* basenames.
lib/storage.ts:506 uses relativePath.startsWith(".."), which also rejects safe files like ..snapshot.json inside the backup dir. this is an edge-case correctness bug in path validation.
proposed fix
- if (relativePath.startsWith("..") || isAbsolute(relativePath)) {
+ if (/^\.\.(?:[\\/]|$)/.test(relativePath) || isAbsolute(relativePath)) {
throw new StorageError(
`Invalid backup name: ${name}`,add a vitest for a basename that starts with .. and stays inside the named-backups directory.
As per coding guidelines, "lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/storage.ts` around lines 505 - 507, The current traversal check rejects
basenames that merely start with ".." (e.g., "..snapshot.json"); change the
validation to only fail when the relative path actually walks up out of
backupDir by splitting on path.sep and checking the first path segment equals
".." (e.g., const firstSegment = relativePath.split(path.sep)[0]; if
(firstSegment === ".." || isAbsolute(relativePath)) throw new
StorageError(...)); reference the variables relativePath, backupDir, backupPath
and the StorageError constructor; add a vitest that asserts a filename like
"..snapshot.json" inside the named-backups directory is accepted and that a path
with a real parent traversal (".."/"../file") is rejected.
1 participant
Summary
Risk
Rollback
Docs
Validation