Debug E2E Test Timing Issues

Debug Command
Read and follow the full procedure from .claude/skills/e2e/debug.md.
ARGUMENTS: https://github.com/entireio/cli/actions/runs/28549683381
Debug Command
Read and follow the full procedure from .claude/skills/e2e/debug.md.
ARGUMENTS: https://github.com/entireio/cli/actions/runs/28549683381
I have a complete picture. Here's the triage report for run 28549683381 (main, "Merge PR #1596"):
Findings
1. claude-code subagent tests — background Task launches break same-turn checkpointing (new, real gap)
Failed: TestSingleSessionSubagentCommitInTurn (Linux, both attempts; Windows, both attempts), TestSubagentCommitFlow (Windows only)
Timeline from entire-logs/entire.log (Linux, single session 48d21dde…):
| Time | Event |
|---|---|
| 21:49:54 | user-prompt-submit — turn starts |
| 21:49:56.849 | pre-task (PreToolUse[Task]) |
| 21:49:56.891 | post-task 42ms later — "no file changes detected, skipping task checkpoint" |
| 21:49:58–50:01 | stop — "no files modified during session, skipping checkpoint" |
| 21:50:04 | subagent commits docs/red.md → prepare-commit-msg: no active sessions → no trailer |
| 21:50:06–08 | second turn-start/stop pair — main agent re-invoked to report "Done!", tree already clean |
The CI job installs Claude Code unpinned (curl … claude.ai/install.sh), and the newly released version launches Task subagents in the background: the Task tool call returns instantly, the foreground turn ends, and the subagent's file-write + commit land after turn-end with no active session. No trailer, no condensation, so entire/checkpoints/v1 never advances. This is exactly the failure mode the test already documents and skips for copilot-cli (single_session_test.go:94-97).
TestSubagentCommitFlow passed on Linux only by winning the race — its log shows the same instant pre-task→post-task pattern, but the subagent's changes were on disk before the stop hook processed. Entire's claude-code integration only wires PreToolUse[Task]/PostToolUse[Task] (claudecode/hooks.go:117-118); SubagentStop is preserved-but-unused, so there's no hook that fires when a background subagent actually finishes.
Classification: real CLI behavior gap surfaced by an agent behavior change — not test flakiness. The e2e-windows job failure is this same issue.
Fix directions: (a) wire SubagentStop for claude-code so completion of a background subagent triggers task-checkpoint/turn-catchup processing; (b) short-term, extend the copilot-cli skip to cover this or pin the Claude Code version in CI while (a) lands.
2. factoryai-droid factory-hooks tests — Enter swallowed by droid TUI (recurring harness flake)
Failed: TestFactoryTaskCheckpointExistsBeforeCommit, TestFactoryCommittedCheckpointExcludesPreExistingUntrackedFiles — also failed in main runs 28540893227 and 28510928696, so this is persistent.
The failure-time pane.txt shows the prompt still sitting unsubmitted in droid's input box, and comparing pane snapshots shows the text still trickling into the input ~10s after send. TmuxSession.Send (e2e/agents/tmux.go:66) sends the whole prompt, sleeps a fixed 200ms, then sends Enter — droid v0.162.1 processes long pasted input slowly enough that Enter arrives mid-ingest and gets swallowed. Only these two tests fail because they send the longest interactive prompts (~380 chars); the shorter TestInteractive* prompts all passed. Entire hooks show only session-start/turn-start — the agent never ran, so the file never appeared.
Classification: test-harness flakiness, no CLI bug.
Fix direction: in Send, wait for the input echo to stabilize before sending Enter (instead of fixed 200ms), and/or verify submission (input box cleared / spinner visible) and retry Enter once.
Suggested next step
Both fixes are actionable: the droid one in e2e/agents/tmux.go, the claude-code one either as a CLI change (SubagentStop wiring — larger) or a test-level skip + issue. Want me to proceed with /e2e:implement for the harness fix and/or the short-term skip?
Foreground confirmed: pre-task → post-task now spans 21.8s of real subagent work (vs 42ms in the CI failure), and the mid-turn commit got its trailer while the session was still active. The prompt fix demonstrably changes the agent's behavior.
Summary
| Test | Agent(s) | Classification | Action Taken | Verified |
|---|---|---|---|---|
| TestSingleSessionSubagentCommitInTurn | claude-code (Linux + Windows) | agent-behavior change (background subagents) | Prompt hardened: "Run the subagent in the foreground and wait for it to finish; never run it in the background" | ✅ real E2E pass (30.3s), foreground timing confirmed in hook log |
| TestSubagentCommitFlow | claude-code (Windows) | same root cause | Same prompt hardening | ✅ real E2E pass (14.7s) |
| TestFactoryTaskCheckpointExistsBeforeCommit | factoryai-droid | test-bug (harness) | TmuxSession.Send rewritten: waits for input echo to fully render before Enter, then verifies the pane reacted and retries Enter up to 3× | ⚠️ canary green (59/59, exercises Send via Vogon); droid itself can't run locally (no droid CLI / FACTORY_API_KEY) — needs the next CI run to confirm |
| TestFactoryCommittedCheckpointExcludesPreExistingUntrackedFiles | factoryai-droid | test-bug (harness) | Same Send fix | Same as above |
Changed files: e2e/agents/tmux.go, e2e/tests/single_session_test.go, e2e/tests/subagent_commit_flow_test.go. mise run fmt && mise run lint clean (0 issues), canary fully green.
Two things worth noting:
-
The canary caught a real bug in my first
Sendattempt — with a fast agent, the post-Enter snapshot could capture already-completed output, deadlockingWaitFor. Fixed by snapshotting post-echo/pre-Enter, so the snapshot can never contain response output. The two previously-failing canary tests now pass in normal time (12–16s vs 66s timeout). -
The underlying product gap remains: when Claude Code runs a subagent in the background, its file changes and commits land after
turn-endwith no active session — Entire silently loses that work (no trailer, no checkpoint). The prompt fix only steers the tests around it. The real fix is wiring Claude Code'sSubagentStopevent (currently unused inclaudecode/hooks.go) so background-subagent completion triggers checkpointing. Want me to file an issue for that?
Side note: TestInteractiveMultiStep/claude-code fails locally on this machine ("Not logged in" in interactive mode) — I verified via a control run on unmodified tmux.go that it's pre-existing and unrelated to these changes; it passes in CI.
The changes are uncommitted in the working tree — let me know if you want them committed.
Base directory for this skill: /Users/pfleidi/.agents/skills/pfleidi/skills/review
Code Review
1. Determine Diff Against origin/main
Always review changes against origin/main directly. Do not use local main, git merge-base, a PR base branch, or a user-specified alternate base.
Get the CURRENT diff — never use cached results from earlier in the conversation. Include both committed and uncommitted changes (staged + unstaged):
Note: git diff origin/main (not origin/main...HEAD) diffs origin/main against the working tree, capturing committed changes, staged changes, and unstaged changes in one pass.
Show the user the file list and total count. If there are 0 changed files, report that to the user and stop — there is nothing to review. Otherwise, immediately proceed to the review agents. Do NOT wait for confirmation.
Before launching agents, build a concise review context and pass it to every agent. Show the context as a table before launching agents so assumptions are visible:
| Context | Source | Value |
|---|---|---|
| User goal | Conversation | One-line summary, or not provided |
| Implementation plan | Conversation / docs | One-line summary, or not provided |
| PR context | PR title/body | One-line summary, or no PR found |
| Commits | git log --oneline origin/main..HEAD | One-line summary of commit intent |
| Changed surface | diff file list | Main packages/files touched |
| Inferred behavior | commits/tests/docs/user text | Intended behavior change, or diff-only inference |
- The user's request and any implementation plan, design notes, or acceptance criteria provided in the conversation.
- Branch commit messages from
git log --oneline origin/main..HEAD. - PR title/body when a PR exists for the branch.
- The changed-file list and any obvious intended behavior changes inferred from commits, tests, docs, or user-facing text.
Treat this context as the statement of intent. If no implementation plan or PR context exists, say that intent is inferred from the diff and commits only.
2. Spawn Parallel Review Agents
Review Philosophy
Pass these rules to every agent:
- It is OK to find nothing. A clean review is a valid outcome. Do NOT manufacture findings to justify the review. Only flag issues you are confident are real problems.
- Be opinionated and consistent. If a pattern is acceptable, don't flag it. If you flag something, commit to that position — don't suggest the opposite approach on a re-review.
- Don't flag trade-offs with no clear winner. If there are two reasonable approaches and neither is clearly better, don't flag it. The author already made a choice.
- High confidence only. Every finding must pass the bar: "I am confident this is a problem, and I can explain specifically what goes wrong if it's not fixed." Vague unease is not a finding.
- Permission-friendly reads. Avoid shell pipelines, command separators, subshells, and output filters for read-only investigation because they create extra permission prompts and block background review agents. Do not run commands like
git show HEAD:path | sed -n '10,40p'. Use workspace file range reads,rgwith path limits, path-scopedgit diff $BASE -- <path>, or one standalonegit show <rev>:<path>only when the output is acceptably small. - Intent-aware review. Review changed code against the review context, not against the old behavior alone. Do not classify an intentional behavior change as Required merely because it differs from
origin/main. A Required finding must either contradict stated intent, break an existing contract that the intent did not change, introduce a concrete bug/security issue, or leave the intended behavior unverified in a way that would likely fail.
Launch four baseline sub-agents in parallel using the Agent tool. Pass each agent origin/main as the base ref, the full list of changed files, the review context, and the review philosophy above.
When the repository is a Go project and the diff includes Go-related files (*.go, go.mod, or go.sum), also launch Agent 5 in the same batch. Do not run the Go-specific agent for non-Go diffs.
Agent 1: Security & Adversarial
Review git diff $BASE with fresh eyes for:
- Injection — command injection, SQL injection, path traversal
- TOCTOU and race conditions — check-then-act patterns, concurrent access without synchronization
- Unvalidated input at system boundaries — user input, API parameters, external data
- Auth/authz gaps — missing permission checks, privilege escalation paths
- Secrets or credentials — hardcoded tokens, leaked keys, credentials in code or config
For EACH finding: read the actual source file and trace whether the code path is reachable in production. Discard any finding you cannot confirm with a concrete code reference.
Agent 2: Correctness & Quality
Review git diff $BASE for:
- Logic errors — off-by-one, wrong comparison, inverted conditions
- Nil/null handling — unchecked nil dereferences, missing error checks (especially unchecked errors in Go)
- Edge cases in concurrency — goroutine leaks, missing locks, channel misuse, deferred unlock ordering
- Redundant state — state that duplicates existing state, cached values that could be derived
- Production test seams — mutable function variables, package-wide settings, reset hooks, or exported knobs added only so tests can swap behavior instead of using dependency injection or a higher-scope test
- Parameter sprawl — adding new parameters instead of restructuring
- Leaky abstractions — exposing internal details, breaking existing abstraction boundaries
- Stringly-typed code — using raw strings where constants or typed values already exist in the codebase
- Test coverage and scope gaps — changed behavior, edge cases, or error paths not exercised by meaningful tests; tests that prove implementation details instead of behavior; or unit tests used where integration/e2e coverage is the right confidence boundary
- Test helper over-abstraction — helpers that hide the behavior, expected values, or assertions and make the test harder to understand than a small amount of duplication
For EACH finding: verify the claim by reading the source. Check call sites to confirm the issue is real, not hypothetical.
Agent 3: Simplification & Efficiency
Review git diff $BASE for:
- Dead code — unreachable branches, unused functions, struct fields that are never read
- Code reuse — search for existing utilities and helpers that could replace newly written code; flag duplicated functionality
- Copy-paste with variation — near-duplicate blocks that should be unified
- Unnecessary abstractions — wrapper types, indirection, or overly defensive fallbacks that mask errors
- Unnecessary work — redundant computations, repeated file reads, duplicate API calls, N+1 patterns
- Missed concurrency — independent operations run sequentially when they could be parallel
- Hot-path bloat — blocking work added to startup or per-request paths
- Unnecessary existence checks — pre-checking file/resource existence before operating (TOCTOU anti-pattern); operate directly and handle the error
- Unnecessary comments — comments explaining WHAT the code does (well-named identifiers already do that); keep only non-obvious WHY
For EACH suggestion: verify it does not break existing behavior by checking call sites and usages. Discard cosmetic-only suggestions (renames, formatting).
Agent 4: Readability & Go Idioms
Review git diff $BASE for code that is hard to read, maintain, or reason about:
- Poor factoring — functions doing multiple jobs, tangled control flow, or missing helper extraction where a small local helper would clarify behavior
- Mixed abstraction levels — high-level orchestration mixed with low-level IO, parsing, protocol, or data-structure details; low-level helpers that also make workflow or policy decisions
- Generated-code smell — repetitive pasted logic, shallow wrappers, generic names, or code that reads like it was assembled without domain intent
- Data-flow opacity — values transformed across too many steps, unclear ownership, hidden mutation, pass-through helper chains, or state threaded through unrelated code
- Control-flow complexity — deeply nested conditionals, boolean flag plumbing, early returns used inconsistently, or error paths that obscure the main path
- Naming clarity — names that hide domain meaning or force callers to inspect implementation to understand usage
- Go API readability — ambiguous
(result, bool)returns outside clear comma-ok/presence checks, oversized interfaces, unnecessary pointer indirection, or cleverness where explicit Go would be clearer - Error readability — errors that lose operation/context, wrap inconsistently, or make call sites branch on strings/booleans instead of clear errors or typed status
For EACH finding: explain the readability cost in concrete maintenance terms. Prefer small, local refactor suggestions. Discard formatting-only, gofmt-only, or personal taste comments.
Agent 5: Clean Go & Modern Go (Go diffs only)
Use the local pfleidi:clean-go skill as the source of truth: skills/pfleidi/clean-go/SKILL.md.
Review only changed Go code plus surrounding source, tests, interfaces, and call sites needed to verify findings. Apply the skill's Clean Go checks and version-gated Modern Go checks. This includes the modern-go guidance incorporated from JetBrains' use-modern-go skill: detect the relevant go.mod target version, only suggest features available for that version, and do not perform blanket modernization.
Focus on concrete changed-code findings around composable functions, abstraction level, function size/signatures, errors, pointers, small interfaces, any/interface{}, testing guidance from skills/pfleidi/testing/SKILL.md, and modern standard-library helpers. Discard findings that would merely restyle existing code or require a broad rewrite unrelated to the current diff.
Second-Pass Coverage Sweep
After the first-pass agents complete, run a second independent review pass before synthesis. The goal is recall: catch high-confidence findings that the lens-specific agents may have missed.
Launch one fresh coverage agent with origin/main as the base ref, the full list of changed files, the review context, and the review philosophy above. Do not pass the first-pass findings to this agent.
Ask the coverage agent to:
- Re-read the changed files and the surrounding code needed to understand each changed path.
- Trace changed behavior through callers, callees, tests, configuration, migrations, generated interfaces, and user/API entry points where relevant.
- Search the repository for related patterns, duplicated logic, and existing helpers that affect the changed code.
- Look across all lenses together: security, correctness, tests, simplification, readability, performance, and Go cleanliness when applicable.
- Prioritize missed Required findings over optional improvements.
- Return only high-confidence findings with concrete file:line evidence and a short explanation of the traced path.
Then compare the second-pass findings with the first-pass findings. Deduplicate overlaps, verify any new claim by reading source yourself, and discard anything that cannot be confirmed.
3. Synthesize Report
After all launched agents complete:
- Collect findings from both the first-pass agents and the second-pass coverage sweep
- Deduplicate — merge findings from different agents that point to the same underlying issue
- Verify — for any finding where the agent did not cite a specific file:line with evidence, read the source and confirm or discard it
- Group by file
- Sort by severity within each file: Critical > High > Medium > Low
Severity Definitions
- Critical — Must fix before merge. Bugs, security vulnerabilities, data loss risk, race conditions with observable impact.
- High — Should fix before merge. Missing error handling, meaningful test gaps, performance issues on hot paths.
- Medium — Worth fixing. Code reuse opportunities, unnecessary complexity, readability problems that make future changes error-prone, minor efficiency improvements.
- Low — Optional. Minor readability improvements or cosmetic suggestions.
Relevance Classification
For each finding, classify as:
- Required — The change does not work correctly without this fix in light of the review context. Bugs, missing error handling that causes failures, security vulnerabilities, race conditions, contradictions of stated intent, or missing tests for intended behavior that would likely fail. The branch should not merge without addressing these.
- Improvement — Valid finding, but the change works correctly without it. Better factoring, clearer Go APIs, using existing helpers, code reuse, unnecessary complexity, style. Worth addressing in a follow-up, not in this branch.
Autofix Eligibility
Mark each Required finding as Autofix eligible or Needs decision:
- Autofix eligible — source-backed, high confidence, minimal fix is clear, no new dependencies, no shared/public interface change, no product/design choice, no broad refactor, and the directly related verification path is clear.
- Needs decision — any Required finding that fails one of the autofix checks, including intentional behavior questions, API shape changes, cross-cutting refactors, or fixes where multiple reasonable approaches exist.
Present findings as compact tables, not prose blocks. Use one summary table for scanning and one details table for evidence and fixes.
Summary table format:
| # | Severity | Sources | Location | Classification | Autofix | Issue | Impact |
|---|---|---|---|---|---|---|---|
| 1 | Medium | correctness + coverage | cmd/entire/cli/checkpoint/v2_committed.go:234 | Required | Eligible | One-sentence problem. | Concrete consequence if not fixed. |
Details table format:
| # | Evidence | Suggested fix | Trade-offs |
|---|---|---|---|
| 1 | Source-backed confirmation from code path, call site, or test gap. | Concrete code change, not vague advice. | One sentence, or None if strictly better. |
Keep table cells short and scannable. Put the smallest useful quote or evidence in the table rather than full paragraphs. Escape | characters inside code or text so the table remains valid Markdown. Use n/a for Autofix on Improvements. The Sources column lists the agents that independently found or confirmed the issue, such as security, correctness, readability, clean-go, or coverage.
If no findings exist at a severity level, omit that section.
If there are 0 findings across all agents, report that the review is clean and stop.
4. Present Report and Proceed With Default Fixes
Present findings in two sections:
Required
Table of findings classified as Required, sorted by severity. Include the Autofix value for each finding. Follow it with the details table for those same Required findings.
Improvements (follow-up)
Table of findings classified as Improvement, continuing the numbering. These are presented for awareness but are NOT included in the fix cycle by default. Follow it with the details table for those same Improvement findings.
End with a one-paragraph summary: total required vs improvement findings, overall merge-readiness assessment, and any patterns across files.
Before editing, present a planned-autofix table for Autofix eligible Required findings:
| # | Location | Planned change | Related test/verification | Files expected |
|---|---|---|---|---|
| 1 | path/file.go:42 | Minimal code change to address the finding. | Focused test or lint/build command. | path/file.go, path/file_test.go |
Do not ask the user to choose a mode. Immediately proceed to Step 5 for Autofix eligible Required findings after showing the planned-autofix table. Do not fix Improvements by default.
If there are Required findings but none are Autofix eligible, stop after the report and list the exact decisions needed.
5. Fix Cycle
Scope Rules
- Make the MINIMAL change that addresses the finding
- Keep the diff limited to files and lines directly required by the finding
- First decide whether the finding is local or systemic. Fix at the narrowest correct level; do not add a local workaround that hides a shared/root-cause bug.
- If the finding requires a behavior-changing code fix, add or update the directly related test in the same fix step. Prefer TDD, but complete the focused red-to-green cycle before stopping: write/update the failing test, confirm it fails, implement the fix, confirm the focused test passes. Do not stop after only adding the failing test unless the user explicitly asks.
- Do NOT rename variables, reformat code, or touch lines outside the finding scope
- Do NOT refactor adjacent code, even if it looks related
- Do NOT create any git commits — code changes only
Default Batched Fixes
Fix all Autofix eligible Required findings in report order by default. Do not ask which findings to fix.
Choose an artifact directory using the AGENTS.md temporary artifact rule with agent name pfleidi-review:
- Use
./tmp/pfleidi-review/only when./tmp/already exists and is already ignored. - If no project-local artifact directory is available, do not create file artifacts by default; keep ledger/log/cache information in the response and mark file paths
n/a. Ask before using/tmp/pfleidi-review/or modifying ignore files.
When an artifact directory is available, create a temporary fix ledger at <artifact-dir>/review-<repo-name>-<timestamp>.md before editing. If no artifact directory is available, keep the same ledger fields in the final summary table instead. Update the ledger after each finding with:
- Finding number, status, and source location.
- Files touched.
- What changed and why.
- Related tests or verification commands.
- Rollback notes sufficient for the user to understand how to revert the finding-specific change manually.
For each Autofix eligible finding:
- Read the relevant code to confirm the fix approach
- Re-check eligibility before editing; if the fix is no longer clearly eligible, mark it
Needs decisionand continue to the next finding - Implement the fix — ONLY the code changes for that single finding
- Add or update the directly related test in the same diff when the fix changes behavior; if using TDD, complete red-to-green before moving on; if no test is added, state why
- Keep the diff limited to files and lines directly required by that finding
- If a fix would require changing a function signature in a shared interface, adding a dependency, expanding scope outside the finding, or making an ambiguous product/design choice, skip that finding as
Needs decisionand continue - Track the exact files changed, what changed, and why the change addresses the finding
If a skipped finding has partial edits, remove only your own partial edits for that finding before continuing. If you cannot safely isolate those partial edits, stop and explain the conflict.
After all eligible fixes are applied, proceed directly to Step 6 (Verify Fixes). Do NOT show a diff yet.
6. Verify Fixes
Run the project's compile/build, lint, and test commands scoped to only the changed files and their directly related tests. Use safe background batches for independent validators instead of running every command sequentially.
When selecting verification commands, reuse <artifact-dir>/verification-<repo-name>.md if an artifact directory is available and the cache is fresh under the cache rules from pfleidi:pr; otherwise discover the smallest relevant lint/test/build commands. Update the cache only when an artifact directory is available.
- Build / compile — run a relevant compile/build command when one is discoverable for the changed production code.
- Lint / static analysis — run the project's documented lint task, scoped to the files that were modified by the fixes when the task supports scoping. Prefer lint-specific task wrappers such as
make lintormise run lintover invoking linter binaries directly. Do not use aggregatecheck,ci, orverifytasks unless you have confirmed they only run lint/static analysis. If the documented lint task cannot be scoped, run the smallest relevant project lint task. - Tests — run only the test files that cover the modified code (same package, same module, co-located test files). Do NOT run the full test suite.
If no compile/build command or project lint task exists, state that explicitly instead of assuming an unavailable command.
Run formatters, generators, snapshot updates, or other mutating commands alone before validators that depend on their output. Run independent read-only validators concurrently when they do not require the same exclusive service, port, database, fixture directory, or generated output. Keep integration/e2e/service-backed commands separate unless the project documents that they are parallel-safe.
For each background batch, start every command from the same working-tree state, capture stdout/stderr/exit status from the tool, do not edit files while the batch is running, and wait for every command to finish. Run each selected validator directly, for example mise run lint, go test ..., or npm test -- .... Do not wrap validators in sh -c, shell redirection, tee, command separators, or pipelines solely to write logs; that defeats command-prefix approvals and causes extra permission prompts. If an artifact directory is available and file logs can be written after the command completes without rerunning through a shell wrapper, save them under <artifact-dir>/logs-<repo-name>-<timestamp>/; otherwise mark the full-log path as n/a. If files change after a failed batch, none of that batch's successful results count as current verification.
Show verification as a compact table:
| Command | Exit | Relevant output | Full log |
|---|---|---|---|
go test ./pkg/foo -run TestBar -count=1 | 0 | Short success excerpt. | <artifact-dir>/logs-.../go-test-pkg-foo.log or n/a |
For failures or short outputs, show complete output in the relevant-output column or immediately below the table. For long successful outputs, show the relevant excerpt and log path.
On Failure: Auto-Fix
If compile/build, lint, or tests fail due to issues introduced by the review fixes:
- Read the error output and identify every failure
- Fix all issues — apply the minimal changes needed
- Re-run the failing commands using the same safe batching rules
- Show the complete output again
Cap at 2 fix attempts. If still failing after 2 rounds, present the remaining failures to the user with full output. Do not describe the work as done or commit-ready while compile/build status is failing or unknown for changed production code.
7. Present Results
Once verification passes (or the user has seen the remaining failures), present a summary table. If compile/build did not pass for code changes, say the work is not commit-ready before the table. Do NOT show a diff — the Edit tool already showed each change inline.
| # | Finding | File | Status | What changed | Why |
|---|---|---|---|---|---|
| 1 | Description | path:line | Fixed | Added nil check in foo | Prevents panic on missing config |
| 3 | Description | path:line | Fixed | Added focused test for invalid input | Verifies intended error path |
| 6 | Description | path:line | Needs decision | None | API shape has two reasonable options |
Also present a rollback/traceability table and the ledger path:
| # | Files touched | Rollback notes |
|---|---|---|
| 1 | path/file.go, path/file_test.go | Revert the nil-check and focused test added for finding #1. |
Ledger: <artifact-dir>/review-<repo-name>-<timestamp>.md, or n/a when no project-local artifact directory was available.
STOP and wait. The user decides what to commit.
8. Post-Fix
- If the user asks for changes, apply them
- If the user asks to revert specific fixes, revert ONLY those changes using surgical git checkout of the affected lines
- Do NOT commit. The user handles commits on their own terms.
Verification
| Command | Exit | Relevant output | Full log |
|---|---|---|---|
mise run fmt && mise run lint | 0 | 0 issues. (golangci-lint on formatted tree) | n/a |
mise run test:e2e:canary | 0 | Vogon 59/59 passed, roger-roger 4/4 passed — includes all interactive tests exercising the modified Send + Vogon REPL loop | e2e/artifacts/canary-2026-07-01T16-08-41 |
(The editor's scannererr note on main.go doesn't trip the project lint — pre-existing scanner loop, linter not enabled for it.)
Fix Results
| # | Finding | File | Status | What changed | Why |
|---|---|---|---|---|---|
| 1 | Retried Enter terminates REPL agents | e2e/vogon/main.go:66-72, e2e/agents/tmux.go:83-86 | Fixed | Vogon now continues on empty lines (exits only on exit/quit); tmux retry comment corrected; stale 700ms-sleep comment (finding 3) rewritten to describe current Send semantics | A retry Enter under CI load can no longer kill the Vogon session mid-test; comments now describe the code that is |
| 2, 4–7 | Improvements (raw-capture why-comment, best-effort doc, doc noise, redundant Capture, dead-pane bail) | e2e/agents/tmux.go | Not fixed (follow-up) | None | Valid but the change works correctly without them; listed above for follow-up |
| # | Files touched | Rollback notes |
|---|---|---|
| 1 | e2e/vogon/main.go, e2e/agents/tmux.go | In main.go, restore line == "" to the break condition and the old sleep comment; in tmux.go, revert the retry-loop comment to the "no-op" wording. Both are uncommitted — git checkout -- <file> reverts. |
Ledger: n/a (no ./tmp/ artifact directory in this repo).
No test was added for the fix itself: the changed code is test infrastructure, and the merge-gating canary (which drives Vogon through the full interactive Send path 59 times) is its focused verification — it passed.
The review fix is uncommitted, on top of the two earlier commits. The residual risk noted in the report: roger-roger's external REPL still exits on empty lines, but its CI job only runs the non-interactive TestExternalAgent* set, so the retry can't reach it. Let me know if you want the fix committed (e.g. amended into the Send commit or as a third commit) and whether to also take any of the follow-up improvements.
Committed. The branch now has three commits on top of origin/main:
Working tree is clean, lint and canary were green on exactly this state. Ready for you to push and let the CI E2E workflow confirm the droid fix.
/simplify → 4 cleanup agents in parallel → apply the fixes
You are improving the quality of the changed code, not hunting for bugs. Review
it for reuse, simplification, efficiency, and altitude issues, then fix what you
find. Do not look for correctness bugs — that is what /code-review is for.
Phase 0 — Gather the diff
Run git diff @{upstream}...HEAD (or git diff main...HEAD / git diff HEAD~1
if there's no upstream) to get the unified diff under review. If there are
uncommitted changes, or the range diff is empty, also run git diff HEAD and
include the working-tree changes in scope — the review often runs before the
commit. If a PR number, branch name, or file path was passed as an argument,
review that target instead. Treat this diff as the review scope.
Phase 1 — Review (4 cleanup agents in parallel)
Launch 4 independent review agents via the Agent tool, all in a
single message so they run concurrently. Pass each agent the diff and one of
the four angles below. Each returns its findings with file, line, a
one-line summary, and the concrete cost (what is duplicated, wasted, or
harder to maintain).
Reuse
Flag new code that re-implements something the codebase already has — Grep shared/utility modules and files adjacent to the change, and name the existing helper to call instead.
Simplification
Flag unnecessary complexity the diff adds: redundant or derivable state, copy-paste with slight variation, deep nesting, dead code left behind. Name the simpler form that does the same job.
Efficiency
Flag wasted work the diff introduces: redundant computation or repeated I/O, independent operations run sequentially, blocking work added to startup or hot paths. Also flag long-lived objects built from closures or captured environments — they keep the entire enclosing scope alive for the object's lifetime (a memory leak when that scope holds large values); prefer a class/struct that copies only the fields it needs. Name the cheaper alternative.
Altitude
Check that each change is implemented at the right depth, not as a fragile bandaid. Special cases layered on shared infrastructure are a sign the fix isn't deep enough — prefer generalizing the underlying mechanism over adding special cases.
Phase 2 — Apply the fixes
Wait for all four agents to complete, dedup findings that point at the same line or mechanism, and fix each remaining one directly. Skip any finding whose fix would change intended behavior, require changes well outside the reviewed diff, or that you judge to be a false positive — note the skip rather than arguing with it. Finish with a brief summary of what was fixed and what was skipped (or confirm the code was already clean).
Verification green: lint 0 issues, canary 59/59 + 4/4.
Simplify Summary
Fixed (all in e2e/agents/tmux.go, one mechanism — three agents converged on it):
- Redundant captures eliminated:
waitForInputIngestednow returns the settled pane content it already holds;Senduses it for both thestableAtSendsnapshot and the first iteration'spreEnter(retries still re-capture fresh). Saves 2tmux capture-panesubprocess spawns perSend, and the snapshot is now by construction the exact content the stability wait verified — closing a tiny gap where the pane could shift between the check and the snapshot. - Comment noise removed: deleted the
paneChangedFromdoc comment that restated its signature; extended thewaitForInputIngesteddoc to state its best-effort contract ("gives up after 15s and returns the last capture") — the one non-obvious thing callers couldn't see.
Skipped (noted by agents, out of scope):
copilot-cli.go'sSendoverride still carries the old fixed-200ms + unverified-Enter pattern this branch replaced in the shared path. Unifying it means parameterizing around Copilot's genuinely different submission semantics (Ctrl+S edit mode, autocomplete) — a follow-up, not this diff.
Clean: reuse (no existing helper duplicated — WaitFor/stableContent have different contracts and are already reused where they fit) and altitude (Send hardening belongs in the shared TmuxSession since the race is universal and droid only widened the window; the vogon change is contract hardening, not a special case; prompt pinning correctly targets the synchronous path while the CLI's background-subagent gap stays a deliberate deferral).
The simplify fix is uncommitted in the working tree — say the word and I'll commit it (it'd fit naturally as an amend-style follow-up commit to the Send rewrite).
Base directory for this skill: /Users/pfleidi/.agents/skills/pfleidi/skills/pr
Create Pull Request
1. Understand the Branch
Use origin/main as the comparison base for understanding branch scope. Do not use local main, a local PR target branch, or direct tree diffs such as git diff main or git diff origin/main for scope checks; those can include upstream-only changes and make this branch look like it reverted unrelated work.
Read the commit history to understand the full scope of changes on this branch.
Review the changed file list from the merge base to the current working tree and confirm every changed file belongs to the PR's stated goal:
If unrelated files or commits are present, STOP and report them. Do not create a PR that bundles unrelated work.
2. Sync with origin/main
Before discovering verification commands, bring the branch up to date with origin/main so verification runs against the merged state.
Check that the working tree is clean:
If there are uncommitted changes, STOP and ask the user to commit or stash them before continuing. A sync into a dirty tree creates ambiguous failure states.
Fetch and merge:
Three outcomes:
- Already up to date — no commits to merge. Proceed to step 3.
- Clean merge — merge commit created (or fast-forward applied). Proceed to step 3.
- Conflicts — merge halts with conflicted files. STOP and report each conflicted file. Do NOT auto-resolve; the user must resolve the conflicts and complete the merge commit themselves. Re-run the PR skill after resolution.
3. Discover Project Verification Commands
Inspect the project to determine how to build, lint, and test. Collect candidate commands from these sources, then deduplicate them before running anything:
- Makefile — look for
build,lint,check,test,ci,verifytargets. Read the target recipes to understand what they run. - mise — check for
.mise.tomlor.mise/*.toml. Look for[tasks]definitions covering build, lint, test. If found, usemise run <task>. - CI workflows — read
.github/workflows/*.yml(or.gitlab-ci.yml, etc.) to understand required coverage. CI is the ground truth for what must pass, but CI matrix shards and CI-only wrappers are not automatically local verification commands. - README.md — look for "Development", "Contributing", "Building", or "Testing" sections that document how to run checks.
- Package manager conventions — detect from project files:
go.mod→go build ./...,go vet ./...,go test ./...; do NOT infer a lint command from Go alonepackage.json→ checkscriptsforbuild,lint,testCargo.toml→cargo build,cargo clippy,cargo testpyproject.toml/setup.py→ check for configured linters,pytest
If no lint command exists after checking all sources, state that explicitly instead of assuming an unavailable linter binary.
Reuse Cached Verification Discovery
Before rediscovering commands from scratch, choose an artifact directory using the AGENTS.md temporary artifact rule with agent name pfleidi-pr:
- Use
./tmp/pfleidi-pr/only when./tmp/already exists and is already ignored. - If no project-local artifact directory is available, do not use a verification cache by default. Ask before using
/tmp/pfleidi-pr/or modifying ignore files.
When an artifact directory is available, check for a verification cache at <artifact-dir>/verification-<repo-name>.md. The cache is only an input-token optimization; never commit it and never trust it blindly. If no artifact directory is available, perform normal discovery and skip writing the cache.
Reuse the cache only when all of these are true:
- It names the same worktree root and remote.
- It lists the verification source files it was based on, such as
Makefile,.mise.toml,.mise/*.toml, CI workflow files, README files, and package manifests. - Those source files still exist or are still intentionally absent.
git diff --name-only origin/main -- <source files>shows no branch changes to those source files.
If the cache is missing, stale, or incomplete, perform normal discovery. After discovery, update the cache with:
- Repository root and remote.
- Verification source files inspected.
- Selected command plan grouped by coverage area.
- Commands intentionally skipped as duplicates, aggregate/subtask overlaps, CI-only jobs, or too-slow shard matrices.
- Any assumptions, such as "no documented lint task found."
Deduplicate Verification Commands
Build a command plan by coverage area, not by source. Do not run every command discovered.
- Run at most one command for each coverage area: build/compile, lint/static analysis, unit/core tests, integration tests, e2e/smoke tests.
- Prefer documented local developer tasks over CI-specific commands when they cover the same area.
- Do not run both an aggregate task and its constituent tasks. For example, if
mise run checkruns lint and tests, either runmise run checkalone or run the narrower lint/test tasks, not both. - Treat CI matrix shards as duplicated slices of one suite. Do not run every
*:shard:*command locally when an unsharded local task covers the suite. - If CI has only sharded commands and no local equivalent, ask before running all shards. Otherwise, run the smallest representative or changed-scope test command and note that the full shard matrix remains for CI.
- Do not run CI-only canary/e2e jobs locally by default. Run them only when the PR changes that surface, when the user asks, or when the project documents them as required local PR verification.
Log which sources you used, which duplicate/CI-only commands you skipped, and what commands you will run. If the deduplication rules require asking before slow CI-only coverage, STOP for confirmation; otherwise immediately proceed to step 4.
4. Run Verification and Auto-Fix
Run the deduplicated command plan in the fewest safe batches. Prefer background processing for independent validation tasks instead of running everything sequentially.
The commands should cover, at minimum:
- Build — the project compiles without errors
- Lint / static analysis — no lint warnings or static analysis failures
- Tests — the selected local test coverage passes without duplicating CI shards or aggregate/subtask combinations
Use the exact commands, flags, and build tags found in step 3 for the commands you selected. Do not invent your own flags.
Parallel Verification Rules
Partition the selected commands into dependency-safe batches before running them:
- Run mutating commands alone and before validators that depend on their output. This includes formatters, generators, codegen, migrations, package installation, or commands known to update snapshots, lockfiles, generated files, caches in the repo, or test fixtures.
- Run dependent commands after their prerequisite batch passes. For example, do not start tests that require generated code until generation succeeds.
- Run independent read-only validation commands concurrently in the same background batch. Build, lint/static analysis, typecheck/vet, and unit tests can usually share a batch when they do not mutate the working tree and do not require the same exclusive service, port, database, or fixture directory.
- Keep integration, e2e, or service-backed commands separate unless the project documents that they are parallel-safe.
- If unsure whether two commands are independent, run them sequentially. Correctness of validation beats speed.
For each background batch:
-
Start every command from the same working-tree state.
-
Run each selected validator directly, for example
mise run lint,go test ..., ornpm test -- .... Do not wrap validators insh -c, shell redirection,tee, command separators, or pipelines solely to capture logs; that defeats command-prefix approvals and causes extra permission prompts. -
Capture each command's stdout, stderr, exit status, and command line from the tool output separately.
-
While the batch is running, do not edit files, start auto-fixes, or treat partial output as a result.
-
Wait for every command in the batch to finish, then show verification as a compact table:
Command Exit Relevant output go test ./pkg/foo -run TestBar -count=10 Short success excerpt. -
For failures or short outputs, show complete output in the relevant-output column or immediately below the table. For long successful outputs, show the relevant excerpt and state that the rest was truncated.
-
If any command in the batch fails, treat the whole batch as failed for the fix loop. Results from other commands in that stale batch may help diagnose, but they do not count as passing verification after files change.
On Failure: Fix and Re-verify
If any command fails, do NOT stop. Instead:
- Read the error output and identify every failure
- Fix all issues — apply the minimal changes needed to make the failing command pass
- Re-run the deduplicated verification plan from the top, using the same safe batching rules (not just the previously failing command — fixes can introduce new issues)
- Show the updated verification table again, including complete failure output for any command that still fails
Repeat this cycle until all commands pass. Cap at 3 fix attempts. If verification still fails after 3 rounds, STOP and present the remaining failures to the user with full failure output — do not keep looping.
5. Prompt for Commit
After all verification passes, check for uncommitted changes:
If there are uncommitted changes (from auto-fixes in step 4):
- Show the diff of all uncommitted changes
- Propose a semantically correct commit message using the subject-plus-context style from
AGENTS.md. The message must describe the net fix (e.g., "fix lint warnings in config parser" not "fix issues found during PR prep"). - If compile/build did not pass for code changes, say the work is not commit-ready and do not ask to commit until the gap is resolved or the user explicitly takes over.
- STOP and wait for user approval. The user may edit the message, split the changes, or commit themselves.
If the user approves the commit, do not rerun the full verification suite before committing unless files changed after step 4. If another sanity check is needed, use the commit-time verification scope from AGENTS.md: lint tasks, a compile/build check for code changes, and tests directly related to the changed code only.
If there are no uncommitted changes, proceed directly to step 6.
6. Push the Branch
If the branch has no upstream yet, use git push -u origin HEAD.
7. Create the PR
Determine a concise PR title (under 70 characters) from the commit history and diff.
Set the target base branch from the user-provided PR base, or main when the user did not provide one. Scope checks still use origin/main; the PR target base controls only the GitHub PR destination.
If the user provided a PR target base, set PR_BASE to that branch name instead.
Determine the pushed source branch:
If HEAD_BRANCH is empty, STOP and report that PR creation needs a named local branch.
Determine the GitHub repository slug from the origin remote before writing the PR body:
Extract GITHUB_REPO as <owner>/<repo> from these origin URL forms:
git@github.com:<owner>/<repo>.githttps://github.com/<owner>/<repo>.gitssh://git@github.com/<owner>/<repo>.gitentire://<mirror-host>/gh/<owner>/<repo>
Strip a trailing .git when present. For entire:// remotes, ignore the mirror host and use only the suffix after /gh/; do not use any checkpoint-storage repository URL as the PR target when the entire://.../gh/... origin is available.
If the origin URL does not expose a GitHub repository, try:
If that still cannot identify a repository, STOP and ask the user for the GitHub target.
Use the same branch-only comparison from step 1 ($MERGE_BASE to the current working tree) when deriving the title, PR body, changed-file list, and mostly-Markdown detection. Do not use local main or direct git diff origin/main output for PR description decisions.
Write the PR body to help a reviewer (human or bot) understand the change without re-deriving it from the diff. Include these sections; omit any that genuinely don't apply:
- Why — the motivation: what problem this solves, what behavior was broken or missing, what constraint forced the change. This is the most important section. Be specific so neither a reviewer nor a bot has to infer the reason from the diff alone.
- What changed — a short, factual summary of the net change. One or two sentences; the diff is the source of truth.
- Usage examples — for a new or changed command, API, config option, workflow, or user-facing behavior, show a small realistic example of how to use it and what to expect. For UI work, add screenshot placeholders such as
Before: <screenshot>andAfter: <screenshot>when actual screenshots are not available yet. - Decisions made during development — non-obvious choices from the development process: why one approach over another, why an existing abstraction wasn't reused, why a check lives where it does, what assumptions shaped the implementation, and what constraints were intentionally accepted.
- Technical tradeoffs — when a real engineering tradeoff was made, name the options weighed, what the chosen approach gives up, and why that tradeoff is acceptable. Skip if the change was mechanical with no meaningful alternatives.
- Reviewer notes — only for migrations, deployment ordering, backwards-incompatible behavior, or known follow-up work not in this PR. Skip otherwise.
- Rendered Markdown (for mostly Markdown PRs) — links to the changed Markdown files rendered on GitHub.
Do NOT include:
- A "Test plan" or "Verification" section listing the CLI commands run. Verification already happened in step 4; the transcript doesn't help the reviewer.
- A list of newly added tests. The diff already shows them; the list rots when tests are renamed or moved.
- A file-by-file changes summary. The diff already shows this too.
Choose the PR creation command from the origin URL.
For entire://.../gh/<owner>/<repo> origins, do not use gh pr create. It may still inspect the local git remote and fail to infer a GitHub host even when --repo "$GITHUB_REPO" is supplied. Use the GitHub API through gh api with the already-derived repository slug and pushed branch:
For normal GitHub remotes, use gh pr create:
If a non-entire:// origin hits the same host-inference failure from gh pr create, use this gh api creation path once instead of retrying gh pr create.
Create a ready-for-review PR only when the user explicitly asks for it with wording such as "non-draft", "ready for review", or "publish it as ready". In that case, omit --draft from gh pr create or omit -F draft=true from the gh api call.
Show the PR URL and STOP.
Rules
- Do NOT add "Generated with Claude Code" or any AI attribution anywhere
- Do NOT summarize what you just did after showing the URL
- Treat a PR as "mostly Markdown" when more than half of changed files are Markdown (
.md,.mdx,.markdown) or the main review target is Markdown documentation. In that case, include GitHub rendered links for the changed Markdown files using the pushed branch, e.g.https://github.com/$GITHUB_REPO/blob/<branch>/<path>.md. Link all changed Markdown files when the list is short; for large docs PRs, link the main entry points or most important changed files. - If the user provides a title or description, use it verbatim instead of generating one, but append rendered Markdown links for mostly Markdown PRs unless the user explicitly says not to modify the description.
- If the PR description contains checklist items (
- [ ]), check them (- [x]) if and only if the corresponding step was actually verified during this workflow (e.g., build passed, tests passed, lint passed). Do NOT check items that were not verified. Do NOT leave items unchecked if they were verified.
Here's a workflow run executing the e2e tests on the current branch: https://github.com/entireio/cli/actions/runs/28554213547
Can you check the state on a regular basis and continue debugging if it returns with additional failures?
Watcher is running cleanly now. All nine E2E jobs are in progress (matrix-setup already green). I'll report back as soon as the run completes — if anything fails, I'll pull the artifacts and continue debugging; if it's green, the droid fix is confirmed and the PR can come out of draft.
Base directory for this skill: /Users/pfleidi/.agents/skills/pfleidi/skills/pr-feedback
PR Feedback
1. Identify the PR
If the user provided a PR number or URL, use that. Otherwise, detect from the current branch:
If no PR exists for the current branch, stop and tell the user.
2. Fetch Review Comments
Fetch PR metadata first so comment decisions have intent context:
Also collect branch commits and changed files for context:
Show the PR context as a table before classifying comments:
| Context | Source | Value |
|---|---|---|
| PR | title/body | One-line PR intent |
| Branch | commits | One-line commit summary |
| Changed surface | diff file list | Main packages/files touched |
| Base/head | PR metadata | base <- head |
Fetch unresolved review threads with GraphQL as the primary source of truth. Group work by thread, not by individual REST comment:
Filter to unresolved threads only. If there are no unresolved threads, report that to the user and stop — there is nothing to fix.
If GraphQL pagination indicates more review threads or thread comments are available, paginate before classifying. Do not classify a partial thread set as complete.
Use REST pull-review comments only as a fallback when GraphQL data is incomplete or a thread cannot be mapped to a review comment ID:
When REST fallback is used, deduplicate by GraphQL thread ID first, then by file/line/body/author. Do not present or fix the same review request twice.
3. Parse, Classify, and Group
Use permission-friendly reads while investigating comments. Avoid shell pipelines, command separators, subshells, and output filters for read-only source inspection because they create extra permission prompts and can block background work. Do not run commands like git show HEAD:path | sed -n '10,40p'. Use workspace file range reads, rg with path limits, path-scoped diffs, or one standalone git show <rev>:<path> only when the output is acceptably small.
For each comment, extract:
- Author — who left it
- Author type — bot, automated reviewer, human reviewer, or maintainer
- File and line — where it points
- Body — the actual feedback (verbatim, not paraphrased)
- Thread context — any replies in the same thread (to understand if it was already discussed or resolved conversationally)
- Thread ID and top-level comment ID — the GraphQL review thread ID and the original top-level review comment
databaseIdneeded to reply and resolve. Replies to replies are not supported; if only a reply ID is available, fetch the full thread and use the first/top-level review comment ID.
Group each unresolved review thread into a single finding. If multiple comments in one thread refine or supersede each other, use the latest unresolved reviewer request as the finding and retain the earlier messages as context.
Classify each finding source:
- Bot — GitHub bot, CI system, or linter/static-analysis account such as
github-actions[bot]orcodecov[bot] - Automated reviewer — review-assistant accounts that produce natural-language suggestions, such as Copilot or CodeRabbit
- Human reviewer — non-bot reviewer
- Maintainer — repository owner/member/maintainer when that can be inferred from GitHub metadata
4. Present Findings
Present two separate sections:
Human Comments
Table ordered by:
- Bugs / correctness issues — reviewer identified broken logic or missing error handling
- Design / architecture feedback — structural changes, API shape, naming of public interfaces
- Style / nits — formatting, naming of local variables, minor readability
Use this table format:
| # | Priority | Location | Reviewer | Request | Key quote | Autofix |
|---|---|---|---|---|---|---|
| 1 | Bug | file.go:42 | reviewer | One-line summary of what the reviewer is asking for. | Short verbatim excerpt. | Eligible, or Needs decision with the exact decision needed. |
For automated reviewers, use the same table and set Reviewer to the tool account, with Priority based on the substance of the request.
Bot Comments (batched)
Table continuing the numbering from above, grouped by tool/bot:
| # | Bot | Location | Required fix | Autofix |
|---|---|---|---|---|
| 8 | linter-name | file.go:42 | One-line summary of the required fix. | Eligible, or Needs decision with the exact decision needed. |
Keep table cells short and scannable. Use the smallest useful verbatim quote, not the full comment body. Escape | characters inside code or text so the table remains valid Markdown.
End with a summary: total human comments, total bot comments, overall assessment of effort.
Do not stop for mode selection. After the decision gate below, proceed by default with bot comments and human comments marked Autofix eligible. Mark a human comment Autofix eligible only when the requested change is source-backed, high confidence, minimal, unambiguous, does not require a product/design decision, does not add a dependency, does not change a shared/public interface, and has a clear verification path.
Leave all other human comments unresolved as Needs decision, with the exact decision needed. Do not reject a reviewer comment by default; rejection requires a user-provided public rationale.
Decision Gate Before Fixes
Before applying any fixes, handle every Needs decision finding first. Do not let bot comments or easy autofixes push these questions to the end.
-
Present a short "Decision needed first" table:
# Location Reviewer Decision needed Why it blocks 3 file.go:42reviewerChoose whether the API should return nilor an empty slice.Either answer changes caller behavior. -
Try to answer each decision from source, PR context, existing project patterns, and the full review thread before asking the user.
-
If the answer is source-backed, low risk, and has one clear implementation, reclassify the finding as Autofix eligible and record the reasoning.
-
If the correct answer is "do not change this", record it as a proposed rejection, but do not publish the rejection without a user-provided public rationale.
-
If any finding still needs a product/design call, shared/public interface decision, dependency choice, or other user judgment, STOP before bot or autofix work. Ask for all remaining decisions in one concise list.
-
Continue to Step 5 only after every decision is either answered, reclassified, proposed for rejection with a user-provided rationale, or explicitly deferred by the user. Deferred Needs decision findings remain unresolved and must be listed again in the final summary.
Before applying any fixes, record the starting commit:
Choose an artifact directory using the AGENTS.md temporary artifact rule with agent name pfleidi-pr-feedback:
- Use
./tmp/pfleidi-pr-feedback/only when./tmp/already exists and is already ignored. - If no project-local artifact directory is available, do not create file artifacts by default; keep ledger/log/cache information in the response and mark file paths
n/a. Ask before using/tmp/pfleidi-pr-feedback/or modifying ignore files.
When an artifact directory is available, create a temporary thread ledger at <artifact-dir>/pr-feedback-<pr-number>.md. If no artifact directory is available, keep the same ledger fields in the final summary table instead. Update the ledger after each thread with:
- Thread ID, source category, reviewer, location, and status.
- Files touched.
- What changed and why.
- Related tests or verification commands.
- Planned review-thread reply body, if any.
- Resolve decision: yes/no and why.
5. Fix Bot Comments (batched)
After the decision gate, fix all bot comments first — these are mechanical and clearing them reduces noise before the human-comment phase.
- For each bot finding:
- Read the relevant code
- Implement the fix — ONLY the changes needed for that single finding
- Track the files changed for this finding so the review-thread reply can identify the commit that contains the fix
- If a fix is ambiguous or would conflict with a human-comment fix already applied, mark it Needs decision and continue
- After all bot fixes are applied, present a summary table. Do NOT show a diff — the Edit tool already showed each change inline.
| # | Finding | File | Bot | Status |
|---|---|---|---|---|
| 8 | Description | path:line | linter-name | Fixed |
| 9 | Description | path:line | linter-name | Fixed |
| 11 | Description | path:line | linter-name | Skipped — conflicts with #3 |
- Proceed directly to Step 6.
6. Fix Human Comments (batched)
After bot fixes, work through Autofix eligible human comments in report order:
- State which finding you are addressing (number and one-line description)
- Read the relevant code and the full comment thread to understand intent
- Re-check eligibility before editing; if the fix is no longer clearly eligible, mark it Needs decision and continue
- Implement the fix — ONLY the changes needed for that single finding
- Track the files changed for this finding so the review-thread reply can identify the commit that contains the fix
- If a comment needs a product/design decision, shared/public interface change, dependency, broad refactor, or has multiple reasonable fixes, mark it Needs decision and continue
- If the user rejects the comment instead of fixing it, record the specific rationale to use in the review-thread reply
Scope Rules
- Make the MINIMAL change that addresses the reviewer's feedback
- Keep the diff limited to files and lines directly required by the feedback
- First decide whether the feedback points to a local or systemic issue. Fix at the narrowest correct level; do not add a local workaround that hides a shared/root-cause bug.
- If the feedback requires a behavior-changing code fix, add or update the directly related test in the same fix. Prefer TDD, but complete the focused red-to-green cycle before stopping: write/update the failing test, confirm it fails, implement the fix, confirm the focused test passes. Do not stop after only adding the failing test unless the user explicitly asks.
- Do NOT rename variables, reformat code, or touch lines outside the feedback scope
- Do NOT refactor adjacent code, even if it looks related
- If the reviewer's comment is ambiguous, mark it Needs decision and continue with unrelated unambiguous comments
- Do NOT create any git commits during the fix cycle. Commits are handled only in the publish step, and only with explicit user approval when needed.
7. Verify Fixes
After all fixes are applied, run the project's compile/build, lint, and test commands scoped to only the changed files and their directly related tests. If no code changed, skip verification and proceed to Step 8. Use safe background batches for independent validators instead of running every command sequentially.
When selecting verification commands, reuse <artifact-dir>/verification-<repo-name>.md if an artifact directory is available and the cache is fresh under the cache rules from pfleidi:pr; otherwise discover the smallest relevant lint/test/build commands. Update the cache only when an artifact directory is available.
- Build / compile — run a relevant compile/build command when one is discoverable for the changed production code.
- Lint / static analysis — run the project's documented lint task, scoped to the files that were modified when the task supports scoping. Prefer lint-specific task wrappers such as
make lintormise run lintover invoking linter binaries directly. Do not use aggregatecheck,ci, orverifytasks unless you have confirmed they only run lint/static analysis. If the documented lint task cannot be scoped, run the smallest relevant project lint task. - Tests — run only the test files that cover the modified code (same package, same module, co-located test files). Do NOT run the full test suite.
If no compile/build command or project lint task exists, state that explicitly instead of assuming an unavailable command.
Run formatters, generators, snapshot updates, or other mutating commands alone before validators that depend on their output. Run independent read-only validators concurrently when they do not require the same exclusive service, port, database, fixture directory, or generated output. Keep integration/e2e/service-backed commands separate unless the project documents that they are parallel-safe.
For each background batch, start every command from the same working-tree state, capture stdout/stderr/exit status from the tool, do not edit files while the batch is running, and wait for every command to finish. Run each selected validator directly, for example mise run lint, go test ..., or npm test -- .... Do not wrap validators in sh -c, shell redirection, tee, command separators, or pipelines solely to write logs; that defeats command-prefix approvals and causes extra permission prompts. If an artifact directory is available and file logs can be written after the command completes without rerunning through a shell wrapper, save them under <artifact-dir>/logs-<pr-number>-<timestamp>/; otherwise mark the full-log path as n/a. If files change after a failed batch, none of that batch's successful results count as current verification.
Show verification as a compact table:
| Command | Exit | Relevant output | Full log |
|---|---|---|---|
go test ./pkg/foo -run TestBar -count=1 | 0 | Short success excerpt. | <artifact-dir>/logs-.../go-test-pkg-foo.log or n/a |
For failures or short outputs, show complete output in the relevant-output column or immediately below the table. For long successful outputs, show the relevant excerpt and log path.
If compile/build, lint, or tests fail due to issues introduced by the fixes:
- Read the error output and identify every failure
- Fix all issues — apply the minimal changes needed
- Re-run the failing commands using the same safe batching rules
- Show the complete output again
Cap at 2 fix attempts. If still failing after 2 rounds, present the remaining failures to the user with full output. Do not describe the work as done or commit-ready while compile/build status is failing or unknown for changed production code.
Once verification passes, show a summary: how many comments were addressed, rejected, intentionally left unresolved, or still blocked. If compile/build did not pass for code changes, say the work is not commit-ready before the summary. Do NOT show a diff — the Edit tool already showed each change inline.
Proceed to Step 8 for threads that were addressed or intentionally rejected. Leave Needs decision threads unresolved and do not post replies to them unless the user provided a public rejection rationale. Do not block publishing addressed threads just because unrelated threads still need a decision.
8. Publish PR Updates
After addressed/rejected threads are ready to publish:
-
Check branch state:
-
If there are uncommitted fix changes, STOP and ask the user whether to commit them now or let the user commit manually. Do not push until the fixes are committed. If compile/build did not pass for code changes, say the work is not commit-ready and do not ask to commit until the gap is resolved or the user explicitly takes over. If the user approves committing after verification, stage only files changed for the PR feedback fixes and write the commit message from the actual diff using the subject-plus-context style from
AGENTS.md. -
Push the committed changes for the current branch:
If the branch has no upstream and the push fails for that reason, use:
Never force-push.
-
Map each addressed finding to the commit or commits that contain its fix. Use the recorded starting commit, changed-file tracking, ledger, and
git log/git showto identify the relevant short SHA(s). If one commit fixes multiple comments, reference the same commit in each review-thread reply. -
Build and show a direct review-thread reply plan before calling the API:
Thread Top-level comment ID Status Reply body Resolve PRRT_...123456789Addressed Addressed in abc1234 by adding the nil check before dereferencing.Yes PRRT_...n/aNeeds decision n/aNo Proceed without asking when every actionable reply body is either addressed or a user-approved rejection. Needs decision rows with
Reply body = n/aandResolve = Nodo not block publishing addressed threads only if they already passed the decision gate and were explicitly deferred by the user. Stop before API calls if a rejection lacks a public rationale, if a top-level comment ID is missing for a thread that should receive a reply, or if any reply body is uncertain. -
Post one direct review-thread reply for each addressed or user-approved rejected thread:
-
For addressed comments, state what changed and the commit SHA(s), e.g.
Addressed in abc1234 by adding the nil check before dereferencing. -
For rejected comments, state why it was not changed, e.g.
Not changed: this path already returns before allocation failure can be observed. -
Keep replies concise but specific. Do not over-explain or repeat the diff.
-
Do not reply to comments the user explicitly chose to leave unresolved unless needed to publish a user-approved rejection rationale.
-
Use the REST review-comment reply endpoint:
This endpoint posts an immediate reply to the existing review thread. Do not use
gh pr review, GraphQLaddPullRequestReviewThreadReply,gh pr comment, or the regular issue comment endpoint as a fallback. -
-
Resolve each addressed or user-approved rejected thread after its direct review-thread reply succeeds:
Do not resolve a thread that did not receive a successful direct reply, is still ambiguous, or still needs user approval.
-
Present a final summary table:
Thread Reviewer Location Status Commented Resolved PRRT_...reviewerfile.go:42Addressed Yes Yes PRRT_...reviewerfile.go:99Needs decision No No Include push result, branch, ledger path, rejected comments and rationale, and any comments intentionally left unresolved or not resolvable via API.
9. Post-Publish
- If the user asks for adjustments, apply them
- If the user asks to revert specific fixes, revert ONLY those changes
- Do NOT commit or push additional changes unless explicitly asked
Verification green:
| Command | Exit | Relevant output | Full log |
|---|---|---|---|
mise run fmt && mise run lint | 0 | 0 issues. | n/a |
mise run test:e2e:canary | 0 | Vogon 59/59, roger-roger 4/4 | e2e/artifacts/canary-2026-07-01T16-28-58 |
Status: 2 threads addressed (#2 partially — consecutive-stable-polls adopted, stableContent part declined with technical explanation; #3 fixed), 1 rejection approved by you (#1).
The fixes are uncommitted. Proposed commit:
fix(e2e): harden input-ingestion wait against bursty renderingRequire two consecutive stable polls in waitForInputIngested so a single quiet interval mid-paste can't fake stability, and document why it compares raw captures instead of stableContent. Re-print the vogon prompt when ignoring an empty line so manual REPL sessions stay readable.
Addresses PR #1599 review feedback.
Commit with this message, then I'll push and reply/resolve the threads?
All verification green (lint 0 issues, canary 59/59 + 4/4).
CI run 28554213547 debrief
| Job | Result | Meaning |
|---|---|---|
| claude-code, e2e-windows | ✅ | Foreground-subagent prompt fix confirmed in CI |
| all other agents | ✅ | No regressions from the Send rewrite |
| factoryai-droid | ❌ same 2 factory tests | Progress, new failure mode: prompt now submits (UserPromptSubmit · 1 hook · 1 completed, Worker "Inspect code and write findings" → Subagent session started) — the swallowed-Enter bug is fixed. The tests failed because droid's PromptPattern() is a bare > that matches the always-visible input box (> Enter to steer), so WaitFor returns mid-turn, and the 10s file wait expires while the Worker (60–120s on CI with haiku) is still running. |
Fix applied: file wait 10s → 90s and rewind-point wait 15s → 30s in both factory tests, with a comment explaining why the file wait must absorb the Worker runtime. Fits the 3-minute test budget; all waits return early on success.
There are now two uncommitted change sets awaiting your approval:
Commit 1 — PR feedback (from before the CI result):
fix(e2e): harden input-ingestion wait against bursty renderingRequire two consecutive stable polls in waitForInputIngested so a single quiet interval mid-paste can't fake stability, and document why it compares raw captures instead of stableContent. Re-print the vogon prompt when ignoring an empty line so manual REPL sessions stay readable.
Addresses PR #1599 review feedback.
Commit 2 — droid Worker runtime:
fix(e2e): give factory hook tests time for the Worker turnDroid's prompt pattern matches the always-visible input box, so WaitFor can return mid-turn; the 10s file wait then expires while the Worker is still executing (60-120s turns on CI). Widen the file and rewind-point waits to absorb the Worker runtime.
Approve both (I'll then push and reply/resolve the two addressed Copilot threads on #1599, leaving the rejected one for the reply with your approved rationale)?
Everything is in flight:
- Committed & pushed:
39568096f7(PR feedback: consecutive stable polls + vogon prompt re-print) anda3db5ced4e(factory test waits widened for the Worker turn) →origin/fix-e2e-tests - PR #1599: all 3 Copilot threads replied to and resolved (2 addressed, 1 rejected with your approved rationale)
- CI: droid-only E2E run dispatched on the branch (run 28556048126) — the last open question is whether the widened waits let the two factory tests pass now that the prompt submits. Watcher polls every 60s; I'll report and dig into artifacts if it comes back red. The push-triggered Lint/Tests checks are also running (local equivalents were green).
The droid job took ~45 min on the full matrix run, so expect the verdict in roughly that window.
Base directory for this skill: /Users/pfleidi/.agents/skills/pfleidi/skills/pr-feedback
PR Feedback
1. Identify the PR
If the user provided a PR number or URL, use that. Otherwise, detect from the current branch:
If no PR exists for the current branch, stop and tell the user.
2. Fetch Review Comments
Fetch PR metadata first so comment decisions have intent context:
Also collect branch commits and changed files for context:
Show the PR context as a table before classifying comments:
| Context | Source | Value |
|---|---|---|
| PR | title/body | One-line PR intent |
| Branch | commits | One-line commit summary |
| Changed surface | diff file list | Main packages/files touched |
| Base/head | PR metadata | base <- head |
Fetch unresolved review threads with GraphQL as the primary source of truth. Group work by thread, not by individual REST comment:
Filter to unresolved threads only. If there are no unresolved threads, report that to the user and stop — there is nothing to fix.
If GraphQL pagination indicates more review threads or thread comments are available, paginate before classifying. Do not classify a partial thread set as complete.
Use REST pull-review comments only as a fallback when GraphQL data is incomplete or a thread cannot be mapped to a review comment ID:
When REST fallback is used, deduplicate by GraphQL thread ID first, then by file/line/body/author. Do not present or fix the same review request twice.
3. Parse, Classify, and Group
Use permission-friendly reads while investigating comments. Avoid shell pipelines, command separators, subshells, and output filters for read-only source inspection because they create extra permission prompts and can block background work. Do not run commands like git show HEAD:path | sed -n '10,40p'. Use workspace file range reads, rg with path limits, path-scoped diffs, or one standalone git show <rev>:<path> only when the output is acceptably small.
For each comment, extract:
- Author — who left it
- Author type — bot, automated reviewer, human reviewer, or maintainer
- File and line — where it points
- Body — the actual feedback (verbatim, not paraphrased)
- Thread context — any replies in the same thread (to understand if it was already discussed or resolved conversationally)
- Thread ID and top-level comment ID — the GraphQL review thread ID and the original top-level review comment
databaseIdneeded to reply and resolve. Replies to replies are not supported; if only a reply ID is available, fetch the full thread and use the first/top-level review comment ID.
Group each unresolved review thread into a single finding. If multiple comments in one thread refine or supersede each other, use the latest unresolved reviewer request as the finding and retain the earlier messages as context.
Classify each finding source:
- Bot — GitHub bot, CI system, or linter/static-analysis account such as
github-actions[bot]orcodecov[bot] - Automated reviewer — review-assistant accounts that produce natural-language suggestions, such as Copilot or CodeRabbit
- Human reviewer — non-bot reviewer
- Maintainer — repository owner/member/maintainer when that can be inferred from GitHub metadata
4. Present Findings
Present two separate sections:
Human Comments
Table ordered by:
- Bugs / correctness issues — reviewer identified broken logic or missing error handling
- Design / architecture feedback — structural changes, API shape, naming of public interfaces
- Style / nits — formatting, naming of local variables, minor readability
Use this table format:
| # | Priority | Location | Reviewer | Request | Key quote | Autofix |
|---|---|---|---|---|---|---|
| 1 | Bug | file.go:42 | reviewer | One-line summary of what the reviewer is asking for. | Short verbatim excerpt. | Eligible, or Needs decision with the exact decision needed. |
For automated reviewers, use the same table and set Reviewer to the tool account, with Priority based on the substance of the request.
Bot Comments (batched)
Table continuing the numbering from above, grouped by tool/bot:
| # | Bot | Location | Required fix | Autofix |
|---|---|---|---|---|
| 8 | linter-name | file.go:42 | One-line summary of the required fix. | Eligible, or Needs decision with the exact decision needed. |
Keep table cells short and scannable. Use the smallest useful verbatim quote, not the full comment body. Escape | characters inside code or text so the table remains valid Markdown.
End with a summary: total human comments, total bot comments, overall assessment of effort.
Do not stop for mode selection. After the decision gate below, proceed by default with bot comments and human comments marked Autofix eligible. Mark a human comment Autofix eligible only when the requested change is source-backed, high confidence, minimal, unambiguous, does not require a product/design decision, does not add a dependency, does not change a shared/public interface, and has a clear verification path.
Leave all other human comments unresolved as Needs decision, with the exact decision needed. Do not reject a reviewer comment by default; rejection requires a user-provided public rationale.
Decision Gate Before Fixes
Before applying any fixes, handle every Needs decision finding first. Do not let bot comments or easy autofixes push these questions to the end.
-
Present a short "Decision needed first" table:
# Location Reviewer Decision needed Why it blocks 3 file.go:42reviewerChoose whether the API should return nilor an empty slice.Either answer changes caller behavior. -
Try to answer each decision from source, PR context, existing project patterns, and the full review thread before asking the user.
-
If the answer is source-backed, low risk, and has one clear implementation, reclassify the finding as Autofix eligible and record the reasoning.
-
If the correct answer is "do not change this", record it as a proposed rejection, but do not publish the rejection without a user-provided public rationale.
-
If any finding still needs a product/design call, shared/public interface decision, dependency choice, or other user judgment, STOP before bot or autofix work. Ask for all remaining decisions in one concise list.
-
Continue to Step 5 only after every decision is either answered, reclassified, proposed for rejection with a user-provided rationale, or explicitly deferred by the user. Deferred Needs decision findings remain unresolved and must be listed again in the final summary.
Before applying any fixes, record the starting commit:
Choose an artifact directory using the AGENTS.md temporary artifact rule with agent name pfleidi-pr-feedback:
- Use
./tmp/pfleidi-pr-feedback/only when./tmp/already exists and is already ignored. - If no project-local artifact directory is available, do not create file artifacts by default; keep ledger/log/cache information in the response and mark file paths
n/a. Ask before using/tmp/pfleidi-pr-feedback/or modifying ignore files.
When an artifact directory is available, create a temporary thread ledger at <artifact-dir>/pr-feedback-<pr-number>.md. If no artifact directory is available, keep the same ledger fields in the final summary table instead. Update the ledger after each thread with:
- Thread ID, source category, reviewer, location, and status.
- Files touched.
- What changed and why.
- Related tests or verification commands.
- Planned review-thread reply body, if any.
- Resolve decision: yes/no and why.
5. Fix Bot Comments (batched)
After the decision gate, fix all bot comments first — these are mechanical and clearing them reduces noise before the human-comment phase.
- For each bot finding:
- Read the relevant code
- Implement the fix — ONLY the changes needed for that single finding
- Track the files changed for this finding so the review-thread reply can identify the commit that contains the fix
- If a fix is ambiguous or would conflict with a human-comment fix already applied, mark it Needs decision and continue
- After all bot fixes are applied, present a summary table. Do NOT show a diff — the Edit tool already showed each change inline.
| # | Finding | File | Bot | Status |
|---|---|---|---|---|
| 8 | Description | path:line | linter-name | Fixed |
| 9 | Description | path:line | linter-name | Fixed |
| 11 | Description | path:line | linter-name | Skipped — conflicts with #3 |
- Proceed directly to Step 6.
6. Fix Human Comments (batched)
After bot fixes, work through Autofix eligible human comments in report order:
- State which finding you are addressing (number and one-line description)
- Read the relevant code and the full comment thread to understand intent
- Re-check eligibility before editing; if the fix is no longer clearly eligible, mark it Needs decision and continue
- Implement the fix — ONLY the changes needed for that single finding
- Track the files changed for this finding so the review-thread reply can identify the commit that contains the fix
- If a comment needs a product/design decision, shared/public interface change, dependency, broad refactor, or has multiple reasonable fixes, mark it Needs decision and continue
- If the user rejects the comment instead of fixing it, record the specific rationale to use in the review-thread reply
Scope Rules
- Make the MINIMAL change that addresses the reviewer's feedback
- Keep the diff limited to files and lines directly required by the feedback
- First decide whether the feedback points to a local or systemic issue. Fix at the narrowest correct level; do not add a local workaround that hides a shared/root-cause bug.
- If the feedback requires a behavior-changing code fix, add or update the directly related test in the same fix. Prefer TDD, but complete the focused red-to-green cycle before stopping: write/update the failing test, confirm it fails, implement the fix, confirm the focused test passes. Do not stop after only adding the failing test unless the user explicitly asks.
- Do NOT rename variables, reformat code, or touch lines outside the feedback scope
- Do NOT refactor adjacent code, even if it looks related
- If the reviewer's comment is ambiguous, mark it Needs decision and continue with unrelated unambiguous comments
- Do NOT create any git commits during the fix cycle. Commits are handled only in the publish step, and only with explicit user approval when needed.
7. Verify Fixes
After all fixes are applied, run the project's compile/build, lint, and test commands scoped to only the changed files and their directly related tests. If no code changed, skip verification and proceed to Step 8. Use safe background batches for independent validators instead of running every command sequentially.
When selecting verification commands, reuse <artifact-dir>/verification-<repo-name>.md if an artifact directory is available and the cache is fresh under the cache rules from pfleidi:pr; otherwise discover the smallest relevant lint/test/build commands. Update the cache only when an artifact directory is available.
- Build / compile — run a relevant compile/build command when one is discoverable for the changed production code.
- Lint / static analysis — run the project's documented lint task, scoped to the files that were modified when the task supports scoping. Prefer lint-specific task wrappers such as
make lintormise run lintover invoking linter binaries directly. Do not use aggregatecheck,ci, orverifytasks unless you have confirmed they only run lint/static analysis. If the documented lint task cannot be scoped, run the smallest relevant project lint task. - Tests — run only the test files that cover the modified code (same package, same module, co-located test files). Do NOT run the full test suite.
If no compile/build command or project lint task exists, state that explicitly instead of assuming an unavailable command.
Run formatters, generators, snapshot updates, or other mutating commands alone before validators that depend on their output. Run independent read-only validators concurrently when they do not require the same exclusive service, port, database, fixture directory, or generated output. Keep integration/e2e/service-backed commands separate unless the project documents that they are parallel-safe.
For each background batch, start every command from the same working-tree state, capture stdout/stderr/exit status from the tool, do not edit files while the batch is running, and wait for every command to finish. Run each selected validator directly, for example mise run lint, go test ..., or npm test -- .... Do not wrap validators in sh -c, shell redirection, tee, command separators, or pipelines solely to write logs; that defeats command-prefix approvals and causes extra permission prompts. If an artifact directory is available and file logs can be written after the command completes without rerunning through a shell wrapper, save them under <artifact-dir>/logs-<pr-number>-<timestamp>/; otherwise mark the full-log path as n/a. If files change after a failed batch, none of that batch's successful results count as current verification.
Show verification as a compact table:
| Command | Exit | Relevant output | Full log |
|---|---|---|---|
go test ./pkg/foo -run TestBar -count=1 | 0 | Short success excerpt. | <artifact-dir>/logs-.../go-test-pkg-foo.log or n/a |
For failures or short outputs, show complete output in the relevant-output column or immediately below the table. For long successful outputs, show the relevant excerpt and log path.
If compile/build, lint, or tests fail due to issues introduced by the fixes:
- Read the error output and identify every failure
- Fix all issues — apply the minimal changes needed
- Re-run the failing commands using the same safe batching rules
- Show the complete output again
Cap at 2 fix attempts. If still failing after 2 rounds, present the remaining failures to the user with full output. Do not describe the work as done or commit-ready while compile/build status is failing or unknown for changed production code.
Once verification passes, show a summary: how many comments were addressed, rejected, intentionally left unresolved, or still blocked. If compile/build did not pass for code changes, say the work is not commit-ready before the summary. Do NOT show a diff — the Edit tool already showed each change inline.
Proceed to Step 8 for threads that were addressed or intentionally rejected. Leave Needs decision threads unresolved and do not post replies to them unless the user provided a public rejection rationale. Do not block publishing addressed threads just because unrelated threads still need a decision.
8. Publish PR Updates
After addressed/rejected threads are ready to publish:
-
Check branch state:
-
If there are uncommitted fix changes, STOP and ask the user whether to commit them now or let the user commit manually. Do not push until the fixes are committed. If compile/build did not pass for code changes, say the work is not commit-ready and do not ask to commit until the gap is resolved or the user explicitly takes over. If the user approves committing after verification, stage only files changed for the PR feedback fixes and write the commit message from the actual diff using the subject-plus-context style from
AGENTS.md. -
Push the committed changes for the current branch:
If the branch has no upstream and the push fails for that reason, use:
Never force-push.
-
Map each addressed finding to the commit or commits that contain its fix. Use the recorded starting commit, changed-file tracking, ledger, and
git log/git showto identify the relevant short SHA(s). If one commit fixes multiple comments, reference the same commit in each review-thread reply. -
Build and show a direct review-thread reply plan before calling the API:
Thread Top-level comment ID Status Reply body Resolve PRRT_...123456789Addressed Addressed in abc1234 by adding the nil check before dereferencing.Yes PRRT_...n/aNeeds decision n/aNo Proceed without asking when every actionable reply body is either addressed or a user-approved rejection. Needs decision rows with
Reply body = n/aandResolve = Nodo not block publishing addressed threads only if they already passed the decision gate and were explicitly deferred by the user. Stop before API calls if a rejection lacks a public rationale, if a top-level comment ID is missing for a thread that should receive a reply, or if any reply body is uncertain. -
Post one direct review-thread reply for each addressed or user-approved rejected thread:
-
For addressed comments, state what changed and the commit SHA(s), e.g.
Addressed in abc1234 by adding the nil check before dereferencing. -
For rejected comments, state why it was not changed, e.g.
Not changed: this path already returns before allocation failure can be observed. -
Keep replies concise but specific. Do not over-explain or repeat the diff.
-
Do not reply to comments the user explicitly chose to leave unresolved unless needed to publish a user-approved rejection rationale.
-
Use the REST review-comment reply endpoint:
This endpoint posts an immediate reply to the existing review thread. Do not use
gh pr review, GraphQLaddPullRequestReviewThreadReply,gh pr comment, or the regular issue comment endpoint as a fallback. -
-
Resolve each addressed or user-approved rejected thread after its direct review-thread reply succeeds:
Do not resolve a thread that did not receive a successful direct reply, is still ambiguous, or still needs user approval.
-
Present a final summary table:
Thread Reviewer Location Status Commented Resolved PRRT_...reviewerfile.go:42Addressed Yes Yes PRRT_...reviewerfile.go:99Needs decision No No Include push result, branch, ledger path, rejected comments and rationale, and any comments intentionally left unresolved or not resolvable via API.
9. Post-Publish
- If the user asks for adjustments, apply them
- If the user asks to revert specific fixes, revert ONLY those changes
- Do NOT commit or push additional changes unless explicitly asked
| Thread | Reviewer | Location | Status | Commented | Resolved |
|---|---|---|---|---|---|
PRRT_...i5o | cursor (Bugbot) | tmux.go:87-97 | Rejected — duplicate of the Copilot thread with your approved rationale | Yes | Yes |
No code changes, nothing to commit or push (branch already at a3db5ced4e on origin). Ledger: n/a.
All review threads on #1599 are now resolved. Still pending: the droid-only E2E run (28556048126) — watcher is active, I'll report when it completes.
The branch's trail (Trail #725, "fix(e2e): droid prompt submission and claude-code subagent flakes") has no findings at all — 0 open, 0 resolved, 0 dismissed, 0 stale, even with all filters widened (--status any --freshness any --include-dismissed).
So nothing from agent reviewers has been filed as trail findings for this branch — the only review feedback so far came through GitHub PR threads (Copilot + Cursor Bugbot), and those are all addressed and resolved. Still pending: the droid-only E2E run 28556048126; the watcher will report when it finishes.
Everything is green. Final state of PR #1599:
| Verification | Result |
|---|---|
| Droid-only E2E (run 28556048126, with Worker-runtime waits) | ✅ success — factory hook tests fixed |
| Full-matrix E2E (run 28554213547) | claude-code + Windows subagent tests ✅ (droid failure since fixed) |
| PR checks (lint, test-core, canary, integration a–c, licenses, binary-size) | ✅ all pass |
| Review threads (Copilot ×3, Bugbot ×1) | All replied + resolved |
| Trail findings | None |
That closes out every failure from the original triage: the swallowed-Enter harness bug, the claude-code background-subagent misses (Linux + Windows), and the follow-on Worker-runtime timeout. The known deferred item remains the CLI's background-subagent tracking gap (SubagentStop wiring) — worth filing as an issue if you want it on the books.
The PR is still a draft — want me to mark it ready for review?
Base directory for this skill: /Users/pfleidi/.agents/skills/pfleidi/skills/pr-feedback
PR Feedback
1. Identify the PR
If the user provided a PR number or URL, use that. Otherwise, detect from the current branch:
If no PR exists for the current branch, stop and tell the user.
2. Fetch Review Comments
Fetch PR metadata first so comment decisions have intent context:
Also collect branch commits and changed files for context:
Show the PR context as a table before classifying comments:
| Context | Source | Value |
|---|---|---|
| PR | title/body | One-line PR intent |
| Branch | commits | One-line commit summary |
| Changed surface | diff file list | Main packages/files touched |
| Base/head | PR metadata | base <- head |
Fetch unresolved review threads with GraphQL as the primary source of truth. Group work by thread, not by individual REST comment:
Filter to unresolved threads only. If there are no unresolved threads, report that to the user and stop — there is nothing to fix.
If GraphQL pagination indicates more review threads or thread comments are available, paginate before classifying. Do not classify a partial thread set as complete.
Use REST pull-review comments only as a fallback when GraphQL data is incomplete or a thread cannot be mapped to a review comment ID:
When REST fallback is used, deduplicate by GraphQL thread ID first, then by file/line/body/author. Do not present or fix the same review request twice.
3. Parse, Classify, and Group
Use permission-friendly reads while investigating comments. Avoid shell pipelines, command separators, subshells, and output filters for read-only source inspection because they create extra permission prompts and can block background work. Do not run commands like git show HEAD:path | sed -n '10,40p'. Use workspace file range reads, rg with path limits, path-scoped diffs, or one standalone git show <rev>:<path> only when the output is acceptably small.
For each comment, extract:
- Author — who left it
- Author type — bot, automated reviewer, human reviewer, or maintainer
- File and line — where it points
- Body — the actual feedback (verbatim, not paraphrased)
- Thread context — any replies in the same thread (to understand if it was already discussed or resolved conversationally)
- Thread ID and top-level comment ID — the GraphQL review thread ID and the original top-level review comment
databaseIdneeded to reply and resolve. Replies to replies are not supported; if only a reply ID is available, fetch the full thread and use the first/top-level review comment ID.
Group each unresolved review thread into a single finding. If multiple comments in one thread refine or supersede each other, use the latest unresolved reviewer request as the finding and retain the earlier messages as context.
Classify each finding source:
- Bot — GitHub bot, CI system, or linter/static-analysis account such as
github-actions[bot]orcodecov[bot] - Automated reviewer — review-assistant accounts that produce natural-language suggestions, such as Copilot or CodeRabbit
- Human reviewer — non-bot reviewer
- Maintainer — repository owner/member/maintainer when that can be inferred from GitHub metadata
4. Present Findings
Present two separate sections:
Human Comments
Table ordered by:
- Bugs / correctness issues — reviewer identified broken logic or missing error handling
- Design / architecture feedback — structural changes, API shape, naming of public interfaces
- Style / nits — formatting, naming of local variables, minor readability
Use this table format:
| # | Priority | Location | Reviewer | Request | Key quote | Autofix |
|---|---|---|---|---|---|---|
| 1 | Bug | file.go:42 | reviewer | One-line summary of what the reviewer is asking for. | Short verbatim excerpt. | Eligible, or Needs decision with the exact decision needed. |
For automated reviewers, use the same table and set Reviewer to the tool account, with Priority based on the substance of the request.
Bot Comments (batched)
Table continuing the numbering from above, grouped by tool/bot:
| # | Bot | Location | Required fix | Autofix |
|---|---|---|---|---|
| 8 | linter-name | file.go:42 | One-line summary of the required fix. | Eligible, or Needs decision with the exact decision needed. |
Keep table cells short and scannable. Use the smallest useful verbatim quote, not the full comment body. Escape | characters inside code or text so the table remains valid Markdown.
End with a summary: total human comments, total bot comments, overall assessment of effort.
Do not stop for mode selection. After the decision gate below, proceed by default with bot comments and human comments marked Autofix eligible. Mark a human comment Autofix eligible only when the requested change is source-backed, high confidence, minimal, unambiguous, does not require a product/design decision, does not add a dependency, does not change a shared/public interface, and has a clear verification path.
Leave all other human comments unresolved as Needs decision, with the exact decision needed. Do not reject a reviewer comment by default; rejection requires a user-provided public rationale.
Decision Gate Before Fixes
Before applying any fixes, handle every Needs decision finding first. Do not let bot comments or easy autofixes push these questions to the end.
-
Present a short "Decision needed first" table:
# Location Reviewer Decision needed Why it blocks 3 file.go:42reviewerChoose whether the API should return nilor an empty slice.Either answer changes caller behavior. -
Try to answer each decision from source, PR context, existing project patterns, and the full review thread before asking the user.
-
If the answer is source-backed, low risk, and has one clear implementation, reclassify the finding as Autofix eligible and record the reasoning.
-
If the correct answer is "do not change this", record it as a proposed rejection, but do not publish the rejection without a user-provided public rationale.
-
If any finding still needs a product/design call, shared/public interface decision, dependency choice, or other user judgment, STOP before bot or autofix work. Ask for all remaining decisions in one concise list.
-
Continue to Step 5 only after every decision is either answered, reclassified, proposed for rejection with a user-provided rationale, or explicitly deferred by the user. Deferred Needs decision findings remain unresolved and must be listed again in the final summary.
Before applying any fixes, record the starting commit:
Choose an artifact directory using the AGENTS.md temporary artifact rule with agent name pfleidi-pr-feedback:
- Use
./tmp/pfleidi-pr-feedback/only when./tmp/already exists and is already ignored. - If no project-local artifact directory is available, do not create file artifacts by default; keep ledger/log/cache information in the response and mark file paths
n/a. Ask before using/tmp/pfleidi-pr-feedback/or modifying ignore files.
When an artifact directory is available, create a temporary thread ledger at <artifact-dir>/pr-feedback-<pr-number>.md. If no artifact directory is available, keep the same ledger fields in the final summary table instead. Update the ledger after each thread with:
- Thread ID, source category, reviewer, location, and status.
- Files touched.
- What changed and why.
- Related tests or verification commands.
- Planned review-thread reply body, if any.
- Resolve decision: yes/no and why.
5. Fix Bot Comments (batched)
After the decision gate, fix all bot comments first — these are mechanical and clearing them reduces noise before the human-comment phase.
- For each bot finding:
- Read the relevant code
- Implement the fix — ONLY the changes needed for that single finding
- Track the files changed for this finding so the review-thread reply can identify the commit that contains the fix
- If a fix is ambiguous or would conflict with a human-comment fix already applied, mark it Needs decision and continue
- After all bot fixes are applied, present a summary table. Do NOT show a diff — the Edit tool already showed each change inline.
| # | Finding | File | Bot | Status |
|---|---|---|---|---|
| 8 | Description | path:line | linter-name | Fixed |
| 9 | Description | path:line | linter-name | Fixed |
| 11 | Description | path:line | linter-name | Skipped — conflicts with #3 |
- Proceed directly to Step 6.
6. Fix Human Comments (batched)
After bot fixes, work through Autofix eligible human comments in report order:
- State which finding you are addressing (number and one-line description)
- Read the relevant code and the full comment thread to understand intent
- Re-check eligibility before editing; if the fix is no longer clearly eligible, mark it Needs decision and continue
- Implement the fix — ONLY the changes needed for that single finding
- Track the files changed for this finding so the review-thread reply can identify the commit that contains the fix
- If a comment needs a product/design decision, shared/public interface change, dependency, broad refactor, or has multiple reasonable fixes, mark it Needs decision and continue
- If the user rejects the comment instead of fixing it, record the specific rationale to use in the review-thread reply
Scope Rules
- Make the MINIMAL change that addresses the reviewer's feedback
- Keep the diff limited to files and lines directly required by the feedback
- First decide whether the feedback points to a local or systemic issue. Fix at the narrowest correct level; do not add a local workaround that hides a shared/root-cause bug.
- If the feedback requires a behavior-changing code fix, add or update the directly related test in the same fix. Prefer TDD, but complete the focused red-to-green cycle before stopping: write/update the failing test, confirm it fails, implement the fix, confirm the focused test passes. Do not stop after only adding the failing test unless the user explicitly asks.
- Do NOT rename variables, reformat code, or touch lines outside the feedback scope
- Do NOT refactor adjacent code, even if it looks related
- If the reviewer's comment is ambiguous, mark it Needs decision and continue with unrelated unambiguous comments
- Do NOT create any git commits during the fix cycle. Commits are handled only in the publish step, and only with explicit user approval when needed.
7. Verify Fixes
After all fixes are applied, run the project's compile/build, lint, and test commands scoped to only the changed files and their directly related tests. If no code changed, skip verification and proceed to Step 8. Use safe background batches for independent validators instead of running every command sequentially.
When selecting verification commands, reuse <artifact-dir>/verification-<repo-name>.md if an artifact directory is available and the cache is fresh under the cache rules from pfleidi:pr; otherwise discover the smallest relevant lint/test/build commands. Update the cache only when an artifact directory is available.
- Build / compile — run a relevant compile/build command when one is discoverable for the changed production code.
- Lint / static analysis — run the project's documented lint task, scoped to the files that were modified when the task supports scoping. Prefer lint-specific task wrappers such as
make lintormise run lintover invoking linter binaries directly. Do not use aggregatecheck,ci, orverifytasks unless you have confirmed they only run lint/static analysis. If the documented lint task cannot be scoped, run the smallest relevant project lint task. - Tests — run only the test files that cover the modified code (same package, same module, co-located test files). Do NOT run the full test suite.
If no compile/build command or project lint task exists, state that explicitly instead of assuming an unavailable command.
Run formatters, generators, snapshot updates, or other mutating commands alone before validators that depend on their output. Run independent read-only validators concurrently when they do not require the same exclusive service, port, database, fixture directory, or generated output. Keep integration/e2e/service-backed commands separate unless the project documents that they are parallel-safe.
For each background batch, start every command from the same working-tree state, capture stdout/stderr/exit status from the tool, do not edit files while the batch is running, and wait for every command to finish. Run each selected validator directly, for example mise run lint, go test ..., or npm test -- .... Do not wrap validators in sh -c, shell redirection, tee, command separators, or pipelines solely to write logs; that defeats command-prefix approvals and causes extra permission prompts. If an artifact directory is available and file logs can be written after the command completes without rerunning through a shell wrapper, save them under <artifact-dir>/logs-<pr-number>-<timestamp>/; otherwise mark the full-log path as n/a. If files change after a failed batch, none of that batch's successful results count as current verification.
Show verification as a compact table:
| Command | Exit | Relevant output | Full log |
|---|---|---|---|
go test ./pkg/foo -run TestBar -count=1 | 0 | Short success excerpt. | <artifact-dir>/logs-.../go-test-pkg-foo.log or n/a |
For failures or short outputs, show complete output in the relevant-output column or immediately below the table. For long successful outputs, show the relevant excerpt and log path.
If compile/build, lint, or tests fail due to issues introduced by the fixes:
- Read the error output and identify every failure
- Fix all issues — apply the minimal changes needed
- Re-run the failing commands using the same safe batching rules
- Show the complete output again
Cap at 2 fix attempts. If still failing after 2 rounds, present the remaining failures to the user with full output. Do not describe the work as done or commit-ready while compile/build status is failing or unknown for changed production code.
Once verification passes, show a summary: how many comments were addressed, rejected, intentionally left unresolved, or still blocked. If compile/build did not pass for code changes, say the work is not commit-ready before the summary. Do NOT show a diff — the Edit tool already showed each change inline.
Proceed to Step 8 for threads that were addressed or intentionally rejected. Leave Needs decision threads unresolved and do not post replies to them unless the user provided a public rejection rationale. Do not block publishing addressed threads just because unrelated threads still need a decision.
8. Publish PR Updates
After addressed/rejected threads are ready to publish:
-
Check branch state:
-
If there are uncommitted fix changes, STOP and ask the user whether to commit them now or let the user commit manually. Do not push until the fixes are committed. If compile/build did not pass for code changes, say the work is not commit-ready and do not ask to commit until the gap is resolved or the user explicitly takes over. If the user approves committing after verification, stage only files changed for the PR feedback fixes and write the commit message from the actual diff using the subject-plus-context style from
AGENTS.md. -
Push the committed changes for the current branch:
If the branch has no upstream and the push fails for that reason, use:
Never force-push.
-
Map each addressed finding to the commit or commits that contain its fix. Use the recorded starting commit, changed-file tracking, ledger, and
git log/git showto identify the relevant short SHA(s). If one commit fixes multiple comments, reference the same commit in each review-thread reply. -
Build and show a direct review-thread reply plan before calling the API:
Thread Top-level comment ID Status Reply body Resolve PRRT_...123456789Addressed Addressed in abc1234 by adding the nil check before dereferencing.Yes PRRT_...n/aNeeds decision n/aNo Proceed without asking when every actionable reply body is either addressed or a user-approved rejection. Needs decision rows with
Reply body = n/aandResolve = Nodo not block publishing addressed threads only if they already passed the decision gate and were explicitly deferred by the user. Stop before API calls if a rejection lacks a public rationale, if a top-level comment ID is missing for a thread that should receive a reply, or if any reply body is uncertain. -
Post one direct review-thread reply for each addressed or user-approved rejected thread:
-
For addressed comments, state what changed and the commit SHA(s), e.g.
Addressed in abc1234 by adding the nil check before dereferencing. -
For rejected comments, state why it was not changed, e.g.
Not changed: this path already returns before allocation failure can be observed. -
Keep replies concise but specific. Do not over-explain or repeat the diff.
-
Do not reply to comments the user explicitly chose to leave unresolved unless needed to publish a user-approved rejection rationale.
-
Use the REST review-comment reply endpoint:
This endpoint posts an immediate reply to the existing review thread. Do not use
gh pr review, GraphQLaddPullRequestReviewThreadReply,gh pr comment, or the regular issue comment endpoint as a fallback. -
-
Resolve each addressed or user-approved rejected thread after its direct review-thread reply succeeds:
Do not resolve a thread that did not receive a successful direct reply, is still ambiguous, or still needs user approval.
-
Present a final summary table:
Thread Reviewer Location Status Commented Resolved PRRT_...reviewerfile.go:42Addressed Yes Yes PRRT_...reviewerfile.go:99Needs decision No No Include push result, branch, ledger path, rejected comments and rationale, and any comments intentionally left unresolved or not resolvable via API.
9. Post-Publish
- If the user asks for adjustments, apply them
- If the user asks to revert specific fixes, revert ONLY those changes
- Do NOT commit or push additional changes unless explicitly asked
Verification:
| Command | Exit | Relevant output | Full log |
|---|---|---|---|
mise run fmt && mise run lint | 0 | 0 issues. | n/a |
go vet -tags e2e ./e2e/tests/ | 0 | clean compile of the e2e test package (canary skips factory tests for vogon, so vet is the focused check) | n/a |
1 comment addressed, nothing rejected or blocked. The fix is uncommitted — proposed commit:
fix(e2e): align factory file wait with documented Worker boundThe adjacent comment documents Worker turns of 60-120s on CI but the file wait was capped at 90s, so a slow Worker could outlive the wait even when it succeeds. Droid's 2x timeout multiplier gives these tests a 6-minute budget, so the wider wait fits comfortably.
Commit and I'll push, reply to the Bugbot thread with the SHA, and resolve it?
All threads on #1599 are resolved again and the check watcher is armed — I'll report when the new round settles (including any fresh Bugbot pass on 515ced2f3d).
The branch is in its final shape: 7 commits covering the Send hardening, vogon REPL contract, foreground-subagent prompts, and the factory-test waits — all verified across canary, local claude-code E2E, and CI (droid-only run green). Once the checks settle, the only remaining call is flipping the PR from draft to ready for review — say the word and I'll do it.