Skip to content

Commit 3b2baa4

Browse files
ysyneuysyneu
committed
refactor(skilldoc): simplify-pass — segment-bounded prefixes, single corpus load, FindFence
- Prefix claims are now hyphen-boundary-bounded (verb == p or p + "-"): an unbounded match could misroute a near-miss verb (rule2-list under rule-) with a clean single-owner partition no topology check would flag. Fence ids normalized to the boundary-safe form (incident[post-mortem]); a trailing-hyphen prefix now claims nothing and dies loudly as a dead-prefix violation. - runGenAll loads the card corpus once and threads it through every group via genGroup (was one full reload per group); genGroup updates the in-memory docs after each write so later groups see current content. runGen's docOrder/byPath bookkeeping dropped — it iterates the docs slice directly. - New skilldoc.FindFence(body, id) centralizes start/end-marker location for both gen and check (the two ad-hoc copies had already drifted in error wording); hasCatchAll bool and a no-op map insert removed; CheckFences reuses groups(d)'s existing order instead of re-sorting.
1 parent 0ba6179 commit 3b2baa4

6 files changed

Lines changed: 124 additions & 86 deletions

File tree

‎internal/cmd/skilldoc/main.go‎

Lines changed: 46 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -81,92 +81,90 @@ func checkCmd() *cobra.Command {
8181
// dump builds the command-tree dump from the live CLI root, in-process.
8282
func dump() skilldoc.Dump { return skilldoc.Build(cli.RootForDump()) }
8383

84-
// runGen regenerates every GENERATED fence of group across the cards under
85-
// <base>, leaving hand-written content outside the fences untouched. A group
86-
// may split its fences across cards (subset fences claiming verb prefixes,
87-
// plus the catch-all for the rest — see skilldoc.RenderGroupFences), so the
88-
// fresh render is computed for the group as a whole, then spliced per card.
89-
func runGen(d skilldoc.Dump, base, group string) error {
90-
docs, err := loadDocs(base)
91-
if err != nil {
92-
return err
93-
}
94-
84+
// genGroup regenerates every GENERATED fence of group across the already-
85+
// loaded docs, leaving hand-written content outside the fences untouched. A
86+
// group may split its fences across cards (subset fences claiming verb
87+
// prefixes, plus the catch-all for the rest — see skilldoc.RenderGroupFences),
88+
// so the fresh render is computed for the group as a whole, then spliced per
89+
// card. Rewritten bodies are written to disk AND updated in docs, so a caller
90+
// looping over groups keeps seeing current content. found is false when no
91+
// fence of the group exists anywhere.
92+
func genGroup(d skilldoc.Dump, base string, docs []skilldoc.Doc, group string) (found bool, err error) {
9593
var ids []string
9694
perDoc := map[string][]string{}
97-
var docOrder []string
9895
for _, doc := range docs {
9996
for _, fl := range skilldoc.FenceLocs(doc.Body) {
10097
spec, err := skilldoc.ParseFenceID(fl.ID)
10198
if err != nil {
102-
return fmt.Errorf("%s: %w", doc.Path, err)
99+
return false, fmt.Errorf("%s: %w", doc.Path, err)
103100
}
104101
if spec.Group != group {
105102
continue
106103
}
107-
if len(perDoc[doc.Path]) == 0 {
108-
docOrder = append(docOrder, doc.Path)
109-
}
110104
perDoc[doc.Path] = append(perDoc[doc.Path], fl.ID)
111105
ids = append(ids, fl.ID)
112106
}
113107
}
114108
if len(ids) == 0 {
115-
return fmt.Errorf("no GENERATED:%s fence found under %s (add the start/end markers first)", group, base)
109+
return false, nil
116110
}
117111

118112
rendered, violations := skilldoc.RenderGroupFences(d, group, ids)
119113
if len(violations) > 0 {
120-
return fmt.Errorf("group %s fence topology: %s", group, strings.Join(violations, "; "))
114+
return true, fmt.Errorf("group %s fence topology: %s", group, strings.Join(violations, "; "))
121115
}
122116

123-
byPath := map[string]skilldoc.Doc{}
124-
for _, doc := range docs {
125-
byPath[doc.Path] = doc
126-
}
127-
for _, p := range docOrder {
128-
doc := byPath[p]
117+
for i, doc := range docs {
118+
docIDs := perDoc[doc.Path]
119+
if len(docIDs) == 0 {
120+
continue
121+
}
129122
body := doc.Body
130-
for _, id := range perDoc[p] {
131-
start, end := skilldoc.FenceStart(id), skilldoc.FenceEnd(id)
132-
si := strings.Index(body, start)
133-
if si < 0 {
134-
return fmt.Errorf("%s: unterminated GENERATED:%s fence", p, id)
123+
for _, id := range docIDs {
124+
start, end, ok := skilldoc.FindFence(body, id)
125+
if !ok {
126+
return true, fmt.Errorf("%s: unterminated GENERATED:%s fence", doc.Path, id)
135127
}
136-
ei := strings.Index(body[si:], end)
137-
if ei < 0 {
138-
return fmt.Errorf("%s: unterminated GENERATED:%s fence", p, id)
139-
}
140-
body = body[:si] + rendered[id] + body[si+ei+len(end):]
128+
body = body[:start] + rendered[id] + body[end:]
141129
}
142130
if body == doc.Body {
143131
continue // already fresh
144132
}
145-
if err := os.WriteFile(filepath.Join(base, p), []byte(body), 0o644); err != nil {
146-
return fmt.Errorf("write card: %w", err)
133+
if err := os.WriteFile(filepath.Join(base, doc.Path), []byte(body), 0o644); err != nil {
134+
return true, fmt.Errorf("write card: %w", err)
147135
}
136+
docs[i].Body = body
137+
}
138+
return true, nil
139+
}
140+
141+
// runGen regenerates one group's fences.
142+
func runGen(d skilldoc.Dump, base, group string) error {
143+
docs, err := loadDocs(base)
144+
if err != nil {
145+
return err
146+
}
147+
found, err := genGroup(d, base, docs, group)
148+
if err != nil {
149+
return err
150+
}
151+
if !found {
152+
return fmt.Errorf("no GENERATED:%s fence found under %s (add the start/end markers first)", group, base)
148153
}
149154
return nil
150155
}
151156

152157
// runGenAll regenerates the fences of every dump group that has at least one
153158
// GENERATED marker in a card under <base>. The group set is derived from the
154-
// dump (intersected with the fences that actually exist), so it stays correct
155-
// as domains are added or renamed — no hardcoded list. Groups without any
156-
// fence (e.g. webhook) are skipped.
159+
// dump (intersected with the fences that actually exist, which genGroup
160+
// reports via found), so it stays correct as domains are added or renamed —
161+
// no hardcoded list. Groups without any fence (e.g. webhook) are skipped.
162+
// The corpus is loaded once and threaded through every group.
157163
func runGenAll(d skilldoc.Dump, base string) error {
158164
docs, err := loadDocs(base)
159165
if err != nil {
160166
return err
161167
}
162-
withFence := map[string]bool{}
163-
for _, doc := range docs {
164-
for _, fl := range skilldoc.FenceLocs(doc.Body) {
165-
if spec, err := skilldoc.ParseFenceID(fl.ID); err == nil {
166-
withFence[spec.Group] = true
167-
}
168-
}
169-
}
170168

171169
seen := map[string]bool{}
172170
var groups []string
@@ -178,10 +176,7 @@ func runGenAll(d skilldoc.Dump, base string) error {
178176
}
179177
sort.Strings(groups)
180178
for _, g := range groups {
181-
if !withFence[g] {
182-
continue
183-
}
184-
if err := runGen(d, base, g); err != nil {
179+
if _, err := genGroup(d, base, docs, g); err != nil {
185180
return fmt.Errorf("gen %s: %w", g, err)
186181
}
187182
}

‎internal/cmd/skilldoc/main_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -350,7 +350,7 @@ func TestRunGen_SplitAcrossCards(t *testing.T) {
350350

351351
rules := filepath.Join(dir, "reference", "rules.md")
352352
svc := filepath.Join(dir, "reference", "svc.md")
353-
writeFile(t, rules, "# rules\n\n"+skilldoc.FenceStart("svc[rule-]")+"\n"+skilldoc.FenceEnd("svc[rule-]")+"\n")
353+
writeFile(t, rules, "# rules\n\n"+skilldoc.FenceStart("svc[rule]")+"\n"+skilldoc.FenceEnd("svc[rule]")+"\n")
354354
writeFile(t, svc, "# svc\n\nintro\n\n"+skilldoc.FenceStart("svc")+"\n"+skilldoc.FenceEnd("svc")+"\n")
355355

356356
if err := runGen(d, dir, "svc"); err != nil {
@@ -392,7 +392,7 @@ func TestRunGen_TopologyViolationFails(t *testing.T) {
392392

393393
// Subset fence only — "list" has no home.
394394
writeFile(t, filepath.Join(dir, "reference", "rules.md"),
395-
"# rules\n\n"+skilldoc.FenceStart("svc[rule-]")+"\n"+skilldoc.FenceEnd("svc[rule-]")+"\n")
395+
"# rules\n\n"+skilldoc.FenceStart("svc[rule]")+"\n"+skilldoc.FenceEnd("svc[rule]")+"\n")
396396

397397
err := runGen(d, dir, "svc")
398398
if err == nil || !strings.Contains(err.Error(), "no catch-all") {
@@ -413,7 +413,7 @@ func TestRunGen_TwoFencesInOneFile(t *testing.T) {
413413

414414
card := filepath.Join(dir, "reference", "svc.md")
415415
writeFile(t, card, "# svc\n\nrules first\n\n"+
416-
skilldoc.FenceStart("svc[rule-]")+"\n"+skilldoc.FenceEnd("svc[rule-]")+"\n\nthen the rest\n\n"+
416+
skilldoc.FenceStart("svc[rule]")+"\n"+skilldoc.FenceEnd("svc[rule]")+"\n\nthen the rest\n\n"+
417417
skilldoc.FenceStart("svc")+"\n"+skilldoc.FenceEnd("svc")+"\n")
418418

419419
if err := runGen(d, dir, "svc"); err != nil {

‎internal/skilldoc/fence.go‎

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,31 @@ func FenceLocs(body string) []FenceLoc {
6868
return locs
6969
}
7070

71+
// FindFence locates the full fenced block for id in body: start is the byte
72+
// offset of the start marker, end is the offset just past the end marker.
73+
// ok is false when either marker is missing.
74+
func FindFence(body, id string) (start, end int, ok bool) {
75+
si := strings.Index(body, FenceStart(id))
76+
if si < 0 {
77+
return 0, 0, false
78+
}
79+
endMarker := FenceEnd(id)
80+
ei := strings.Index(body[si:], endMarker)
81+
if ei < 0 {
82+
return 0, 0, false
83+
}
84+
return si, si + ei + len(endMarker), true
85+
}
86+
87+
// matchesPrefix reports whether verb falls under the claim prefix p: the verb
88+
// IS p, or continues past it at a hyphen boundary — so "rule" claims
89+
// "rule-create" but never "rule2-list". An unbounded prefix match would let a
90+
// near-miss verb join the wrong card with a clean single-owner partition that
91+
// no topology check could flag.
92+
func matchesPrefix(verb, p string) bool {
93+
return verb == p || strings.HasPrefix(verb, p+"-")
94+
}
95+
7196
// RenderGroupFences renders the fenced block for every fence of one command
7297
// group. ids must be the complete set of fence ids that exist for the group
7398
// across all cards — the catch-all fence renders whatever its sibling subset
@@ -99,12 +124,9 @@ func RenderGroupFences(d Dump, group string, ids []string) (map[string]string, [
99124

100125
byID := make(map[string][]Command, len(specs))
101126
catchAll := ""
102-
hasCatchAll := false
103127
for _, s := range specs {
104-
byID[s.ID()] = nil
105128
if len(s.Prefixes) == 0 {
106129
catchAll = s.ID()
107-
hasCatchAll = true
108130
}
109131
}
110132

@@ -119,7 +141,7 @@ func RenderGroupFences(d Dump, group string, ids []string) (map[string]string, [
119141
for _, s := range specs {
120142
matched := false
121143
for _, p := range s.Prefixes {
122-
if strings.HasPrefix(verb, p) {
144+
if matchesPrefix(verb, p) {
123145
prefixHit[s.ID()+"\x00"+p] = true
124146
matched = true
125147
}
@@ -133,7 +155,7 @@ func RenderGroupFences(d Dump, group string, ids []string) (map[string]string, [
133155
violations = append(violations, fmt.Sprintf("verb %q claimed by %s", verb, strings.Join(owners, " and ")))
134156
case len(owners) == 1:
135157
byID[owners[0]] = append(byID[owners[0]], c)
136-
case hasCatchAll:
158+
case catchAll != "":
137159
byID[catchAll] = append(byID[catchAll], c)
138160
default:
139161
unclaimed = append(unclaimed, verb)

‎internal/skilldoc/fence_test.go‎

Lines changed: 37 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -45,10 +45,10 @@ func TestParseFenceID_Shapes(t *testing.T) {
4545

4646
func TestFenceLocs_FindsStartsOnly(t *testing.T) {
4747
body := "intro\n" +
48-
FenceStart("svc[rule-]") + "\ncontent\n" + FenceEnd("svc[rule-]") + "\n\nmore prose\n" +
48+
FenceStart("svc[rule]") + "\ncontent\n" + FenceEnd("svc[rule]") + "\n\nmore prose\n" +
4949
FenceStart("svc") + "\ncontent\n" + FenceEnd("svc") + "\n"
5050
locs := FenceLocs(body)
51-
if len(locs) != 2 || locs[0].ID != "svc[rule-]" || locs[1].ID != "svc" {
51+
if len(locs) != 2 || locs[0].ID != "svc[rule]" || locs[1].ID != "svc" {
5252
t.Fatalf("FenceLocs = %+v, want the two start markers in order", locs)
5353
}
5454
if locs[0].Offset >= locs[1].Offset {
@@ -67,11 +67,11 @@ func partitionDump() Dump {
6767

6868
func TestRenderGroupFences_SubsetPlusCatchAll(t *testing.T) {
6969
d := partitionDump()
70-
out, violations := RenderGroupFences(d, "svc", []string{"svc[rule-]", "svc"})
70+
out, violations := RenderGroupFences(d, "svc", []string{"svc[rule]", "svc"})
7171
if len(violations) != 0 {
7272
t.Fatalf("unexpected violations: %v", violations)
7373
}
74-
subset, catchAll := out["svc[rule-]"], out["svc"]
74+
subset, catchAll := out["svc[rule]"], out["svc"]
7575
if !strings.Contains(subset, "### rule-create") || !strings.Contains(subset, "### rule-delete") {
7676
t.Errorf("subset fence should carry the rule verbs:\n%s", subset)
7777
}
@@ -84,7 +84,7 @@ func TestRenderGroupFences_SubsetPlusCatchAll(t *testing.T) {
8484
if strings.Contains(catchAll, "### rule-create") {
8585
t.Errorf("catch-all fence must not repeat claimed verbs:\n%s", catchAll)
8686
}
87-
if !strings.HasPrefix(subset, FenceStart("svc[rule-]")) || !strings.HasSuffix(subset, FenceEnd("svc[rule-]")) {
87+
if !strings.HasPrefix(subset, FenceStart("svc[rule]")) || !strings.HasSuffix(subset, FenceEnd("svc[rule]")) {
8888
t.Errorf("subset fence markers must carry the full fence id:\n%s", subset)
8989
}
9090
}
@@ -96,9 +96,9 @@ func TestRenderGroupFences_Violations(t *testing.T) {
9696
ids []string
9797
want string // substring of some violation
9898
}{
99-
{name: "double claim", ids: []string{"svc[rule-]", "svc[rule-create]", "svc"}, want: `verb "rule-create" claimed by`},
99+
{name: "double claim", ids: []string{"svc[rule]", "svc[rule-create]", "svc"}, want: `verb "rule-create" claimed by`},
100100
{name: "dead prefix", ids: []string{"svc[nope]", "svc"}, want: `prefix "nope" claims no verb`},
101-
{name: "no catch-all", ids: []string{"svc[rule-]"}, want: "no catch-all fence for unclaimed verbs: create, list"},
101+
{name: "no catch-all", ids: []string{"svc[rule]"}, want: "no catch-all fence for unclaimed verbs: create, list"},
102102
{name: "duplicate id", ids: []string{"svc", "svc"}, want: `fence "svc" appears more than once`},
103103
{name: "foreign group", ids: []string{"other", "svc"}, want: `fence "other" does not belong to group "svc"`},
104104
}
@@ -118,21 +118,21 @@ func TestRenderGroupFences_Violations(t *testing.T) {
118118

119119
func TestCheckFences_SplitAcrossDocs(t *testing.T) {
120120
d := partitionDump()
121-
fresh, violations := RenderGroupFences(d, "svc", []string{"svc[rule-]", "svc"})
121+
fresh, violations := RenderGroupFences(d, "svc", []string{"svc[rule]", "svc"})
122122
if len(violations) != 0 {
123123
t.Fatalf("unexpected violations: %v", violations)
124124
}
125125

126126
docs := []Doc{
127-
{Path: "rules", Body: "# Rules\n\n" + fresh["svc[rule-]"] + "\n"},
127+
{Path: "rules", Body: "# Rules\n\n" + fresh["svc[rule]"] + "\n"},
128128
{Path: "svc", Body: "# Svc\n\n" + fresh["svc"] + "\n"},
129129
}
130130
if issues := CheckFences(d, docs); len(issues) != 0 {
131131
t.Errorf("fresh split fences: want 0 issues, got %+v", issues)
132132
}
133133

134134
stale := []Doc{
135-
{Path: "rules", Body: "# Rules\n\n" + FenceStart("svc[rule-]") + "\n\nWRONG\n\n" + FenceEnd("svc[rule-]") + "\n"},
135+
{Path: "rules", Body: "# Rules\n\n" + FenceStart("svc[rule]") + "\n\nWRONG\n\n" + FenceEnd("svc[rule]") + "\n"},
136136
{Path: "svc", Body: "# Svc\n\n" + fresh["svc"] + "\n"},
137137
}
138138
issues := CheckFences(d, stale)
@@ -145,8 +145,8 @@ func TestCheckFences_TopologyIssues(t *testing.T) {
145145
d := partitionDump()
146146
// A subset fence with no catch-all anywhere: the group's remaining verbs
147147
// have no home.
148-
fresh, _ := RenderGroupFences(d, "svc", []string{"svc[rule-]", "svc"})
149-
docs := []Doc{{Path: "rules", Body: "# Rules\n\n" + fresh["svc[rule-]"] + "\n"}}
148+
fresh, _ := RenderGroupFences(d, "svc", []string{"svc[rule]", "svc"})
149+
docs := []Doc{{Path: "rules", Body: "# Rules\n\n" + fresh["svc[rule]"] + "\n"}}
150150
issues := CheckFences(d, docs)
151151
if len(issues) != 1 || issues[0].Kind != "fence-topology" {
152152
t.Fatalf("want 1 fence-topology issue, got %+v", issues)
@@ -162,3 +162,28 @@ func TestCheckFences_TopologyIssues(t *testing.T) {
162162
t.Errorf("unknown group marker should be flagged: %+v", issues)
163163
}
164164
}
165+
166+
// TestRenderGroupFences_PrefixIsSegmentBounded pins the hyphen-boundary claim
167+
// semantics: "rule" claims "rule-create" but must NOT claim "rule2-list" — an
168+
// unbounded prefix match would misroute it with a clean single-owner
169+
// partition that no topology check could flag.
170+
func TestRenderGroupFences_PrefixIsSegmentBounded(t *testing.T) {
171+
mk := func(verb string) Command {
172+
return Command{Path: "svc " + verb, Group: "svc", Short: "S " + verb, Use: verb}
173+
}
174+
d := Dump{Commands: []Command{mk("rule-create"), mk("rule2-list"), mk("rule")}}
175+
out, violations := RenderGroupFences(d, "svc", []string{"svc[rule]", "svc"})
176+
if len(violations) != 0 {
177+
t.Fatalf("unexpected violations: %v", violations)
178+
}
179+
subset, catchAll := out["svc[rule]"], out["svc"]
180+
if !strings.Contains(subset, "### rule-create") || !strings.Contains(subset, "### rule\n") {
181+
t.Errorf("subset must claim the exact verb and its hyphen extensions:\n%s", subset)
182+
}
183+
if strings.Contains(subset, "### rule2-list") {
184+
t.Errorf("subset must not claim the near-miss verb:\n%s", subset)
185+
}
186+
if !strings.Contains(catchAll, "### rule2-list") {
187+
t.Errorf("near-miss verb must land in the catch-all:\n%s", catchAll)
188+
}
189+
}

‎internal/skilldoc/validate.go‎

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -92,14 +92,12 @@ func CheckFences(d Dump, docs []Doc) []Issue {
9292
}
9393
}
9494

95-
groupOrder := make([]string, 0, len(byGroup))
96-
for g := range byGroup {
97-
groupOrder = append(groupOrder, g)
98-
}
99-
sort.Strings(groupOrder)
100-
101-
for _, group := range groupOrder {
102-
locs := byGroup[group]
95+
// groups(d) is already sorted; every byGroup key is a member of it.
96+
for _, group := range groups(d) {
97+
locs, present := byGroup[group]
98+
if !present {
99+
continue
100+
}
103101
ids := make([]string, len(locs))
104102
for i, l := range locs {
105103
ids[i] = l.id
@@ -114,9 +112,8 @@ func CheckFences(d Dump, docs []Doc) []Issue {
114112
})
115113
}
116114
for _, l := range locs {
117-
end := FenceEnd(l.id)
118-
ei := strings.Index(l.body[l.off:], end)
119-
if ei < 0 {
115+
start, end, ok := FindFence(l.body, l.id)
116+
if !ok {
120117
issues = append(issues, Issue{
121118
Doc: l.doc,
122119
Line: lineOf(l.body, l.off),
@@ -125,8 +122,7 @@ func CheckFences(d Dump, docs []Doc) []Issue {
125122
})
126123
continue
127124
}
128-
block := l.body[l.off : l.off+ei+len(end)]
129-
if fresh, ok := rendered[l.id]; ok && block != fresh {
125+
if fresh, rok := rendered[l.id]; rok && l.body[start:end] != fresh {
130126
issues = append(issues, Issue{
131127
Doc: l.doc,
132128
Line: lineOf(l.body, l.off),

0 commit comments

Comments
 (0)