Skip to content

[review-retro] PR #732 — no automated review ran; agent-fix.md has git add -A and incomplete IsProcessing reset #736

@github-actions

Description

@github-actions

PR: feat: agent-fix workflow — label-triggered issue-to-PR pipeline
Merged: 2026-04-22T22:02:25Z by PureWeen
Review runs: 0 automated reviews
Root cause: PR was opened and merged in 3 seconds (22:02:22 → 22:02:25). review-on-open.agent had no time to trigger, let alone complete its 90-minute run.


Skill Coverage Analysis

No review ran, so no skills were invoked. Had a review run, these skills were relevant:

Skill Relevant? Referenced? Gap?
gh-aw-guide ✅ Yes (workflow changes) ❌ No review ran ⚠️ Would have caught git add -A
processing-state-safety ✅ Yes (CopilotService.cs) ❌ No review ran ⚠️ Would have caught incomplete IsProcessing reset
multi-agent-orchestration ✅ Yes (SendViaOrchestrator) ❌ No review ran
performance-optimization ✅ Yes (Organization, Persistence) ❌ No review ran
copilot-sdk-reference ✅ Yes (Events, SDK event handling) ❌ No review ran
android-wifi-deploy ❌ No
maui-ai-debugging ❌ No

Review Accuracy

Metric Count
True Positives 0
True Positives (Fixed) 0
False Positives 0
False Negatives (missed) 2
Unresolved 2

Missed Findings (False Negatives)

1. 🔴 git add -A in agent-fix.md Step 5

File: .github/workflows/agent-fix.md, line 108
Skill: gh-aw-guide / project git conventions

The agent-fix workflow instructs the running agent to commit changes with:

git add -A
git commit -m "fix: ..."

The project convention explicitly forbids this: "NEVER use git add -A or git add . blindly — always review what's being staged first with git status." In an automated agent workflow, git add -A risks committing screenshots (screenshot_*.png), generated files, and other unintended artifacts. The .gitignore partially mitigates this, but not completely (e.g., PolyPilot.app/ patterns catch dirs, not all generated outputs).

Fix: Replace git add -A with git status && git add (specific-files) or instruct the agent to use git add selectively after reviewing git status output.


2. 🟡 Incomplete IsProcessing reset in bridge failure path

File: PolyPilot/Services/CopilotService.cs, lines 3419–3430
Skill: processing-state-safety

The catch block for bridge client send failure resets only 5 fields manually:

session.IsProcessing = false;
session.IsResumed = false;
session.ProcessingStartedAt = null;
session.ToolCallCount = 0;
session.ProcessingPhase = 0;

But ClearProcessingState() resets 19 companion fields including HasUsedToolsThisTurn, ActiveToolCallCount, CancelTurnEndFallback(), CancelToolHealthCheck(), ClearDeferredIdleTracking(), MessageQueue.Clear(), SendingFlag, SuccessfulToolCountThisTurn, ToolHealthStaleChecks, EventCountThisTurn, TurnEndReceivedAtTicks, IsReconnectedSend, and ProcessingClearedAtTicks. Missing these can leave stale state that causes stuck-session symptoms on the next turn.

The processing-state-safety skill documents: "Every code path that sets IsProcessing = false MUST call ClearProcessingState instead of manual field clearing." This path violates that invariant.

Fix: Replace the manual reset with ClearProcessingState(state, accumulateApiTime: false) and replace OnStateChanged?.Invoke() with the standard pattern.


Process Recommendation

This PR was merged 3 seconds after opening, bypassing all automated review. Consider adding a branch protection rule requiring the review / Expert Code Review (auto) status check to pass before merge is allowed. Even a lightweight "review started" check would prevent immediate self-merges on non-trivial PRs.


Auto-generated by review-retro workflow · expires in 30 days

Generated by Review Retrospective for issue #732 ·

  • expires on May 22, 2026, 10:19 PM UTC

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions