-
Notifications
You must be signed in to change notification settings - Fork 212
fix(core): recover html from fenced revision replies #19
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,3 @@ | ||
| import { basename } from 'node:path'; | ||
| import { type ArtifactEvent, createArtifactParser } from '@open-codesign/artifacts'; | ||
| import { type RetryReason, complete, completeWithRetry } from '@open-codesign/providers'; | ||
| import type { | ||
|
|
@@ -75,28 +74,68 @@ interface ModelRunInput { | |
| messages: ChatMessage[]; | ||
| } | ||
|
|
||
| function createHtmlArtifact(content: string, index: number): Artifact { | ||
| return { | ||
| id: `design-${index + 1}`, | ||
| type: 'html', | ||
| title: 'Design', | ||
| content, | ||
| designParams: [], | ||
| createdAt: new Date().toISOString(), | ||
| }; | ||
| } | ||
|
|
||
| function collect(events: Iterable<ArtifactEvent>, into: Collected): void { | ||
| for (const ev of events) { | ||
| if (ev.type === 'text') { | ||
| into.text += ev.delta; | ||
| } else if (ev.type === 'artifact:end') { | ||
| into.artifacts.push({ | ||
| id: ev.identifier || `design-${into.artifacts.length + 1}`, | ||
| type: 'html', | ||
| title: 'Design', | ||
| content: ev.fullContent, | ||
| designParams: [], | ||
| createdAt: new Date().toISOString(), | ||
| }); | ||
| const artifact = createHtmlArtifact(ev.fullContent, into.artifacts.length); | ||
| if (ev.identifier) artifact.id = ev.identifier; | ||
| into.artifacts.push(artifact); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| function extractHtmlDocument(source: string): string | null { | ||
| const doctypeMatch = source.match(/<!doctype html[\s\S]*?<\/html>/i); | ||
| if (doctypeMatch) return doctypeMatch[0].trim(); | ||
|
|
||
| const htmlMatch = source.match(/<html[\s\S]*?<\/html>/i); | ||
| if (htmlMatch) return htmlMatch[0].trim(); | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| function extractFallbackArtifact(text: string): { artifact: Artifact | null; message: string } { | ||
| const fencedMatches = [...text.matchAll(/```(?:html)?\s*([\s\S]*?)```/gi)]; | ||
| for (const match of fencedMatches) { | ||
| const block = match[1]; | ||
| const matchedText = match[0]; | ||
| if (!block || !matchedText) continue; | ||
|
|
||
| const html = extractHtmlDocument(block); | ||
| if (!html) continue; | ||
|
|
||
| return { | ||
| artifact: createHtmlArtifact(html, 0), | ||
| message: text.replace(matchedText, '').trim(), | ||
| }; | ||
| } | ||
|
|
||
| const html = extractHtmlDocument(text); | ||
| if (!html) return { artifact: null, message: text.trim() }; | ||
|
|
||
| return { | ||
| artifact: createHtmlArtifact(html, 0), | ||
| message: text.replace(html, '').trim(), | ||
| }; | ||
| } | ||
|
|
||
| function formatDesignSystem(designSystem: StoredDesignSystem): string { | ||
| const repoLabel = basename(designSystem.rootPath); | ||
| const lines = [ | ||
| '## Design system to follow', | ||
| `Repository: ${repoLabel}`, | ||
| `Root path: ${designSystem.rootPath}`, | ||
| `Summary: ${designSystem.summary}`, | ||
| ]; | ||
| if (designSystem.colors.length > 0) lines.push(`Colors: ${designSystem.colors.join(', ')}`); | ||
|
|
@@ -114,7 +153,7 @@ function formatAttachments(attachments: AttachmentContext[]): string | null { | |
| if (attachments.length === 0) return null; | ||
| const body = attachments | ||
| .map((file, index) => { | ||
| const lines = [`${index + 1}. ${file.name}`]; | ||
| const lines = [`${index + 1}. ${file.name} (${file.path})`]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Major] This now includes absolute local paths in prompt context. That leaks filesystem structure to the provider even though the basename/filename was enough before. Suggested fix: import { basename } from 'node:path';
const lines = [
'## Design system to follow',
`Repository: ${basename(designSystem.rootPath)}`,
`Summary: ${designSystem.summary}`,
];
const lines = [`${index + 1}. ${file.name}`]; |
||
| if (file.note) lines.push(`Note: ${file.note}`); | ||
| if (file.excerpt) lines.push(`Excerpt:\n${file.excerpt}`); | ||
| return lines.join('\n'); | ||
|
|
@@ -159,6 +198,7 @@ function buildRevisionPrompt(input: ApplyCommentInput, contextSections: string[] | |
| const parts = [ | ||
| 'Revise the existing HTML artifact below.', | ||
| 'Keep the overall structure, copy, and layout intact unless the user request requires a broader change.', | ||
| 'Prioritize the selected element first and avoid unrelated edits.', | ||
| `User request: ${input.comment.trim()}`, | ||
| `Selected element tag: <${input.selection.tag}>`, | ||
| `Selected element selector: ${input.selection.selector}`, | ||
|
|
@@ -172,7 +212,7 @@ function buildRevisionPrompt(input: ApplyCommentInput, contextSections: string[] | |
| parts.push(contextSections.join('\n\n')); | ||
| } | ||
| parts.push( | ||
| 'Return the full updated HTML artifact. Do not explain the diff line by line; a short summary outside the artifact is enough.', | ||
| 'Return exactly one full updated HTML artifact wrapped in the required <artifact> tag. Do not use Markdown code fences. A short summary outside the artifact is enough.', | ||
| ); | ||
| return parts.join('\n\n'); | ||
| } | ||
|
|
@@ -197,6 +237,14 @@ async function runModel(input: ModelRunInput): Promise<GenerateOutput> { | |
| collect(parser.feed(result.content), collected); | ||
| collect(parser.flush(), collected); | ||
|
|
||
| if (collected.artifacts.length === 0) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Blocker] This silently repairs malformed model output instead of surfacing the contract failure. Principle 10 forbids silent fallbacks, so if we keep this recovery path it needs to be explicit in the returned message (or throw with context). Suggested fix: if (collected.artifacts.length === 0) {
const fallback = extractFallbackArtifact(collected.text);
if (fallback.artifact) {
collected.artifacts.push(fallback.artifact);
collected.text = [
'Warning: recovered HTML from a Markdown-fenced response because the model skipped the required <artifact> tag.',
fallback.message,
].filter(Boolean).join('\n\n');
}
}
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Blocker] This still silently repairs malformed model output instead of surfacing the contract failure. Principles §10 requires the caller/UI to see that recovery happened, otherwise a bad provider response is indistinguishable from a valid Suggested fix: if (collected.artifacts.length === 0) {
const fallback = extractFallbackArtifact(collected.text);
if (fallback.artifact) {
collected.artifacts.push(fallback.artifact);
collected.text = [
'Recovered HTML from a fenced response because the model skipped the required <artifact> tag.',
fallback.message,
].filter(Boolean).join('\n\n');
}
} |
||
| const fallback = extractFallbackArtifact(collected.text); | ||
| if (fallback.artifact) { | ||
| collected.artifacts.push(fallback.artifact); | ||
| collected.text = fallback.message; | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| message: collected.text.trim(), | ||
| artifacts: collected.artifacts, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Major] This now sends the absolute design-system root path to the provider, and the same leak happens again below with
file.path. That is a privacy regression: the prompt only needs repo/file names here, not the user’s local filesystem layout.Suggested fix: