Skip to content

Commit 2af6f67

Browse files
committed
fix(skilldoc): never assert single-object shape on a missed wrapper header
responseShapeLine only recognized the items[]-wrapper shape by the literal substring "nested under items[]" in cligen's header. If that header wording ever drifts, the classification silently falls through to "single object" even though the sole top-level field is exactly one of cligen's own list-envelope wire names (items/docs/list, array type, per listEnvelope in internal/cmd/cligen/main.go) — asserting the one shape claim this feature exists to get right, wrong, with the generator's own authority behind it. Guard against that: when the fallback classification's sole field matches a known wrapper wire name, emit nothing instead of asserting "single object". A missing line is recoverable via --help; a confidently wrong one is not. Verified this guard is a no-op against every real command today (cligen's own header already says "wrapped" for every such case) — it only protects against future drift. Also generalizes the ground-truth cross-check from one hand-picked command (schedule list) to every real command that documents a Response fields block (~200), each rendered as an isolated single-command fence to avoid same-named-verb collisions across subgroups (e.g. "incident get" vs "incident war-room get" both heading "### get" in the full group fence). Classification drift anywhere now fails the build, not just for the one verb someone happened to write a test against.
1 parent 148eedb commit 2af6f67

3 files changed

Lines changed: 235 additions & 76 deletions

File tree

‎internal/cmd/skilldoc/main_test.go‎

Lines changed: 124 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -158,53 +158,16 @@ func TestRunGenAll_FillsEveryCardAndSkipsCardless(t *testing.T) {
158158
}
159159
}
160160

161-
// sectionFor returns the slice of a generated fence from "### <verb>" up to
162-
// the next "### " heading (or end of string). Local copy of the helper
163-
// skilldoc's own tests use — kept private to each package since exporting it
164-
// would blur GenerateFence's real API surface just for test convenience.
165-
func sectionFor(fence, verb string) string {
166-
start := strings.Index(fence, "### "+verb)
167-
if start < 0 {
168-
return ""
169-
}
170-
rest := fence[start+len("### "+verb):]
171-
if next := strings.Index(rest, "\n### "); next >= 0 {
172-
return fence[start : start+len("### "+verb)+next]
173-
}
174-
return fence[start:]
175-
}
176-
177-
// TestGenerateFence_ScheduleList_ResponseShapeMatchesRealLong is the
178-
// ground-truth cross-check the response-shape feature exists for. Before this
179-
// change, skills/flashduty/reference/schedule.md carried zero envelope
180-
// guidance for any of its verbs (unlike incident.md/change.md/automation.md/
181-
// enrichment.md/monit.md, the only 5 hand-written cards that happened to
182-
// note their envelope shape). `schedule list` in particular is real,
183-
// commonly-invoked, and — per the actual live CLI tree, not a fixture —
184-
// documents an `{items: [...]}` page wrapper, NOT a bare top-level array
185-
// (that phrasing belongs to the deprecated `oncall schedule list` twin, which
186-
// calls the same SDK method but is a different command path with no card).
187-
//
188-
// The expected shape/fields below are derived by independently re-scanning
189-
// that real Long text with throwaway logic — not by calling skilldoc's own
190-
// extractor and not by pasting a literal expected string — so this fails if
191-
// the generator's extraction ever silently drifts from what cligen actually
192-
// wrote for this command.
193-
func TestGenerateFence_ScheduleList_ResponseShapeMatchesRealLong(t *testing.T) {
194-
d := dump()
195-
196-
var long string
197-
found := false
198-
for _, c := range d.Commands {
199-
if c.Path == "schedule list" {
200-
long, found = c.Long, true
201-
break
202-
}
203-
}
204-
if !found {
205-
t.Fatal("schedule list not found in the real CLI dump — has it been renamed?")
206-
}
207-
161+
// independentlyClassifyResponse re-derives a command's response envelope
162+
// shape ("object" | "array" | "wrapped") and its top-level (or, for the
163+
// wrapped shape, per-row) field names straight from its raw Long text, using
164+
// logic deliberately NOT shared with skilldoc's own responseShapeLine
165+
// extractor (internal/skilldoc/generate.go) — a from-scratch re-read of the
166+
// same ground truth, not a call into the code under test. ok is false when
167+
// Long documents no Response fields block, or the block yields zero fields
168+
// at the target depth (skilldoc's own extractor also emits nothing for that
169+
// case — nothing to cross-check).
170+
func independentlyClassifyResponse(long string) (shape string, fields []string, ok bool) {
208171
lines := strings.Split(long, "\n")
209172
headerLine := -1
210173
for i, l := range lines {
@@ -214,44 +177,132 @@ func TestGenerateFence_ScheduleList_ResponseShapeMatchesRealLong(t *testing.T) {
214177
}
215178
}
216179
if headerLine < 0 {
217-
t.Fatal("schedule list's real Long carries no Response fields block — has cligen's output changed?")
180+
return "", nil, false
218181
}
219-
if !strings.Contains(lines[headerLine], "nested under items[]") {
220-
t.Fatalf("expected schedule list to be an items[]-wrapped page response; real header was:\n%s", lines[headerLine])
182+
183+
header := lines[headerLine]
184+
switch {
185+
case strings.Contains(header, "nested under items[]"):
186+
shape = "wrapped"
187+
case strings.Contains(header, "TOP-LEVEL array"):
188+
shape = "array"
189+
default:
190+
shape = "object"
191+
}
192+
prefix := " - "
193+
if shape == "wrapped" {
194+
prefix = " - " // one level under the sole top-level "items" row
221195
}
222-
var wantFields []string
223196
for _, l := range lines[headerLine+1:] {
224197
if strings.TrimSpace(l) == "" {
225198
break
226199
}
227-
if strings.HasPrefix(l, " - ") { // one level under the sole top-level "items" row
228-
name := strings.TrimPrefix(l, " - ")
229-
if sp := strings.IndexAny(name, " ("); sp >= 0 {
230-
name = name[:sp]
231-
}
232-
wantFields = append(wantFields, name)
200+
if !strings.HasPrefix(l, prefix) {
201+
continue
233202
}
203+
name := strings.TrimPrefix(l, prefix)
204+
if sp := strings.IndexAny(name, " ("); sp >= 0 {
205+
name = name[:sp]
206+
}
207+
fields = append(fields, name)
234208
}
235-
if len(wantFields) == 0 {
236-
t.Fatal("independent scan of the real Long found no row fields under items — test logic is broken")
237-
}
209+
return shape, fields, len(fields) > 0
210+
}
238211

239-
fresh := skilldoc.GenerateFence(d, "schedule")
240-
listSection := sectionFor(fresh, "list")
241-
if listSection == "" {
242-
t.Fatal("generated schedule fence has no `list` section")
243-
}
244-
if !strings.Contains(listSection, "page wrapper") || !strings.Contains(listSection, "jq '.items[]'") {
245-
t.Errorf("schedule list card section must document the items[] page wrapper, got:\n%s", listSection)
246-
}
247-
if strings.Contains(listSection, "TOP-LEVEL array") {
248-
t.Errorf("schedule list is NOT a top-level array — must not carry that phrasing:\n%s", listSection)
212+
// isCligenWrapperWireName mirrors (independently — not by import) the three
213+
// wire names cligen's own listEnvelope (internal/cmd/cligen/main.go) treats
214+
// as a paginated-list envelope field.
215+
func isCligenWrapperWireName(name string) bool {
216+
return name == "items" || name == "docs" || name == "list"
217+
}
218+
219+
// responseLineOf returns the "- response: ..." bullet inside a rendered fence
220+
// section, and whether one was present.
221+
func responseLineOf(section string) (string, bool) {
222+
for _, l := range strings.Split(section, "\n") {
223+
if strings.HasPrefix(strings.TrimSpace(l), "- response: ") {
224+
return l, true
225+
}
249226
}
250-
for _, f := range wantFields {
251-
if !strings.Contains(listSection, f+" (") {
252-
t.Errorf("schedule list card section missing real row field %q (from live Long):\n%s", f, listSection)
227+
return "", false
228+
}
229+
230+
// TestGenerateFence_ResponseShapeMatchesRealLong_AllCommands is the
231+
// coverage-complete ground-truth cross-check: for every real command whose
232+
// live Long (built from the actual CLI tree, not a fixture) documents a
233+
// Response fields block, independently reclassify its envelope shape and
234+
// field names (independentlyClassifyResponse, above — separate logic from
235+
// the generator) and assert the fence GenerateFence renders for that verb
236+
// agrees. A single hand-picked example (`schedule list`) proved the
237+
// mechanism works but only ever covered one of ~200 documented commands;
238+
// this walks the whole real dump, so a classification drift ANYWHERE in the
239+
// generator fails the build, not just for the one verb someone happened to
240+
// write a test against.
241+
func TestGenerateFence_ResponseShapeMatchesRealLong_AllCommands(t *testing.T) {
242+
d := dump()
243+
244+
checked := 0
245+
for _, c := range d.Commands {
246+
wantShape, wantFields, hasBlock := independentlyClassifyResponse(c.Long)
247+
if !hasBlock {
248+
continue
249+
}
250+
checked++
251+
252+
// Render this ONE command in isolation (a single-command dump filtered
253+
// to its own group) rather than slicing a section out of the whole
254+
// group's fence: several real groups (e.g. "incident") flatten
255+
// same-named leaves from different subgroups — "incident get" and
256+
// "incident war-room get" both render as a "### get" heading — so a
257+
// substring/heading search across the full group fence can grab the
258+
// wrong command's section. A single-command fence has exactly one
259+
// response line, unambiguously.
260+
fence := skilldoc.GenerateFence(skilldoc.Dump{Commands: []skilldoc.Command{c}}, c.Group)
261+
gotLine, hasLine := responseLineOf(fence)
262+
263+
// Mirrors the wrapper-drift guard in responseShapeLine: a response
264+
// this classifier reads as a top-level object whose sole field is one
265+
// of cligen's own list-envelope wire names (items/docs/list, array
266+
// type) is a case the generator deliberately suppresses rather than
267+
// assert a possibly-wrong shape. No real command hits this today
268+
// (cligen's own header would already say "wrapped" for it), but nothing
269+
// here should hard-fail if drift ever makes one — that is the guard
270+
// working as designed, not a bug.
271+
if wantShape == "object" && len(wantFields) == 1 && isCligenWrapperWireName(wantFields[0]) {
272+
if hasLine {
273+
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)
274+
}
275+
continue
276+
}
277+
278+
if !hasLine {
279+
t.Errorf("%s: generated fence has no response line for a documented Response fields block", c.Path)
280+
continue
281+
}
282+
switch wantShape {
283+
case "wrapped":
284+
if !strings.Contains(gotLine, "page wrapper") || !strings.Contains(gotLine, "jq '.items[]'") {
285+
t.Errorf("%s: want items[] page wrapper, got:\n%s", c.Path, gotLine)
286+
}
287+
case "array":
288+
if !strings.Contains(gotLine, "TOP-LEVEL array") {
289+
t.Errorf("%s: want TOP-LEVEL array, got:\n%s", c.Path, gotLine)
290+
}
291+
default: // "object"
292+
if !strings.Contains(gotLine, "single object") {
293+
t.Errorf("%s: want single object, got:\n%s", c.Path, gotLine)
294+
}
253295
}
296+
for _, f := range wantFields {
297+
if !strings.Contains(gotLine, f+" (") {
298+
t.Errorf("%s: missing real field %q (from live Long) in generated line:\n%s", c.Path, f, gotLine)
299+
}
300+
}
301+
}
302+
if checked < 100 {
303+
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)
254304
}
305+
t.Logf("cross-checked response shape/fields for %d real commands", checked)
255306
}
256307

257308
func TestRunGen_FillsFence(t *testing.T) {

‎internal/skilldoc/generate.go‎

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -329,6 +329,16 @@ var responseHeaderRe = regexp.MustCompile(`^Response fields \((.*)\):$`)
329329
// " - person_ids (array<integer>) ...". Capture groups: indent, name, type.
330330
var responseFieldRe = regexp.MustCompile(`^( *)- ([a-zA-Z0-9_]+) \(([^)]*)\)`)
331331

332+
// wrapperWireNames are the exact wire names cligen's own listEnvelope
333+
// (internal/cmd/cligen/main.go) treats as a paginated-list envelope field:
334+
// a sole array-typed sibling named items, docs, or list. Mirrored here as a
335+
// sanity check, not a duplicate classifier — see the guard in
336+
// responseShapeLine below.
337+
var wrapperWireNames = map[string]bool{"items": true, "docs": true, "list": true}
338+
339+
// respField is one parsed Response-fields bullet row.
340+
type respField struct{ name, typ string }
341+
332342
// responseShapeLine renders the one-line response-shape summary for a
333343
// command's Long, or "" when Long documents no Response fields block (mutation
334344
// verbs with an empty body, and a few hand-written commands that predate
@@ -363,7 +373,7 @@ func responseShapeLine(long string) string {
363373
fieldIndent = " " // one level under the sole top-level "items" row
364374
}
365375

366-
var fields []string
376+
var fields []respField
367377
for _, line := range lines[headerIdx+1:] {
368378
if strings.TrimSpace(line) == "" {
369379
break
@@ -372,12 +382,28 @@ func responseShapeLine(long string) string {
372382
if m == nil || m[1] != fieldIndent {
373383
continue
374384
}
375-
fields = append(fields, m[2]+" ("+m[3]+")")
385+
fields = append(fields, respField{m[2], m[3]})
376386
}
377387
if len(fields) == 0 {
378388
return ""
379389
}
380390

391+
// Safety net: this parser only ever recognizes the wrapped shape by the
392+
// literal substring "nested under items[]" in the header (see the doc
393+
// comment above the const block). If cligen's wording for that header
394+
// ever drifts without this parser being updated to match, `wrapped` goes
395+
// false here even though the response really is a page wrapper — and the
396+
// sole top-level field is then exactly one of cligen's own wrapper wire
397+
// names (items/docs/list, from listEnvelope in cligen/main.go), holding
398+
// an array. Asserting "single object" in that case would be confidently
399+
// WRONG about the one thing this whole feature exists to get right, so
400+
// refuse to guess: say nothing rather than assert a shape we can no
401+
// longer be sure of. A missing line is recoverable via `--help`; a
402+
// wrong one isn't.
403+
if !wrapped && len(fields) == 1 && wrapperWireNames[fields[0].name] && strings.HasPrefix(fields[0].typ, "array") {
404+
return ""
405+
}
406+
381407
var shape string
382408
switch {
383409
case wrapped:
@@ -387,5 +413,9 @@ func responseShapeLine(long string) string {
387413
default:
388414
shape = "single object (`data` unwrapped to the top level)"
389415
}
390-
return fmt.Sprintf("- response: %s — fields: %s\n", shape, strings.Join(fields, "; "))
416+
names := make([]string, len(fields))
417+
for i, f := range fields {
418+
names[i] = f.name + " (" + f.typ + ")"
419+
}
420+
return fmt.Sprintf("- response: %s — fields: %s\n", shape, strings.Join(names, "; "))
391421
}

‎internal/skilldoc/response_shape_test.go‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,45 @@ Request fields:
4848
--schedule-id int (required) — Schedule ID.
4949
`
5050

51+
// driftedWrapperHeaderLong simulates a future cligen wording change to the
52+
// items[]-wrapper header that no longer contains the literal substring
53+
// "nested under items[]" this parser keys on (e.g. cligen's header text was
54+
// reworded from "rows are nested under items[]" to "rows appear inside
55+
// items[]"). The response is STILL genuinely items[]-wrapped — same shape as
56+
// itemsWrappedShapeLong above, just described differently — so this must
57+
// never be reported as "single object".
58+
const driftedWrapperHeaderLong = `List schedules.
59+
60+
Response fields ('data' envelope is unwrapped — rows appear inside items[]):
61+
- items (array<object>) (required) — Schedules on this page.
62+
- schedule_id (integer) (required) — Schedule ID.
63+
`
64+
65+
// driftedWrapperHeaderDocsLong is the same drift scenario but with cligen's
66+
// "docs" wire name (listEnvelope in internal/cmd/cligen/main.go recognizes
67+
// items/docs/list interchangeably as a list-envelope field) instead of
68+
// "items", to prove the guard isn't hardcoded to one wire name.
69+
const driftedWrapperHeaderDocsLong = `List reports.
70+
71+
Response fields ('data' envelope is unwrapped — rows appear inside docs[]):
72+
- docs (array<object>) (required) — Reports on this page.
73+
- report_id (integer) (required) — Report ID.
74+
`
75+
76+
// singleArrayFieldObjectLong is a GENUINE single-object response (per
77+
// cligen's own header, and correctly so — this mirrors the real
78+
// Automations.RuleReadList shape, whose sole top-level field is "rules", not
79+
// one of cligen's own list-envelope wire names) whose one top-level field
80+
// happens to be an array. The guard must NOT suppress this: only the exact
81+
// wrapper wire names (items/docs/list) are a drift signal, not "any object
82+
// with one array field".
83+
const singleArrayFieldObjectLong = `List automation rules.
84+
85+
Response fields ('data' envelope is unwrapped — these fields are at the top level):
86+
- rules (array<object>) (required) — Automation rules.
87+
- rule_id (string) (required) — Rule ID.
88+
`
89+
5190
func TestResponseShapeLine_Object(t *testing.T) {
5291
got := responseShapeLine(objectShapeLong)
5392
if !strings.Contains(got, "single object") {
@@ -112,6 +151,45 @@ func TestResponseShapeLine_NoBlockIsEmpty(t *testing.T) {
112151
}
113152
}
114153

154+
// TestResponseShapeLine_DriftedWrapperHeaderYieldsNothingNotWrongClaim covers
155+
// the failure mode a reviewer traced: if cligen's items[]-wrapper header
156+
// wording ever drifts away from the literal "nested under items[]" substring
157+
// this parser matches on, the shape must NOT silently fall through to
158+
// "single object" — that would be the exact confidently-wrong claim this
159+
// whole feature exists to prevent. It must instead emit nothing. Covers both
160+
// wrapper wire names cligen's listEnvelope recognizes: "items" and "docs".
161+
func TestResponseShapeLine_DriftedWrapperHeaderYieldsNothingNotWrongClaim(t *testing.T) {
162+
for _, tc := range []struct {
163+
name string
164+
long string
165+
}{
166+
{"items", driftedWrapperHeaderLong},
167+
{"docs", driftedWrapperHeaderDocsLong},
168+
} {
169+
t.Run(tc.name, func(t *testing.T) {
170+
got := responseShapeLine(tc.long)
171+
if got != "" {
172+
t.Errorf("drifted wrapper header must yield no response line (not a false 'single object' claim), got:\n%s", got)
173+
}
174+
})
175+
}
176+
}
177+
178+
// TestResponseShapeLine_SingleArrayFieldObjectIsNotSuppressed proves the
179+
// drift guard is scoped to cligen's own wrapper wire names (items/docs/list),
180+
// not "any object shape with exactly one array field" — a real, correctly
181+
// classified single-object response (mirroring Automations.RuleReadList,
182+
// whose sole field is "rules") must still render normally.
183+
func TestResponseShapeLine_SingleArrayFieldObjectIsNotSuppressed(t *testing.T) {
184+
got := responseShapeLine(singleArrayFieldObjectLong)
185+
if !strings.Contains(got, "single object") {
186+
t.Errorf("a genuine single-object response with one array field must not be suppressed by the wrapper-drift guard:\n%s", got)
187+
}
188+
if !strings.Contains(got, "rules (array<object>)") {
189+
t.Errorf("missing the rules field:\n%s", got)
190+
}
191+
}
192+
115193
// TestGenerateFence_InjectsResponseShapePerVerb is the fence-level integration
116194
// check: a group with one verb of each shape must surface a "- response: "
117195
// line in that verb's own section, and a verb with no documented response

0 commit comments

Comments
 (0)