feat: more copyable prompts#36
Conversation
jaskfla
force-pushed
the
pre-prompts
branch
2 times, most recently
from
3162ba5 to
23a9610
Compare
![review-hero[bot]](https://avatars.githubusercontent.com/in/2896273?s=60&v=4){kind=small}
🦸 Review Hero Summary6 agents reviewed this PR | 1 critical | 0 suggestions | 0 nitpicks | Filtering: consensus 3 voters[object Promise] |
|
🦸 Review Hero Summary |
|
🦸 Review Hero Summary DetailsLocal fix prompt (copy to your coding agent)Fix these issues identified on the pull request. One commit per issue fixed. *** ```md [object Promise] ``` |
jaskfla
force-pushed
the
pre-prompts
branch
2 times, most recently
from
229c697 to
f5c6d3e
Compare
🦸 Review Hero Summary5 agents reviewed this PR | 1 failed | 1 critical | 0 suggestions | 0 nitpicks | Filtering: consensus 3 voters, 1 below threshold Below consensus threshold (1 unique issue not confirmed by majority)
Local fix prompt (copy to your coding agent)
|
🦸 Review Hero Summary6 agents reviewed this PR | 2 critical | 1 suggestion | 0 nitpicks | Filtering: consensus 3 voters, 1 below threshold Below consensus threshold (1 unique issue not confirmed by majority)
Local fix prompt (copy to your coding agent)
`scripts/local-fix-prompt.mjs:32`: Code fence injection: each item is embedded
inside a triple-backtick fence
(`\`\`\`md\n${item}\n\`\`\``), but neither `sanitise()`nor prettier escapes triple-backtick sequences inside the content. A review comment that contains a bare`\n\`\`\``will prematurely close the fence and inject arbitrary markdown into the outer document — including outside the`<details>`block — which ends up in the prompt copied to a coding agent. The attack path is: adversarial string in PR code/comments → echoed verbatim in AI finding → breaks fence in the local fix prompt → prompt injection into the downstream coding agent. Fix: escape any occurrence of three or more consecutive backticks inside`item`before wrapping it in a fence, or use an alternative delimiter that can't appear in the content (e.g. a tilde fence`~~~`). |
🦸 Review Hero Summary6 agents reviewed this PR | 2 critical | 1 suggestion | 0 nitpicks | Filtering: consensus 3 voters, 1 below threshold Below consensus threshold (1 unique issue not confirmed by majority)
Local fix prompt (copy to your coding agent)Fix these issues identified on the pull request. One commit per issue fixed. `scripts/local-fix-prompt.mjs:34`: prettier always appends a trailing newline to
its output, so `formatMarkdown(item)` returns `"item text\n"`. The code block
template becomes `"`md\nitem text\n" + "\n`"` = `"`md\nitem text\n\n`"` — an
empty line appears before every closing fence. This is cosmetically wrong and
then gets passed to the outer `formatMarkdown` call, which may reformat those
blank-line-terminated fences unpredictably. Fix: trim the result before
concatenating: `(await formatMarkdown(item)).trimEnd()`.`scripts/local-fix-prompt.mjs:47`: The template literal indents
`${codeBlocks.join(...)}` with 2 spaces (matching the surrounding `<details>`
body), but JavaScript template literal interpolation only applies that leading
whitespace to the first line of the substituted value. Each subsequent line of a
multi-line code block — the content line(s) and the closing ` `
`— receives 0 spaces of indentation. The resulting string fed to the outer`formatMarkdown`call has opening fences at 2-space indent and closing fences at 0-space indent. Per CommonMark, a closing fence must be indented no more than the opening fence's level, so this technically satisfies the spec, but prettier may renormalize or mangle the indentation in unpredictable ways. Fix: build`codeBlocks`without template-literal indentation (join at column 0), or use`dedent`/manual
string building instead of a multi-line template literal.`scripts/local-fix-prompt.mjs:38`: prettier with parser: 'markdown' recognises
the 'md' language tag on fenced code blocks and will recursively re-format their
content during the outer formatMarkdown() call. This means each item goes
through formatMarkdown twice: once explicitly (line 34) and once implicitly as
part of formatting the outer document. While likely idempotent for simple
content, it is fragile — any item where formatting is not idempotent (e.g. long
lines that get re-wrapped differently in context) will produce unexpected
output. Consider using a language tag that prettier does not format (e.g. 'text'
or 'plain') to prevent the double-format, or skip the per-item formatMarkdown
call and let the outer call handle everything. |
🦸 Review Hero Summary6 agents reviewed this PR | 0 critical | 1 suggestion | 0 nitpicks | Filtering: consensus 3 voters, 1 below threshold Below consensus threshold (1 unique issue not confirmed by majority)
Local fix prompt (copy to your coding agent)Fix these issues identified on the pull request. One commit per issue fixed. `scripts/local-fix-prompt.mjs:35`: prettify() can throw (e.g. if prettier's
markdown parser chokes on the generated string, or a transient runtime error).
Neither buildLocalFixPrompt nor its callers in auto-fix.mjs:591 and
orchestrate.mjs:903 wrap this in try/catch. A prettier failure will propagate up
through main() and prevent any summary comment from being posted at all. Wrap
the prettify call in a try/catch and fall back to returning the unformatted
string if it fails. |
🦸 Review Hero Summary6 agents reviewed this PR | 0 critical | 1 suggestion | 0 nitpicks | Filtering: consensus 3 voters, 2 below threshold Below consensus threshold (2 unique issues not confirmed by majority)
Local fix prompt (copy to your coding agent)Fix these issues identified on the pull request. One commit per issue `scripts/local-fix-prompt.mjs:10`: The `fence` function wraps `str` in
triple-backtick fences without escaping any triple-backtick sequences inside
`str`. If a review comment (even after `sanitise()`) contains a line that is
exactly ` `
`, it will close the opening fence early and inject the remainder of the item as raw markdown outside the fence. In a GitHub comment context this means an attacker who can influence review comment text could inject arbitrary markdown (links, images, HTML) into the summary comment. Fix: before wrapping, replace every occurrence of `
` ` inside `str` with a visually equivalent but non-fence sequence, e.g.
`str.replace(/`{3,}/g, match =>
match.replace(/`/g, '\`'))` or use a tilde fence (`~~~`) and disallow `~~~` in
content instead.
|
🦸 Review Hero Summary6 agents reviewed this PR | 0 critical | 2 suggestions | 0 nitpicks | Filtering: consensus 3 voters, 1 below threshold Below consensus threshold (1 unique issue not confirmed by majority)
Local fix prompt (copy to your coding agent)Fix these issues identified on the pull request. One commit per issue `scripts/local-fix-prompt.mjs:38`: Prettier always appends a trailing newline to
its output. When `fence` receives `"item text\n"`, the result is
`"\`\`\`md\nitem
text\n\n\`\`\`"`— an extra blank line appears before the closing fence. Fix: trim the result before fencing:`return
fence((await formatMarkdown(item)).trimEnd())`.
`scripts/local-fix-prompt.mjs:53`: The template-literal indentation
(` ${codeBlocks.join("\n\n")}`) applies two leading spaces only to the very
first code block. Subsequent blocks after each `\n\n` join separator get no
indentation, making the output inconsistent. Either outdent the whole template
or join with `"\n\n "` so every block gets the same prefix:
`codeBlocks.join("\n\n ")`.
|
| import { format as prettify } from "prettier"; | ||
|
|
||
| async function formatMarkdown(str) { | ||
| return await prettify(str, { parser: "markdown", proseWrap: "always" }); |
There was a problem hiding this comment.
Could increase print width from default of 80. I have no strong opinion here; happy for reviewer to apply this suggestion directly:
| return await prettify(str, { parser: "markdown", proseWrap: "always" }); | |
| return await prettify(str, { | |
| parser: "markdown", | |
| printWidth: 100, | |
| proseWrap: "always", | |
| }); |
🦸 Review Hero Summary6 agents reviewed this PR | 0 critical | 1 suggestion | 0 nitpicks | Filtering: consensus 3 voters, 2 below threshold Below consensus threshold (2 unique issues not confirmed by majority)
Local fix prompt (copy to your coding agent)Fix these issues identified on the pull request. One commit per issue fixed. `scripts/local-fix-prompt.mjs:38`: Prettier always appends a trailing newline to
its output, so `fence(await formatMarkdown(item))` produces
`"`md\n<content>\n\n`"` — an extra blank line appears at the end of every
rendered code block. Fix by trimming the prettier output before fencing:
`return fence((await formatMarkdown(item)).trimEnd())`.
|
| "dependencies": { | ||
| "@actions/core": "^1.11.1" | ||
| "@actions/core": "^1.11.1", | ||
| "prettier": "^3.8.1" |
There was a problem hiding this comment.
I could go either way on this PR. This Prettier works out to be an 8+ MB dependency
| prompt + | ||
| "\n\n</details>" | ||
| const codeBlocks = await Promise.all( | ||
| items.map(async (item) => { |
There was a problem hiding this comment.
If we always had valid Markdown in these items, we could just format the return value <details> ... </details>, because Prettier picks up on the ```md tag.
Formatting each prompt independently so that one badly formatted prompt doesn’t cause the entire output to be unformatted
1 participant
Attempting to do #35, but this approach adds Prettier as a dependency (8.6 MB!!!!), which I’m really not a fan of.
We’ll see where this goes.
🦸 Review Hero