Skip to content

Check Clean and Fast-Forward Before Resetting Local Main - #939

Closed
ptr727 wants to merge 2 commits into
developfrom
task/fix-checkout-safety-timeout
Closed

Check Clean and Fast-Forward Before Resetting Local Main#939
ptr727 wants to merge 2 commits into
developfrom
task/fix-checkout-safety-timeout

Conversation

@ptr727

@ptr727 ptr727 commented Aug 23, 2026

Copy link
Copy Markdown
Owner

Fixes two real findings the promotion PR #936 review round caught (qodo, against the merge-and-release skill's hub Skills-refresh step):

  • git checkout -B main origin/main force-reset local main unconditionally, without checking the checkout was clean or the update was a fast-forward, which could silently discard local work. Now checks both and stops rather than reconciling either problem.
  • The release-watch bound was a literal timeout placeholder rather than a defined value (CodeRabbit). Now set to 2700 seconds, matching scripts/pr_review.py's own default.

…e release-watch timeout

Two real findings from the promotion PR's own review round: the hub Skills
refresh force-reset local main with git checkout -B main origin/main without
first checking the checkout was clean or the update was a fast-forward, risking
a silent discard of local work under this fleet's no-discard rule. Now checks
both first and stops rather than reconciling either problem. The release-watch
bound was a literal timeout placeholder rather than a defined value, now set to
2700 seconds, matching the pr_review.py script's own default, adjustable when
the maintainer states a different bound for a specific release.
Copilot AI lite review requested due to automatic review settings August 23, 2026 02:34
@coderabbitai

coderabbitai Bot commented Aug 23, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@ptr727, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 39 seconds

Limit details: You’ve used all 10 included reviews currently available.

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a16545c5-574b-4529-a967-99cdf5043ca2

📥 Commits

Reviewing files that changed from the base of the PR and between e11669c and 3fab2a1.

📒 Files selected for processing (4)
  • .agents/skills/merge-and-release/SKILL.md
  • .claude-plugin/fleet-skills/.source-digest
  • .claude-plugin/fleet-skills/skills/merge-and-release/SKILL.md
  • .github/skills/merge-and-release/SKILL.md

Comment @coderabbitai help to get the list of available commands.

Copilot AI 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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Add safety checks before resetting local main; set finite release-watch timeout

🐞 Bug fix ⚙️ Configuration changes 🕐 10-20 Minutes

Grey Divider

AI Description

• Require clean working tree and fast-forward update before force-resetting local main.
• Set gh run watch bound to a finite 2700s timeout (matching scripts/pr_review.py).
• Propagate the updated merge-and-release skill spec to all distributed copies.
Diagram

graph TD
  A(["Skills refresh step"]) --> B[("Local git checkout")] --> C{"Clean tree?"}
  C -- "no" --> X["Stop + report (no-discard) "]
  C -- "yes" --> D["git fetch origin main"] --> E{"Fast-forward?"}
  E -- "no" --> X
  E -- "yes" --> F["git checkout -B main origin/main"] --> G["skills_install.py --report + install"] --> H["gh run watch (timeout 2700s)"]

  subgraph Legend
    direction LR
    _p(["Process"]) ~~~ _d{"Decision"} ~~~ _db[("Repo/State")]
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Auto-stash / auto-clean before updating main
  • ➕ Keeps the workflow moving without manual intervention
  • ➕ Can be convenient for ephemeral CI workdirs
  • ➖ Violates/weakens the stated no-discard-work rule
  • ➖ Stash/apply can fail or introduce subtle state changes
  • ➖ Harder to reason about provenance of local modifications
2. Backup-and-reset (create safety branch/tag then hard reset)
  • ➕ Avoids silent loss by preserving the prior main tip
  • ➕ Still achieves a deterministic checkout for installation
  • ➖ Still mutates local state in potentially surprising ways
  • ➖ Requires naming/cleanup policy for backups
  • ➖ May be undesirable in shared or long-lived workdirs
3. Make timeout a parameter (skill input/env), default 2700s
  • ➕ Allows per-release tuning without editing the skill spec
  • ➕ Keeps the default consistent across tools
  • ➖ Adds configuration surface area and validation needs
  • ➖ May be overkill if overrides are rare and explicitly instructed

Recommendation: Current approach (stop-and-report on dirty/diverged state + fixed default timeout) is the best fit given the explicit no-discard-work rule and the desire for deterministic release monitoring. Consider parameterizing the timeout only if maintainers frequently request different bounds.

Files changed (4) +58 / -37

Bug fix (3) +57 / -36
SKILL.mdDocument safe main refresh guards and set 2700s release-watch timeout +19/-12

Document safe main refresh guards and set 2700s release-watch timeout

• Replaces the placeholder release-watch timeout guidance with an explicit 2700s bound. Adds preconditions to the hub refresh step: require a clean working tree and verify 'origin/main' is a fast-forward of local 'main' before 'git checkout -B' resets it; otherwise stop and report.

.agents/skills/merge-and-release/SKILL.md

SKILL.mdPropagate safe main refresh and 2700s watch timeout to fleet plugin copy +19/-12

Propagate safe main refresh and 2700s watch timeout to fleet plugin copy

• Mirrors the same release-watch timeout (2700s) and the same clean/fast-forward safety gates before resetting local 'main' in the fleet-skills packaged copy.

.claude-plugin/fleet-skills/skills/merge-and-release/SKILL.md

SKILL.mdPropagate safe main refresh and 2700s watch timeout to GitHub skills copy +19/-12

Propagate safe main refresh and 2700s watch timeout to GitHub skills copy

• Mirrors the same release-watch timeout (2700s) and the same clean/fast-forward safety gates before resetting local 'main' in the .github skills copy.

.github/skills/merge-and-release/SKILL.md

Other (1) +1 / -1
.source-digestUpdate fleet-skills source digest for refreshed skill content +1/-1

Update fleet-skills source digest for refreshed skill content

• Updates the plugin source digest to reflect the new merge-and-release skill specification content.

.claude-plugin/fleet-skills/.source-digest

Copilot AI 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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@qodo-code-review

qodo-code-review Bot commented Aug 23, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Broken inline command wrap ✓ Resolved 🐞 Bug ≡ Correctness
Description
The updated SKILL.md wraps an inline backticked command across a newline (``python3\n
scripts/skills_install.py --report``), which can be interpreted/copied as two separate shell
commands when consuming the raw skill text, breaking the hub refresh step.
Code

.agents/skills/merge-and-release/SKILL.md[R113-114]

+   old, pre-fetch commit, and skip the refresh silently. Only then run `python3
+   scripts/skills_install.py --report`, then
Relevance

●●● Strong

Recent accepted precedents consistently fix paste-unsafe or malformed documentation commands and
wrapped prose.

PR-#2
PR-#517
PR-#127

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The PR introduces a new hard-wrapped inline-code command for python3 … --report that spans a
newline inside backticks. Because .agents/skills/ is explicitly the hand-authored source that some
tools read directly, raw newlines in command literals can cause incorrect execution/copying.
CommonMark rendering also normalizes line endings inside code spans, meaning rendered output can
differ from raw source, increasing the risk of copy/paste errors.

.agents/skills/merge-and-release/SKILL.md[110-115]
scripts/build_dist.py[4-9]
🌐 CommonMark code spans normalize content by converting line endings to spaces, so hard-wrapping inside backticks changes the literal text and can introduce copy/paste surprises.
PR-#2

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The skill text now contains inline code spans that include a physical newline (hard-wrapped inside backticks). When the skill is consumed as raw markdown (not rendered), this can be copied/executed as two commands (e.g., running `python3` alone, then trying to execute `scripts/skills_install.py` as a shell program).

### Issue Context
This repo treats `.agents/skills/` as the single hand-authored source that some consumers read directly, so the raw markdown text matters.

### Fix Focus Areas
- .agents/skills/merge-and-release/SKILL.md[113-114]
- .claude-plugin/fleet-skills/skills/merge-and-release/SKILL.md[113-114]
- .github/skills/merge-and-release/SKILL.md[113-114]

### Suggested change
Rewrite multi-line commands as fenced code blocks (triple backticks) with one command per line, or keep each inline backticked command on a single physical line (no newlines inside backticks).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 68 rules
✅ Skills: 5 invoked
  comment-and-doc-style
  dotnet-codestyle
  python-codestyle
  shell-codestyle
  workflow-ci-contract
✅ Web pages:
  +2 more
Review mode: ⚖️ Balanced: This changes runtime release workflow instructions governing checkout safety and bounded waits across three copies; it has meaningful destructive-operation and operational-risk implications, but the logic is localized rather than defect-dense enough for extended review.

Grey Divider

Tip of the day
💡 Did you know, you can turn these tips off under Display preferences

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread .agents/skills/merge-and-release/SKILL.md Outdated

Copilot AI 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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Distinct from the earlier-disproven CommonMark rendering claim, qodo caught a
real risk here: this repo's SKILL.md is also read as raw text by Codex and
opencode, not only rendered. A naive raw-text extraction of a backtick-wrapped
command that itself contains a real newline can split into two shell commands
instead of one. Kept this specific command on a single physical source line.
Copilot AI review requested due to automatic review settings August 23, 2026 02:41

Copilot AI 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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@ptr727

ptr727 commented Aug 23, 2026

Copy link
Copy Markdown
Owner Author

Closing as superseded. This caught a real bug (git checkout -B main origin/main force-resetting local main unconditionally) independently of another session working the same defect at the same time via #936's own review round, #937, and #940. Develop now carries a more thorough fix for that half (git checkout + git merge --ff-only, a clean-tree preflight covering gitignored paths, --exit-code tri-state ls-remote handling, a rev-parse --verify fix, and remote-only cleanup recovery), which is why this PR now shows conflicting against develop.

The other half of this PR, the literal 'timeout ' placeholder in the release-watch step, is still unaddressed on develop and genuinely valid. Filing that as its own targeted follow-up instead of trying to rebase this one.

@ptr727 ptr727 closed this Aug 23, 2026
@ptr727
ptr727 deleted the task/fix-checkout-safety-timeout branch August 23, 2026 04:06
ptr727 added a commit that referenced this pull request Aug 23, 2026
Fixes the still-valid half of #939 (closed as superseded on its other
half, checkout-safety, already fixed more thoroughly on develop via
#937/#940).

Step 6's run-watch bound was a literal `<seconds>` template placeholder,
never a runnable command as written. Filled in `2700` (45 minutes),
matching `scripts/pr_review.py`'s own default (verified:
`ap.add_argument("--timeout", type=int, default=2700, ...)`).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Bug Fixes**
  * Release monitoring now uses a finite 45-minute timeout by default.
* Supports equivalent bounded waiting across different host
environments.
* Clearly distinguishes timed-out runs from completed results and
generated versions.
* Reports failed, timed-out, or unstarted runs without automatically
retrying.

* **Documentation**
* Updated release monitoring guidance to reflect the new timeout and
reporting behavior.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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.

2 participants