Skip to content

Commit 860f9e2

Browse files
committed
fix(skilldoc): don't misparse a description's bracketed prose as an enum
The Request-fields enum extractor matched the FIRST "[...]" bracket anywhere in a flag's tail text and treated its contents as an enum declaration. cligen's own trailing enum bracket is always the last thing on the line (before an optional constraint suffix), but a description can legitimately quote an unrelated bracket earlier in its own prose -- e.g. a regex character class ("1-40 chars of '[a-zA-Z0-9_]'"). The old code fabricated a one-value enum from that character class AND deleted it from the sentence, leaving an empty quoted pair where the class used to be. Only recognize a bracket anchored to the end of the tail (once an optional trailing "(constraint)" is set aside) as a genuine enum -- exactly where cligen's generator appends one. Anything earlier in the text is left untouched, description and constraint both. Regenerating all cards with this fix touches only field.md (the sole card affected): the fabricated enum tag is gone and the character class renders intact in the description.
1 parent 29f33b9 commit 860f9e2

3 files changed

Lines changed: 157 additions & 10 deletions

File tree

‎internal/skilldoc/generate.go‎

Lines changed: 47 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -255,10 +255,46 @@ type requestFields struct {
255255
var (
256256
flagLineRe = regexp.MustCompile(`^\s{2}--([a-z0-9-]+)\s+\S+\s*(.*)$`)
257257
bodyLineRe = regexp.MustCompile(`^\s{2}([a-z0-9_]+)\s+\(([^,)]*)[^)]*\)\s*(.*)$`)
258-
enumRe = regexp.MustCompile(`\[([^\]]+)\]`)
259258
requiredTag = "(required)"
260259
)
261260

261+
// trailingConstraintRe matches a trailing "(...)" constraint parenthetical --
262+
// cligen appends this immediately after an enum bracket when a flag has both
263+
// (internal/cmd/cligen/main.go writes f.Enum then f.Constraint, in that
264+
// order). Setting it aside first lets enumTailRe check what's left for a
265+
// genuine trailing enum bracket without the constraint's own parens getting
266+
// in the way, and lets that constraint be reattached untouched afterward.
267+
var trailingConstraintRe = regexp.MustCompile(`\s*\([^()]*\)\s*$`)
268+
269+
// enumTailRe matches a "[a, b, c]" bracket anchored to the END of its input --
270+
// exactly where cligen's own generator appends a real enum ("line += " [" +
271+
// strings.Join(f.Enum, ", ") + "]"", see cligen/main.go). Anchoring to the
272+
// end is what excludes an arbitrary "[...]" appearing earlier in a
273+
// description's free text -- e.g. a regex character class quoted inside a
274+
// constraint sentence ("1-40 chars of '[a-zA-Z0-9_]'", see
275+
// zz_generated_alert_enrichment.go's field-name flag) -- which an unanchored
276+
// match would misidentify as a one-value enum declaration AND delete from
277+
// the description, corrupting both the fabricated enum tag and the sentence.
278+
var enumTailRe = regexp.MustCompile(`\s*\[([^\]]+)\]\s*$`)
279+
280+
// splitTrailingEnum peels cligen's own trailing enum bracket, if any, off
281+
// tail: enum is the bracket's inner text ("" when there is none), and rest is
282+
// tail with that bracket removed. A trailing "(constraint)" is set aside
283+
// before looking for the bracket and reattached untouched afterward, so it
284+
// never masks a genuine enum and is never itself mistaken for one. When the
285+
// tail-most token (once any constraint is set aside) isn't a bracket at all,
286+
// enum is "" and rest is tail unchanged -- any "[...]" earlier in the text is
287+
// incidental prose, not a declared enum, and must survive intact.
288+
func splitTrailingEnum(tail string) (enum, rest string) {
289+
constraint := trailingConstraintRe.FindString(tail)
290+
body := strings.TrimSuffix(tail, constraint)
291+
loc := enumTailRe.FindStringSubmatchIndex(body)
292+
if loc == nil {
293+
return "", tail
294+
}
295+
return body[loc[2]:loc[3]], body[:loc[0]] + body[loc[1]:] + constraint
296+
}
297+
262298
// parseRequestFields extracts the per-flag required/enum/usage and the
263299
// body-only (--data) field summaries from a command's Long "Request fields:"
264300
// block. Returns empty maps when the block is absent (read-only verbs).
@@ -303,13 +339,14 @@ func parseRequestFields(long string) requestFields {
303339
return rf
304340
}
305341

306-
// parseEnum pulls the enum members out of a trailing "[a, b, c]" marker.
342+
// parseEnum pulls the enum members out of tail's genuine trailing "[a, b, c]"
343+
// marker (see splitTrailingEnum), or returns nil when tail carries none.
307344
func parseEnum(tail string) []string {
308-
m := enumRe.FindStringSubmatch(tail)
309-
if m == nil {
345+
enum, _ := splitTrailingEnum(tail)
346+
if enum == "" {
310347
return nil
311348
}
312-
parts := strings.Split(m[1], ",")
349+
parts := strings.Split(enum, ",")
313350
out := make([]string, 0, len(parts))
314351
for _, p := range parts {
315352
if v := strings.TrimSpace(p); v != "" {
@@ -319,11 +356,12 @@ func parseEnum(tail string) []string {
319356
return out
320357
}
321358

322-
// cleanUsage strips the leading em-dash, the (required) tag, and the trailing
323-
// enum bracket from a flag line's tail, leaving the human description.
359+
// cleanUsage strips the leading em-dash, the (required) tag, and tail's
360+
// genuine trailing enum bracket (see splitTrailingEnum) from a flag line's
361+
// tail, leaving the human description -- including any trailing constraint
362+
// and any incidental "[...]" that isn't cligen's own enum marker.
324363
func cleanUsage(tail string) string {
325-
s := tail
326-
s = enumRe.ReplaceAllString(s, "")
364+
_, s := splitTrailingEnum(tail)
327365
s = strings.ReplaceAll(s, requiredTag, "")
328366
s = strings.TrimSpace(s)
329367
s = strings.TrimPrefix(s, "—")

‎internal/skilldoc/generate_test.go‎

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,115 @@ func TestGenerateFence_PositionalArg(t *testing.T) {
158158
}
159159
}
160160

161+
// TestSplitTrailingEnum_IgnoresBracketedProseInsideDescription reproduces the
162+
// real `field create --field-name` bug: its description quotes a regex
163+
// character class mid-sentence ("1-40 chars of '[a-zA-Z0-9_]'") followed by
164+
// more prose and a trailing constraint. An unanchored bracket match treats
165+
// that as a one-value enum declaration, fabricating an "enum: a-zA-Z0-9_" tag
166+
// AND deleting the bracket from the sentence (leaving an empty pair of
167+
// quotes where the character class used to be). Since
168+
// the bracket isn't at the tail's end (more text and the constraint follow
169+
// it), it must be left alone entirely: no enum, description word-for-word.
170+
func TestSplitTrailingEnum_IgnoresBracketedProseInsideDescription(t *testing.T) {
171+
tail := `(required) — Machine name. Must start with a letter or underscore; 1–40 chars of '[a-zA-Z0-9_]'. Immutable after creation. (≤39 chars)`
172+
if enum := parseEnum(tail); enum != nil {
173+
t.Errorf("bracketed prose must not be parsed as an enum, got %v", enum)
174+
}
175+
got := cleanUsage(tail)
176+
want := `Machine name. Must start with a letter or underscore; 1–40 chars of '[a-zA-Z0-9_]'. Immutable after creation. (≤39 chars)`
177+
if got != want {
178+
t.Errorf("cleanUsage must leave the character class and constraint untouched:\ngot: %q\nwant: %q", got, want)
179+
}
180+
}
181+
182+
// TestSplitTrailingEnum_RecognizesGenuineTrailingEnum proves the fix doesn't
183+
// overcorrect: cligen's own trailing enum bracket ("line += " [" + ... +
184+
// "]"", cligen/main.go) -- anchored at the tail's end, nothing else
185+
// following it -- is still recognized and still stripped from the usage
186+
// text.
187+
func TestSplitTrailingEnum_RecognizesGenuineTrailingEnum(t *testing.T) {
188+
tail := `(required) — Event type. [incident, maintenance]`
189+
if enum := parseEnum(tail); !equalStrings(enum, []string{"incident", "maintenance"}) {
190+
t.Errorf("genuine trailing enum not recognized, got %v", enum)
191+
}
192+
if got, want := cleanUsage(tail), "Event type."; got != want {
193+
t.Errorf("cleanUsage should strip the genuine enum bracket:\ngot: %q\nwant: %q", got, want)
194+
}
195+
}
196+
197+
// TestSplitTrailingEnum_GenuineEnumWithTrailingConstraint covers a flag that
198+
// has BOTH an enum and a constraint (cligen writes enum then constraint, in
199+
// that order, when both are present): the enum must still be recognized and
200+
// stripped, and the constraint must survive untouched and back in its
201+
// original trailing position -- not swallowed by the enum-bracket removal.
202+
func TestSplitTrailingEnum_GenuineEnumWithTrailingConstraint(t *testing.T) {
203+
tail := `— Some usage. [a, b] (≤5 chars)`
204+
if enum := parseEnum(tail); !equalStrings(enum, []string{"a", "b"}) {
205+
t.Errorf("enum with a trailing constraint not recognized, got %v", enum)
206+
}
207+
if got, want := cleanUsage(tail), "Some usage. (≤5 chars)"; got != want {
208+
t.Errorf("cleanUsage must strip only the enum bracket, keeping the constraint:\ngot: %q\nwant: %q", got, want)
209+
}
210+
}
211+
212+
// TestSplitTrailingEnum_SingleValueEnumAtEnd covers a one-member enum (e.g.
213+
// the real `--environment-kind [byoc]`), proving the fix doesn't require a
214+
// comma to recognize a genuine trailing bracket as an enum.
215+
func TestSplitTrailingEnum_SingleValueEnumAtEnd(t *testing.T) {
216+
tail := `— Pin to a specific runner. [byoc]`
217+
if enum := parseEnum(tail); !equalStrings(enum, []string{"byoc"}) {
218+
t.Errorf("single-value trailing enum not recognized, got %v", enum)
219+
}
220+
}
221+
222+
// TestGenerateFence_BracketedProseSurvivesAlongsideGenuineEnum is the
223+
// fence-level integration check: one command with a description-embedded
224+
// character class (must NOT become an enum) and, on a different flag in the
225+
// same command, a genuine trailing enum (must still render as one) —
226+
// proving the fix distinguishes them within the same Request-fields block,
227+
// not just in isolation.
228+
func TestGenerateFence_BracketedProseSurvivesAlongsideGenuineEnum(t *testing.T) {
229+
d := Dump{Commands: []Command{{
230+
Path: "field create",
231+
Group: "field",
232+
Short: "Create field",
233+
Use: "create",
234+
Long: `Create field.
235+
236+
Request fields:
237+
--field-name string (required) — Machine name. Must start with a letter or underscore; 1–40 chars of '[a-zA-Z0-9_]'. Immutable after creation. (≤39 chars)
238+
--field-type string (required) — Field input type. [checkbox, text]
239+
`,
240+
Flags: []Flag{{Name: "field-name", Type: "string"}, {Name: "field-type", Type: "string"}},
241+
}}}
242+
sec := sectionFor(GenerateFence(d, "field"), "create")
243+
if !strings.Contains(sec, "1–40 chars of '[a-zA-Z0-9_]'") {
244+
t.Errorf("character class embedded in prose must render intact:\n%s", sec)
245+
}
246+
if strings.Contains(sec, "chars of ''") {
247+
t.Errorf("character class must not be deleted from the description:\n%s", sec)
248+
}
249+
if strings.Contains(sec, "enum: a-zA-Z0-9_") {
250+
t.Errorf("character class must not be fabricated into an enum tag:\n%s", sec)
251+
}
252+
if !strings.Contains(sec, "enum: checkbox | text") {
253+
t.Errorf("the genuine enum on the OTHER flag in the same command must still render:\n%s", sec)
254+
}
255+
}
256+
257+
// equalStrings compares two string slices for the enum-parsing tests above.
258+
func equalStrings(a, b []string) bool {
259+
if len(a) != len(b) {
260+
return false
261+
}
262+
for i := range a {
263+
if a[i] != b[i] {
264+
return false
265+
}
266+
}
267+
return true
268+
}
269+
161270
// sectionFor returns the slice of out from "### <verb>" to the next "### " or end.
162271
func sectionFor(out, verb string) string {
163272
start := strings.Index(out, "### "+verb)

‎skills/flashduty/reference/field.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ fduty field update <field-id> \
4545
Create field
4646
- `--description` string — Optional free-text description. (≤499 chars)
4747
- `--display-name` string (required) — Human-readable name. Must be unique within the account. (≤39 chars)
48-
- `--field-name` string (required) — Machine name. Must start with a letter or underscore; 1–40 chars of ''. Immutable after creation. (≤39 chars) · enum: a-zA-Z0-9_
48+
- `--field-name` string (required) — Machine name. Must start with a letter or underscore; 1–40 chars of '[a-zA-Z0-9_]'. Immutable after creation. (≤39 chars)
4949
- `--field-type` string (required) — Field input type. Immutable after creation. · enum: checkbox | multi_select | single_select | text
5050
- `--options` stringSlice — Required and non-empty for 'single_select'/'multi_select' (unique strings, each 1–200 chars). Must be omitted or empty for 'checkbox'/'text'.
5151
- `--value-type` string (required) — Stored value type. 'checkbox' requires 'bool'; 'single_select'/'multi_select'/'text' require 'string'. Immutable after creation. · enum: string | bool | float

0 commit comments

Comments
 (0)