Skip to content

fix: invert watchdog op order — mutation before comment strip#53

Merged
samtuckerdavis merged 3 commits intomainfrom
bug/46-watchdog-label-mutation
May 1, 2026
Merged

fix: invert watchdog op order — mutation before comment strip#53
samtuckerdavis merged 3 commits intomainfrom
bug/46-watchdog-label-mutation

Conversation

@OriginalGary
Copy link
Copy Markdown
Contributor

Summary

  • Adds scripts/pipeline_watchdog.py: mutation-first strip_status_done, structured failure logging, and drift_repair_pass for healing stale comment+label drift
  • Adds scripts/tests/test_pipeline_watchdog.py: 6 tests covering all three fixed behaviour paths, each naming the rule it encodes and the mutation that kills it
  • All 6 tests green

Closes #46

Plan link

#46 (comment)

Test plan

  • test_mutation_first_behaviour: asserts gh issue edit --remove-label call precedes gh issue comment call (call-order assertion)
  • test_comment_suppressed_on_mutation_failure: mock returns exit 1; asserts no comment call appears
  • test_comment_posted_on_mutation_success: mock returns exit 0; asserts comment call appears
  • test_failure_logged_on_nonzero_exit: mock returns exit 1; asserts structured JSON entry in orchestrator log with repo, issue, exit_code
  • test_drift_repair_fires_on_stale_comment: issue has strip-comment + status:done; asserts edit call fires
  • test_drift_repair_noop_when_label_absent: issue has strip-comment but status:done gone; asserts no edit call

Adversarial considerations

  • The gh_path parameter accepts caller-supplied binary paths; programmatic callers are internal tooling and trusted. CLI input is not exec'd without argument list construction.
  • The log file is append-only JSON lines; no executable content written.
  • No credentials or tokens are stored or logged.
  • The drift-repair scan reads issue state before attempting mutation — no unconditional mutation on re-run.

…tion-first, failure suppression, drift-repair
Fixes the bug where the pipeline-watchdog posted the strip-announcement
comment before attempting the label mutation. When the mutation failed
silently the comment was the only durable trace, leaving status:done on
the issue.

Three changes:
1. Mutation-first: gh issue edit --remove-label runs before gh issue comment;
   comment is only posted when mutation exits 0.
2. Failure logging: non-zero mutation exit writes a structured JSON entry to
   the orchestrator log (repo, issue, exit_code, stderr).
3. Drift-repair pass: scans a list of (repo, issue) pairs; for each issue
   that still has the strip-comment AND status:done, re-attempts removal.

Closes #46
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 1, 2026

Warning

Rate limit exceeded

@OpenGaryBot has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 10 minutes and 39 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: f4dc13e8-a123-433e-a1d0-893e4dbd8514

📥 Commits

Reviewing files that changed from the base of the PR and between 97e162f and d7a9db5.

📒 Files selected for processing (2)
  • scripts/pipeline_watchdog.py
  • scripts/tests/test_pipeline_watchdog.py
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch bug/46-watchdog-label-mutation
  • 🛠️ fix NAV violations: Commit on current branch
  • 🛠️ fix NAV violations: Create PR

Review rate limit: 0/5 reviews remaining, refill in 10 minutes and 39 seconds.

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

@OriginalGary
Copy link
Copy Markdown
Contributor Author

Adversarial

Security review of scripts/pipeline_watchdog.py.

Input boundaries:

  • repo--repo arg in gh CLI list (no shell=True). Safe.
  • issue_number → typed int, converted via str(). CLI validates with isdigit(). Safe.
  • gh_path → internal tooling parameter only. Acceptable trust model.
  • log_path → parent.mkdir() called on it. Internal tooling. Acceptable.

Log file: Append-only JSON lines via json.dumps(). No raw string interpolation. No executable content. Safe.

subprocess.run: All calls use argument lists, never shell=True. No shell injection possible. Safe.

Comment body: Fixed f-string with only a module constant interpolated. No user-supplied content. Safe.

Drift-repair idempotency: _is_stale checks state before acting. No unconditional mutation. Safe.

No credentials stored. No new third-party dependencies. Only stdlib.

Adversarial scenarios:

  1. Concurrent runs: second call exits non-zero, logs failure, suppresses duplicate comment. No race condition. Safe.
  2. gh not authenticated: exits non-zero, failure logged, comment suppressed. Graceful degradation. Safe.
  3. Injected strip-comment marker: worst case is an unnecessary idempotent label-remove call. Accepted risk.
  4. CLI --log path traversal: fails at file open with permission error. Not a silent data write. Safe.

Verdict: pass

No Critical or High findings.

@samtuckerdavis samtuckerdavis merged commit 7b7bcd6 into main May 1, 2026
6 checks passed
@samtuckerdavis samtuckerdavis deleted the bug/46-watchdog-label-mutation branch May 1, 2026 17:41
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.

[process] pipeline-watchdog status:done strip comment doesn't land label mutation

2 participants