Repository navigation
refactor(tools): remove the skill limits shell fallback #847
Description
Problem
The Go checker from #971 and its parity suite from #972 are on origin/main (591fd70), but nothing calls the checker: internal/agentrepo/command.go registers no check-skill-limits command, and tools/skill-limits.sh:39 still holds the active shell algorithm. #938 supplies the registration and a wrapper that probes for the command and otherwise runs the frozen tools/skill-limits-legacy.sh. That fallback keeps two implementations of one gate, and internal/skills/parity_unix_test.go (reads the legacy script at line 142, runs Bash at line 155, requires active and frozen scripts to match at line 312) cannot survive its deletion.
This issue ends the fallback once the N-2 window in docs/compatibility.md allows it. Part of #588. Follows #808 and #938.
Acceptance criteria
- AC1:
tools/skill-limits.shruns the Gocheck-skill-limitscommand with the caller's arguments and relays its standard output, standard error and exit status unchanged;tools/skill-limits-legacy.shno longer exists and no capability probe remains in the entry point. AC2: A caller whoseDropped: an older binary cannot print a diagnostic it does not contain, and the supported invocation (AC5) never meets one, so no new version logic is added (Replace complex shell with tested Go, starting with gates and merging #588 freeze).agentrepolacks the subcommand exits nonzero with a minimum-version message on standard error and nothing on standard output.AC3: The existing parity cases pass against the Go path alone.Dropped: those cases compare against the deleted shell. AC7 and AC8 keep the same behavior as Go-only expectations.- AC4: The change alters no generated skill file and
mise run check-generated-skillspasses. - AC5:
mise run lint-skillsandtools/skill-limits.shcheck skills with the repository's own build, so anagentrepoonPATH, of any version, is never run. - AC6: When
miseis not available,tools/skill-limits.shexits nonzero with nothing on standard output and a message on standard error. - AC7: For every case in the retained corpus (name lengths 63, 64, 65; description lengths 1023, 1024, 1025; uppercase and punctuation; each reserved name; folded, single-line and missing frontmatter; XML-like text; non-ASCII text at both boundaries; null bytes; an unreadable manifest after a valid one; invalid UTF-8), the command's standard output, standard error and exit status equal the expectations recorded from the frozen shell, with exactly two documented differences: the empty standard error for invalid UTF-8 (feat(skills): add the Go skill limits checker #971 A5), and the absence of Bash's
ignored null byte in inputwarning lines, which the recording drops (test(skills): compare the Go checker with the frozen shell checker #972 B2a). - AC8: With no targets the command checks packaged and repository-local skills and never deployed mirrors; with explicit targets it checks them in the order given and skips a missing manifest;
--before the targets is accepted. - AC9: Non-functional: once the repository build is provisioned (toolchain and module cache present), a run of
tools/skill-limits.shleaves repository bytes, the index and refs unchanged and makes no network connection. - AC10: Non-functional: the corpus gives the same verdicts and diagnostics on Linux, macOS and Windows, and under differing locales.
- AC11: For every exit status N from 0 to 255 that the Go command returns,
tools/skill-limits.shexits with N after one invocation, relays the command's standard error unchanged and runs nothing else. - AC12: The change adds no new shell logic: no added line in any
.shfile other than the singleexecof the entry point, and no remaining shell body that parses manifests or chooses between implementations. - AC13: The change merges only after the first release containing
agentrepo check-skill-limitsand at least two later releases are tagged, and the pull request cites them with dates. - AC14:
docs/script-inventory.mdrecords the Go destination fortools/skill-limits.sh, and the migration note names the first release carrying the command and states that no consumer binary floor changes.
Scope and non-goals
Delete tools/skill-limits-legacy.sh, the capability probe in tools/skill-limits.sh, and the shell comparison in internal/skills/parity_unix_test.go with its Bash helpers and fake python3. Retain the behavioral corpus as Go-only tests whose expectations are recorded from the frozen shell before it is deleted. Update the two documents in AC14.
Non-goal: porting any other script. Non-goal: removing a fallback owned by another port, including #846. Non-goal: raising the agentrepo floor of the shared skill loader (DT_AGENTREPO_MIN_VERSION). Non-goal: changing the repository launcher, which belongs to the launcher prerequisite below.
Approach
Verified on origin/main: tools/skill-limits.sh is called only by mise.toml:439 (lint-skills) and is outside the vendored payload under .devtools/. tools/check-versions.sh:4 forwards with exec mise --quiet run run agentrepo check-versions "$@", and the shim here takes the same shape: one exec of the repository launcher, so a binary built from the same tree always runs and no older binary needs to diagnose itself. AC2 is dropped for that reason, and the absent case is covered by AC6.
The launcher does not satisfy AC11 today. [tasks.run] (mise.toml:193-202) ends in go run, and go run reports a failed child as exit status N on standard error and exits 1, so a status of 3 reaches the caller as 1. Probed on 2026-10-31 with a Go program exiting 3: go run returned 1 and printed exit status 3, while mise run of a task exiting 3 returned 3. The shim therefore needs a launcher that runs a built binary, such as the binary devtools:build writes to bin/ (.devtools/tasks/go.toml:10); the launcher change and its test live in the launcher prerequisite under Dependencies, not here. The shim's exact spelling is then chosen against that launcher.
On the branch head before the deletion, run the existing parity suite once to capture each corpus case's three streams and status as test data, with the null-byte warning lines dropped as parity_unix_test.go:174 does today, add command-level tests over that data, then delete the shell comparison. TestCorpus, TestDiscovery, TestExplicitTargets, TestLocaleIndependence, TestInvalidUTF8, TestUnreadableAfterValid and TestFilesystemOnly from #971 already run Go-only and stay. Rewrite the #938 Bats file to the AC5, AC6 and AC11 cases and drop the fallback cases. Do not add parity checks that need deleted shell.
Dependencies
Blocked by #808
Blocked by #333
Blocked by #544
Blocked on: #938
#808 supplies the port, #938 (a pull request, so a URL line rather than a native link) the registration, and #333 and #544 are the retirement prerequisites #808 names.
Two prerequisites are not yet owned by any issue. Each is to be filed and linked as a native blocker before work starts:
- Title:
docs(compat): announce the skill-limits fallback retirement. Scope: add to the migration note a statement thattools/skill-limits-legacy.shand the probe intools/skill-limits.share removed after the N-2 window, naming refactor(tools): remove the skill limits shell fallback #847, and ship it in a tagged release. That release must be at least two releases older than the one this change ships in, which starts the warning perioddocs/compatibility.md:5requires. - Title:
fix(tools): forward the child's exit status from the repository launcher. Scope: makemise run run <bin> ...return the exact exit status of the Go command with its standard error relayed unchanged, tested through the real launch path with a child that exits with several statuses, using tested Go or the existing build task and no new shell logic.
Release-window evidence on 2026-10-31: the newest tag is v0.23.0 (2026-10-29), and git grep check-skill-limits v0.23.0 -- internal finds nothing, so no release carries the command yet. Recent tags are v0.20.0 (2026-10-25), v0.21.0 (2026-10-25), v0.22.0 and v0.22.1 (2026-10-26). Let X be the first tag whose internal/agentrepo/command.go registers the command. The pull request needs tags X, X+1 and X+2, and the announcement above in a release at least two releases older than the merge.
Deferred review findings from the pull request become issues with a Problem section and numbered ACs, are listed under Related, and any code that defers work links its issue. #973 tests the fake python3, which this change deletes, so close it with that reason.
Size
S. About 200 added and 600 deleted lines across the legacy script, the parity suite and the Bats file; the launcher change is excluded and sits in its own prerequisite. If the changed-line count exceeds 800, split the recorded-transcript tests into a preceding pull request that adds them while the shell still exists.
Verify plan
- AC1:
git grep -n -E 'skill-limits-legacy|command -v|agentrepo' -- tools/skill-limits.shreturns nothing but the one launcher line,git grep -n 'skill-limits-legacy'returns nothing outsidedocs/history and the PR text, andtools/skill-limits.shhas one non-comment line. Unrelated probes such aspackages/agents/source/lib/lib.sh:149are out of scope. A Bats case also runs the shim over a clean and a failing fixture and compares both streams with the Go command's. - AC4:
mise run generate-skillsleaves no diff, thenmise run check-generated-skillspasses. - AC5: a Bats case puts a stub
agentrepothat records any call first onPATH, runsmise run lint-skills, and asserts the record is empty and the verdict matches. - AC6: a Bats case runs the script with a
PATHlackingmiseand asserts nonzero, empty standard output and a standard error message. - AC7: a new Go test compares the command's streams and status with the recorded data for each corpus case, including the null-byte cases at
internal/skills/check_test.go:49and:53, and asserts the two documented differences and no others. - AC8:
TestDiscoveryandTestExplicitTargetsplus a command-level case for--. - AC9: a Bats case provisions the build first, then runs
tools/skill-limits.shandmise run lint-skillswithGOPROXY=off,MISE_OFFLINE=1and proxy variables pointing at a listener that counts connections, and asserts zero connections and identical repository bytes, index and refs before and after.TestFilesystemOnlyadditionally coversskills.Check. - AC10:
go test ./internal/skillspasses in thetest (ubuntu-24.04),test (ubuntu-24.04-arm),test (macos-15)andtest (windows-2025)jobs, andTestLocaleIndependencecovers locales. - AC11: a Bats case runs the shim through the real launcher against a test command that exits with 0, 1, 2, 3 and 255, and asserts each status, one invocation (a counter file) and unchanged standard error. A stub
miseis not used for this case. - AC12:
git diff origin/main -- '*.sh'shows no added line besides theexec, andgit grep -n -E 'if |case |for ' tools/skill-limits.shreturns nothing. - AC13: the pull request's Verification lists
git tag --containsoutput for the registering commit, the three tags with dates, and the announcement. - AC14:
git grep -n skill-limits docs/script-inventory.md docs/agent-skills-migration.mdshows the updated text. - Definition of done: D1 each AC is verified at the merged head, with the commands and results cited in the pull request's Verification. D2 an independent review approves the exact merged head, and every finding has a recorded disposition. D3 required checks pass with no gate loosened. D4 the leak scans the repository specifies are clean, and every post made for this work passes the per-post leak review before it is posted. D5 every commit, including the squash, is signed. D6 the documentation matches the change (AC14). D7 every commit, including the squash, has an
Assisted-bytrailer and the pull request has theprovenance:agent-assistedlabel. D8 the pull request stays within the size limits. D9 follow-ups and deferred findings are filed as issues with a Problem section and numbered ACs, listed under Related, and linked from any code that defers them. D10 the merge closes the issue through a closing reference, the board shows Done, and the finalStatus:comment names the merge commit.
Risks
A consumer or script that calls tools/skill-limits.sh with its own agentrepo now runs the repository build instead; no caller outside mise.toml exists on origin/main. The release window can stay open for weeks, which only delays the work. If the launcher prerequisite slips, AC11 cannot pass and the change waits rather than shipping a shim that turns every failure into status 1. Undo: revert the change, which restores the shim from #938 and the frozen script.
Operator authority
None.
Activity
tbhb-dev-reviews commentedon Oct 31, 2026
Refinement: NOT READY — release blockers and the Go-only verification contract need clarification
Checked
Gaps
Split
Keep one XS retirement PR after the port and release/window blockers clear. Command integration belongs to #938 and distribution work to its existing issues; no additional implementation split is needed.
Definition of done
Ticks mark coverage promised by the issue's ACs and Verify plan, not implementation already completed.
Round: 1 of 3
tbhb-dev-reviews commentedon Oct 31, 2026
Refinement: NOT READY — native blockers and the replacement invocation still need repair
Checked
Gaps
Round 1 native-dependency gap remains unresolved. The live blocked-by endpoint still returns an empty list. Body text does not create native relationships. Add native blockers for refactor(tools): move skill limit checks to Go #808, feat(skills): route checks through the Go command #938, Ship the Go binaries as release assets installed through mise's github backend #333 and feat(distribution): migrate consumer tool pins to release assets #544. Their rationale is now reconciled with refactor(tools): move skill limit checks to Go #808, and AC13 plus docs/compatibility.md:3 and docs/compatibility.md:5 define release/warning evidence, but the announcement prerequisite is explicitly still unfiled. File and link that issue and its native dependency rather than leaving the warning period unowned. The v0.23.0 tree contains no check-skill-limits registration; the capability release window has not started.
The proposed invocation does not establish AC11. tools/check-versions.sh:4 forwards through mise's run task, which executes Go via
go runat mise.toml:201. That launcher reports a failed child and returns its own failure status rather than forwarding arbitrary child statuses such as 3. A stub mise exiting 3 bypasses this real path and cannot prove the Go command's status survives. Define and verify the real launcher contract, including failure stderr, using a failing child through the actual launch path. Keep launcher behavior in tested Go or existing tooling with only the permitted exec shim. State observable forwarding behavior in AC1 and keep exact launch spelling in Approach.AC7 contradicts a second documented stderr difference. The retained corpus includes null-byte cases at internal/skills/check_test.go:49 and internal/skills/check_test.go:53. internal/skills/parity_unix_test.go:174 removes Bash's “ignored null byte in input” warnings before comparing stderr; merged test(skills): compare the Go checker with the frozen shell checker #972 documents this as B2a. AC7 permits only the invalid-UTF-8 exception. Include null-byte warning normalization in the recorded-expectation contract and Verify row, preserving existing Go behavior. This remains part of Round 1 gap 4.
Verify needs scoped checks and a consistent offline boundary. AC1's repository-wide grep cannot return nothing: unrelated probes remain at packages/agents/source/lib/lib.sh:149 and packages/agents/source/skills/fix-pr/shared/scripts/diagnose.sh:5. Scope the probe assertion to the skill-limits entry point and check legacy references separately. For AC9, TestFilesystemOnly calls skills.Check directly at internal/skills/check_test.go:258; it does not exercise mise, the launcher or RunCheck's Git lookup at internal/skills/check.go:28. The proposed command-level snapshot checks mutation but supplies no network assertion. State whether offline/read-only covers the provisioned checker or the whole supported wrapper, then verify that same boundary with network denial/traps and repository-state checks. Record any provisioning prerequisite explicitly.
Split
Keep one S retirement PR after its native prerequisites and release/warning window clear. File the described announcement issue before work starts. Integration stays in #938 and distribution in #333/#544. If transcript retention would reach 800 changed code lines, land those Go expectations in a preceding PR while the reference exists; keep each PR under 800.
Definition of done
Ticks mark coverage promised by the ACs and Verify plan, not completed implementation. D1 remains open because current verification promises conflict with origin/main.
Round: 2 of 3
tbhb-dev-reviews commentedon Oct 31, 2026
Refinement: NOT READY — two prerequisites remain unowned and verification leaves contract gaps
Checked
Gaps
The new blockers are still proposals rather than owned dependencies. Dependencies explicitly says the warning announcement and real-launcher repair are not owned by any issue. File and refine those bounded prerequisites, then attach their native blocked-by relationships before this item is Ready. Existing native blockers are now correct, but neither refactor(tools): move skill limit checks to Go #808 nor Ship the Go binaries as release assets installed through mise's github backend #333/feat(distribution): migrate consumer tool pins to release assets #544 owns these additional outcomes. The launcher is presently go run at mise.toml:201; the build task writes under bin/ at .devtools/tasks/go.toml:10. The newest local tag, v0.23.0, contains no check-skill-limits registration. AC13 appropriately waits for the command-bearing releases, but the separate two-release removal warning required by docs/compatibility.md:5 still needs its filed owner. A promise to file blockers after refinement does not establish implementation readiness.
AC9's network assertion does not prove its stated boundary. GOPROXY=off, MISE_OFFLINE=1 and proxy variables plus a zero-count proxy listener only observe clients that honor those settings. Direct connections, including subprocess traffic outside that proxy, can bypass the listener. Specify network denial or observation covering the whole wrapper/launcher process tree, and retain the repository/index/ref snapshot. State explicitly whether provisioning includes an already-built binary and other launcher outputs: AC9 currently names only the toolchain/module cache, while Verify provisions the build first. That distinction matters if the launcher creates bin/ during the measured run. TestFilesystemOnly at internal/skills/check_test.go:232 exercises Check directly and cannot establish the wrapper contract.
AC11's universal status promise has only five samples. The AC requires every status from 0 through 255, while Verify checks only 0, 1, 2, 3 and 255. Exercise all 256 through the real launcher with the stated stderr and one-invocation assertions, including statuses such as 126, 127 and 128; alternatively narrow the observable contract to the supported status domain and justify it. The launcher prerequisite must supply the same testable boundary. Keep this in Go/integration coverage without adding shell decision logic.
Split
Keep retirement as one S PR after the announcement and launcher prerequisites are filed, refined and natively linked, and the release/warning conditions clear. Keep launcher implementation outside this PR. The estimate of about 200 added plus 600 deleted lines reaches the hard boundary: split recorded-transcript coverage into a preceding PR if retirement would reach 800 changed code lines, so each PR stays strictly below 800. After this third NOT READY, the owner should rethink the item around those explicit prerequisite issues rather than start another refinement round on the same unresolved scope. No operator decision is needed.
Definition of done
Ticks mark duties covered by the issue's ACs and Verify plan, not completed implementation. D1 remains open because the proposed checks do not yet establish the complete AC9/AC11 contracts.
Round: 3 of 3