diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 0000000..dc20082 --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,17 @@ +{ + "hooks": { + "PostToolUse": [ + { + "matcher": "Edit|Write|MultiEdit", + "hooks": [ + { + "type": "command", + "command": "input=$(cat); case \"$input\" in *'.go\"'*) cd \"$CLAUDE_PROJECT_DIR\" || exit 0; out=$(go test ./internal/sourceguard/ -count=1 2>&1) || { printf '%s\\n' \"$out\" >&2; exit 2; };; esac", + "statusMessage": "non-ASCII source guard", + "timeout": 60 + } + ] + } + ] + } +} diff --git a/internal/cli/output.go b/internal/cli/output.go index 7d22da2..b2d0881 100644 --- a/internal/cli/output.go +++ b/internal/cli/output.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "strings" + "text/tabwriter" "github.com/skillrig/cli/pkg/skillcore" ) @@ -156,8 +157,16 @@ func renderSearchResult(w io.Writer, origin string, matches []skillcore.CatalogE var b strings.Builder + // Align name/version/description into fixed-width columns so the ragged, + // unreadable output of issue #21 becomes a clean table. The table is rendered + // before the footer so the summary line is not drawn into the columns. + rows := make([][]string, 0, len(matches)) for _, e := range matches { - fmt.Fprintf(&b, "%s %s — %s\n", e.Name, e.Version, truncateDesc(e.Description)) + rows = append(rows, []string{e.Name, e.Version, "— " + truncateDesc(e.Description)}) + } + + if err := writeAlignedColumns(&b, rows); err != nil { + return err } fmt.Fprintf(&b, "%d skill(s) · run: skillrig add \n", len(matches)) @@ -167,18 +176,42 @@ func renderSearchResult(w io.Writer, origin string, matches []skillcore.CatalogE return err } +// writeAlignedColumns renders rows as space-padded, fixed-width columns via the +// stdlib text/tabwriter (elastic tabstops) so columns line up across rows in +// compact human output. It is the one shared table renderer for search and the +// verify failure list — no duplicated tabwriter setup. Each row's cells are +// tab-joined; the final cell is newline-terminated, so it is never right-padded +// (no trailing whitespace). Cell width is counted in runes (tabwriter assumes +// equal-width code points), matching truncateDesc's rune budget so the column +// math and the clip agree. An empty rows slice writes nothing. +func writeAlignedColumns(w io.Writer, rows [][]string) error { + tw := tabwriter.NewWriter(w, 0, 0, 2, ' ', 0) + for _, cells := range rows { + if _, err := fmt.Fprintln(tw, strings.Join(cells, "\t")); err != nil { + return err + } + } + + return tw.Flush() +} + // searchEmptyFooter is the next-step hint for an empty search result (still exit 0). const searchEmptyFooter = "→ broaden the query, or run skillrig search with no filter to list all" // truncateDesc collapses a description's newlines to spaces and clips it to -// searchDescWidth for compact human output (the complete text is in --json). +// searchDescWidth for compact human output (the complete text is in --json). The +// width budget and the clip are counted in runes, not bytes: byte-slicing could +// split a multibyte rune into invalid UTF-8, and rune counting matches how +// tabwriter measures cell width, so the column budget and the alignment agree. func truncateDesc(s string) string { s = strings.ReplaceAll(s, "\n", " ") - if len(s) <= searchDescWidth { + + r := []rune(s) + if len(r) <= searchDescWidth { return s } - return s[:searchDescWidth-1] + "…" + return string(r[:searchDescWidth-1]) + "…" } // indexResult is the presentation-layer view of an index generation: where the @@ -283,12 +316,21 @@ func renderVerifyFailure(w io.Writer, r skillcore.Report) error { fmt.Fprintf(&b, "verify FAILED: %d of %d skills\n", failed, total) + // Align the failing skills' reason column the same way search aligns its + // table — the marker+name stays the first (tab-terminated) cell so "✗ " + // reads as one token, and reasons line up across rows. + rows := make([][]string, 0, failed) + for _, v := range r.Verdicts { if v.Status == skillcore.StatusOK { continue } - fmt.Fprintf(&b, " ✗ %s %s\n", v.Name, verdictLine(v)) + rows = append(rows, []string{" ✗ " + v.Name, verdictLine(v)}) + } + + if err := writeAlignedColumns(&b, rows); err != nil { + return err } b.WriteString(verifyFailFooter + "\n") diff --git a/internal/sourceguard/ascii_test.go b/internal/sourceguard/ascii_test.go new file mode 100644 index 0000000..5adf6c7 --- /dev/null +++ b/internal/sourceguard/ascii_test.go @@ -0,0 +1,119 @@ +// Package sourceguard holds a repo-wide source hygiene guard. It has no +// production code — only a test that every .go file in the module stays ASCII +// except for an explicitly sanctioned set of glyphs. +// +// Why: skillrig's human output deliberately uses a few non-ASCII glyphs (the +// "→" next-step hint, "✓/✗" verify status, the "—/·/…" separators). That set is +// intentional and kept (see issue #21 discussion). What we want to prevent is +// the ACCIDENTAL introduction of *other* non-ASCII runes — smart quotes from a +// copy-paste, a non-breaking space, a homoglyph — which render unpredictably in +// terminals/CI and are nearly invisible in review. golangci-lint cannot catch +// this: asciicheck only inspects identifiers, and bidichk only catches dangerous +// invisible/bidi runes, not ordinary glyphs in string literals. +// +// This test is the single source of truth for the allowlist. It runs as part of +// `go test ./...` (so `make check` and CI enforce it for free) and is also what +// the PostToolUse agent-loop hook invokes. To introduce a new glyph, add it to +// sanctioned below with a comment — a deliberate, reviewable edit. +package sourceguard + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// sanctioned is the set of non-ASCII runes allowed anywhere in .go source. Each +// entry is intentional; adding one is a deliberate decision recorded here. +var sanctioned = map[rune]string{ + '→': "U+2192 next-step footer hints (init/add/search/index/verify)", + '✓': "U+2713 verify OK status", + '✗': "U+2717 verify failure status", + '—': "U+2014 em dash — prose/help/output separator", + '·': "U+00B7 search footer separator (N skill(s) · run:)", + '…': "U+2026 truncation/elision marker", + '•': "U+2022 bullet in `add` help text", + '§': "U+00A7 section sign in Constitution references (comments)", + '≤': "U+2264 bounded-output notes in comments (≤2 lines)", + '–': "U+2013 en dash in numeric ranges in comments (rows 1–7)", + '⇒': "U+21D2 logical implication in a comment", +} + +// TestSourceIsASCIIExceptSanctioned walks the module and fails on any non-ASCII +// rune that is not in sanctioned, reporting file:line:col and the offending rune +// so the fix is obvious: either revert to ASCII, or (if genuinely intended) add +// the rune to sanctioned. +func TestSourceIsASCIIExceptSanctioned(t *testing.T) { + t.Parallel() + + root := moduleRoot(t) + + var violations []string + + walkErr := filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error { + if err != nil { + return err + } + + if d.IsDir() { + if skipDir(d.Name()) { + return filepath.SkipDir + } + + return nil + } + + if !strings.HasSuffix(d.Name(), ".go") { + return nil + } + + violations = append(violations, scanFile(t, path)...) + + return nil + }) + if walkErr != nil { + t.Fatalf("walking module from %q: %v", root, walkErr) + } + + if len(violations) > 0 { + t.Fatalf("found %d unsanctioned non-ASCII rune(s) in .go source:\n%s\n\n"+ + "fix: revert to ASCII, or if the glyph is intentional add it to "+ + "sanctioned in internal/sourceguard/ascii_test.go with a justification.", + len(violations), strings.Join(violations, "\n")) + } +} + +// scanFile returns one violation string per unsanctioned non-ASCII rune in the +// file at path, tracking the path relative to the module root for readability. +func scanFile(t *testing.T, path string) []string { + t.Helper() + + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("reading %q: %v", path, err) + } + + var out []string + + rel := relOrPath(path) + + for i, text := range strings.Split(string(data), "\n") { + col := 0 + for _, r := range text { + col++ + + if r < 128 { + continue + } + + if _, ok := sanctioned[r]; ok { + continue + } + + out = append(out, formatViolation(rel, i+1, col, r)) + } + } + + return out +} diff --git a/internal/sourceguard/helpers_test.go b/internal/sourceguard/helpers_test.go new file mode 100644 index 0000000..c37f99a --- /dev/null +++ b/internal/sourceguard/helpers_test.go @@ -0,0 +1,64 @@ +package sourceguard + +import ( + "fmt" + "os" + "path/filepath" + "testing" +) + +// modRoot is the module root discovered by moduleRoot, reused to render paths +// relative to it. Set once before the parallel scan reads it. +var modRoot string + +// moduleRoot walks up from the test's working directory (the package dir) until +// it finds go.mod, and returns that directory — the module root to scan. +func moduleRoot(t *testing.T) string { + t.Helper() + + dir, err := os.Getwd() + if err != nil { + t.Fatalf("getwd: %v", err) + } + + for { + if _, statErr := os.Stat(filepath.Join(dir, "go.mod")); statErr == nil { + modRoot = dir + + return dir + } + + parent := filepath.Dir(dir) + if parent == dir { + t.Fatalf("no go.mod found walking up from working directory") + } + + dir = parent + } +} + +// skipDir reports whether a directory should be skipped: VCS/tooling/build +// directories that either are not our source or hold generated/vendored trees. +func skipDir(name string) bool { + switch name { + case ".git", ".claude", "vendor", "node_modules", "dist", "testdata": + return true + default: + return false + } +} + +// relOrPath renders path relative to the module root, falling back to the +// absolute path if that fails — purely for readable violation messages. +func relOrPath(path string) string { + if rel, err := filepath.Rel(modRoot, path); err == nil { + return rel + } + + return path +} + +// formatViolation renders one offending rune as "rel:line:col: U+XXXX 'r'". +func formatViolation(rel string, line, col int, r rune) string { + return fmt.Sprintf(" %s:%d:%d: %U %q", rel, line, col, r, string(r)) +}