feat(ui): structured {message, skill} JSON output for Skill Builder (#252 part 2)#276
Conversation
initializ-mk
left a comment
initializ-mk
left a comment
There was a problem hiding this comment.
Review: structured {message, skill} JSON output
The design is right: mis-nested quadruple fences are a real LLM failure mode, the envelope mirrors the existing skill_draft shape so only transport changes, and the brace scanner correctly skips string literals with escape handling (the brace-in-string test pins exactly the case that motivates it). The edit-mode prompt rewrite is careful — full skill_md atomically, **Changed:** moved to message, and scripts defined as the complete post-edit set, which crisply resolves the old "do NOT re-emit dropped scripts" ambiguity. UI handling is thorough (pending state, no-message fallback in onDone), and docs are updated in the same PR including the stale "three essentials" fix.
Two parser gaps (inline on skill_envelope.go) both land on the same promise the PR makes — "a model that hasn't adopted the new format never silently loses the draft" — and share one ~10-line fix. Plus one process item:
3. Process: stale base — this PR has never run CI
The base feat/skill-builder-convergence (#258) merged on 2026-07-10, and the PR body's own plan says "rebase to main once #258 merges." That's now — and gh pr checks reports no checks at all on this branch, so the "build + lint clean, tests green" claims are local-only. Please retarget to main so CI runs before merge.
Minors / nits
- No handler-level test for the new SSE sequence (
progress→message→skill_draft→done), including the empty-message edge the UI specifically handles. The envelope parser is well-tested; the wire contract isn't. - Empty-response edge: a fully empty model response yields no
messageevent and an empty bubble afteronDoneclears pending — cosmetic; worth a "(no response)" placeholder if easy.
Verdict
Fix the two parser findings before merge (single small change: both-keys guard + candidate iteration, plus two tests), and retarget to main — mandatory anyway for CI. Everything else is in good shape.
| // that happens to unmarshal into skillEnvelope with all-zero fields. We | ||
| // require the literal "message" or "skill" key to be present. | ||
| func looksLikeEnvelope(jsonStr string) bool { | ||
| return strings.Contains(jsonStr, `"message"`) || strings.Contains(jsonStr, `"skill"`) |
There was a problem hiding this comment.
Important: the single-key guard lets an incidental JSON object hijack the parse — silent draft loss.
extractJSONObject returns the FIRST balanced {...} in the response, and this guard accepts a single key ("message" OR "skill"). Consider a legacy-format model whose prose includes a small JSON sample before the fences — very plausible in this domain, since tool Output examples are JSON:
The tool returns {"message": "ok"} on success. Here's the skill:
````skill.md
...
`{"message": "ok"}` is valid JSON and contains `"message"`, so it parses as structured with `skill: null` → the fenced draft is **never extracted**, the user sees "ok" as the reply, and the draft is silently lost — exactly the failure the fallback exists to prevent. The doc comment here worries about all-zero-field false positives, but the single-key check doesn't close the door.
**Fix:** require BOTH `"message"` AND `"skill"` keys — the prompt mandates exactly two fields, so a real envelope always carries both — and combine with candidate iteration (see the `extractJSONObject` comment). Please add this scenario as a test.
| // early). This is what makes the {skill_md: "...markdown with { }..."} | ||
| // payload parse reliably. | ||
| func extractJSONObject(s string) string { | ||
| start := strings.IndexByte(s, '{') |
There was a problem hiding this comment.
Medium (same root): only the first { candidate is ever tried.
If the model emits the correct envelope but with a brace-bearing preamble — "Sure — for parsing {json} data, here you go: {…envelope…}" — the first candidate {json} fails to unmarshal and parseSkillEnvelope goes straight to legacy fallback. Since the response has no fences, extractArtifacts finds nothing: the draft is lost AND the chat displays the raw JSON envelope (full skill_md and all) as the message.
Fix: on unmarshal/guard failure, advance past the failed { and scan for the next candidate instead of abandoning the structured path — loop until a candidate unmarshals as an envelope or the string is exhausted. Resolves both this and the hijack case above; worth a test with brace-bearing prose before a valid envelope.
| fullResponse.WriteString(chunk) | ||
| data, _ := json.Marshal(map[string]string{"content": chunk}) | ||
| _, _ = fmt.Fprintf(w, "event: chunk\ndata: %s\n\n", data) | ||
| _, _ = fmt.Fprint(w, "event: progress\ndata: {}\n\n") |
There was a problem hiding this comment.
Nit: this fires one SSE event + flush per token chunk — hundreds of progress events for a 10–20 KB draft. Harmless but chatty; throttling to one ping per ~500ms (track last-sent time in the closure) would keep the connection warm at a fraction of the writes.
…252 part 2) Replace the fragile quadruple-backtick fence output format with a structured JSON envelope, the second half of #252 (part 1 shipped convergence rules + install recipes in #258). Each turn the builder now returns a single JSON object: {"message": "<chat reply>", "skill": null | {"skill_md": "...", "scripts": {...}}} `skill` is null while interviewing and carries the full draft once draftable. LLMs frequently mis-nested the old ````skill.md / ````script: fences (inner triple-backtick JSON schemas had to nest safely); a JSON envelope is far more robust to produce and consume. - skill_envelope.go: parseSkillEnvelope + extractJSONObject. The brace scanner skips string literals so a '}' inside the embedded SKILL.md (very likely in a tool's Output JSON schema) doesn't terminate the object early. Falls back to legacy fence extraction when the model ignores the JSON instruction, so an un-migrated model degrades gracefully instead of losing the draft. - handlers_skill_builder.go: OnDone parses the envelope and emits `message` then `skill_draft`. Raw chunks are no longer forwarded to chat (streaming half-formed JSON would be unreadable); a content-free `progress` event keeps the connection warm and drives a "designing" affordance. - skill_builder_context.go: rewrote the Output Format section to specify the envelope; reframed the worked examples as skill_md CONTENT; updated edit-mode rules to route the updated skill through skill.skill_md and the Changed: summary through message. Kept every Forge-correctness rule ($1 input, JSON output, ## Tool: sections with Input/Output/Examples, safety, edit-mode tool-name preservation). - app.js: consume `progress`/`message` events; render a "Designing your skill…" placeholder until the message arrives. - Tests: parser (interviewing / draft / fenced-JSON / legacy-fallback / brace-in-string) + prompt assertions (structured output, edit-mode shape). - Docs: new "Structured output" section; corrected stale "three essentials".
… review)
Both parser findings share the promise the PR makes — "a model that hasn't
adopted the new format never silently loses the draft":
- require BOTH `message` AND `skill` keys (was: either). An incidental
`{"message":"ok"}` in prose — plausible since tool-output examples are JSON —
no longer hijacks the parse as an envelope and drops a fenced draft.
- iterate ALL balanced {...} candidates, not just the first. A valid envelope
after a brace-bearing preamble is now found instead of being abandoned to the
legacy path (which would show the raw JSON as the message). extractJSONObject
keeps returning the first object for existing callers; parseSkillEnvelope
scans via the new jsonObjectAt(s, from).
- Tests: incidental-JSON-does-not-hijack, brace-preamble-finds-envelope,
interviewing-both-keys (skill:null must still parse).
Nits from the review:
- Throttle `progress` SSE pings to ~2/sec (was one write+flush per token —
hundreds for a large draft). The UI only needs a periodic keepalive.
- app.js: show "(no response)" instead of a blank bubble on an empty response.
- Handler-level SSE-sequence tests: progress → message → skill_draft → done for
a structured envelope (message carries no raw JSON), and the empty-response
edge (done only, no message/skill_draft).
initializ-mk
force-pushed
the
feat/skill-builder-json-output
branch
from
11bb781 to
215350d
Compare
initializ-mk
changed the base branch from
feat/skill-builder-convergence
to
main
|
Thanks — both parser findings and the process item are addressed. Findings 1 + 2 (silent draft loss) — fixed with the single change you suggested.
Process: retargeted to Nits:
Local: build + |
initializ-mk
left a comment
initializ-mk
left a comment
There was a problem hiding this comment.
Re-review of 215350d — parser fixes verified; two CI problems block a meaningful merge
Finding 1 (single-key guard) — fixed. looksLikeEnvelope requires BOTH keys, TestParseSkillEnvelope_InterviewingBothKeys pins that the legitimate skill: null interviewing turn still parses, and TestParseSkillEnvelope_IncidentalJSONDoesNotHijack reproduces the exact hijack scenario and asserts the fenced draft survives.
Finding 2 (candidate iteration) — fixed. jsonObjectAt(s, from) + the candidate loop, with extractJSONObject retained for existing callers; TestParseSkillEnvelope_BracePreambleFindsEnvelope pins the exact preamble example. One minor residual inline (non-blocking).
All three nits taken: progress throttled to ~2/sec (first ping still immediate — lastProgress zero value), "(no response)" placeholder, and a genuinely good handler-level SSE harness (real UIServer with injected LLMStreamFunc; asserts the full progress → message → skill_draft → done sequence, no raw JSON in the message, done last, and the empty-response edge).
Two CI problems
-
CI still hasn't run on this branch. The workflow triggers on
pull_request→main/develop; the old stacked base explains the original silence, but retargeting fires theeditedevent, which is not in the default trigger types (opened/synchronize/reopened). If the fix commit was pushed before the retarget, nothing has triggered since. An empty-commit push or close/reopen will kick it. -
Even when CI runs, it will not execute this PR's tests. The workflow's vet / fmt / test commands cover
./forge-core/... ./forge-cli/... ./forge-skills/... ./forge-plugins/...—./forge-ui/...is absent (a separate workspace module, so it needs explicit listing). This PR adds ~300 lines of parser/handler logic with a good test suite that CI is structurally blind to — the same class of gap #260 fixed forforge-skills("silently untested"). Please add./forge-ui/...to the CI loop (ride-along here or an immediate follow-up), otherwise a green checkmark on this PR certifies nothing about its code.
Verdict
Code changes are merge-ready (residual hardening optional). Hold the merge until CI triggers AND covers forge-ui — right now there is no automated verification of this PR's test suite on record.
| if cand == "" { | ||
| break | ||
| } | ||
| from = end |
There was a problem hiding this comment.
Minor residual (non-blocking, two-line hardening): a candidate that contains both key substrings but unmarshals with all-zero fields still commits the structured path. The classic LLM wrapper tic —
{"response": {"message": "…", "skill": {…}}}— passes looksLikeEnvelope (both substrings present), unmarshals into a zero skillEnvelope (unknown field ignored), and returns structured with empty message + no draft. And because from = end here jumps past the WHOLE outer object, the inner real envelope is never tried.
Hardening: reject zero-value envelopes (message == "" && env.Skill == nil → continue; a real envelope that degenerate is useless anyway, and the legacy fallback shows the raw text), and advance from to just past the failed candidate's { (start+1) rather than past the whole object, so nested candidates are reachable.
… review)
Both parser findings share the promise the PR makes — "a model that hasn't
adopted the new format never silently loses the draft":
- require BOTH `message` AND `skill` keys (was: either). An incidental
`{"message":"ok"}` in prose — plausible since tool-output examples are JSON —
no longer hijacks the parse as an envelope and drops a fenced draft.
- iterate ALL balanced {...} candidates, not just the first. A valid envelope
after a brace-bearing preamble is now found instead of being abandoned to the
legacy path (which would show the raw JSON as the message). extractJSONObject
keeps returning the first object for existing callers; parseSkillEnvelope
scans via the new jsonObjectAt(s, from).
- Tests: incidental-JSON-does-not-hijack, brace-preamble-finds-envelope,
interviewing-both-keys (skill:null must still parse).
Nits from the review:
- Throttle `progress` SSE pings to ~2/sec (was one write+flush per token —
hundreds for a large draft). The UI only needs a periodic keepalive.
- app.js: show "(no response)" instead of a blank bubble on an empty response.
- Handler-level SSE-sequence tests: progress → message → skill_draft → done for
a structured envelope (message carries no raw JSON), and the empty-response
edge (done only, no message/skill_draft).
initializ-mk
force-pushed
the
feat/skill-builder-json-output
branch
from
215350d to
9d076f1
Compare
…ects (#276 re-review) Two CI problems the re-review flagged, plus the optional parser hardening: 1. CI was structurally blind to forge-ui. The vet / fmt / test / lint / integration steps listed forge-core/cli/skills/plugins but not ./forge-ui/... (a separate workspace module needing explicit listing), so this PR's ~300 lines of parser/handler tests never ran — a green check certified nothing about the code. Added ./forge-ui/... to every step (+ a `go mod verify` for it). Same class of gap #260 closed for forge-skills. (Finding 1 — the workflow never *triggering* — was already fixed by the earlier amend+force-push; this makes the run actually exercise forge-ui.) 2. Parser hardening: a wrapper object like `{"response": {"message":…, "skill":…}}` passed the both-keys substring guard but unmarshalled to a zero-value envelope (unknown top-level field ignored) and, because we advanced past the WHOLE object, the inner real envelope was never tried. Now: reject zero-value envelopes (message=="" && skill==nil → continue), and advance by start+1 (just inside the failed object) so nested candidates are reachable. jsonObjectAt returns the opening-brace index instead of the end. Tests: wrapper-reaches-inner-envelope and non-envelope-wrapper-falls- back-to-legacy. Verified locally with the exact CI command set including ./forge-ui/...: vet, gofmt -l, go test, and golangci-lint all clean.
|
Thanks for the sharp re-review — both CI problems fixed, plus the residual hardening. CI problem 1 (never triggered). Already resolved before this push: the CI problem 2 (forge-ui not covered) — the important one. You're right: the vet / fmt / test / lint / integration steps listed forge-core/cli/skills/plugins but not Residual hardening (inline on The forge-ui CI coverage rides along here since this is the PR that adds forge-ui tests; happy to split it into its own PR if you'd prefer it land independently of the feature. |
… review)
Both parser findings share the promise the PR makes — "a model that hasn't
adopted the new format never silently loses the draft":
- require BOTH `message` AND `skill` keys (was: either). An incidental
`{"message":"ok"}` in prose — plausible since tool-output examples are JSON —
no longer hijacks the parse as an envelope and drops a fenced draft.
- iterate ALL balanced {...} candidates, not just the first. A valid envelope
after a brace-bearing preamble is now found instead of being abandoned to the
legacy path (which would show the raw JSON as the message). extractJSONObject
keeps returning the first object for existing callers; parseSkillEnvelope
scans via the new jsonObjectAt(s, from).
- Tests: incidental-JSON-does-not-hijack, brace-preamble-finds-envelope,
interviewing-both-keys (skill:null must still parse).
Nits from the review:
- Throttle `progress` SSE pings to ~2/sec (was one write+flush per token —
hundreds for a large draft). The UI only needs a periodic keepalive.
- app.js: show "(no response)" instead of a blank bubble on an empty response.
- Handler-level SSE-sequence tests: progress → message → skill_draft → done for
a structured envelope (message carries no raw JSON), and the empty-response
edge (done only, no message/skill_draft).
1 participant
What
Part 2 of #252 — replace the Skill Builder's fragile quadruple-backtick fence output with a structured
{message, skill}JSON envelope. (Part 1, #258, shipped the convergence rules + install recipes.)Each turn the builder returns a single JSON object:
{"message": "<chat reply>", "skill": null | {"skill_md": "…", "scripts": {…}}}skillisnullwhile interviewing and carries the full draft once draftable. The UI rendersmessagein chat and loadsskill_md/scriptsinto the editor.Why
LLMs frequently mis-nested the old
````skill.md/````script:fences — inner triple-backtick JSON schemas had to nest safely inside quadruple fences. A JSON envelope is far more robust to produce and consume (issue #252, motivation #2).Design decisions
skill: {skill_md, scripts}(LLM writes the full SKILL.md markdown into a JSON string) rather than a fully-structured tool→markdown compiler in Go. Matches theskill_draftpayload the editor/save endpoint already consume; the per-tool{input, output, examples}requirement stays satisfied because the emittedskill_mdstill carries the mandated## Tool:sections. A structured compiler would be a much larger, drift-prone change — out of scope for part 2.progressevent keeps the SSE connection warm and drives a "Designing your skill…" affordance;message+skill_draftarrive atdone.parseSkillEnvelopefalls back to legacy fence extraction if a model ignores the JSON instruction, so an un-migrated model never silently loses the draft.extractJSONObjectscans by brace depth while skipping string literals, so a}inside the embedded SKILL.md (very likely in a tool's Output JSON schema) doesn't terminate the object early. Also tolerates a```jsonwrapper or surrounding prose.Forge-correctness preserved
Every runtime rule the prompt already got right is retained and test-pinned:
$1input (INPUT="${1:-}"), structured JSON script output,## Tool:sections with Input/Output/Examples, safety constraints,set -euo pipefail, python-via-bins, and edit-mode## Tool:name preservation (#193).Tests
skill_envelope_test.go: interviewing (skill:null) / draft / fenced-JSON / legacy-fence fallback / brace-in-string / no-object.skill_builder_prompt_test.go: new structured-output + edit-mode-shape assertions; existing convergence / install-recipe / correctness assertions still green.Docs
docs/ui/skill-builder-llm.md: new "Structured output" section; corrected a stale "three essentials" (part 1 made it four).Acceptance criteria (#252)
{message, skill}envelope; handler consumes it instead of fence-parsingbinsinstall recipes (shipped in part 1 / feat(ui): skill-builder interview convergence + custom-binary install recipes (#252 part 1) #258){input, output, examples}retained in compiled## Tool:sectionsStacking
Based on
feat/skill-builder-convergence(#258) since both touch the prompt. Rebase tomainonce #258 merges.