Skip to content

feat(skills): check skills in Go - #830

Draft
tbhb-agent wants to merge 1 commit into
mainfrom
fix/808-20261026
Draft

tbhb-agent wants to merge 1 commit into
mainfrom
fix/808-20261026

Conversation

@tbhb-agent

@tbhb-agent tbhb-agent commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Adds internal/skills, a Go package that applies the skill limits checks and reports the same findings as the shell checker. The change also adds tools/skill-limits-legacy.sh, a verbatim copy of the shell checker that the parity tests run as their reference. The entry point still uses the shell, and no task invokes the Go package yet.

Why

Epic #588 moves shell logic to Go. Parsing, discovery, and filesystem traversal decide which manifests the checker accepts, and a port that drifts on any of them changes accepted manifests during the transition. The parity tests run the frozen script and the Go package over the same trees and compare findings, so drift fails a test.

The legacy script is a frozen reference copy. It doesn't add shell logic, and a later change removes it with the original checker.

A stacked follow-up switches the entry point to the Go package. Splitting the port keeps each pull request under the size limit.

Consumers don't need to act. Nothing in the published payload changes.

Verification

  • TestFrozenParity, TestDiscoveryParity, TestHiddenDiscoveryParity and TestExplicitTraversalParity compare Go findings with the frozen script over default, hidden, and explicit paths.
  • TestCorpus, TestUnreadableAfterValid, TestDiscovery and TestReadFailure cover manifest verdicts, unreadable files, and discovery.
  • TestLocaleIndependence and TestOfflineReadOnly cover locale handling and a check that doesn't write or use the network.
  • go test ./internal/skills passed on the head this description covers.

Risk

A parity gap could let the Go package accept a manifest the shell rejects, or the reverse. No task invokes the package yet, so a gap has no effect until the entry point switches. Reverting the commit removes the package and the legacy script.

Compatibility: none

Related

Part of #808.

Part of #588.

@tbhb-agent tbhb-agent added change:feat PR adds or changes a feature (Conventional Commit type feat) provenance:agent-assisted PR contains commits authored with AI assistance (see Assisted-by trailers) labels Oct 26, 2026

@tbhb-dev-reviews tbhb-dev-reviews Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed head: f95b2ab
PR: #830
Model: OpenAI/GPT-6, effort unspecified
Repo criteria: none: no file
Loosens: yes, empty folded descriptions bypass validation; no owner-tier or configured gate paths touched
Verdict: REQUEST_CHANGES
Findings:

  1. [blocking] internal/skills/check.go:115: A matched folded description containing only whitespace becomes empty, triggering single-line fallback and accepting >- as its description. For description: >-\n \nother: yes\n, the frozen shell reports description-missing; Go succeeds. Fix: distinguish an absent match from an empty extracted value and add transcript parity coverage.
  2. [blocking] internal/skills/check.go:80: Go globbing includes dot-prefixed directories that the frozen Bash globs exclude. A malformed .claude/skills/.hidden/SKILL.md therefore changes a previously clean default invocation into failure; hidden package directories also change discovery. Fix: preserve Bash’s exclusion of dot-prefixed names at each wildcard component and test both discovery trees.
  3. [blocking] internal/skills/check.go:53: filepath.Join cleans explicit targets before filesystem resolution. With link pointing to another directory, link/../skill can resolve to a different manifest in the frozen shell than the cleaned Go path. A missing intermediate component can likewise become a checked manifest instead of being skipped. Fix: preserve target path components during lookup and add parity cases for symlink-parent traversal and missing intermediate directories.
    AC: AC1 not met — discovery includes additional hidden directories; internal/skills/check.go:80.
    AC: AC2 not met — explicit-target normalization changes manifest selection; internal/skills/check.go:53.
    AC: AC3 not met — whitespace-only folded descriptions change verdict; internal/skills/check.go:115.
    AC: AC4 met — independent unreadable findings; internal/skills/check_test.go:115 and internal/skills/parity_unix_test.go:160.
    AC: AC5 not met — findings, totals and status diverge for the cases above; internal/skills/check.go:69.
    AC: AC6 met — offline execution and command traps; internal/skills/parity_unix_test.go:121.
    AC: AC7 met — help probe and frozen fallback; tools/skill-limits.sh:8 and tests/tools/skill-limits.bats:30.
    AC: AC8 met — supported failures and failed probes propagate; tests/tools/skill-limits.bats:45 and tests/tools/skill-limits.bats:63.
    AC: AC9 not met — fallback remains available, but supported Go invocations introduce compatibility regressions; internal/skills/check.go:53.
    AC: AC10 met — locale corpus and successful Linux, macOS and Windows CI; internal/skills/check_test.go:142.
    AC: AC11 met — repository snapshots and mutation traps; internal/skills/parity_unix_test.go:95 and internal/skills/parity_unix_test.go:121.
    DoD: 5/10; unmet: D1, D2, D3, D9, D10
    Rationale: All 24 head check-runs completed successfully, including platform tests, lint and security checks.
    D3 remains unmet because the folded-description regression loosens validation despite green CI.
    D4 is supported by passing leak checks and inspection of the diff and PR body.
    D5: the sole commit has verification.verified=true.
    D6: the description states No doc impact and explains why.
    D7: the commit carries Assisted-by and the PR carries provenance:agent-assisted.
    D8: nine files cover one gate, its command, tests and unchanged frozen fallback.
    D9: Related lists no fallback-retirement follow-up required by #808.
    D1, D2 and D10 remain pending; Related currently uses “Refs” rather than a closing reference.
    No generated skill outputs changed. The exact head was rechecked.
    Round: 1 of 3

@tbhb-dev-reviews tbhb-dev-reviews Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed head: 7a4a191
PR: #830
Model: OpenAI/GPT-6, effort unspecified
Repo criteria: none: no file
Loosens: no; no owner-tier or configured gate paths touched
Verdict: REQUEST_CHANGES
Findings:

  1. [should-fix] tools/skill-limits.sh:16: The fallback-retirement follow-up required by #808 remains unlisted under Related, as noted in round 1. Fix: file or identify its issue with a Problem section, numbered acceptance criteria, compatibility prerequisites and a reference to #808; list it under Related.
    AC: AC1 met — discovery and mirror exclusion, including hidden directories; internal/skills/parity_unix_test.go:119 and internal/skills/parity_unix_test.go:129.
    AC: AC2 met — explicit ordering, missing manifests and filesystem traversal; internal/skills/parity_unix_test.go:114 and internal/skills/parity_unix_test.go:148.
    AC: AC3 met — boundary, UTF-8, reserved-name, XML and frontmatter corpus; internal/skills/check_test.go:25.
    AC: AC4 met — unreadable manifests cannot inherit prior fields; internal/skills/check_test.go:115, internal/skills/parity_unix_test.go:203 and tests/tools/skill-limits.bats:92.
    AC: AC5 met — exact stdout, stderr, status and repository snapshot comparisons; internal/skills/parity_unix_test.go:98.
    AC: AC6 met — network command traps; internal/skills/parity_unix_test.go:164.
    AC: AC7 met — read-only help probe and absent/older-binary fallback; internal/agentrepo/skilllimits_test.go:17 and tests/tools/skill-limits.bats:30.
    AC: AC8 met — supported failures and failed probes propagate without fallback or repetition; tests/tools/skill-limits.bats:45 and tests/tools/skill-limits.bats:63.
    AC: AC9 met — frozen fallback and unchanged task compatibility across absent, older and new binaries; tests/tools/skill-limits.bats:109.
    AC: AC10 met — locale and multibyte corpus, with successful Linux, macOS and Windows head checks; internal/skills/check_test.go:142.
    AC: AC11 met — repository snapshots and mutation traps; internal/skills/parity_unix_test.go:103 and internal/skills/parity_unix_test.go:164.
    DoD: 6/10; unmet: D1, D2, D9, D10
    Rationale: Round 1 finding 1 fixed: matched empty folded descriptions retain their empty value; internal/skills/check.go:116, with regression coverage at internal/skills/check_test.go:39.
    Round 1 finding 2 fixed: wildcard components exclude hidden names; internal/skills/check.go:82, covered at internal/skills/parity_unix_test.go:129.
    Round 1 finding 3 fixed: lookup preserves filesystem path components; internal/skills/check.go:54, covered at internal/skills/parity_unix_test.go:148.
    The implementation delta since round 1 adds these corrections and regression tests; no additional regression found.
    D3: all 24 exact-head check-runs succeeded, including platform tests, lint and security checks.
    D4: successful gitleaks and lint checks support leak cleanliness; inspected diff and live description contain no apparent secrets.
    D5/D7: both commits have verification.verified=true and Assisted-by trailers; provenance:agent-assisted is present.
    D6/D8: the description explains No doc impact; nine changed files remain focused on one gate, compatibility glue and tests. No generated skill outputs changed.
    D9 remains unmet. The closing reference is now present, but D1, D2 and D10 remain pending until merge.
    Round: 2 of 3

Consumers need the same findings while the checker moves to Go.
Freeze the shell behavior so parsing, discovery, and filesystem
traversal don't change accepted manifests during the transition.
The entry point keeps using the shell in this first change.

Assisted-by: codex:gpt-6-astra
Signed-off-by: Tony Burns <tony@tonyburns.net>
@tbhb-agent tbhb-agent changed the title feat(skills): check skill limits in Go feat(skills): check skills in Go Oct 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change:feat PR adds or changes a feature (Conventional Commit type feat) provenance:agent-assisted PR contains commits authored with AI assistance (see Assisted-by trailers)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant