Repository navigation
feat(skills): check skills in Go - #830
Draft
tbhb-agent wants to merge 1 commit into
Draft
tbhb-agent wants to merge 1 commit into
tbhb-agent wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
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:
- [blocking] internal/skills/check.go:115: A matched folded description containing only whitespace becomes empty, triggering single-line fallback and accepting
>-as its description. Fordescription: >-\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. - [blocking] internal/skills/check.go:80: Go globbing includes dot-prefixed directories that the frozen Bash globs exclude. A malformed
.claude/skills/.hidden/SKILL.mdtherefore 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. - [blocking] internal/skills/check.go:53: filepath.Join cleans explicit targets before filesystem resolution. With
linkpointing to another directory,link/../skillcan 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-agent
force-pushed
the
fix/808-20261026
branch
from
October 27, 2026 03:19
f95b2ab to
7a4a191
Compare
There was a problem hiding this comment.
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:
- [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
force-pushed
the
fix/808-20261026
branch
from
October 27, 2026 16:58
7a4a191 to
41395fa
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 addstools/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,TestHiddenDiscoveryParityandTestExplicitTraversalParitycompare Go findings with the frozen script over default, hidden, and explicit paths.TestCorpus,TestUnreadableAfterValid,TestDiscoveryandTestReadFailurecover manifest verdicts, unreadable files, and discovery.TestLocaleIndependenceandTestOfflineReadOnlycover locale handling and a check that doesn't write or use the network.go test ./internal/skillspassed 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.