visual-artifacts v1: PR 1 static renderer and trust contract#217
Merged
Conversation
Introduce bin/render-artifact.sh and the supporting libraries that turn a Nanostack JSON artifact into a local, static HTML view under $NANOSTACK_STORE/visual/. JSON remains canonical; the renderer is strictly downstream and writes only to the visual root. PR 1 wires the /plan renderer end to end. think, review, security, qa, ship are reserved for PR 2 and exit 1 with a clear message; journal and stack are reserved for PR 3 and exit 2; --interactive is reserved for PR 4 and exits 2. Key files: - reference/visual-artifact-contract.md: normative contract. - bin/lib/html-escape.sh: nano_html_escape, nano_attr_escape, nano_json_string. Every JSON-derived scalar passes through one of these before reaching HTML. - bin/lib/visual-render.sh: shared page shell, CSP, locked trust badge wording (verified / unverified / tampered), output path safety, symlinked visual root rejection. - bin/render-artifact.sh: argument parsing, source resolution via bin/find-artifact.sh, trust verification via bin/lib/artifact-trust.sh, schema validation via bin/lib/artifact-schemas.sh, manifest writer, /plan body renderer. Atomic write with $path.tmp.$$ rename. - ci/e2e-visual-artifacts.sh: 49 checks across 9 cells (happy path, XSS, --strict integrity_missing, integrity_mismatch always fails, --out path safety, reserved features, --manifest-only, phase mismatch, symlinked visual root). - ci/check-visual-artifact-templates.sh: 20 static checks for forbidden patterns (http://, https://, fetch, XMLHttpRequest, localStorage, document.cookie, eval) plus required markers (CSP, data-nanostack-visual, locked badge wording). - .github/workflows/lint.yml: new visual-artifact-contract job that runs both checks on every push. Trust contract: - integrity_mismatch always fails with exit 3. - integrity_missing fails under --strict (exit 3); without --strict the render proceeds and the badge shows 'unverified'. - --out outside the visual root fails with exit 4. - A symlinked visual root fails with exit 4. Total: 69 contract checks locked in CI.
Two PR 1 pass 1 codex findings. 1. Legacy --from-session plan artifacts store .summary as a string, so jq -r '.summary.goal' aborted the body renderer under set -e. The schema warning was already emitted in the page head, but the body never reached the user. Normalize .summary and .context_checkpoint into objects at the top of render_plan_body (defaulting missing arrays to empty arrays), then read every field from the normalized JSON. Adds two regression cells: --from-session legacy plan + an artifact with both summary and context_checkpoint as strings. 2. --out under $NANOSTACK_STORE/visual was rejected on fresh stores because the visual root did not yet exist; the canonical walk-up landed on $NANOSTACK_STORE, which is outside the visual root. Pre-create the visual root before the safety check so realpath has a stable target. Adds a regression cell that confirms --out works when visual/ does not pre-exist. Test count: 49 -> 59 (cells 9a, 9b cover the regressions).
Codex PR 1 pass 2 caught a path-safety gap. A path like $NANOSTACK_STORE/visual/new/../../outside.html passed the previous "walk up to nearest existing ancestor" check because the 'new' segment was missing on disk; the walk landed on visual/, the prefix matched, and the final mv wrote to $NANOSTACK_STORE/outside.html outside the visual root. Replace the realpath walk-up with lexical normalization. The new nano_visual_normalize_path resolves "." and ".." string-wise so non-existent intermediate segments do not anchor the comparison. Both the candidate path and the visual root are normalized lexically; symlink protection on the visual root itself is still provided by nano_visual_assert_safe_root. Adds a regression cell that exercises the documented escape and a check that no file is left behind outside visual/ after the rejection. Test count: 59 -> 61.
Codex PR 1 pass 3 finding. A relative NANOSTACK_STORE override (e.g. NANOSTACK_STORE=.nano-rel) propagated into the renderer's output_path, source path, and stdout. The visual artifact contract requires output_path to be absolute, so the manifest emitted under a relative store violated the contract. After mkdir -p of the parent directories (so $(cd .. && pwd) has a real target), resolve HTML_PATH, MANIFEST_PATH, and ART_PATH to absolute via a small nano_resolve_abs helper. The fix runs late enough that the path-safety check still operates on the caller's literal --out string, and the manifest writer sees only absolutes. Adds cell 9d: chdir to a fresh project, set NANOSTACK_STORE to a relative ".nano-rel", render, and assert the stdout, manifest output_path, and manifest source path are all absolute. Test count: 61 -> 64.
Codex PR 1 pass 4 finding. A pre-existing symlink under visual/ (for example visual/plan -> /tmp/outside) was accepted by mkdir -p and the renderer's atomic mv then wrote the HTML or manifest into the symlink target. nano_visual_assert_safe_root only guarded the root itself, so the new path-safety contract was incomplete. Add nano_visual_assert_safe_descend: walk from the visual root down to (but not including) the leaf file, asserting -L is false at every intermediate. render-artifact.sh calls it for both the HTML path and the manifest path before mkdir -p runs. Adds two regression cells: a symlinked visual/plan and a symlinked visual/manifests, each confirming exit 4 and no file written through the symlink target. Test count: 64 -> 68.
Codex PR 1 pass 5 finding. The descend check walked every directory component but stopped before the leaf, so an --out whose final segment was a pre-existing symlink to a directory escaped the path-safety contract: the atomic mv moved the temp file INTO the symlink target instead of overwriting the link. Extend nano_visual_assert_safe_descend to refuse leaves that are symlinks (return 4) or directories (return 4). The renderer writes a regular file at that path; symlinks and directories at the leaf were never part of the contract. Adds cell 9g: a symlinked leaf at --out and a directory at the --out path. Both must exit 4 and leave no file in the symlink target. Test count: 68 -> 71.
Three findings from codex PR 1 pass 6. P1 — write to the normalized path A path like --out $NANOSTACK_STORE/visual/link/../evil.html with link a symlink to /tmp/outside collapsed lexically to visual/ evil.html and passed the safety check. The kernel still resolved the original path at write time: it followed link to /tmp/outside, took .. back to /tmp/, and wrote evil.html outside the visual root. Reassign HTML_PATH and MANIFEST_PATH to their normalized form before mkdir -p / mv so the kernel never traverses a `..` after a symlinked component. P2 — unique manifest stem per render Two same-second renders shared a manifest stem, so the second render overwrote the first manifest while the first HTML kept pointing at the now-stale path. Append the renderer's PID to the timestamp stem; each render-artifact.sh invocation is its own process so the stem is unique across same-second invocations. P3 — non-object JSON exits 1 A top-level array, string, or number artifact crashed jq -r '.phase // ""' with exit 5 under set -e, violating the CLI contract (exit 1 for input errors). Switch to `.phase?` so the path error is suppressed and the existing phase-mismatch branch returns exit 1 cleanly. Adds three regression cells covering the symlink+.. bypass, the same-second manifest uniqueness, and the three non-object JSON shapes (array, string, number). Test count: 71 -> 79.
Codex PR 1 pass 7 finding. find-artifact.sh registers the producing phase via session.sh phase-start as a convenience for downstream skills; render-artifact.sh hitting that code path through --latest made a strictly-downstream viewer mutate session.json. A user who opened the latest plan as HTML would silently start a plan phase. Add --no-session-sync to find-artifact.sh. The flag bypasses the phase-start side effect while preserving every other behavior (integrity checks, max-age filter, project matching). render-artifact.sh now uses --no-session-sync on its --latest lookup, restoring the read-only contract documented in reference/visual-artifact-contract.md. Adds cell 9l: start a session, snapshot phase_log, run a render, assert phase_log is unchanged and plan is not flagged in_progress. Test count: 79 -> 81.
Codex PR 1 pass 8. Two findings.
P2 — predictable temp file
Temp filenames followed a guessable pattern ($HTML_PATH.tmp.<pid>).
An attacker with write access to a parent directory could
pre-create a symlink at that path, and bash's > redirect would
follow the symlink and write outside visual/.
Replace the manual temp naming with mktemp("$path.tmp.XXXXXX").
mktemp opens with O_EXCL so a pre-existing symlink races into a
clear failure (exit 4) instead of silent follow. The cleanup trap
keeps unlinking on early exit. Cell 9n verifies no .tmp.* leftover
after a successful render.
P3 — glob expansion during normalization
The IFS split inside nano_visual_normalize_path and
nano_visual_assert_safe_descend used an unquoted `set -- $raw`,
which performs pathname expansion against the current working
directory. An --out like "star*.html" could be silently rewritten
to a matching real filename, so the renderer wrote to a different
path than the caller asked for and the manifest recorded the wrong
output_path.
Save the current `set -f` state, disable globbing for the split,
and restore the previous state when the helpers return. Cell 9m
locks the contract: a literal glob path stays literal.
Test count: 81 -> 85.
Codex PR 1 pass 9. After the PR 1 pass 8 switch to mktemp for the temp files, the --manifest-only branch moved the manifest into place and disabled the cleanup trap, leaving the HTML temp file behind. The intent of --manifest-only is "write no HTML artifacts", so the leftover violated the contract and would accumulate stale *.html.tmp.* files under visual/plan/ on every CI trust-check run. Remove TMP_HTML explicitly in the --manifest-only branch before disabling the trap. Cell 7 now also asserts zero *.tmp.* files remain after a --manifest-only render. Test count: 85 -> 86.
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 1 of the Visual Artifacts v1 round (architect spec, 2026-05-11). Introduces a local, static HTML renderer for Nanostack JSON artifacts. JSON remains canonical; the renderer is strictly downstream and writes only under
$NANOSTACK_STORE/visual/. PR 1 wires the/planrenderer end to end; the other core phases are reserved for PR 2.What changed
reference/visual-artifact-contract.md— normative contract: CLI shape, exit codes, store layout, manifest schema (schema_version: "1"), HTML safety rules, locked trust badge wording (verified/unverified/tampered).bin/lib/html-escape.sh—nano_html_escape,nano_attr_escape,nano_json_string. Every JSON-derived scalar passes through one of these before reaching HTML.bin/lib/visual-render.sh— shared page shell, CSP, output path safety, lexical path normalization, symlinked-descend rejection.bin/render-artifact.sh— argument parser, source resolution viabin/find-artifact.sh --no-session-sync, trust verification viabin/lib/artifact-trust.sh, schema validation viabin/lib/artifact-schemas.sh, manifest writer,/planbody renderer. Atomic write viamktemp+ rename.bin/find-artifact.sh— adds--no-session-syncflag so a strictly read-only consumer can resolve--latestwithout mutatingsession.json.ci/e2e-visual-artifacts.sh— 86 checks across 21 cells.ci/check-visual-artifact-templates.sh— 20 static checks (forbidden patterns, required markers)..github/workflows/lint.yml— newvisual-artifact-contractjob.Contract invariants
$NANOSTACK_STORE/visual/. Outside paths exit 4.integrity_mismatchalways fails (exit 3).integrity_missingfails under--strict; without--strictit renders with theunverifiedbadge.nano_html_escapeornano_attr_escape.default-src 'none'. No external URLs, nofetch, nolocalStorage, noeval.--interactivereserved for PR 4 (exit 2).journal/stackreserved for PR 3 (exit 2).think/review/security/qa/shipreserved for PR 2 (exit 1).Path-safety hardening (codex passes 2 – 8)
..after a missing segment escaped visual/NANOSTACK_STOREproduced relative manifest pathsnano_resolve_abscanonicalizes before manifest writenano_visual_assert_safe_descendrejects symlinked components--outaccepted..collapsed lexically but kernel resolved at writeHTML_PATH/MANIFEST_PATHto normalized form[], string, number) crashed with jq exit 5.phase?suppresses path error so input-error branch returns exit 1--latestmutatedsession.jsonviafind-artifact.sh--no-session-syncflag onfind-artifact.sh$path.tmp.$$)mktemp $path.tmp.XXXXXXwith O_EXCLset -faroundset -- $rawsplit--manifest-onlyleft HTML temp behindrm -f $TMP_HTMLbefore trap resetFinal codex pass returned clean.
Test counts
ci/e2e-visual-artifacts.shacross 21 cells.ci/check-visual-artifact-templates.sh.visual-artifact-contractjob inlint.yml.Test plan
ci/e2e-visual-artifacts.sh86/86ci/check-visual-artifact-templates.sh20/20ci/e2e-artifact-trust.sh29/29 (no regression onbin/find-artifact.sh)python3 -c "import yaml; yaml.safe_load(open('.github/workflows/lint.yml'))"passes