diff --git a/apps/daemon/src/artifact-stub-guard.ts b/apps/daemon/src/artifact-stub-guard.ts index 466d96c6e4..f348ad2a01 100644 --- a/apps/daemon/src/artifact-stub-guard.ts +++ b/apps/daemon/src/artifact-stub-guard.ts @@ -6,7 +6,7 @@ // The guard is structural: it compares the new body's size against the // largest prior sibling sharing the same identifier. It does not pattern- // match on phrasing, so it works regardless of which agent backend produced -// the regression. False positives are bounded by MIN_PRIOR_BYTES (we won't +// the regression. False positives are bounded by minPriorBytes (we won't // compare against priors that are themselves small) and minRetainedRatio. import type { Dirent } from 'node:fs'; @@ -36,8 +36,7 @@ export interface ArtifactStubGuardWarning { } export interface EvaluateArtifactStubGuardInput { - projectDir: string; - safeName: string; + scanDir: string; identifier: string; newSize: number; config: ArtifactStubGuardConfig; @@ -71,40 +70,59 @@ export const DEFAULT_ARTIFACT_STUB_GUARD_CONFIG: ArtifactStubGuardConfig = { minPriorBytes: 4096, }; -function escapeRegExp(value: string): string { - return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); -} - // HTML-rendered manifest kinds. Decks are HTML files on disk and have the // same regression failure mode as plain html artifacts (the agent emits a // placeholder where a multi-KB framework should be), so they're guarded // alongside `html`. export const STUB_GUARDED_MANIFEST_KINDS: ReadonlySet = new Set(['html', 'deck']); +function escapeRegExp(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} + +// Mirror of the slugifier in `apps/web/src/components/ProjectView.tsx`'s +// `persistArtifact`. The web path slugifies the identifier for the +// filename basename but keeps the *raw* identifier in the manifest, so a +// regex anchored on the raw identifier alone can miss its own slug-form +// siblings on disk. We try both forms. +export function slugifyArtifactIdentifier(value: string): string { + return value + .toLowerCase() + .replace(/[^a-z0-9_-]+/g, '-') + .replace(/^-+|-+$/g, '') + .slice(0, 60); +} + // Finds prior HTML siblings on disk that share an identifier with a // newly-written artifact. The frontend's collision-suffixing scheme means -// related entries match `(-\d+)?\.html?`. We scan once per -// write; project directories are small so this is cheap. +// related entries match `(-\d+)?\.html?`. The scan deliberately +// includes any file at the same path as the new write — when an agent +// overwrites `dashboard.html` with the same name, the file currently on +// disk is the prior content (the overwrite happens after this scan). export async function findPriorArtifactSiblings( - projectDir: string, + scanDir: string, identifier: string, - excludeSafeName: string, ): Promise { if (identifier.length === 0) return []; - const pattern = new RegExp(`^${escapeRegExp(identifier)}(?:-\\d+)?\\.html?$`); + const tokens = new Set(); + if (identifier.length > 0) tokens.add(identifier); + const slug = slugifyArtifactIdentifier(identifier); + if (slug.length > 0) tokens.add(slug); + if (tokens.size === 0) return []; + const alternation = Array.from(tokens, escapeRegExp).join('|'); + const pattern = new RegExp(`^(?:${alternation})(?:-\\d+)?\\.html?$`); let entries: Dirent[]; try { - entries = await readdir(projectDir, { withFileTypes: true }); + entries = await readdir(scanDir, { withFileTypes: true }); } catch { return []; } const results: PriorArtifactSibling[] = []; for (const entry of entries) { if (!entry.isFile()) continue; - if (entry.name === excludeSafeName) continue; if (!pattern.test(entry.name)) continue; try { - const st = await stat(path.join(projectDir, entry.name)); + const st = await stat(path.join(scanDir, entry.name)); results.push({ name: entry.name, size: st.size }); } catch { // ignore unreadable entries; they don't influence the guard decision @@ -188,10 +206,6 @@ export async function evaluateArtifactStubGuard( ): Promise { if (input.config.mode === 'off') return { outcome: 'pass' }; if (input.identifier.length === 0) return { outcome: 'pass' }; - const priors = await findPriorArtifactSiblings( - input.projectDir, - input.identifier, - input.safeName, - ); + const priors = await findPriorArtifactSiblings(input.scanDir, input.identifier); return classifyArtifactStubGuard(priors, input.identifier, input.newSize, input.config); } diff --git a/apps/daemon/src/projects.ts b/apps/daemon/src/projects.ts index a44c52e442..71a8757c0a 100644 --- a/apps/daemon/src/projects.ts +++ b/apps/daemon/src/projects.ts @@ -413,9 +413,11 @@ export async function writeProjectFile( // Other kinds (markdown, svg, code-snippet) can legitimately be small // and are skipped. if (identifier.length > 0 && STUB_GUARDED_MANIFEST_KINDS.has(validatedManifest.kind)) { + // Scan the directory the new file actually lands in, not the project + // root — writeProjectFile accepts nested paths like reports/X.html + // and a root-only scan would miss prior siblings in subdirectories. const guard = await evaluateArtifactStubGuard({ - projectDir: dir, - safeName, + scanDir: path.dirname(target), identifier, newSize: Buffer.byteLength(body), config: readArtifactStubGuardConfigFromEnv(), diff --git a/apps/daemon/tests/artifact-stub-guard.test.ts b/apps/daemon/tests/artifact-stub-guard.test.ts index dea16657ee..7ca8aa2242 100644 --- a/apps/daemon/tests/artifact-stub-guard.test.ts +++ b/apps/daemon/tests/artifact-stub-guard.test.ts @@ -116,7 +116,7 @@ describe('findPriorArtifactSiblings', () => { return dir; } - it('finds bare and suffixed siblings while excluding the new entry', async () => { + it('finds bare and suffixed siblings, including the same-named target if it exists', async () => { const dir = await makeDir(); await writeFile(path.join(dir, 'report.html'), 'a'.repeat(20_000)); await writeFile(path.join(dir, 'report-2.html'), 'b'.repeat(40_000)); @@ -124,13 +124,17 @@ describe('findPriorArtifactSiblings', () => { await writeFile(path.join(dir, 'unrelated.html'), 'x'.repeat(50_000)); await writeFile(path.join(dir, 'report-2.html.artifact.json'), '{}'); - const priors = await findPriorArtifactSiblings(dir, 'report', 'report-3.html'); + // The target 'report-3.html' is included because it currently exists on + // disk and its current size is the prior content (the overwrite that + // would replace it has not happened yet at scan time). This is the + // same-name-overwrite case: see lefarcen P1. + const priors = await findPriorArtifactSiblings(dir, 'report'); const names = priors.map((p) => p.name).sort(); - expect(names).toEqual(['report-2.html', 'report.html']); + expect(names).toEqual(['report-2.html', 'report-3.html', 'report.html']); }); it('returns an empty list when the directory does not exist', async () => { - const priors = await findPriorArtifactSiblings('/nonexistent/od/projects/missing', 'dashboard', 'dashboard.html'); + const priors = await findPriorArtifactSiblings('/nonexistent/od/projects/missing', 'dashboard'); expect(priors).toEqual([]); }); @@ -139,8 +143,9 @@ describe('findPriorArtifactSiblings', () => { await writeFile(path.join(dir, 'landing.html'), 'a'.repeat(1_000)); await writeFile(path.join(dir, 'landing-page.html'), 'b'.repeat(1_000)); - const priors = await findPriorArtifactSiblings(dir, 'landing', 'landing.html'); - expect(priors).toEqual([]); + const priors = await findPriorArtifactSiblings(dir, 'landing'); + const names = priors.map((p) => p.name).sort(); + expect(names).toEqual(['landing.html']); }); it('also matches .htm siblings', async () => { @@ -148,10 +153,21 @@ describe('findPriorArtifactSiblings', () => { await writeFile(path.join(dir, 'overview-doc.htm'), 'a'.repeat(20_000)); await writeFile(path.join(dir, 'overview-doc-2.html'), 'b'.repeat(30_000)); - const priors = await findPriorArtifactSiblings(dir, 'overview-doc', 'overview-doc-3.html'); + const priors = await findPriorArtifactSiblings(dir, 'overview-doc'); const names = priors.map((p) => p.name).sort(); expect(names).toEqual(['overview-doc-2.html', 'overview-doc.htm']); }); + + it('matches siblings using the slugified form of a non-slug identifier', async () => { + const dir = await makeDir(); + // Frontend persistArtifact slugifies "Landing Page" -> "landing-page" + // for the filename but keeps the raw "Landing Page" in the manifest. + // Both forms must find the same prior sibling on disk. + await writeFile(path.join(dir, 'landing-page.html'), 'a'.repeat(40_000)); + + const priors = await findPriorArtifactSiblings(dir, 'Landing Page'); + expect(priors.map((p) => p.name)).toEqual(['landing-page.html']); + }); }); describe('evaluateArtifactStubGuard (integration with disk scan)', () => { @@ -174,8 +190,7 @@ describe('evaluateArtifactStubGuard (integration with disk scan)', () => { await writeFile(path.join(dir, 'presentation.html'), 'p'.repeat(60_000)); const result = await evaluateArtifactStubGuard({ - projectDir: dir, - safeName: 'presentation-2.html', + scanDir: dir, identifier: 'presentation', newSize: 200, config: rejectingConfig(), @@ -190,8 +205,7 @@ describe('evaluateArtifactStubGuard (integration with disk scan)', () => { await writeFile(path.join(dir, 'presentation.html'), 'p'.repeat(60_000)); const result = await evaluateArtifactStubGuard({ - projectDir: dir, - safeName: 'presentation-2.html', + scanDir: dir, identifier: 'presentation', newSize: 50_000, config: rejectingConfig(), diff --git a/apps/daemon/tests/projects-stub-guard.test.ts b/apps/daemon/tests/projects-stub-guard.test.ts index b040ce08b9..6a86149ee9 100644 --- a/apps/daemon/tests/projects-stub-guard.test.ts +++ b/apps/daemon/tests/projects-stub-guard.test.ts @@ -240,4 +240,79 @@ describe('artifact stub guard via /api/projects/:id/files', () => { expect(body.error.code).toBe('ARTIFACT_REGRESSION'); expect(body.error.details?.priorName).toBe('overview-doc.htm'); }); + + it('rejects a same-name overwrite that shrinks the existing file (lefarcen P1)', async () => { + const projectId = await createProject('overwrite'); + + const firstResp = await postFile(projectId, { + name: 'dashboard.html', + content: htmlBody(20_000), + artifactManifest: manifestFor('dashboard'), + }); + expect(firstResp.status).toBe(200); + + // Same name, same identifier, stub body: existing file is the prior. + const stubResp = await postFile(projectId, { + name: 'dashboard.html', + content: 'see dashboard.html', + artifactManifest: manifestFor('dashboard'), + }); + expect(stubResp.status).toBe(422); + const body = (await stubResp.json()) as { + error: { code: string; details?: { priorName?: string } }; + }; + expect(body.error.code).toBe('ARTIFACT_REGRESSION'); + expect(body.error.details?.priorName).toBe('dashboard.html'); + + // Confirm the original 20 KB file is intact (not overwritten). + const filesResp = await fetch(`${baseUrl}/api/projects/${projectId}/files`); + const files = (await filesResp.json()) as { files: Array<{ name: string; size: number }> }; + const dashboard = files.files.find((f) => f.name === 'dashboard.html'); + expect(dashboard?.size).toBeGreaterThan(15_000); + }); + + it('rejects stub regressions in subdirectories (Codex/mrcfps P2)', async () => { + const projectId = await createProject('nested'); + + const firstResp = await postFile(projectId, { + name: 'reports/overview.html', + content: htmlBody(20_000), + artifactManifest: manifestFor('overview'), + }); + expect(firstResp.status).toBe(200); + + const stubResp = await postFile(projectId, { + name: 'reports/overview-2.html', + content: 'see reports/overview.html', + artifactManifest: manifestFor('overview'), + }); + expect(stubResp.status).toBe(422); + const body = (await stubResp.json()) as { + error: { code: string; details?: { priorName?: string } }; + }; + expect(body.error.code).toBe('ARTIFACT_REGRESSION'); + expect(body.error.details?.priorName).toBe('overview.html'); + }); + + it('finds slug-form sibling when manifest carries non-slug identifier (Codex/lefarcen/mrcfps P2)', async () => { + const projectId = await createProject('slug'); + + // Frontend wrote the previous artifact under a slugified name but the + // manifest carried the raw identifier "Landing Page". + const firstResp = await postFile(projectId, { + name: 'landing-page.html', + content: htmlBody(20_000), + artifactManifest: { ...manifestFor('landing-page'), metadata: { identifier: 'Landing Page' } }, + }); + expect(firstResp.status).toBe(200); + + const stubResp = await postFile(projectId, { + name: 'landing-page-2.html', + content: 'see landing-page.html', + artifactManifest: { ...manifestFor('landing-page'), metadata: { identifier: 'Landing Page' } }, + }); + expect(stubResp.status).toBe(422); + const body = (await stubResp.json()) as { error: { code: string } }; + expect(body.error.code).toBe('ARTIFACT_REGRESSION'); + }); }); diff --git a/apps/web/src/components/ProjectView.tsx b/apps/web/src/components/ProjectView.tsx index 3c3079d349..e3cc284826 100644 --- a/apps/web/src/components/ProjectView.tsx +++ b/apps/web/src/components/ProjectView.tsx @@ -1468,6 +1468,19 @@ export function ProjectView({ }); if (file) { setFilesRefresh((n) => n + 1); + // Surface the daemon's stub-guard warning when it fires in `warn` + // mode (the default). Without this the warning would land in the + // file metadata silently and the user would never see that the + // model shipped a placeholder. Reject mode returns null from + // writeProjectTextFile (the 422 is collapsed by the API client) — + // surfacing that case requires plumbing error responses through + // writeProjectTextFile, tracked separately. + if (file.stubGuardWarning) { + setError( + `Saved "${file.name}", but the model may have shipped a placeholder: ` + + `${file.stubGuardWarning.message}`, + ); + } // Auto-open the freshly-persisted artifact as a tab so the user // sees it without an extra click. The Write-tool path already does // this for tool-emitted files; this handles the artifact-tag path.