visual-artifacts v1: PR 4 --interactive copy-only mode#220
Merged
Conversation
Enables the documented interactive layer from the architect spec.
v1 scope is copy-only: /plan and /review get an actions card with
three buttons (copy as prompt / copy as Markdown / copy as JSON
patch). The buttons write to the clipboard via
navigator.clipboard.writeText; nothing else.
Hard rules locked in CI:
- The page's <script> body uses ONLY navigator.clipboard.writeText.
- No fetch, no XMLHttpRequest, no sendBeacon, no localStorage, no
sessionStorage, no document.cookie, no eval, no new Function, no
document.write, no window.open, no <form>, no addEventListener
for submit. CI greps the renderer for each.
- CSP under --interactive adds `script-src 'unsafe-inline'`; static
CSP keeps the tighter form. Both still lock `base-uri 'none'` and
`form-action 'none'`.
- The manifest records `interactive: true`.
XSS defense: a payload containing literal `</script>` would close
the surrounding inline <script> block in the HTML parser regardless
of JS quote context. _js_safe_for_script substitutes `</` -> `<\/`
on the JSON-encoded payload before embedding. JSON.parse reads both
forms transparently. The manual-copy <pre> blocks rely on
nano_html_escape so the visible escaped text is also safe.
Payload shapes (built from the normalized JSON so malformed
artifacts still produce useful output):
- /plan: "Approve the plan" prompt; ## Plan summary Markdown;
{action: "plan.approve", ...} JSON patch.
- /review: "Address review findings" prompt; severity table
Markdown; {action: "review.triage", blocking_ids,
should_fix_ids, scope_drift} JSON patch.
CI extensions:
- 5 new e2e cells (23d-23h): copy buttons present in interactive,
absent in static; interactive CSP allows script-src; forbidden
APIs absent from rendered HTML; review interactive payload;
</script>-in-payload XSS containment.
- 7 new template safety checks (16-19): _js_safe_for_script wired,
interactive CSP arm, no document.write / window.open / form
submit listeners.
- check_absent now ignores comment lines so documenting the
forbidden APIs in source comments does not fail the lint.
Test counts:
- e2e: 266 -> 304 (+38)
- template: 31 -> 38 (+7)
- Total contract surface: 297 -> 342
Three codex PR 4 pass 1 findings. P2 — --interactive widened CSP for unsupported phases Passing --interactive to security/qa/ship/think/journal/stack still switched the page CSP to `script-src 'unsafe-inline'` and wrote interactive: true in the manifest even though no copy controls were emitted. Reject --interactive outside /plan and /review with exit 1 so the wider policy never applies to a page that has no script body. P2 — plan copy payloads crashed on non-string fields A schema-warning plan with .summary.goal as a number, .summary.planned_files containing integers, or .summary.risks containing objects made jq exit 5 on the string concatenation / join in the copy payload builders. Wrap every concatenated value with `tostring` and every list element with `map(tostring)`. P2 — review copy payloads had the same gap .summary.blocking as a string, .scope_drift.status as a number, or a finding .description as an object would crash the jq builders. Same `tostring` coercion for every concatenated value. Regression cells: - 23i: --interactive rejected with exit 1 for security/qa/ship/ think/journal/stack. - 23j: schema-warning plan (numeric goal, object risks) renders HTML under --interactive. - 23k: schema-warning review (string blocking count, numeric scope drift, object description) renders HTML under --interactive. Test count: 304 -> 312.
…onally Codex PR 4 pass 2 (P3). Cells 23j and 23k used `|| true` plus a conditional `[ -n "$HTML" ] && assert_true ...`, which silently passed when the renderer aborted with exit non-zero and printed no path. Exactly the regression the cells describe (--interactive crashing on schema-warning artifacts) would not have been caught. Switch to an unconditional structure: capture exit code with `set +e` / `set -e` boundaries, assert exit 0, assert path non-empty, assert file exists. Test count: 312 -> 316 (cells 23j and 23k each gain two checks).
Codex PR 4 pass 3 finding (P2). The previous `</` -> `<\/` escape only neutralized the literal closing-tag sequence. A payload containing `<!--<script>` could still drive the HTML parser into the "script data double escaped" state, which then eats the renderer's legitimate closing `</script>` and breaks the interactive controls (or worse, executes injected JavaScript that happens to follow). Switch _js_safe_for_script to encode EVERY `<` as `<`. JSON.parse reads `<` as the literal `<` so the clipboard content the user pastes is unchanged. The HTML parser never sees a `<` inside the script body and stays in the standard "script data" state until the real closing tag. Cell 23l: a plan with `<!--<script>alert(1)</script>` as the goal must produce HTML where the inline script body contains NO raw `<!--`, `<script`, `</script`, or `<!CDATA` sequence. The encoded `u003c` marker must appear somewhere in the output (proves the escape ran). Cell 23h updated to assert the new encoded form. Test count: 316 -> 321 (5 new assertions in 23l).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR 4 of the Visual Artifacts v1 round. Enables the documented copy-only interactive layer from the architect spec. v1 scope is copy-only:
/planand/reviewget an actions card with three buttons (copy as prompt / copy as Markdown / copy as JSON patch). The buttons write to the clipboard vianavigator.clipboard.writeText; nothing else.What changed
Renderer
render_copy_actionsshared helper: emits an actions card with three buttons and an inline<script>that wiresnavigator.clipboard.writeText. Each payload also renders inside<details><pre>for manual copy when the clipboard API is blocked._js_safe_for_script: encodes every<in the JSON-embedded payload as<so the HTML parser cannot exit the script body via<!--<script>,</script>, or<!CDATA[sequences.--interactivearg-parse, manifestinteractive: true, CSP switch.--interactiverejected outside/planand/reviewwith exit 1 so the wider CSP never applies where no script is emitted.Payload shapes (built from the normalized JSON; every concatenation runs through
tostring)/plan: "Approve the plan" prompt; "## Plan summary" Markdown;{action: "plan.approve", goal, scope, planned_files, out_of_scope}JSON patch./review: "Address review findings" prompt; severity table Markdown;{action: "review.triage", blocking_ids, should_fix_ids, scope_drift}JSON patch.Hard rules locked in CI
navigator.clipboard.writeText. Nofetch, noXMLHttpRequest, nosendBeacon, nolocalStorage, nosessionStorage, nodocument.cookie, noeval, nonew Function, nodocument.write, nowindow.open, no<form>, noaddEventListener('submit', ...).--interactiveaddsscript-src 'unsafe-inline'; static CSP keeps the tighter form. Both still lockbase-uri 'none'andform-action 'none'.interactive: true.Codex review trail (4 passes, clean)
|| truemask<!--<script>payload could trigger double-escape HTML parser state</-><\/to every<-><Test counts
ci/e2e-visual-artifacts.shci/check-visual-artifact-templates.shTest plan
ci/e2e-visual-artifacts.sh321/321ci/check-visual-artifact-templates.sh38/38</script>and<!--<script>in artifact content cannot escape the script bodyinteractiveflag flips between true/false correctly