Merge pull request #114 from flashcatcloud/fix/card-response-shapes

Fix/card response shapes

Author: ysyneu

internal/cmd/skilldoc/main_test.go

@@ -158,6 +158,153 @@ func TestRunGenAll_FillsEveryCardAndSkipsCardless(t *testing.T) {
 	}
 }
 
+// independentlyClassifyResponse re-derives a command's response envelope
+// shape ("object" | "array" | "wrapped") and its top-level (or, for the
+// wrapped shape, per-row) field names straight from its raw Long text, using
+// logic deliberately NOT shared with skilldoc's own responseShapeLine
+// extractor (internal/skilldoc/generate.go) — a from-scratch re-read of the
+// same ground truth, not a call into the code under test. ok is false when
+// Long documents no Response fields block, or the block yields zero fields
+// at the target depth (skilldoc's own extractor also emits nothing for that
+// case — nothing to cross-check).
+func independentlyClassifyResponse(long string) (shape string, fields []string, ok bool) {
+	lines := strings.Split(long, "\n")
+	headerLine := -1
+	for i, l := range lines {
+		if strings.HasPrefix(l, "Response fields (") {
+			headerLine = i
+			break
+		}
+	}
+	if headerLine < 0 {
+		return "", nil, false
+	}
+
+	header := lines[headerLine]
+	switch {
+	case strings.Contains(header, "nested under items[]"):
+		shape = "wrapped"
+	case strings.Contains(header, "TOP-LEVEL array"):
+		shape = "array"
+	default:
+		shape = "object"
+	}
+	prefix := "  - "
+	if shape == "wrapped" {
+		prefix = "    - " // one level under the sole top-level "items" row
+	}
+	for _, l := range lines[headerLine+1:] {
+		if strings.TrimSpace(l) == "" {
+			break
+		}
+		if !strings.HasPrefix(l, prefix) {
+			continue
+		}
+		name := strings.TrimPrefix(l, prefix)
+		if sp := strings.IndexAny(name, " ("); sp >= 0 {
+			name = name[:sp]
+		}
+		fields = append(fields, name)
+	}
+	return shape, fields, len(fields) > 0
+}
+
+// isCligenWrapperWireName mirrors (independently — not by import) the three
+// wire names cligen's own listEnvelope (internal/cmd/cligen/main.go) treats
+// as a paginated-list envelope field.
+func isCligenWrapperWireName(name string) bool {
+	return name == "items" || name == "docs" || name == "list"
+}
+
+// responseLineOf returns the "- response: ..." bullet inside a rendered fence
+// section, and whether one was present.
+func responseLineOf(section string) (string, bool) {
+	for _, l := range strings.Split(section, "\n") {
+		if strings.HasPrefix(strings.TrimSpace(l), "- response: ") {
+			return l, true
+		}
+	}
+	return "", false
+}
+
+// TestGenerateFence_ResponseShapeMatchesRealLong_AllCommands is the
+// coverage-complete ground-truth cross-check: for every real command whose
+// live Long (built from the actual CLI tree, not a fixture) documents a
+// Response fields block, independently reclassify its envelope shape and
+// field names (independentlyClassifyResponse, above — separate logic from
+// the generator) and assert the fence GenerateFence renders for that verb
+// agrees. A single hand-picked example (`schedule list`) proved the
+// mechanism works but only ever covered one of ~200 documented commands;
+// this walks the whole real dump, so a classification drift ANYWHERE in the
+// generator fails the build, not just for the one verb someone happened to
+// write a test against.
+func TestGenerateFence_ResponseShapeMatchesRealLong_AllCommands(t *testing.T) {
+	d := dump()
+
+	checked := 0
+	for _, c := range d.Commands {
+		wantShape, wantFields, hasBlock := independentlyClassifyResponse(c.Long)
+		if !hasBlock {
+			continue
+		}
+		checked++
+
+		// Render this ONE command in isolation (a single-command dump filtered
+		// to its own group) rather than slicing a section out of the whole
+		// group's fence: several real groups (e.g. "incident") flatten
+		// same-named leaves from different subgroups — "incident get" and
+		// "incident war-room get" both render as a "### get" heading — so a
+		// substring/heading search across the full group fence can grab the
+		// wrong command's section. A single-command fence has exactly one
+		// response line, unambiguously.
+		fence := skilldoc.GenerateFence(skilldoc.Dump{Commands: []skilldoc.Command{c}}, c.Group)
+		gotLine, hasLine := responseLineOf(fence)
+
+		// Mirrors the wrapper-drift guard in responseShapeLine: a response
+		// this classifier reads as a top-level object whose sole field is one
+		// of cligen's own list-envelope wire names (items/docs/list, array
+		// type) is a case the generator deliberately suppresses rather than
+		// assert a possibly-wrong shape. No real command hits this today
+		// (cligen's own header would already say "wrapped" for it), but nothing
+		// here should hard-fail if drift ever makes one — that is the guard
+		// working as designed, not a bug.
+		if wantShape == "object" && len(wantFields) == 1 && isCligenWrapperWireName(wantFields[0]) {
+			if hasLine {
+				t.Errorf("%s: expected the wrapper-drift guard to suppress this line (sole field %q looks like a list envelope), got:\n%s", c.Path, wantFields[0], gotLine)
+			}
+			continue
+		}
+
+		if !hasLine {
+			t.Errorf("%s: generated fence has no response line for a documented Response fields block", c.Path)
+			continue
+		}
+		switch wantShape {
+		case "wrapped":
+			if !strings.Contains(gotLine, "page wrapper") || !strings.Contains(gotLine, "jq '.items[]'") {
+				t.Errorf("%s: want items[] page wrapper, got:\n%s", c.Path, gotLine)
+			}
+		case "array":
+			if !strings.Contains(gotLine, "TOP-LEVEL array") {
+				t.Errorf("%s: want TOP-LEVEL array, got:\n%s", c.Path, gotLine)
+			}
+		default: // "object"
+			if !strings.Contains(gotLine, "single object") {
+				t.Errorf("%s: want single object, got:\n%s", c.Path, gotLine)
+			}
+		}
+		for _, f := range wantFields {
+			if !strings.Contains(gotLine, f+" (") {
+				t.Errorf("%s: missing real field %q (from live Long) in generated line:\n%s", c.Path, f, gotLine)
+			}
+		}
+	}
+	if checked < 100 {
+		t.Fatalf("only cross-checked %d commands — expected on the order of 200; did Response-fields detection break, or has the real CLI shrunk?", checked)
+	}
+	t.Logf("cross-checked response shape/fields for %d real commands", checked)
+}
+
 func TestRunGen_FillsFence(t *testing.T) {
 	dir := t.TempDir()
 	d := fixtureDump()

internal/skilldoc/generate.go

@@ -17,9 +17,14 @@ const (
 // GenerateFence renders the factual fenced block for one command group: a
 // section per leaf verb with its short description and a flag table (name,
 // type, required, usage + enum), plus a body-only (--data) note when the
-// command has nested JSON-only fields. Required-ness and enums are sourced from
-// the authoritative "Request fields:" text in each command's Long; the flag
-// list falls back to the dump's Flags when no such block exists (read-only
+// command has nested JSON-only fields, plus a one-line response-shape summary
+// (top-level object vs. bare array vs. `{items: [...]}` page wrapper, and the
+// field names at that level) when the command documents one. Required-ness
+// and enums are sourced from the authoritative "Request fields:" text in each
+// command's Long; the response shape is likewise sourced from that same
+// Long's "Response fields (...):" block (cligen's own ground truth — see
+// responseShapeLine), not re-derived or hand-curated. The flag list falls
+// back to the dump's Flags when no Request-fields block exists (read-only
 // verbs). Output is deterministic.
 func GenerateFence(d Dump, group string) string {
 	cmds := groupCommands(d, group)
@@ -84,6 +89,9 @@ func writeCommand(b *strings.Builder, c Command) {
 	if len(fields.bodyOnly) > 0 {
 		fmt.Fprintf(b, "- body-only (`--data`): %s\n", strings.Join(fields.bodyOnly, "; "))
 	}
+	if shape := responseShapeLine(c.Long); shape != "" {
+		b.WriteString(shape)
+	}
 }
 
 // positionalsOf returns the placeholder tokens after the leaf verb in a Use
@@ -295,3 +303,119 @@ func cleanUsage(tail string) string {
 	s = strings.TrimPrefix(s, "—")
 	return strings.TrimSpace(s)
 }
+
+// --- Long "Response fields:" parser -----------------------------------------
+//
+// cligen classifies every documented response into exactly one of three
+// envelope shapes and says so verbatim in the "Response fields (...):" header
+// it writes into Long — this parser only ever recognizes those three; it does
+// not infer a shape of its own. That header (and the field list under it) is
+// authoritative today but surfaces only via `--help`, which an agent that
+// reads just the card fence never invokes. responseShapeLine folds a
+// one-line summary of it into the fence so every generated verb — not only
+// the handful some earlier hand-written card happened to cover — tells the
+// agent up front whether `--json` is a bare array, a single object, or a
+// `{items: [...]}` page wrapper, and exactly which field names exist at that
+// level. Guessing a field name silently returns null instead of an error, so
+// this is the difference between an agent noticing its own mistake and not.
+
+// responseHeaderRe matches the header line, capturing the parenthetical shape
+// description cligen wrote (verbatim, no leading indent — it starts a new
+// paragraph in Long).
+var responseHeaderRe = regexp.MustCompile(`^Response fields \((.*)\):$`)
+
+// responseFieldRe matches one Response-fields bullet row at any indent depth,
+// e.g. "  - account_id (integer) (required) — ..." or, one level deeper,
+// "    - person_ids (array<integer>) ...". Capture groups: indent, name, type.
+var responseFieldRe = regexp.MustCompile(`^( *)- ([a-zA-Z0-9_]+) \(([^)]*)\)`)
+
+// wrapperWireNames are the exact wire names cligen's own listEnvelope
+// (internal/cmd/cligen/main.go) treats as a paginated-list envelope field:
+// a sole array-typed sibling named items, docs, or list. Mirrored here as a
+// sanity check, not a duplicate classifier — see the guard in
+// responseShapeLine below.
+var wrapperWireNames = map[string]bool{"items": true, "docs": true, "list": true}
+
+// respField is one parsed Response-fields bullet row.
+type respField struct{ name, typ string }
+
+// responseShapeLine renders the one-line response-shape summary for a
+// command's Long, or "" when Long documents no Response fields block (mutation
+// verbs with an empty body, and a few hand-written commands that predate
+// cligen). The three shapes cligen's header can name:
+//
+//   - top-level object: the block's own fields are the response.
+//   - top-level array: `--json` is a bare array of these row objects — pipe
+//     `jq '.[]'`, never `.items[]`.
+//   - `{items: [...]}` page wrapper: the block's sole top-level field is
+//     `items`; the row fields are nested one level (2 spaces) deeper under it.
+//
+// Field names (with their documented type) are read from whichever indent
+// depth holds the actual row/object fields for the detected shape, so the
+// summary always names the fields an agent would pipe `jq` at — not the
+// wrapper key.
+func responseShapeLine(long string) string {
+	lines := strings.Split(long, "\n")
+	headerIdx, header := -1, ""
+	for i, line := range lines {
+		if m := responseHeaderRe.FindStringSubmatch(line); m != nil {
+			headerIdx, header = i, m[1]
+			break
+		}
+	}
+	if headerIdx < 0 {
+		return ""
+	}
+
+	wrapped := strings.Contains(header, "nested under items[]")
+	fieldIndent := "  "
+	if wrapped {
+		fieldIndent = "    " // one level under the sole top-level "items" row
+	}
+
+	var fields []respField
+	for _, line := range lines[headerIdx+1:] {
+		if strings.TrimSpace(line) == "" {
+			break
+		}
+		m := responseFieldRe.FindStringSubmatch(line)
+		if m == nil || m[1] != fieldIndent {
+			continue
+		}
+		fields = append(fields, respField{m[2], m[3]})
+	}
+	if len(fields) == 0 {
+		return ""
+	}
+
+	// Safety net: this parser only ever recognizes the wrapped shape by the
+	// literal substring "nested under items[]" in the header (see the doc
+	// comment above the const block). If cligen's wording for that header
+	// ever drifts without this parser being updated to match, `wrapped` goes
+	// false here even though the response really is a page wrapper — and the
+	// sole top-level field is then exactly one of cligen's own wrapper wire
+	// names (items/docs/list, from listEnvelope in cligen/main.go), holding
+	// an array. Asserting "single object" in that case would be confidently
+	// WRONG about the one thing this whole feature exists to get right, so
+	// refuse to guess: say nothing rather than assert a shape we can no
+	// longer be sure of. A missing line is recoverable via `--help`; a
+	// wrong one isn't.
+	if !wrapped && len(fields) == 1 && wrapperWireNames[fields[0].name] && strings.HasPrefix(fields[0].typ, "array") {
+		return ""
+	}
+
+	var shape string
+	switch {
+	case wrapped:
+		shape = "`{items: [...]}` page wrapper — pipe `--json | jq '.items[]'` (NOT top-level `.[]`)"
+	case strings.Contains(header, "TOP-LEVEL array"):
+		shape = "TOP-LEVEL array — pipe `--json | jq '.[]'` (NOT `.items[]`)"
+	default:
+		shape = "single object (`data` unwrapped to the top level)"
+	}
+	names := make([]string, len(fields))
+	for i, f := range fields {
+		names[i] = f.name + " (" + f.typ + ")"
+	}
+	return fmt.Sprintf("- response: %s — fields: %s\n", shape, strings.Join(names, "; "))
+}

internal/skilldoc/report_evidence_binding_test.go

@@ -0,0 +1,91 @@
+package skilldoc
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// TestSkillCardBindsEvidenceToScopeNotJustVerb locks in the generalized
+// evidence-binding rule: a claim about a specific time window or entity may
+// only be made if a tool call this turn actually covered that window or
+// entity, not merely the same verb. This patches the gap behind three
+// production failures — reporting per-day/WoW figures from a rolling-window
+// call, inventing a baseline window no call touched, and generalizing one
+// entity's config from its siblings' — all of which passed the older,
+// narrower "did you run the command at all" check.
+func TestSkillCardBindsEvidenceToScopeNotJustVerb(t *testing.T) {
+	card, err := os.ReadFile(filepath.Join("..", "..", "skills", "flashduty", "SKILL.md"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	body := string(card)
+
+	_, section, found := strings.Cut(body, "## Output — prefer `toon`")
+	if !found {
+		t.Fatal("SKILL.md is missing the Output section that carries the evidence-binding rule")
+	}
+	section, _, found = strings.Cut(section, "## Command names")
+	if !found {
+		t.Fatal("SKILL.md is missing the section after Output — prefer `toon`")
+	}
+
+	// The rule must bind a claim to the scope actually queried (time window,
+	// entity) — not just to having run the right verb at some point this
+	// turn. A call for one window or entity must not license a claim about
+	// a different one.
+	if !strings.Contains(section, "scope") {
+		t.Error("SKILL.md evidence-binding rule must talk about matching the queried scope, not just the verb")
+	}
+	if !strings.Contains(section, "window") || !strings.Contains(section, "entity") {
+		t.Error("SKILL.md evidence-binding rule must name both axes it covers: time window and entity")
+	}
+	// It must forbid generalizing from adjacent evidence (a wider/different
+	// window, a sibling entity) — the specific failure mode this rule exists
+	// to stop, not just "don't invent from nothing".
+	if !strings.Contains(section, "does not transfer") && !strings.Contains(section, "extrapolat") {
+		t.Error("SKILL.md evidence-binding rule must forbid extrapolating a claim from a window or entity you queried differently")
+	}
+	// It must give a concrete fallback action, mirroring incident.md's
+	// established phrasing, not just a prohibition with nothing to do
+	// instead.
+	if !strings.Contains(section, "未查询") || !strings.Contains(section, "<command>") {
+		t.Error("SKILL.md evidence-binding rule must give a concrete fallback action (未查询 — 可运行 <command>), not just a prohibition")
+	}
+}
+
+// TestInsightCardTiesWindowComparisonsToSkillRule locks in the insight-card
+// instance of the same rule: day-over-day / week-over-week claims require a
+// single call spanning every window compared, with --aggregate-unit as the
+// concrete way to get one. This is the domain-specific reinforcement, not a
+// duplicate of the general SKILL.md rule.
+func TestInsightCardTiesWindowComparisonsToSkillRule(t *testing.T) {
+	card, err := os.ReadFile(filepath.Join("..", "..", "skills", "flashduty", "reference", "insight.md"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	body := string(card)
+
+	_, gotchas, found := strings.Cut(body, "## Gotchas")
+	if !found {
+		t.Fatal("insight card is missing the Gotchas section")
+	}
+	gotchas, _, found = strings.Cut(gotchas, "## Worked example")
+	if !found {
+		t.Fatal("insight card is missing the section after Gotchas")
+	}
+
+	if !strings.Contains(gotchas, "window") {
+		t.Error("insight Gotchas must warn that a window-over-window claim needs a call spanning every window compared")
+	}
+	if !strings.Contains(gotchas, "--aggregate-unit") {
+		t.Error("insight Gotchas must point to --aggregate-unit as the concrete way to get real per-bucket figures in one call")
+	}
+	// It should point back to the general rule rather than re-deriving it —
+	// SKILL.md and insight.md must not carry two competing versions of the
+	// same rule.
+	if !strings.Contains(gotchas, "SKILL.md") {
+		t.Error("insight Gotchas should reference the general SKILL.md evidence-binding rule instead of restating it")
+	}
+}