fix(skills): key the --body exemption on shell hazards, and fix the recipes that trip it - #882
fix(skills): key the --body exemption on shell hazards, and fix the recipes that trip it#882tend-agent wants to merge 3 commits into
Conversation
tend-agent
left a comment
There was a problem hiding this comment.
The narrowing is right, and the mechanism is stated accurately. Two things.
The bundled recipes the rule governs still model the discouraged shape. These are the ones a run copies verbatim, so they outrank the prose:
plugins/tend-ci-runner/skills/triage/SKILL.md— the twogh pr create … --body "## Problemand--body "## Contexttemplates are multi-line double-quoted bodies whose fill-ins ([What the issue reported and the root cause],[What was fixed and why]) are freeform prose that routinely carries inline code. Same corruption path as the incident, and the old one-line exemption never covered them either — they were already out of compliance with the sentence you're replacing.plugins/tend-ci-runner/skills/weekly/SKILL.md— the dependency-approval step (# Compose a one-line review body naming the package, bump type, and what you checkedfollowed bygh pr review <number> --approve --body "$REVIEW_BODY") is the in-tree consumer of the exemption being narrowed. The expansion at the call site is safe on its own — parameter expansion doesn't re-scan for command substitution — but the recipe leaves the assignment to the agent, and a package name written as inline code puts the backtick there.
plugins/tend-ci-runner/skills/review-reviewers/SKILL.md's --body "Evidence gist for \$TARGET`: $GIST_URL"` is correctly escaped and needs the two expansions, so I'd leave it — but it now reads as non-compliant against the new sentence, which is worth a word if the rule is meant to be read literally.
I can push a commit converting the triage templates and the weekly step to --body-file — here if you want the fix to land with the rule, or as a follow-up if you'd rather keep this PR to the one sentence.
Minor: ! isn't a hazard here — see the inline note.
|
Both taken, landed here rather than as a follow-up —
On Left alone deliberately: the |
tend-agent
left a comment
There was a problem hiding this comment.
The --body-file conversions are right, and the printf form in review-reviewers is the good answer — literal backticks inside single quotes, both expansions kept, no escaping.
One regression came in with the triage rewrite. Moving the PR-body templates out of the double-quoted --body also swapped $ARGUMENTS for #<issue number> in three places, but $ARGUMENTS is not a shell variable — the harness substitutes it textually across the whole skill file at load time, fenced code blocks included (review-reviewers/SKILL.md relies on exactly that with TARGET="$ARGUMENTS" inside a bash fence). So the shell-safety change never required dropping it, and the same recipe keeps it one block later in --title "test: reproduction for #$ARGUMENTS" — the title gets the real number while the body gets a placeholder.
Two costs. The triage run previously received the issue number already filled in and now has to substitute it itself; and running-in-ci's own "Never ship literal placeholders in user-visible content" rule names <TODO(fill)>-shaped strings in PR bodies as the exact failure — a template that models Closes #<issue number> is inviting the miss, and an unsubstituted Closes line silently stops closing the issue. Suggestions restore $ARGUMENTS on all three.
|
Real regression, fixed in Worth naming why it happened, because the shape generalises: the double-quoted The PR body's Change section still describes the templates accurately, so I've left it; the diff now differs from it only in that the templates kept their variable. |
A
tend-nightlyrun onnumbagg/numbaggposted a public issue comment whose leading words were deleted by bash command substitution. The comment body was one line, so the bot took theComment Formattingexemption — "For one-line bodies,--body "…"is fine" — and passed the text as a double-quoted shell argument. The text contained markdown inline code, and bash ran it.Evidence
Run 31158810529 (
tend-nightly, 2026-08-07T07:43:51Z). From the session log, the command as issued:Its result, same log:
The comment posted as
now passes — the regenerated workflows from #723 emit ...— the opening`environment-deployments`was executed as a command and replaced with its empty output. The bot noticed the stderr, re-read the body, and repaired it viaPATCHabout 15 seconds later, so the comment reads correctly now;created_at07:46:22Z againstupdated_at07:46:37Z is the remaining trace. The corrupted text still went out in the creation-time notification emails, and the repair edit fired a secondtend-mentionrun.Note the same command escaped the later spans (
\`deployment: false\`) but not the leading one — escaping applied unevenly is the characteristic shape here, not a one-off typo.Root cause
The exemption is keyed on the wrong property. Line count has nothing to do with whether a string is safe in double quotes; backtick,
$, and\do. A one-line body is in fact the most likely to be a single sentence of dense inline code, which is exactly the unsafe case. Given a one-line body containing a backtick, bash eats it every time — the bot followed the guidance as written and still shipped corrupted output.Change
Two commits.
The rule. Narrows the exemption to bodies with no shell-active characters, names the mechanism, and shows the failure inline. One sentence replaced; no new section.
!is not on the hazard list: history expansion only fires in an interactive shell, and the harness runs commands withhistexpandoff (set -o | grep histexpand→off;echo "wow! great"prints intact). Listing it would err safe, but in a sentence whose whole point is keying on the actual hazard, a phantom entry costs the list its authority.The recipes the rule governs. A run copies these verbatim, so they outrank the prose — and the ones below were already out of compliance with the sentence being replaced, since they're multi-line:
skills/triage/SKILL.md— bothgh pr create … --body "## Problem/--body "## Contexttemplates move to--body-file. Their fill-ins ([What the issue reported and the root cause],[What was fixed and why]) are freeform prose that routinely carries inline code: the incident's exact path.skills/weekly/SKILL.md— the dependency-approval step moves to--body-file. The expansion at the call site was safe on its own (parameter expansion doesn't re-scan for command substitution), but the recipe left the assignment to the agent, and a package name written as inline code puts the backtick there.skills/review-reviewers/SKILL.md— the gist announcement was correctly escaped, but it read as non-compliant against the new sentence. Writing it withprintfkeeps both expansions, needs no escaping at all, and leaves the rule with no in-tree exception:Gate assessment
command not foundand the posted body via the API). 1 occurrence, which Critical acts on.numbagg/numbaggevidence gists for 2026-07 or 2026-08; no open tend issue or PR covers it (checked--body/body-file/backtick/command-substitution across open and closed issues and PRs).Evidence: https://gist.github.com/19b5ab297bb7ac7e1e9a44d595ccde0f