Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings
- [Major] Contradictory external-resource policy for charts — the new chart contract instructs loading Chart.js from jsDelivr, but output rules allow only Tailwind CDN and Google Fonts. This creates conflicting directives and can cause invalid artifacts or non-deterministic model behavior, evidence
packages/core/src/prompts/index.ts:419(new) vspackages/core/src/prompts/output-rules.v1.txt:26(existing policy).
Suggested fix:// packages/core/src/prompts/index.ts (CHART_RENDERING) - **Chart.js via CDN** — preferred for interactive charts with hover/animation. Include `<script src="https://cdn.jsdelivr.net/npm/chart.js"></script>` and a `<canvas>` per chart. + **Inline SVG** — preferred default for chart rendering in HTML artifacts. + **Chart.js** — use only when allowed by global output resource policy; otherwise render equivalent charts with inline SVG.
Summary
- Review mode: initial
- 1 issue found in modified lines.
docs/VISION.mdanddocs/PRINCIPLES.md: Not found in repo/docs.
Testing
- Not run (automation)
|
|
||
| ## Rendering choice (pick ONE per artifact) | ||
| - **Inline SVG** — preferred for static charts up to ~30 data points. Hand-code paths, axes, gridlines, labels. | ||
| - **Chart.js via CDN** — preferred for interactive charts with hover/animation. Include \`<script src="https://cdn.jsdelivr.net/npm/chart.js"></script>\` and a \`<canvas>\` per chart. |
There was a problem hiding this comment.
[Major] This line conflicts with the existing output-resource policy: output-rules only permits Tailwind CDN + Google Fonts, but this adds jsDelivr Chart.js. Please align by making inline SVG the default and gating Chart.js on allowed-resource policy.
Suggested fix:
- **Chart.js via CDN** — preferred for interactive charts with hover/animation. Include `<script src="https://cdn.jsdelivr.net/npm/chart.js"></script>` and a `<canvas>` per chart.
+ **Inline SVG** — preferred default for chart rendering in HTML artifacts.
+ **Chart.js** — use only when allowed by global output resource policy; otherwise render equivalent charts with inline SVG.
There was a problem hiding this comment.
Findings
- [Major] Chart source policy now conflicts with self-contained output rules — the new chart contract permits
Recharts via React + esm.shand opt-inChart.js via CDN, which contradicts the existing rule that only Tailwind CDN and Google Fonts are allowed external resources; this can regress generated artifacts into disallowed external script usage. Evidence:packages/core/src/prompts/index.ts:421,packages/core/src/prompts/index.ts:422,packages/core/src/prompts/index.ts:423.
Suggested fix:// Keep chart policy aligned with self-contained output rules. - **Recharts via React + esm.sh** — allowed only when the artifact is a React document that already loads ESM modules; never for plain HTML. - **Chart.js via CDN** — opt-in fallback only. Do NOT add `<script src="https://cdn.jsdelivr.net/npm/chart.js"></script>` unless the user has explicitly relaxed the self-contained constraint (e.g. "you may use Chart.js" / "external CDNs are fine"). When that opt-in is absent, treat Chart.js as forbidden and render with inline SVG. + For artifacts in this app, render charts with inline SVG only. + Do not import chart libraries from CDNs or ESM hosts.
Summary
- Review mode: initial
- 1 major issue found in the latest diff.
- Residual risk/testing gap: no assertion currently guards against accidental reintroduction of
esm.sh/chart.jsguidance in non-tweak prompts.
Testing
- Not run (automation).
- Suggested tests: add prompt-composition assertions that
composeSystemPrompt({ mode: 'create' })andcomposeSystemPrompt({ mode: 'revise' })do not includeesm.shorcdn.jsdelivr.net/npm/chart.js.
open-codesign Bot
| Plain HTML artifacts MUST stay self-contained per the output rules (Tailwind + Google Fonts are the only permitted external resources). That means: | ||
|
|
||
| - **Inline SVG** — DEFAULT for every chart in a plain HTML artifact, regardless of point count or interactivity. Hand-code paths, axes, gridlines, labels. Add lightweight CSS \`:hover\` / \`transition\` for interactivity instead of reaching for a charting library. | ||
| - **Recharts via React + esm.sh** — allowed only when the artifact is a React document that already loads ESM modules; never for plain HTML. |
There was a problem hiding this comment.
[Major] This new guidance allows external chart libraries (esm.sh, CDN Chart.js) that conflict with the existing self-contained output rule (only Tailwind CDN + Google Fonts). Please make chart rendering policy self-contained here to prevent generation regressions.
|
Closing. The chart contract permitted Recharts via esm.sh + Chart.js via CDN, which conflicts with the no-CDN hard constraint in CLAUDE.md (only Tailwind + Google Fonts allowed external). #89 (dashboard ambient) already showed inline SVG charts work well — that's the path forward. Will reopen as 'inline-chart-recipes' contract in v0.2. |
|
Reopening after research. Confirmed Claude Artifacts (Anthropic's reference) DOES allow CDN scripts via cdnjs.cloudflare.com whitelist. Our 'no CDN' hard constraint in CLAUDE.md was overly rigid. Plan: revise this PR's chart contract to use cdnjs whitelist (Recharts / Chart.js only, pinned versions), drop esm.sh which is too open. CLAUDE.md amendment will land separately. |
hqhq1025
force-pushed
the
wt/loop-feat-chart-contract
branch
from
afdff6e to
9a451d0
Compare
|
Refactored on top of #102 (cdnjs whitelist). What changed since the previous revision (commit
Addresses the original codex finding that Verified cdnjs versions used by the whitelist (queried
(The chart contract itself does not pin numeric versions — that is the model's responsibility per the output-rules format
|
There was a problem hiding this comment.
Findings
- [Minor] Missing revise-mode regression coverage for chart contract composition —
composeSystemPrompt()now addsCHART_RENDERINGfor all non-tweakmodes, which includesrevise, but tests only assertcreateincludes andtweakexcludes. This leaves the newly introducedrevisepath unguarded against accidental regressions. Evidencepackages/core/src/prompts/index.ts:816,packages/core/src/generate.test.ts:1063
Suggested fix:it('revise mode includes the chart rendering contract', () => { const prompt = composeSystemPrompt({ mode: 'revise' }); expect(prompt).toContain('Chart rendering contract'); });
Summary
- Review mode: follow-up after new commits
- 1 minor issue found in the latest diff.
- Context note:
docs/VISION.mdanddocs/PRINCIPLES.mdare Not found in repo/docs in this checkout.
Testing
- Not run (automation).
open-codesign Bot
| expect(prompt).not.toContain('cdn.jsdelivr.net/npm/chart.js'); | ||
| }); | ||
|
|
||
| it('tweak mode does NOT include the chart rendering contract', () => { |
There was a problem hiding this comment.
This diff adds chart-contract composition for all non-tweak modes (create + revise), but tests only cover create include and tweak exclude. Please add an explicit revise include assertion to guard the newly introduced path.
Replaces the original esm.sh / jsdelivr references with deferral to the project's approved cdnjs whitelist (PR #102). The chart contract now: - Defers JS-library loading to output-rules' cdnjs whitelist (DRY) - Adds chart-recipe guidance: when to use bar vs line vs donut, axis labelling, accessible color choices, hover state — chart skill body content rather than just a CDN reference - Complements the data-viz-recharts skill instead of duplicating it
hqhq1025
force-pushed
the
wt/loop-feat-chart-contract
branch
from
9a451d0 to
d7ffd66
Compare
|
Addressed Codex [Minor]: added |
There was a problem hiding this comment.
Findings
- No high-confidence issues found in added/modified lines of the latest diff.
Summary
- Review mode: follow-up after new commits
- No new issues found in this diff.
- Context note:
docs/VISION.mdanddocs/PRINCIPLES.mdwere Not found in repo/docs in this checkout. - Residual risk: runtime verification is limited because local test execution was unavailable in this environment.
Testing
- Not run (automation):
pnpmis not available in this runner, sopackages/core/src/generate.test.tscould not be executed here.
open-codesign Bot
1 participant
Summary
The system prompt now includes a dedicated Chart rendering contract section so the model emits real chart markup instead of label lists or empty section headers. Refactored to align with the cdnjs whitelist landed in #102.
What changed
packages/core/src/prompts/chart-rendering.v1.txt(mirrored asCHART_RENDERINGinindex.ts).createandrevisemodes; excluded fromtweakmode.output-rules.v1.txtrather than re-listing libraries (DRY against feat(core): permit cdnjs library whitelist in generated artifacts #102 — single source of truth).data-viz-recharts.mdskill (Recharts-specific styling) without duplicating it.Addressing the original codex finding
The original PR was flagged because it used
esm.shandcdn.jsdelivr.net, which violated the then-currentno CDNrule. After #102 we have a verified cdnjs slug whitelist (recharts,Chart.js,d3,three.js,lodash.js,PapaParse). This refactor:esm.sh/jsdelivrreference from the chart contract.esm.sh/recharts,cdn.jsdelivr.net/npm/chart.js).Tests
generate.test.ts:createmode includes chart contract + cdnjs deferral;tweakmode excludes it..txt↔ TS constant in sync).Four PRINCIPLES checks
chartRenderingkey inPROMPT_SECTIONSmap (additive)output-ruleswhitelist (DRY)