Skip to content

backport: add self-review step to instructions#598

Open
TomasTomecek wants to merge 1 commit into
packit:mainfrom
TomasTomecek:backport-review-work
Open

backport: add self-review step to instructions#598
TomasTomecek wants to merge 1 commit into
packit:mainfrom
TomasTomecek:backport-review-work

Conversation

@TomasTomecek

Copy link
Copy Markdown
Member

Summary

  • Adds a self-review checklist (5 criteria) as the final step before the backport agent reports success, covering patch correctness, spec file integrity, no unrelated changes, SRPM existence, and patch naming
  • Extracts shared self-review criteria into _self_review.j2 to avoid duplication between y-stream and z-stream templates
  • Criteria 1, 2a/2b, and 5 are self-guarding for zero-patch scenarios (spec-only changes)
  • Criterion 2b accounts for %autosetup/%autopatch specs that don't need explicit %patch directives
  • Criterion 2c (pre-existing Patch tags intact) always applies, even on the spec-only path
  • Y-stream Criterion 3 qualifies the Release field exception to exclude the spec-only path

Test plan

  • Both templates render identically to pre-dedup output (verified via rendered diff)
  • pytest ymir/agents/tests/unit/test_jinja2_templates.py — 31 tests pass

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a reusable self-review template (_self_review.j2) for backport instructions, integrating it into both standard and z-stream backport instruction templates. The review feedback suggests quoting the SRPM path placeholder in the shell command to prevent word-splitting, and utilizing Jinja2 block assignments ({% set %} ... {% endset %}) instead of escaped multi-line strings to improve template readability and maintainability.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Criterion 4 — Completeness:
Verify the SRPM generated in step {{ srpm_step }} exists on disk. Use the path that the
SRPM generation command printed, and run:
`test -f <path-to-srpm> && echo exists || echo missing`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Quoting the file path placeholder in the shell command is a safer practice to prevent potential word-splitting or syntax errors if the generated SRPM path contains spaces or special characters.

   `test -f "<path-to-srpm>" && echo exists || echo missing`

Comment on lines +245 to +246
{% set spec_only_note = ' Note: If this was a spec-only change (step 3d path), no patch files are\n generated — criteria 1, 2a, 2b, and 5 will not apply. Criterion 2c still\n applies: verify that pre-existing Patch tags were not accidentally modified.' %}
{% set criterion_3_body = ' From the git diff output, verify the `%changelog` section was not modified.\n (Changing the Release field is acceptable in y-stream, unless this was a\n spec-only change via step 3d — in that case the Release field must not be\n modified either.)' %}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using Jinja2's block assignment ({% set var %} ... {% endset %}) is the idiomatic and much more readable way to define multi-line strings in templates. It avoids manual \n escape sequences and improves maintainability.

{% set spec_only_note %}
   Note: If this was a spec-only change (step 3d path), no patch files are
   generated — criteria 1, 2a, 2b, and 5 will not apply. Criterion 2c still
   applies: verify that pre-existing Patch tags were not accidentally modified.
{% endset %}
{% set criterion_3_body %}
   From the git diff output, verify the `%changelog` section was not modified.
   (Changing the Release field is acceptable in y-stream, unless this was a
   spec-only change via step 3d — in that case the Release field must not be
   modified either.)
{% endset %}

{% set self_review_step = 7 %}
{% set repo_context = 'any cloned source repository' %}
{% set spec_only_note = '' %}
{% set criterion_3_body = ' From the git diff output, verify all of the following:\n a. The `%changelog` section was not modified.\n b. Your changes (visible in the diff) did not modify the `Release:` field.' %}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using Jinja2's block assignment ({% set var %} ... {% endset %}) is the idiomatic and much more readable way to define multi-line strings in templates. It avoids manual \n escape sequences and improves maintainability.

{% set criterion_3_body %}
   From the git diff output, verify all of the following:
   a. The `%changelog` section was not modified.
   b. Your changes (visible in the diff) did not modify the `Release:` field.
{% endset %}

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