Skip to content

fix(cli): sanitize tabs in aligned table output (PR #22 review)#26

Open
so0k wants to merge 3 commits into
mainfrom
fix/table-tab-sanitization
Open

fix(cli): sanitize tabs in aligned table output (PR #22 review)#26
so0k wants to merge 3 commits into
mainfrom
fix/table-tab-sanitization

Conversation

@so0k
Copy link
Copy Markdown
Contributor

@so0k so0k commented Jun 2, 2026

Follow-up to #22, addressing the Qodo review on that PR.

Summary

  • 🐞 Bug — tabs corrupt the table (writeAlignedColumns). Cells were \t-joined and fed to text/tabwriter, so a tab inside any cell (catalog description or skill name/reason — taken verbatim, not guaranteed tab-free) was read as an extra column separator and broke alignment. Now each cell is sanitized (\t/\n/\r → space) before joining, with a unit test proving a tabbed cell can't inject a column.
  • Style — tabwriter.NewWriter args broken onto one-arg-per-line (rule 782553 / our own golang-code-style).
  • Maintainability — hook command extracted to a script. The PostToolUse hook's long single-line shell command (>120 chars, rule 782552) now lives in .claude/hooks/ascii-guard.sh, referenced from .claude/settings.json. Behavior unchanged.

Not included (deliberate)

  • Qodo flagged truncateDesc using instead of ASCII ... (rule 783450). That conflicts with the maintainer's decision to keep the intentional output glyphs, and changing an org-wide rule vs. complying is a pending governance call — left out of this PR.

Test plan

  • make check green: gofmt, go vet, golangci-lint 0 issues, all tests (incl. integration).
  • New TestWriteAlignedColumns_NeutralizesTabsInCells passes.
  • Extracted hook script re-pipe-tested: clean .go → exit 0, non-Go → exit 0 (skipped), injected U+201C → exit 2 with location on stderr.

🤖 Generated with Claude Code

so0k and others added 2 commits June 2, 2026 11:22
writeAlignedColumns joined cells with \t and fed them to text/tabwriter, so
a tab inside any cell (catalog description or skill name/reason, taken
verbatim and not guaranteed tab-free) was read as an extra column separator
and corrupted the alignment this slice added. Sanitize \t/\n/\r to spaces in
each cell before joining, with a unit test proving a tabbed cell can no
longer inject a column. Also break tabwriter.NewWriter onto one argument per
line (style: 4+ args), per the Qodo review on PR #22.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The PostToolUse hook embedded a long multi-statement shell command on a
single JSON line (>120 chars, unreviewable). Move the logic into
.claude/hooks/ascii-guard.sh and reference it from .claude/settings.json, so
the guard is readable, executable, and independently testable. Behavior is
unchanged (clean .go -> 0, non-go -> 0, violation -> exit 2). Per the Qodo
review on PR #22.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@qodo-code-review
Copy link
Copy Markdown

Review Summary by Qodo

Sanitize control characters in aligned table output

🐞 Bug fix ✨ Enhancement

Grey Divider

Walkthroughs

Description
• Sanitize tabs/newlines in table cells to prevent alignment corruption
  - Tabs were read as column separators by tabwriter, breaking layout
  - Newlines/carriage-returns broke row invariants
  - All control characters replaced with spaces before joining
• Add unit test proving tabbed cells cannot inject columns
• Improve code style by breaking tabwriter.NewWriter onto separate lines
• Extract long hook command into dedicated shell script for maintainability
Diagram
flowchart LR
  A["Cell content<br/>with tabs/newlines"] --> B["cellSanitizer<br/>replaces control chars"]
  B --> C["Safe cells<br/>joined with tabs"]
  C --> D["tabwriter<br/>renders aligned columns"]
  E["Long hook command<br/>in JSON"] --> F["Extracted to<br/>ascii-guard.sh"]
  F --> G["Referenced from<br/>settings.json"]

Loading

Grey Divider

File Changes

1. internal/cli/output.go 🐞 Bug fix +30/-7

Sanitize control characters in table cells

• Added cellSanitizer variable using strings.NewReplacer to neutralize tabs, newlines, and
 carriage returns in cell content
• Modified writeAlignedColumns to sanitize each cell before joining with tabs
• Reformatted tabwriter.NewWriter call with one argument per line and inline comments for clarity
• Updated function documentation to explain sanitization and control character handling

internal/cli/output.go


2. internal/cli/output_test.go 🧪 Tests +41/-0

Add test for tab sanitization in table cells

• Created new test file with TestWriteAlignedColumns_NeutralizesTabsInCells function
• Test verifies that embedded tabs in cell content are converted to spaces
• Confirms that tabbed cells render as exactly two lines (tab did not split into new column)
• Validates that sanitized content remains as single contiguous token

internal/cli/output_test.go


3. .claude/hooks/ascii-guard.sh Refactor +22/-0

Extract ascii-guard hook into standalone script

• New executable shell script extracted from inline JSON hook command
• Reads PostToolUse payload from stdin and checks for .go file references
• Runs go test ./internal/sourceguard/ to validate non-ASCII source guard
• Returns exit code 2 with stderr output if violations detected
• Dependency-free implementation compatible with macOS and Linux

.claude/hooks/ascii-guard.sh


View more (1)
4. .claude/settings.json ⚙️ Configuration changes +1/-1

Reference extracted hook script in settings

• Replaced long inline shell command (>120 chars) with reference to external script
• PostToolUse hook now calls bash "$CLAUDE_PROJECT_DIR/.claude/hooks/ascii-guard.sh"
• Improves readability and maintainability of hook configuration
• Behavior unchanged; guard still blocks edits introducing unsanctioned non-ASCII runes

.claude/settings.json


Grey Divider

ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented Jun 2, 2026

Code Review by Qodo

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

Grey Divider


Remediation recommended

1. Hook script path brittle ✓ Resolved 🐞 Bug ☼ Reliability
Description
The PostToolUse hook now invokes the guard via `bash
"$CLAUDE_PROJECT_DIR/.claude/hooks/ascii-guard.sh", which becomes bash
"/.claude/hooks/ascii-guard.sh" when CLAUDE_PROJECT_DIR` is unset/empty and will fail to execute
the guard. This is a new failure mode vs. the prior inline command and can silently disable the
non-ASCII guard in environments that don’t export CLAUDE_PROJECT_DIR.
Code

.claude/settings.json[9]

Evidence
The settings hook uses $CLAUDE_PROJECT_DIR directly in the script path, while the script itself is
written to handle an unset project dir (defaulting cd to .), indicating the env var may not be
guaranteed and should be defaulted consistently to avoid bypassing the guard.

.claude/settings.json[1-13]
.claude/hooks/ascii-guard.sh[9-22]

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

## Issue description
`.claude/settings.json` executes the hook script using a path prefixed by `$CLAUDE_PROJECT_DIR`. If that env var is unset/empty, the resulting path is `/.claude/hooks/ascii-guard.sh`, so `bash` fails to find the script and the guard never runs.

## Issue Context
The script itself already tolerates an unset `CLAUDE_PROJECT_DIR` when changing directories (`cd "${CLAUDE_PROJECT_DIR:-.}"`), so the settings command should similarly provide a default.

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

## Suggested fix
Change the command to use a default:
- `bash "${CLAUDE_PROJECT_DIR:-.}/.claude/hooks/ascii-guard.sh"`

(Alternative: invoke a relative path and keep the script’s internal `cd` as-is, but only if the hook runner always starts in repo root.)

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


Grey Divider

Qodo Logo

…review)

When CLAUDE_PROJECT_DIR is unset/empty the hook command resolved to
/.claude/hooks/ascii-guard.sh, so bash could not find the script and the
guard was silently skipped. Default it to '.' (matching the script's own
cd "${CLAUDE_PROJECT_DIR:-.}") so the guard still runs. Per the Qodo review
on PR #26.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant