Skip to content

fix(generator): don't double braces in a review prompt that never reaches format() - #935

Open
tend-agent wants to merge 1 commit into
mainfrom
fix/review-prompt-brace-doubling
Open

fix(generator): don't double braces in a review prompt that never reaches format()#935
tend-agent wants to merge 1 commit into
mainfrom
fix/review-prompt-brace-doubling

Conversation

@tend-agent

Copy link
Copy Markdown
Collaborator

Problem

generate_review is the only generator that emits the prompt inside a GitHub Actions expression, and it escapes braces for format() unconditionally — including on the path where format() is never used.

_escape_braces doubles every brace that isn't the {pr_number} placeholder. That is correct going into format(), which collapses each {{ / }} pair back to a single brace. But generate_review only wraps the result in format() when the placeholder was actually found:

format_body, needs_format = _escape_braces(raw_prompt, "pr_number")
escaped = format_body.replace("'", "''")
if needs_format:
    prompt_expr = f"format('{escaped}', github.event.pull_request.number)"
else:
    prompt_expr = f"'{escaped}'"     # <- doubled braces, nothing to collapse them

An adopter whose workflows.review.prompt contains braces but no {pr_number} gets the doubled pairs written straight into a bare string literal. Reproduced against main (fd23cd0) with prompt: "Review this PR. Use the {code-review} skill.":

${{ 'Review this PR. Use the {{code-review}} skill.' }}

Nothing downstream collapses that pair — format() is what does the collapsing and it isn't in the expression. So the agent receives {{code-review}} where the adopter wrote {code-review}, and the doubled }} also lands inside a ${{ … }} expression, which is not a shape the generator should be emitting either way.

Only review is affected: every other workflow renders its prompt as a plain YAML block scalar (block_prompt in macros.yaml.j2, and the literal bodies in ci-fix.yaml.j2 / mention.yaml.j2), so no brace escaping is involved.

Solution

Return the prompt untouched when the placeholder isn't present. Escaping is a format() concern, so it now happens only on the branch that produces a format() call.

Testing

Two tests in test_generate.py pin the two branches — braces preserved without the placeholder, braces doubled and {pr_number}{0} with it. The first fails on main:

assert '{{generated}}' not in "${{ 'Review...rated}}.' }}"
  '{{generated}}' is contained here:
    ${{ 'Review this PR. Skip files matching {{generated}}.' }}

test_prompt_with_numbered_placeholders in test_config_edge_cases.py pinned the old output and had to change — worth a look, since it's a test asserting the defect rather than an incidental snapshot. Its rationale was "escaped to prevent format() runtime errors", but its fixture ("Fix issue {1} and {2}") has no {pr_number}, so no format() call is generated and there is no runtime error to prevent. It now asserts the prompt is emitted verbatim. The sibling test_prompt_with_zero_placeholder, whose fixture does carry {pr_number}, is unchanged and still passes.

Full generator suite: 387 passed. ruff check / ruff format --check at the pinned v0.14.11 are clean.

Scope

No generated file in this repo changes — tend's own .config/tend.yaml sets no custom review prompt, and the default (/tend-ci-runner:review {pr_number}) carries the placeholder, so it takes the format() branch as before. The behavior change is visible only to an adopter with a brace-carrying custom review prompt, whose next regen will drop the spurious doubling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant