fix(skills): stop the bundled skill using :iid, which is not a placeholder
Why
Reported in #8531 (closed). The bundled glab skill
used :iid in five glab api examples, for both merge requests and issues:
$ glab api projects/:id/merge_requests/:iid
{"error":"merge_request_iid is invalid"}glab: HTTP 400glab api expands a fixed set of placeholders, and iid is not in it:
var placeholderRE = regexp.MustCompile(`:(group/:namespace/:repo|namespace/:repo|fullpath|id|user|username|group|namespace|repo|branch)\b`)Because the regexp never matches, the token is not rejected either — it is passed through and sent to the API verbatim, which is why the failure surfaces as a 400 from GitLab rather than as an error from glab.
A skill is instructions an agent follows literally, so this is not a cosmetic typo. An agent following it as written gets HTTP 400 on every merge request and issue call and, as the reporter notes, typically falls back to guessing.
What
Not by adding :iid as a placeholder. Three of the five uses are issue endpoints
(projects/:id/issues/:iid/notes), and an iid resolved from the current branch's merge
request is meaningless there — it would turn a loud 400 into a request against the wrong
issue.
- the five examples use a real number
- a short Placeholders section lists the tokens that are expanded, says anything else is
sent verbatim, and gives
glab mr view -F json | jq .iidfor getting a real one
Stopping it recurring
api.Placeholders is now the single definition of the set, with placeholderRE built from
it, and a test checks every glab api line in every bundled skill against that list.
The test reads the exported list, not a copy, so a placeholder added or removed cannot silently disagree with the documentation.
The refactor is behaviour-preserving, and that is checked rather than assumed — a temporary
test compared placeholderRE.String() against the original literal and passed, so the
compiled pattern is byte-identical. Alternation order matters here (a longer token must
precede any token that prefixes it), and the slice preserves the original order.
Verification
| check | result |
|---|---|
go test ./internal/commands/skills/... ./internal/commands/api/... |
ok, all packages |
go build ./... |
clean |
make gen-docs then git status |
clean |
Mutation-tested. Putting one :iid back:
--- FAIL: TestBundledSkillsOnlyUseRealPlaceholders
Messages: SKILL.md:184 uses ":iid", which glab api does not expand and sends verbatim:
glab api projects/:id/merge_requests/:iid -X PUT -f "assignee_id=1"It names the file, the line, the token and the offending command, so the next person hits a usable message rather than a bare assertion.
The scan only looks at lines containing glab api, so https://, a -H "Content-Type: ..."
header and {"position_type":"text"} in JSON are all untouched — and the two remaining :iid
mentions in the new prose, which exist to say it is not a placeholder, are correctly ignored.
Closes #8531 (closed)