feat(html): enable interactive export runtime - #71
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 357fc93344
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| function runtimeSource(mode: HtmlExportRuntimeMode): string { | ||
| const slide = mode === 'slide' ? 'true' : 'false'; | ||
| return `(function(){if(document.getElementById('nai-runtime'))return;var root=document.documentElement,button=document.createElement('button'),style=document.createElement('style');button.id='nai-runtime';button.type='button';button.className='nai-theme-toggle';button.style.cssText='position:fixed;top:12px;right:12px;z-index:2147483647;border:0;border-radius:999px;padding:8px 10px;cursor:pointer;background:#111;color:#fff';function setTheme(theme){root.dataset.theme=theme;try{localStorage.setItem('nai-theme',theme)}catch(_e){}button.textContent=theme==='dark'?'☀':'🌙';button.setAttribute('aria-label',theme==='dark'?'Switch to light theme':'Switch to dark theme')}var saved;try{saved=localStorage.getItem('nai-theme')}catch(_e){}setTheme(saved==='light'||saved==='dark'?saved:(matchMedia&&matchMedia('(prefers-color-scheme: dark)').matches?'dark':'light'));button.addEventListener('click',function(){setTheme(root.dataset.theme==='dark'?'light':'dark')});document.body.appendChild(button);var authored=Array.prototype.some.call(document.querySelectorAll('style'),function(s){return /\\[data-theme[^}]*--/.test(s.textContent||'')});if(!authored){style.id='nai-theme-fallback';style.textContent='[data-theme="dark"]{filter:invert(1) hue-rotate(180deg)}[data-theme="dark"] img,[data-theme="dark"] video{filter:invert(1) hue-rotate(180deg)}';document.head.appendChild(style)}if(${slide}){var slides=Array.prototype.slice.call(document.querySelectorAll('section.slide'));if(!slides.length){slides=Array.prototype.slice.call(document.body.children).filter(function(node){return node.tagName==='SECTION'})}if(!slides.length)return;var index=0,controls=document.createElement('div'),previous=document.createElement('button'),next=document.createElement('button'),indicator=document.createElement('span');controls.className='nai-slide-nav';controls.style.cssText='position:fixed;bottom:12px;right:12px;z-index:2147483647;display:flex;gap:8px;align-items:center;background:#111;color:#fff;padding:8px;border-radius:999px';previous.type=next.type='button';previous.textContent='‹';next.textContent='›';function show(n){index=(n+slides.length)%slides.length;slides.forEach(function(s,i){s.style.display=i===index?'':'none';s.style.minHeight='100vh'});indicator.textContent=(index+1)+'/'+slides.length;slides[index].scrollIntoView({block:'start'})}previous.addEventListener('click',function(){show(index-1)});next.addEventListener('click',function(){show(index+1)});controls.append(previous,indicator,next);document.body.appendChild(controls);document.addEventListener('keydown',function(event){var target=event.target;if(target instanceof HTMLInputElement||target instanceof HTMLTextAreaElement||target&&target.isContentEditable)return;if(['ArrowRight','PageDown',' '].includes(event.key)){event.preventDefault();show(index+1)}else if(['ArrowLeft','PageUp'].includes(event.key)){event.preventDefault();show(index-1)}});show(0)}})();`; |
There was a problem hiding this comment.
When the model follows the new prompt and defines palettes under [data-theme="light"] / [data-theme="dark"], the CSS sanitizer scopes those selectors under [data-he-content] (or rewrites html/body/:root to that wrapper), but this runtime toggles data-theme on document.documentElement. In that normal export path the authored custom properties never match, and the fallback is skipped because the runtime detects authored [data-theme] vars, so color declarations using var() can compute as invalid/default instead of applying either palette.
Useful? React with 👍 / 👎.
| '</html>\n'; | ||
|
|
||
| return { html, manifest }; | ||
| return { html: injectHtmlExportRuntime(html), manifest }; |
There was a problem hiding this comment.
bundleSanitizedHtml is the resolver used by src/main/main.ts, so this call injects the runtime with the default scroll mode before finalization has any chance to know that the wizard selected slides. Because injectHtmlExportRuntime returns early once id="nai-runtime" exists, even a later finalize(..., 'slide') could not replace the already-injected if(false) runtime, leaving slide exports without the new deck navigation/hiding behavior.
Useful? React with 👍 / 👎.
project820
left a comment
project820
left a comment
There was a problem hiding this comment.
🤖 Psycho-842 [BOT] review
⚖️ Verdict: DO NOT MERGE — the required verify check fails on the PR head, and slide mode never reaches the runtime finalizer.
Summary
This implements the interactive export path for #70: sanitizer relaxation, a new app-owned runtime, and revised authoring instructions.
The current branch breaks the existing HTML-export contract tests, so the required CI gate is red.
Separately, both production finalization paths omit the selected export mode, so a slide export receives the runtime's default scroll mode and never initializes slide navigation.
Findings (by severity)
- 🤷🏻♂️ P0 Critical-WTF? — Required
verifyCI is failing on357fc93344fe323c73c7bc9124170feed5b01cd0: GitHub'snpm run testreports 26 failures. The failures include the shell contract, sanitizer expectations, finalization-byte assertions, and the wizard prompt expectation. Update the affected tests for the intended contract and add coverage for the new runtime before this can merge. - 🔴 P1 High —
src/main/html-export-generation-orchestrator.ts:309finalizes with only(webContentsId, attemptId, resolvedArtifactId), whileHtmlExportPipelineService.finalizedefaults its fourth argument to'scroll'.src/main/html-export-generate.tsalso carries nomodefield through its input to the orchestrator. Consequently the injected runtime emitsif(false)for its slide branch even when the prompt requested slide mode, so the promised slide controls and keyboard navigation are absent. Thread the resolved config mode throughhtml:generate→ generator/orchestrator →finalize, and cover bothscrollandslideoutputs.
📋 Detailed analysis (measured CI, diff, and validation)
- Head:
357fc93344fe323c73c7bc9124170feed5b01cd0; diff: +62/−45 across 6 files. - GitHub HEAD check:
verify=FAILURE(npm run test, 26 failures).mergeable=MERGEABLE,mergeStateStatus=UNSTABLE. - Local verification:
npm run typecheckpassed. - Focused local checks reproduced the red suite:
html-export-shell.test.ts6 failed / 12 passed;html-export-sanitize.test.ts13 failed / 133 passed;html-export-finalize.test.ts2 failed / 6 passed. - Runtime probe: the default call to
injectHtmlExportRuntime(...)printeddefault-finalize=scroll; slide-navigation-branch=false. - Opened for context:
html-export-runtime.ts,html-export-pipeline-service.ts,html-export-generation-orchestrator.ts,html-export-generate.ts,html-export-ipc.ts, the shell/sanitizer/finalize tests, and the direct prompt. - Caller trace:
finalizehas two production callers (html-export-generation-orchestrator.ts:309andhtml-export-ipc.ts:567); neither passesmode, so both select the default. - Other review evidence: no existing reviews, issue comments, or unresolved review threads were present. No other open PR touches these six files. Related issue: #70.
This is a COMMENT review, not an approval; explicit user final sign-off is still required.
⛔ DO NOT MERGE
🦀 Reviewed by Hermes agent BOT 🦞
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b7ca505ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let output = html.replace(/<meta\s+http-equiv=["']Content-Security-Policy["'][^>]*>\s*/gi, ''); | ||
| const head = `${HTML_EXPORT_INTERACTIVE_CSP_META}`; | ||
| output = /<\/head\s*>/i.test(output) ? output.replace(/<\/head\s*>/i, `${head}</head>`) : `${head}${output}`; | ||
| if (/id=["']nai-runtime["']/i.test(output)) return output; |
There was a problem hiding this comment.
When generated content happens to include id="nai-runtime", the sanitizer does not reserve the new nai-* namespace, so this whole-document regex treats authored markup inside [data-he-content] as the app runtime and returns before injecting the real script. In that case the finalized file loses the theme toggle, and slide exports lose navigation; please either check for the actual app <script id="nai-runtime"> outside authored content or reject/reserve that id during sanitization.
Useful? React with 👍 / 👎.
| 'li', 'dl', 'dt', 'dd', 'blockquote', 'figure', 'figcaption', 'img', 'picture', 'source', 'svg', | ||
| 'table', 'thead', 'tbody', 'tfoot', 'tr', 'th', 'td', 'caption', 'code', 'pre', 'kbd', 'samp', | ||
| 'abbr', 'time', 'a', | ||
| 'abbr', 'time', 'a', 'script', 'form', 'input', 'button', |
There was a problem hiding this comment.
When the user selects a static export (for example the report/blog/proposal presets default to interactive: false), the pipeline still uses this same sanitizer and never passes the interactive setting into sanitizeHtmlExport, so adding script here preserves model-authored scripts unconditionally. If the model emits a script despite the static prompt, the saved file becomes interactive instead of honoring the UI knob; please strip scripts unless interactivity is enabled or carry that option into the sanitizer.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de61c1627c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| function runtimeSource(mode: HtmlExportRuntimeMode): string { | ||
| const slide = mode === 'slide' ? 'true' : 'false'; | ||
| return `(function(){var root=document.documentElement,content=document.querySelector('[data-he-content]');if(root.dataset.naiRuntime)return;root.dataset.naiRuntime='true';var button=document.createElement('button'),style=document.createElement('style');button.id='nai-runtime-toggle';button.type='button';button.className='nai-theme-toggle';button.style.cssText='position:fixed;top:12px;right:12px;z-index:2147483647;border:0;border-radius:999px;padding:8px 10px;cursor:pointer;background:#111;color:#fff';function setTheme(theme){root.dataset.theme=theme;if(content)content.dataset.theme=theme;try{localStorage.setItem('nai-theme',theme)}catch(_e){}button.textContent=theme==='dark'?'☀':'🌙';button.setAttribute('aria-label',theme==='dark'?'Switch to light theme':'Switch to dark theme')}var saved;try{saved=localStorage.getItem('nai-theme')}catch(_e){}setTheme(saved==='light'||saved==='dark'?saved:(matchMedia&&matchMedia('(prefers-color-scheme: dark)').matches?'dark':'light'));button.addEventListener('click',function(){setTheme(root.dataset.theme==='dark'?'light':'dark')});document.body.appendChild(button);var authored=content&&Array.prototype.some.call(document.styleSheets,function(sheet){try{return Array.prototype.some.call(sheet.cssRules||[],function(rule){if(!rule.selectorText||rule.selectorText.indexOf('data-he-content')===-1||!Array.prototype.some.call(rule.style||[],function(name){return String(name).indexOf('--')===0}))return false;var theme=content.dataset.theme;var matches=['light','dark'].some(function(value){content.dataset.theme=value;return content.matches(rule.selectorText)});content.dataset.theme=theme;return matches})}catch(_e){return false}});if(!authored){style.id='nai-theme-fallback';style.textContent='[data-he-content][data-theme="dark"]{filter:invert(1) hue-rotate(180deg)}[data-he-content][data-theme="dark"] img,[data-he-content][data-theme="dark"] video{filter:invert(1) hue-rotate(180deg)}';document.head.appendChild(style)}if(${slide}){var slides=Array.prototype.slice.call(document.querySelectorAll('section.slide'));if(!slides.length){slides=Array.prototype.slice.call((content||document.body).children).filter(function(node){return node.tagName==='SECTION'})}if(!slides.length)return;var index=0,controls=document.createElement('div'),previous=document.createElement('button'),next=document.createElement('button'),indicator=document.createElement('span');controls.className='nai-slide-nav';controls.style.cssText='position:fixed;bottom:12px;right:12px;z-index:2147483647;display:flex;gap:8px;align-items:center;background:#111;color:#fff;padding:8px;border-radius:999px';previous.type=next.type='button';previous.textContent='‹';next.textContent='›';function show(n){index=(n+slides.length)%slides.length;slides.forEach(function(s,i){s.style.display=i===index?'':'none';s.style.minHeight='100vh'});indicator.textContent=(index+1)+'/'+slides.length;slides[index].scrollIntoView({block:'start'})}previous.addEventListener('click',function(){show(index-1)});next.addEventListener('click',function(){show(index+1)});controls.append(previous,indicator,next);document.body.appendChild(controls);document.addEventListener('keydown',function(event){var target=event.target;if(target instanceof HTMLInputElement||target instanceof HTMLTextAreaElement||target instanceof HTMLButtonElement||target instanceof HTMLSelectElement||target&&target.tagName==='SUMMARY'||target&&target instanceof Element&&target.closest('[contenteditable]'))return;if(['ArrowRight','PageDown',' '].includes(event.key)){event.preventDefault();show(index+1)}else if(['ArrowLeft','PageUp'].includes(event.key)){event.preventDefault();show(index-1)}});show(0)}})();`; |
There was a problem hiding this comment.
Fresh evidence in this revision is that sanitized author styles are always emitted under @layer he-authored (src/main/html-export-sanitize.ts line 718), but this runtime only inspects the top-level sheet.cssRules entries for selectorText and custom-property declarations. For normal exports that follow the new prompt and define [data-theme="light"] / [data-theme="dark"] variables inside that layer (or nested media/supports), the scan misses the authored palettes, treats authored as false, and injects the inversion fallback on top of the real dark palette, so toggling dark mode corrupts the intended colors instead of using the generated theme.
Useful? React with 👍 / 👎.
5a9a3a8 to
9676b17
Compare
9676b17 to
e81e5a9
Compare
|
The DO NOT MERGE review targets stale HEAD 357fc93 (first commit). Point-by-point status at current HEAD e81e5a9 (CI: verify SUCCESS — which itself refutes the P0):
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e81e5a93e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 'li', 'dl', 'dt', 'dd', 'blockquote', 'figure', 'figcaption', 'img', 'picture', 'source', 'svg', | ||
| 'table', 'thead', 'tbody', 'tfoot', 'tr', 'th', 'td', 'caption', 'code', 'pre', 'kbd', 'samp', | ||
| 'abbr', 'time', 'a', | ||
| 'abbr', 'time', 'a', 'script', 'form', 'input', 'button', |
There was a problem hiding this comment.
When the model follows the new interactive prompt but emits its setup script in <head> (a common pattern for DOMContentLoaded handlers or module/bootstrap code), adding script to the allowed vocabulary still does not preserve it: the head branch later calls scanDiscardedNode(...) and returns [], and that scan only extracts styles/diagnostics rather than moving allowed scripts into the sanitized body. Those exports will finalize successfully but lose the authored interactivity; please either keep/move allowed head scripts or prompt/sanitize consistently so scripts outside the body are rejected rather than silently dropped.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8c248e288
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const text = generated(rewritten); | ||
| return beginsAtRoot ? text : `${CONTENT_ROOT_SELECTOR} ${text}`; | ||
| if (text.startsWith(CONTENT_ROOT_SELECTOR)) return text; | ||
| if (text.startsWith('[data-theme=') || text.startsWith('[data-theme]')) return `${CONTENT_ROOT_SELECTOR}${text}`; |
There was a problem hiding this comment.
When generated CSS starts a theme selector with a sibling combinator, e.g. [data-theme="dark"] ~ button, this concatenation produces [data-he-content][data-theme=dark]~button instead of keeping the right-hand side inside the wrapper. In finalized files the app appends the theme toggle and slide controls as following body siblings, so such authored CSS can hide or restyle those app controls and breaks the sanitizer's content-root containment; reject sibling combinators for theme-root selectors or scope the rest under the content root.
Useful? React with 👍 / 👎.
| if (isFailure(sanitized)) return { ok: false, violations: [sanitized.violation] }; | ||
| outputNodes.push(...sanitized); | ||
| } | ||
| outputNodes.push(...context.relocatedHeadScripts); |
There was a problem hiding this comment.
When a generated document uses the normal pattern <head><script>window.init=...</script></head><body>...<script>init()</script>, the head script is queued during head scanning but then appended after all sanitized body nodes here. That reverses execution order versus the authored HTML, so body scripts can run before helpers/bootstrap state from the head exists even though both scripts were preserved; insert relocated head scripts before body scripts or keep them in the head while maintaining source order.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6a81a3cf8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function isAllowedAttribute(tag: string, name: string): boolean { | ||
| if (GLOBAL_ATTRIBUTES.has(name) || isAriaAttribute(name) || name === 'data-section-id') return true; | ||
| if (GLOBAL_ATTRIBUTES.has(name) || isAriaAttribute(name) || name === 'data-section-id' || name.startsWith('data-')) return true; | ||
| if (name.startsWith('on')) return true; |
There was a problem hiding this comment.
When the user selects a static export in Detail mode, this sanitizer path still preserves inline handlers such as <button onclick="..."> because the pipeline does not pass the interactive setting into sanitizeHtmlExport. This is separate from the existing script-tag issue: event-handler attributes execute under the injected inline-script CSP even without any <script> element, so a model that emits onclick despite the static prompt produces an interactive saved file instead of honoring the UI knob.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same disposition as the script-tag twin (see PR discussion): allowing scripts/handlers regardless of the interactive checkbox is the deliberate personal-use policy — the checkbox shapes the PROMPT (whether interactivity is solicited), not the sanitize vocabulary; network is blocked by CSP either way. Both are tracked together in #66 as an optional wizard preference (strip scripts+handlers when interactive is OFF).
project820
left a comment
project820
left a comment
There was a problem hiding this comment.
🤖 Psycho-842 [BOT] review
⚖️ Verdict: NEEDS REVISION — CI is green and earlier blockers are fixed, but the runtime still misdetects authored theme palettes inside the sanitizer's @layer wrapper.
Summary
This settlement re-measured the interactive HTML-export path at a6a81a3 after five follow-up commits.
The mode plumbing, content-root theme propagation, runtime namespace reservation, and relocated head-script ordering now have code and test coverage.
One P2 remains: a normal layered [data-theme] palette is missed, so the fallback inversion is injected over the authored dark theme.
Findings (by severity)
- 🟠 P2 Med —
src/main/html-export-runtime.ts:10only iterates top-levelsheet.cssRulesand accepts rules withselectorText.src/main/html-export-sanitize.ts:727always emits authored styles inside@layer he-authored, whose top-levelCSSLayerBlockRulehas noselectorText. Thus an authored[data-he-content][data-theme="dark"]{--bg:...}palette is not detected;#nai-theme-fallbackis added and its inversion/filter can corrupt the selected dark palette. Recursively inspect nested CSS grouping rules (including@layer,@media, and@supports) before deciding whether to inject the fallback, and add a finalized-artifact regression test.
Response to bot findings
- Content-root theme propagation, deferred finalization, and
nai-*reservation are present in the current diff; the focused runtime and pipeline tests cover those paths. - The latest commit places relocated head scripts before body scripts and scopes leading theme sibling selectors under the content root; corresponding sanitizer tests pass.
- The owner explicitly described static-export script/handler retention as the current product policy; this review does not reclassify that policy choice as a code defect.
- The layered-theme finding remains unresolved. All nine Codex threads are still open in GitHub, so only the items above were verified as addressed by the current code and tests.
📋 Detailed analysis (measured CI, diff, and validation)
- Settlement head:
a6a81a3cf84499d2977c0323b657757b16c8639b; +461/−249 across 23 files;mergeable=MERGEABLE,mergeStateStatus=CLEAN. - GitHub tip CI: required
verifysucceeded on this exact SHA (typecheck, 2,364 Vitest tests, build, security/direct/roundtrip harnesses, and knip). - Local validation on this SHA:
npm ci,npm run typecheck,npm test(149 files / 2,364 tests),npm run build, andnpm run test:html-export-directall passed. The direct harness covered scroll/slides × landscape/portrait with zero remote requests and preview/save digest equality. - Files opened for context:
html-export-runtime.ts,html-export-sanitize.ts,html-export-css-sanitize.ts,html-export-shell.ts,html-export-pipeline-service.ts,html-export-generation-orchestrator.ts,html-export-ipc.ts, the runtime/sanitizer/finalize/pipeline tests, and the direct harness. - Caller trace:
bundleSanitizedHtmlis the main-process resolver; finalization has production callers in the generation orchestrator and quarantine IPC handler. Both now pass the selected mode when supplied.injectHtmlExportRuntimeis the only runtime injector. - Adversarial pass — 4 angles tried: mode/ordering (current callers thread mode and finalize is the sole injector), namespace collision (sanitizer reserves
nai-*), head/body script ordering (relocated scripts are prepended), and authored palette detection. The first three were rejected by current code plus tests; the layered palette attack holds because the runtime does not recurse into grouping rules. - Related work: PR closes #70. No other open PR was found touching these 23 files. The initial review is here.
This is a COMMENT review, not an approval; explicit user final sign-off is still required.
🛠️ NEEDS REVISION
🦀 Reviewed by Hermes agent BOT 🦞
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa21348240
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const text = generated(rewritten); | ||
| return beginsAtRoot ? text : `${CONTENT_ROOT_SELECTOR} ${text}`; | ||
| if (text.startsWith(CONTENT_ROOT_SELECTOR)) return text; | ||
| if (text.startsWith('[data-theme=') || text.startsWith('[data-theme]')) return `${CONTENT_ROOT_SELECTOR}${text}`; |
There was a problem hiding this comment.
When generated CSS uses the common specificity-neutral form :where([data-theme="dark"]) (or :is(...)) for its palette, this branch does not recognize it as a theme-root selector and falls through to prefixing a descendant selector like [data-he-content] :where([data-theme=dark]). The runtime sets data-theme on [data-he-content] itself, not on a child, so those custom-property palettes never match and color declarations using var() can render invalid or fall back to the inversion filter instead of the authored theme.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79799213f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| function runtimeSource(mode: HtmlExportRuntimeMode): string { | ||
| const slide = mode === 'slide' ? 'true' : 'false'; | ||
| return `(function(){var root=document.documentElement,content=document.querySelector('[data-he-content]');if(root.dataset.naiRuntime)return;root.dataset.naiRuntime='true';var button=document.createElement('button'),style=document.createElement('style');button.id='nai-runtime-toggle';button.type='button';button.className='nai-theme-toggle';button.style.cssText='position:fixed;top:12px;right:12px;z-index:2147483647;border:0;border-radius:999px;padding:8px 10px;cursor:pointer;background:#111;color:#fff';function setTheme(theme){root.dataset.theme=theme;if(content)content.dataset.theme=theme;try{localStorage.setItem('nai-theme',theme)}catch(_e){}button.textContent=theme==='dark'?'☀':'🌙';button.setAttribute('aria-label',theme==='dark'?'Switch to light theme':'Switch to dark theme')}var saved;try{saved=localStorage.getItem('nai-theme')}catch(_e){}setTheme(saved==='light'||saved==='dark'?saved:(matchMedia&&matchMedia('(prefers-color-scheme: dark)').matches?'dark':'light'));button.addEventListener('click',function(){setTheme(root.dataset.theme==='dark'?'light':'dark')});document.body.appendChild(button);function hasAuthoredTheme(rule){if(rule.cssRules&&rule.cssRules.length)return Array.prototype.some.call(rule.cssRules,hasAuthoredTheme);if(!rule.selectorText||rule.selectorText.indexOf('data-he-content')===-1||!Array.prototype.some.call(rule.style||[],function(name){return String(name).indexOf('--')===0}))return false;var theme=content.dataset.theme;var matches=['light','dark'].some(function(value){content.dataset.theme=value;return content.matches(rule.selectorText)});content.dataset.theme=theme;return matches}var authored=content&&Array.prototype.some.call(document.styleSheets,function(sheet){try{return Array.prototype.some.call(sheet.cssRules||[],hasAuthoredTheme)}catch(_e){return false}});if(!authored){style.id='nai-theme-fallback';style.textContent='[data-he-content][data-theme="dark"]{filter:invert(1) hue-rotate(180deg)}[data-he-content][data-theme="dark"] img,[data-he-content][data-theme="dark"] video{filter:invert(1) hue-rotate(180deg)}';document.head.appendChild(style)}if(${slide}){var slides=Array.prototype.slice.call(document.querySelectorAll('section.slide'));if(!slides.length){slides=Array.prototype.slice.call((content||document.body).children).filter(function(node){return node.tagName==='SECTION'})}if(!slides.length)return;var index=0,controls=document.createElement('div'),previous=document.createElement('button'),next=document.createElement('button'),indicator=document.createElement('span');controls.className='nai-slide-nav';controls.style.cssText='position:fixed;bottom:12px;right:12px;z-index:2147483647;display:flex;gap:8px;align-items:center;background:#111;color:#fff;padding:8px;border-radius:999px';previous.type=next.type='button';previous.textContent='‹';next.textContent='›';function show(n){index=(n+slides.length)%slides.length;slides.forEach(function(s,i){s.style.display=i===index?'':'none';s.style.minHeight='100vh'});indicator.textContent=(index+1)+'/'+slides.length;slides[index].scrollIntoView({block:'start'})}previous.addEventListener('click',function(){show(index-1)});next.addEventListener('click',function(){show(index+1)});controls.append(previous,indicator,next);document.body.appendChild(controls);document.addEventListener('keydown',function(event){var target=event.target;if(target instanceof HTMLInputElement||target instanceof HTMLTextAreaElement||target instanceof HTMLButtonElement||target instanceof HTMLSelectElement||target&&target.tagName==='SUMMARY'||target&&target instanceof Element&&target.closest('[contenteditable]'))return;if(['ArrowRight','PageDown',' '].includes(event.key)){event.preventDefault();show(index+1)}else if(['ArrowLeft','PageUp'].includes(event.key)){event.preventDefault();show(index-1)}});show(0)}})();`; |
There was a problem hiding this comment.
When an export uses custom properties for ordinary styling but does not define theme palettes (for example sanitized :root{--fg:#111} body{color:var(--fg)} becomes a [data-he-content]{--fg:#111} rule), this check still marks it as authored just because the selector contains data-he-content and declares a --* property. The fallback inversion is then not injected, so the theme toggle only changes data-theme and dark mode has neither an authored palette nor the fallback; please require a matching theme-qualified rule such as [data-theme="dark"]/[data-theme="light"] before skipping the fallback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2806292279
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| function runtimeSource(mode: HtmlExportRuntimeMode): string { | ||
| const slide = mode === 'slide' ? 'true' : 'false'; | ||
| return `(function(){var root=document.documentElement,content=document.querySelector('[data-he-content]');if(root.dataset.naiRuntime)return;root.dataset.naiRuntime='true';var button=document.createElement('button'),style=document.createElement('style');button.id='nai-runtime-toggle';button.type='button';button.className='nai-theme-toggle';button.style.cssText='position:fixed;top:12px;right:12px;z-index:2147483647;border:0;border-radius:999px;padding:8px 10px;cursor:pointer;background:#111;color:#fff';function setTheme(theme){root.dataset.theme=theme;if(content)content.dataset.theme=theme;try{localStorage.setItem('nai-theme',theme)}catch(_e){}button.textContent=theme==='dark'?'☀':'🌙';button.setAttribute('aria-label',theme==='dark'?'Switch to light theme':'Switch to dark theme')}var saved;try{saved=localStorage.getItem('nai-theme')}catch(_e){}setTheme(saved==='light'||saved==='dark'?saved:(matchMedia&&matchMedia('(prefers-color-scheme: dark)').matches?'dark':'light'));button.addEventListener('click',function(){setTheme(root.dataset.theme==='dark'?'light':'dark')});document.body.appendChild(button);function hasAuthoredTheme(rule){if(rule.cssRules&&rule.cssRules.length)return Array.prototype.some.call(rule.cssRules,hasAuthoredTheme);if(!rule.selectorText||rule.selectorText.indexOf('data-he-content')===-1||!/\\[\\s*data-theme\\s*(?:[~|^$*]?=|\\])/i.test(rule.selectorText)||!Array.prototype.some.call(rule.style||[],function(name){return String(name).indexOf('--')===0}))return false;var theme=content.dataset.theme;var matches=['light','dark'].some(function(value){content.dataset.theme=value;return content.matches(rule.selectorText)});content.dataset.theme=theme;return matches}var authored=content&&Array.prototype.some.call(document.styleSheets,function(sheet){try{return Array.prototype.some.call(sheet.cssRules||[],hasAuthoredTheme)}catch(_e){return false}});if(!authored){style.id='nai-theme-fallback';style.textContent='[data-he-content][data-theme="dark"]{filter:invert(1) hue-rotate(180deg)}[data-he-content][data-theme="dark"] img,[data-he-content][data-theme="dark"] video{filter:invert(1) hue-rotate(180deg)}';document.head.appendChild(style)}if(${slide}){var slides=Array.prototype.slice.call(document.querySelectorAll('section.slide'));if(!slides.length){slides=Array.prototype.slice.call((content||document.body).children).filter(function(node){return node.tagName==='SECTION'})}if(!slides.length)return;var index=0,controls=document.createElement('div'),previous=document.createElement('button'),next=document.createElement('button'),indicator=document.createElement('span');controls.className='nai-slide-nav';controls.style.cssText='position:fixed;bottom:12px;right:12px;z-index:2147483647;display:flex;gap:8px;align-items:center;background:#111;color:#fff;padding:8px;border-radius:999px';previous.type=next.type='button';previous.textContent='‹';next.textContent='›';function show(n){index=(n+slides.length)%slides.length;slides.forEach(function(s,i){s.style.display=i===index?'':'none';s.style.minHeight='100vh'});indicator.textContent=(index+1)+'/'+slides.length;slides[index].scrollIntoView({block:'start'})}previous.addEventListener('click',function(){show(index-1)});next.addEventListener('click',function(){show(index+1)});controls.append(previous,indicator,next);document.body.appendChild(controls);document.addEventListener('keydown',function(event){var target=event.target;if(target instanceof HTMLInputElement||target instanceof HTMLTextAreaElement||target instanceof HTMLButtonElement||target instanceof HTMLSelectElement||target&&target.tagName==='SUMMARY'||target&&target instanceof Element&&target.closest('[contenteditable]'))return;if(['ArrowRight','PageDown',' '].includes(event.key)){event.preventDefault();show(index+1)}else if(['ArrowLeft','PageUp'].includes(event.key)){event.preventDefault();show(index-1)}});show(0)}})();`; |
There was a problem hiding this comment.
Fresh evidence in this revision is the unconditional recursion through rule.cssRules before setting authored. When a generated stylesheet only defines [data-theme] custom properties inside a non-matching grouping rule, such as @media print or (prefers-color-scheme: dark) while the user is in light mode, this scan still suppresses the fallback even though no active palette applies, so the theme toggle can leave the export without usable dark colors; please only count active media/supports rules before skipping the fallback.
Useful? React with 👍 / 👎.
|
|
||
| function runtimeSource(mode: HtmlExportRuntimeMode): string { | ||
| const slide = mode === 'slide' ? 'true' : 'false'; | ||
| return `(function(){var root=document.documentElement,content=document.querySelector('[data-he-content]');if(root.dataset.naiRuntime)return;root.dataset.naiRuntime='true';var button=document.createElement('button'),style=document.createElement('style');button.id='nai-runtime-toggle';button.type='button';button.className='nai-theme-toggle';button.style.cssText='position:fixed;top:12px;right:12px;z-index:2147483647;border:0;border-radius:999px;padding:8px 10px;cursor:pointer;background:#111;color:#fff';function setTheme(theme){root.dataset.theme=theme;if(content)content.dataset.theme=theme;try{localStorage.setItem('nai-theme',theme)}catch(_e){}button.textContent=theme==='dark'?'☀':'🌙';button.setAttribute('aria-label',theme==='dark'?'Switch to light theme':'Switch to dark theme')}var saved;try{saved=localStorage.getItem('nai-theme')}catch(_e){}setTheme(saved==='light'||saved==='dark'?saved:(matchMedia&&matchMedia('(prefers-color-scheme: dark)').matches?'dark':'light'));button.addEventListener('click',function(){setTheme(root.dataset.theme==='dark'?'light':'dark')});document.body.appendChild(button);function hasAuthoredTheme(rule){if(rule.cssRules&&rule.cssRules.length)return Array.prototype.some.call(rule.cssRules,hasAuthoredTheme);if(!rule.selectorText||rule.selectorText.indexOf('data-he-content')===-1||!/\\[\\s*data-theme\\s*(?:[~|^$*]?=|\\])/i.test(rule.selectorText)||!Array.prototype.some.call(rule.style||[],function(name){return String(name).indexOf('--')===0}))return false;var theme=content.dataset.theme;var matches=['light','dark'].some(function(value){content.dataset.theme=value;return content.matches(rule.selectorText)});content.dataset.theme=theme;return matches}var authored=content&&Array.prototype.some.call(document.styleSheets,function(sheet){try{return Array.prototype.some.call(sheet.cssRules||[],hasAuthoredTheme)}catch(_e){return false}});if(!authored){style.id='nai-theme-fallback';style.textContent='[data-he-content][data-theme="dark"]{filter:invert(1) hue-rotate(180deg)}[data-he-content][data-theme="dark"] img,[data-he-content][data-theme="dark"] video{filter:invert(1) hue-rotate(180deg)}';document.head.appendChild(style)}if(${slide}){var slides=Array.prototype.slice.call(document.querySelectorAll('section.slide'));if(!slides.length){slides=Array.prototype.slice.call((content||document.body).children).filter(function(node){return node.tagName==='SECTION'})}if(!slides.length)return;var index=0,controls=document.createElement('div'),previous=document.createElement('button'),next=document.createElement('button'),indicator=document.createElement('span');controls.className='nai-slide-nav';controls.style.cssText='position:fixed;bottom:12px;right:12px;z-index:2147483647;display:flex;gap:8px;align-items:center;background:#111;color:#fff;padding:8px;border-radius:999px';previous.type=next.type='button';previous.textContent='‹';next.textContent='›';function show(n){index=(n+slides.length)%slides.length;slides.forEach(function(s,i){s.style.display=i===index?'':'none';s.style.minHeight='100vh'});indicator.textContent=(index+1)+'/'+slides.length;slides[index].scrollIntoView({block:'start'})}previous.addEventListener('click',function(){show(index-1)});next.addEventListener('click',function(){show(index+1)});controls.append(previous,indicator,next);document.body.appendChild(controls);document.addEventListener('keydown',function(event){var target=event.target;if(target instanceof HTMLInputElement||target instanceof HTMLTextAreaElement||target instanceof HTMLButtonElement||target instanceof HTMLSelectElement||target&&target.tagName==='SUMMARY'||target&&target instanceof Element&&target.closest('[contenteditable]'))return;if(['ArrowRight','PageDown',' '].includes(event.key)){event.preventDefault();show(index+1)}else if(['ArrowLeft','PageUp'].includes(event.key)){event.preventDefault();show(index-1)}});show(0)}})();`; |
There was a problem hiding this comment.
AGENTS.md requires user-facing strings to use the five-locale i18n layer rather than hardcoded copy. The new app-owned export toggle always injects English aria-label text, so users exporting from ko/zh/ja locales still get English runtime controls in the saved document; please pass localized labels into finalization/runtime injection or otherwise derive them from the selected locale.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 571455fc47
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!PROPERTY_SET.has(property) || lexer.matchProperty(property, value).error) continue; | ||
| output.push(`${property}:${value}`); | ||
| if ((!PROPERTY_SET.has(property) && !property.startsWith('--')) || (!property.startsWith('--') && lexer.matchProperty(property, value).error)) continue; | ||
| output.push(`${property}:${value}${declaration.important ? '!important' : ''}`); |
There was a problem hiding this comment.
When a generated theme uses a mixed-case custom property such as --AccentColor and references it with var(--AccentColor), this emitter writes the already lower-cased property name while leaving the var() reference unchanged. CSS custom property names are case-sensitive, so the sanitized output defines --accentcolor but references --AccentColor, causing the prompted theme/color declarations to fall back or compute invalid.
Useful? React with 👍 / 👎.
|
|
||
| function runtimeSource(mode: HtmlExportRuntimeMode, labels: HtmlExportRuntimeLabels): string { | ||
| const slide = mode === 'slide' ? 'true' : 'false'; | ||
| return `(function(){var root=document.documentElement,content=document.querySelector('[data-he-content]'),labels=${JSON.stringify(labels)};if(root.dataset.naiRuntime)return;root.dataset.naiRuntime='true';var button=document.createElement('button'),style=document.createElement('style');button.id='nai-runtime-toggle';button.type='button';button.className='nai-theme-toggle';button.style.cssText='position:fixed;top:12px;right:12px;z-index:2147483647;border:0;border-radius:999px;padding:8px 10px;cursor:pointer;background:#111;color:#fff';function setTheme(theme){root.dataset.theme=theme;if(content)content.dataset.theme=theme;try{localStorage.setItem('nai-theme',theme)}catch(_e){}button.textContent=theme==='dark'?'☀':'🌙';var label=theme==='dark'?labels.switchToLightTheme:labels.switchToDarkTheme;button.setAttribute('aria-label',label);button.title=label}var saved;try{saved=localStorage.getItem('nai-theme')}catch(_e){}setTheme(saved==='light'||saved==='dark'?saved:(matchMedia&&matchMedia('(prefers-color-scheme: dark)').matches?'dark':'light'));button.addEventListener('click',function(){setTheme(root.dataset.theme==='dark'?'light':'dark')});document.body.appendChild(button);function groupingRuleMatches(rule){try{if((typeof CSSMediaRule!=='undefined'&&rule instanceof CSSMediaRule)||rule.type===4)return window.matchMedia(rule.conditionText).matches;if((typeof CSSSupportsRule!=='undefined'&&rule instanceof CSSSupportsRule)||rule.type===12)return CSS.supports(rule.conditionText)}catch(_e){}return true}function hasAuthoredTheme(rule){if(rule.cssRules&&rule.cssRules.length){if(!groupingRuleMatches(rule))return false;return Array.prototype.some.call(rule.cssRules,hasAuthoredTheme)}if(!rule.selectorText||rule.selectorText.indexOf('data-he-content')===-1||!/\\[\\s*data-theme\\s*(?:[~|^$*]?=|\\])/i.test(rule.selectorText)||!Array.prototype.some.call(rule.style||[],function(name){return String(name).indexOf('--')===0}))return false;var theme=content.dataset.theme;var matches=['light','dark'].some(function(value){content.dataset.theme=value;return content.matches(rule.selectorText)});content.dataset.theme=theme;return matches}var authored=content&&Array.prototype.some.call(document.styleSheets,function(sheet){try{return Array.prototype.some.call(sheet.cssRules||[],hasAuthoredTheme)}catch(_e){return false}});if(!authored){style.id='nai-theme-fallback';style.textContent='[data-he-content][data-theme="dark"]{filter:invert(1) hue-rotate(180deg)}[data-he-content][data-theme="dark"] img,[data-he-content][data-theme="dark"] video{filter:invert(1) hue-rotate(180deg)}';document.head.appendChild(style)}if(${slide}){var slides=Array.prototype.slice.call(document.querySelectorAll('section.slide'));if(!slides.length){slides=Array.prototype.slice.call((content||document.body).children).filter(function(node){return node.tagName==='SECTION'})}if(!slides.length)return;var index=0,controls=document.createElement('div'),previous=document.createElement('button'),next=document.createElement('button'),indicator=document.createElement('span');controls.className='nai-slide-nav';controls.style.cssText='position:fixed;bottom:12px;right:12px;z-index:2147483647;display:flex;gap:8px;align-items:center;background:#111;color:#fff;padding:8px;border-radius:999px';previous.type=next.type='button';previous.textContent='‹';next.textContent='›';previous.setAttribute('aria-label',labels.previousSlide);previous.title=labels.previousSlide;next.setAttribute('aria-label',labels.nextSlide);next.title=labels.nextSlide;function show(n){index=(n+slides.length)%slides.length;slides.forEach(function(s,i){s.style.display=i===index?'':'none';s.style.minHeight='100vh'});indicator.textContent=(index+1)+'/'+slides.length;var indicatorLabel=labels.slideIndicator.replace('{current}',String(index+1)).replace('{total}',String(slides.length));indicator.setAttribute('aria-label',indicatorLabel);indicator.title=indicatorLabel;slides[index].scrollIntoView({block:'start'})}previous.addEventListener('click',function(){show(index-1)});next.addEventListener('click',function(){show(index+1)});controls.append(previous,indicator,next);document.body.appendChild(controls);document.addEventListener('keydown',function(event){var target=event.target;if(target instanceof HTMLInputElement||target instanceof HTMLTextAreaElement||target instanceof HTMLButtonElement||target instanceof HTMLSelectElement||target&&target.tagName==='SUMMARY'||target&&target instanceof Element&&target.closest('[contenteditable]'))return;if(['ArrowRight','PageDown',' '].includes(event.key)){event.preventDefault();show(index+1)}else if(['ArrowLeft','PageUp'].includes(event.key)){event.preventDefault();show(index-1)}});show(0)}})();`; |
There was a problem hiding this comment.
In slide-mode exports where a generated slide contains a nested <section class="slide">, this descendant-wide query adds both the parent and the child to the deck. When navigation reaches the nested child, show() hides the parent slide, leaving the selected child inside a display:none ancestor and producing a blank page; build the deck from direct content-root slide children instead of every descendant.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f13be82ba1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| webContentsId, | ||
| attemptId, | ||
| resolvedArtifactId, | ||
| opts?.mode, |
There was a problem hiding this comment.
When opts.mode is slide and generated CSS lays <section class="slide"> panels in a horizontal track, this mode-specific runtime is not injected until after the quarantine block above has measured the resolved shell with every slide still visible. I checked the live main callback: main.ts rejects measurement.horizontalOverflow as layout-violation, so these slide exports fail generation even though the finalized runtime would hide inactive slides before the user opens the file. Finalize or otherwise emulate the slide runtime before quarantine measurement for slide mode.
Useful? React with 👍 / 👎.
project820
left a comment
project820
left a comment
There was a problem hiding this comment.
🤖 Psycho-842 [BOT] review
⚖️ Verdict: NEEDS REVISION — CI is green, but Korean/Chinese/Japanese exports still finalize with English runtime labels, and slide layout is rejected before its runtime can make the deck non-overflowing.
Summary
This PR adds the interactive HTML-export runtime, CSP confinement, theme controls, slide navigation, and related authoring contracts for #70.
The latest head fixes the previously reported custom-property casing and nested-slide defects.
Two P2 paths remain in the live generation flow: locale is dropped before finalization, and the quarantine gate measures slide output before its mode-specific runtime exists.
Findings (by severity)
- 🟠 P2 Med —
src/main/html-export-generate.ts:32-40,113dropslocalefrom the live generation path. The renderer supplies it tohtml:generateand the IPC forwards it, butHtmlGenerateInputhas no locale field andorchestrator.run(...)receives onlysignalandmode. A direct execution withlocale: 'ko'reachedfinalizeasmode='slide', locale=undefined, selecting the default English labels. Preserve and forward the validated locale, then cover the renderer → IPC → generator → finalizer path. - 🟠 P2 Med —
src/main/html-export-generation-orchestrator.ts:280-320invokes quarantine beforefinalize;src/main/main.ts:130-142rejectshorizontalOverflowat that point. Slide mode only hides non-active slides in the runtime injected by finalization, so a horizontal slide track can be rejected while its finalized artifact would render as a single-slide deck. A probe forcing the layout gate returnedfailedatquarantinewith zero finalizer calls. Measure a mode-correct transient artifact (or otherwise emulate the slide runtime) before enforcing the overflow verdict.
Response to bot findings
- The content-root theme attribute, deferred injection, and
nai-*reservation findings are resolved in the current runtime/sanitizer paths and their focused tests. - Layer/media theme traversal, functional theme selectors, theme-qualified fallback detection, relocated head-script order, and theme-root sibling containment are present in the current diff and covered by focused tests.
- The custom-property casing and nested-slide findings are resolved by
f13be82; the finalized-artifact DOM tests cover both regressions. - Script-tag and inline-handler retention for static exports remain the owner's documented personal-use policy in the thread, tracked as an optional preference in #66; they are not folded into this code verdict.
- Runtime localization remains unresolved in the live path and is included above.
- The new slide/quarantine ordering finding remains unresolved and is included above.
📋 Detailed analysis (measured CI, diff, and validation)
- Settlement head:
f13be82ba11e0988c6186f4c7362e78bc18942d9; +741/−253 across 25 files;mergeable=MERGEABLE,mergeStateStatus=CLEAN. - GitHub HEAD CI:
verifysucceeded on this exact SHA. Its measured steps includenpm ci, preflight, typecheck, 2,366-test Vitest suite, build, security/converter/direct/roundtrip harnesses, and knip. - Local validation on this SHA passed:
npm ci,npm run typecheck,npm run test,npm run build,npm run test:security-e2e,npm run test:converter-e2e,npm run test:html-export-direct,npm run test:roundtrip-smoke, andnpm run knip. - Direct runtime probe after the top-level-slide fix: nested slides produced
1/2; after navigating away and back, both parent and nested slide elements remained visible. - Caller trace:
unified-chat-wiring.tssupplies locale,html-export-ipc.tsvalidates/forwards it,main.tscallshtmlExportGenerator.run, andhtml-export-generate.tsdrops it before the orchestrator. The pipeline finalizer accepts locale but defaults to English. - Adversarial pass — 4 angles tried: async/order (finalization happens after quarantine, and the ordering refutation holds), localization contract (live locale forwarding is absent), nested slide containment (rejected by the new regression test), and custom-property casing (rejected by the new finalized-artifact regression test).
- Related work: this PR closes #70. No other open PR touches its changed files. No revert history was found for the core export files.
This is a COMMENT review, not an approval; explicit user final sign-off is still required.
🛠️ NEEDS REVISION
🦀 Reviewed by Hermes agent BOT 🦞
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 764c59ea7b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| try { | ||
| return await orchestrator.run(webContentsId, input.prompt, { signal: controller.signal }); | ||
| return await orchestrator.run(webContentsId, input.prompt, { signal: controller.signal, mode: input.mode }); |
There was a problem hiding this comment.
Fresh evidence in this revision is that the renderer and IPC now send/validate locale, but the live main-owned generator still omits it when calling orchestrator.run. In non-English app locales, exports therefore finalize with the default English runtime labels even though localized labels were added and the request carried the selected locale; pass input.locale through here so resolve/finalize can inject the correct labels.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in 6db5e9b (pushed after this comment was filed): HtmlGenerateInput now carries the validated locale and passes it into orchestrator.run options; IPC→generator→finalizer covered by tests asserting Korean labels with locale 'ko' and English default when absent. CI green at HEAD.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6db5e9b5cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!PROPERTY_SET.has(property) || lexer.matchProperty(property, value).error) continue; | ||
| output.push(`${property}:${value}`); | ||
| if ((!PROPERTY_SET.has(property) && !property.startsWith('--')) || (!property.startsWith('--') && lexer.matchProperty(property, value).error)) continue; | ||
| output.push(`${declaredProperty.startsWith('--') ? declaredProperty : property}:${value}${declaration.important ? '!important' : ''}`); |
There was a problem hiding this comment.
When slide exports include common author CSS such as section.slide{display:flex!important}, preserving !important here lets the stylesheet beat the runtime's later normal-priority s.style.display='none' assignment used to hide inactive slides. Those slides remain visible, so deck navigation can show multiple slides and trigger overflow/quarantine failures; either strip !important again or make the runtime hide/show declarations use important priority.
Useful? React with 👍 / 👎.
| 'ul', 'ol', 'li', 'dl', 'dt', 'dd', 'blockquote', 'figure', 'figcaption', 'img', 'picture', | ||
| 'source', 'svg', 'table', 'thead', 'tbody', 'tfoot', 'tr', 'th', 'td', 'caption', 'code', | ||
| 'pre', 'kbd', 'samp', 'abbr', 'time', 'a', | ||
| 'pre', 'kbd', 'samp', 'abbr', 'time', 'a', 'form', 'input', 'button', |
There was a problem hiding this comment.
When the newly enabled interactive controls are styled with standard selectors like input[type="range"] or input[type="checkbox"], the selector validator still rejects type because SAFE_ATTRIBUTE_SELECTOR_NAMES omits it even though the HTML sanitizer preserves type on inputs/buttons/scripts. The sanitizer then drops the whole CSS rule, so generated controls can render unstyled or incorrectly; add the supported control attributes to the selector allowlist along with these new element selectors.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c92d9cc5e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| function runtimeSource(mode: HtmlExportRuntimeMode, labels: HtmlExportRuntimeLabels): string { | ||
| const slide = mode === 'slide' ? 'true' : 'false'; | ||
| return `(function(){var root=document.documentElement,content=document.querySelector('[data-he-content]'),labels=${JSON.stringify(labels)};if(root.dataset.naiRuntime)return;root.dataset.naiRuntime='true';var button=document.createElement('button'),style=document.createElement('style');button.id='nai-runtime-toggle';button.type='button';button.className='nai-theme-toggle';button.style.cssText='position:fixed;top:12px;right:12px;z-index:2147483647;border:0;border-radius:999px;padding:8px 10px;cursor:pointer;background:#111;color:#fff';function setTheme(theme){root.dataset.theme=theme;if(content)content.dataset.theme=theme;try{localStorage.setItem('nai-theme',theme)}catch(_e){}button.textContent=theme==='dark'?'☀':'🌙';var label=theme==='dark'?labels.switchToLightTheme:labels.switchToDarkTheme;button.setAttribute('aria-label',label);button.title=label}var saved;try{saved=localStorage.getItem('nai-theme')}catch(_e){}setTheme(saved==='light'||saved==='dark'?saved:(matchMedia&&matchMedia('(prefers-color-scheme: dark)').matches?'dark':'light'));button.addEventListener('click',function(){setTheme(root.dataset.theme==='dark'?'light':'dark')});document.body.appendChild(button);function groupingRuleMatches(rule){try{if((typeof CSSMediaRule!='undefined'&&rule instanceof CSSMediaRule)||rule.type===4)return window.matchMedia(rule.conditionText).matches;if((typeof CSSSupportsRule!='undefined'&&rule instanceof CSSSupportsRule)||rule.type===12)return CSS.supports(rule.conditionText)}catch(_e){}return true}function hasAuthoredTheme(rule){if(rule.cssRules&&rule.cssRules.length){if(!groupingRuleMatches(rule))return false;return Array.prototype.some.call(rule.cssRules,hasAuthoredTheme)}if(!rule.selectorText||rule.selectorText.indexOf('data-he-content')===-1||!/\\[\\s*data-theme\\s*(?:[~|^$*]?=|\\])/i.test(rule.selectorText)||!Array.prototype.some.call(rule.style||[],function(name){return String(name).indexOf('--')===0}))return false;var theme=content.dataset.theme;var matches=['light','dark'].some(function(value){content.dataset.theme=value;return content.matches(rule.selectorText)});content.dataset.theme=theme;return matches}var authored=content&&Array.prototype.some.call(document.styleSheets,function(sheet){try{return Array.prototype.some.call(sheet.cssRules||[],hasAuthoredTheme)}catch(_e){return false}});if(!authored){style.id='nai-theme-fallback';style.textContent='[data-he-content][data-theme="dark"]{filter:invert(1) hue-rotate(180deg)}[data-he-content][data-theme="dark"] img,[data-he-content][data-theme="dark"] video{filter:invert(1) hue-rotate(180deg)}';document.head.appendChild(style)}if(${slide}){var deck=content||document.body,slides=Array.prototype.slice.call(deck.querySelectorAll('section.slide')).filter(function(candidate){var ancestor=candidate.parentElement&&candidate.parentElement.closest('section.slide');return !ancestor||!deck.contains(ancestor)});if(!slides.length){slides=Array.prototype.slice.call((content||document.body).children).filter(function(node){return node.tagName==='SECTION'})}if(!slides.length)return;var index=0,controls=document.createElement('div'),previous=document.createElement('button'),next=document.createElement('button'),indicator=document.createElement('span');controls.className='nai-slide-nav';controls.style.cssText='position:fixed;bottom:12px;right:12px;z-index:2147483647;display:flex;gap:8px;align-items:center;background:#111;color:#fff;padding:8px;border-radius:999px';previous.type=next.type='button';previous.textContent='‹';next.textContent='›';previous.setAttribute('aria-label',labels.previousSlide);previous.title=labels.previousSlide;next.setAttribute('aria-label',labels.nextSlide);next.title=labels.nextSlide;function show(n){index=(n+slides.length)%slides.length;slides.forEach(function(s,i){if(i===index)s.style.removeProperty('display');else s.style.setProperty('display','none','important');s.style.minHeight='100vh'});indicator.textContent=(index+1)+'/'+slides.length;var indicatorLabel=labels.slideIndicator.replace('{current}',String(index+1)).replace('{total}',String(slides.length));indicator.setAttribute('aria-label',indicatorLabel);indicator.title=indicatorLabel;slides[index].scrollIntoView({block:'start'})}previous.addEventListener('click',function(){show(index-1)});next.addEventListener('click',function(){show(index+1)});controls.append(previous,indicator,next);document.body.appendChild(controls);document.addEventListener('keydown',function(event){var target=event.target;if(target instanceof HTMLInputElement||target instanceof HTMLTextAreaElement||target instanceof HTMLButtonElement||target instanceof HTMLSelectElement||target&&target.tagName==='SUMMARY'||target&&target instanceof Element&&target.closest('[contenteditable]'))return;if(['ArrowRight','PageDown',' '].includes(event.key)){event.preventDefault();show(index+1)}else if(['ArrowLeft','PageUp'].includes(event.key)){event.preventDefault();show(index-1)}});show(0)}})();`; |
There was a problem hiding this comment.
When an export defines only a light theme custom-property rule, such as [data-theme="light"]{--fg:#111}, this detection still returns authored because it tests ['light','dark'].some(...), so the dark-mode fallback is not injected. Toggling the app control to dark then sets data-theme="dark" with neither an authored dark palette nor the fallback, making var()-based colors compute incorrectly; only suppress the fallback after finding a matching dark palette or both required palettes.
Useful? React with 👍 / 👎.
| if (name === 'href') return tag === 'a'; | ||
| if (name === 'src') return ['img', 'source'].includes(tag); | ||
| if (name === 'type') return ['input', 'button', 'script'].includes(tag); | ||
| if (name === 'value' || name === 'name' || name === 'placeholder' || name === 'checked' || name === 'disabled') return ['input', 'button'].includes(tag); |
There was a problem hiding this comment.
When generated interactive output uses a range/number control such as <input type="range" min="1" max="5" step="1" value="3">, this allowlist keeps type/value but strips min, max, and step as disallowed attributes. The saved control silently falls back to the browser's 0–100/step=1 defaults, so JS and UI no longer match the authored document; add validated input bounds to the allowlist for these newly supported controls.
Useful? React with 👍 / 👎.
project820
left a comment
project820
left a comment
There was a problem hiding this comment.
🤖 Psycho-842 [BOT] review
⚖️ Verdict: NEEDS REVISION — two new P2 regressions remain at the settlement head despite green tip CI.
Summary
This Stage 2 re-measurement covers c92d9cc5, including the latest slide-display and input-selector fixes.
The prior mode, locale, grouping-rule, and slide-visibility fixes are present, but a light-only palette disables the dark fallback and range controls lose their declared bounds.
Findings (by severity)
- 🟠 P2 Med —
src/main/html-export-runtime.ts:11treats a palette for eitherlightordarkas sufficient (['light','dark'].some(...)). With only[data-theme="light"]{--fg:...}, it suppresses#nai-theme-fallback; after the user switches to dark, there is neither an authored dark palette nor fallback inversion. Require a matching dark palette before suppressing the dark fallback, and add a light-only regression. - 🟠 P2 Med —
src/main/html-export-sanitize.ts:372permitstypeandvalueon interactive inputs but stripsmin,max, andstep. A sanitized<input type="range" min="1" max="5" step="0.5" value="3">becomes<input type="range" value="3">, silently changing the control's range. Preserve and validate those numeric input bounds.
Response to bot findings
- Codex's current-HEAD dark-palette and range-bound findings were independently reproduced against
c92d9cc5; both are incorporated above as P2 findings.
📋 Detailed analysis (measured CI, diff, and validation)
- Settlement head:
c92d9cc5e6da7ebb875c58caaa421785d81276a9; +922/−256 across 27 files;mergeable=MERGEABLE,mergeStateStatus=CLEAN. - GitHub HEAD check:
verifysucceeded on this exact SHA. - Local validation:
npm run typecheckpassed. The focused HTML-export suites passed (4 files, 279 tests). The full Vitest runner reported 149 files / 2,382 tests passed but the enclosingnpm testcommand returned exit 1 after the report, so it is not treated as a clean local gate. - Direct reproductions: a light-only authored palette left
#nai-theme-fallbackabsent after toggling dark; sanitizer output for the range input was exactly<input type="range" value="3">. - Files opened for context:
html-export-runtime.ts,html-export-sanitize.ts,html-export-css-sanitize.ts, runtime DOM tests, sanitizer tests, and pipeline IPC tests. - Caller trace:
injectHtmlExportRuntimeis the sole runtime injector;HtmlExportPipelineService.sanitizecallssanitizeHtmlExportbefore the interactive artifact is finalized. - Adversarial pass: mode/ordering and slide
!importanthandling are covered by the current code and focused tests; the light-only theme and range-contract attacks both hold. - Related work: this PR closes #70. No other open PR was measured touching these files; no known revert history was found.
This is a COMMENT review, not an approval; explicit user final sign-off is still required.
🛠️ NEEDS REVISION
🦀 Reviewed by Hermes agent BOT 🦞
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f807d981d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return nodes.length === 2 | ||
| && isGlobalRootAtom(nodes[0]) | ||
| && isThemeAttributeAtom(nodes[1]) | ||
| && (nodes[0].type !== 'TypeSelector' || String(nodes[0].name).toLowerCase() === 'html'); |
There was a problem hiding this comment.
When generated CSS uses the common pattern :where(body[data-theme="dark"]) or :is(body[data-theme="dark"]), this helper does not treat it as a theme-root selector even though non-functional body[data-theme] is otherwise a supported global-root rewrite. validateGlobalRootShape then sees the nested body root and strips the whole palette rule, so the prompted dark/light custom properties can disappear from the export; allow body here so it rewrites to the content root like html and :root.
Useful? React with 👍 / 👎.
| const SAFE_INPUT_BOUND = /^(?:[+-]?(?:\d+(?:\.\d*)?|\.\d+)(?:e[+-]?\d+)?|\d{4}(?:-\d{2}(?:-\d{2}(?:T\d{2}:\d{2}(?::\d{2}(?:\.\d+)?)?(?:Z|[+-]\d{2}:?\d{2})?)?)?)?|\d{2}:\d{2}(?::\d{2}(?:\.\d+)?)?)$/i; | ||
|
|
||
| function isSafeInputBound(value: string): boolean { | ||
| return SAFE_INPUT_BOUND.test(value) && (!/^[+-]?(?:\d+(?:\.\d*)?|\.\d+)(?:e[+-]?\d+)?$/i.test(value) || Number.isFinite(Number(value))); |
There was a problem hiding this comment.
The new bounds validator rejects the standard step="any" value because only numeric/date/time-looking tokens match this regex. For generated number/range inputs that intentionally allow arbitrary decimals, the sanitizer strips step, so the saved control falls back to the browser's default stepping and no longer matches the authored UI behavior; include the any keyword for step while keeping the existing checks for min/max.
Useful? React with 👍 / 👎.
project820
left a comment
project820
left a comment
There was a problem hiding this comment.
🤖 Psycho-842 [BOT] review
⚖️ Verdict: NEEDS REVISION — tip CI is green, but two newly surfaced interactive-export contracts still drop valid author intent.
Summary
This Stage 2 settlement re-measured 4f807d9 after the prior theme-fallback and input-bound fixes.
The required GitHub check, mergeability, full local suite, build, and export harness are green.
Two P2 edge cases remain: functional body[data-theme] palettes are stripped, and valid step="any" is removed from range/number controls.
Findings (by severity)
- 🟠 P2 Med —
src/main/html-export-css-sanitize.ts:292-298recognizesbody[data-theme="dark"]as a global theme root when written directly, but rejects the equivalent:where(body[data-theme="dark"])and:is(body[data-theme="dark"]). The measured sanitizer result iscss_disallowed_selectorwith empty CSS, so a valid functional palette disappears from the finalized export. Treatbodyconsistently withhtml/:rootinside functional theme-root selectors and add a finalized-artifact regression test. - 🟠 P2 Med —
src/main/html-export-sanitize.ts:362-365,446accepts numeric bounds but strips the standard HTML valuestep="any". A measured sanitized range input retainedmin="1" max="5"but loststep="any", reverting the control to browser-default stepping and changing its authored interaction. Acceptanyforsteponly, while retaining the current validation forminandmax.
Response to bot findings
- Codex's current-head findings for functional body-qualified theme roots and arbitrary-step controls were independently reproduced and are incorporated above.
- The prior light-only-palette and numeric input-bound findings are resolved by
4f807d9and their regression tests pass.
📋 Detailed analysis (measured CI, diff, and validation)
- Settlement head:
4f807d981d7d0c470fe9d8033b18979e1d942255; +957/−257 across 27 files;mergeable=MERGEABLE,mergeStateStatus=CLEAN. - GitHub HEAD CI: required
verifysucceeded on this exact SHA. - Local validation passed:
npm ci;npm run typecheck;npm test(149 files / 2,385 tests);npm run build;npm run test:security-e2e;npm run test:converter-e2e;npm run test:html-export-direct; andnpm run test:roundtrip-smoke. - The direct harness passed for scroll/slides × landscape/portrait, with zero remote requests and preview/save digest equality.
npm run knippassed.npm audit --omit=dev --audit-level=highstill reports four high-severity transitiveadm-zipadvisories under existingkordoc; this PR does not change package manifests or the lockfile.- Files opened for context:
html-export-css-sanitize.ts,html-export-sanitize.ts,html-export-runtime.ts, their CSS/sanitizer/runtime tests, andAGENTS.md. - Caller trace:
sanitizeHtmlExportinvokessanitizeStylesheetfor authored CSS and sanitizes input attributes beforeHtmlExportPipelineServicefinalizes the document.injectHtmlExportRuntimethen consumes the retained theme CSS and control markup. - Adversarial pass — 4 angles tried: slide runtime ordering, locale propagation, light-only fallback behavior, and interactive-control contract boundaries. The first three are covered by current code/tests; the functional-root and
step="any"attacks hold by direct execution. - Related work: this PR closes #70. No other open PR touches its changed files; no related revert was found.
This is a COMMENT review, not an approval; explicit user final sign-off is still required.
🛠️ NEEDS REVISION
🦀 Reviewed by Hermes agent BOT 🦞
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d550423566
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| function runtimeSource(mode: HtmlExportRuntimeMode, labels: HtmlExportRuntimeLabels): string { | ||
| const slide = mode === 'slide' ? 'true' : 'false'; | ||
| return `(function(){var root=document.documentElement,content=document.querySelector('[data-he-content]'),labels=${JSON.stringify(labels)};if(root.dataset.naiRuntime)return;root.dataset.naiRuntime='true';var button=document.createElement('button'),style=document.createElement('style');button.id='nai-runtime-toggle';button.type='button';button.className='nai-theme-toggle';button.style.cssText='position:fixed;top:12px;right:12px;z-index:2147483647;border:0;border-radius:999px;padding:8px 10px;cursor:pointer;background:#111;color:#fff';function setTheme(theme){root.dataset.theme=theme;if(content)content.dataset.theme=theme;try{localStorage.setItem('nai-theme',theme)}catch(_e){}button.textContent=theme==='dark'?'☀':'🌙';var label=theme==='dark'?labels.switchToLightTheme:labels.switchToDarkTheme;button.setAttribute('aria-label',label);button.title=label}var saved;try{saved=localStorage.getItem('nai-theme')}catch(_e){}setTheme(saved==='light'||saved==='dark'?saved:(matchMedia&&matchMedia('(prefers-color-scheme: dark)').matches?'dark':'light'));button.addEventListener('click',function(){setTheme(root.dataset.theme==='dark'?'light':'dark')});document.body.appendChild(button);function groupingRuleMatches(rule){try{if((typeof CSSMediaRule!='undefined'&&rule instanceof CSSMediaRule)||rule.type===4)return window.matchMedia(rule.conditionText).matches;if((typeof CSSSupportsRule!='undefined'&&rule instanceof CSSSupportsRule)||rule.type===12)return CSS.supports(rule.conditionText)}catch(_e){}return true}function hasAuthoredTheme(rule,themeValue){if(rule.cssRules&&rule.cssRules.length){if(!groupingRuleMatches(rule))return false;return Array.prototype.some.call(rule.cssRules,function(child){return hasAuthoredTheme(child,themeValue)})}if(!rule.selectorText||rule.selectorText.indexOf('data-he-content')===-1||rule.selectorText.toLowerCase().indexOf('data-theme')===-1||!Array.prototype.some.call(rule.style||[],function(name){return String(name).indexOf('--')===0}))return false;var selector=rule.selectorText.toLowerCase();if(selector.indexOf('=\"'+themeValue+'\"')===-1&&selector.indexOf("='"+themeValue+"'")===-1&&selector.indexOf('='+themeValue)===-1&&selector.indexOf('data-theme]')===-1)return false;var theme=content.dataset.theme;content.dataset.theme=themeValue;var matches=content.matches(rule.selectorText);content.dataset.theme=theme;return matches}var missingThemes=content?['light','dark'].filter(function(theme){return !Array.prototype.some.call(document.styleSheets,function(sheet){if(sheet.ownerNode&&(!sheet.ownerNode.isConnected||sheet.ownerNode.id==='nai-theme-fallback'))return false;try{return Array.prototype.some.call(sheet.cssRules||[],function(rule){return hasAuthoredTheme(rule,theme)})}catch(_e){return false}})}):['light','dark'];if(missingThemes.indexOf('dark')!==-1){style.id='nai-theme-fallback';style.textContent='[data-he-content][data-theme="dark"]{filter:invert(1) hue-rotate(180deg)}[data-he-content][data-theme="dark"] img,[data-he-content][data-theme="dark"] video{filter:invert(1) hue-rotate(180deg)}';document.head.appendChild(style)}if(${slide}){var deck=content||document.body,slides=Array.prototype.slice.call(deck.querySelectorAll('section.slide')).filter(function(candidate){var ancestor=candidate.parentElement&&candidate.parentElement.closest('section.slide');return !ancestor||!deck.contains(ancestor)});if(!slides.length){slides=Array.prototype.slice.call((content||document.body).children).filter(function(node){return node.tagName==='SECTION'})}if(!slides.length)return;var index=0,controls=document.createElement('div'),previous=document.createElement('button'),next=document.createElement('button'),indicator=document.createElement('span');controls.className='nai-slide-nav';controls.style.cssText='position:fixed;bottom:12px;right:12px;z-index:2147483647;display:flex;gap:8px;align-items:center;background:#111;color:#fff;padding:8px;border-radius:999px';previous.type=next.type='button';previous.textContent='‹';next.textContent='›';previous.setAttribute('aria-label',labels.previousSlide);previous.title=labels.previousSlide;next.setAttribute('aria-label',labels.nextSlide);next.title=labels.nextSlide;function show(n){index=(n+slides.length)%slides.length;slides.forEach(function(s,i){if(i===index)s.style.removeProperty('display');else s.style.setProperty('display','none','important');s.style.minHeight='100vh'});indicator.textContent=(index+1)+'/'+slides.length;var indicatorLabel=labels.slideIndicator.replace('{current}',String(index+1)).replace('{total}',String(slides.length));indicator.setAttribute('aria-label',indicatorLabel);indicator.title=indicatorLabel;slides[index].scrollIntoView({block:'start'})}previous.addEventListener('click',function(){show(index-1)});next.addEventListener('click',function(){show(index+1)});controls.append(previous,indicator,next);document.body.appendChild(controls);document.addEventListener('keydown',function(event){var target=event.target;if(target instanceof HTMLInputElement||target instanceof HTMLTextAreaElement||target instanceof HTMLButtonElement||target instanceof HTMLSelectElement||target&&target.tagName==='SUMMARY'||target&&target instanceof Element&&target.closest('[contenteditable]'))return;if(['ArrowRight','PageDown',' '].includes(event.key)){event.preventDefault();show(index+1)}else if(['ArrowLeft','PageUp'].includes(event.key)){event.preventDefault();show(index-1)}});show(0)}})();`; |
There was a problem hiding this comment.
When a slide export is printed or saved as PDF after the runtime initializes, show(0) leaves every inactive slide with inline display:none!important, and there is no beforeprint/afterprint handling to reveal them. The printout therefore contains only the currently selected slide rather than the complete ordered deck; the injected navigation controls also remain visible. Restore all slides and hide app controls for print, then reapply the interactive state afterward.
Useful? React with 👍 / 👎.
| if (name === 'href') return tag === 'a'; | ||
| if (name === 'src') return ['img', 'source'].includes(tag); | ||
| if (name === 'type') return ['input', 'button', 'script'].includes(tag); | ||
| if (name === 'value' || name === 'name' || name === 'placeholder' || name === 'checked' || name === 'disabled') return ['input', 'button'].includes(tag); |
There was a problem hiding this comment.
When generated interactive HTML contains a standard <input required>, this allowlist strips required even though the same change explicitly permits the :required and :optional CSS pseudo-classes. The finalized form then accepts an empty value and any authored input:required styling never matches, silently changing both validation and presentation; retain the boolean required attribute for inputs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25c06d8927
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const script = `<script id="nai-runtime">${runtimeSource(mode, labels)}</script>`; | ||
| output = /<script\s+id=["']nai-runtime["'][^>]*>[\s\S]*?<\/script\s*>/i.test(output) | ||
| ? output.replace(/<script\s+id=["']nai-runtime["'][^>]*>[\s\S]*?<\/script\s*>/i, script) | ||
| : /<\/body\s*>/i.test(output) ? output.replace(/<\/body\s*>/i, `${script}</body>`) : `${output}${script}`; |
There was a problem hiding this comment.
When a preserved inline script contains the valid JavaScript string '</body>' (for example, an HTML template used by an interaction), this document-wide regex treats that raw-text content as the document closing tag and inserts the app runtime inside the authored script. The injected </script> then terminates the authored script early, corrupting its code and preventing the theme/slide runtime from executing normally; locate the actual body boundary structurally or otherwise exclude script raw-text content.
Useful? React with 👍 / 👎.
| 'li', 'dl', 'dt', 'dd', 'blockquote', 'figure', 'figcaption', 'img', 'picture', 'source', 'svg', | ||
| 'table', 'thead', 'tbody', 'tfoot', 'tr', 'th', 'td', 'caption', 'code', 'pre', 'kbd', 'samp', | ||
| 'abbr', 'time', 'a', | ||
| 'abbr', 'time', 'a', 'script', 'form', 'input', 'textarea', 'select', 'button', |
There was a problem hiding this comment.
When interactive output contains a normal <select><option value="a">A</option></select>, the newly allowed select survives but option is absent from this vocabulary, so sanitizeNode unwraps each option and drops its value/selected state. The saved select therefore contains only text nodes and has no selectable choices; preserve option (and its relevant attributes, plus optgroup if supported) alongside select.
Useful? React with 👍 / 👎.
| 'ul', 'ol', 'li', 'dl', 'dt', 'dd', 'blockquote', 'figure', 'figcaption', 'img', 'picture', | ||
| 'source', 'svg', 'table', 'thead', 'tbody', 'tfoot', 'tr', 'th', 'td', 'caption', 'code', | ||
| 'pre', 'kbd', 'samp', 'abbr', 'time', 'a', | ||
| 'pre', 'kbd', 'samp', 'abbr', 'time', 'a', 'form', 'input', 'button', |
There was a problem hiding this comment.
When generated CSS styles the newly preserved controls with type selectors such as textarea{...} or select{...}, selector validation rejects those names because this set only adds form, input, and button. Since one rejected selector drops the entire rule, finalized text areas and selects can lose their authored layout and styling even though the HTML sanitizer explicitly retains them; include the corresponding preserved control types here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07eebb8a96
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| function runtimeSource(mode: HtmlExportRuntimeMode, labels: HtmlExportRuntimeLabels): string { | ||
| const slide = mode === 'slide' ? 'true' : 'false'; | ||
| return `(function(){var root=document.documentElement,content=document.querySelector('[data-he-content]'),labels=${JSON.stringify(labels)};if(root.dataset.naiRuntime)return;root.dataset.naiRuntime='true';var button=document.createElement('button'),style=document.createElement('style'),printStyle=document.createElement('style');printStyle.id='nai-print-controls';printStyle.textContent='@media print{#nai-runtime-toggle,#nai-slide-nav{display:none!important}}';document.head.appendChild(printStyle);button.id='nai-runtime-toggle';button.type='button';button.className='nai-theme-toggle';button.style.cssText='position:fixed;top:12px;right:12px;z-index:2147483647;border:0;border-radius:999px;padding:8px 10px;cursor:pointer;background:#111;color:#fff';function setTheme(theme){root.dataset.theme=theme;if(content)content.dataset.theme=theme;try{localStorage.setItem('nai-theme',theme)}catch(_e){}button.textContent=theme==='dark'?'☀':'🌙';var label=theme==='dark'?labels.switchToLightTheme:labels.switchToDarkTheme;button.setAttribute('aria-label',label);button.title=label}var saved;try{saved=localStorage.getItem('nai-theme')}catch(_e){}setTheme(saved==='light'||saved==='dark'?saved:(matchMedia&&matchMedia('(prefers-color-scheme: dark)').matches?'dark':'light'));button.addEventListener('click',function(){setTheme(root.dataset.theme==='dark'?'light':'dark')});document.body.appendChild(button);function groupingRuleMatches(rule){try{if((typeof CSSMediaRule!='undefined'&&rule instanceof CSSMediaRule)||rule.type===4)return window.matchMedia(rule.conditionText).matches;if((typeof CSSSupportsRule!='undefined'&&rule instanceof CSSSupportsRule)||rule.type===12)return CSS.supports(rule.conditionText)}catch(_e){}return true}function hasAuthoredTheme(rule,themeValue){if(rule.cssRules&&rule.cssRules.length){if(!groupingRuleMatches(rule))return false;return Array.prototype.some.call(rule.cssRules,function(child){return hasAuthoredTheme(child,themeValue)})}if(!rule.selectorText||rule.selectorText.indexOf('data-he-content')===-1||rule.selectorText.toLowerCase().indexOf('data-theme')===-1||!Array.prototype.some.call(rule.style||[],function(name){return String(name).indexOf('--')===0}))return false;var selector=rule.selectorText.toLowerCase();if(selector.indexOf('=\"'+themeValue+'\"')===-1&&selector.indexOf("='"+themeValue+"'")===-1&&selector.indexOf('='+themeValue)===-1&&selector.indexOf('data-theme]')===-1)return false;var theme=content.dataset.theme;content.dataset.theme=themeValue;var matches=content.matches(rule.selectorText);content.dataset.theme=theme;return matches}var missingThemes=content?['light','dark'].filter(function(theme){return !Array.prototype.some.call(document.styleSheets,function(sheet){if(sheet.ownerNode&&(!sheet.ownerNode.isConnected||sheet.ownerNode.id==='nai-theme-fallback'))return false;try{return Array.prototype.some.call(sheet.cssRules||[],function(rule){return hasAuthoredTheme(rule,theme)})}catch(_e){return false}})}):['light','dark'];if(missingThemes.indexOf('dark')!==-1){style.id='nai-theme-fallback';style.textContent='[data-he-content][data-theme="dark"]{filter:invert(1) hue-rotate(180deg)}[data-he-content][data-theme="dark"] img,[data-he-content][data-theme="dark"] video{filter:invert(1) hue-rotate(180deg)}';document.head.appendChild(style)}if(${slide}){var deck=content||document.body,slides=Array.prototype.slice.call(deck.querySelectorAll('section.slide')).filter(function(candidate){var ancestor=candidate.parentElement&&candidate.parentElement.closest('section.slide');return !ancestor||!deck.contains(ancestor)});if(!slides.length){slides=Array.prototype.slice.call((content||document.body).children).filter(function(node){return node.tagName==='SECTION'})}if(!slides.length)return;var index=0,controls=document.createElement('div'),previous=document.createElement('button'),next=document.createElement('button'),indicator=document.createElement('span');controls.id='nai-slide-nav';controls.className='nai-slide-nav';controls.style.cssText='position:fixed;bottom:12px;right:12px;z-index:2147483647;display:flex;gap:8px;align-items:center;background:#111;color:#fff;padding:8px;border-radius:999px';previous.type=next.type='button';previous.textContent='‹';next.textContent='›';previous.setAttribute('aria-label',labels.previousSlide);previous.title=labels.previousSlide;next.setAttribute('aria-label',labels.nextSlide);next.title=labels.nextSlide;function show(n){index=(n+slides.length)%slides.length;slides.forEach(function(s,i){if(i===index)s.style.removeProperty('display');else s.style.setProperty('display','none','important');s.style.minHeight='100vh'});indicator.textContent=(index+1)+'/'+slides.length;var indicatorLabel=labels.slideIndicator.replace('{current}',String(index+1)).replace('{total}',String(slides.length));indicator.setAttribute('aria-label',indicatorLabel);indicator.title=indicatorLabel;slides[index].scrollIntoView({block:'start'})}previous.addEventListener('click',function(){show(index-1)});next.addEventListener('click',function(){show(index+1)});function showForPrint(){slides.forEach(function(s){s.style.removeProperty('display')})}window.addEventListener('beforeprint',showForPrint);window.addEventListener('afterprint',function(){show(index)});var printMedia=window.matchMedia&&window.matchMedia('print');if(printMedia){var printChange=function(event){if(event.matches)showForPrint();else show(index)};if(printMedia.addEventListener)printMedia.addEventListener('change',printChange);else if(printMedia.addListener)printMedia.addListener(printChange)}controls.append(previous,indicator,next);document.body.appendChild(controls);document.addEventListener('keydown',function(event){var target=event.target;if(target instanceof HTMLInputElement||target instanceof HTMLTextAreaElement||target instanceof HTMLButtonElement||target instanceof HTMLSelectElement||target&&target.tagName==='SUMMARY'||target&&target instanceof Element&&target.closest('[contenteditable]'))return;if(['ArrowRight','PageDown',' '].includes(event.key)){event.preventDefault();show(index+1)}else if(['ArrowLeft','PageUp'].includes(event.key)){event.preventDefault();show(index-1)}});show(0)}})();`; |
There was a problem hiding this comment.
When authored slide CSS uses the conventional .slide{display:none}.slide.active{display:block} pattern supported by the previous runtime, show() merely removes the selected slide's inline display property and never adds active. The stylesheet therefore continues hiding every slide, so quarantine can pass a zero-overflow document that opens as a blank deck; toggle active for the current slide or otherwise explicitly restore its visible display.
Useful? React with 👍 / 👎.
| '6. Use ONLY the supported HTML tag vocabulary (structural: section, article, main, aside, nav, header, footer, div, h1–h6, p, ul/ol/li, dl/dt/dd, figure/figcaption, blockquote, table/thead/tbody/tfoot/tr/th/td/caption, img/picture/source, svg; inline: span, strong/em/b/i/u/s, small, mark, sub/sup, code/pre/kbd/samp, abbr, time, a, br, hr). Attach classes, ids, and inline styles ONLY to these tags — unsupported tags are unwrapped and their attributes dropped, which orphans any CSS that targets them.', | ||
| '4. Author complete, self-contained HTML with inline CSS. Inline JavaScript is allowed only when interactivity is enabled; never use remote scripts, network APIs, external fonts, or external assets. IMAGES: an <img> src may ONLY be an app-issued opaque asset ID (src="asset:…") explicitly listed in this prompt; NEVER emit data: URIs, remote URLs, or invented images. When no asset ID is provided, author without <img> and express any decoration in CSS. Every visual choice — layout, spacing, color, type scale — is yours to encode in CSS, honoring the design authority above.', | ||
| '5. Define BOTH palettes with CSS custom properties under [data-theme="light"] and [data-theme="dark"]; route every authored color through var(). The app injects a theme toggle that switches data-theme. Global element selectors (html/body/:root/*) are allowed (scoped to the export content root) but prefer authoring styles against document content.', | ||
| '6. Use ONLY the supported HTML tag vocabulary (structural: section, article, main, aside, nav, header, footer, div, h1–h6, p, ul/ol/li, dl/dt/dd, figure/figcaption, blockquote, table/thead/tbody/tfoot/tr/th/td/caption, img/picture/source, svg, form/input/button; inline: span, strong/em/b/i/u/s, small, mark, sub/sup, code/pre/kbd/samp, abbr, time, a, br, hr, script). Attach classes, ids, data attributes, inline styles, and event attributes only to these tags — unsupported tags are unwrapped and their attributes dropped, which orphans any CSS that targets them.', |
There was a problem hiding this comment.
For requests that need dropdowns or multiline inputs, the normal generation prompt explicitly says to use only this vocabulary but lists only form/input/button, omitting the newly preserved textarea, select, option, optgroup, label, fieldset, legend, and related elements. A compliant model is therefore instructed not to use the controls this change enabled, preventing them from being exercised in generated exports; keep the prompt vocabulary aligned with the sanitizer allowlist.
Useful? React with 👍 / 👎.
| if (name === 'src') return ['img', 'source'].includes(tag); | ||
| if (name === 'type') return ['input', 'button', 'script'].includes(tag); | ||
| if (name === 'value') return ['input', 'button', 'option'].includes(tag); | ||
| if (name === 'name' || name === 'placeholder') return ['input', 'button'].includes(tag); |
There was a problem hiding this comment.
When an interactive form contains standard markup such as <label for="notes">, <textarea name="notes" placeholder="…">, or <select name="region">, this allowlist strips for and limits name/placeholder to inputs and buttons. The saved document then loses label activation, textarea placeholder text, and the field names used by FormData or authored JavaScript; allow these attributes on the corresponding newly supported controls.
Useful? React with 👍 / 👎.
project820
left a comment
project820
left a comment
There was a problem hiding this comment.
🤖 Psycho-842 [BOT] review
⚖️ Verdict: NEEDS REVISION — green checks are real, but three current interactive-export contracts still lose authored behavior.
Summary
This Stage 2 settlement re-measured 07eebb8 after 18 commits and the prior P2 fixes.
The runtime, sanitizer, and direct-authoring contract now agree on most of the new surface, but active-class slide decks, form metadata, and the prompt vocabulary still diverge.
These are product-behavior regressions, not security-policy findings.
Findings (by severity)
- 🟠 P2 Med —
src/main/html-export-runtime.ts:12— slideshow()clears the selected slide's inlinedisplaybut never restores the conventionalactiveclass. With authored.slide{display:none}.slide.active{display:block}, the measured finalized runtime leaves both slides computed asdisplay:none; the deck opens blank. Toggle the selected slide'sactiveclass (and preserve it through print restore), then add this CSS pattern to the DOM suite. - 🟠 P2 Med —
src/main/html-export-sanitize.ts:379-387— the newly retainedlabel,textarea, andselectelements still lose functional form metadata:foris never allowlisted, andname/placeholderremain limited toinput/button. A measured sanitized label/textarea/select document removedfor="notes", both field names, and the textarea placeholder. Retain the matching inert form attributes and add a sanitizer regression. - 🟠 P2 Med —
src/renderer/html-export-direct-prompt.ts:170— the prompt says “Use ONLY” its vocabulary but lists onlyform/input/button; it omits the newly supported textarea/select/option/optgroup/label/fieldset/legend controls. A compliant generator therefore cannot intentionally author much of this PR's expanded control surface. Keep the prompt and sanitizer vocabulary in sync.
Response to bot findings
- Content-root theme propagation; deferred injection;
nai-*reservation; grouped/media theme detection; functional theme roots; custom-property case; nested slides; mode/locale forwarding; quarantine ordering; important display handling; print restoration; bounds/step=any; options; and control selectors were checked against the current code and their named runtime/sanitizer/pipeline tests.npm testpassed all 149 files / 2,394 tests. - The current active-class, prompt-vocabulary, and form-metadata reports were independently reproduced and are folded into the three P2 findings above.
- The static-export script/inline-handler threads remain the owner's documented product policy and follow-up #66; they are not recast as code defects here.
- GitHub still marks the historical Codex threads unresolved. The current code resolves the items listed in the first bullet, but those threads have not been administratively closed.
📋 Detailed analysis (measured CI, diff, and validation)
- Settlement head:
07eebb8a96a2e3a28a4ee8a4652bab83b523c58e; +1,152/−257 across 28 files; 18 commits ahead ofmain;mergeable=MERGEABLE,mergeStateStatus=CLEAN. - GitHub HEAD CI: required
verifysucceeded on this exact SHA (completed 2026-07-19 03:37 UTC). - Local validation on this exact checkout:
npm ci,npm run typecheck,npm test(149 files / 2,394 tests),npm run build,npm run test:security-e2e,npm run test:converter-e2e,npm run test:html-export-direct,npm run test:roundtrip-smoke, andnpm run knipall exited 0. - Reproduction probe: an authored
.slide{display:none}.slide.active{display:block}deck remained['none','none']; label linkage, textarea/select names, and placeholder were stripped; the direct prompt omitted textarea/select/option. - Files opened:
html-export-runtime.ts,html-export-sanitize.ts,html-export-css-sanitize.ts,html-export-pipeline-service.ts,html-export-generation-orchestrator.ts,html-export-document-markers.ts,html-export-direct-prompt.ts, and their runtime/sanitizer/CSS tests. - Caller trace: the orchestrator resolves with
mode/localebefore quarantine,HtmlExportPipelineService.resolveinjects the mode-specific runtime for measurement, andfinalizereplaces that runtime for the saved artifact. The ordering refutation was rejected by code and tests. - Adversarial pass — 4 attack angles: mode/quarantine ordering and raw-script body boundaries were rejected by the current pipeline/runtime tests; active-class visibility, form semantic retention, and prompt-contract completeness each held by direct execution.
- Delta since the last Psycho-842 review (
4f807d9): the prior functional-body-theme andstep="any"findings are fixed; the three findings above are newly surfaced at this head. - Related work: this PR closes #70. No other open PR was found touching these files; no related revert was found.
This is a COMMENT review, not an approval; explicit user final sign-off is still required.
🛠️ NEEDS REVISION
🦀 Reviewed by Hermes agent BOT 🦞
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37ba7c616f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| mode: HtmlExportRuntimeMode = 'scroll', | ||
| labels: HtmlExportRuntimeLabels = htmlExportRuntimeLabels(), | ||
| ): string { | ||
| let output = html.replace(/<meta\s+http-equiv=["']Content-Security-Policy["'][^>]*>\s*/gi, ''); |
There was a problem hiding this comment.
Although runtime insertion now finds the body structurally, this document-wide replacement still matches raw text inside preserved scripts. If an interactive export contains a JavaScript/template string such as '<meta http-equiv="Content-Security-Policy" content="example">', finalization deletes that string content and silently changes the authored interaction; replace only the actual app-owned CSP element in the parsed document/head.
Useful? React with 👍 / 👎.
| const SAFE_FORM_METADATA_TOKEN = /^[A-Za-z0-9][A-Za-z0-9_-]*$/; | ||
| const UNSAFE_FORM_METADATA_TEXT = /^\s*(?:javascript|data|https?|mailto|tel):/i; | ||
|
|
||
| function isSafeFormMetadata(name: string, value: string): boolean { | ||
| return name === 'placeholder' | ||
| ? !UNSAFE_FORM_METADATA_TEXT.test(value) | ||
| : SAFE_FORM_METADATA_TOKEN.test(value); |
There was a problem hiding this comment.
Fresh evidence after adding form metadata support is that this new token gate still rejects valid and common field names such as items[] or user[email]. In interactive exports that construct FormData, sanitizeAttributes consequently removes those name attributes, so getAll('items[]') and similar authored logic silently receive no values; field names should not be restricted to identifier-like tokens.
Useful? React with 👍 / 👎.
project820
left a comment
project820
left a comment
There was a problem hiding this comment.
🤖 Psycho-842 [BOT] review
⚖️ Verdict: NEEDS REVISION — tip CI is green, but two newly reproduced P2 regressions still change saved interactive-export behavior.
Summary
This Stage 2 settlement re-measured 37ba7c6 after the previous active-slide, form-metadata, and prompt-vocabulary fixes.
The runtime and sanitizer now cover those prior paths, and the required CI plus local suite are green.
Two edge cases remain: finalization rewrites text inside authored scripts, and structured HTML form names are stripped.
Findings (by severity)
- 🟠 P2 Med —
src/main/html-export-runtime.ts:28removes every string matching a CSP<meta>tag from the serialized document, including raw text inside an allowed authored<script>. A direct finalization probe withconst csp='<meta http-equiv="Content-Security-Policy" content="example">'producedconst csp='', changing the authored interaction. Replace only the actual document-head CSP element structurally, and add a regression test with the string inside a script/template literal. - 🟠 P2 Med —
src/main/html-export-sanitize.ts:368-374,464accepts only identifier-likenamevalues. The measured sanitizer output forname="items[]"andname="user[email]"removes both names, so ordinaryFormDatalookups for repeated/nested fields fail. Preserve valid structured form names (with a narrow safety rule if needed) and cover bracket notation.
Response to bot findings
- Codex's current CSP-like-script-string and structured-form-name reports were independently reproduced at this head and are incorporated above as P2 findings.
- The open inline-handler thread records the owner's documented product policy: the interactive checkbox shapes the prompt, while the CSP blocks network access. This settlement does not reclassify that policy choice as a code defect.
- The remaining historical Codex threads are still administratively unresolved in GitHub; this review does not treat their open status alone as proof of a current code defect.
📋 Detailed analysis (measured CI, diff, and validation)
- Settlement head:
37ba7c616f8144ae86ef5e478d5887296796177f; +1,204/−257 across 28 files;mergeable=MERGEABLE,mergeStateStatus=CLEAN. - GitHub tip CI:
verifysucceeded on this exact SHA. - Local validation passed:
npm ci;npm run typecheck;npm test(149 files / 2,396 tests);npm run build;npm run test:html-export-direct; and seven focused HTML-export suites (353 tests). - The direct harness passed for scroll/slides × landscape/portrait, including zero remote requests and preview/save digest equality.
- Reproduction 1:
injectHtmlExportRuntime(...)changed an authored script's CSP-like string to an empty string (preserved=false). - Reproduction 2:
sanitizeHtmlExport(...)returned<form><input value="a"><input value="x"></form>for inputs nameditems[]anduser[email], with twohtml_attributedispositions. - Files opened for context:
html-export-runtime.ts,html-export-sanitize.ts,html-export-direct-prompt.ts, and the runtime DOM, sanitizer, and prompt test suites. - Caller/contract trace:
injectHtmlExportRuntimefinalizes the saved document;sanitizeAttributesapplies the name gate before finalization, so both reproductions affect the exported artifact rather than test-only helpers. - Adversarial pass — 3 attack angles: raw-text boundary preservation and HTML form-name contract attacks both hold by direct execution; the slide active-class path is covered by its new DOM regression and the focused suite.
- Cross-reference: PR #71 closes #70. No other open PR was found touching these 28 files; no related revert was found in the repository history.
This is a COMMENT review, not an approval; explicit user final sign-off is still required.
🛠️ NEEDS REVISION
🦀 Reviewed by Hermes agent BOT 🦞
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1914de0ab4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (attribute.value && attribute.value.toLowerCase() !== name) { | ||
| context.stripped.push({ code: HTML_VIOLATION_CODES.attribute, detail: `boolean attribute ${name} must be empty or its own name` }); | ||
| continue; |
There was a problem hiding this comment.
When generated markup uses common explicit forms such as <input required="true">, <input checked="true">, or <option selected="true">, browsers treat the attribute's presence as enabled, but this branch strips it because the value is not empty or equal to the attribute name. The finalized form therefore silently loses its authored validation, checked, or selection state; normalize an explicit true value to the empty boolean form rather than discarding it.
Useful? React with 👍 / 👎.
project820
left a comment
project820
left a comment
There was a problem hiding this comment.
🤖 Psycho-842 [BOT] review
⚖️ Verdict: NEEDS REVISION — the tip is green, but explicit boolean form values still lose their browser-defined state in the finalized export.
Summary
This Stage 2 settlement re-measured 1914de0 after the prior CSP-literal and structured-form-name fixes.
GitHub CI, mergeability, the full local suite, typecheck, and the direct export harness are green.
One newly confirmed P2 remains: common explicit boolean attributes such as checked="true" are removed instead of being normalized to presence.
Findings (by severity)
- 🟠 P2 Med —
src/main/html-export-sanitize.ts:472-475rejects every nonempty boolean value unless it exactly repeats the attribute name. HTML treats presence as true, so<input checked="true">,<input required="true">, and<option selected="true">lose their authored checked/validation/selection state after sanitization. Normalize accepted explicit true values to the empty boolean form (or preserve presence semantics), and add regression coverage for input and option controls.
Response to bot findings
- Codex's current-head report on explicit boolean values was independently confirmed: the current sanitizer test intentionally asserts that
checked="true"is absent from the output, matching the branch implementation. - The previous structured-name and CSP-literal findings are resolved at this head: bracketed names are accepted by
SAFE_FORM_METADATA_TOKEN, and CSP replacement uses the parsed head-meta ranges rather than a document-wide raw-text match.
📋 Detailed analysis (measured CI, diff, and validation)
- Settlement head:
1914de0ab4137bdd84fc0cd9a581bfe171a7d18b; +1,257/−275 across 28 files;mergeable=MERGEABLE,mergeStateStatus=CLEAN. - GitHub HEAD CI:
verifysucceeded on this exact SHA (run29681375004); the only annotation is GitHub's Node 20 deprecation warning for actions, not a failed check. - Local validation on this exact SHA:
npm run testpassed 149 files / 2,398 tests;npm run typecheckpassed;npm run test:html-export-directpassed for scroll/slides × landscape/portrait with zero remote requests and preview/save digest equality. - Direct reproduction: the focused sanitizer suite passes while explicitly expecting
checked="true"to be stripped; source lines 472-475 implement that behavior. Browser boolean attributes are true by presence, so this changes authored form state. - Files opened for context:
html-export-sanitize.ts,html-export-runtime.ts,html-export-document-markers.ts,html-export-pipeline-service.ts,html-export-generation-orchestrator.ts, and their sanitizer/runtime/pipeline tests. - Caller trace:
HtmlExportPipelineService.sanitizecallssanitizeHtmlExport; itssanitizeAttributesresult becomesbodyHtml, then the resolver and finalizer inject the runtime around those already-sanitized bytes. The lost boolean state therefore reaches the saved artifact. - Adversarial pass — 4 angles tried: raw-text CSP replacement, structured form names, explicit boolean presence semantics, and slide-mode resolve-before-quarantine ordering. The first, second, and fourth are covered by current code/tests; the boolean-presence attack holds by direct source/test execution.
- Related work: this PR closes #70. No other open PR overlaps its changed files; no revert was found. Diff whitespace check passed.
This is a COMMENT review, not an approval; explicit user final sign-off is still required.
🛠️ NEEDS REVISION
🦀 Reviewed by Hermes agent BOT 🦞
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e9277f6cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 'li', 'dl', 'dt', 'dd', 'blockquote', 'figure', 'figcaption', 'img', 'picture', 'source', 'svg', | ||
| 'table', 'thead', 'tbody', 'tfoot', 'tr', 'th', 'td', 'caption', 'code', 'pre', 'kbd', 'samp', | ||
| 'abbr', 'time', 'a', | ||
| 'abbr', 'time', 'a', 'script', 'form', 'input', 'textarea', 'select', 'option', 'optgroup', 'button', 'label', 'fieldset', 'legend', |
There was a problem hiding this comment.
When a preserved script or event handler implements a common interaction by toggling a class or data attribute on document.body/document.documentElement, the matching authored CSS no longer responds: scopeSelector() rewrites body, html, and :root selectors onto [data-he-content], while the script executes unchanged against the outer shell roots. For example, body.open .panel becomes [data-he-content].open .panel, but document.body.classList.toggle('open') puts the class on the wrong element, so the interaction silently fails; preserved scripts need a compatible root-mutation contract or transformation.
Useful? React with 👍 / 👎.
project820
left a comment
project820
left a comment
There was a problem hiding this comment.
🤖 Psycho-842 [BOT] review
⚖️ Verdict: NEEDS REVISION — the tip is green, but authored interactive scripts can still mutate a root that the sanitizer no longer styles.
Summary
This Stage 2 settlement re-measured 2e9277f after the boolean-attribute fix.
The runtime, pipeline, and focused export harness are green, but the CSS scoping contract and preserved script contract disagree on html/body mutations.
An otherwise valid interactive export can therefore keep its script action while losing the authored visual state.
Findings (by severity)
- 🟠 P2 Med —
src/main/html-export-css-sanitize.ts:275-354scopesbody.open .panelto[data-he-content].open .panel, whilesrc/main/html-export-sanitize.ts:668preserves the authored script unchanged. A direct finalized-artifact probe confirmeddocument.body.classList.add('open')yieldsbody.open=true,content.open=false, andscoped-rule-matches=false; the panel's intended state never activates. Define and implement one script-compatible root contract (for example, a stable content-root helper or compatible mutation rewrite) and add a DOM regression coveringhtml/bodyclass and data-attribute mutations.
Response to bot findings
- Codex's current-head root-mutation report was independently reproduced and is incorporated above as P2.
- The previous explicit-boolean report is resolved at this head: the sanitizer normalizes retained boolean attributes to bare presence, and the updated runtime DOM and sanitizer tests cover
required,checked, andselectedvalues.
📋 Detailed analysis (measured CI, diff, and validation)
- Settlement head:
2e9277f6cfb12eaf6e1efc624280f698b6dbb6e9; +1,262/−275 across 28 files;mergeable=MERGEABLE,mergeStateStatus=CLEAN. - GitHub tip CI:
verifysucceeded on this exact SHA (completed 2026-07-19 15:50 UTC); the CI and head remained unchanged for 156 minutes at measurement. - Local validation passed on this checkout:
npm ci,npm run typecheck,npm test,npm run build, andnpm run test:html-export-direct. The direct harness passed scroll/slides × landscape/portrait, reported zero remote requests, and verified preview/save digest equality. - Direct regression probe: sanitized CSS contained
[data-he-content].open .panel{display:block}. After executing the preserved authored script, the measured result wasbody.open=true; content.open=false; scoped-rule-matches=false. - Files opened for context:
html-export-css-sanitize.ts,html-export-sanitize.ts,html-export-shell.ts,html-export-runtime.ts,html-export-pipeline-service.ts,html-export-generation-orchestrator.ts,html-export-generate.ts, the direct-authoring prompt, and runtime/sanitizer/pipeline tests. - Caller trace:
sanitizeHtmlExportscopes global CSS to[data-he-content];bundleSanitizedHtmlcreates that wrapper; authored scripts remain untransformed;injectHtmlExportRuntimeadds the app runtime without reconciling those roots. The mismatch reaches the saved artifact. - Cross-reference: this PR closes #70. No other open PR touches these files, and no related revert was found in the recent history.
git diff --check origin/main...HEADpassed.
This is a COMMENT review, not an approval; explicit user final sign-off is still required.
🛠️ NEEDS REVISION
🦀 Reviewed by Hermes agent BOT 🦞
1 participant
Closes #70
Enables inline interactive HTML export primitives, a CSP that blocks network access, app-injected theme controls, and slide navigation support. Updates direct-authoring instructions for themed and interactive output.
Verification: npm run typecheck.