From b20d972f782f808a8b84c15d7befcbb2ba228e20 Mon Sep 17 00:00:00 2001 From: Akeem Jenkins Date: Fri, 26 Jun 2026 23:01:58 -0600 Subject: [PATCH 1/2] fix: validate suggests absolute path for relative link errors --- internal/validate/validate.go | 54 ++++++++++++++++++- internal/validate/validate_test.go | 84 ++++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 1 deletion(-) diff --git a/internal/validate/validate.go b/internal/validate/validate.go index 408a3c5..0da6277 100644 --- a/internal/validate/validate.go +++ b/internal/validate/validate.go @@ -95,6 +95,13 @@ func validateBody(r *Report, c *concept.Concept) { // validateLinks checks that all cross-links within the bundle resolve to an // existing concept. Both absolute (/path/to/concept.md) and relative links // are checked. +// +// Relative links (without a leading /) resolve from the concept's own +// directory: a link [X](organizations/cloaked) in pages/about.md targets +// pages/organizations/cloaked, not organizations/cloaked. When such a +// relative link is broken but the same target would resolve as an absolute +// path, the error message suggests the absolute form (e.g. +// /organizations/cloaked) so authors can fix the link. func validateLinks(r *Report, b *bundle.Bundle) { for _, c := range b.Concepts { links := extractLinks(c.Body) @@ -104,12 +111,47 @@ func validateLinks(r *Report, b *bundle.Bundle) { continue // external URL or non-concept link, skip } if !b.HasConcept(target) { - r.add(c.ID, SeverityError, fmt.Sprintf("broken link: [%s] -> %s (concept %s not found)", link.Text, link.Target, target)) + // The link didn't resolve as written. Check whether the raw + // target would resolve as an absolute (bundle-root-relative) + // path; if so, the author likely intended an absolute link. + if absTarget := absoluteTarget(link.Target); absTarget != "" && b.HasConcept(absTarget) { + r.add(c.ID, SeverityError, fmt.Sprintf( + "broken link: [%s] -> %s (relative links resolve from the current concept's directory; use /%s for an absolute path)", + link.Text, link.Target, absTarget)) + } else { + r.add(c.ID, SeverityError, fmt.Sprintf( + "broken link: [%s] -> %s (concept %s not found)", + link.Text, link.Target, target)) + } } } } } +// absoluteTarget converts a raw link target into the concept ID it would +// have if treated as an absolute (bundle-root-relative) path. It strips a +// leading /, removes any fragment (#) or query (?), and drops a trailing +// .md suffix. It returns "" for external URLs or empty targets. +func absoluteTarget(raw string) string { + target := strings.TrimSpace(raw) + if target == "" || isExternalURL(target) { + return "" + } + // Strip any fragment (#section) or query (?query). + if idx := strings.IndexAny(target, "#?"); idx != -1 { + target = target[:idx] + } + if target == "" { + return "" + } + target = strings.TrimPrefix(target, "/") + target = strings.TrimSuffix(target, ".md") + if target == "" { + return "" + } + return concept.ConceptID(target) +} + // Link represents a markdown link found in a concept body. type Link struct { Text string @@ -117,6 +159,16 @@ type Link struct { } // ExtractLinks parses all markdown links [text](target) from a body. +// +// Link targets may be absolute or relative. Absolute targets begin with / +// and resolve against the bundle root (e.g. /tables/users.md -> the +// concept tables/users regardless of where the linking concept lives). +// Relative targets lack a leading / and resolve against the linking +// concept's own directory: a target organizations/cloaked in a concept at +// pages/about resolves to pages/organizations/cloaked, not +// organizations/cloaked. External URLs (http://, https://, mailto:, and +// pure-fragment links like #section) are returned as links but are ignored +// by concept resolution. func ExtractLinks(body string) []Link { return extractLinks(body) } diff --git a/internal/validate/validate_test.go b/internal/validate/validate_test.go index 3964853..4ad91c4 100644 --- a/internal/validate/validate_test.go +++ b/internal/validate/validate_test.go @@ -1,7 +1,12 @@ package validate import ( + "os" + "path/filepath" + "strings" "testing" + + "github.com/okfcli/okf/internal/bundle" ) func TestExtractLinks(t *testing.T) { @@ -84,3 +89,82 @@ func TestNormalizePath(t *testing.T) { } } } + +// --- validateLinks integration tests --- + +func TestValidateLinks_RelativeLinkSuggestsAbsolutePath(t *testing.T) { + // A concept at pages/about has [Cloaked](organizations/cloaked). The + // relative target resolves to pages/organizations/cloaked (broken), but + // the absolute target organizations/cloaked exists. The error should + // suggest the absolute path. + b := testBundle(t, map[string]string{ + "pages/about.md": "---\ntype: Page\ntitle: About\n---\n\nSee [Cloaked](organizations/cloaked).", + "organizations/cloaked.md": "---\ntype: Org\ntitle: Cloaked\n---\n\nbody", + }) + + r := &Report{} + validateLinks(r, b) + + var msg string + for _, f := range r.Findings { + if f.ConceptID == "pages/about" && strings.Contains(f.Message, "broken link") { + msg = f.Message + break + } + } + if msg == "" { + t.Fatalf("no broken-link finding for pages/about: %+v", r.Findings) + } + if !strings.Contains(msg, "use /organizations/cloaked for an absolute path") { + t.Errorf("error %q does not suggest absolute path", msg) + } +} + +func TestValidateLinks_NonexistentConceptNoAbsolutePathSuggestion(t *testing.T) { + // A link to a genuinely nonexistent concept should report "broken link:" + // but must NOT mention "absolute path". + b := testBundle(t, map[string]string{ + "pages/about.md": "---\ntype: Page\ntitle: About\n---\n\nSee [Ghost](/nonexistent/ghost).", + }) + + r := &Report{} + validateLinks(r, b) + + var msg string + for _, f := range r.Findings { + if f.ConceptID == "pages/about" && strings.Contains(f.Message, "broken link") { + msg = f.Message + break + } + } + if msg == "" { + t.Fatalf("no broken-link finding for pages/about: %+v", r.Findings) + } + if !strings.Contains(msg, "broken link:") { + t.Errorf("error %q does not contain 'broken link:'", msg) + } + if strings.Contains(msg, "absolute path") { + t.Errorf("error %q should not mention 'absolute path' for a genuinely missing concept", msg) + } +} + +// --- helpers --- + +func testBundle(t *testing.T, files map[string]string) *bundle.Bundle { + t.Helper() + dir := t.TempDir() + for path, content := range files { + full := filepath.Join(dir, path) + if err := os.MkdirAll(filepath.Dir(full), 0755); err != nil { + t.Fatalf("mkdir: %v", err) + } + if err := os.WriteFile(full, []byte(content), 0644); err != nil { + t.Fatalf("write: %v", err) + } + } + b, err := bundle.Load(dir) + if err != nil { + t.Fatalf("Load: %v", err) + } + return b +} From 2a9092d1a0a34ff06c0b2f0fbfdf472e634f2455 Mon Sep 17 00:00:00 2001 From: Akeem Jenkins Date: Wed, 8 Jul 2026 10:48:53 -0600 Subject: [PATCH 2/2] validate: simplify link suggestion, preserve #fragment Address review nits on the relative-link absolute-path suggestion: - Replace the duplicated absoluteTarget helper with resolveLink("", link), whose empty fromConceptID yields the bundle-root interpretation. Guard the suggestion on absTarget != target so an already-absolute broken link (and root-level concepts where relative and absolute coincide) falls through to the plain broken-link message rather than suggesting itself. - Preserve the #fragment on the suggested absolute path (e.g. organizations/cloaked#section -> /organizations/cloaked#section). Fragments remain ignored for concept resolution. - Add tests for the already-absolute-and-broken case (no "absolute path" suggestion) and for fragment preservation in the suggestion. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/validate/validate.go | 43 ++++++++++-------------- internal/validate/validate_test.go | 53 ++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 25 deletions(-) diff --git a/internal/validate/validate.go b/internal/validate/validate.go index 0da6277..4744910 100644 --- a/internal/validate/validate.go +++ b/internal/validate/validate.go @@ -111,13 +111,18 @@ func validateLinks(r *Report, b *bundle.Bundle) { continue // external URL or non-concept link, skip } if !b.HasConcept(target) { - // The link didn't resolve as written. Check whether the raw + // The link didn't resolve as written. Check whether the same // target would resolve as an absolute (bundle-root-relative) // path; if so, the author likely intended an absolute link. - if absTarget := absoluteTarget(link.Target); absTarget != "" && b.HasConcept(absTarget) { + // resolveLink with an empty fromConceptID yields the bundle-root + // interpretation. This only differs from the as-written result + // for relative links (a leading-/ target resolves identically + // regardless of fromConceptID), so an already-absolute broken + // link correctly falls through to the plain message below. + if absTarget := resolveLink("", link); absTarget != "" && absTarget != target && b.HasConcept(absTarget) { r.add(c.ID, SeverityError, fmt.Sprintf( - "broken link: [%s] -> %s (relative links resolve from the current concept's directory; use /%s for an absolute path)", - link.Text, link.Target, absTarget)) + "broken link: [%s] -> %s (relative links resolve from the current concept's directory; use /%s%s for an absolute path)", + link.Text, link.Target, absTarget, fragmentOf(link.Target))) } else { r.add(c.ID, SeverityError, fmt.Sprintf( "broken link: [%s] -> %s (concept %s not found)", @@ -128,28 +133,16 @@ func validateLinks(r *Report, b *bundle.Bundle) { } } -// absoluteTarget converts a raw link target into the concept ID it would -// have if treated as an absolute (bundle-root-relative) path. It strips a -// leading /, removes any fragment (#) or query (?), and drops a trailing -// .md suffix. It returns "" for external URLs or empty targets. -func absoluteTarget(raw string) string { - target := strings.TrimSpace(raw) - if target == "" || isExternalURL(target) { - return "" - } - // Strip any fragment (#section) or query (?query). - if idx := strings.IndexAny(target, "#?"); idx != -1 { - target = target[:idx] - } - if target == "" { - return "" - } - target = strings.TrimPrefix(target, "/") - target = strings.TrimSuffix(target, ".md") - if target == "" { - return "" +// fragmentOf returns the #fragment portion of a raw link target (including the +// leading #), or "" if there is none. Fragments are ignored for concept +// resolution, but preserving them in a suggested path keeps the hint faithful +// to what the author wrote (e.g. organizations/cloaked#section suggests +// /organizations/cloaked#section). +func fragmentOf(raw string) string { + if idx := strings.IndexByte(raw, '#'); idx != -1 { + return raw[idx:] } - return concept.ConceptID(target) + return "" } // Link represents a markdown link found in a concept body. diff --git a/internal/validate/validate_test.go b/internal/validate/validate_test.go index 4ad91c4..f8fae12 100644 --- a/internal/validate/validate_test.go +++ b/internal/validate/validate_test.go @@ -148,6 +148,59 @@ func TestValidateLinks_NonexistentConceptNoAbsolutePathSuggestion(t *testing.T) } } +func TestValidateLinks_AlreadyAbsoluteBrokenNoSuggestion(t *testing.T) { + // A link that is already written as an absolute path (/...) but does not + // resolve must NOT produce an "absolute path" suggestion — there is + // nothing to suggest, so it falls through to the plain broken-link message. + b := testBundle(t, map[string]string{ + "pages/about.md": "---\ntype: Page\ntitle: About\n---\n\nSee [Cloaked](/organizations/cloaked).", + }) + + r := &Report{} + validateLinks(r, b) + + var msg string + for _, f := range r.Findings { + if f.ConceptID == "pages/about" && strings.Contains(f.Message, "broken link") { + msg = f.Message + break + } + } + if msg == "" { + t.Fatalf("no broken-link finding for pages/about: %+v", r.Findings) + } + if strings.Contains(msg, "absolute path") { + t.Errorf("error %q should not suggest an absolute path for an already-absolute broken link", msg) + } +} + +func TestValidateLinks_SuggestionPreservesFragment(t *testing.T) { + // A broken relative link that carries a #fragment should retain the + // fragment in the suggested absolute path (fragments are ignored for + // resolution but kept in the displayed hint). + b := testBundle(t, map[string]string{ + "pages/about.md": "---\ntype: Page\ntitle: About\n---\n\nSee [Cloaked](organizations/cloaked#section).", + "organizations/cloaked.md": "---\ntype: Org\ntitle: Cloaked\n---\n\nbody", + }) + + r := &Report{} + validateLinks(r, b) + + var msg string + for _, f := range r.Findings { + if f.ConceptID == "pages/about" && strings.Contains(f.Message, "broken link") { + msg = f.Message + break + } + } + if msg == "" { + t.Fatalf("no broken-link finding for pages/about: %+v", r.Findings) + } + if !strings.Contains(msg, "use /organizations/cloaked#section for an absolute path") { + t.Errorf("error %q does not preserve the #fragment in the suggested path", msg) + } +} + // --- helpers --- func testBundle(t *testing.T, files map[string]string) *bundle.Bundle {