Skip to content

fix: implement actual data collection for backup security manager #333

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
daggerstuff wants to merge 5 commits into
base: staging
Choose a base branch
from
+87 −8
Open

fix: implement actual data collection for backup security manager #333

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d07a470
fix: implement actual data collection for backup security manager
google-labs-jules[bot] Mar 31, 2026
41e59c7
fix: implement actual data collection for backup security manager
google-labs-jules[bot] Apr 1, 2026
29312af
fix: implement actual data collection for backup security manager
google-labs-jules[bot] Apr 1, 2026
42a1b88
fix: implement actual data collection for backup security manager
google-labs-jules[bot] Apr 1, 2026
cac6640
fix: implement actual data collection for backup security manager
google-labs-jules[bot] Apr 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 87 additions & 8 deletions src/lib/security/backup/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -798,7 +798,7 @@ export class BackupSecurityManager {
}
} catch (error: unknown) {
logger.error(
`Error searching for backup metadata in ${location}: ${error instanceof Error ? String(error) : String(error)}`,
`Error searching for backup metadata in ${location}: ${error instanceof Error ? error.message : String(error)}`,
)
}
}
Expand All @@ -809,16 +809,95 @@ export class BackupSecurityManager {
/**
* Get data to backup based on backup type
*/
private async getLastBackupTime(requireFull = false): Promise<Date | null> {
let latestTimestamp = 0;

const storageEntries = Array.from(this.storageProviders.entries());
for (let i = 0; i < storageEntries.length; i++) {
const [, provider] = storageEntries[i];
if (!provider) continue;

try {
const files = await provider.listFiles("backups/");
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai bot Apr 1, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

P2: getLastBackupTime uses listFiles("backups/"), but storage providers treat the argument as a full glob pattern (not a recursive prefix). For providers like LocalFileSystemProvider, the pattern is anchored, so backups/ never matches nested metadata files and the last-backup scan always returns null.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/security/backup/index.ts, line 821:

<comment>`getLastBackupTime` uses `listFiles("backups/")`, but storage providers treat the argument as a full glob pattern (not a recursive prefix). For providers like `LocalFileSystemProvider`, the pattern is anchored, so `backups/` never matches nested metadata files and the last-backup scan always returns null.</comment>

<file context>
@@ -810,83 +809,95 @@ export class BackupSecurityManager {
-        const files = await provider.listFiles("backups/") // Fallback to list all and filter if pattern not supported
-        const metaFiles = files.filter(f => f.endsWith(".meta.json"))
-        for (const file of metaFiles) {
+        const files = await provider.listFiles("backups/");
+        const metaFiles = files.filter((f) => f.endsWith(".meta.json"));
+        for (const metaFile of metaFiles) {
</file context>
Suggested change
const files = await provider.listFiles("backups/");
const files = await provider.listFiles("backups/*/*/*/*.meta.json");
Fix with Cubic

const metaFiles = files.filter((f) => f.endsWith(".meta.json"));
for (const metaFile of metaFiles) {
try {
const metadataBuffer = await provider.getFile(metaFile);
const metadata = JSON.parse(new TextDecoder().decode(metadataBuffer)) as BackupMetadata;

if (metadata.status === BackupStatus.COMPLETED) {
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai bot Mar 31, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

P1: Incremental/differential baseline selection only considers COMPLETED backups, but created backups remain PENDING in normal flow, causing repeated fallback windows and potential data omission.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/security/backup/index.ts, line 832:

<comment>Incremental/differential baseline selection only considers COMPLETED backups, but created backups remain PENDING in normal flow, causing repeated fallback windows and potential data omission.</comment>

<file context>
@@ -809,16 +810,83 @@ export class BackupSecurityManager {
+            const metadataBuffer = await provider.getFile(file)
+            const metadata = JSON.parse(new TextDecoder().decode(metadataBuffer)) as BackupMetadata
+
+            if (metadata.status === BackupStatus.COMPLETED) {
+              if (requireFull && metadata.type !== BackupType.FULL) continue
+              if (!requireFull && metadata.type !== BackupType.FULL && metadata.type !== BackupType.DIFFERENTIAL && metadata.type !== BackupType.INCREMENTAL) continue
</file context>
Fix with Cubic

if (requireFull && metadata.type !== BackupType.FULL) continue;
if (!requireFull && metadata.type !== BackupType.FULL && metadata.type !== BackupType.DIFFERENTIAL && metadata.type !== BackupType.INCREMENTAL) continue;

const timestamp = new Date(metadata.timestamp).getTime();
if (timestamp > latestTimestamp) {
latestTimestamp = timestamp;
}
}
} catch {
// Ignore individual file parsing errors
Comment on lines +837 to +838
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

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

Swallowing all per-file parsing errors makes it hard to diagnose corrupted/partial metadata and can silently degrade baseline discovery (e.g., always falling back). Consider at least logging at debug/warn with the metaFile name (and the caught error message), or explicitly handling expected parse failures vs unexpected ones.

Suggested change
} catch {
// Ignore individual file parsing errors
} catch (error: unknown) {
logger.warn(
`Failed to parse backup metadata file "${metaFile}": ${
error instanceof Error ? error.message : String(error)
}`,
);

Copilot uses AI. Check for mistakes.
}
}
} catch (error: unknown) {
logger.error(`Error searching for latest backup metadata: ${error instanceof Error ? error.message : String(error)}`);
}
}

return latestTimestamp > 0 ? new Date(latestTimestamp) : null;
}

private async getDataForBackup(type: BackupType): Promise<Uint8Array> {
// Implementation would collect app data based on backup type
// For now return dummy data for demonstration
// [PIX-44] TODO: No more fucking cop-outs
const dummyData = {
message: `This is a ${type} backup created at ${new Date().toISOString()}`,
let appDataJson = '{"timestamp":"' + new Date().toISOString() + '","type":"' + type + '","data":{';
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

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

Building a large JSON payload via repeated string += ... inside nested loops can become very costly (frequent reallocations/copies) as the backup grows. Prefer accumulating into an array of string chunks and join('') once, or write into a dedicated buffer/stream abstraction; it keeps the same sequential read pattern without the quadratic-ish string concatenation overhead.

Copilot uses AI. Check for mistakes.

try {
const mongooseModule = "mongoose";
const mongoose = (await import(/* @vite-ignore */ mongooseModule)).default || await import(/* @vite-ignore */ mongooseModule);
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

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

This does two dynamic imports and can yield inconsistent types: if .default is missing, mongoose becomes the module namespace object, so mongoose.modelNames() may not exist. Import once, then resolve default vs namespace from that single result (e.g., store the import result in a variable and use mod.default ?? mod). This also avoids the extra module load.

Suggested change
const mongoose = (await import(/* @vite-ignore */ mongooseModule)).default || await import(/* @vite-ignore */ mongooseModule);
const mongooseImport = await import(/* @vite-ignore */ mongooseModule);
const mongoose = (mongooseImport as any).default ?? mongooseImport;

Copilot uses AI. Check for mistakes.
const models = mongoose.modelNames();

let isFirstModel = true;

let baselineTime: Date | null = null;
if (type === BackupType.DIFFERENTIAL || type === BackupType.INCREMENTAL) {
const requireFull = type === BackupType.DIFFERENTIAL;
const lastBackupTime = await this.getLastBackupTime(requireFull);
baselineTime = lastBackupTime || new Date(Date.now() - 24 * 60 * 60 * 1000);
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

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

The 24 * 60 * 60 * 1000 fallback is a magic number and the rationale isn’t encoded in the code. Extract this to a named constant (e.g., DEFAULT_BASELINE_LOOKBACK_MS) and add a short comment explaining why 24h is the chosen fallback.

Copilot uses AI. Check for mistakes.
}
Comment on lines +860 to +864
Copy link
Copy Markdown

@charliecreates charliecreates bot Mar 31, 2026

Choose a reason for hiding this comment

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

Differential/incremental backups fall back to baselineTime = now - 24h when no prior baseline is found. That can produce invalid backup chains (missing older documents), making restores incomplete. For DIFFERENTIAL, if there’s no prior FULL, the correct behavior is usually to fail or force a FULL backup.

Suggestion

Treat “no baseline found” as a hard error (or automatically behave as FULL) to avoid silently producing incomplete backups.

Example:

const lastBackupTime = await this.getLastBackupTime(requireFull)
if (!lastBackupTime) {
  if (type === BackupType.DIFFERENTIAL) {
    throw new Error('No FULL backup found; cannot create DIFFERENTIAL backup')
  }
  // INCREMENTAL: either throw or treat as FULL
  query = {}
} else {
  query = { updatedAt: { $gte: lastBackupTime } }
}

Reply with "@CharlieHelps yes please" if you want me to add a commit implementing this baseline behavior.


for (const modelName of models) {
const Model = mongoose.model(modelName);
const query: Record<string, unknown> = {};

if (baselineTime && Model.schema.paths.updatedAt) {
query.updatedAt = { $gte: baselineTime };
}

if (!isFirstModel) {
appDataJson += ',';
}
appDataJson += '"' + modelName + '":[';
Comment on lines +874 to +877
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

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

Building a large JSON payload via repeated string += ... inside nested loops can become very costly (frequent reallocations/copies) as the backup grows. Prefer accumulating into an array of string chunks and join('') once, or write into a dedicated buffer/stream abstraction; it keeps the same sequential read pattern without the quadratic-ish string concatenation overhead.

Copilot uses AI. Check for mistakes.
isFirstModel = false;

const cursor = Model.find(query).lean().cursor();
let isFirstDoc = true;

for await (const doc of cursor) {
if (!isFirstDoc) {
appDataJson += ',';
}
appDataJson += JSON.stringify(doc);
isFirstDoc = false;
}
Comment on lines +883 to +889
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

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

Building a large JSON payload via repeated string += ... inside nested loops can become very costly (frequent reallocations/copies) as the backup grows. Prefer accumulating into an array of string chunks and join('') once, or write into a dedicated buffer/stream abstraction; it keeps the same sequential read pattern without the quadratic-ish string concatenation overhead.

Copilot uses AI. Check for mistakes.

appDataJson += ']';
}

appDataJson += '}}';
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

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

Building a large JSON payload via repeated string += ... inside nested loops can become very costly (frequent reallocations/copies) as the backup grows. Prefer accumulating into an array of string chunks and join('') once, or write into a dedicated buffer/stream abstraction; it keeps the same sequential read pattern without the quadratic-ish string concatenation overhead.

Copilot uses AI. Check for mistakes.
} catch (error: unknown) {
logger.error(`Failed to collect data for backup: ${error instanceof Error ? error.message : String(error)}`);
throw error; // Fail loudly to prevent silent data corruption
}

// Use TextEncoder for cross-environment compatibility
return new TextEncoder().encode(JSON.stringify(dummyData))
return new TextEncoder().encode(appDataJson);
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

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

Building a large JSON payload via repeated string += ... inside nested loops can become very costly (frequent reallocations/copies) as the backup grows. Prefer accumulating into an array of string chunks and join('') once, or write into a dedicated buffer/stream abstraction; it keeps the same sequential read pattern without the quadratic-ish string concatenation overhead.

Copilot uses AI. Check for mistakes.
}

/**
Expand Down
Loading