fix(cli): align search & verify table output (#21)

[so0k]

Summary

Fixes #21 — skillrig search printed name version — description with single-space separators and no column padding, so every row's columns started at a different offset and the list was unreadable. This renders the search list (and the verify failure list, which had the same defect) through a shared text/tabwriter helper — stdlib elastic tabstops, no new dependency (confirmed none of go-toml/cobra/yaml provide table formatting, and Cobra has no fixed-width helper).

Changes

• internal/cli/output.go
  • New writeAlignedColumns helper (one shared tabwriter renderer for both search and the verify failure list — no duplicated setup).
  • truncateDesc is now rune-safe: the old s[:n] byte-slice could split a multibyte rune into invalid UTF-8, and rune width matches how tabwriter measures cell width.
• Glyph guards (we keep the intentional output glyphs → ✓ ✗ — · … but guard against accidental new non-ASCII — smart quotes, NBSP, homoglyphs — which golangci-lint can't catch: asciicheck is identifiers-only, bidichk is dangerous-invisibles-only):
  • internal/sourceguard/ — a test-only package that scans every .go file and fails on any non-ASCII rune outside a documented allowlist (file:line:col + codepoint). Runs via go test ./..., so make check and CI enforce it for free — no workflow edit.
  • .claude/settings.json — a PostToolUse hook (Edit|Write|MultiEdit) that runs the guard after .go edits and blocks (exit 2) on a violation, feeding the location back. Validated end-to-end (caught an injected U+2019).

Before / after (skillrig search)

# before — ragged
agentic-go-cli-design  1.0.0  — Design principles...
aws-inventory  1.0.0  — Enumerate every taggable...

# after — aligned
agentic-go-cli-design  1.0.0  — Design principles...
aws-inventory          1.0.0  — Enumerate every taggable...

Testing

• make check — green: gofmt clean, go vet clean, golangci-lint 0 issues, all tests.
• make test-integration — TestQuickstart_* (builds & execs the real binary) pass; the search/verify assertions check output shape (bounded line count, name/footer presence), which the tabwriter change preserves.
• New internal/sourceguard guard proven to catch injected smart quotes; the PostToolUse hook proven to fire and block.

Closes #21.

[qodo-code-review]

Review Summary by Qodo

(Agentic_describe updated until commit 0d66d94)

Fix table alignment and add non-ASCII source guard

🐞 Bug fix ✨ Enhancement

Walkthroughs

Description

• Fix ragged table output in search and verify commands using stdlib text/tabwriter
• Make truncateDesc rune-safe to prevent UTF-8 corruption from multibyte character splitting
• Add internal/sourceguard test package to guard against accidental non-ASCII runes in source
• Add Claude agent-loop hook in .claude/settings.json to enforce ASCII guard on .go edits

Diagram

flowchart LR
  A["search/verify output"] -->|"ragged columns"| B["renderSearchResult<br/>renderVerifyFailure"]
  B -->|"new: writeAlignedColumns"| C["text/tabwriter<br/>elastic tabstops"]
  C -->|"aligned output"| D["fixed-width columns"]
  E["truncateDesc<br/>byte-slicing"] -->|"UTF-8 corruption risk"| F["rune-safe conversion"]
  F -->|"rune counting"| G["matches tabwriter width"]
  H[".go source edits"] -->|"PostToolUse hook"| I["sourceguard test"]
  I -->|"sanctioned allowlist"| J["intentional glyphs only"]

Loading

File Changes

1. internal/cli/output.go 🐞 Bug fix +47/-5

Align table output and fix rune-safe truncation

• Added writeAlignedColumns helper using text/tabwriter for shared table rendering across
 search and verify commands
• Refactored renderSearchResult to build rows and pass through writeAlignedColumns instead of
 direct formatting
• Refactored renderVerifyFailure to align failure reasons using the same writeAlignedColumns
 helper
• Made truncateDesc rune-safe by converting to rune slice before slicing, preventing UTF-8
 corruption from multibyte characters

internal/cli/output.go

---

2. internal/sourceguard/ascii_test.go 🧪 Tests +119/-0

Add non-ASCII source hygiene test guard

• New test package that scans all .go files for unsanctioned non-ASCII runes
• Maintains explicit allowlist of intentional glyphs (→, ✓, ✗, —, ·, …, •, §, ≤, –, ⇒) with
 justifications
• Reports violations as file:line:col: U+XXXX 'rune' for easy identification and fixing
• Runs via go test ./... so enforcement is automatic in make check and CI

internal/sourceguard/ascii_test.go

---

3. internal/sourceguard/helpers_test.go 🧪 Tests +64/-0

Test helper utilities for source guard

• Helper functions for module root discovery by walking up to go.mod
• Directory skip logic to exclude VCS, tooling, and build directories
• Path formatting utilities for readable violation messages
• Violation formatting as rel:line:col: U+XXXX 'r' for clear error reporting

internal/sourceguard/helpers_test.go

---

View more (1)

4. .claude/settings.json ⚙️ Configuration changes +17/-0

Add agent-loop hook for ASCII guard enforcement

• New Claude agent-loop hook configuration file
• PostToolUse hook triggers on .go file edits (Edit|Write|MultiEdit)
• Runs go test ./internal/sourceguard/ and blocks with exit 2 on violations
• Feeds violation output back to agent for immediate feedback and correction

.claude/settings.json

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (3) 📎 Requirement gaps (0)

Context used

✅ Tickets: 🎫 Bug: Tables are unreadable in CLI

✅ Compliance rules (platform): 152 rules

1. tabwriter.NewWriter args not multiline 📘 Rule violation ⚙ Maintainability

Description

The new tabwriter.NewWriter call passes 6 arguments on a single line, which violates the style
rule requiring one argument per line for calls with 4+ arguments. This reduces readability and makes
future diffs noisier.

Code

internal/cli/output.go[188]

Evidence

PR Compliance ID 782553 requires calls with 4+ arguments to use one argument per line. The newly
added tabwriter.NewWriter(w, 0, 0, 2, ' ', 0) is a 6-argument call on one line.

Rule 782553: Function calls with 4+ arguments must use one argument per line
internal/cli/output.go[188-188]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A function call with 4+ arguments is written on a single line, but the style guide requires one argument per line.

## Issue Context
`tabwriter.NewWriter(w, 0, 0, 2, ' ', 0)` has 6 arguments.

## Fix Focus Areas
- internal/cli/output.go[188-188]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Hook command line too long 📘 Rule violation ⚙ Maintainability

Description

The new .claude/settings.json hook embeds a very long shell command on a single line, exceeding
the ~120 character guideline. This makes the configuration hard to review and maintain.

Code

.claude/settings.json[9]

Evidence

PR Compliance ID 782552 requires breaking lines exceeding ~120 characters at semantic boundaries.
The added hook command is a single long line containing multiple shell statements and
conditionals.

Rule 782552: Break lines exceeding ~120 characters at semantic boundaries
.claude/settings.json[9-9]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A single JSON line contains an overlong shell command, exceeding the ~120 char line-length guideline.

## Issue Context
JSON strings can’t be cleanly line-wrapped; the maintainable pattern is to move complex shell logic into a small script file and reference it from the hook.

## Fix Focus Areas
- .claude/settings.json[9-9]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. truncateDesc uses … 📘 Rule violation ⚙ Maintainability

Description

The updated truncation logic appends a Unicode ellipsis … rather than the required .... This
deviates from the compliance requirement for human-output truncation formatting.

Code

internal/cli/output.go[214]

Evidence

PR Compliance ID 783450 requires human-oriented formatting to truncate descriptions to 80 characters
and append ... if truncated. The modified truncateDesc implementation returns ... + "…"
(Unicode ellipsis) instead of ....

Rule 783450: Human output must truncate descriptions to 80 characters and replace newlines with spaces
internal/cli/output.go[206-215]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Human output truncation is required to append `...`, but the code appends the Unicode ellipsis `…`.

## Issue Context
The truncation budget is `searchDescWidth` (80), so changing the suffix may require adjusting the slice length to keep the final rendered width at 80 characters.

## Fix Focus Areas
- internal/cli/output.go[206-215]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (1)

4. Tabs break table output 🐞 Bug ≡ Correctness

Description

writeAlignedColumns feeds tab-delimited strings to text/tabwriter, so any '\t' present inside a cell
(skill name/description/reason) is interpreted as a new column and corrupts the rendered table.
Because descriptions and skill names are currently taken verbatim from manifests/paths, this can be
triggered by accidental or malicious tab characters in catalog content.

Code

internal/cli/output.go[R187-191]

Evidence

The table renderer uses literal tab characters as column separators, but upstream inputs (manifest
description and skill name validation) are not guaranteed to be tab-free, so a tab can enter a cell
and be misinterpreted as an extra delimiter.

internal/cli/output.go[179-196]
internal/cli/output.go[201-215]
pkg/skillcore/manifest.go[41-80]
pkg/skillcore/add.go[381-392]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`writeAlignedColumns` joins cells with `"\t"` and sends them to `text/tabwriter`. If any cell content itself contains a tab (`\t`), `tabwriter` treats it as an extra column separator, breaking alignment and potentially making output misleading/unreadable.

## Issue Context
- Search rows include `CatalogEntry.Description` (only `\n` is replaced today).
- Verify failure rows include skill names derived from paths / manifest names.
- Neither skill names nor descriptions are sanitized to exclude `\t`.

## Fix Focus Areas
- internal/cli/output.go[179-196]
- internal/cli/output.go[201-215]
- pkg/skillcore/manifest.go[41-80]
- pkg/skillcore/add.go[381-392]

## Suggested fix
- In `writeAlignedColumns`, sanitize each cell before `strings.Join`:
 - Replace `\t`, `\n`, and `\r` with a single space (or remove them), e.g. `strings.NewReplacer("\t"," ","\n"," ","\r"," ")`.
 - Optionally collapse repeated spaces if desired.
- Additionally (or alternatively) extend `truncateDesc` to replace `\t` with spaces so descriptions cannot inject columns.
- Consider adding a small unit test for `writeAlignedColumns` (or `truncateDesc`) demonstrating that an input containing `\t` does not create extra columns / does not misalign output.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-code-review]

Code Review by Qodo

New Review Started

This review has been superseded by a new analysis